1364945 - [runtime] runtime.openOptionsPage does not work on Android

Status: VERIFIED FIXED

(WebExtensions :: Android, defect, P2)

Description

[Matthew Wein [:mattw]]

MDN documentation: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/openOptionsPage

runtime.openOptionsPage depends on global.openOptionsPage defined in ext-browser.js - http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-browser.js#59 - which is not defined for mobile. This global function has a further dependency - window.BrowserOpenAddonsMgr defined in http://searchfox.org/mozilla-central/source/browser/base/content/browser.js#6613 - which also appears to not be defined for mobile.

Comment 1

[Matthew Wein [:mattw]]

The existing tests for runtime.openOptionsPage are located in browser/ - http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_runtime_openOptionsPage.js and http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_runtime_openOptionsPage_uninstall.js. These should  be moved to toolkit/ once the fixes are made.

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1364945 - Fix runtime.openOptionsPage on Firefox for Android.

Review commit: https://reviewboard.mozilla.org/r/165700/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/165700/

Comment 3

[Manish Jethani]

runtime.openOptionsPage is defined on Android, yet it doesn't seem to do anything. It doesn't even throw an error when you call it. This makes it impossible to do feature detection.

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1364945 - Fix missing button to open options in a new tab in the Android addon details page.

Review commit: https://reviewboard.mozilla.org/r/170156/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170156/

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8894536 [details]
Bug 1364945 - Fix runtime.openOptionsPage on Firefox for Android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165700/diff/1-2/

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8898769 [details]
Bug 1364945 - Fix missing button to open options in a new tab in the Android addon details page.

https://reviewboard.mozilla.org/r/170156/#review176088

I'm not able to really know what the css changes are doing or what they look like, but otherwise ok with this.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8894536 [details]
Bug 1364945 - Fix runtime.openOptionsPage on Firefox for Android.

https://reviewboard.mozilla.org/r/165700/#review175560

This is hitting further outside webextensions on android, I'd like to have someone on android look at this.  I'm not happy with the ping/pong notifications, it feels quite hacky.  If it's the only way to accomplish this then fine, but yuck.

