Closed Bug 909093 Opened 11 years ago Closed 10 years ago

Update SeaMonkey to use nsIContentPrefService2

Categories

(SeaMonkey :: General, defect)

defect
Not set
normal

Tracking

(seamonkey2.26 fixed, seamonkey2.27 affected, seamonkey2.28 fixed)

RESOLVED FIXED
seamonkey2.28
Tracking Status
seamonkey2.26 --- fixed
seamonkey2.27 --- affected
seamonkey2.28 --- fixed

People

(Reporter: mcsmurf, Assigned: neil)

References

Details

Attachments

(2 files, 4 obsolete files)

There are some places where SeaMonkey still uses nsIContentPrefService. nsIContentPrefService has been deprecated and new nsIContentPrefService2 should be used.
Example for such usage of the old interface: http://hg.mozilla.org/comm-central/annotate/7e5a9f1ce6ec/suite/common/viewZoomOverlay.js
Depends on: 913948
No longer depends on: 913948
Attached patch Possible patch (obsolete) — Splinter Review
Only requesting mcsmurf because he might have looked at the code recently. Feel free to retarget the review.

Most of the API has an equivalent. However we need to fetch the global value ourself. Also there's no has(Cached)Pref. For hasCachedPref we just need to check whether getCachedByDomainAndName is not null. For hasPref I just check to see whether the pref has an undefined cached value, as if it has a defined cached value then we won't be using the global setting, and if it's not cached yet then we're probably querying it already.

I am writing this late at night and already while typing this comment I think I can see two bugs. Oops...
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #801311 - Flags: review?(bugzilla)
Attachment #801311 - Flags: feedback?(philip.chee)
Attached patch my patch (just for reference) (obsolete) — Splinter Review
Just for reference: This is what I had created as patch. I never got it working, there was/is a bug in my patch that I was not able to spot when I worked on this :/
Attachment #801380 - Attachment is patch: true
Neil: I would like to compare/integrate your patch with my patch, that is why this is taking a bit longer. My patch is a bit more complicated but this is what the current FF code for this currently looks like. The additional code was required to make all tests iirc as there were problems with the async handling of the zoom events in some tests (random oranges, async race conditions).
I am very much confused as to why we need to stop using Sevices.contentprefs directly. That should already be nsIContentPrefService2 anyhow, I thought?
Hmm, I see it apparently isn't, looking at http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/modules/Services.jsm#47 - but then the patch should be done there and just move that one to the non-deprecated one.
Attached patch My patch, part 2 (obsolete) — Splinter Review
This is what I have so far, unfortunately not everything is working yet, not sure why (maybe wrong usage of getBrowser()?). What's not working is when I have two tabs open with the same page, change the zoom in one tab (that change gets reflected in the other tab, that's ok) and then reset the zoom with the shortcut Ctrl+0. This will not reset the zoom in the other tab.
Attachment #801380 - Attachment is obsolete: true
Comment on attachment 801311 [details] [diff] [review]
Possible patch

