1259398 - Make file-watcher more generic

Status: RESOLVED FIXED

(DevTools :: Framework, defect, P3)

Description

[Alexandre Poirot [:ochameau]]

Today devtools/shared/file-watcher is entangled with BrowserLoader and prevents using it for any other usage.
It would be great to make it generic enough to be able to use it in parralel for other features. Like bug 1257525.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

I'm moving all devtools specifics to browser-loader.js
so that file-watcher is very generic.

I also had to change a bit the way we look for srcdir.
Looking up from resource://devtools/ resolution doesn't work for me,
whereas I have a quite common setup: my objdir is a sub folder of my srcdir.
But resource://devtools/ maps to obj-firefox-opt/dist/bin/browser/chrome/devtools/modules/devtools/
So that two levels up doesn't get out of the obdir and for some reason, findSourceDir resolves before reaching the srcdir.
I'm not looking for Firefox binary path, which is here: obj-firefox-opt/dist/bin/firefox
and immediately know how to get to the parent folder of the objdir.

With this change, we can reasonably start thinking writing a test for this.

Comment 2

[James Long (:jlongster)]

Comment on attachment 8734554 [details] [diff] [review]
patch v1

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

Please do not move it into BrowserLoader. BrowserLoader just has one purpose: to load frontend modules. The only reason it cares about hot reloading at all is because it needs to wrap React components. Otherwise, it should care less. It definitely shouldn't implement the resolution semantics.

I agree with the desire to make the file watcher more generic though. Can you basically do what you've done here, but instead of moving everything in BrowserLoader just move it into a devtools-specific file-watcher module? So we'd have 2 file watcher modules: one that automatically watches devtools files based on the pref, and the other which is more generic and can be used by anything.

::: devtools/client/shared/browser-loader.js
@@ +83,5 @@
>      "Cannot use both `baseURI` and `useOnlyShared`.");
>  
>    const loaderOptions = devtools.require("@loader/options");
>    const dynamicPaths = {};
> +  this.componentProxies = new Map();

Please keep this as a normal local variable that is closed over.

@@ +212,5 @@
>        }
>      }
> +  },
> +
> +  onPrefChange: function() {

No, this is not going to work.

The point of the file-watcher module was that it's a single module that is watching files. We absolutely do not want multiple instances of BrowserLoaders to each be file watching; that would mean every single tool is watching all the directories so if you have 5 tools you have 5 watchers.

The file-watcher module was intentionally made a "service" that does the watching and devtools can subscribe/unsubscribe to it. There's only ever 1 thing doing the actual watching.

Also, BrowserLoader itself should not be concerned at all with file watching. BrowserLoader should stay small and focused.

::: devtools/client/shared/css-reload.js
@@ +105,5 @@
>  function watchCSS(window) {
>    if (Services.prefs.getBoolPref("devtools.loader.hotreload")) {
> +    // If the pref is true, the file watcher has already been initialized
> +    // by BrowserLoader. So that we just have to listen for file changed.
> +    const { BrowserLoader } = Cu.import("resource://devtools/client/shared/browser-loader.js", {});

Please don't import BrowserLoader here. BrowserLoader is morphing into a huge module and all it's meant to be is a loader for modules in browser scope. It has nothing to do with this.

Comment 3

[James Long (:jlongster)]

The way srcdir resolution works is is basically keeps walking up until it finds a "devtools/client" directory (I think). This does assume the objdir is underneath the srcdir, but it was the best way to work across platforms. There's not a great way to go from objdir->srcdir at runtime.

But anyway, as long as everything still works for devtools, I'm fine splitting file-watcher into generic/specific modules.

Comment 4

[James Long (:jlongster)]

I hope my comments don't seem too strong, I should have avoid words like "absolutely".

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

Sure. Here is a version with another intermediate module: devtools-file-watcher.
Which basically takes out any devtools specifics out of file-watcher,
without putting it into browser-loader.

Comment 6

[James Long (:jlongster)]

Comment on attachment 8735876 [details] [diff] [review]
patch v2

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

The new logic for figuring out the srcdir does not work. Can you revert those changes and use what was previously there? It worked before even with your addon. The path it is watching for me is:

/Users/james/projects/mozilla/gecko-dev/obj-x86_64-apple-darwin15.3.0/dist/devtools/client

When it should be /Users/james/projects/mozilla/gecko-dev/devtools/client

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

Fixed findSourceDir, OS.File.exists (and most[all?] OS APIs returns Promises).
Also restored to previous algorithm to find srcdir.

Comment 8

[James Long (:jlongster)]

Comment on attachment 8736776 [details] [diff] [review]
patch v3

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

Looks good to me! Can you also test it with your addon to make sure it resolves to the right path? It should watch whatever path your addon maps resource:// devtools to. I don't have that set up right now. (although I'm not sure anyone is using that path, if you already have it set up just do a quick test)

Comment 9

[Alexandre Poirot [:ochameau]]

(In reply to James Long (:jlongster) from comment #8)
> Comment on attachment 8736776 [details] [diff] [review]
> patch v3
> 
> Review of attachment 8736776 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me! Can you also test it with your addon to make sure it
> resolves to the right path? It should watch whatever path your addon maps
> resource:// devtools to. I don't have that set up right now. (although I'm
> not sure anyone is using that path, if you already have it set up just do a
> quick test)

Yes. Still works.
I was about to automatically turn it on when using the addon but heard about bug 1252497.

But I'm planning to expose css hotreload to all addons instead, in bug 1257525, and that will works with the devtools addon.

Comment 10

[Alexandre Poirot [:ochameau]]

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

Comment 11

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/234d4a1ad9e8dc2baeb43e24554987857c25c1b4
Bug 1259398 - Make devtools file-watcher module more generic to watch any directory. r=jlongster

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/234d4a1ad9e8