1250784 - Implement the options V2 API

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Kris Maglione [:kmag]]

This includes the relevant UI, manifest sections, and API:

https://developer.chrome.com/extensions/optionsV2
https://developer.chrome.com/extensions/runtime#method-openOptionsPage

Comment 1

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1250784: Part 1 - [webext] Add support for options_ui via inline browsers in the Add-on Manager. r?Mossop

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

Comment 2

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1250784: Part 2 - [webext] Fix some issues that cause noisy tests. r=Mossop

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

Comment 3

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1250784: Part 3 - [webext] Add suport for runtime.openOptionsPage. r=Mossop

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

Comment 4

[Dave Townsend [:mossop]]

Comment on attachment 8723898 [details]
MozReview Request: Bug 1250784: Part 1 - [webext] Add support for options_ui via inline browsers in the Add-on Manager. r?Mossop

https://reviewboard.mozilla.org/r/36783/#review35605

::: toolkit/components/extensions/Extension.jsm:598
(Diff revision 1)
> +   * already been set.

Throw an exception in that case then

::: toolkit/components/extensions/Extension.jsm
(Diff revision 1)
> -// All moz-extension URIs use a machine-specific UUID rather than the

Why is this move necessary? It seems to needlessly throw away blame.

::: toolkit/mozapps/extensions/content/extensions.js:3436
(Diff revision 1)
> +    browser.setAttribute("class", "inline-options-browser");

Looks like this browser is never removed so I'm assuming they stack up with every visit to the details page. Can we just define a single browser in extensions.xul and show/hide it as necessary?

I'd still want to unload whatever page is there when leaving the details view regardless.

::: toolkit/mozapps/extensions/content/extensions.js:3458
(Diff revision 1)
> +                                  parseFloat(style.marginBottom)));

You probasbly want to use getBoundingClientRect() on the document element

::: toolkit/mozapps/extensions/test/browser/browser_inlinesettings_browser.js:21
(Diff revision 1)
> +    AddonManager.getInstallForFile(

Please use installTemporaryAddon or this will fail in beta.

::: toolkit/mozapps/extensions/test/browser/browser_inlinesettings_browser.js:142
(Diff revision 1)
> +});

Verify that the options browser has navigated away/is gone from the document.

Go to an add-on with no options and verify the options browser is hidden/doesn't exist.

Re-open the detail view for this add-on and verify the options browser is back again.

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 8723899 [details]
MozReview Request: Bug 1250784: Part 2 - [webext] Fix some issues that cause noisy tests. r=Mossop

https://reviewboard.mozilla.org/r/36785/#review35611

::: browser/base/content/browser.js:4634
(Diff revision 1)
> +  _startedLoadTimer: new WeakSet(),

Thank you so so much!

::: browser/base/content/browser.js:4646
(Diff revision 1)
>            TelemetryStopwatch.finish("FX_PAGE_LOAD_MS", aBrowser);

You're going to have to remove from _startedLoadTimer here and below so later loads of the same browser don't have the same issue.

::: browser/base/content/browser.js:4650
(Diff revision 1)
> +                 this._startedLoadTimer.add(aBrowser)) {

I think you mean this._startedLoadTimer.has here

Comment 6

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/36783/#review35605

> Why is this move necessary? It seems to needlessly throw away blame.

Our ESLint rules prevent referencing a function before it's been defined. The other option is to pre-declare it and then assign to a variable later, which I prefer to avoid doing if I can help it.

> Looks like this browser is never removed so I'm assuming they stack up with every visit to the details page. Can we just define a single browser in extensions.xul and show/hide it as necessary?
> 
> I'd still want to unload whatever page is there when leaving the details view regardless.

It should be removed along with the other inline settings nodes. I can add a test just to be sure.

For the moment, I'd like to avoid re-using browsers for extension pages. I don't entirely trust our current docshell tracking code. I can revisit the possibility when I've had a chance to give it a thorough audit, though.

> You probasbly want to use getBoundingClientRect() on the document element

I don't think that would work. We need to calculate the height of the contents of the element, without overflow. The bounding client rect will give us the current size of the element, but that won't help us set the display height to match the scroll height of the contents.

> Please use installTemporaryAddon or this will fail in beta.

Ah. Quite. I updated the other tests in this patch, but it looks like I missed this one.

Comment 7

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/36785/#review35611

> You're going to have to remove from _startedLoadTimer here and below so later loads of the same browser don't have the same issue.

I can't think of any cases where we'd fail to start the load timer for a browser after we've done it once, but I think you're right, it makes sense to remove it just to be safe.

> I think you mean this._startedLoadTimer.has here

Yeah, I noticed that just after I pushed, but didn't trust mozreview enough to update. I meant to add a comment to the review.

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 8723900 [details]
MozReview Request: Bug 1250784: Part 3 - [webext] Add suport for runtime.openOptionsPage. r=Mossop

https://reviewboard.mozilla.org/r/36787/#review35617

::: browser/base/content/browser.js:6292
(Diff revision 1)
> +        resolve();

I think it would make sense to have this resolve to the extensions.xul window.

::: browser/base/content/browser.js:6297
(Diff revision 1)
> -  var newLoad = !switchToTabHavingURI("about:addons", true);
> +    var newLoad = !switchToTabHavingURI("about:addons", true);

While you're here remove this unused variable please.

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8723898 [details]
MozReview Request: Bug 1250784: Part 1 - [webext] Add support for options_ui via inline browsers in the Add-on Manager. r?Mossop

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

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8723899 [details]
MozReview Request: Bug 1250784: Part 2 - [webext] Fix some issues that cause noisy tests. r=Mossop

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

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8723900 [details]
MozReview Request: Bug 1250784: Part 3 - [webext] Add suport for runtime.openOptionsPage. r=Mossop

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

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 8723898 [details]
MozReview Request: Bug 1250784: Part 1 - [webext] Add support for options_ui via inline browsers in the Add-on Manager. r?Mossop

https://reviewboard.mozilla.org/r/36783/#review35921

Quick tip, it's helpful to me if you go to the issues from the previous review and mark them as fixed/dropped as appropriate when posting an updated patch so I know what I should expect to see.

Comment 13

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/36783/#review35921

Sorry, I still haven't quite managed to get used to mozreview. I've just about gotten used to replying here rather than in Bugzilla. I guess remembering to mark issues fixed is the next hurdle.

Comment 14

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/c058cdf475473dd25d00738727fb45c2d1af0999
Bug 1250784: Part 1 - [webext] Add support for options_ui via inline browsers in the Add-on Manager. r=Mossop

https://hg.mozilla.org/integration/fx-team/rev/667f52a7694dca2a330965ba89a1bd13fe1cc0c1
Bug 1250784: Part 2 - [webext] Fix some issues that cause noisy tests. r=Mossop

https://hg.mozilla.org/integration/fx-team/rev/78a3194d7fac22cc9878f0e496eb1d82cede73ec
Bug 1250784: Part 3 - [webext] Add suport for runtime.openOptionsPage. r=Mossop

Comment 15

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/5105e61dec9f879b87b374c4dd11981c9c18b7f3
Bug 1250784: Follow-up: Fix merge conflict in bookmarks API. r=me

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c058cdf47547
https://hg.mozilla.org/mozilla-central/rev/667f52a7694d
https://hg.mozilla.org/mozilla-central/rev/78a3194d7fac
https://hg.mozilla.org/mozilla-central/rev/5105e61dec9f

Comment 17

[Will Bamberg [:wbamberg]]

Is it correct that we don't do anything with chrome_style?

Comment 18

[Will Bamberg [:wbamberg]]

=(In reply to Will Bamberg [:wbamberg] from comment #17)
> Is it correct that we don't do anything with chrome_style?

"not yet"

Comment 19

[Will Bamberg [:wbamberg]]

Some docs:

* a bit on this here: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Anatomy_of_a_WebExtension#Options_pages
* and a page on the key: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/options_ui

Let me know if we need anything else/different.

I have an example add-on, but would like to wait until bug 1256282 is fixed before publishing it.

Comment 20

[Kris Maglione [:kmag]]

Looks good to me.

The only issue I have is with "chrome_style", which, when we add support, will probably be called "browser_style", and will probably default to true.

Comment 21

[Will Bamberg [:wbamberg]]

(In reply to Kris Maglione [:kmag] from comment #20)
> Looks good to me.
> 
> The only issue I have is with "chrome_style", which, when we add support,
> will probably be called "browser_style", and will probably default to true.

OK, so I think that if we're never planning to support "chrome_style" we should:

(1) remove "chrome_style" from the table
(2) note that we don't support "chrome_style" in the "Chrome incompatibilities" page
(3) add a row for "browser_style" once we support it.

So that's what I've done, and I'll mark this bug dev-doc-complete.

Comment 22

[Blake Winton (:bwinton) (:☕️)]

(In reply to Will Bamberg [:wbamberg] from comment #18)
> > Is it correct that we don't do anything with chrome_style?
> "not yet"

Is there a bug for implementing "chrome_style"?  (I just saw the screenshots in Andy's blog post, and the serif font in the middle of the page looks pretty jarring…  ;)

Comment 23

[Kris Maglione [:kmag]]

Nope. Feel free to file one.

Comment 24

[:Gijs (he/him)]

Is it intentional that you can't use inline event handler attributes in the options page? If so, can someone update https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Implement_a_settings_page and https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/options_ui to that effect?

Comment 25

[Tomislav Jovanovic :zombie]

Yes, web extension pages have a default Content Security Policy [1] that prohibits inline javascript.  It's applied to all extension's pages (by default), and documented in the Options pages page [2], linked from the option_ui page.

1) https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Content_Security_Policy
2) https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Options_pages