This patch starts the process of moving our Cu.import-loaded modules to jetpack-loaded modules. The long-term benefit is that it lets us more easily load devtools separately from Firefox, for lightweight hacking on the tools without a complete Firefox deve environment.

Cc-o-rama. The latest patch converts the inspector and tilt. The remaining pieces should be easy enough (you'll see in the patch that's a mostly mechanical conversion), but I didn't touch them yet because most of them have entry points other than the toolbox, and that will take a bit of extra thought. Please take a glance and see if the general idea of it is to your liking. Everyone should take a look at the parts they care about and raise a flag if there's a problem, but I'm likely to ask for official review from jwalker and one of alex/irakli/mossop.

Comment on attachment 730990 [details] [diff] [review] WIP Review of attachment 730990 [details] [diff] [review]: ----------------------------------------------------------------- I'm insanely happy about this! Let's convert the debugger, too! ::: browser/devtools/framework/gDevTools.jsm @@ +179,5 @@ > DevTools.prototype = { > + // This is a gross gross hack. In one place (computed-view.js) we use > + // Iterator, but the addon-sdk loader takes Iterator off the global. > + // Give computed-view.js a way to get back to the Iterator until we have > + // a chance to fix that crap. I'm curious as to why the sdk loader does that, or is it just a bug? @@ +436,5 @@ > + } > + > + // Register the set of default tools > + for (let definition of this.Tools.defaultTools) { > + this.unregisterTool(definition.id); Typo in comment. @@ +457,5 @@ > + > + this._chooseProvider(); > + > + this._provider.load(); > + this._newProvider(); this._provider.load() is redundant here, since it's the first thing that _newProvider does. Also, _chooseProvider seems to always be followed by _newProvider, so you could also add the call to the latter in the former. ::: browser/devtools/framework/Toolbox.jsm @@ +51,5 @@ > * @see Consuming promises in https://addons.mozilla.org/en-US/developers/docs/sdk/latest/packages/api-utils/promise.html > * @see https://bugzilla.mozilla.org/show_bug.cgi?id=790195 > * @see https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/promise.js#L179 > */ > +let promised = (function() { This can go now if you feel like it, bug 790195 has landed. Promise.all, too. ::: browser/devtools/markupview/Makefile.in @@ +10,5 @@ > > include $(DEPTH)/config/autoconf.mk > > libs:: > + $(NSINSTALL) $(srcdir)/*.js $(FINAL_TARGET)/modules/devtools/markupview I'm curious about this sort of change: do you have another patch that mechanically copies jsm -> js?

Tilt changes look ok. Is this patch going to wait on bug 855544 to land or viceversa?

Comment on attachment 730990 [details] [diff] [review] WIP Review of attachment 730990 [details] [diff] [review]: ----------------------------------------------------------------- Except the override manifest hack, I'm really excited to see how simple this patch is! Do not hesitate to ping us and fill bugs to avoid you any workaround. Otherwise, have you tried pushing it to try server? (I'm looking forward talos numbers) ::: browser/devtools/framework/gDevTools.jsm @@ +49,5 @@ > + delete this.loader; > + delete this.require; > + delete this.Tools; > + delete this.Toolbox; > + delete this.TargetFactory; We may wanna move this SDK-specific code to the loader, or somehow share this piece of code in order to nuke all compartments created by this destroyed loader: https://github.com/mozilla/addon-sdk/blob/master/app-extension/bootstrap.js#L314-L326 (Here bootstrap.js isn't shipped into firefox, so that there is no way to share that piece of code) @@ +69,5 @@ > + "devtools": "file://" + devtoolsDir, > + } > + }); > + > + this.require = Require(this.loader, { id: "devtools-srcdir" }); This `id` will be printed whenever top-level this.require() calls will fail (most likely when the module won't be found). It will also be used if any of this call uses a relative path. Now, I'm not really saying devtools-srcdir is a bad choice. I just want to highlight why we require an id argument here and what are its usages. There is also no usage of any main module. You may gain from this feature if you have anything that looks like a main module. Wouldn't the `DevTools` class be something like this? @@ +91,5 @@ > + delete this.require; > + delete this.Tools; > + delete this.Toolbox; > + delete this.TargetFactory; > + }, I don't really buy into BuiltinProvider and SrcDirProvider. this.{require, Tools, Toolbox, TargetFactory} definitions and unload method should definitively be unique/shared. I would imagine that the only thing that will differ between builtin and external would be the loader path argument and the writeManifest call. @@ +152,5 @@ > + }).then(() => { > + Components.manager.addBootstrappedManifestLocation(new FileUtils.File(dir)); > + }); > + } > +}; :'( I'm literally crying!! Do we have a bug filled to implement an api to register new override rules? Otherwise, here you are not only overriding devtools uri, but all of them, all `content` rules, right? So that it may also bring easy reload-without-building development to other components!! Do you think it would be possible to filter to only devtools stuff if we want to implement similar feature for let's say browser chrome code? @@ +179,5 @@ > DevTools.prototype = { > + // This is a gross gross hack. In one place (computed-view.js) we use > + // Iterator, but the addon-sdk loader takes Iterator off the global. > + // Give computed-view.js a way to get back to the Iterator until we have > + // a chance to fix that crap. I don't really know why we prevent it from being exposed: https://github.com/mozilla/addon-sdk/blob/master/lib/toolkit/loader.js#L185 I can understand why we do that for Components and debug, but I would say we can just remove this delete statement and get it back. --> bug 857113

