Update SeaMonkey to use nsIContentPrefService2

RESOLVED FIXED in seamonkey2.28

Status

SeaMonkey
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mcsmurf, Assigned: neil@parkwaycc.co.uk)

Tracking

Trunk
seamonkey2.28

SeaMonkey Tracking Flags

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

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

4 years ago
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
(Assignee)

Updated

4 years ago
Depends on: 913948
(Assignee)

Updated

4 years ago
No longer depends on: 913948
(Assignee)

Comment 1

4 years ago
Created attachment 801311 [details] [diff] [review]
Possible patch

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)
(Reporter)

Comment 2

4 years ago
Created attachment 801380 [details] [diff] [review]
my patch (just for reference)

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 :/
(Reporter)

Updated

4 years ago
Attachment #801380 - Attachment is patch: true
(Reporter)

Comment 3

4 years ago
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).

Comment 4

4 years ago
I am very much confused as to why we need to stop using Sevices.contentprefs directly. That should already be nsIContentPrefService2 anyhow, I thought?

Comment 5

4 years ago
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.
(Reporter)

Comment 6

4 years ago
Created attachment 811753 [details] [diff] [review]
My patch, part 2

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 7

4 years ago
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+
(Assignee)

Comment 8

4 years ago
(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 :-(
(Assignee)

Comment 9

4 years ago
(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.
(Assignee)

Comment 10

4 years ago
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.)

Updated

4 years ago
Duplicate of this bug: 952242
(Reporter)

Comment 12

4 years ago
Created attachment 8388053 [details] [diff] [review]
Updated patch, part 3

This should fix all bugs...
Attachment #811753 - Attachment is obsolete: true
(Reporter)

Comment 13

4 years ago
Created attachment 8388056 [details] [diff] [review]
Updated patch, part 3

Forgot to remove debug output
Attachment #8388053 - Attachment is obsolete: true
(Assignee)

Comment 14

4 years ago
(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.
(Assignee)

Comment 15

4 years ago
Created attachment 8388105 [details] [diff] [review]
Addressed review comments
Attachment #801311 - Attachment is obsolete: true
Attachment #801311 - Flags: review?(bugzilla)
Attachment #8388105 - Flags: review?(bugzilla)
Attachment #8388105 - Flags: feedback?(philip.chee)
(Reporter)

Comment 16

4 years ago
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 17

4 years ago
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+
(Assignee)

Comment 18

4 years ago
Pushed comm-central changeset 960e67998413.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.28
(Reporter)

Comment 19

4 years ago
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)

Updated

4 years ago
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
status-seamonkey2.26: --- → fixed
status-seamonkey2.27: --- → affected
status-seamonkey2.28: --- → fixed
You need to log in before you can comment on or make changes to this bug.