Closed Bug 1463772 Opened Last year Closed Last year

Move tracking protection blocklist selection to GeckoRuntimeSettings

Categories

(GeckoView :: General, enhancement, P1)

51 Branch
All
Android
enhancement

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: esawin, Assigned: esawin)

References

Details

Attachments

(2 files, 4 obsolete files)

We allow toggling tracking protection in GeckoSessionSettings, however, the blocklist selection is a runtime setting. We should move that setting out to GeckoRuntimeSettings to make this clear.
Assignee: nobody → esawin
Priority: -- → P1
Move blocklist selection to GeckoRuntimeSettings and extend categories to better support tests.
Attachment #8980445 - Flags: review?(snorp)
Attachment #8980445 - Flags: review?(nchen)
Comment on attachment 8980445 [details] [diff] [review]
0001-Bug-1463772-1.0-Move-and-refactor-tracking-protectio.patch

Review of attachment 8980445 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java
@@ +207,5 @@
> +         *                   Use one or more of the
> +         *                   {@link TrackingProtectionDelegate#CATEGORY_AD TrackingProtectionDelegate.CATEGORY_*} flags.
> +         * @return This Builder instance.
> +         **/
> +        public @NonNull Builder trackingProtection(

I think this should be `trackingProtectionCategories`. Otherwise it's hard to tell the difference between this and the same setting in `GeckoSessionSettings`.

@@ +275,1 @@
>          mCookieBehavior, mCookieLifetime, mCookieLifetimeDays

Might as well make this list alphabetical?

@@ +545,5 @@
> +     * @return categories The categories of trackers that are set to be blocked.
> +     *                    Use one or more of the
> +     *                    {@link TrackingProtectionDelegate#CATEGORY_AD TrackingProtectionDelegate.CATEGORY_*} flags.
> +     **/
> +    public @TrackingProtectionDelegate.Category int getTrackingProtection() {

Same thing with naming here.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +2620,5 @@
>       * GeckoSession applications implement this interface to handle tracking
>       * protection events.
>       **/
>      public interface TrackingProtectionDelegate {
>          @IntDef(flag = true,

@Retention(RetentionPolicy.SOURCE)

@@ +2629,5 @@
>          static final int CATEGORY_AD = 1 << 0;
>          static final int CATEGORY_ANALYTIC = 1 << 1;
>          static final int CATEGORY_SOCIAL = 1 << 2;
>          static final int CATEGORY_CONTENT = 1 << 3;
> +        static final int CATEGORY_ANY = (1 << 4) - 1;

I think this should be "CATEGORY_ALL"

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/TrackingProtection.java
@@ +19,2 @@
>          StringBuilder builder = new StringBuilder();
> +        builder.append(TEST);

`new StringBuilder(TEST)`

@@ +51,5 @@
>          }
>          if (list.indexOf(CONTENT) != -1) {
>              category |= TrackingProtectionDelegate.CATEGORY_CONTENT;
>          }
> +        if (list.indexOf(TEST) != -1) {

In `buildPrefValue` we always add `TEST`, so would this always be true here?
Attachment #8980445 - Flags: review?(nchen) → review+
(In reply to Jim Chen [:jchen] [:darchons] from comment #2)
> @@ +2629,5 @@
> >          static final int CATEGORY_AD = 1 << 0;
> >          static final int CATEGORY_ANALYTIC = 1 << 1;
> >          static final int CATEGORY_SOCIAL = 1 << 2;
> >          static final int CATEGORY_CONTENT = 1 << 3;
> > +        static final int CATEGORY_ANY = (1 << 4) - 1;
> 
> I think this should be "CATEGORY_ALL"

I think ANY makes sense because we may get that in the onTrackerBlocked callback meaning it's a tracker that could be associated with any or an unspecified category (e.g., we're using it for tests).

> @@ +51,5 @@
> >          }
> >          if (list.indexOf(CONTENT) != -1) {
> >              category |= TrackingProtectionDelegate.CATEGORY_CONTENT;
> >          }
> > +        if (list.indexOf(TEST) != -1) {
> 
> In `buildPrefValue` we always add `TEST`, so would this always be true here?

No, this is only true when we actually block an entity from the test list.
Attachment #8980445 - Attachment is obsolete: true
Attachment #8980445 - Flags: review?(snorp)
Attachment #8980723 - Flags: review?(snorp)
This is a basic TP test based on the test blocklist. Testing the real blocklists is problematic since they are maintained out of tree in shavar.
Attachment #8980724 - Flags: review?(nchen)
(In reply to Eugen Sawin [:esawin] from comment #3)
> Created attachment 8980723 [details] [diff] [review]
> 0001-Bug-1463772-1.2-Move-and-refactor-tracking-protectio.patch
> 
> > I think this should be "CATEGORY_ALL"
> 
> I think ANY makes sense because we may get that in the onTrackerBlocked
> callback meaning it's a tracker that could be associated with any or an
> unspecified category (e.g., we're using it for tests).

Maybe a "CATEGORY_UNKNOWN" constant with value 0 would make more sense for that case, since an unspecified category is not actually a combination of the other categories? Instead, "CATEGORY_ALL" would be a combination of all known categories.
Comment on attachment 8980724 [details] [diff] [review]
0002-Bug-1463772-2.0-Add-basic-tracking-protection-tests..patch

Review of attachment 8980724 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt
@@ +25,5 @@
>  
> +    @Setting(key = Setting.Key.USE_TRACKING_PROTECTION, value = "true")
> +    @Test fun trackingProtectionBasic() {
> +        val category = TrackingProtectionDelegate.CATEGORY_ANY;
> +        sessionRule.runtime.getSettings().setTrackingProtectionCategories(category)

`sessionRule.runtime.settings.trackingProtectionCategories = category`

@@ +31,5 @@
> +
> +        sessionRule.waitUntilCalled(
> +                object : Callbacks.TrackingProtectionDelegate {
> +            @AssertCalled(count = 1)
> +            override fun onTrackerBlocked(session: GeckoSession, uri: String,

Should assert `uri` value as well.
Attachment #8980724 - Flags: review?(nchen) → review+
Addressed comments.
Attachment #8980724 - Attachment is obsolete: true
Attachment #8981416 - Flags: review+
(In reply to Jim Chen [:jchen] [:darchons] from comment #5)
> Maybe a "CATEGORY_UNKNOWN" constant with value 0 would make more sense for
> that case, since an unspecified category is not actually a combination of
> the other categories? Instead, "CATEGORY_ALL" would be a combination of all
> known categories.

I think it would be odd to allow blocking "unknown" tracker types.
How about just naming the category what it is, CATEGORY_TEST?
Attachment #8980723 - Attachment is obsolete: true
Attachment #8980723 - Flags: review?(snorp)
Attachment #8981418 - Flags: review?(snorp)
Attachment #8981418 - Flags: review?(nchen)
Comment on attachment 8981418 [details] [diff] [review]
0001-Bug-1463772-1.3-Move-and-refactor-tracking-protectio.patch

Review of attachment 8981418 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +2631,5 @@
>          static final int CATEGORY_AD = 1 << 0;
>          static final int CATEGORY_ANALYTIC = 1 << 1;
>          static final int CATEGORY_SOCIAL = 1 << 2;
>          static final int CATEGORY_CONTENT = 1 << 3;
> +        static final int CATEGORY_TEST = 1 << 4;

We should have docs for each of these. It's not clear what TEST does without reading the comments on this bug.
Attachment #8981418 - Flags: review?(snorp) → review+
Comment on attachment 8981418 [details] [diff] [review]
0001-Bug-1463772-1.3-Move-and-refactor-tracking-protectio.patch

Review of attachment 8981418 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/TrackingProtection.java
@@ +25,4 @@
>          if ((categories & TrackingProtectionDelegate.CATEGORY_AD) ==
>              TrackingProtectionDelegate.CATEGORY_AD) {
> +            if (builder.length() > 0) {
> +                builder.append(",");

Use ',' instead of ",". Also, it would be simpler to always append the comma: `builder.append(FOO).append(',')`, and then trim the final ',' before returning.
Attachment #8981418 - Flags: review?(nchen) → review+
Addressed comments.
Attachment #8981418 - Attachment is obsolete: true
Attachment #8981622 - Flags: review+
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/410b2cbefe3a
[1.4] Move and refactor tracking protection blocklist selection. r=snorp,jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/951c7d8ee079
[2.1] Add basic tracking protection tests. r=jchen
https://hg.mozilla.org/mozilla-central/rev/410b2cbefe3a
https://hg.mozilla.org/mozilla-central/rev/951c7d8ee079
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 62 → mozilla62
You need to log in before you can comment on or make changes to this bug.