1106415 - Allow the add-on SDK to dynamically insert add-on options

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Add-on Manager, defect)

Description

[:Margaret Leibovic]

It looks like there are 2 things we need to do:

1) Always fire the OPTIONS_NOTIFICATION_DISPLAYED notification after loading options.xul (even if we don't find valid options in there)
2) Don't clear the optionsURL attribute on the add-on item (this will require some CSS tweaks)

Comment 1

[Adrian Aichner [:anaran]]

Sounds good to me, thanks!

Comment 2

[:Margaret Leibovic]

Attachment: Allow the add-on SDK to dynamically insert add-on options

The add-on SDK uses a placeholder options URL, then waits for the "addon-options-displayed" notification to fire before dynamically populating the options.

This patch fires that notification whether or not we find valid settings items, and it doesn't reset the optionsURL attribute to hide the options elements. The CSS changes I made just remove some lines that are never actually needed.

The one risk with this patch is that if an add-on author supplies a bad options file, we may just show an empty options section, but I think that's fine, since an add-on author should be able to see and debug that.

anaran, can you try out this build and let me know if it works for you?

http://people.mozilla.org/~mleibovic/tmp/fennec-jetpack-debug.apk

Comment 3

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

Comment on attachment 8530684 [details] [diff] [review]
Allow the add-on SDK to dynamically insert add-on options

>diff --git a/mobile/android/chrome/content/aboutAddons.js b/mobile/android/chrome/content/aboutAddons.js
>-          } else {
>-            // No options, so hide the header and reset the list item
>-            detailItem.setAttribute("optionsURL", "");
>-            aListItem.setAttribute("optionsURL", "");

By removing this, do we always show empty options section, even if the add-on supplies no options? If so, maybe we should display an empty message?

r+ even with my question, expecting that we'd address it one way or another.

Comment 4

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #3)
> Comment on attachment 8530684 [details] [diff] [review]
> Allow the add-on SDK to dynamically insert add-on options
> 
> >diff --git a/mobile/android/chrome/content/aboutAddons.js b/mobile/android/chrome/content/aboutAddons.js
> >-          } else {
> >-            // No options, so hide the header and reset the list item
> >-            detailItem.setAttribute("optionsURL", "");
> >-            aListItem.setAttribute("optionsURL", "");
> 
> By removing this, do we always show empty options section, even if the
> add-on supplies no options? If so, maybe we should display an empty message?

No, if the add-on supplies no options, the optionsURL attribute will already be set to "":
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutAddons.js#233

I tested this with add-ons that both supply and don't supply options.

Comment 5

[Adrian Aichner [:anaran]]

Hi Margaret, the delay was caused by some complications.

First of all see Bug 1102504 comment 7. (I'll send another pull request for that bug too).

With this build of fennec leibovic (cute fennec image by the way) I get inline options, but more than I would like.

I have to use cfx (see bug 1102504) to test this to get the addon-sdk changes being used at all.

With that I get a new set of all preferences options displays in about:addons after each disable/enable cycle.

I even see options from another add-on leaking into the other add-on.

Comment 6

[Adrian Aichner [:anaran]]

Comment on attachment 8530684 [details] [diff] [review]
Allow the add-on SDK to dynamically insert add-on options

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

This all looks good to me.
I expect the issues described in Comment 5 to be caused by native-options.js in the addon-sdk, which I will take a look at again.

Comment 7

[:Margaret Leibovic]

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

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9bbaa27f0975

Comment 9

[Adrian Aichner [:anaran]]

(In reply to adrian.aichner from comment #6)
> Comment on attachment 8530684 [details] [diff] [review]
> Allow the add-on SDK to dynamically insert add-on options
> 
> Review of attachment 8530684 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This all looks good to me.
> I expect the issues described in Comment 5 to be caused by native-options.js
> in the addon-sdk, which I will take a look at again.

This issue might need a fix in the android aboutAddons.js after all.

[:Margaret] can you please take a look at
https://github.com/anaran/addon-sdk/commit/7fb09539b012eadce3e1d6cb999471abd1fc28e9#diff-5065117436342ee1a2547f0204d87e36R169