473065 - use a more specific prefBranch (browser.tabs.) in the tabbrowser-tabs binding

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Simon Bünzli]

... and just one observer, in case further prefs are to be observed (such as the one added by bug 465673).

Bonus points for removing the unused mPrefs property and the _mPrefs field; and maybe also for using a "browser." prefBranch for the tabbrowser's own mPrefs.

Comment 1

[Michael Kohler [:mkohler]]

Is this still wanted?

Comment 2

[Michael Kohler [:mkohler]]

Attachment: Patch v1 (trivial)

* removes unused mPrefs property
* the other changes are not necessary because we now use Services.prefs.*

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 456177 [details] [diff] [review]
Patch v1 (trivial)

Err, isn't this actually in the section for stuff that we kept only for add-on compatibility?

Comment 4

[Michael Kohler [:mkohler]]

Attachment: just use one observer

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 456908 [details] [diff] [review]
just use one observer

I'm not sure this is actually a win, as it means that _prefObserver will be called for prefs it doesn't care about.

r- because you missed related code in the destructor.

Comment 6

[Michael Kohler [:mkohler]]

Attachment: just use one observer v2

(In reply to comment #5)
> I'm not sure this is actually a win, as it means that _prefObserver will be
> called for prefs it doesn't care about.

Adding the two other observers takes a few milliseconds, too.And the browser.tabs.* preferences don't get changed every few minutes, do they?

> r- because you missed related code in the destructor.

*facepalm*

Comment 7

[:Gavin Sharp [email: [e-mail]]]

None of these prefs change very often (if at all), AFAIK, so I think memory footprint is a more important consideration than the number of cycles spent during a pref change. It's not a particularly big deal either way, though, especially since it's just three separate references to the same underlying observer (which is wrapped only once, IIRC).

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 456914 [details] [diff] [review]
just use one observer v2

># HG changeset patch
># User Michael Kohler <[e-mail]>
>Bug 473065 - use a more specific prefBranch (browser.tabs.) in the tabbrowser-tabs binding. r=dao
>
>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml
>@@ -2379,29 +2379,24 @@
>           var tab = this.firstChild;
>           tab.setAttribute("label",
>                            this.tabbrowser.mStringBundle.getString("tabs.emptyTabTitle"));
>           tab.setAttribute("crop", "end");
>           tab.setAttribute("validate", "never");
>           tab.setAttribute("onerror", "this.removeAttribute('image');");
>           this.adjustTabstrip();
> 
>-          Services.prefs.addObserver("browser.tabs.closeButtons", this._prefObserver, false);
>-          Services.prefs.addObserver("browser.tabs.autoHide", this._prefObserver, false);
>-          Services.prefs.addObserver("browser.tabs.closeWindowWithLastTab", this._prefObserver, false);
>-
>+          Services.prefs.addObserver("browser.tabs.", this._prefObserver, false);
>           window.addEventListener("resize", this, false);
>         ]]>
>       </constructor>
> 
>       <destructor>
>         <![CDATA[
>-          Services.prefs.removeObserver("browser.tabs.closeButtons", this._prefObserver);
>-          Services.prefs.removeObserver("browser.tabs.autoHide", this._prefObserver);
>-          Services.prefs.removeObserver("browser.tabs.closeWindowWithLastTab", this._prefObserver);
>+          Services.prefs.removeObserver("browser.tabs.", this._prefObserver);
>         ]]>
>       </destructor>
> 
>       <field name="tabbrowser" readonly="true">
>         document.getElementById(this.getAttribute("tabbrowser"));
>       </field>
> 
>       <field name="tabbox" readonly="true">

Comment 9

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/97fd6a75cf7a