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

Status: RESOLVED FIXED

(Toolkit :: Preferences, enhancement)

Description

[:aceman]

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.

Comment 1

[:aceman]

Attachment: patch

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.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 3

[:aceman]

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?

Comment 4

[:aceman]

Attachment: patch for toolkit v2

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.

Comment 5

[:aceman]

Attachment: patch for TB

This is the corresponding patch for TB to show how the _instantApplyInitialized field would be used.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Blair, do you think instead of this fix we should do some refactoring to remove references to a browser.* preference from /toolkit?

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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.

Comment 9

[:aceman]

Attachment: patch for toolkit v2.1

Thanks, added the comment.

Comment 10

[Richard Marti (:Paenglab)]

Comment on attachment 8581135 [details] [diff] [review]
patch for TB

This looks good and no error is shown. Thanks a lot for solving this.

Comment 11

[:aceman]

Thanks.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/b45bb3e0a30d

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Kent, can you please take care of landing the c-c patch after this hits m-c?

Comment 14

[Kent James (: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

Comment 15

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/b45bb3e0a30d

Comment 16

[Kent James (:rkent)]

Comment on attachment 8581135 [details] [diff] [review]
patch for TB

http://hg.mozilla.org/comm-central/rev/ceced11e5b78