[runtime] runtime.openOptionsPage does not work on Android

ASSIGNED
Assigned to

Status

()

Toolkit
WebExtensions: Android
P2
normal
ASSIGNED
3 months ago
9 hours ago

People

(Reporter: mattw, Assigned: rpl)

Tracking

(Blocks: 4 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [runtime], android, triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

3 months ago
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.
(Reporter)

Comment 1

3 months ago
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.

Updated

3 months ago
Assignee: nobody → mwein
Whiteboard: [runtime] → [runtime], android, triaged
(Reporter)

Updated

3 months ago
Blocks: 1302504

Updated

2 months ago
Blocks: 1226547
Comment hidden (mozreview-request)
(Assignee)

Updated

16 days ago
Assignee: matthewjwein → lgreco
Status: NEW → ASSIGNED

Comment 3

8 days ago
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.
(Assignee)

Updated

6 days ago
Duplicate of this bug: 1391002
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

5 days ago
Attachment #8894536 - Flags: review?(mixedpuppy)
Attachment #8898769 - Flags: review?(mixedpuppy)
(Assignee)

Updated

5 days ago
Blocks: 1388098

Comment 7

a day ago
mozreview-review
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.
Attachment #8898769 - Flags: review?(mixedpuppy) → review+

Comment 8

a day ago
mozreview-review
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
Attachment #8894536 - Flags: review?(mixedpuppy)
(Assignee)

Comment 9

13 hours ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

13 hours ago
Created attachment 8899888 [details]
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.
(Assignee)

Comment 13

13 hours ago
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.
Attachment #8894536 - Flags: review?(s.kaspari)
(Assignee)

Comment 14

13 hours ago
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.
Attachment #8898769 - Flags: review?(s.kaspari)

Comment 15

9 hours ago
mozreview-review
Comment on attachment 8894536 [details]
Bug 1364945 - Fix runtime.openOptionsPage on Firefox for Android.

https://reviewboard.mozilla.org/r/165700/#review176456
Attachment #8894536 - Flags: review?(mixedpuppy) → review+
You need to log in before you can comment on or make changes to this bug.