(In reply to Alexandre Poirot (:ochameau) from comment #4) > Comment on attachment 730990 [details] [diff] [review] > WIP > > Review of attachment 730990 [details] [diff] [review]: > ----------------------------------------------------------------- > > Except the override manifest hack, I'm really excited to see how simple this > patch is! Yeah, will do (see my event target stuff for the first of my pingings). > Do not hesitate to ping us and fill bugs to avoid you any workaround. > Otherwise, have you tried pushing it to try server? (I'm looking forward > talos numbers) I will push today. > > ::: browser/devtools/framework/gDevTools.jsm > @@ +49,5 @@ > > + delete this.loader; > > + delete this.require; > > + delete this.Tools; > > + delete this.Toolbox; > > + delete this.TargetFactory; > > We may wanna move this SDK-specific code to the loader, > or somehow share this piece of code in order to nuke all compartments > created by this destroyed loader: > https://github.com/mozilla/addon-sdk/blob/master/app-extension/bootstrap. > js#L314-L326 > (Here bootstrap.js isn't shipped into firefox, so that there is no way to > share that piece of code) Oh yeah, nuking compartments would be great. Thanks for the pointer. > > @@ +69,5 @@ > > + "devtools": "file://" + devtoolsDir, > > + } > > + }); > > + > > + this.require = Require(this.loader, { id: "devtools-srcdir" }); > > This `id` will be printed whenever top-level this.require() calls will fail > (most likely when the module won't be found). > It will also be used if any of this call uses a relative path. > > Now, I'm not really saying devtools-srcdir is a bad choice. > I just want to highlight why we require an id argument here and what are its > usages. > > There is also no usage of any main module. > You may gain from this feature if you have anything that looks like a main > module. Wouldn't the `DevTools` class be something like this? Yeah, I think so. I'm going to rethink the gDevTools/main module/API entry point thing a bit. > > + }).then(() => { > > + Components.manager.addBootstrappedManifestLocation(new FileUtils.File(dir)); > > + }); > > + } > > +}; > > :'( I'm literally crying!! > Do we have a bug filled to implement an api to register new override rules? > > Otherwise, here you are not only overriding devtools uri, but all of them, > all `content` rules, right? > So that it may also bring easy reload-without-building development to other > components!! Do you think it would be possible to filter to only devtools > stuff if we want to implement similar feature for let's say browser chrome > code? The manifest that it generates is based on the jar.mn, and only overrides specific urls. So only urls from browser/devtools/jar.mn and toolkit/devtools/jar.mn are overridden. It really is ugly. > I don't really know why we prevent it from being exposed: > https://github.com/mozilla/addon-sdk/blob/master/lib/toolkit/loader.js#L185 > I can understand why we do that for Components and debug, but I would say we > can just remove this delete statement and get it back. > --> bug 857113 Thanks.

https://tbpl.mozilla.org/?tree=Try&rev=03de0f48d91d

(In reply to Victor Porof [:vp] from comment #3) > Tilt changes look ok. > Is this patch going to wait on bug 855544 to land or viceversa? Loser has to rebase. (No specific plan).

(In reply to Dave Camp (:dcamp) from comment #6) > https://tbpl.mozilla.org/?tree=Try&rev=03de0f48d91d That's not going to succeed. I'll try to get another one up this evening.

Comment on attachment 730990 [details] [diff] [review] WIP Review of attachment 730990 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/commandline/BuiltinCommands.jsm @@ +1518,5 @@ > + > +(function(module) { > + gcli.addCommand({ > + name: "tools", > + description: "Commands for managing developer tools.", Insert discussion of l10n for developers here. Historically we've l10ned all strings even if hidden behind a chrome pref. But I can see a good argument for not doing that here. The debate about l10n here is similar in some ways to the debate about l10n for doc comments. @@ +1535,5 @@ > + } > + ], > + returnType: "string", > + exec: function(args, context) { > + let promise = context.createPromise(); We're about to deprecate the old way, in favor of the much more obvious: let deferred = context.defer(); ::: browser/devtools/inspector/Breadcrumbs.jsm @@ +49,5 @@ > this.DOMHelpers = new DOMHelpers(this.chromeWin); > this._init(); > } > > +module.exports = HTMLBreadcrumbs; Wouldn't: exports.HTMLBreadcrumbs = HTMLBreadcrumbs; Allow for greater flexibility of this module in the future? (Also elsewhere). ::: browser/devtools/styleinspector/CssHtmlTree.jsm @@ +6,5 @@ > > +const {Cc, Ci, Cu} = require("chrome"); > + > +var ToolDefinitions = require("devtools/framework/tool-definitions"); > +var {CssLogic} = require("devtools/styleinspector/css-logic"); Nit: const > let > var ? ::: browser/devtools/styleinspector/StyleInspector.jsm @@ +6,5 @@ > > +const {Cc, Cu, Ci} = require("chrome"); > + > +let ToolDefinitions = require("devtools/framework/tool-definitions"); > +let {lazyGetter} = require("devtools/shared/helpers"); I don't see this defined anywhere. Is it in the patch? Also is 'helpers' a good name given that we're using that in command testing already? ::: browser/devtools/webconsole/test/head.js @@ +9,5 @@ > Cu.import("resource://gre/modules/devtools/WebConsoleUtils.jsm", tempScope); > let WebConsoleUtils = tempScope.WebConsoleUtils; > Cu.import("resource:///modules/devtools/gDevTools.jsm", tempScope); > let gDevTools = tempScope.gDevTools; > +let TargetFactory = gDevTools.TargetFactory; We talked about having main.TargetFactory or similar. I like that idea. I'm noting it here for Future Generations. (Hello, Future Generations)

New version feels a bit better to me: * devtools/framework/tool-definitions was converted into a main.js. That just seemed to make sense. It now also takes care of registering and unregistering the tools it adds to gDevTools itself, rather than gDevTools understanding the defaultTools thing. * I export a DevToolsModule obj from gDevTools that'll have the properties from main's exports. * I left in the Iterator hack - it will be easy to remove by rewriting an ugly bit of computed-view.js, but I'd like to do that as a followup. * i18n for the gcli commands is... started but not complete, and I just remembered that. Next version will fix. * Providers are still there, but trimmed down quite a bit. * instead of helpers.js adding two functions that is used *everywhere* (lazyGetter and lazyImport), I add those to the module globals in main.js. It's starting to feel kinda nice, I think, though we have a ways to go before it's pretty. Pushed to try at: https://tbpl.mozilla.org/?tree=Try&rev=9d2010055d71

Comment on attachment 733160 [details] [diff] [review] WIP 2 Review of attachment 733160 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/framework/ToolDefinitions.jsm @@ +13,5 @@ > + > +loaderOptions.globals.lazyGetter = XPCOMUtils.defineLazyGetter.bind(XPCOMUtils); > +loaderOptions.globals.lazyImporter = XPCOMUtils.defineLazyModuleGetter.bind(XPCOMUtils); > + > +let {on, off} = require("sdk/system/events"); I added to the devtools thursday meeting, a discussion of let module = import(...); module.func(...); vs: let func = import(...).func; func(...); I'm not proposing that we change this code, but it might be worth deciding if a) we want to all do this the same way and b) which way. @@ +18,5 @@ > + > +Cu.import("resource://gre/modules/Services.jsm"); > +Cu.import("resource:///modules/devtools/gDevTools.jsm"); > + > +lazyGetter(exports, "Toolbox", () => require("devtools/framework/toolbox").Toolbox); There is an element of surprise here, especially if in another js file: Where does this global come from? Would it be possible to have "require.lazyGetter"? Then it would obviously come from the host environment. If not, then perhaps loader.lazyGetter would at least give you a clue, and you'd only have to learn once, not once for each global. @@ +19,5 @@ > +Cu.import("resource://gre/modules/Services.jsm"); > +Cu.import("resource:///modules/devtools/gDevTools.jsm"); > + > +lazyGetter(exports, "Toolbox", () => require("devtools/framework/toolbox").Toolbox); > +lazyGetter(exports, "TargetFactory", () => require("devtools/framework/target").TargetFactory); And perhaps we could have an API like this: lazyModuleGetter(exports, "TargetFactory", "devtools/framework/target"); It seems like that's how it's used mostly. ::: browser/devtools/shared/DeveloperToolbar.jsm @@ +30,5 @@ > XPCOMUtils.defineLazyModuleGetter(this, "PluralForm", > "resource://gre/modules/PluralForm.jsm"); > > +XPCOMUtils.defineLazyModuleGetter(this, "DevToolsModule", > + "resource:///modules/devtools/gDevTools.jsm"); Maybe this is bikeshedding, but I think of things with an initial capital as constructor functions, other things as instances, so it feels like this should be devtoolsModule, or maybe just 'devtools' since we don't have the 'Module' suffix on other modules. ::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties @@ +718,5 @@ > pagemodRemoveAttributeResult=Elements matched by selector: %1$S. Attributes removed: %2$S. > > +# LOCALIZATION NOTE (toolsDesc) A very short description of the 'tools' > +# command, the parent command for tool-hacking commands. > +toolsDesc=Hack the Firefox Developer Tools. Nit: GCLI descriptions are titles and shouldn't have periods at the end.

Comment on attachment 733160 [details] [diff] [review] WIP 2 Review of attachment 733160 [details] [diff] [review]: ----------------------------------------------------------------- I have the same question than panos about jsm. I can see you copying js and jsm, but jsm are now used as modules and should be .js. Like Sidebar.jsm, you do require it with require(devtools/framework/sidebar) I don't see how it can possibly work with jsm extension. Also, I don't see any main.js in this patch? Did you forgot putting it in patch or how does that work? Otherwise, I tried to look at talos results, but it doesn't say much to me: http://perf.snarkfest.net/compare-talos/index.html?oldRevs=6a3cd3b1cc20&newRev=9d2010055d71&submit=true (I compared to the first revision in history, that had talos tests being run) It may be easier to land it and look closery at https://areweslimyet.com/ ...? ::: browser/devtools/framework/gDevTools.jsm @@ +16,5 @@ > Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js"); > + > +XPCOMUtils.defineLazyModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm"); > + > +let {Loader, Require, main, unload} = Cu.import("resource://gre/modules/commonjs/toolkit/loader.js", {}).Loader; Here is a case highlighting Joe's comment 11 about how much we should use destructuring. I have an opinion about this. We should not do that everywhere everytime, especially in big files and for symbols that are very generic. When you read the code and see "main()" or "unload()" being used, you have absolutely no idea what it actually does nor where is really comes from. You have to search in scope and eventually find it in module header, or not. You also have main and unload methods on Provider singletons... That's not your fault, this pattern ends up being very common and misleading. @@ +66,5 @@ > + return this._writeManifest(devtoolsDir).then((data) => { > + this._writeManifest(toolkitDir); > + }).then((r) => r, (err) => { > + Cu.reportError(err); > + }); Wait... what?! It took me some time to figure out that the comma was actual argument separator. Couldn't we do .then(null, Cu.reportError); ?

Comment on attachment 733160 [details] [diff] [review] WIP 2 Sorry for the flag reset... And nevermind my question about jsm and main.js. It appears that splinter review is really bad with file renaming :/

(In reply to Joe Walker [:jwalker] from comment #11) > > +lazyGetter(exports, "Toolbox", () => require("devtools/framework/toolbox").Toolbox); > > +lazyGetter(exports, "TargetFactory", () => require("devtools/framework/target").TargetFactory); > > And perhaps we could have an API like this: > > lazyModuleGetter(exports, "TargetFactory", "devtools/framework/target"); > > It seems like that's how it's used mostly. I have a somewhat sneaky reason for not doing that - the jetpack packager (which we might end up using in some cases) looks for require('..') to decide which things the package might use.

(In reply to Dave Camp (:dcamp) from comment #14) > I have a somewhat sneaky reason for not doing that - the jetpack packager > (which we might end up using in some cases) looks for require('..') to > decide which things the package might use. Duh, yes of course. That's what the GCLI loader does too.

https://tbpl.mozilla.org/?tree=Try&rev=ad0f96d03241 With the changes joe requested in v2 and a bit of rebasing.

Fixed netmonitor tests: https://tbpl.mozilla.org/?tree=Try&rev=38a1304d2be0

Grumble. Fix incoming. https://hg.mozilla.org/integration/fx-team/rev/394cd944912b

https://hg.mozilla.org/integration/fx-team/rev/643194ceabe4 In addition to fixing the failing xpcshell test, I noticed a small MaxHeap regression. I suspect it had to do with devtools.setProvider(), which was forcing lazy initialization of the toolbox and target factory. I got an r+ from harth and anton for the fix that you can see in the latest patch.

It looks like this caused a Trace Mallocs MaxHeap regression on multiple platforms.

(In reply to Matt Brubeck (:mbrubeck) from comment #23) > It looks like this caused a Trace Mallocs MaxHeap regression on multiple > platforms. (but smaller than the MaxHeap regression from the previous patch noted in comment 22, which seems to be partially fixed in the new version)

Backed out for the MaxHeap regression. I think I have a fix, but want to land it fresh.

Correct me if I'm wrong, but to avoid opening a bug like bug 812762 and that each l10n team working on central has to localize those strings twice, I think you should replace "Firefox" with &brandShortName before landing. (Hopefully, no one localized those strings before the backout, so you don't have to change the name of the entities http://mxr.mozilla.org/l10n-central/search?string=toolsManual&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=l10n-central )

Will do, thanks for the reminder.

Rebased and using observer service manually to avoid pulling in modules at startup.

A bit late but I have a couple of notes, not sure if you want a new bug for this. toolsBuiltinReloaded=Built-in tools loaded. This is inconsistent with all other strings where you used "builtin". You can fix this without renaming the entity since it's just like fixing a typo. toolsSrcdirReloaded=Tools loaded from %1$s. toolsBuiltinReloaded=Built-in tools loaded. toolsReloaded="Tools reloaded." Why this difference in structure with strings carrying similar sense?

(In reply to Francesco Lodolo [:flod] from comment #31) > A bit late but I have a couple of notes, not sure if you want a new bug for > this. Anyway, a new bug will be needed because &brandShortName has not been properly replaced in the UI. Dave, you took c#26 literally :)

(In reply to Théo Chevalier [:tchevalier] from comment #32) > Anyway, a new bug will be needed because &brandShortName has not been > properly replaced in the UI. Dave, you took c#26 literally :) I didn't understand your comment, then I realized that's a .properties file

Yeah, please file a followup. I really shouldn't be allowed to touch things.