Update assorted contentPrefs usage to nsIContentPrefService2

RESOLVED FIXED in mozilla23

Status

()

RESOLVED FIXED
6 years ago
6 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

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

Comment 1

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

Comment 2

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

Comment 3

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

Comment 4

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

Updated

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

Comment 5

6 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

6 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

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

Updated

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

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

Comment 9

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

Comment 10

6 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 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 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 17

6 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]
https://hg.mozilla.org/mozilla-central/rev/82be854a0ac1
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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.