Last Comment Bug 693265 - Carefully review the exposed GCLI API
: Carefully review the exposed GCLI API
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 10
Assigned To: Joe Walker [:jwalker] (needinfo me or ping on irc)
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on:
Blocks: 683506 GCLI-SHIP
  Show dependency treegraph
 
Reported: 2011-10-10 03:46 PDT by Joe Walker [:jwalker] (needinfo me or ping on irc)
Modified: 2011-10-27 13:21 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1, upload 1 (13.48 KB, patch)
2011-10-19 03:45 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
dcamp: review+
mihai.sucan: review-
Details | Diff | Splinter Review
part 1, upload 2 (13.78 KB, patch)
2011-10-25 08:10 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
mihai.sucan: review+
Details | Diff | Splinter Review

Description Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-10 03:46:36 PDT
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?
Comment 1 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-19 03:45:16 PDT
Created attachment 568001 [details] [diff] [review]
part 1, upload 1

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 2 Mihai Sucan [:msucan] 2011-10-19 07:36:14 PDT
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 3 Dave Camp (:dcamp) 2011-10-19 10:09:20 PDT
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.
Comment 4 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-25 06:40:50 PDT
(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!
Comment 5 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-25 08:10:20 PDT
Created attachment 569379 [details] [diff] [review]
part 1, upload 2
Comment 6 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-25 08:58:47 PDT
Try build: https://tbpl.mozilla.org/?tree=Try&rev=b65f1a65c796
Comment 7 Mihai Sucan [:msucan] 2011-10-25 12:41:27 PDT
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.
Comment 8 Rob Campbell [:rc] (:robcee) 2011-10-26 07:27:45 PDT
https://hg.mozilla.org/integration/fx-team/rev/bfb669703e33
Comment 9 :Margaret Leibovic 2011-10-27 11:43:36 PDT
https://hg.mozilla.org/mozilla-central/rev/bfb669703e33
Comment 10 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-10-27 12:33:05 PDT
Part 2 is on it's way. Reopening.
Comment 11 Rob Campbell [:rc] (:robcee) 2011-10-27 13:21:02 PDT
https://hg.mozilla.org/mozilla-central/rev/bfb669703e33

Note You need to log in before you can comment on or make changes to this bug.