Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

initial attempt to expose exception list for tracking protection #4441

Closed
wants to merge 1 commit into from

Conversation

Amejia481
Copy link
Contributor

I want to get some feedback around exposing of the runtime.contentBlockingController api . The idea of this API is that consumers can have a way to indicate that they don't want to have tracking protection ON for some specific sites.

This is my attempt to expose this API please if you have any additional recommendations of how to implement it better please let me know!

For this geckoView provides the runtime.contentBlockingController object to interact with an ExceptionList.

The API structure:

        // Allows to provide a previous exception list
        val exceptionList = runtime.contentBlockingController.ExceptionList("Json")
        runtime.contentBlockingController.restoreExceptionList(exceptionList)

        // Adds this session to the list
        runtime.contentBlockingController.addException(geckoSession)


        // Deletes this session from the list
        runtime.contentBlockingController.removeException(geckoSession)
       
        // Remove everything from the list.
        runtime.contentBlockingController.clearExceptionList()

        // Returns an gecko future indicating the actual exceptionList object
        runtime.contentBlockingController.saveExceptionList().accept { exceptionList  ->
            println(exceptionList?.uris) // Array<String>
            println(exceptionList?.toJson()) // JSONObject
        }

        // Returns a gecko future indicating if this session is in the exception list
        runtime.contentBlockingController.checkException(geckoSession).accept { contains ->
            println(contains) // boolean
        }

The API is not ideal, because of the geckoview policy for not maintaining state, as a result, we have to persist all the data on our side and provide it back to the gecko runtime.


Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

@Amejia481 Amejia481 changed the title initial attend to expose exception list for tracking protection initial attempt to expose exception list for tracking protection Sep 17, 2019
Copy link
Contributor

@csadilek csadilek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Amejia481 this makes sense to me and I think it's in the right direction! Basically, if we have to store the exception list we need a concept of a storage for it and concrete implementations used by the engines, which is what you've done. The use cases will help shielding consumers from the complexity so I like that too.

I have one concern about "restoring" the exception list and when that operation has to complete. Does GeckoView guarantee, that as long as we call restore before we load sessions, all sessions will take the exception list into account? Or is possible for sessions to finish loading before the exception list has been restored (which would be a problem)? That latter we'd need to verify with the GV team. If that's not handled internally, I don't see how we could do that in A-C right now.

@Amejia481
Copy link
Contributor Author

@csadilek thanks for the feedback!

I have one concern about "restoring" the exception list and when that operation has to complete. Does GeckoView guarantee, that as long as we call restore before we load sessions, all sessions will take the exception list into account? Or is possible for sessions to finish loading before the exception list has been restored (which would be a problem)? That latter we'd need to verify with the GV team. If that's not handled internally, I don't see how we could do that in A-C right now.

I was talking with someone from the GV team about it and they say that we shouldn't worry about it, if we have any related issue that let them know!

@csadilek
Copy link
Contributor

I was talking with someone from the GV team about it and they say that we shouldn't worry about it, if we have any related issue that let them know!

@Amejia481 I think we need to address that, not necessarily right away, but I would file an issue. There's a race condition here between loading the exception list on startup (e.g. on some IO thread) and loading the first session. At least in theory it's possible for the sessions to be loaded before the exception lists are available. We've had similar bugs in Fenix before when restoring sessions from disk.

@Amejia481
Copy link
Contributor Author

Amejia481 commented Sep 26, 2019

Closed in favor of #4554

@Amejia481 Amejia481 closed this Sep 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants