If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Implement the options V2 API

RESOLVED FIXED in Firefox 48

Status

()

Toolkit
WebExtensions: Untriaged
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla48
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [options])

MozReview Requests

()

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

Attachments

(3 attachments)

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

https://developer.chrome.com/extensions/optionsV2
https://developer.chrome.com/extensions/runtime#method-openOptionsPage
Flags: blocking-webextensions+
Iteration: 47.2 - Feb 22 → 47.3 - Mar 7

Updated

2 years ago
No longer blocks: 1208765
Blocks: 1213990
Created 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 commit: https://reviewboard.mozilla.org/r/36783/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36783/
Attachment #8723898 - Flags: review?(dtownsend)
Created attachment 8723899 [details]
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/
Attachment #8723899 - Flags: review?(dtownsend)
Created attachment 8723900 [details]
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/
Attachment #8723900 - Flags: review?(dtownsend)
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.
Attachment #8723898 - Flags: review?(dtownsend)
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
Attachment #8723899 - Flags: review?(dtownsend) → review+
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.
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.
Attachment #8723900 - Flags: review?(dtownsend) → review+
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.
Attachment #8723898 - Flags: review?(dtownsend)
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 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/
Attachment #8723899 - Attachment description: MozReview Request: Bug 1250784: Part 2 - [webext] Fix some issues that cause noisy tests. r?Mossop → MozReview Request: Bug 1250784: Part 2 - [webext] Fix some issues that cause noisy tests. r=Mossop
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/
Attachment #8723900 - Attachment description: MozReview Request: Bug 1250784: Part 3 - [webext] Add suport for runtime.openOptionsPage. r?Mossop → MozReview Request: Bug 1250784: Part 3 - [webext] Add suport for runtime.openOptionsPage. r=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.
Attachment #8723898 - Flags: review?(dtownsend) → review+
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.
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
https://hg.mozilla.org/integration/fx-team/rev/5105e61dec9f879b87b374c4dd11981c9c18b7f3
Bug 1250784: Follow-up: Fix merge conflict in bookmarks API. r=me

Comment 16

2 years ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Keywords: dev-doc-needed
Is it correct that we don't do anything with chrome_style?
Flags: needinfo?(kmaglione+bmo)
=(In reply to Will Bamberg [:wbamberg] from comment #17)
> Is it correct that we don't do anything with chrome_style?

"not yet"
Flags: needinfo?(kmaglione+bmo)
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.
Flags: needinfo?(kmaglione+bmo)
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.
Flags: needinfo?(kmaglione+bmo)
(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.
Keywords: dev-doc-needed → dev-doc-complete
(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…  ;)
Flags: needinfo?(kmaglione+bmo)
Nope. Feel free to file one.
Flags: needinfo?(kmaglione+bmo)
Depends on: 1275287

Updated

8 months ago
Depends on: 1334712

Comment 24

6 months ago
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?
Flags: needinfo?(kmaglione+bmo)
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
Flags: needinfo?(kmaglione+bmo)

Updated

5 months ago
Blocks: 1233004
You need to log in before you can comment on or make changes to this bug.