The default bug view has changed. See this FAQ.

Carefully review the exposed GCLI API

RESOLVED FIXED in Firefox 10

Status

()

Firefox
Developer Tools
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jwalker, Assigned: jwalker)

Tracking

unspecified
Firefox 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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?
Assignee: nobody → jwalker
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.
Attachment #568001 - Flags: review?(mihai.sucan)
Attachment #568001 - Flags: review?(dcamp)
Blocks: 683506
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!
Attachment #568001 - Flags: review?(mihai.sucan) → review-

Comment 3

6 years ago
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.
Attachment #568001 - Flags: review?(dcamp) → 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!
Created attachment 569379 [details] [diff] [review]
part 1, upload 2
Attachment #568001 - Attachment is obsolete: true
Attachment #569379 - Flags: review?(mihai.sucan)
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.
Attachment #569379 - Flags: review?(mihai.sucan) → review+
https://hg.mozilla.org/integration/fx-team/rev/bfb669703e33
Status: NEW → ASSIGNED
Whiteboard: [fixed-in-fx-team]

Comment 9

6 years ago
https://hg.mozilla.org/mozilla-central/rev/bfb669703e33
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 10
Part 2 is on it's way. Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/bfb669703e33
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.