Closed
Bug 909093
Opened 11 years ago
Closed 10 years ago
Update SeaMonkey to use nsIContentPrefService2
Categories
(SeaMonkey :: General, defect)
SeaMonkey
General
Tracking
(seamonkey2.26 fixed, seamonkey2.27 affected, seamonkey2.28 fixed)
RESOLVED
FIXED
seamonkey2.28
People
(Reporter: mcsmurf, Assigned: neil)
References
Details
Attachments
(2 files, 4 obsolete files)
29.41 KB,
patch
|
Details | Diff | Splinter Review | |
8.73 KB,
patch
|
philip.chee
:
review+
philip.chee
:
feedback+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
Attachment #801380 -
Attachment is patch: true
Reporter | ||
Comment 3•11 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•11 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•11 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•11 years ago
|
||
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•10 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•10 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•10 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•10 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.)
Reporter | ||
Comment 12•10 years ago
|
||
This should fix all bugs...
Attachment #811753 -
Attachment is obsolete: true
Reporter | ||
Comment 13•10 years ago
|
||
Forgot to remove debug output
Attachment #8388053 -
Attachment is obsolete: true
Assignee | ||
Comment 14•10 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•10 years ago
|
||
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•10 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•10 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•10 years ago
|
||
Pushed comm-central changeset 960e67998413.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.28
Reporter | ||
Comment 19•10 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)
Comment 20•10 years ago
|
||
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.
Description
•