Closed Bug 1359090 Opened 8 years ago Closed 8 years ago

Load browser-level stylesheets from DevTools dynamically

Categories

(DevTools :: Framework, enhancement, P3)

Product:
DevTools
Component:
Framework
Type:
enhancement

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

Attachments

(2 files)

Bug 1359090 - extract appendStyleSheet util from theme-switching to dedicated util;
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1359090 - load devtools-browser.css dynamically when starting devtools;
59 bytes, text/x-review-board-request
ochameau
: review+
Details 
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.
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 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 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 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+
Thanks for the review ! Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=82d30e950dddba8085300e60e41bf20f3c320bad
Assignee: nobody → jdescottes
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.
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.
(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.
Indeed thanks! 

Forgot to add the html namespace to responsivedesign.css.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2454fb5c48473eff311bce3399fb0a9ac26009a0
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
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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: