Add support for options_ui on Android

NEW
Assigned to

Status

()

Toolkit
WebExtensions: Android
P2
normal
8 months ago
6 days ago

People

(Reporter: mattw, Assigned: mattw)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 affected)

Details

(Whiteboard: triaged)

MozReview Requests

()

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

Attachments

(1 attachment)

Comment hidden (empty)

Updated

8 months ago
Blocks: 467520

Updated

8 months ago
Whiteboard: triaged

Updated

7 months ago
Blocks: 1226547
No longer blocks: 467520

Updated

7 months ago
Duplicate of this bug: 1292347

Updated

5 months ago
webextensions: --- → +

Updated

5 months ago
Assignee: nobody → mwein

Updated

3 months ago
Blocks: 1185785
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8855447 - Flags: feedback?(kmaglione+bmo)

Comment 3

2 months ago
mozreview-review
Comment on attachment 8855447 [details]
Bug 1302504 - use mozbrowser to add support for options_ui on Android

https://reviewboard.mozilla.org/r/127308/#review130042

::: mobile/android/chrome/content/aboutAddons.js:28
(Diff revision 1)
> +XPCOMUtils.defineLazyModuleGetter(this, "AppConstants",
> +                                  "resource://gre/modules/AppConstants.jsm");

This looks unused?

::: mobile/android/chrome/content/aboutAddons.js:356
(Diff revision 1)
> -    // Retrieve the extensions preferences
> -    try {
> -      let optionsURL = aListItem.getAttribute("optionsURL");
> +    let optionsURL = aListItem.getAttribute("optionsURL");
> +    switch (parseInt(addon.optionsType)) {
> +      case AddonManager.OPTIONS_TYPE_INLINE_BROWSER:
> +        optionsHeader.classList.add("hidden");

I'm not sure we really want to hide this. But it we do, please remove "hidden" before the switch-case, and re-add it when it's necessary.

::: mobile/android/chrome/content/aboutAddons.js:365
(Diff revision 1)
> +    let list = document.querySelector("#addons-list");
> +    list.style.display = "none";
> +    let details = document.querySelector("#addons-details");
> +    details.style.display = "block";
> +    document.documentElement.setAttribute("details", "true");

Hm. This is kind of gross, but I guess it's not your fault.

::: mobile/android/chrome/content/aboutAddons.js:373
(Diff revision 1)
> +    details.style.display = "block";
> +    document.documentElement.setAttribute("details", "true");
> +  },
> +
> +  createWebExtensionOptions: async function(destination, optionsURL, browserStyle) {
> +    let frame = document.createElementNS("http://www.w3.org/1999/xhtml", "html:iframe");

No need for the "html:" prefix. Or the namespace. This is already an XHTML document with a default namespace, so just `document.createElement("iframe")`

::: mobile/android/chrome/content/aboutAddons.js:374
(Diff revision 1)
> +    document.documentElement.setAttribute("details", "true");
> +  },
> +
> +  createWebExtensionOptions: async function(destination, optionsURL, browserStyle) {
> +    let frame = document.createElementNS("http://www.w3.org/1999/xhtml", "html:iframe");
> +    frame.setAttribute("id", "options");

Please use the same ID that we use for the desktop <browser>.
Attachment #8855447 - Flags: feedback?(kmaglione+bmo) → feedback+
(Assignee)

Comment 4

a month ago
mozreview-review-reply
Comment on attachment 8855447 [details]
Bug 1302504 - use mozbrowser to add support for options_ui on Android

https://reviewboard.mozilla.org/r/127308/#review130042

> This looks unused?

Removed.

> I'm not sure we really want to hide this. But it we do, please remove "hidden" before the switch-case, and re-add it when it's necessary.

On desktop we don't show the options header for webextions options, so I think we should do the same here.

> Hm. This is kind of gross, but I guess it's not your fault.

Yeah, it's not ideal. I'll see if I can make this cleaner.

> No need for the "html:" prefix. Or the namespace. This is already an XHTML document with a default namespace, so just `document.createElement("iframe")`

Nice, will update.

> Please use the same ID that we use for the desktop <browser>.

I didn't see one for desktop so I added an ID there. Let me know if I should do something different.
(Assignee)

Comment 5

a month ago
mozreview-review-reply
Comment on attachment 8855447 [details]
Bug 1302504 - use mozbrowser to add support for options_ui on Android

https://reviewboard.mozilla.org/r/127308/#review130042

> On desktop we don't show the options header for webextions options, so I think we should do the same here.

However, on desktop we also don't show an options header for non-webextensions..
Comment hidden (mozreview-request)
(Assignee)

Updated

28 days ago
Attachment #8855447 - Flags: review?(kmaglione+bmo) → review?(mixedpuppy)
(Assignee)

Comment 7

28 days ago
I will follow up with a test for this in a second commit.

Comment 8

28 days ago
mozreview-review
Comment on attachment 8855447 [details]
Bug 1302504 - use mozbrowser to add support for options_ui on Android

https://reviewboard.mozilla.org/r/127308/#review137004

::: mobile/android/chrome/content/aboutAddons.js:373
(Diff revision 2)
> +
> +  createWebExtensionOptions: async function(destination, optionsURL, browserStyle) {
> +    destination.innerHTML = "";
> +
> +    let frame = document.createElement("iframe");
> +    frame.setAttribute("id", "addon-options");

If its possible for more than one to be in dom, use a unique id.  not clear where the id is being used either.

::: mobile/android/chrome/content/aboutAddons.js:434
(Diff revision 2)
>            let id = aListItem.getAttribute("addonID");
>            Services.obs.notifyObservers(document, AddonManager.OPTIONS_NOTIFICATION_DISPLAYED, id);
>          }
>        }
>        xhr.send(null);
>      } catch (e) { }

pretty big catch here, is it necessary to wrap all this?  can we at least log the error?

::: toolkit/mozapps/extensions/content/extensions.js:3627
(Diff revision 2)
>  
>    createOptionsBrowser: Task.async(function*(parentNode) {
>      let browser = document.createElement("browser");
>      browser.setAttribute("type", "content");
>      browser.setAttribute("disableglobalhistory", "true");
> +    browser.setAttribute("id", "addon-options");

dito on the previous comment about the id for the options browser.
Attachment #8855447 - Flags: review?(mixedpuppy)
(Assignee)

Updated

6 days ago
Depends on: 1364945
You need to log in before you can comment on or make changes to this bug.