Closed Bug 1243406 Opened 4 years ago Closed 4 years ago

Enable CSS/SVG/PNG hot reloading for all panels

Categories

(DevTools :: Framework, defect)

defect
Not set

Tracking

(firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: jlong, Assigned: jlong)

References

Details

Attachments

(1 file, 9 obsolete files)

Currently hot reloading requires the BrowserLoader, and that's always going to be the case for JS files. However there's not reason why we can't expose an API for pages to start hot reloading CSS files, since that's pretty simple.
Blocks: 1239059
Blocks: 1243490
No longer blocks: 1243490
Note: we should disable the hot reloading pref for the Browser Toolbox so it doesn't get messed up if CSS / JS breaks (and we want to be able to debug what broke in the main toolbox).  Pref can be set here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox-process-window.js#53
Assignee: nobody → jlong
Changing the title because after some research I decided to move the CSS hot reloading entirely into its own module (BrowserLoader doesn't do it at all) so that it can be enabled everywhere by default. Anything that doesn't use BrowserLoader can only hot reload CSS anyway, there's no way to hot reload JS without it.
Summary: Expose hot reloading API for pages not using BrowserLoader → Enable CSS hot reloading for all panels
Attached patch 1243406.patch (obsolete) — Splinter Review
After extensive research, this is the best approach I could come up with.

Here is a video of tweaking the entire theme live as well as individual panel styles: http://jlongster.com/s/devtools-css-reload.mp4

I made a new module that listens to file change events and hot reloads CSS. It does this by scanning the xml-stylesheets and link tags for CSS that has the same path as the file that changed. This is a little ambiguous because we only have a path like `client/foo/bar.css`, and we don't know whether to resolve that to `chrome://devtools/content/foo/bar.css` or `resource://devtools/client/foo/bar.css`. I just scan for both. Additionally, if the path starts with `client/themes` we resolve it to `chrome://devtools/skin/file.css`.

This seems to cover all the cases that I tested. We now support xml-stylesheets and link tags, removing the old node and adding a new one to force the stylesheet to be refreshed.

I hit several caching issues. There's some really strange behavior: clearing the startup cache seems to work for xml-stylesheets, but not for link tags. Link tags that are added again with the same URL will *not* pick up changes, strangely. To cover all cases, I clear the startup cache and append a random query string.

Another problem is @imports: this won't work with them, but I special-cased the two imports that the theme files use (variables and toolbars) so that we always reload the whole them file instead. We don't seem to use imports elsewhere (which is good), so it shouldn't be a big problem. I can't append a query string to an import to force it to refresh, but luckily clearing the startup cache actually seems to work for those 2 files, not sure why it works there and not elsewhere.

Lastly, to enable this everywhere by default, I enable the watching in theme-switching.js because everything already includes that and this is CSS-related.
Attachment #8715828 - Flags: review?(bgrinstead)
Attachment #8715828 - Flags: feedback?(jryans)
Attached patch 1243406.patch (obsolete) — Splinter Review
Fixed some styling issues
Attachment #8715828 - Attachment is obsolete: true
Attachment #8715828 - Flags: review?(bgrinstead)
Attachment #8715828 - Flags: feedback?(jryans)
Attachment #8715830 - Flags: review?(bgrinstead)
Attachment #8715830 - Flags: feedback?(jryans)
Comment on attachment 8715830 [details] [diff] [review]
1243406.patch

Review of attachment 8715830 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/shared/theme-switching.js
@@ +159,5 @@
>  
>    const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
>    Cu.import("resource://gre/modules/Services.jsm");
>    Cu.import("resource://devtools/client/framework/gDevTools.jsm");
> +  const { require } = Components.utils.import("resource://devtools/shared/Loader.jsm", {});

Nit: Could use `Cu` here
Attachment #8715830 - Flags: feedback?(jryans) → feedback+
Summary: Enable CSS hot reloading for all panels → Enable CSS/SVG/PNG hot reloading for all panels
Attached patch 1243406.patch (obsolete) — Splinter Review
New patch that goes ahead and does hot reloading for SVG and PNGs referenced in CSS files as well.
Attachment #8715830 - Attachment is obsolete: true
Attachment #8715830 - Flags: review?(bgrinstead)
Attachment #8715921 - Flags: review?(bgrinstead)
Attached patch 1243406.patch (obsolete) — Splinter Review
A few more style tweaks
Attachment #8715921 - Attachment is obsolete: true
Attachment #8715921 - Flags: review?(bgrinstead)
Attachment #8715947 - Flags: review?(bgrinstead)
Attached patch 1243406.patch (obsolete) — Splinter Review
one last style tweak
Attachment #8715947 - Attachment is obsolete: true
Attachment #8715947 - Flags: review?(bgrinstead)
Attachment #8715951 - Flags: review?(bgrinstead)
Comment on attachment 8715951 [details] [diff] [review]
1243406.patch

Review of attachment 8715951 [details] [diff] [review]:
-----------------------------------------------------------------

For some reason this isn't detecting changes for me for css files in the themes/ directory.  It is emitting file-changed events on client/framework/toolbox.js though
Which file did you change? Can you add some logging to the css-reload.js function to see where it is stopping?

It might be nice to have some logging in there by default, but I didn't want to be too noisy.
(In reply to James Long (:jlongster) from comment #10)
> Which file did you change? Can you add some logging to the css-reload.js
> function to see where it is stopping?
> 
> It might be nice to have some logging in there by default, but I didn't want
> to be too noisy.

In onFileChanged: `console.log("detected change", relativePath);`.  I don't see changes to toolbars.css, light-theme.css etc.  But if I change any js file it comes through properly.  Seems like maybe something in watchFiles although I haven't been able to debug it.  Does it work for you with the latest version of the patch when changing those files?
Yep, definitely. That's weird, so it's not even the CSS not being reloaded properly, the CSS files aren't even picked up as being changed?

Can you log both `resolvedRootURI` and `localURI` in `watchFiles`? Do you have any other things set up that might mess with the local path that it's trying to watch?
(In reply to James Long (:jlongster) from comment #12)
> Yep, definitely. That's weird, so it's not even the CSS not being reloaded
> properly, the CSS files aren't even picked up as being changed?
> 
> Can you log both `resolvedRootURI` and `localURI` in `watchFiles`? Do you
> have any other things set up that might mess with the local path that it's
> trying to watch?

Weird, the theme directory it's looking at is /fx-team/objdir.noindex/dist/Nightly.app/Contents/Resources/browser/chrome/devtools/modules/devtools/client/themes, but looking in that folder I only see variables.css.   I see a bunch of css files in /fx-team/objdir.noindex/dist/Nightly.app/Contents/Resources/browser/chrome/devtools/skin.  Seems like something's weird with my object directory... I wonder if a full rebuild will help.
Note the logic in `watchFiles`: it actually should resolve to your real gecko directory, like `/Users/james/projects/gecko-dev`, not the one deep in the obj directory. It looks for a path with the pattern `obj-.*` and strips it off. I am very curious why your path is not formed that way, any ideas? That means it's not watching your real directory.

We do still require symlinks within the obj directory because we resolve *back* to a chrome or resource URL and load that, which requires the symlinks to read the actual file. But we watch your real directory because we wouldn't get modified times on a symlink.
Attached patch 1243406.patch (obsolete) — Splinter Review
This patch fixes SVG/PNG reloading in imported stylesheets, it wasn't reloading them before.

Still going to figure out a better way to resolve to the local user's directory to fix bgrins' problem.
Attachment #8715951 - Attachment is obsolete: true
Attachment #8715951 - Flags: review?(bgrinstead)
Attached patch 1243406.patch (obsolete) — Splinter Review
Now also update the toolbar icons. They were <image> tags with a `src`, so now we scan for those too.
Attachment #8716425 - Attachment is obsolete: true
Attached patch 1243406.patch (obsolete) — Splinter Review
Alright can you try this patch? I know longer look for a certain format of an "obj" directory anymore, this should be a lot more reliable.
Attachment #8716434 - Attachment is obsolete: true
Attachment #8716454 - Flags: review?(bgrinstead)
Comment on attachment 8716454 [details] [diff] [review]
1243406.patch

Review of attachment 8716454 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/shared/file-watcher.js
@@ +25,5 @@
>    // We need to figure out a local path to watch. We start with
>    // whatever devtools points to.
>    let resolvedRootURI = resolveResourceURI("resource://devtools");
> +
> +  const appNameIndex = resolvedRootURI.indexOf(AppConstants.MOZ_MACBUNDLE_NAME);

MOZ_MACBUNDLE_NAME won't work for other OSes, since that value won't appear in the path anywhere.  Maybe we should either expose MOZ_OBJDIR, or :bgrins should add "obj-" to the name of his objdir.
Hm, ok. Two things I need to do next week:

1. To figure out which files to watch, given a the objdir path to resource://devtools, walk up every directory and look for the `devtools/client` directory. Once found, watch that path. That should work reliable for every config except those where the objdir lives outside of the srcdir, but oh well. For those people, we could also support a custom config setting that explicitly gives us the path to watch (todo later).

2. Fix the splitters.css file which is imported by a few different files. I will just hardcode this for now as I do the 2 other files that are imported. If we want to start importing more, I can implement a more general solution later (when any theme files change just reload all of them? I don't know).
Comment on attachment 8716454 [details] [diff] [review]
1243406.patch

Clearing review (comment 19)
Attachment #8716454 - Flags: review?(bgrinstead)
Attached patch 1243406.patch (obsolete) — Splinter Review
This is a final patch. I solved the srcdir problem by walking up from the objdir until I find a parent directory with a specific devtools path, and I deem that the srcdir. This means it still won't work for people who store the objdir outside of the srcdir path, but that seems very rare and this should work for most people.

Helen, can you also try this patch and make sure it still works? There are some significant changes regarding the CSS reloading. I also made sure that the `splitters.css` file is hot reloadable now (it's one of the few ones that are imported and I have to special-case those)
Attachment #8716454 - Attachment is obsolete: true
Attachment #8719995 - Flags: review?(bgrinstead)
(In reply to James Long (:jlongster) from comment #21)
> Helen, can you also try this patch and make sure it still works? There are
> some significant changes regarding the CSS reloading. I also made sure that
> the `splitters.css` file is hot reloadable now (it's one of the few ones
> that are imported and I have to special-case those)

ni? so the request doesn't get lost
Flags: needinfo?(hholmes)
James, can you rebase the patch?
Comment on attachment 8719995 [details] [diff] [review]
1243406.patch

Review of attachment 8719995 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/shared/css-reload.js
@@ +7,5 @@
> +const { getTheme } = require("devtools/client/shared/theme");
> +
> +function iterStyleNodes(window, func) {
> +  for (let node of window.document.childNodes) {
> +    if (node.nodeType === 7) {

Please add a comment that this is for ProcessingInstruction nodes

@@ +107,5 @@
> +      if (relativePath.match(/\.css$/)) {
> +        if (relativePath.startsWith("client/themes")) {
> +          let path = relativePath.replace(/^client\/themes\//, "");
> +
> +          // Special-case a few files that get imported from other CSS

Curious, why do we need to special case these files - won't _replaceResourceInSheet handle this case since it's run over all stylesheets in the document?

@@ +109,5 @@
> +          let path = relativePath.replace(/^client\/themes\//, "");
> +
> +          // Special-case a few files that get imported from other CSS
> +          // files. We just manually hot reload the parent CSS file.
> +          if (path === "variables.css" || path === "toolbars.css") {

This also needs to include common.css assuming Bug 1246733 sticks (and I guess maybe also splitters.css since that's a nested import from common.css?)

::: devtools/client/shared/file-watcher-worker.js
@@ +72,5 @@
>  onmessage = function(event) {
> +  const { path, devtoolsPath, fileRegex } = event.data;
> +
> +  // We need to figure out a src dir to watch. These are the actual
> +  // files the user is working with, not the any files in the obj dir.

grammar: 'not the any'

@@ +84,5 @@
> +  // specific place on the filesystem for loading devtools externally.
> +  //
> +  // `devtoolsPath` is currently the devtools directory inside of the
> +  // obj dir, and we search for `devtools/client`, so go up 2 levels.
> +  const searchPoint = OS.Path.dirname(

Well, I can confirm this works on my computer while the previous approach did not.  And since it's only affecting people who flip this pref it seems generally low-risk.  Still, I'm not sure if I'm the best person to review this part though.. maybe jryans?

Also, what happens if the pref has been flipped active and you are running a binary, not in a local build?
Attachment #8719995 - Flags: review?(jryans)
Attachment #8719995 - Flags: review?(bgrinstead)
Attachment #8719995 - Flags: feedback+
Status: NEW → ASSIGNED
(In reply to Brian Grinstead [:bgrins] from comment #24)
> Comment on attachment 8719995 [details] [diff] [review]
> 1243406.patch
> 
> 
> @@ +107,5 @@
> > +      if (relativePath.match(/\.css$/)) {
> > +        if (relativePath.startsWith("client/themes")) {
> > +          let path = relativePath.replace(/^client\/themes\//, "");
> > +
> > +          // Special-case a few files that get imported from other CSS
> 
> Curious, why do we need to special case these files - won't
> _replaceResourceInSheet handle this case since it's run over all stylesheets
> in the document?

That's only for updating svg/png files. I can't change import rules dynamically; they are readonly. Updating CSS files involves scanning the DOM for all references to a file, and refreshing that.

That makes me wonder if CSSImportRule objects have the `cssText` property (which it should) and I actually can set that, overriding the entire rule with a new "@imports ..." string. Even if that works, I'd rather not make reloading CSS so heavy and have to scan the entire DOM *and* the entire set of rules.

imports are generally avoid because it blocks CSS parsing. When the browser hits an import it has to stop everything and go fetch the file, which you really don't want.

Of course, if we had a build step we could easily use them and flatten it out at build time.

> 
> @@ +109,5 @@
> > +          let path = relativePath.replace(/^client\/themes\//, "");
> > +
> > +          // Special-case a few files that get imported from other CSS
> > +          // files. We just manually hot reload the parent CSS file.
> > +          if (path === "variables.css" || path === "toolbars.css") {
> 
> This also needs to include common.css assuming Bug 1246733 sticks (and I
> guess maybe also splitters.css since that's a nested import from common.css?)

Ok. I'm not a huge fan of using imports too much for the reason stated above. But I suppose it's not as huge of a deal since we aren't going across a network. Still, it makes hot reloading a good bit harder.

> @@ +84,5 @@
> > +  // specific place on the filesystem for loading devtools externally.
> > +  //
> > +  // `devtoolsPath` is currently the devtools directory inside of the
> > +  // obj dir, and we search for `devtools/client`, so go up 2 levels.
> > +  const searchPoint = OS.Path.dirname(
> 
> Well, I can confirm this works on my computer while the previous approach
> did not.  And since it's only affecting people who flip this pref it seems
> generally low-risk.  Still, I'm not sure if I'm the best person to review
> this part though.. maybe jryans?
> 
> Also, what happens if the pref has been flipped active and you are running a
> binary, not in a local build?

It will walk up to the root path and not find the src dir, and just start watching the files within the app. Theoretically you could actually dig into those the app and open those files and start messing with stuff! I'm assuming resource:// files are shipped as normal files. Worst case, the worker will stat the directory it resolves to and errors out if it doesn't exist.
Attached patch 1243406.patchSplinter Review
Addressed bgrins' comments
Attachment #8719995 - Attachment is obsolete: true
Attachment #8719995 - Flags: review?(jryans)
Attachment #8720381 - Flags: review?(jryans)
(In reply to James Long (:jlongster) from comment #25)
> > @@ +109,5 @@
> > > +          let path = relativePath.replace(/^client\/themes\//, "");
> > > +
> > > +          // Special-case a few files that get imported from other CSS
> > > +          // files. We just manually hot reload the parent CSS file.
> > > +          if (path === "variables.css" || path === "toolbars.css") {
> > 
> > This also needs to include common.css assuming Bug 1246733 sticks (and I
> > guess maybe also splitters.css since that's a nested import from common.css?)
> 
> Ok. I'm not a huge fan of using imports too much for the reason stated
> above. But I suppose it's not as huge of a deal since we aren't going across
> a network. Still, it makes hot reloading a good bit harder.

Yeah, imports aren't ideal in some ways but it makes it really easy to centrally manage these styles for all themes.  Another option would be to have the light and dark theme definitions include variables.css, common.css and toolbars.css in front of their theme file so that theme-switching will automatically add them as separate resources.  https://dxr.mozilla.org/mozilla-central/source/devtools/client/definitions.js#419.

One downside to that is that if someone was directly loading light-theme / dark-theme.css outside of theme-switching it wouldn't work properly.  Like here, for instance: https://dxr.mozilla.org/mozilla-central/source/devtools/client/projecteditor/chrome/content/projecteditor.xul#5.  We have a feature in theme-switching where we could force the theme to light for that window so might be able to work around it there.
Comment on attachment 8720381 [details] [diff] [review]
1243406.patch

Review of attachment 8720381 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, seems to work well, thanks for doing this!  Also seems to work with Alex's reload add-on as well.

::: devtools/client/shared/css-reload.js
@@ +33,5 @@
> +      // xml-stylesheet declaration
> +      if (node.data.includes(fileURI)) {
> +        const newNode = window.document.createProcessingInstruction(
> +          "xml-stylesheet",
> +          "href=\"" + fileURI + "?s=" + Math.random() +

Nit: Use template strings for less pain

@@ +101,5 @@
> +}
> +
> +function watchCSS(window) {
> +  if (Services.prefs.getBoolPref("devtools.loader.hotreload")) {
> +    const watcher = require("devtools/client/shared/file-watcher");

It could be a slight improvement to call `Math.random` only once per run, and pass down to the other functions.  Then, if someone observes URLs in the inspector, each "pass" only uses a single value.

@@ +122,5 @@
> +
> +        replaceCSS(
> +          window,
> +          "chrome://devtools/content/" +
> +            relativePath.replace(/^client\//, "")

Nit: probably fits on the line above?

::: devtools/client/shared/file-watcher.js
@@ +7,3 @@
>  const { Services } = require("resource://gre/modules/Services.jsm");
>  const EventEmitter = require("devtools/shared/event-emitter");
> +Cu.import("resource://gre/modules/AppConstants.jsm");

Seems unused?  If needed, use `const thing = require(jsm)` format for easier linting.

::: devtools/client/shared/theme-switching.js
@@ +158,5 @@
>    }
>  
>    const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
>    Cu.import("resource://gre/modules/Services.jsm");
> +  const { require } = Components.utils.import("resource://devtools/shared/Loader.jsm", {});

Cu
Attachment #8720381 - Flags: review?(jryans) → review+
It seems like it's still working over here—I've been using it for bugs 1242709 and 1205330 without complaint.
Flags: needinfo?(hholmes)
https://hg.mozilla.org/mozilla-central/rev/88df606b81da
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.