As said in bug 1172010 comment 2, there is way to shuffle gDevTools.jsm into one or multiple commonjs modules. It may relates to bug 1188401 works also. We should move gDevTools to a module so that its content it automatically re-evaluated when we reload devtools codebase via `tools reload` gclit command!

One important note is that gDevTools.jsm mixes concerns about the tools themselves together with how they tie into Firefox (the gDevToolsBrowser object). I would definitely like to at least have all the Firefox-specific bits in their own file (and that part might need to stay a JSM), which has the side benefit of helping reuse DevTools in non-Firefox (browser.html, etc.). I may have to do some of this as part of the giant file move (bug 912121). There are probably even more separate files that could be created too.

We also have to worry about addon-compat if we end up removing gDevTools.jsm. We should keep it around along with a simple test for it (even if it's just loading and exposing a couple of other modules) to avoid breaking every devtools addon

Moving most of gDevtools logic to a module is going to allow simplifying tools reload. Not only will it allow reloading gDevtools code. It will also simplify a lot reload code itself. Today, we have to cleanup the state of gDevtools a lot, to prevent duplicating tools and themes. It introduces complexity for nothing like theme reset issues from bug 1217153. That's because gDevtools is a JSM and it stays alive even during tools reload. If it becomes a module, the whole module will be garbaged and a new one is going to be spawn. Its state won't have to be cleaned up manually. The only thing we have to still ensure is resetting all the stuff registered into firefox UI. Some addon-sdk or web extensions helpers or API may be of great help here.

Here is a set of patch that only make sense if all applied. This first one, moves all browser related code to a dedicated module. This is just a cut-paste.

Same things for the rest of gDevTools.jsm, move the gDevTools implementation to its own module.

I have to stop modifying attributes on the exported object as the SDK loader freeze it.

And here is the necessary glue to keep gDevTools.jsm working and also support hot reload. I analysed the usage of all methods and dropped comments about that in code. I'm wondering if we can drop various methods? I want to followup in order to stop using gDevtools.jsm from /devtools/ and instead pull the related modules via require. Then, this gDevTools.jsm will only be necessary for addons, but I don't think they use many methods. I tend to think we can just keep {show,close,get}Toolbox and register/unregister methods. Another followup would be to try to use WebExtensions, or AddonSDK or custom addon custom that will be able to apply/unapply changes made to browser/. So that we don't need to keep any devtools code in browser/.

Brian, Do not hesitate to provide your feedback for this change and the possible followups! I'll try to highlight the upcoming changes around gDevTools in the next meeting.

Comment on attachment 8713215 [details] [diff] [review] fix immutability Review of attachment 8713215 [details] [diff] [review]: ----------------------------------------------------------------- Would it make more sense to export as a property to avoid this? Like: exports.devtools = new DevTools(); instead of module.exports.

Comment on attachment 8713225 [details] [diff] [review] Stop exporting unused DevTools symbol Review of attachment 8713225 [details] [diff] [review]: ----------------------------------------------------------------- This seems fine, assuming you verify it's not used by add-ons.

Comment on attachment 8713219 [details] [diff] [review] Add wrappers to modules Review of attachment 8713219 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/framework/gDevTools.jsm @@ +10,5 @@ > > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > Cu.import("resource://gre/modules/Services.jsm"); > > +const {loader} = Cu.import("resource://devtools/shared/Loader.jsm", {}); Nit: use spaces to match Components above @@ +20,5 @@ > + * retrieve the very last version of the modules. > + */ > +Object.defineProperty(this, "require", { > + get() { > + let {require} = Cu.import("resource://devtools/shared/Loader.jsm", {}); Nit: spaces @@ +41,5 @@ > + * > + * It is an instance of a DevTools class that holds a set of tools. It has the > + * same lifetime as the browser. > + */ > +this.gDevTools = { Feels like these methods could be defined with an array of properties which pass their args through, instead of manually defining each one? However, the notes of where each things is used are great, so we should preserve that at least. @@ +196,3 @@ > > // Load the browser devtools main module as the loader's main module. > loader.main("devtools/client/main"); Does this line still belong here? Should it move to the browser module? (I am not sure.)

Comment on attachment 8713213 [details] [diff] [review] Move gDevToolsBrowser to its own module Review of attachment 8713213 [details] [diff] [review]: ----------------------------------------------------------------- Since browser.js is specific to Firefox, shouldn't it move into the /browser tree and out of /devtools? Assuming that happens, I guess it might be named "devtools" or similar, since "browser.js" would be confusing there.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #16) > Comment on attachment 8713213 [details] [diff] [review] > Move gDevToolsBrowser to its own module > > Review of attachment 8713213 [details] [diff] [review]: > ----------------------------------------------------------------- > > Since browser.js is specific to Firefox, shouldn't it move into the /browser > tree and out of /devtools? Assuming that happens, I guess it might be named > "devtools" or similar, since "browser.js" would be confusing there. Agree the 'browser.js' name should change. Maybe 'devtools-browser.js'. I'm not sure about moving it into browser/ at least from a code ownership standpoint (we will be the ones usually editing/reviewing changes to the file).

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14) > Comment on attachment 8713225 [details] [diff] [review] > Stop exporting unused DevTools symbol > > Review of attachment 8713225 [details] [diff] [review]: > ----------------------------------------------------------------- > > This seems fine, assuming you verify it's not used by add-ons. Doesn't seem to be used: https://mxr.mozilla.org/addons/search?string=DevTools%28&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=addons Only one match, the last entry, but it is a custom DevTools class.

(In reply to Brian Grinstead [:bgrins] from comment #17) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #16) > > Comment on attachment 8713213 [details] [diff] [review] > > Move gDevToolsBrowser to its own module > > > > Review of attachment 8713213 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Since browser.js is specific to Firefox, shouldn't it move into the /browser > > tree and out of /devtools? Assuming that happens, I guess it might be named > > "devtools" or similar, since "browser.js" would be confusing there. > > Agree the 'browser.js' name should change. Maybe 'devtools-browser.js'. > I'm not sure about moving it into browser/ at least from a code ownership > standpoint (we will be the ones usually editing/reviewing changes to the > file). +1 I want to keep that in /devtools. We have various browser specific code, in toolbox host and elsewhere. I would like to try to use some Addon APIs in this module to remove the hardcoded code from /browser/. So that this become an addon and shouldn't live in /browser/. Again, as with the addon hack. It doesn't mean actually be an addon, but at least use Addon APIs to remove any devtools things from /browser/.

browser.js was bad. Too generic, means not much. devtools-browser.js still sounds quite generic, but matches well gDevToolsBrowser. I imagine I'll go with that name until I convert all this to addon. Also, given that, I think I'm going to let /browser/ import gDevTools.jsm and instead of trying to require the devtools-browser.js module directly, I'll try to replace everything with addon APIs usages. But for the devtools codebase, I'll try to replace all usages of Cu.import(gDevTools) by require(devtools/client/framework/devtools). Would that plan work for you?

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #13) > Comment on attachment 8713215 [details] [diff] [review] > fix immutability > > Review of attachment 8713215 [details] [diff] [review]: > ----------------------------------------------------------------- > > Would it make more sense to export as a property to avoid this? Like: > > exports.devtools = new DevTools(); > > instead of module.exports. Given my previous comment about replacing Cu.import by require, may be I should go with exports.gDevTools to help replacing this.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #15) > @@ +196,3 @@ > > > > // Load the browser devtools main module as the loader's main module. > > loader.main("devtools/client/main"); > > Does this line still belong here? Should it move to the browser module? (I > am not sure.) Oh, yes, it should be moved to one of the two modules. But it is hard to say which one. main.js is a weird beast. It may be better to just merge it with the new devtools.js as there is a dependency loop between these two. It's just that main.js code should be executed *after* browser.js. Here is how it looks like: main.js call devtools.js:registerTool -> emits tool-registered -> browser.js listen for these events So in order to work, we can't move it to devtools.js as it has to be executed after browser.js. So move the require(main) to browser.js would work. But it would really make sense to merge it into devtools.js. By default, I'll move the require(main) to browser.js until we get a better picture.

(In reply to Alexandre Poirot [:ochameau] from comment #19) > (In reply to Brian Grinstead [:bgrins] from comment #17) > > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #16) > > > Comment on attachment 8713213 [details] [diff] [review] > > > Move gDevToolsBrowser to its own module > > > > > > Review of attachment 8713213 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > Since browser.js is specific to Firefox, shouldn't it move into the /browser > > > tree and out of /devtools? Assuming that happens, I guess it might be named > > > "devtools" or similar, since "browser.js" would be confusing there. > > > > Agree the 'browser.js' name should change. Maybe 'devtools-browser.js'. > > I'm not sure about moving it into browser/ at least from a code ownership > > standpoint (we will be the ones usually editing/reviewing changes to the > > file). > > +1 I want to keep that in /devtools. We have various browser specific code, > in toolbox host and elsewhere. I would like to try to use some Addon APIs in > this module to remove the hardcoded code from /browser/. So that this become > an addon and shouldn't live in /browser/. > Again, as with the addon hack. It doesn't mean actually be an addon, but at > least use Addon APIs to remove any devtools things from /browser/. Okay, makes sense. Seems fine to keep in /devtools then.

(In reply to Alexandre Poirot [:ochameau] from comment #20) > browser.js was bad. Too generic, means not much. > devtools-browser.js still sounds quite generic, but matches well > gDevToolsBrowser. > I imagine I'll go with that name until I convert all this to addon. > Also, given that, I think I'm going to let /browser/ import gDevTools.jsm > and instead of trying to require the devtools-browser.js module directly, > I'll try to replace everything with addon APIs usages. > > But for the devtools codebase, I'll try to replace all usages of > Cu.import(gDevTools) by require(devtools/client/framework/devtools). > > Would that plan work for you? I am not exactly sure what "addon APIs" you have in mind, do you mean for adding toolbar buttons and such? Anyway, at a high level, it sounds good to me.

(In reply to Alexandre Poirot [:ochameau] from comment #22) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #15) > > @@ +196,3 @@ > > > > > > // Load the browser devtools main module as the loader's main module. > > > loader.main("devtools/client/main"); > > > > Does this line still belong here? Should it move to the browser module? (I > > am not sure.) > > Oh, yes, it should be moved to one of the two modules. > But it is hard to say which one. > main.js is a weird beast. It may be better to just merge it with the new > devtools.js as there is a dependency loop between these two. It's just that > main.js code should be executed *after* browser.js. > Here is how it looks like: > main.js call devtools.js:registerTool -> emits tool-registered -> browser.js > listen for these events > So in order to work, we can't move it to devtools.js as it has to be > executed after browser.js. > So move the require(main) to browser.js would work. > But it would really make sense to merge it into devtools.js. > > By default, I'll move the require(main) to browser.js until we get a better > picture. I agree it's a confusing loop for now. Your plan sounds fine. Maybe add a comment above the loader.main call to explain the current situation.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #24) > I am not exactly sure what "addon APIs" you have in mind, do you mean for > adding toolbar buttons and such? Yes, adding shortcuts, tracking windows, adding menuitem,... I don't know yet which one precisely. I would like to target WebExtensions one as it seems to be best option for the future. It may require contributing to WebExtensions if one is missing.

Renamed to devtools-browser.js and exports gDevToolsBrowser.

Exports gDevTools to prevent immutability issues.

Automate wrapper implementation, kept the comments. Let require(main) move for another patch. Rebase to use new module names/exports.

gDevTools is no longer available in global scope.

Comment on attachment 8714812 [details] [diff] [review] Move gDevToolsBrowser to its own module - v2 Review of attachment 8714812 [details] [diff] [review]: ----------------------------------------------------------------- I am assuming this patch is still basically a copy / paste, did not review closely. ::: devtools/client/framework/devtools-browser.js @@ +820,5 @@ > +gDevTools.on("toolbox-destroyed", gDevToolsBrowser._updateMenuCheckbox); > + > +Services.obs.addObserver(gDevToolsBrowser.destroy, "quit-application", false); > + > + Nit: At most one blank line at EOF

Comment on attachment 8714817 [details] [diff] [review] Add wrappers to modules - v2 Review of attachment 8714817 [details] [diff] [review]: ----------------------------------------------------------------- Please add a comment near the start of the file explaining that this JSM still exists mostly to support existing DevTools add-ons, in addition to the in-tree callers until you migrate them. It currently reads a bit oddly (since it seems like we should just update all the in-tree callers and delete it). I know you plan to migrate at least the in-tree ones later, and perhaps the APIs not used from add-ons can be removed from here then.

With the additional comment.

https://hg.mozilla.org/integration/fx-team/rev/16a675e05315e798fcd5990f983bac910b685d71 Bug 1188405 - Convert gDevTools/gDevToolsBrowser into modules. r=jryans

(In reply to Alexandre Poirot [:ochameau] from comment #37) > https://hg.mozilla.org/integration/fx-team/rev/ > 16a675e05315e798fcd5990f983bac910b685d71 > Bug 1188405 - Convert gDevTools/gDevToolsBrowser into modules. r=jryans This broke the browser toolbox for me :(

Backout: https://hg.mozilla.org/integration/fx-team/rev/398e12c2457a

It looks like we miss calling main js. This is automatically done by devtools/client/framework/devtools-browser, but here, I'm not sure anyone loads this module. And I'm not sure there is any point in loading it in the browser toolbox. But we do need to load main.js to have tools and themes correctly registered. This toolbox-process-window.js file is loaded early in the related xul. I imagine that earliest we can get?

Comment on attachment 8716107 [details] [diff] [review] Ensure initializing main client module when running the Browser Toolbox. Review of attachment 8716107 [details] [diff] [review]: ----------------------------------------------------------------- I am assuming you did indeed test that Browser Toolbox works after this. ::: devtools/client/framework/toolbox-process-window.js @@ +7,5 @@ > > var { gDevTools } = Cu.import("resource://devtools/client/framework/gDevTools.jsm", {}); > var { require } = Cu.import("resource://devtools/shared/Loader.jsm", {}); > +// Require this module just to setup things like themes and tools > +require("devtools/client/main"); It's probably best to use `loader.main` for consistency here. (I am not sure we actually _need_ the special "main module" behavior at all, but let's at least keep both call sites consistent.) Overall, this seems like yet another sign that the "devtools/client/main" module is strange and should be reworked in some way.

Thanks to the browser_toolbox test, I've seen some exception during its shutdown. Using require("...main") or loader.main("...main") here ends up loading main.js twice. Instead we should load devtools-browser and let it be the only callsite for loader.main. This really needs some followup!

Comment on attachment 8717054 [details] [diff] [review] Ensure initializing main client module when running the Browser Toolbox - v2 Review of attachment 8717054 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/framework/toolbox-process-window.js @@ +7,5 @@ > > +var { loader, require } = Cu.import("resource://devtools/shared/Loader.jsm", {}); > +// Require this module just to setup things like themes and tools > +// devtools-browser is special as it loads main module > +require("devtools/client/framework/devtools-browser"); Add a reference to some bug that cleans up this mess with "devtools/client/main", etc. (file one if it does not exist yet).

https://hg.mozilla.org/integration/fx-team/rev/15bba224f31148cb7a830c9d7c81659c1d2123f4 Bug 1188405 - Convert gDevTools/gDevToolsBrowser into modules. r=jryans https://hg.mozilla.org/integration/fx-team/rev/cb04e167e1df430f985451628b796b8d26819772 Bug 1188405 - Ensure initializing main client module when running the Browser Toolbox. r=jryans

https://hg.mozilla.org/mozilla-central/rev/15bba224f311 https://hg.mozilla.org/mozilla-central/rev/cb04e167e1df

Adding dev-doc-needed keyword here - if gDevTools.jsm is deprecated then the alternatives need to be documented. As of now, https://developer.mozilla.org/en-US/docs/Tools/DevToolsAPI still recommends importing gDevTools.js. I guess that the proper way for non-SDK add-ons is importing require from Loader.jsm and doing require("devtools/client/framework/devtools") now.