Closed Bug 1144296 Opened 5 years ago Closed 5 years ago

instantApply in preferences.xml is being set while it is readonly property

Categories

(Toolkit :: Preferences, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: aceman, Assigned: aceman)

Details

Attachments

(2 files, 2 obsolete files)

Warning: TypeError: setting a property that has only a getter
Source File: chrome://global/content/bindings/preferences.xml
Line: 597

The code is:
this.instantApply = psvc.getBoolPref("browser.preferences.instantApply");
if (this.instantApply) {

It seems to me "instantApply" property is readonly and only has a getter. So this code seems bogus.
Attached patch patch (obsolete) — Splinter Review
Removing that line works for me. I tested that the this.instantApply still returns a proper value after that line is removed. At least in Thunderbird on Linux.
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Attachment #8578850 - Flags: review?(gavin.sharp)
Attachment #8578850 - Flags: review?(gavin.sharp) → review?(jaws)
Comment on attachment 8578850 [details] [diff] [review]
patch

Review of attachment 8578850 [details] [diff] [review]:
-----------------------------------------------------------------

Are you sure this is right? When I look at this, I see `instantApply` is defined as a field for this binding (prefwindow), so setting the value should work as intended.

::: toolkit/content/widgets/preferences.xml
@@ -620,5 @@
>      <implementation implements="nsITimerCallback">
>        <constructor>
>        <![CDATA[
>          if (this.type != "child") {
>            var psvc = Components.classes["@mozilla.org/preferences-service;1"]

If the line can actually be removed, then `psvc` will be unused, so that can be removed too.
Attachment #8578850 - Flags: review?(jaws) → feedback+
I can try some tests whether that warning is wrong and is not seeing that <field> definition properly. Maybe it is confused by the other declarations of "instantApply" (e.g. as property) in the same file. Maybe the field should be renamed, e.g. to _instantApply as usual?
Attached patch patch for toolkit v2 (obsolete) — Splinter Review
OK, so it seems this is a conflict with the instantApply field in prefWindow binding (read-write) and the instantApply property (readonly) defined in Thunderbirds prefTab binding extending prefWindow, see http://mxr.mozilla.org/comm-central/source/mail/components/preferences/preferences.xml#86 .

This is my proposal for a fix. Can it be done without the new field? I found out the constructor of prefWindow is rund before the constructor of prefTab so we can't set the field's value in prefTab's constructor before it is used.
Attachment #8578850 - Attachment is obsolete: true
Attachment #8581134 - Flags: review?(jaws)
Attached patch patch for TBSplinter Review
This is the corresponding patch for TB to show how the _instantApplyInitialized field would be used.
Comment on attachment 8581134 [details] [diff] [review]
patch for toolkit v2

Review of attachment 8581134 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/preferences.xml
@@ +694,5 @@
>          }
>        ]]>
>        </destructor>
>  
> +      <field name="_instantApplyInitialized">false</field>

Can you please add a comment to say that derived bindings may overwrite this field to skip the reading of the browser.preferences.instantApply pref?

On another note, it is odd to the me that a /toolkit/ binding is checking a "browser.*" pref value. This seems broken, and it has existed since hg@1.
Attachment #8581134 - Flags: review?(jaws) → review+
Blair, do you think instead of this fix we should do some refactoring to remove references to a browser.* preference from /toolkit?
Flags: needinfo?(bmcbride)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> Blair, do you think instead of this fix we should do some refactoring to
> remove references to a browser.* preference from /toolkit?

In general, having browser.* prefs in toolkit is indeed something we should be trying hard to avoid. But in a case like this, where it's existed for so long and it's just one pref, I don't think it's worth the trouble.
Flags: needinfo?(bmcbride)
Attachment #8581135 - Flags: feedback?(richard.marti)
Thanks, added the comment.
Attachment #8581134 - Attachment is obsolete: true
Attachment #8588555 - Flags: review+
Comment on attachment 8581135 [details] [diff] [review]
patch for TB

This looks good and no error is shown. Thanks a lot for solving this.
Attachment #8581135 - Flags: feedback?(richard.marti) → feedback+
Attachment #8581135 - Flags: review?(mkmelin+mozilla)
Severity: normal → enhancement
OS: Linux → All
Hardware: x86 → All
Attachment #8581135 - Flags: review?(mkmelin+mozilla) → review+
Thanks.
Keywords: checkin-needed
Whiteboard: [checkin: one patch is for m-c, one for c-c]
https://hg.mozilla.org/integration/fx-team/rev/b45bb3e0a30d
Keywords: checkin-needed
Whiteboard: [checkin: one patch is for m-c, one for c-c] → [fixed-in-fx-team]
Kent, can you please take care of landing the c-c patch after this hits m-c?
Flags: needinfo?(rkent)
Yes, I can do the c-c checkin. Let me lead the need-info as a reminder since I have to wait for m-c
https://hg.mozilla.org/mozilla-central/rev/b45bb3e0a30d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.