When Tracking Protection is disabled, we could still use the Tracking Protection list to lower the priority of those HTTP requests to nsISupportsPriority::PRIORITY_LOWEST. Patrick says: to the extent that TP resources are separate origins than other resources it wouldn't actually turn into much of a practical difference. Different origins are basically run in parallel right now and the prioritizations apply within the origin. There are some exceptions to this when different origins are carried on the same connection - and we will see more of that in the H2/CDN world. And generally doing more with priority information is an evolving area of interest, so marking TP channels as low priority at least creates the meta information to do the right thing when the rest of the stack has more creative things to do.

Clearing the dependency on bug 1029886, because this has no effect on tracking.

(In reply to Chris Peterson [:cpeterson] from comment #0) > When Tracking Protection is disabled, we could still use the Tracking > Protection list This requires that the TP list be updated for every user regardless whether TP is enforced. Either one of bug 1134284 or bug 1138979 would cause this as a side-effect.

the change here is pretty much in the TP code (setpriority() where we currently cancel cancel()), so I'll change the component to reflect that

Chris, why exactly is bug 1309653 blocking this one?

(In reply to Honza Bambas (:mayhemer) from comment #4) > Chris, why exactly is bug 1309653 blocking this one? It doesn't need to block. Bug 1309653 used to be called "Deprioritize network loads in background pages", but it looks like the scope of that bug changed.

Kershaw, is this something you could work on?

You will also need to figure out how much this is blocked by bug 1170190.

(In reply to Honza Bambas (:mayhemer) from comment #6) > Kershaw, is this something you could work on? Yes, I think I can do this.

I think this bug could need two steps to complete: 1. Figure out how to classify URLs when TP is disabled. Modify nsChannelClassifier and nsUrlClassifierDBService to make them work without TP being enabled. This might be the most difficult one, since I am not sure how much complicated it is. 2. Based on the classified result to adjust the channel's priority and mark LOAD_BACKGROUND. This seems easier compared to step 1. Maybe just call |SetPriority| and directly change |mLoadFlags| is enough? Honza, could you have a look at my rough plan and point out if I overlook something? Thanks.

(In reply to Honza Bambas (:mayhemer) from comment #7) > You will also need to figure out how much this is blocked by bug 1170190. It seems that those discussions in bug 1170190 are quite high level and the scope is not clear to me. I think we can just start with this bug.

(In reply to Kershaw Chang [:kershaw] from comment #9) > I think this bug could need two steps to complete: > > 1. Figure out how to classify URLs when TP is disabled. > Modify nsChannelClassifier and nsUrlClassifierDBService to make them work > without TP being enabled. > This might be the most difficult one, since I am not sure how much > complicated it is. Hmm.. could you just directly use the same code as we use here? https://dxr.mozilla.org/mozilla-central/rev/3e73fd638e687a4d7f46613586e5156b8e2af846/netwerk/protocol/http/nsHttpChannel.cpp#5891 I mean, independently on whether classification is enforced or allowed for the load. If so, I think this makes few things easier. If not, I'll start looking and asking. > > 2. Based on the classified result to adjust the channel's priority and mark > LOAD_BACKGROUND. > This seems easier compared to step 1. Maybe just call |SetPriority| and > directly change |mLoadFlags| is enough? Yes, it's should be exactly that easy, nothing more needed. Just do it before you go out to network. > > Honza, could you have a look at my rough plan and point out if I overlook > something? Thanks. If you find it easy to find out if the URL is on the tracking list, I think this can be done in a single bug and patch.

First of all, if you need help with this, I'm happy to assist and you can also chat with Dimi, Henry and Thomas in Taipei who are all working on the URL Classifier at the moment. (In reply to Kershaw Chang [:kershaw] from comment #9) > 1. Figure out how to classify URLs when TP is disabled. > Modify nsChannelClassifier and nsUrlClassifierDBService to make them work > without TP being enabled. Given that bug 1312515 (or bug 1312514) and bug 1170190 will all need this, we could split this up in a separate dependent bug. Up to you. We're going to need two things: (a) Enable list updates when TP is disabled. (b) Annotate channels used to load a tracking resource. Currently we look at the two TP prefs: https://dxr.mozilla.org/mozilla-central/rev/3e73fd638e687a4d7f46613586e5156b8e2af846/toolkit/components/url-classifier/SafeBrowsing.jsm#182 to determine whether or not to update the list: https://dxr.mozilla.org/mozilla-central/rev/3e73fd638e687a4d7f46613586e5156b8e2af846/toolkit/components/url-classifier/SafeBrowsing.jsm#302-315 We could add a new pref here like "privacy.trackingprotection.annotate_channels" or something like that. If it's off, then we won't update the list or annotate the channels and so none of the perf features will do anything. I'm not exactly sure at which point we'll want to annotate the channels, but I suspect we could refactor the existing tracking protection code to flag the channel as a tracking resource. Then any other piece of code could look at that flag and decide to deprioritize, suspend or cancel the channel as appropriate. (In reply to Kershaw Chang [:kershaw] from comment #10) > (In reply to Honza Bambas (:mayhemer) from comment #7) > > You will also need to figure out how much this is blocked by bug 1170190. > > It seems that those discussions in bug 1170190 are quite high level and the > scope is not clear to me. > I think we can just start with this bug. Had bug 1170190 been resolved first, you would have had most of #1 already done. If you do that work however, that bug is no longer blocking you.

Summary: - As described in comment #12, this patch adds a new pref to control TP list updates when the TP is disabled. - Remove |mCheckTracking| in nsUrlClassifierDBService, since it's not used. Hi Francois, Could you have a look at this patch? It seems that nsIURIClassifier::classify already depends on the parameter aTrackingProtectionEnabled [1] to decide whether to classify the URI against the TP list. So, I think we can just use nsIURIClassifier::classify to annotate channels if TP is disabled. [1] http://searchfox.org/mozilla-central/rev/846adaea6ccd1428780ed47d0d94da18478acdbb/netwerk/base/nsIURIClassifier.idl#56

Summary: - Save the result of |ShouldEnableTrackingProtection|. Since this function is called in twice in [1] and [2], which are in the same call path. - Lower the priority of the channel and set new load flags when TP is disabled. Honza, Could you please have a look and provide some feedback? Thanks. [1] http://searchfox.org/mozilla-central/rev/846adaea6ccd1428780ed47d0d94da18478acdbb/netwerk/protocol/http/nsHttpChannel.cpp#5883 [2] http://searchfox.org/mozilla-central/rev/846adaea6ccd1428780ed47d0d94da18478acdbb/netwerk/base/nsChannelClassifier.cpp#344 (via |nsChannelClassifier::Start|)

Comment on attachment 8809423 [details] [diff] [review] Part2: Lower the priority of the channel loading tracking resource Review of attachment 8809423 [details] [diff] [review]: ----------------------------------------------------------------- looks good ! ::: netwerk/base/nsChannelClassifier.cpp @@ +680,5 @@ > this, errorName.get())); > } > MarkEntryClassified(aErrorCode); > > + MOZ_ASSERT(mTrackingProtectionEnabled); I only don't follow this assertion

Comment on attachment 8809416 [details] [diff] [review] Part1: Enable TP list updates Review of attachment 8809416 [details] [diff] [review]: ----------------------------------------------------------------- I suggested different names inline, but this is the right approach. ::: modules/libpref/init/all.js @@ +1221,5 @@ > pref("privacy.trackingprotection.enabled", false); > // Enforce tracking protection in Private Browsing mode > pref("privacy.trackingprotection.pbmode.enabled", true); > +// Enforce annotating channels in all modes > +pref("privacy.trackingprotection.annotate_channels.enabled", false); Now that I think of it, the `enabled` part is a little redundant. We could just use `privacy.trackingprotection.annotate_channels`. ::: toolkit/components/url-classifier/SafeBrowsing.jsm @@ +119,5 @@ > + phishingEnabled: false, > + malwareEnabled: false, > + trackingEnabled: false, > + blockedEnabled: false, > + annotatingEnabled: false, Maybe `trackingAnnotations` instead of `annotatingEnabled` so that it's clear that we annotating based on the TP list? ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp @@ -98,5 @@ > > #define CHECK_PHISHING_PREF "browser.safebrowsing.phishing.enabled" > #define CHECK_PHISHING_DEFAULT false > > -#define CHECK_TRACKING_PREF "privacy.trackingprotection.enabled" I'm surprised we're not using these, but if it compiles and the tests pass, I guess it's true.

(In reply to Honza Bambas (:mayhemer) from comment #15) > Comment on attachment 8809423 [details] [diff] [review] > Part2: Lower the priority of the channel loading tracking resource > > Review of attachment 8809423 [details] [diff] [review]: > ----------------------------------------------------------------- > > looks good ! > > ::: netwerk/base/nsChannelClassifier.cpp > @@ +680,5 @@ > > this, errorName.get())); > > } > > MarkEntryClassified(aErrorCode); > > > > + MOZ_ASSERT(mTrackingProtectionEnabled); > > I only don't follow this assertion At this point that |mTrackingProtectionEnabled| should contain a value, so add an assertion here to make sure that |ShouldEnableTrackingProtection| has been called before.

Comment on attachment 8809423 [details] [diff] [review] Part2: Lower the priority of the channel loading tracking resource Review of attachment 8809423 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/nsChannelClassifier.cpp @@ +697,5 @@ > + } > + nsLoadFlags newLoadFlags; > + mChannel->GetLoadFlags(&newLoadFlags); > + newLoadFlags |= nsIChannel::LOAD_BACKGROUND; > + mChannel->SetLoadFlags(newLoadFlags); It seems that only marking the load flag with LOAD_BACKGROUND is not enough, we should also update |mForegroundCount| in this channel's loadgroup.

(In reply to Kershaw Chang [:kershaw] from comment #18) > Comment on attachment 8809423 [details] [diff] [review] > Part2: Lower the priority of the channel loading tracking resource > > Review of attachment 8809423 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: netwerk/base/nsChannelClassifier.cpp > @@ +697,5 @@ > > + } > > + nsLoadFlags newLoadFlags; > > + mChannel->GetLoadFlags(&newLoadFlags); > > + newLoadFlags |= nsIChannel::LOAD_BACKGROUND; > > + mChannel->SetLoadFlags(newLoadFlags); > > It seems that only marking the load flag with LOAD_BACKGROUND is not enough, > we should also update |mForegroundCount| in this channel's loadgroup. Honza, I was wondering is it really worth to mark the channel as LOAD_BACKGROUND? If yes, it means that we might need to add new function in nsILoadGroup to update |mForegroundCount|. Moreover, it could cause that |onStopRequest| is not always called after |onStartRequest|, since it is not called when the load flags has LOAD_BACKGROUND [1]. [1] http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/netwerk/base/nsLoadGroup.cpp#623

(In reply to Kershaw Chang [:kershaw] from comment #19) > (In reply to Kershaw Chang [:kershaw] from comment #18) > > Comment on attachment 8809423 [details] [diff] [review] > > Part2: Lower the priority of the channel loading tracking resource > > > > Review of attachment 8809423 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: netwerk/base/nsChannelClassifier.cpp > > @@ +697,5 @@ > > > + } > > > + nsLoadFlags newLoadFlags; > > > + mChannel->GetLoadFlags(&newLoadFlags); > > > + newLoadFlags |= nsIChannel::LOAD_BACKGROUND; > > > + mChannel->SetLoadFlags(newLoadFlags); > > > > It seems that only marking the load flag with LOAD_BACKGROUND is not enough, > > we should also update |mForegroundCount| in this channel's loadgroup. > > Honza, > > I was wondering is it really worth to mark the channel as LOAD_BACKGROUND? > If yes, it means that we might need to add new function in nsILoadGroup to > update |mForegroundCount|. Moreover, it could cause that |onStopRequest| is > not always called after |onStartRequest|, since it is not called when the > load flags has LOAD_BACKGROUND [1]. > > > [1] > http://searchfox.org/mozilla-central/rev/ > 4b6cab91f93c73ae591dafaea40fd5704b41810e/netwerk/base/nsLoadGroup.cpp#623 And what about: - remove the channel from it's load group (if there is one) - change the flag - add the channel again to the same load group Assert is all happens on the main thread! Load groups are not thread safe.

Hmm... but what about the child process... right... Maybe file a new bug and let's solve the background flag there. If we only lower the priority (in a patch for this bug), I think we have 80% of the requirement fulfilled here. Makes sense?

(In reply to Honza Bambas (:mayhemer) from comment #21) > Hmm... but what about the child process... right... > > Maybe file a new bug and let's solve the background flag there. If we only > lower the priority (in a patch for this bug), I think we have 80% of the > requirement fulfilled here. > > Makes sense? Sure. I'll file a new bug once this bug has fixed.

Summary: Change the names based on the previous feedback.

Summary: Remove the setting the load flags part. Only set the priority. In addition, I found it's difficult for me to create a test case, since it's not easy to get a request's priority in js code. I've tried to use nsIWebProgressListener to monitor priority changes, but I can't get the latest value after setting to PRIORITY_LOWEST. Maybe we have to notify nsIWebProgressListener of the priority changes like what we did in [1]. Or do you think we can land this patch without having a test case? [1] http://searchfox.org/mozilla-central/rev/62db1c9021cfbde9fa5e6e9601de16c21f4c7ce4/netwerk/base/nsChannelClassifier.cpp#554-567

Comment on attachment 8811698 [details] [diff] [review] Part2: Lower the priority of the channel loading tracking resource - v2 Review of attachment 8811698 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! Looks good to me. r=honzab ::: netwerk/base/nsChannelClassifier.cpp @@ +357,5 @@ > + trackingProtectionEnabled = mTrackingProtectionEnabled.value(); > + } > + > + bool annotateChannelEnabled = > + Preferences::GetBool("privacy.trackingprotection.annotate_channels"); nit: since this is on a critical path, having a cache for the preference seems wiser. (Preferences::AddBoolVarCache) @@ +680,5 @@ > this, errorName.get())); > } > MarkEntryClassified(aErrorCode); > > + MOZ_ASSERT(mTrackingProtectionEnabled); add a comment to this assertion why it's here and what exactly it asserts @@ +682,5 @@ > MarkEntryClassified(aErrorCode); > > + MOZ_ASSERT(mTrackingProtectionEnabled); > + if (aErrorCode == NS_ERROR_TRACKING_URI && > + !mTrackingProtectionEnabled.valueOr(false)) { this is probably why I didn't (and still don't) understand the assertion. Just before this condition you assert mTrackingProtectionEnabled is set a value. But here you branch when it's set false or not keeping a value. There is probably some mistake here, either in the assertion or in the condition (should only be && !mTrackingProtectionEnabled.value()?) Or you want a fallback path for release build when this would break? Or I need more coffee today? Not sure :)

(In reply to Honza Bambas (:mayhemer) from comment #25) > Comment on attachment 8811698 [details] [diff] [review] > Part2: Lower the priority of the channel loading tracking resource - v2 > > Review of attachment 8811698 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks! Looks good to me. > > r=honzab > > ::: netwerk/base/nsChannelClassifier.cpp > @@ +357,5 @@ > > + trackingProtectionEnabled = mTrackingProtectionEnabled.value(); > > + } > > + > > + bool annotateChannelEnabled = > > + Preferences::GetBool("privacy.trackingprotection.annotate_channels"); > > nit: since this is on a critical path, having a cache for the preference > seems wiser. (Preferences::AddBoolVarCache) > > @@ +680,5 @@ > > this, errorName.get())); > > } > > MarkEntryClassified(aErrorCode); > > > > + MOZ_ASSERT(mTrackingProtectionEnabled); > > add a comment to this assertion why it's here and what exactly it asserts > > @@ +682,5 @@ > > MarkEntryClassified(aErrorCode); > > > > + MOZ_ASSERT(mTrackingProtectionEnabled); > > + if (aErrorCode == NS_ERROR_TRACKING_URI && > > + !mTrackingProtectionEnabled.valueOr(false)) { > > this is probably why I didn't (and still don't) understand the assertion. > Just before this condition you assert mTrackingProtectionEnabled is set a > value. But here you branch when it's set false or not keeping a value. > > There is probably some mistake here, either in the assertion or in the > condition (should only be && !mTrackingProtectionEnabled.value()?) Or you > want a fallback path for release build when this would break? Or I need > more coffee today? Not sure :) Yes, I want a fallback path for release build. Not sure what to do is correct here. Do you think |!mTrackingProtectionEnabled.value()| is ok and we don't have to worry about release build? BTW, could you please also provide some feedback about test case in comment#24? Thanks.

(In reply to Kershaw Chang [:kershaw] from comment #26) > Yes, I want a fallback path Yes, that's a good rational for me. Just add a comment to the code and you are done. > BTW, could you please also provide some feedback about test case in > comment#24? Thanks. Will do shortly.

(In reply to Kershaw Chang [:kershaw] from comment #24) > In addition, I found it's difficult for me to create a test case, since it's > not easy to get a request's priority in js code. If you just want to check the priority have been set to some expected value, you may use either on-modify-request or on-examine-response (the former would be more appropriate but the latter would do too) to inspect the channel directly (GetPriority). > I've tried to use > nsIWebProgressListener to monitor priority changes, but I can't get the > latest value after setting to PRIORITY_LOWEST. Maybe we have to notify > nsIWebProgressListener of the priority changes like what we did in [1]. I would not bother. > > Or do you think we can land this patch without having a test case? If the test would become too complicated, manual testing (in your discretion before landing) for now is probably enough. Prioritization is generally a hard thing to test.

(In reply to Honza Bambas (:mayhemer) from comment #28) > (In reply to Kershaw Chang [:kershaw] from comment #24) > > In addition, I found it's difficult for me to create a test case, since it's > > not easy to get a request's priority in js code. > > If you just want to check the priority have been set to some expected value, > you may use either on-modify-request or on-examine-response (the former > would be more appropriate but the latter would do too) to inspect the > channel directly (GetPriority). > Thanks! "on-examine-response" is exactly what I need.

Francois, could you please also take a look at this test case? Thanks.

Comment on attachment 8811695 [details] [diff] [review] Part1: Enable TP list updates - v2 Review of attachment 8811695 [details] [diff] [review]: ----------------------------------------------------------------- ::: modules/libpref/init/all.js @@ +1222,5 @@ > // Enforce tracking protection in all modes > pref("privacy.trackingprotection.enabled", false); > // Enforce tracking protection in Private Browsing mode > pref("privacy.trackingprotection.pbmode.enabled", true); > +// Enforce annotating channels based on tracking protection list in all modes nit: I would suggest just "Annotate channels based on the tracking protection list in all modes"

Comment on attachment 8812092 [details] [diff] [review] Part3: test case Review of attachment 8812092 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine. I've suggested two small tweaks which would expand the scope of the test. Also, if you want, you could test the case where `privacy.trackingprotection.annotate_channels = false` and see that the priority isn't changed there. ::: toolkit/components/url-classifier/tests/mochitest/test_trackingprotection_annotateChannels.html @@ +40,5 @@ > +} > + > +var trackerPriorityMap = { > + "evil.js": Ci.nsISupportsPriority.PRIORITY_LOWEST, > + "evil.css": Ci.nsISupportsPriority.PRIORITY_LOWEST, It would be great if you could also add a filename here that is expected to be the default priority. That way this test case would ensure two things: 1. we make the tracking elements PRIORITY_LOWEST 2. we don't touch the priority of non-tracking elements @@ +77,5 @@ > + > +SpecialPowers.pushPrefEnv( > + {"set" : [["urlclassifier.trackingTable", "test-track-simple"], > + ["privacy.trackingprotection.enabled", false], > + ["privacy.trackingprotection.pbmode.enabled", true], I would suggest setting this one to false so that we also test that the list is available when only the channel annotation is enabled.

Thanks for the previous review. I've modified the patch based on your comment. Please take a look. Thanks.

Update the patch to remove a redundant file. Sorry for the spam.

Comment on attachment 8813059 [details] [diff] [review] Part3: test case - v2 Review of attachment 8813059 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/url-classifier/tests/mochitest/test_trackingprotection_annotateChannels.html @@ +90,5 @@ > +SpecialPowers.pushPrefEnv( > + {"set" : [["urlclassifier.trackingTable", "test-track-simple"], > + ["privacy.trackingprotection.enabled", false], > + ["privacy.trackingprotection.pbmode.enabled", false], > + ["channelclassifier.allowlist_example", true]]}, Maybe we need to set "privacy.trackingprotection.annotate_channels = false" explicitly, in case we default to "true" in the future.

(In reply to François Marier [:francois] from comment #35) > Comment on attachment 8813059 [details] [diff] [review] > Part3: test case - v2 > > Review of attachment 8813059 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: > toolkit/components/url-classifier/tests/mochitest/ > test_trackingprotection_annotateChannels.html > @@ +90,5 @@ > > +SpecialPowers.pushPrefEnv( > > + {"set" : [["urlclassifier.trackingTable", "test-track-simple"], > > + ["privacy.trackingprotection.enabled", false], > > + ["privacy.trackingprotection.pbmode.enabled", false], > > + ["channelclassifier.allowlist_example", true]]}, > > Maybe we need to set "privacy.trackingprotection.annotate_channels = false" > explicitly, in case we default to "true" in the future. Nice catch! Thanks for the review.

Carry reviewer's name.

Carry reviewer's name.

Carry reviewer's name.

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/504e1e32c8b8 Part1: Enable to update TP list if TP is disabled, r=francois https://hg.mozilla.org/integration/mozilla-inbound/rev/76a1099ef0b4 Part2: Lower the priority of channel loading tracking resource, r=honzab https://hg.mozilla.org/integration/mozilla-inbound/rev/6c26d0bd467b Part3: Test case, r=francois

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=39798545&repo=mozilla-inbound

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/43b2fafc68f4 Backed out changeset 6c26d0bd467b https://hg.mozilla.org/integration/mozilla-inbound/rev/91924feee6b0 Backed out changeset 76a1099ef0b4 https://hg.mozilla.org/integration/mozilla-inbound/rev/7e68f80c7826 Backed out changeset 504e1e32c8b8 for test failures in own tests

(In reply to Carsten Book [:Tomcat] from comment #42) > backed out for test failures like > https://treeherder.mozilla.org/logviewer.html#?job_id=39798545&repo=mozilla- > inbound I believe this is due to that the channel's priority could be changed in other places, like [1]. Actually, "on-examine-response" could be notified after [1], so we cannot always get the correct priority at that time. Not to mention that "on-examine-response" is a little late. We should really check the priority before calling |SetupTransaction|. Honza, do you think is it worth to add another observer for the test purpose in this bug? [1] http://searchfox.org/mozilla-central/rev/b4e6fa3527436bdbcd9dd2e1982382fe423431f3/netwerk/base/nsLoadGroup.cpp#617

I think this has been discussed on IRC. The result was (IIRC) to introduce a new notification.

(In reply to Honza Bambas (:mayhemer) from comment #45) > I think this has been discussed on IRC. The result was (IIRC) to introduce > a new notification. After adding a new notification, I am still not able to get the priority value I want. I also found that the priority had been set by NetworkPrioritizer.jsm and there no way to guarantee when the priority would be changed. So, I think the best way to test this is by xpcselltest without the interference from NetworkPrioritizer.

Honza, could you take a look at this patch? Or should I ask Francois to review?

Test result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc3dac3dd5051deaa9a4ff5c4117f572484e3ed6&selectedJob=32060504

(In reply to Kershaw Chang [:kershaw] from comment #46) > (In reply to Honza Bambas (:mayhemer) from comment #45) > > I think this has been discussed on IRC. The result was (IIRC) to introduce > > a new notification. > > After adding a new notification, I am still not able to get the priority > value I want. I also found that the priority had been set by > NetworkPrioritizer.jsm Ups. Then your test did the right job and _actually found a problem!_ How exactly does NetworkPrioritizer.jsm plays with the already set priorities? Does it destroy the priority you are trying to set in the classifier? If so, we probably have to fix NetworkPrioritizer.jsm and not the tests to mask this. > and there no way to guarantee when the priority would > be changed. > So, I think the best way to test this is by xpcselltest without the > interference from NetworkPrioritizer. To make a very basic test yes an xpcshell test is a good thing to have. To test how it behaves when being part of a navigation when all the surrounding mechanisms are involved (like during a normal use of the browser) we need ALSO the previous test (a mochitest). Or am I missing something?

Comment on attachment 8815556 [details] [diff] [review] Part3: Unit test Review of attachment 8815556 [details] [diff] [review]: ----------------------------------------------------------------- roughly looked only, looks good. if works reliably, land. r=honzab ::: netwerk/test/unit/test_trackingProtection_annotateChannels.js @@ +1,1 @@ > + remove this blank like add a comment with steps what this test is performing so one doesn't need to study the code to know

(In reply to Honza Bambas (:mayhemer) from comment #49) > (In reply to Kershaw Chang [:kershaw] from comment #46) > > (In reply to Honza Bambas (:mayhemer) from comment #45) > > > I think this has been discussed on IRC. The result was (IIRC) to introduce > > > a new notification. > > > > After adding a new notification, I am still not able to get the priority > > value I want. I also found that the priority had been set by > > NetworkPrioritizer.jsm > > Ups. Then your test did the right job and _actually found a problem!_ > > How exactly does NetworkPrioritizer.jsm plays with the already set > priorities? Does it destroy the priority you are trying to set in the > classifier? If so, we probably have to fix NetworkPrioritizer.jsm and not > the tests to mask this. > Yes, NetworkPrioritizer could affect the priority which is already set in the classifier. We still have a potential issue here. Let me file another bug to address this.

Rebase.

Rebase

Carry reviewer's name.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/fc772f9c5fb4 Part 1: Enable to update TP list if TP is disabled. r=francois https://hg.mozilla.org/integration/mozilla-inbound/rev/fb18b507a912 Part 2: Lower the priority of channel loading tracking resource. r=honzab https://hg.mozilla.org/integration/mozilla-inbound/rev/33de2e2e2cf3 Part 3: Unit test. r=honzab

https://hg.mozilla.org/mozilla-central/rev/fc772f9c5fb4 https://hg.mozilla.org/mozilla-central/rev/fb18b507a912 https://hg.mozilla.org/mozilla-central/rev/33de2e2e2cf3

Kershaw, is this still dependent on bug 1170190? If not, can you please remove the dep? Thanks.