> +    // Fetch the initial global value.
> +    this.contentPrefs.getGlobal(this.name, null, {
> +      self: this,
Can we use more of .bind() ? Unless there is a reason for not using bind().

> +      handleCompletion: function(aReason) {},
> +      handleError: function(aResult) {},
> +      handleResult: function(aPref) {
> +        this.self.globalValue = this._ensureValid(aPref.value);
Shouldn't this be: this.self_ensureValid() ?

Some questions. I'm a bit baffled by the following:
Why is _applyPrefToSetting() called in both handleCompletion() and in handleResult()?

>       this.contentPrefs.getByDomainAndName(aURI.spec, this.name, loadContext, {
>         self: this,
>         handleCompletion: function(aReason) {
>           // If the current page doesn't have a site-specific preference,
>           // then its zoom should be set to the default preference.
>           if (!this.self.contentPrefs.getCachedByDomainAndName(aURI.spec, this.self.name, loadContext).value)
>             this.self._applyPrefToSetting();
>         },
>         handleError: function(aResult) {},
>         handleResult: function(aPref) {
>           // Check that we're still where we expect to be in case this took a
>           // while. Null check currentURI, since the window may have been
>           // destroyed before we were called.
>           if (aBrowser.currentURI && aURI.equals(aBrowser.currentURI))
>             this.self._applyPrefToSetting(aPref.value, aBrowser);
>         }
>       });

Other than that, everything seems to work
Attachment #801311 - Flags: feedback?(philip.chee) → feedback+
(In reply to Philip Chee from comment #7)
> (From update of attachment 801311 [details] [diff] [review])
> > +    // Fetch the initial global value.
> > +    this.contentPrefs.getGlobal(this.name, null, {
> > +      self: this,
> Can we use more of .bind() ? Unless there is a reason for not using bind().
Need to do it for each function. (In this case there is only one function, so it's not too bad. But in fact in this case I can simplify the code by just using this itself.)

> > +        this.self.globalValue = this._ensureValid(aPref.value);
> Shouldn't this be: this.self_ensureValid() ?
Good catch. I didn't notice because by default there is no global value.

> Why is _applyPrefToSetting() called in both handleCompletion() and in
> handleResult()?
I can't remember :-(
(In reply to Philip Chee from comment #7)
> (From update of attachment 801311 [details] [diff] [review])
> > +        this.self.globalValue = this._ensureValid(aPref.value);
> Shouldn't this be: this.self_ensureValid() ?
> 
> Some questions. I'm a bit baffled by the following:
> Why is _applyPrefToSetting() called in both handleCompletion() and in
> handleResult()?
Interestingly what I currently have in my tree isn't what's in this patch. In particular, I have this.self.onContentPrefSet(null, this.name, aPref.value); above and handleCompletion below is just a dummy function.
Ah, I was looking at the wrong tree. The question is, which one was newer? (Unfortunately I didn't think to check before editing them... oops.)
Attached patch Updated patch, part 3 (obsolete) — Splinter Review
This should fix all bugs...
Attachment #811753 - Attachment is obsolete: true
Forgot to remove debug output
Attachment #8388053 - Attachment is obsolete: true
(In reply to Philip Chee from comment #7)
> Why is _applyPrefToSetting() called in both handleCompletion() and in
> handleResult()?

We don't know if there is a pref or not. Worse, the API doesn't easily tell us. So, we have to jump through all those hoops.

If there is a result, it will be passed to us in handleResult.

handleCompletion will be called whether or not there was a result, but without telling the result.

Thinking about it, it's probably best for handleResult to save the result and then use it (or lack of result) in handleCompletion.
Attachment #801311 - Attachment is obsolete: true
Attachment #801311 - Flags: review?(bugzilla)
Attachment #8388105 - Flags: review?(bugzilla)
Attachment #8388105 - Flags: feedback?(philip.chee)
So we want to take your patch? ;) The differences between your and my patch seem to rather high. The getBrowserToken thing is mostly to get tests running properly (async handling)
Comment on attachment 8388105 [details] [diff] [review]
Addressed review comments

Looks good. f+ also r+ if needed
Attachment #8388105 - Flags: review+
Attachment #8388105 - Flags: feedback?(philip.chee)
Attachment #8388105 - Flags: feedback+
Pushed comm-central changeset 960e67998413.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.28
Comment on attachment 8388105 [details] [diff] [review]
Addressed review comments

I still wonder if my patch is better... :o but maybe one day I'll get those tests working.
Attachment #8388105 - Flags: review?(bugzilla)
Blocks: 1018792
NOTE this cset was not landed for SeaMonkey 2.27, however

Transplanted for SeaMonkey 2.26.1 on comm-release

$ hg tip
changeset:   20176:91519486358d
branch:      SEA_2_26_1_RELBRANCH
tag:         tip
user:        Neil Rashbrook <neil@parkwaycc.co.uk>
date:        Tue Apr 15 11:31:17 2014 +0100
summary:     Bug 909093 Update SeaMonkey to use nsIContentPrefService2 r=Ratty
You need to log in before you can comment on or make changes to this bug.