Closed Bug 1106415 Opened 7 years ago Closed 7 years ago

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

Categories

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

35 Branch
x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 37

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(1 file)

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)
Sounds good to me, thanks!
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
Attachment #8530684 - Flags: review?(mark.finkle)
Attachment #8530684 - Flags: feedback?(adrian.aichner)
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.
Attachment #8530684 - Flags: review?(mark.finkle) → review+
(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.
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 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.
Attachment #8530684 - Flags: feedback?(adrian.aichner) → feedback+
https://hg.mozilla.org/mozilla-central/rev/9bbaa27f0975
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
(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
Blocks: 1110502
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.