Update assorted contentPrefs usage to nsIContentPrefService2

RESOLVED FIXED in mozilla23

Status

()

Toolkit
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Felipe, Assigned: Felipe)

Tracking

unspecified
mozilla23
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Updated

5 years ago
Assignee: nobody → felipc
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
Created attachment 723974 [details] [diff] [review]
Update removeGroupedPrefs usage
(Assignee)

Comment 2

5 years ago
Created attachment 723975 [details] [diff] [review]
Update ForgetAboutSite.jsm
(Assignee)

Comment 3

5 years ago
Created attachment 723976 [details] [diff] [review]
Update DownloadLastDir
(Assignee)

Comment 4

5 years ago
Created attachment 727818 [details] [diff] [review]
Update removeGroupedPrefs usage
Attachment #727818 - Flags: review?(adw)
(Assignee)

Updated

5 years ago
Attachment #723974 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
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.
Attachment #723976 - Flags: review?(adw)
(Assignee)

Comment 6

5 years ago
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?
Attachment #723975 - Flags: review?(adw)
Attachment #723975 - Flags: feedback?(adw)

Updated

5 years ago
Attachment #727818 - Flags: review?(adw) → review+

Updated

5 years ago
Attachment #723976 - Flags: review?(adw) → review+

Comment 7

5 years ago
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.
Attachment #723975 - Flags: review?(adw)
Attachment #723975 - Flags: review+
Attachment #723975 - Flags: feedback?(adw)
(Assignee)

Comment 8

5 years ago
Created attachment 728838 [details] [diff] [review]
Test adjustment for ForgetAboutSite
Attachment #728838 - Flags: review?(adw)
(Assignee)

Comment 9

5 years ago
Created attachment 728841 [details] [diff] [review]
Update mobile browser.js
Attachment #728841 - Flags: review?(adw)
(Assignee)

Comment 10

5 years ago
(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

5 years ago
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.
Attachment #728838 - Flags: review?(adw) → review+

Comment 12

5 years ago
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.)
Attachment #728841 - Flags: review?(adw) → review+
(Assignee)

Comment 13

5 years ago
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
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
Whiteboard: [leave open]
(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
https://hg.mozilla.org/mozilla-central/rev/acf32763c61d
https://hg.mozilla.org/mozilla-central/rev/beb450416330
https://hg.mozilla.org/mozilla-central/rev/4bccb17667f0
(Assignee)

Comment 17

5 years ago
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
Whiteboard: [leave open]
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
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/82be854a0ac1
https://hg.mozilla.org/mozilla-central/rev/82be854a0ac1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.