Currently we export gcli.addCommand() and gcli.removeCommand() directly. We should review the commands we ship with to ensure that those functions are sufficint for most uses. We could either expose more, or hide those if we decide to. Currently we export most things one way or another via 'gcli._internal'. What constitutes a 'published API' and does this contravene that? Should we change _internal to something more obvious _internalUseOnly or _imOnlyPlayingAndPromiseNotToComplainIfThisBreaks?

The GCLI side of this had been reviewed by dcamp in https://github.com/mozilla/gcli/pull/37 This patch removes the parallel l10n code (which was going to get duplicated everywhere) from GcliCommands. It also strips out the [un]setDocument code as we've found a better way. Mihai - I've added you so you can check the changes to HUDService. We're going to add a JSM for debugging commands soon, and the command loading system was quite specific to GcliCommands. With this change, adding a new JSM containing commands is a single line change rather than a 7 line change. I would like to commit this patch without closing the bug.

Comment on attachment 568001 [details] [diff] [review] part 1, upload 1 Review of attachment 568001 [details] [diff] [review]: ----------------------------------------------------------------- Patch looks fine for me, but I am not sure that the changes in HUDService are really needed. Please see the comments below. The HUDService tests fail now. Giving r- for this reason. Looking forward for the updated patch. Thank you! ::: browser/devtools/webconsole/HUDService.jsm @@ +134,5 @@ > + * @returns an object containing the EXPORTED_SYMBOLS from all the command > + * modules. In general there is no reason when JSMs need to export symbols > + * except when they need the host environment to inform them of things like the > + * current window/document/etc. > + */ Nit: please use the jsdoc style of HUDService. @return instead of @returns and put the explanation on a new line. @@ +136,5 @@ > + * except when they need the host environment to inform them of things like the > + * current window/document/etc. > + */ > +function loadCommands() { > + var cmdObj = {}; s/var/let @@ +141,5 @@ > + > + Cu.import("resource:///modules/GcliCommands.jsm", cmdObj); > + > + return cmdObj; > +} Why not make cmdObj a lazy getter? This is the same as it was, with the difference that cmdObj is not a lazy getter and the plan is to allow more than one JSM to be loaded into cmdObj. Also I would prefer a better name for the global variable "cmdObj". Why rename away from GcliCommands? They are still GCLI commands, even if they come from multiple JSMs. Please explain - I am sure I am missing some pieces of the GCLI future. ;) Thanks!

Comment on attachment 568001 [details] [diff] [review] part 1, upload 1 r+ for the gcli stuff I reviewed elsewhere, not intended to override mihai's HUDService review.

(In reply to Mihai Sucan [:msucan] from comment #2) > Comment on attachment 568001 [details] [diff] [review] [diff] [details] [review] > part 1, upload 1 > > Review of attachment 568001 [details] [diff] [review] [diff] [details] [review]: > ----------------------------------------------------------------- > > Patch looks fine for me, but I am not sure that the changes in HUDService > are really needed. Please see the comments below. > > The HUDService tests fail now. Giving r- for this reason. > > Looking forward for the updated patch. Thank you! > > ::: browser/devtools/webconsole/HUDService.jsm > @@ +134,5 @@ > > + * @returns an object containing the EXPORTED_SYMBOLS from all the command > > + * modules. In general there is no reason when JSMs need to export symbols > > + * except when they need the host environment to inform them of things like the > > + * current window/document/etc. > > + */ > > Nit: please use the jsdoc style of HUDService. @return instead of @returns > and put the explanation on a new line. > > @@ +136,5 @@ > > + * except when they need the host environment to inform them of things like the > > + * current window/document/etc. > > + */ > > +function loadCommands() { > > + var cmdObj = {}; > > s/var/let > > @@ +141,5 @@ > > + > > + Cu.import("resource:///modules/GcliCommands.jsm", cmdObj); > > + > > + return cmdObj; > > +} > > Why not make cmdObj a lazy getter? Because we might not need to use the return value, on the other hand we might. Currently we don't have any commands that need to be hooked into the environment, but this is how they would get hooked in. Without any need to get hooked in, if we used lazy loading, the 'load' code would look like this: let ignore = cmdObj; Which is clearly insane. > This is the same as it was, with the difference that cmdObj is not a lazy > getter and the plan is to allow more than one JSM to be loaded into cmdObj. > > Also I would prefer a better name for the global variable "cmdObj". Why > rename away from GcliCommands? They are still GCLI commands, even if they > come from multiple JSMs. > > Please explain - I am sure I am missing some pieces of the GCLI future. ;) > Thanks!

Try build: https://tbpl.mozilla.org/?tree=Try&rev=b65f1a65c796

Comment on attachment 569379 [details] [diff] [review] part 1, upload 2 Review of attachment 569379 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for your explanation Joe! Patch looks good! All tests pass now.

Part 2 is on it's way. Reopening.