669390 - Allow setting a preference value from the menulist inline preferences

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, enhancement)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0a1) Gecko/20110703 Firefox/7.0a1

Found that while working with the example extension from bug 653637. First, in the test there is no preference connected to that instance of the menulist. Second, even if you set a preference to the setting element, it will not be set or updated. So this element is useless at the moment.

Example code:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/addons/browser_inlinesettings1/options.xul

Also I do not see that we check if the correct preference has been set for any kind of element.

We should consider to probably remove that type of element for Firefox 7. See also the broken styling reported as bug 669342.

Comment 1

[Dave Townsend [:mossop]]

Menulists aren't meant to be linked to preferences right now.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So what is the purpose of menulists then? For what can those be used if I can't permanently store a setting?

Comment 3

[Dave Townsend [:mossop]]

They can be, but it requires manual code to do so at the moment.

Comment 4

[Geoff Lankow (:darktrojan)]

Attachment: patch

This goes on top of my patch for bug 669345.

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: changes for mobile

Just the changes for mobile, plus bug 669345 comment 4, because it's easier to do it here than deal with the bitrot.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 550270 [details] [diff] [review]
changes for mobile

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

r+ on the toolkit stuff

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 550270 [details] [diff] [review]
changes for mobile

mobile part looks good

Comment 8

[Geoff Lankow (:darktrojan)]

Alright, let's land this sucker.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/fx-team/rev/46ad396b43cb
https://hg.mozilla.org/integration/fx-team/rev/10987bba3dd6

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Backed out due to either this or bug 669345 leaking. No idea whats going on, but it started happening with that push - on all debug builds.
https://hg.mozilla.org/integration/fx-team/rev/51d7ece2bb10


http://tinderbox.mozilla.org/showlog.cgi?log=Fx-Team/1312863307.1312868107.7800.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Fx-Team/1312863348.1312867944.7375.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Fx-Team/1312867017.1312871451.22166.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Fx-Team/1312862532.1312866829.2356.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Fx-Team/1312865060.1312868669.9847.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Fx-Team/1312862109.1312866139.32626.gz

Comment 11

[Geoff Lankow (:darktrojan)]

Attachment: changes to tests

Found the leak. Also found I'm testing something that doesn't really exist any more.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 552035 [details] [diff] [review]
changes to tests

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

Odd that the only thing that caught that typo was a leak.

Could you roll this into the original patch before it's landed? It means slightly bitrotting the mobile patch, but it's nice to not have individual commits be broken if possible.

Comment 13

[Geoff Lankow (:darktrojan)]

Attachment: patch v2

Comment 14

[Geoff Lankow (:darktrojan)]

Attachment: patch for mobile, v2

Comment 15

[Geoff Lankow (:darktrojan)]

Attachment: patch v3

Comment 16

[Geoff Lankow (:darktrojan)]

Attachment: patch for mobile, v3

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/6ece2d6ece8e
http://hg.mozilla.org/integration/mozilla-inbound/rev/2de3cff973b2

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/6ece2d6ece8e
http://hg.mozilla.org/mozilla-central/rev/2de3cff973b2

Comment 19

[Geoff Lankow (:darktrojan)]

https://developer.mozilla.org/en/Extensions/Inline_Options updated.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0a2) Gecko/20110824 Firefox/8.0a2