850210 - Update assorted contentPrefs usage to nsIContentPrefService2

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: Update removeGroupedPrefs usage

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Attachment: Update ForgetAboutSite.jsm

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Attachment: Update DownloadLastDir

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Attachment: Update removeGroupedPrefs usage

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 723976 [details] [diff] [review]
Update DownloadLastDir

DownloadLastDir has other usage that I'll spin off to another bug, but this straightforward change can be kept here.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 723975 [details] [diff] [review]
Update ForgetAboutSite.jsm

Hey Drew, this is the one that looks most tricky in this bug. As far as I can tell, the removeBySubdomain properly replaces the manual queries that were being done above to list all subdomains and remove each one at a time.
But can you take a look and see if that's indeed the case?

Comment 7

[Drew Willcoxon :adw]

Comment on attachment 723975 [details] [diff] [review]
Update ForgetAboutSite.jsm

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

r+, but see below about a necessary change.

(In reply to :Felipe Gomes from comment #6)
> Comment on attachment 723975 [details] [diff] [review]
> Update ForgetAboutSite.jsm
> 
> Hey Drew, this is the one that looks most tricky in this bug. As far as I
> can tell, the removeBySubdomain properly replaces the manual queries that
> were being done above to list all subdomains and remove each one at a time.
> But can you take a look and see if that's indeed the case?

Yes, that's right.

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm
@@ +202,5 @@
> +    let cps2 = Cc["@mozilla.org/content-pref/service;1"].
> +               getService(Ci.nsIContentPrefService2);
> +    cps2.removeBySubdomain(aDomain, null, {
> +      handleCompletion: function() onContentPrefsRemovalFinished(),
> +      handleError: function() onContentPrefsRemovalFinished()

handleCompletion is always called, so you shouldn't do anything on handleError here.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Attachment: Test adjustment for ForgetAboutSite

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Attachment: Update mobile browser.js

Comment 10

[:Felipe Gomes (needinfo for replies!)]

(In reply to :Felipe Gomes from comment #5)
> DownloadLastDir has other usage that I'll spin off to another bug, but this
> straightforward change can be kept here.

Bug 854299

Comment 11

[Drew Willcoxon :adw]

Comment on attachment 728838 [details] [diff] [review]
Test adjustment for ForgetAboutSite

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

::: toolkit/forgetaboutsite/test/unit/test_removeDataFromDomain.js
@@ +568,5 @@
> +    observe: function(aSubject, aTopic, aData)
> +    {
> +      Services.obs.removeObserver(observer, "browser:purge-domain-data");
> +      Services.tm.mainThread.dispatch(function() {
> +        deferred.resolve();

Is it really necessary to wait for another turn of the event loop to resolve the promise?  If so, please add a brief comment explaining why.

Comment 12

[Drew Willcoxon :adw]

Comment on attachment 728841 [details] [diff] [review]
Update mobile browser.js

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

r+ with the following change:

::: mobile/android/chrome/content/browser.js
@@ +6655,5 @@
>        Services.perms.remove(aURI.host, aType);
>        // Clear content prefs set in ContentPermissionPrompt.js
> +      Cc["@mozilla.org/content-pref/service;1"]
> +        .getService(Ci.nsIContentPrefService2)
> +        .removeByDomainAndName(aURI, aType + ".request.remember", aContext);

The first argument to removeByDomainAndName should be a string, so you need aURI.spec.  (That's one of the gotchas with the new API.  The old one accepted nsIURIs, but the new one only takes strings.)

Comment 13

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/acf32763c61d
https://hg.mozilla.org/integration/mozilla-inbound/rev/beb450416330
https://hg.mozilla.org/integration/mozilla-inbound/rev/badf8d382f79
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bccb17667f0

Comment 14

[Ryan VanderMeulen [:RyanVM]]

I backed out Part 3 due to a high number of failures in browser_privatebrowsing_DownloadLastDirWithCPS.js after it landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e7b1dd41868

https://tbpl.mozilla.org/php/getParsedLog.php?id=21280689&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=21280668&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=21278817&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=21281200&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=21282113&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=21284023&tree=Mozilla-Inbound

Comment 15

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #14)
> I backed out Part 3 due to a high number of failures in
> browser_privatebrowsing_DownloadLastDirWithCPS.js after it landed.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/0e7b1dd41868
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=21280689&tree=Mozilla-
> Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=21280668&tree=Mozilla-
> Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=21278817&tree=Mozilla-
> Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=21281200&tree=Mozilla-
> Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=21282113&tree=Mozilla-
> Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=21284023&tree=Mozilla-
> Inbound

No issues with the numerous post-backout retriggers.
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=0e7b1dd41868

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/acf32763c61d
https://hg.mozilla.org/mozilla-central/rev/beb450416330
https://hg.mozilla.org/mozilla-central/rev/4bccb17667f0

Comment 17

[:Felipe Gomes (needinfo for replies!)]

Thanks for only backing out the part related to the test, Ryan. That test had to be rewritten anyway for bug 854299 so I now relanded both changes together. It should be a lot more reliable now with all the changes, but there's still a small chance that the intermittent will come back (/knock on wood).  If it does and it's not too blatant we can disable the test for a bit until I figure out another way to fix it (backing it out now will also require backing out all of bug 854299).

Relanded part 3:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a255a998af6e

And follow-up test rewrite from bug 854299:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e63c771fec44

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Hoping to see a green try run before pushing it again...

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c804845016a
https://hg.mozilla.org/integration/mozilla-inbound/rev/929ea187b775

Comment 19

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/82be854a0ac1

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/82be854a0ac1