Closed
Bug 1250784
Opened 8 years ago
Closed 8 years ago
Implement the options V2 API
Categories
(WebExtensions :: Untriaged, defect)
WebExtensions
Untriaged
Tracking
(firefox48 fixed)
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: kmag, Assigned: kmag)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, Whiteboard: [options])
Attachments
(3 files)
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+
Assignee | ||
Updated•8 years ago
|
Iteration: 47.2 - Feb 22 → 47.3 - Mar 7
Updated•8 years ago
|
No longer blocks: webext-port-reddit-enhancement-suite
Assignee | ||
Comment 1•8 years ago
|
||
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)
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
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 4•8 years ago
|
||
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 5•8 years ago
|
||
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+
Assignee | ||
Comment 6•8 years ago
|
||
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.
Assignee | ||
Comment 7•8 years ago
|
||
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.
Updated•8 years ago
|
Attachment #8723900 -
Flags: review?(dtownsend) → review+
Comment 8•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Attachment #8723898 -
Flags: review?(dtownsend)
Assignee | ||
Comment 9•8 years ago
|
||
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/
Assignee | ||
Comment 10•8 years ago
|
||
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
Assignee | ||
Comment 11•8 years ago
|
||
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 12•8 years ago
|
||
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+
Assignee | ||
Comment 13•8 years ago
|
||
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.
Assignee | ||
Comment 14•8 years ago
|
||
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
Assignee | ||
Comment 15•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/5105e61dec9f879b87b374c4dd11981c9c18b7f3 Bug 1250784: Follow-up: Fix merge conflict in bookmarks API. r=me
Comment 16•8 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
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Updated•8 years ago
|
Keywords: dev-doc-needed
Comment 17•8 years ago
|
||
Is it correct that we don't do anything with chrome_style?
Flags: needinfo?(kmaglione+bmo)
Comment 18•8 years ago
|
||
=(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)
Comment 19•8 years ago
|
||
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)
Assignee | ||
Comment 20•8 years ago
|
||
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)
Comment 21•8 years ago
|
||
(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
Comment 22•8 years ago
|
||
(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)
Comment 24•7 years 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)
Comment 25•7 years ago
|
||
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•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•