Last Comment Bug 689404 - Changes to inline settings broke existing Firefox Mobile add-ons
: Changes to inline settings broke existing Firefox Mobile add-ons
Status: VERIFIED FIXED
[qa+]
: addon-compat, verified-aurora, verified-beta
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Geoff Lankow (:darktrojan)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-26 17:53 PDT by Mark Finkle (:mfinkle) (use needinfo?)
Modified: 2011-10-12 07:44 PDT (History)
10 users (show)
hskupin: in‑testsuite+
hskupin: in‑litmus?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
fixed


Attachments
patch (1.05 KB, patch)
2011-09-26 20:24 PDT, Mark Finkle (:mfinkle) (use needinfo?)
no flags Details | Diff | Review
patch (7.51 KB, patch)
2011-09-26 20:49 PDT, Geoff Lankow (:darktrojan)
bmcbride: review+
christian: approval‑mozilla‑beta+
Details | Diff | Review

Description Mark Finkle (:mfinkle) (use needinfo?) 2011-09-26 17:53:04 PDT
bug 669390 changed <setting type="control"> to only allow <button> children and added <setting type="multi"> to handle <menulist> and <radiogroup> children.

This breaks many Firefox Mobile add-ons which use <setting type="control"> for <menulist> children.

We need a way to keep <setting type="control"> to keep working for add-on compatibility. Ideas?
Comment 1 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-26 19:04:49 PDT
One simple idea is to allow <setting type="control"> to have <menulist> children. Keep <setting type="multi"> as is, allowing <menulist> too.

This would allow old code still use the old binding, but new code will use the new binding.
Comment 2 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-09-26 19:21:27 PDT
Yea, that's the easiest way (er, and the only way?). In one of the reviews, I think I explicitly asked for <setting type="control"> to stop including <menulist>, since it was something we didn't want to support forever. But if that's causing pain...
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-26 19:35:54 PDT
It is causing pain. We can work to get add-ons using the new binding, but we can't go cold turkey now.

This affects Fx8, which becomes "Beta" tomorrow/today.
Comment 4 Dave Townsend [:mossop] 2011-09-26 19:52:01 PDT
Blair do you have time to put together a patch for this? We need to get it in nightlies a.s.a.p. so we can get the confidence to push it to the betas.
Comment 5 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-26 19:57:10 PDT
I am trying a patch too
Comment 6 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-26 20:24:28 PDT
Created attachment 562650 [details] [diff] [review]
patch

This patch merely adds "menulist" back to the allowed children of <setting type="control">. This means the developer is responsible for handling the saving and loading of the preference to the control.

If the developer wants to let the binding handle the preference syncing, they should use <setting type="multi">

This patch does correct the problem in Firefox Mobile. We should consider this a compatibility fix only. Let's not try to make type="control" into something bigger than it already is.
Comment 7 Geoff Lankow (:darktrojan) 2011-09-26 20:49:13 PDT
Created attachment 562655 [details] [diff] [review]
patch

My patch is better! :-P

And also already on try. https://tbpl.mozilla.org/?tree=Try&rev=5c3a17a73d94
Comment 9 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-09-27 05:39:20 PDT
https://hg.mozilla.org/mozilla-central/rev/ac92a6d8297c
Comment 10 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-27 05:55:41 PDT
Thanks for the quick turn around guys!
Comment 11 Henrik Skupin (:whimboo) 2011-09-27 06:28:03 PDT
Mark, do you have an example extension which was broken?
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-27 07:59:03 PDT
(In reply to Henrik Skupin (:whimboo) from comment #11)
> Mark, do you have an example extension which was broken?

Bigger Text, by Matt Brubeck is one I tested. There are a few others too. Basically any add-on for Fennec that supplied it's own options is likely to fail.
Comment 13 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-27 14:17:13 PDT
Comment on attachment 562655 [details] [diff] [review]
patch

This patch was landed on Fx9 before Aurora, so it's already on Aurora now. We do need it on Beta though.

Safe, easy patch - with tests!
Comment 15 Henrik Skupin (:whimboo) 2011-09-27 17:01:18 PDT
QA should track this fix across branches. Ioana, can one of you please verify this fix on the target branches, once it has been landed? Thanks.
Comment 16 Andreea Pod 2011-10-12 07:27:12 PDT
Add-ons are working fine, including Bigger Text add-on. 

Verified fixed on Aurora 9 and Firefox 8 Beta 3: 
Mozilla /5.0 (Android;Linux armv7l;rv:9.0a2) Gecko/20111012 Firefox/9.0a2 Fennec/9.0a2  and  
Mozilla /5.0 (Android;Linux armv7l;rv:8.0) Gecko/20111011 Firefox/8.0 Fennec/8.0
Comment 17 Henrik Skupin (:whimboo) 2011-10-12 07:44:10 PDT
Geoff, I assume there is no manual test necessary? Please update the flag accordingly.

Note You need to log in before you can comment on or make changes to this bug.