::: mobile/android/chrome/content/aboutAddons.js:105
(Diff revision 2)
>  function init() {
>    window.addEventListener("popstate", onPopState);
>  
>    AddonManager.addInstallListener(Addons);
>    AddonManager.addAddonListener(Addons);
> -  Addons.init();
> +  Addons.init().then(() => {

Does the observer stuff need to happen prior to the rest of init?  Should you use await Addons.init?

::: mobile/android/chrome/content/aboutAddons.js:313
(Diff revision 2)
>      return element;
>    },
>  
>    init: function init() {
> -    let self = this;
> -    AddonManager.getAllAddons(function(aAddons) {
> +    return new Promise(resolve => {
> +      AddonManager.getAllAddons(aAddons => {

Can't you just "return AddonManager.getAllAddons(...)" ?

::: mobile/android/chrome/content/aboutAddons.js:337
(Diff revision 2)
> -      list.appendChild(browseItem);
> +        list.appendChild(browseItem);
> +
> +        resolve();
> -    });
> +      });
>  
> -    document.getElementById("uninstall-btn").addEventListener("click", Addons.uninstallCurrent.bind(this));
> +      document.getElementById("uninstall-btn").addEventListener("click", Addons.uninstallCurrent.bind(this));

these probably don't need to be part of the promise, could happen prior to the getAllAddons call I think.

::: mobile/android/chrome/content/browser.js:1434
(Diff revision 2)
> +        emWindow = subject;
> +      };
> +
> +      Services.obs.addObserver(receivePong, "EM-pong");
> +      Services.obs.notifyObservers(null, "EM-ping");
> +      Services.obs.removeObserver(receivePong, "EM-pong");

this pingpong stuff feels very strange, I'd like ot understand why its necessary

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8894536 [details]
Bug 1364945 - Fix runtime.openOptionsPage on Firefox for Android.

https://reviewboard.mozilla.org/r/165700/#review175560

I'm so sorry Shane, once I read your review comments I realized that I forgot to mention in a bugzilla comment (or in inline comment in the patch) the reason for the changes applied to aboutAddons.js (especially the observer and the related observice service messages):

To be able to fully support `runtime.openOptionsPage` we need to cover two scenarios:

- the options page is opened in a new tab (when `options_ui.open_in_tab` is true in the add-on manifest)
- the options page is embedded into the "about:addons" add-on detail view

On the desktop version of `runtime.openOptionsPage` we are currently using `BrowserOpenAddonsMgr`:

- http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/browser/components/extensions/ext-browser.js#91-93

Which internally uses this "EM-" messages:

- http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/browser/base/content/browser.js#6753-6802

This messages are used to detect if the "about:addons" page was already opened in a tab and ready to switch to the add-on details (which is done using the "EM-ping"/"EM-pong" messages), or if we need to wait it to be fully loaded (with is done using the "EM-loaded" message).

The "about:addons" page on Android doesn't currently provide anything similar to `BrowserOpenAddonsMgr` and so it doesn't send the same messages that the desktop version is using to achive this "coordination" between the Firefox internals and the "about:addons" page implementation.

I would like to propose a different way to do it, but given the current timing I opted to use the same strategy used by the "about:addons" on desktop as a first step (and evaluate during this review if we want to use a different strategy now or defer it to a follow up which could change it on both Android and Desktop).

I totally agree that this bits of the patch would be better reviewed from someone that works on the Android front end (I'm going to add sebastian on both these patches after I applied and pushed the other changes needed based on your review).

> Does the observer stuff need to happen prior to the rest of init?  Should you use await Addons.init?

The observer should only happen when the "about:addons" page has been initialized, I think that it is better to leave them as the last step of the init to prevent race conditions.

I'm also going to rewrite this functions as async, it is going to be more readable.
(I've been a bit too conservative of the existent syntaxes in this file, I think that we can introduce some async functions instead of the `new Promise` and `.then(() => { ... })` from this patch).

> Can't you just "return AddonManager.getAllAddons(...)" ?

ah, good catch, I didn't noticed that the getAllAddons has been converted into a promised function (and by searching into searchfox it seems that we have currently code that is manually promisifying it).

I'm converting this into an async function.

> these probably don't need to be part of the promise, could happen prior to the getAllAddons call I think.

These are the handlers for the buttons in the add-on detail page, it is probably better to subscribe these listeners when the list of add-ons has been already retrieved (but in any case these button are only visible when you enter into the add-on details view, which will not happen until the add-ons list have been retrieved and rendered).

> this pingpong stuff feels very strange, I'd like ot understand why its necessary

I know, I didn't like it neither (The details related to "why they are necessary and where they are coming from" are in the main comment above).

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8894536 [details]
Bug 1364945 - Fix runtime.openOptionsPage on Firefox for Android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165700/diff/2-3/

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8898769 [details]
Bug 1364945 - Fix missing button to open options in a new tab in the Android addon details page.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170156/diff/1-2/

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: addon_options_open_in_tab_button.png

(In reply to Shane Caraveo (:mixedpuppy) from comment #7)
> I'm not able to really know what the css changes are doing or what they look
> like, but otherwise ok with this.

That's true, I'm attaching a screenshot of it here.

Basically it makes the button to look more like the other buttons already provided by the add-ons details page.

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8894536 [details]
Bug 1364945 - Fix runtime.openOptionsPage on Firefox for Android.

Hi Sebastian,
do you mind to take a look at the bits related to the aboutAddons.js file from this patch? (or to redirect it to someone who could)

More details about the reason and goal of the change are in the first part of Comment 9.

Comment 14

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8898769 [details]
Bug 1364945 - Fix missing button to open options in a new tab in the Android addon details page.

Hi Sebastian,
do you mind to take a look at the bits related to the aboutAddons.css file from this patch? (or to redirect it to someone who could)

The css applied to the "button#open-addon-options" selector makes the new "Options" button, that is added when the optionsType of an addon is AddonManager.OPTIONS_TYPE_TAB (the code that handle the button creation and its click event is in Attachment 8894536 [details]), to look more like the other buttons from the add-on details view. 

Attachment 8899888 [details] is a screenshot of it.

Comment 15

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8894536 [details]
Bug 1364945 - Fix runtime.openOptionsPage on Firefox for Android.

https://reviewboard.mozilla.org/r/165700/#review176456

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8894536 [details]
Bug 1364945 - Fix runtime.openOptionsPage on Firefox for Android.

https://reviewboard.mozilla.org/r/165700/#review176878

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8898769 [details]
Bug 1364945 - Fix missing button to open options in a new tab in the Android addon details page.

https://reviewboard.mozilla.org/r/170156/#review176880

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f59af84e1c18
Fix runtime.openOptionsPage on Firefox for Android. r=mixedpuppy,sebastian
https://hg.mozilla.org/integration/autoland/rev/8cfc2230679e
Fix missing button to open options in a new tab in the Android addon details page. r=mixedpuppy,sebastian

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f59af84e1c18
https://hg.mozilla.org/mozilla-central/rev/8cfc2230679e

Comment 20

[CosminB]

Attachment: Videos.zip

This issue is verified as fixed on Fennec 57.0a1 (2017-08-25) under Android 6.0.1

Verified with https://addons-dev.allizom.org/en-US/firefox/addon/optionsui/ the open_in_tab and without, all the scenarios work as expected.

Please see the attached videos.

Comment 21

[ntninja]

Thanks for fixing this! I just wanted to mention that the “Browser Compatibility”-table on
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/openOptionsPage
still lists this feature as “Not supported” for Firefox for Android.

Will it be updated automatically once 57 enters Beta?

Comment 22

[Will Bamberg [:wbamberg]]

I've updated the compat data for this function: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/openOptionsPage.

Let me know if we need anything else.

Comment 23

[Krasnaya Ploshchad’]

I installed URLRedirector (https://addons.mozilla.org/zh-CN/firefox/addon/urlredirector/) in Firefox Beta for Android 57.0, after installation, I opened about:addons page, press URLRedirector item, then I see the option button shown.

Comment 24

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Will Bamberg [:wbamberg] from comment #22)
> I've updated the compat data for this function:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/
> openOptionsPage.
> 
> Let me know if we need anything else.

Thanks Will, the updated compat data looks correct.