Closed
Bug 1359090
Opened 8 years ago
Closed 8 years ago
Load browser-level stylesheets from DevTools dynamically
Categories
(DevTools :: Framework, enhancement, P3)
DevTools
Framework
Tracking
(firefox55 fixed)
RESOLVED
FIXED
Firefox 55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: jdescottes, Assigned: jdescottes)
References
Details
Attachments
(2 files)
As of now, 3 devtools stylesheets are loaded in the browser XUL document Two are included in browser.css using a %include: - %include ../../../devtools/client/themes/responsivedesign.inc.css - %include ../../../devtools/client/themes/commandline.inc.css (http://searchfox.org/mozilla-central/rev/313e5199bf58200f158c6fcbe193e41b88ed58a6/browser/themes/osx/browser.css#3106-3107) One is loaded directly in the document as a separated stylesheet: - <?xml-stylesheet href="chrome://devtools/skin/devtools-browser.css" type="text/css"?> (http://searchfox.org/mozilla-central/rev/313e5199bf58200f158c6fcbe193e41b88ed58a6/browser/base/content/browser.xul#12) As we are removing DevTools from mozilla-central, these stylesheets should dynamically be loaded by DevTools on startup.
Comment 1•8 years ago
|
||
Triaging dt-addon blockers as P3/enhancement since we're not using priorities for this project (filter on CLIMBING SHOES).
Severity: normal → enhancement
Priority: -- → P3
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•8 years ago
|
||
mozreview-review |
Comment on attachment 8862206 [details] Bug 1359090 - extract appendStyleSheet util from theme-switching to dedicated util; https://reviewboard.mozilla.org/r/134186/#review137288 ::: devtools/client/shared/stylesheet-utils.js:9 (Diff revision 2) > + > +/* eslint-env browser */ > +"use strict"; > + > +/* > + * Append a new processing instruction and return an object with I would add a note about this being specific to XUL document. That's the case, right? Or does these weird xml stylesheet instruction also works for html? ::: devtools/client/shared/stylesheet-utils.js:10 (Diff revision 2) > +/* eslint-env browser */ > +"use strict"; > + > +/* > + * Append a new processing instruction and return an object with > + * - styleSheet: DOMNode s/DOMNode/XMLStylesheetProcessingInstruction/ ::: devtools/client/shared/stylesheet-utils.js:13 (Diff revision 2) > +/* > + * Append a new processing instruction and return an object with > + * - styleSheet: DOMNode > + * - loadPromise: Promise that resolves once the sheets loads or errors > + */ > +function appendStyleSheet(url, document) { super-nit: (document, url) would look like a more natural argument order.
Attachment #8862206 -
Flags: review?(poirot.alex) → review+
Comment 7•8 years ago
|
||
mozreview-review |
Comment on attachment 8862207 [details] Bug 1359090 - load devtools-browser.css dynamically when starting devtools; https://reviewboard.mozilla.org/r/134188/#review137268 ::: devtools/client/framework/devtools-browser.js:536 (Diff revision 2) > gDevToolsBrowser._trackedBrowserWindows.add(win); > > BrowserMenus.addMenus(win.document); > > + // Add the devtools-browser stylesheet to browser window's document. > + appendStyleSheet("chrome://devtools/skin/devtools-browser.css", win.document); In bug 1359855, devtools is already reported to be too time consuming on Firefox startup. I'm concerned about doing that here. Do you think we could possibly insert the stylesheets lazily? I imagine it would be especially interesting to lazify commandline/responsivedesign ones. Having said that, I looked into profiler results and appendStyleSheet barely appears in the profile: https://perfht.ml/2qiy9wu But this would only highlight the synchronous cost of appendStyleSheet. It is harder to see any asynchronous cost of it making browser.xul document slower to refresh. Finally, we should try to remove these stylesheet elements on _forgetBrowserWindow to better support live reload of the addon via Ctrl+Alt+R. ::: devtools/client/jar.mn:110 (Diff revision 2) > content/responsive.html/index.xhtml (responsive.html/index.xhtml) > content/responsive.html/index.js (responsive.html/index.js) > content/dom/dom.html (dom/dom.html) > content/dom/main.js (dom/main.js) > % skin devtools classic/1.0 %skin/ > - skin/devtools-browser.css (themes/devtools-browser.css) > +* skin/devtools-browser.css (themes/devtools-browser.css) The addon won't support any pre-processing, unless we implement one. Given that we have almost no preprocessing needs, It would be better to not introduce one. You could use @import in devtools-browser.css, or inject the three stylesheets separately.
Attachment #8862207 -
Flags: review?(poirot.alex)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8862207 [details] Bug 1359090 - load devtools-browser.css dynamically when starting devtools; https://reviewboard.mozilla.org/r/134188/#review137374 Looks good, thanks! Be sure to fix the error in responsivedesign.jsm and have a green light from try. (do not hesitate to push to the three platforms) ::: devtools/client/framework/devtools-browser.js:507 (Diff revision 3) > + loadBrowserStyleSheet: function (win) { > + if (this._browserStyleSheets.has(win)) { > + return Promise.resolve(); > + } > + > + // Add the devtools-browser stylesheet to browser window's document. This comment would be better before loadBrowserStyleSheet as a function description. ::: devtools/client/framework/toolbox-hosts.js:60 (Diff revision 3) > * Create a box at the bottom of the host tab. > */ > create: function () { > let deferred = defer(); > > + gDevToolsBrowser.loadBrowserStyleSheet(this.hostTab.ownerGlobal).then(() => { Could you use async/await to prevent have to shift all the code? async create() { .. await gDevToolsBrowser.loadBrowserStyleSheet(..); ::: devtools/client/framework/toolbox-hosts.js:208 (Diff revision 3) > * Create a box in the sidebar of the host tab. > */ > create: function () { > let deferred = defer(); > > + gDevToolsBrowser.loadBrowserStyleSheet(this.hostTab.ownerGlobal).then(() => { Same. ::: devtools/client/responsivedesign/responsivedesign.jsm:232 (Diff revision 3) > debug("INIT BEGINS"); > let ready = this.waitForMessage("ResponsiveMode:ChildScriptReady"); > this.mm.loadFrameScript("resource://devtools/client/responsivedesign/responsivedesign-child.js", true); > yield ready; > > + yield gDevToolsBrowser.loadBrowserStyleSheet(this.chromeWindow); s/this.chromeWindow/this.mainWindow/ ::: devtools/client/themes/devtools-browser.css (Diff revision 3) > > -/* Bottom-docked toolbox minimize transition */ > -.devtools-toolbox-bottom-iframe { > - transition: margin-bottom .1s; > -} > - Why do you remove that? This feature is disabled but still in the codebase: http://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox-hosts.js#124
Attachment #8862207 -
Flags: review?(poirot.alex) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•8 years ago
|
||
Thanks for the review ! Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=82d30e950dddba8085300e60e41bf20f3c320bad
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jdescottes
Assignee | ||
Comment 13•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8862207 [details] Bug 1359090 - load devtools-browser.css dynamically when starting devtools; https://reviewboard.mozilla.org/r/134188/#review137374 > Could you use async/await to prevent have to shift all the code? > async create() { > .. > await gDevToolsBrowser.loadBrowserStyleSheet(..); Done. I initially decided against it in order to preserve consistency in the file but I'm fine either way. > Why do you remove that? > This feature is disabled but still in the codebase: > http://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox-hosts.js#124 I thought it was a leftover. I remembered a conversation on IRC where we said this was no longer available, but maybe I misunderstood. I will check how/when we trigger this code path.
Assignee | ||
Comment 14•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8862206 [details] Bug 1359090 - extract appendStyleSheet util from theme-switching to dedicated util; https://reviewboard.mozilla.org/r/134186/#review137288 > I would add a note about this being specific to XUL document. > That's the case, right? Or does these weird xml stylesheet instruction also works for html? From what I can tell, even though createProcessingInstruction is supported on HTML documents, adding an xml-stylesheet processing instruction doesn't do anything. Updated the comment as suggested.
Comment 15•8 years ago
|
||
(In reply to Julian Descottes [:jdescottes] from comment #13) > > Why do you remove that? > > This feature is disabled but still in the codebase: > > http://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox-hosts.js#124 > > I thought it was a leftover. I remembered a conversation on IRC where we > said this was no longer available, but maybe I misunderstood. > > I will check how/when we trigger this code path. There is only a key shortcut: CmdOrCtrl+Shift+U The button to toggle this mode is hidden, see bug 1177463.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•8 years ago
|
||
Indeed thanks! Forgot to add the html namespace to responsivedesign.css. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2454fb5c48473eff311bce3399fb0a9ac26009a0
Comment 18•8 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3d382689c523 extract appendStyleSheet util from theme-switching to dedicated util;r=ochameau https://hg.mozilla.org/integration/autoland/rev/2bf614540982 load devtools-browser.css dynamically when starting devtools;r=ochameau
Comment 19•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3d382689c523 https://hg.mozilla.org/mozilla-central/rev/2bf614540982
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•