Closed Bug 1311541 Opened 8 years ago Closed 6 years ago

Ship all devtools CSS as resource:// URIs

Categories

(DevTools :: General, defect, P3)

Product:
DevTools
Component:
General
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED INVALID

People

(Reporter: ntim, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat, Whiteboard: [reserve-html])

Attachments

(3 files, 2 obsolete files)

Bug 1311541 - load devtools CSS and images as resources;
58 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1311541 - load common.css in all devtools documents;
58 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1311541 - use resource:// urls for devtools css in addon-sdk;
58 bytes, text/x-review-board-request
jryans
: review+
Details 
We inconsistently use chrome:// and resource:// for CSS, we should stick to one protocol. resource:// is preferred, since we're aiming to run devtools as a webpage, and CSS also doesn't need chrome privileges.
Thanks for logging this Tim! 
I'm all in favor of doing this across all devtools, I will block Bug 1310337 on this one.
Blocks: 1310337
Blocks: devtools-html-phase2
Flags: qe-verify?
Priority: P3 → P2
Whiteboard: [devtools-html]
Copied from bug 1315679.

(In reply to Julian Descottes [:jdescottes] from comment #1)
> (In reply to Julian Descottes [:jdescottes] from comment #0)
> > 
> > As I understand from the comments in Bug 1311178, this means we should make
> > sure all our stylesheets are always requested as resource:// urls in the
> > first place.
> 
> jryans, just want to make sure I'm not missing something before I start
> working on this, so let me know what you think.
> I will probably clean up the loading of theme files first to unblock the
> devtools-html bugs.

Correct, we should ship all CSS and images as resource:// within Firefox.  The absolute URLs you found should be changed back to relative so that the CSS will load over https:// as well.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Priority: P2 → P1
Iteration: --- → 52.3 - Nov 14
Blocks: 1291049
No longer blocks: 1310337
Florian: asking you because you recently modified browser_parsable_css.js in order to inspect as many CSS files as possibles. With this bug we aim to load all devtools CSS files as resource:// and therefore they will no longer be available from chrome:// urls. 

This makes browser_parsable_css.js fail as it excepts some whitelisted image references to be found in devtools.css. We can remove the whitelist entries but I'd also like to have your opinion on what we should do here. Should we modify the test in order to consider CSS files loaded as resource:// as well?
Flags: needinfo?(florian)
Flags: qe-verify? → qe-verify+
QA Contact: petruta.rasa
For QA verification: this bug will impact the way we load our CSS and image files. 

We should make sure all devtools panels still look the same, that images are not missing. I will be testing this manually as well, but having an extra external check would be nice!
(In reply to Julian Descottes [:jdescottes] from comment #8)

> This makes browser_parsable_css.js fail as it excepts some whitelisted image
> references to be found in devtools.css. We can remove the whitelist entries
> but I'd also like to have your opinion on what we should do here. Should we
> modify the test in order to consider CSS files loaded as resource:// as well?

Just removing the whitelist entries is fine for this bug. It would indeed be nice to cover files in resource:// with this test or a similar one, but I would suggest doing it in a separate bug as you may have surprises from outside of devtools when you start to check something that wasn't checked yet.
Flags: needinfo?(florian)
It looks like moving from chrome:// to resource:// is introducing a performance regression (~3% on Talos damp tests [1]). This also seems to push some tests into timeout territory (https://treeherder.mozilla.org/#/jobs?repo=try&revision=947e5893c7d28a181fe2651e87cea9dbb73b5d86)

Ryan: by any chance, do you know anything that could explain the performance impact we are seeing here? Are resource:// urls known to be slower to fetch than chrome:// urls ?

[1] https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=6c9ecb3e9c33adb239cdb9a6f5ebee765561941e&newProject=try&newRevision=13e07371ff03a4c7f348df3d498d58adcc244237&framework=1&filter=damp&showOnlyImportant=0
Flags: needinfo?(jryans)
Just a quick update of my investigations on this topic. It looks like we have two intermittents related to this change:
- browser_console_addonsdk_loader_exception.js
- browser_console_consolejsm_output.js

They fail on various platforms with a timeout. When they succeed, they are not typically close to the timeout threshold so no point in requesting longer timeout here. 

One of them, browser_console_addonsdk_loader_exception, is timing out right after triggering an executeSoon. This looks already racy, so if using resource:// urls slows down the test a bit it could explain the failure. 

For the other one, for now I have no idea what's causing it to fail. 

While investigating this I realized that there were a lot of "invalid variable" logs in the test:

> JavaScript Warning: "Property contained reference to invalid variable. [...]

which did not occur when using chrome urls:// . Turning on CSS warnings in the browser console I see a few similar logs when using devtools with my patch. This seems strange, as the mentioned variables are properly applied when inspecting the impacted elements.

Maybe this additional logging explains part of the slowdown seen on Talos.
Maybe one of the resource URIs containing the specific variables is not loading.  What's the variable?
For browser_console_consolejsm_output.js we have:

--devtools-splitter-bottom-width
--icon-filter
--theme-body-background
--theme-command-line-image
--theme-splitter-color
--theme-tab-toolbar-background
--theme-toolbar-background
--theme-tooltip-shadow
--toolbar-button-border-color

(https://treeherder.mozilla.org/logviewer.html#?job_id=30892063&repo=try#L3643)

That means variables from variables.css, from splitter.css, from toolbar.css. As I said, despite the warnings, the variables are processed correctly and the properties have the correct values.

FWIW, I tried moving all our variables in variables.css and importing this file in all other stylesheets that use variables, it seems to remove the warnings.

I also did a try run using local workarounds to ensure we had no "invalid variable" warnings in browser_console_consolejsm_output.js and it looks like the test no longer fails: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac260b431ea18339218ce068bb9499c8b6259e7f&selectedJob=30909384 (see dt7 on OSX opt M-e10s)
(In reply to Julian Descottes [:jdescottes] from comment #16)
> It looks like moving from chrome:// to resource:// is introducing a
> performance regression (~3% on Talos damp tests [1]). This also seems to
> push some tests into timeout territory
> (https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=947e5893c7d28a181fe2651e87cea9dbb73b5d86)
> 
> Ryan: by any chance, do you know anything that could explain the performance
> impact we are seeing here? Are resource:// urls known to be slower to fetch
> than chrome:// urls ?
> 
> [1]
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=6c9ecb3e9c33adb239cdb9a6f5ebee76
> 5561941e&newProject=try&newRevision=13e07371ff03a4c7f348df3d498d58adcc244237&
> framework=1&filter=damp&showOnlyImportant=0

I am not aware of anything that would make resource:// slower than chrome://...  I don't think the startup caching is different between them...  Maybe :ochameau knows some secret about this.

I'd say focus on the errors in those tests for the moment, as solving the root cause of those may also impact the perf issues.
Flags: needinfo?(jryans) → needinfo?(poirot.alex)
I don't know any performance difference between both. But I wouldn't be surprised if there is some...

That shouldn't be related to the startup cache as it seems to be only used for XBL, XUL and ScriptLoader. Unless XUL usage of the startup cache depends on CSS being loaded from chrome in the startup cache codepath...

Otherwise I don't see any particular optimization in the chrome:// protocol handler:
http://searchfox.org/mozilla-central/source/chrome/nsChromeProtocolHandler.cpp#138-151

Did you tried looking at CSS and images patches independently? I imagine if that really comes from the protocol change you would have a similar regression in both patches. Otherwise, if only the CSS patch regress, there might be something in your patch...
Flags: needinfo?(poirot.alex)
Iteration: 52.3 - Nov 14 → 53.1 - Nov 28
Thanks for the suggestions all! 

As I mentioned in a previous comment, the test failures go away when I add workarounds to avoid having any warning logged. 

During standup we discussed trying to load the variables.css stylesheet directly in the document instead of in an @import of the theme file. 
I did that on the following try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e5b638e994d8c08e21efe21299e05b67cb0345f . No errors so far. 

Will trigger talos jobs as well now.
Blocks: 1313768
As discussed in standup I tried moving theme-switching to be loaded earlier, but the warnings are still logged (btw, could not use link tags in XUL file, so I tried with the inspector, just to know if that worth investigating).

We could go ahead with the proposal to load light-theme.css in each document, but it will still look hacky to someone who hasn't been involved in this bug. 

> We load light-theme.css first because we need the variables imported inside light-theme.css 
> in the other stylesheets loaded by the document.

The root cause of the issue is that we load some CSS files using link/xml-stylesheet tags, but we load the theme file separately while it happens to contain variables needed by potentially any stylesheet.

I was thinking about some options to fix this:
  1. we could merge all the theme files in a common devtools.css file and load it in every document. We would have to update the selectors in light/dark-theme.css to be scoped under :root.theme-light/dark (maybe impossible for some selectors?). We could still have theme switching load additional CSS files (as we do for floating-scrollbars-dark-theme.css).

  2. we create a devtools.css which simply @import variables/common/toolbars/tooltips/splitters.css (or we repurpose common.css to do that). This generic/common CSS file is loaded in every document, and light/dark/firebug-theme.css only define the theme specific selectors that are needed. I think it's easier to explain that all documents need to load "devtools.css" rather than "light-theme.css"

Brian, let me know what you think!
Flags: needinfo?(bgrinstead)
(In reply to Julian Descottes [:jdescottes] from comment #29)
> As discussed in standup I tried moving theme-switching to be loaded earlier,
> but the warnings are still logged (btw, could not use link tags in XUL file,
> so I tried with the inspector, just to know if that worth investigating).
> 
> We could go ahead with the proposal to load light-theme.css in each
> document, but it will still look hacky to someone who hasn't been involved
> in this bug. 
> 
> > We load light-theme.css first because we need the variables imported inside light-theme.css 
> > in the other stylesheets loaded by the document.
> 
> The root cause of the issue is that we load some CSS files using
> link/xml-stylesheet tags, but we load the theme file separately while it
> happens to contain variables needed by potentially any stylesheet.
> 
> I was thinking about some options to fix this:
>   1. we could merge all the theme files in a common devtools.css file and
> load it in every document. We would have to update the selectors in
> light/dark-theme.css to be scoped under :root.theme-light/dark (maybe
> impossible for some selectors?). We could still have theme switching load
> additional CSS files (as we do for floating-scrollbars-dark-theme.css).
> 
>   2. we create a devtools.css which simply @import
> variables/common/toolbars/tooltips/splitters.css (or we repurpose common.css
> to do that). This generic/common CSS file is loaded in every document, and
> light/dark/firebug-theme.css only define the theme specific selectors that
> are needed. I think it's easier to explain that all documents need to load
> "devtools.css" rather than "light-theme.css"
> 
> Brian, let me know what you think!

I like (2), with repurposing common.css since all documents already have a reference to common.css already (although we may need to reorder it to make sure it's always loaded at the top.
Flags: needinfo?(bgrinstead)
Attachment #8809009 - Attachment is obsolete: true
Attachment #8811000 - Attachment is obsolete: true
(In reply to Brian Grinstead [:bgrins] from comment #30)
> 
> I like (2), with repurposing common.css since all documents already have a
> reference to common.css already (although we may need to reorder it to make
> sure it's always loaded at the top.

Submitted for review (try https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f777d2f250bf5b227ba77c6efa5176d835f3e2a)
However common.css is not referenced at the moment in devtools' documents, unless I'm missing something?
(In reply to Julian Descottes [:jdescottes] from comment #33)
> (In reply to Brian Grinstead [:bgrins] from comment #30)
> > 
> > I like (2), with repurposing common.css since all documents already have a
> > reference to common.css already (although we may need to reorder it to make
> > sure it's always loaded at the top.
> 
> Submitted for review (try
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=9f777d2f250bf5b227ba77c6efa5176d835f3e2a)
> However common.css is not referenced at the moment in devtools' documents,
> unless I'm missing something?

You are right, I was scanning and saw it referenced in a few docs but none were in the toolbox
As discussed during standup, trying to load common.css in light-theme / dark-theme on top of the one loaded from the documents, to ensure backward compatibility for addons.

talos: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=3f26eb87adbebc644948abf6ab740f6cb7d3b47c&newProject=try&newRevision=5b2cb9752e1ef7458f5c738bff22553c616d15f7&framework=1&filter=damp&showOnlyImportant=0

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f95f13c2054bf81ddfb5b71165398274a5ab1e82

It will be interesting to see what comes up on try and talos, because locally the invalid variable warnings come back when using this approach. So we might have to go for the 2rd approach we discussed, which is to have a single entry point for our shared stylesheets, load it in the documents, and modify theme-switching.js in order to load this stylesheet if and only if it is not already loaded by the document.
(In reply to Julian Descottes [:jdescottes] from comment #37)
> As discussed during standup, trying to load common.css in light-theme /
> dark-theme on top of the one loaded from the documents, to ensure backward
> compatibility for addons.
> 
> talos:
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=3f26eb87adbebc644948abf6ab740f6c
> b7d3b47c&newProject=try&newRevision=5b2cb9752e1ef7458f5c738bff22553c616d15f7&
> framework=1&filter=damp&showOnlyImportant=0
> 
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=f95f13c2054bf81ddfb5b71165398274a5ab1e82
> 
> It will be interesting to see what comes up on try and talos, because
> locally the invalid variable warnings come back when using this approach. So
> we might have to go for the 2rd approach we discussed, which is to have a
> single entry point for our shared stylesheets, load it in the documents, and
> modify theme-switching.js in order to load this stylesheet if and only if it
> is not already loaded by the document.

Hm, why would the variable warnings come back in this case?  For future reference, the 3rd approach we discussed:

1. common.css is loaded in all docs
2. common.css @imports light-theme.css, dark-theme.css, and firebug-theme.css
3. those 3 files make all of their rules prefixed with the class name (ideally in follow-ups we would also de-dupe code, break things out into variables, and eventually be able to delete these files)
4. theme switching is now in charge of loading common.css initially if it isn't there
5. theme switching can drop the responsibility of adding / removing theme CSS files.

One downside: theme definitions become less powerful, since you can't specify arbitrary CSS files to load.  If we wanted to keep things generally the way they are (not sure we do), then we could keep things similar to your posted patches, except add in part (4) from above.
Yep sorry I meant 3rd not `2rd`, thanks for describing it in details.

> Hm, why would the variable warnings come back in this case?

I was also puzzled by this, but I realized I didn't change the selectors defined in variables.css (and toolbars/splitters.css) to allow variables to be applied when no theme class is applied on the document element. I should have changed the `:root.theme-light` to `:root, :root.theme-light`. 

So I figure that we got the warnings with this patch because theme-switching.js is waiting for stylesheets to be loaded before changing the classname. And since we import common.css in the theme files, the load took more time and the first common.css (loaded from the document) must have started to apply before the theme-light class was added.

So I modified yesterday's patch to allow variables to be used when no theme class is applied (https://treeherder.mozilla.org/#/jobs?repo=try&revision=4081c8ffb5cbb91b525c68ed9e3d3b8975a31221).

(FYI, I also tried to set the theme class earlier if no previous theme was applied, without changing anything to common.css or light-theme.css. Unsurprisingly it gave the same results as my earlier attempts to add the ":root" selectors: only some of the warnings go away.)
OK, what do you think - should we try to proceed with it as-is change then, or go ahead and also modify theme-switching to load common.css if it's not already there to avoid the double import?
Comment on attachment 8808991 [details]
Bug 1311541 - load devtools CSS and images as resources;

https://reviewboard.mozilla.org/r/91686/#review95042
Attachment #8808991 - Flags: review?(bgrinstead) → review+
Comment on attachment 8808992 [details]
Bug 1311541 - load common.css in all devtools documents;

https://reviewboard.mozilla.org/r/91688/#review94606

Clearing review since we need to either add in the changes you have been working with on talos or something additional (see Comment 41)

::: devtools/client/projecteditor/chrome/content/projecteditor.xul:5
(Diff revision 6)
>  <?xml version="1.0"?>
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<?xml-stylesheet href="resource://devtools/client/themes/common.css" type="text/css"?>

Just a note for project editor + webide + about:debugging.  These were already including common.css so this will be introducing a number of new CSS rules due to common.css importing more things.  We should check to make sure they haven't changed (a quick look doesn't show anything obvious).
Attachment #8808992 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #41)
> OK, what do you think - should we try to proceed with it as-is change then,
> or go ahead and also modify theme-switching to load common.css if it's not
> already there to avoid the double import?

I think we should go ahead with the double import for now. The talos regression is almost identical with [1] or without [2] it.

I will submit the additional changes for review today.

[1] https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=3f26eb87adbebc644948abf6ab740f6cb7d3b47c&newProject=try&newRevision=2643cb117fab6d97164501877f70be1bec449afc&framework=1&filter=damp&showOnlyImportant=0
[2] https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=3f26eb87adbebc644948abf6ab740f6cb7d3b47c&newProject=try&newRevision=7213b7c042179a7963b13a55a6d3fc29d8b10810&framework=1&filter=damp&showOnlyImportant=0
Some add-ons are still referencing chrome css [1]
- geckoprofiler (https://dxr.mozilla.org/addons/source/addons/658622)
  - light-theme.css
  - dark-theme.css
- p0Lorem (https://dxr.mozilla.org/addons/source/addons/670056)
  - common.css (actually references devtools/skin/themes/common.css, which I think is invalid)
- FireEyes II (https://dxr.mozilla.org/addons/source/addons/680081/)
  - widgets.css
- XUL Developer (https://dxr.mozilla.org/addons/source/addons/720863/)
  - common.css
  - webconsole.css
  - light-theme.css
  - dark-theme.css

Addons still referencing chrome:// images [2]
- DevTools Prototyper by ntim (https://dxr.mozilla.org/addons/source/addons/553224)
  - profiler-stopwatch.svg
- CuteMenus Classic Mode (https://dxr.mozilla.org/addons/source/addons/589874)
  - lots of them ...
- User Style Manager (https://dxr.mozilla.org/addons/source/addons/619568)
  - (max version is 46, so probably not updated anymore) 
  - tool-options.svg
- FireEyes II (https://dxr.mozilla.org/addons/source/addons/680081/)
  - itemArrow-dark-ltr.svg
  - itemArrow-ltr.svg
- Fast RPC (https://dxr.mozilla.org/addons/source/addons/682320/)
  - (max version is 48)
  - tool-network.svg

[1] https://dxr.mozilla.org/addons/search?q=regexp%3Achrome%5C%3A%5C%2F%5C%2Fdevtools%5C%2F.*%5C.css%5C%22&redirect=false
[2] https://dxr.mozilla.org/addons/search?q=regexp%3Achrome\%3A\%2F\%2Fdevtools\%2F.*\.(svg|png)\%22&redirect=false
Setting addon-compat due to devtools css and image paths changing from chrome uris to resource uris.  The list of addons using these is in Comment 45.  Here's the fix:

Replace chrome://devtools/skin/ with resource://devtools/client/themes (for example, chrome://devtools/skin/light-theme.css -> resource://devtools/client/themes/light-theme.css).
 
Replace chrome://devtools/content/ with resource://devtools/client (for example, chrome://devtools/content/aboutdebugging/aboutdebugging.css -> resource://devtools/client/aboutdebugging/aboutdebugging.css).
Keywords: addon-compat
In general, I am less worried about add-on compat these days, given the add-on team's 2017 plans[1], which will eventually block non-WebExtensions by Firefox 57.

Anyway, hopefully this bug won't be too bad for add-ons that want to carry on for now.

[1]: https://wiki.mozilla.org/Add-ons/2017
Comment on attachment 8813602 [details]
Bug 1311541 - use resource:// urls for devtools css in addon-sdk;

https://reviewboard.mozilla.org/r/95040/#review95430

Thanks, looks resonable to me!
Attachment #8813602 - Flags: review?(jryans) → review+
Thanks for the review! 
I tried to find a maintainer for each of the addons found in comment 45 and sent a heads-up email.
Iteration: 53.1 - Nov 28 → 53.2 - Dec 12
No longer blocks: 1291049
I don't know why, but there's definitely a regression showing up with the change to resource URIs.  I suppose we could try to not double load common.css and see if that helps perf (we could add some logic in theme-switching to make sure common.css is loaded on the page before loading the theme files if necessary).
(In reply to Brian Grinstead [:bgrins] from comment #56)
> I don't know why, but there's definitely a regression showing up with the
> change to resource URIs.  I suppose we could try to not double load
> common.css and see if that helps perf (we could add some logic in
> theme-switching to make sure common.css is loaded on the page before loading
> the theme files if necessary).

I am getting similar results without the double load. However it looks like the last rebase created regressions in a few spots according to your last try push and I think I should fix that first.
Iteration: 53.2 - Dec 12 → 53.3 - Dec 26
There was a question in #devtools-html regarding whether this was still needed for devtools-html. I quickly said "yes" but I think I should expand a bit.

#1 Stylesheets

At the moment the inspector in a content tab is displayed with acceptable styles because the launchpad (ex devtools-local-toolbox) is including devtools themes stylesheets in its bundle.

So right now, we are loading all the inspector.xhtml (& markup.xhtml) specific stylesheets by rewriting the href attribute of the link tags (removing chrome:// and resource:// basically). We then load light-theme.css from mc, but half the imports fail here because they target absolute resource:// urls.

This means that if you try to update anything in variables.css, common.css, splitters.css, toolbars.css and tooltips.css in mc, it won't have any impact since these stylesheets are actually coming from the launchpad. 

Having only resource:// urls and relative paths for devtools stylesheets means we could load all the stylesheets from mc. We would probably need to find a way to remove the stylesheets coming from the launchpad to avoid conflicts though. 

For the purpose of working on the inspector, I guess the most important is to be able to modify the inspector.xhtml stylesheets so the current situation is acceptable for now. But having consistent stylesheets would be the best in the long run. If someone adds a variable in variables.css and starts using it in inspector.css, the variable would be unavailable until launchpad updates its stylesheets.

#2 Images

Since we don't inline svgs & pngs, stylesheets referencing chrome:// or resource:// image URLs will lead to missing icons in the UI. For example, the add (+) icon is missing for this reason at the moment.

# Summary

This highlights how resource:// URLs would make our life easier in the short term for devtools-html. 

Another alternative is to do something closer to what debugger.html/launchpad are doing, ie. bundle the inspector stylesheets inside the inspector bundle and start inlining images. We would still need to purge our stylesheets from chrome:// resource:// urls, but maybe that can be done by rewriting the files when bundling?
So is it correct that by switching to resource URIs we can begin using relative paths to images inside CSS and we can't do that now?  Or that we can begin using relative paths from the HTML in <link> tags?  Or both?
(In reply to Brian Grinstead [:bgrins] from comment #64)
> So is it correct that by switching to resource URIs we can begin using
> relative paths to images inside CSS and we can't do that now?  Or that we
> can begin using relative paths from the HTML in <link> tags?  Or both?

That's the main benefit here. To be totally accurate, once all CSSs and images ship as resource:// we can:
- use relative urls for images in CSSs
- use relative urls for @import url() in CSSs

However, our xhtml files are still loaded as chrome:// . This means that link tags will still need to use absolute resource:// as href for link tags. But this is something we already workaround by rewriting the href attribute in our local-toolbox.js script.
(In reply to Julian Descottes [:jdescottes] from comment #65)
> (In reply to Brian Grinstead [:bgrins] from comment #64)
> > So is it correct that by switching to resource URIs we can begin using
> > relative paths to images inside CSS and we can't do that now?  Or that we
> > can begin using relative paths from the HTML in <link> tags?  Or both?
> 
> That's the main benefit here. To be totally accurate, once all CSSs and
> images ship as resource:// we can:
> - use relative urls for images in CSSs
> - use relative urls for @import url() in CSSs
> 
> However, our xhtml files are still loaded as chrome:// . This means that
> link tags will still need to use absolute resource:// as href for link tags.
> But this is something we already workaround by rewriting the href attribute
> in our local-toolbox.js script.

OK, in that case this is a really big benefit.  If there's no more perf regression as in Comment 58 then let's get this landed
Comment on attachment 8808992 [details]
Bug 1311541 - load common.css in all devtools documents;

https://reviewboard.mozilla.org/r/91688/#review99170
Attachment #8808992 - Flags: review?(bgrinstead) → review+
(In reply to (Unavailable until Jan 4) Brian Grinstead [:bgrins] from comment #66)
> OK, in that case this is a really big benefit.  If there's no more perf
> regression as in Comment 58 then let's get this landed

I think my comment was confusing: the try & talos tests you started in Comment 55 highlighted a plain regression which was making tests fail and introducing a 4-5% perf regression on Talos.

With the regression fixed, tests are passing but we still have a 2-3% perf regression. Should we land as is or keep investigating ?
Flags: needinfo?(bgrinstead)
talos isn't that helpful. Is that only DAMP regression or do you see something else?

Did you tried the profiler? (I tried to apply these patches, but it fails on recent master)
https://new.cleopatra.io/

If that's easy for you, could you share profiles without/with the patch?
(Install the addon, launch firefox, stop the profiler, wait 5s, start the profiler, open the toolbox on the inspector, wait for end of inspector load, press Ctrl+Shift+2, click on permalink and share)
(In reply to Alexandre Poirot [:ochameau] from comment #69)
> talos isn't that helpful. Is that only DAMP regression or do you see
> something else?
> 
> Did you tried the profiler? (I tried to apply these patches, but it fails on
> recent master)
> https://new.cleopatra.io/
> 
> If that's easy for you, could you share profiles without/with the patch?
> (Install the addon, launch firefox, stop the profiler, wait 5s, start the
> profiler, open the toolbox on the inspector, wait for end of inspector load,
> press Ctrl+Shift+2, click on permalink and share)

Thanks for the info Alex!

(just uploaded my patches rebased on a recent master, in case anyone wants to try)

Here are the two profiles:
- with patches: https://new.cleopatra.io/public/1b66711e155851108fdb9dbec69e5d72cbb56985/summary/?thread=0
- without patches: https://new.cleopatra.io/public/e53d7188794a798ffc1478dda4b4f6fdca2fafe8/summary/?thread=0

Can't really see a lot of difference between the two results, except that appendStylesheets pops up in the performance report using my patches, but only accounts for 1ms.
I imagine that's just a random difference but I see a notable difference on gcli/templater in the child process:
https://new.cleopatra.io/public/e53d7188794a798ffc1478dda4b4f6fdca2fafe8/calltree/?jsOnly&range=1.2998_3.0110&search=gcli%2Ftemplater&thread=2
versus
https://new.cleopatra.io/public/1b66711e155851108fdb9dbec69e5d72cbb56985/calltree/?jsOnly&range=1.3539_3.0488&search=gcli%2Ftemplater&thread=2
12ms versus 1ms.

Something that sounds slightly more related to your patch (could it change anything to l10n urls?)
https://new.cleopatra.io/public/e53d7188794a798ffc1478dda4b4f6fdca2fafe8/calltree/?jsOnly&range=1.2998_3.0110&search=node-properties&thread=2
versus
https://new.cleopatra.io/public/1b66711e155851108fdb9dbec69e5d72cbb56985/calltree/?jsOnly&range=1.3539_3.0488&search=node-properties&thread=2
29ms versus 12ms.
node-properties.parse seems to be slower. but again that may also be a random slowness. Running multiple profiles can confirm this. Numbers that appear in the profiler are much more stable than talos. So running 3 times gives you a very good insight on what changes with a patch.

Looking at the profile, it looks slightly slower in the child process, when we setup most actors (this in in the "GeckoMain[tab]" timeline, during the second big chunk. This is where I found these two slower code.
Iteration: 53.3 - Dec 26 → 53.4 - Jan 9
(In reply to Alexandre Poirot [:ochameau] from comment #74)
> Something that sounds slightly more related to your patch (could it change
> anything to l10n urls?)
> https://new.cleopatra.io/public/e53d7188794a798ffc1478dda4b4f6fdca2fafe8/
> calltree/?jsOnly&range=1.2998_3.0110&search=node-properties&thread=2
> versus
> https://new.cleopatra.io/public/1b66711e155851108fdb9dbec69e5d72cbb56985/
> calltree/?jsOnly&range=1.3539_3.0488&search=node-properties&thread=2
> 29ms versus 12ms.
> node-properties.parse seems to be slower. but again that may also be a
> random slowness. Running multiple profiles can confirm this. Numbers that
> appear in the profiler are much more stable than talos. So running 3 times
> gives you a very good insight on what changes with a patch.

This shouldn't be changing l10n uris.  Since it sounds like the perf hit isn't showing up in profiles let's go ahead and land it.
Flags: needinfo?(bgrinstead)
Iteration: 53.4 - Jan 9 → ---
Priority: P1 → P3
Whiteboard: [devtools-html] → [reserve-html]
Product: Firefox → DevTools
Not relevant anymore.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: