Closed Bug 656668 Opened 13 years ago Closed 13 years ago

Create Export from GCLI project to JSM

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 10

People

(Reporter: jwalker, Assigned: jwalker)

References

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 file, 10 obsolete files)

Bug to hold patch of 3rd party code import form GCLI in jsm/commonjs form.
Attached patch Adds gcli.jsm (obsolete) — Splinter Review
Note - the case of the JSM is likely to change
Attached patch upload 2 (obsolete) — Splinter Review
depends on bug653140-require.patch
Status: NEW → ASSIGNED
Blocks: GCLI-API
is the first patch here (adds gcli.jsm) obsoleted by "upload 2"?
from the docs (https://github.com/joewalker/gcli/blob/master/docs/index.md)

Environments:
• Be usable in modern browsers plus IE8

Hopefully when that comes along support for other browsers can be handled via pluggable code.

GCLI should be:
• primarily for technical users.

Curious about what level of technical users this caters to. I would think because of the integrated help system that it is a somewhat "nicer" command line to deal with.

Parameter Types (In Writing Commands section):

deferred

Some explanation of what a deferred parameter type would be good. Maybe I missed it?

Is it possible to have sub-sub-commands?

Documentation is solid. R+ on that.

I'm a little hesitant about these tools that require dryice to make them. It's... a foreign build mechanism with some funny dependencies. I.e., node.js.

That may not be a problem as long as dryice doesn't decay and we get stuck with an orphaned version of these tools as a result. Unlikely, given the contributors list to dryice, but possible.

Onto the code...
License quibble:

Your license blocks still have First Last (email@address) instead of First Last <email@address>.

canon.js:

line 45+:
var Status = require('gcli/types').Status;

are these requires async? 

var types = require('gcli/types');
var BooleanType = require('gcli/types').BooleanType;

Could these be written as:

var types = require('gcli/types');
var Status = types.Status;
var BooleanType = types.BooleanType;

?

Are the extra requires needed? (I guess so if async)

line 318, canon.removeCommand:
    var name = typeof command === 'string' ?

Am I misreading, missing something or is command supposed to be commandOrName there?

canon.getCommands = function getCommands() {
    return Object.keys(commands).map(function(name) {
        return commands[name];
    }, this);
};

are the commands not keyed by name? could you not just return Object.keys() here or is this just to create a copy of that array? Maybe a comment to that effect for clarity.

contrast with:

* WARNING: This returns live data. Do not mutate the returned array.
*/
canon.getCommandNames = function getCommandNames() {
    return commandNames;
};

Here you're returning an array that can cause damage. Does this need to be a deepCopy instead or is there good reason for doing this?

ReportList.prototype.maxRequestLength = 100;

magic number!

canon looks pretty good

cli.js:

(same require questions as canon.js)

Inputter looks good to me.

Overall, this looks very nice, code-wise. On the module itself, I don't have any reservations about this, especially considering the require.js is going to be incorporated and the console dependency will be as well.

With some unittests that prove that adding a command and usage of those commands work, this should be acceptable.

nice work!
(In reply to comment #4)
> from the docs (https://github.com/joewalker/gcli/blob/master/docs/index.md)
> 
> Environments:
> • Be usable in modern browsers plus IE8
> 
> Hopefully when that comes along support for other browsers can be handled
> via pluggable code.

Currently I don't test on IE8 at all. I'm very tempted to drop support for it because then I can use Object.defineProperty to give me getters and setters, but Ace supports it...


> GCLI should be:
> • primarily for technical users.
> 
> Curious about what level of technical users this caters to. I would think
> because of the integrated help system that it is a somewhat "nicer" command
> line to deal with.

That's an interesting question, and one that I've resolved to not answer. I have a feeling that this could be useful in theory to maybe 50% of computer users, which is probably more than the current aim of 'technical users', but it's important to focus on a niche to start with.


> Parameter Types (In Writing Commands section):
> 
> deferred
> 
> Some explanation of what a deferred parameter type would be good. Maybe I
> missed it?

Here's what I added to the docs:

### Deferred types

Deferred types are needed when the type of some parameter depends on the type
of another parameter. For example:

    > set height 100
    > set name "Joe Walker"

We can achieve this as follows:

    gcli.addCommand({
      name: 'set',
      params: [
        {
          name: 'setting',
          type: { name: 'selection', values: [ 'height', 'name' ] }
        },
        {
          name: 'value',
          type: {
            name: 'deferred',
            defer: function() { ... }
          }
        }
      ],
      ...
    });

Several details are left out of this example, like how the defer function knows
what the current setting is. See the ``pref`` command in Ace for an example.


> Is it possible to have sub-sub-commands?

Yes, or at least yes in theory, I'm not sure it's properly tested. Bug 663081


> Documentation is solid. R+ on that.
> 
> I'm a little hesitant about these tools that require dryice to make them.
> It's... a foreign build mechanism with some funny dependencies. I.e.,
> node.js.

True, however it's not complex code, so it wouldn't be a lot to re-write, and it's hard to imagine a tool without some sort of annoying dependency. The previous tool was RequireJS, which needed Rhino and therefore Java. Dryice parses Javascript, so writing it in Python would be an exercise in self-torture. Also node is quickly becoming standard fare.


> That may not be a problem as long as dryice doesn't decay and we get stuck
> with an orphaned version of these tools as a result. Unlikely, given the
> contributors list to dryice, but possible.
> 
> Onto the code...
(In reply to comment #5)
> License quibble:
> 
> Your license blocks still have First Last (email@address) instead of First
> Last <email@address>.

Yes. Bug 659686 is about re-licensing, I plan to fix the headers at that point.


> canon.js:
> 
> line 45+:
> var Status = require('gcli/types').Status;
> 
> are these requires async? 
> 
> var types = require('gcli/types');
> var BooleanType = require('gcli/types').BooleanType;
> 
> Could these be written as:
> 
> var types = require('gcli/types');
> var Status = types.Status;
> var BooleanType = types.BooleanType;
> 
> ?
> 
> Are the extra requires needed? (I guess so if async)

They are semi-sync. Clearly as they are returning something, the actual call is synchronous. In theory however, since this runs in a browser, and this could require a new script tag, they can't be properly sync. So the job of the module loader is to look for the require lines and make sure that before this module is loaded, that we can provide all the answers.

It's possible that the first call to require('gcli/types') could do work (i.e. load that module) but the spec says that the second call MUST return the same as the first. So your re-write is functionally equivalent, it just takes up more space.


> line 318, canon.removeCommand:
>     var name = typeof command === 'string' ?
> 
> Am I misreading, missing something or is command supposed to be
> commandOrName there?

Good spot. Thanks.


> canon.getCommands = function getCommands() {
>     return Object.keys(commands).map(function(name) {
>         return commands[name];
>     }, this);
> };
> 
> are the commands not keyed by name? could you not just return Object.keys()
> here or is this just to create a copy of that array? Maybe a comment to that
> effect for clarity.

commands is an object not an array, so what I really want here is Object.values(commands), but that doesn't exist.
Comment added.


> contrast with:
> 
> * WARNING: This returns live data. Do not mutate the returned array.
> */
> canon.getCommandNames = function getCommandNames() {
>     return commandNames;
> };
> 
> Here you're returning an array that can cause damage. Does this need to be a
> deepCopy instead or is there good reason for doing this?


Good point. Now it says

/**
 * Get an array containing the names of the registered commands.
 */
canon.getCommandNames = function getCommandNames() {
    return commandNames.slice(0);
};


> ReportList.prototype.maxRequestLength = 100;
> 
> magic number!

I'm conflicted on this - I could add some sort of a  definition for this, but I'm not sure it would make the code any clearer would it? In a way this is the definition because it's used a few lines lower down in addReport()


> canon looks pretty good
> 
> cli.js:
> 
> (same require questions as canon.js)
> 
> Inputter looks good to me.
> 
> Overall, this looks very nice, code-wise. On the module itself, I don't have
> any reservations about this, especially considering the require.js is going
> to be incorporated and the console dependency will be as well.
> 
> With some unittests that prove that adding a command and usage of those
> commands work, this should be acceptable.
> 
> nice work!

There are unit tests in gclitest, which cover those cases.
Review/feedback comments:

Generally JavaScript code in Mozilla's codebase adheres to the following practice:

- 2 spaces for indentation. GCLI uses 4 spaces.
- function arguments have the "a" prefix.
- functions are named even if they are methods.
- license headers need to follow the tri-license boilerplate more closely. I see you have Bug 659686, so this does not really apply.

I am not sure how much these weigh in an official review, but the more we align with the codebase, the better it is. I believe some of the reviewers might want some changes like these.


docs/index.md:

- In User Guide > Accessibility ... that doesn't really tell much about the accessibility of the UI. Do you use ARIA? Did you test it with a screen reader? And so on.

- Array types: "There can only be one ungrouped parameters with an array type, and it must be at the end of the list of parameters. This avoids confusion as to which parameter and argument should be assigned."

I find it confusing "to which parameter and argument". Could you please elaborate? The explanation is not really clear to me.


gcli/index.js:

- You have several TODOs that need to be addressed. You need to fix those things or make bug reports for each of them, and add the bug numbers to the comments.

(this applies to all files where you have TODOs)
(In reply to comment #8)
> Review/feedback comments:
> 
> Generally JavaScript code in Mozilla's codebase adheres to the following
> practice:
> 
> - 2 spaces for indentation. GCLI uses 4 spaces.
> - function arguments have the "a" prefix.
> - functions are named even if they are methods.
> - license headers need to follow the tri-license boilerplate more closely. I
> see you have Bug 659686, so this does not really apply.
> 
> I am not sure how much these weigh in an official review, but the more we
> align with the codebase, the better it is. I believe some of the reviewers
> might want some changes like these.

FWIW, GCLI is truly an external project (similar to Orion). I don't think it makes sense to try to apply Mozilla code conventions to an external project.
I find inconsistency in using single or double quotes for strings, across different files in GCLI.

There is some code commented out across different files. Maybe you can clean it up?

canon.js:

- lines 91 and 96: you don't make use of the this variable in the loop functions. (in the Command constructor)

- line 99, function Command(): you do not tell why all default params need to come before param groups.

- lines 113 and 117: inconsistency between "} else {" and "}\nelse {". Please make sure the entire codebase uses a consistent code style.

- You seem to inconsistently use strict value checks versus non-strict value checks. For example, see lines 160 and 174.

util.js:

- line 89: why do you use handler.apply() not call()?

types.js:

- line 719: please fix the comment for the DeferredType constructor.

argument.js:

- again, a couple of TODOs.

gcli/index.js:

- does require(ui/field) which is unused.

gcli/ui/index.js:

- does require() a few modules that go unused, for the popup, the menu and so on.

build/prefix-gcli.js:

- the introductory comment could use some links to dryice, requirejs, and other projects named. I would think of it as a minimal README, for someone who's new.

- fmt() could have only one return. I've got used to see code written such that it doesn't "spam" a function with too many returns.

The same holds true for other functions in the file, like stringify()

... more comments to come ...
(In reply to comment #10)
> I find inconsistency in using single or double quotes for strings, across
> different files in GCLI.
> 
> There is some code commented out across different files. Maybe you can clean
> it up?
> 
> canon.js:
> 
> - lines 91 and 96: you don't make use of the this variable in the loop
> functions. (in the Command constructor)

True, but are you arguing that I should take it out? It's there deliberately because the code in question is clearly part of 'this' (even though it doesn't access 'this'). Discovering that you've forgotten to bind is a common source of bugs. Since the code is clearly part of this, I like to make it actually part of this. I guess it's the principle of least surprise.


> - line 99, function Command(): you do not tell why all default params need
> to come before param groups.

Good point, comment added.


> - lines 113 and 117: inconsistency between "} else {" and "}\nelse {".
> Please make sure the entire codebase uses a consistent code style.

It should be with the \n everywhere. Thanks.


> - You seem to inconsistently use strict value checks versus non-strict value
> checks. For example, see lines 160 and 174.
> 
> util.js:

In Bespin we generally used strict equals everywhere, except for (foo == null) which is the same as saying (foo === null || foo === undefined).


> - line 89: why do you use handler.apply() not call()?

No good reason. Fixed.

 
> types.js:
>
> - line 719: please fix the comment for the DeferredType constructor.

:) That was a confusing comment, technically parsable but only if you knew where to put the quotation marks. It now reads: A type for "we don't know right now, but hope to soon".


> argument.js:
> 
> - again, a couple of TODOs.
> 
> gcli/index.js:
> 
> - does require(ui/field) which is unused.
> 
> gcli/ui/index.js:
> 
> - does require() a few modules that go unused, for the popup, the menu and
> so on.

Good point. I'm a little unsure how we should handle this, it really depends on how we import the modules into Firefox. There is probably an 'ideal' solution that could be more engineering that we need, I'll see how we go.


> build/prefix-gcli.js:
> 
> - the introductory comment could use some links to dryice, requirejs, and
> other projects named. I would think of it as a minimal README, for someone
> who's new.

Good point. Done


> - fmt() could have only one return. I've got used to see code written such
> that it doesn't "spam" a function with too many returns.
> 
> The same holds true for other functions in the file, like stringify()
> 
> ... more comments to come ...

Thanks.
Don't spend too much time on that bit of code - it's already being reviewed by Mossop.

I'm not a big fan of the 'single return' meme because it leads to deeply nested code, but more importantly when you're understanding the code, it prevents you from saying "right from here on in, I can assume this", you can only say "right for this block, I can assume this". Anything to reduce the things that we have to hold in our heads as we understand code is a good thing in my book.

Thanks.
General comment about jsdoc comments: as far as I know it would be good for consistency to use periods when you end function descriptions, parameter descriptions and return descriptions. Current code has some inconsistencies wrt. the use of periods.


prefix-gcli.jsm:

- line 105: you use the "a" prefix for the first three fmt() function arguments only. Also, the options argument is not described in the jsdoc above the function.

- line 166: the stringify() function:

  var str = aThing.toString().replace(/[\n\r]/g, " ").replace(/ +/g, " ");

why not a single .replace(/\s+/g, " ") ?

- some functions use the "a" prefix for arguments, some don't. Please aim for consistency.

- line 308: in the formatTrace() function the forEach() call doesn't need the scope (this).

- line 459: return undefined in Domain.require(). Isn't return; the same?

inputter.js:

- line 138: in getDimensionRect() ... any specific reason why you use getClientRects()[0]? I believe you would better use getBoundingClientRect().

getClientRects() gives you a list client rects for each line of the text, think of an inline element that spans across multiple lines. Gecko gives you a single rect if it's a block element, or multiple rects if it's inline. getBoundingClientRect() gives you the union of the client rects.

Also, judging by where you use getDimensionRect() ... I believe you want getBoundingClientRect() but I might be wrong.

- line 156: please use constants for the key codes.

(it seems constants for key codes are needed in multiple places within the file)

- line 199: the idea there sounds like a todo/fixme, also the code is commented out. Why not put it into a bug report?

- line 317: Completer.decorate(). It seems to me I wouldn't like this. I would prefer the completer element to have some class names and I can control the way it is styled from CSS. I don't think I like the idea of having code copy styles from the input, to decide how the completer should look like, nor should it include hard-coded visual styling - for example I see hard-coded background colors and paddings, so on.

Thoughts?


(... more comments to come ...)
(In reply to comment #8)
> docs/index.md:
> 
> - In User Guide > Accessibility ... that doesn't really tell much about the
> accessibility of the UI. Do you use ARIA? Did you test it with a screen
> reader? And so on.

Good point, fixed.


> - Array types: "There can only be one ungrouped parameters with an array
> type, and it must be at the end of the list of parameters. This avoids
> confusion as to which parameter and argument should be assigned."
> 
> I find it confusing "to which parameter and argument". Could you please
> elaborate? The explanation is not really clear to me.

"to which parameter _an_ argument ..."


> gcli/index.js:
> 
> - You have several TODOs that need to be addressed. You need to fix those
> things or make bug reports for each of them, and add the bug numbers to the
> comments.
> 
> (this applies to all files where you have TODOs)

Will do.

I've also added a section to the documentation about coding conventions.

Thanks.
(In reply to comment #11)
> I'm not a big fan of the 'single return' meme because it leads to deeply
> nested code

I concur wholeheartedly!
General comments (discussed on Skype):

- The scope of the project is quite big/wide. I believe we should focus on the Firefox Developer tools environment. Albeit, having this work on the web, in other web apps, is almost "free" - comes at almost no additional work.

- Browser compatibility should not drag us. Maybe the code should really focus on the latest/modern browsers only.

- Some of the methods take a wide range of objects as arguments. For example canon addCommand() takes an object that is really flexible and I would say "prone to errors". "Too much variety" if I may say. I would imagine different methods for different syntaxes of adding commands (see the docs about the three syntaxes of commands), and more formal definitions of the parameters. I mean, for each parameter type you should clearly define the expected object properties and so on. Now it all seems like "too flexible" and "easy to break". Such flexibility is really good but I believe it might not scale in the future.

A clearer and better defined approach to this stuff would be welcome. (most of it might just be a problem of docs actually)

- The code has its own custom events mechanism (in gcli/util.js). Panorama has one as well, Orion has one as well, Ace has one, every project has one. We should have something lightweight and common to all Firefox projects. (of course, this is really beyond the purpose of this bug, but it's worth mentioning)


inputter.js:

line 98 (the Inputter constructor function):

    if (options.completer == null) {
        options.completer = new Completer(options);
    }
    else if (typeof options.completer === 'function') {
        options.completer = new options.completer(options);
    }

See, options.completer == null relies on options.completer (which is undefined) to be equal to null (without a strict type check), but then you go on to check if it's typeof "function", but you do a strict type check on the string. To me these things seem inconsistent.

How I would write that:

    if (!options.completer) {
        // you don't care if it's === null or === undefined or false.
        options.completer = new Completer(options);
    }
    else if (typeof options.completer == 'function') {
        // a strict type check here has no added benefit.
        options.completer = new options.completer(options);
    }

When I see you check for null I would expect you look for strict check, but you don't, you actually rely on the non-strict check behavior. Somehow seeing null there makes me think of a strict check.

(just pointing out how this kind of code creates confusion. I see this in several files. I know, it's just a matter of being used with a different style ;) )

util.js:

- some parts of the file use 2 spaces indentation, others use 4 spaces.
- magic numbers, please use constants.


This is review is almost complete. I will look into the code a bit more today and provide you with the final comments.
(In reply to comment #12)
> prefix-gcli.jsm:
> 
> - line 105: you use the "a" prefix for the first three fmt() function
> arguments only. Also, the options argument is not described in the jsdoc
> above the function.

Fixed, here and in one other place.


> - line 166: the stringify() function:
> 
>   var str = aThing.toString().replace(/[\n\r]/g, " ").replace(/ +/g, " ");
> 
> why not a single .replace(/\s+/g, " ") ?

Done.


> - line 308: in the formatTrace() function the forEach() call doesn't need
> the scope (this).

Agreed. Fixed.


> - line 459: return undefined in Domain.require(). Isn't return; the same?

It is, however 'return undefined' makes it clear to developers and to code checkers that we a specifically returning nothing as opposed to forgetting to return something.


> inputter.js:
> 
> - line 138: in getDimensionRect() ... any specific reason why you use
> getClientRects()[0]? I believe you would better use getBoundingClientRect().
> 
> getClientRects() gives you a list client rects for each line of the text,
> think of an inline element that spans across multiple lines. Gecko gives you
> a single rect if it's a block element, or multiple rects if it's inline.
> getBoundingClientRect() gives you the union of the client rects.
> 
> Also, judging by where you use getDimensionRect() ... I believe you want
> getBoundingClientRect() but I might be wrong.

I think you're right. Will test and check.


> - line 156: please use constants for the key codes.
> 
> (it seems constants for key codes are needed in multiple places within the
> file)

Bug 664136


> - line 199: the idea there sounds like a todo/fixme, also the code is
> commented out. Why not put it into a bug report?

Done. Bug 664135


> - line 317: Completer.decorate(). It seems to me I wouldn't like this. I
> would prefer the completer element to have some class names and I can
> control the way it is styled from CSS. I don't think I like the idea of
> having code copy styles from the input, to decide how the completer should
> look like, nor should it include hard-coded visual styling - for example I
> see hard-coded background colors and paddings, so on.
> 
> Thoughts?

For the browser case this is fairly much a no-brainer. There's no point in making the user do all the CSS work.
I do wonder if we're not just writing code for idealistic reasons here? What is the actual benefit of doing this?

Thanks.
(In reply to comment #16)
> > - line 317: Completer.decorate(). It seems to me I wouldn't like this. I
> > would prefer the completer element to have some class names and I can
> > control the way it is styled from CSS. I don't think I like the idea of
> > having code copy styles from the input, to decide how the completer should
> > look like, nor should it include hard-coded visual styling - for example I
> > see hard-coded background colors and paddings, so on.
> > 
> > Thoughts?
> 
> For the browser case this is fairly much a no-brainer. There's no point in
> making the user do all the CSS work.
> I do wonder if we're not just writing code for idealistic reasons here? What
> is the actual benefit of doing this?

Hm, the designer needs to style the input anyhow. You just give him the freedom to style the completer as well.

I was just suggesting this - it's not a blocking review request, hehe. I think I would've went the route of putting the code in CSS, not in JS. That's all.
(In reply to comment #15)
> General comments (discussed on Skype):
> 
> - The scope of the project is quite big/wide. I believe we should focus on
> the Firefox Developer tools environment. Albeit, having this work on the
> web, in other web apps, is almost "free" - comes at almost no additional
> work.
> 
> - Browser compatibility should not drag us. Maybe the code should really
> focus on the latest/modern browsers only.

I mostly agree - that's the way the code is currently being written, however Ace has broader goals - back to IE7. At some stage it might make sense to go and do extra testing on older browsers, and I'm not taking out existing compat code for that reason.

Chrome support has been almost entirely free (at least I can't remember having to do significant work to make it work in chrome)


> - Some of the methods take a wide range of objects as arguments. For example
> canon addCommand() takes an object that is really flexible and I would say
> "prone to errors". "Too much variety" if I may say. I would imagine
> different methods for different syntaxes of adding commands (see the docs
> about the three syntaxes of commands), and more formal definitions of the
> parameters. I mean, for each parameter type you should clearly define the
> expected object properties and so on. Now it all seems like "too flexible"
> and "easy to break". Such flexibility is really good but I believe it might
> not scale in the future.
> 
> A clearer and better defined approach to this stuff would be welcome. (most
> of it might just be a problem of docs actually)

Interesting comment, thanks. I'll give that code a careful checkout.
Bug 664153


> - The code has its own custom events mechanism (in gcli/util.js). Panorama
> has one as well, Orion has one as well, Ace has one, every project has one.
> We should have something lightweight and common to all Firefox projects. (of
> course, this is really beyond the purpose of this bug, but it's worth
> mentioning)

Agreed.


> inputter.js:
> 
> line 98 (the Inputter constructor function):
> 
>     if (options.completer == null) {
>         options.completer = new Completer(options);
>     }
>     else if (typeof options.completer === 'function') {
>         options.completer = new options.completer(options);
>     }
> 
> See, options.completer == null relies on options.completer (which is
> undefined) to be equal to null (without a strict type check), but then you
> go on to check if it's typeof "function", but you do a strict type check on
> the string. To me these things seem inconsistent.
>
>
> How I would write that:
> 
>     if (!options.completer) {
>         // you don't care if it's === null or === undefined or false.
>         options.completer = new Completer(options);
>     }
>     else if (typeof options.completer == 'function') {
>         // a strict type check here has no added benefit.
>         options.completer = new options.completer(options);
>     }
> 
> When I see you check for null I would expect you look for strict check, but
> you don't, you actually rely on the non-strict check behavior. Somehow
> seeing null there makes me think of a strict check.
> 
> (just pointing out how this kind of code creates confusion. I see this in
> several files. I know, it's just a matter of being used with a different
> style ;) )

I'm sure you're aware that null == undefined

The trouble is that, when executed on user input, (!options.completer) could go in many unexpected places. (options.completer == null) is a simple way of saying "is there anything there". Now I agree that (options.completer === undefined) is probably more technically correct, but then again maybe (!("completer" in options)) is even stricter!

Personal opinion: (options.completer == null) is the best way of saying "did the user intend to give us anything".


> util.js:
> 
> - some parts of the file use 2 spaces indentation, others use 4 spaces.
> - magic numbers, please use constants.
> 
> 
> This is review is almost complete. I will look into the code a bit more
> today and provide you with the final comments.

Constants - raised bug as noted elsewhere
Space - fixed.

Thanks,
More comments:

- Packaging of CSS and PNG files into the JSM. This is not really the usual approach in the Mozilla codebase. Styling should be in a separate file, skinnable.

Please cleanup the jsm so that resources are not included.

- The Requisition object name is a bit confusing given it's a bit of an uncommon word for non-native British English speakers.

- The ReportList object is also confusingly named, and that of the RequestsView object as well (which lives in request_view.js ... notice the minor naming difference).

(the above comments have been discussed on Skype, just mentioning them here for posterity)

- domtemplate.js uses a different coding style when compared to gcli, also different jsdoc styling.


Overall the codebase of GCLI is quite solid. Really good work!

r+ on the code. Once the comments are addressed you can ask for an official review.
(In reply to comment #19)
> More comments:
> 
> - Packaging of CSS and PNG files into the JSM. This is not really the usual
> approach in the Mozilla codebase. Styling should be in a separate file,
> skinnable.
> 
> Please cleanup the jsm so that resources are not included.

Bug 664188


> - The Requisition object name is a bit confusing given it's a bit of an
> uncommon word for non-native British English speakers.

Added explanation and links to the docs.


> - The ReportList object is also confusingly named, and that of the
> RequestsView object as well (which lives in request_view.js ... notice the
> minor naming difference).

Bug 664191


> (the above comments have been discussed on Skype, just mentioning them here
> for posterity)
> 
> - domtemplate.js uses a different coding style when compared to gcli, also
> different jsdoc styling.

True - I'm avoiding changing that too much because it might need to be shared in some way.


> Overall the codebase of GCLI is quite solid. Really good work!
> 
> r+ on the code. Once the comments are addressed you can ask for an official
> review.

Thanks.
Attached patch upload 3 (obsolete) — Splinter Review
Dave, Might you be able to review the rest of the GCLI?

This patch adds the implementation of the GCLI as exported from https://github.com/mozilla/gcli/

It has had a fairly decent going over by Mihai, Robcce and Nick (Fitzgerald) who is now working on the project with me.

Please could you keep us informed about progress so we can avoid changing files as or after review.


Some extra info:

https://github.com/joewalker/gcli/blob/master/docs/index.md is a handy thing to know about - it's mostly an embedders guide, but at the bottom there is an 'about the code' section that's worth reading.

I suggest you don't work from the patch attached, but instead from the files in github - links provided.

I'd probably start with the canon:
https://github.com/joewalker/gcli/blob/master/lib/gcli/canon.js

And then look at cli.js which is the meat of the whole thing:
https://github.com/joewalker/gcli/blob/master/lib/gcli/cli.js

The 2 top level files which package this up for firefox are:
https://github.com/joewalker/gcli/blob/master/lib/gcli/index.js
https://github.com/joewalker/gcli/blob/master/lib/gcli/ui/start/firefox.js

Then the other files in the lib dir:
https://github.com/joewalker/gcli/tree/master/lib/gcli

From the UI section, I'd just look at inputter and history - the other files are not going in, in this release.
https://github.com/joewalker/gcli/blob/master/lib/gcli/ui/inputter.js
https://github.com/joewalker/gcli/blob/master/lib/gcli/ui/history.js

The only other file that's going in is
https://github.com/joewalker/gcli/blob/master/lib/gcli/ui/domtemplate.js
I suspect that this file will have been reviewed elsewhere. I am checking.

I think that's plenty to be going on with!

Thanks,
Assignee: nobody → jwalker
Attachment #532164 - Attachment is obsolete: true
Attachment #534480 - Attachment is obsolete: true
Attachment #541477 - Flags: review?(dtownsend)
If robcee and mihai have done code review on this then I don't think I need to. is there any API change going on, if so could you give me a short summary of it?
Will do.

Can we take it that this level of review is OK for the future? i.e internal review within devtools, and API level change reviewed by you?
(In reply to comment #23)
> Will do.
> 
> Can we take it that this level of review is OK for the future? i.e internal
> review within devtools, and API level change reviewed by you?

We're working on formalising the devtools review process, till then lets keep making the requests as a double check. API change will always need superreview from someone on the superreview list, like me.
Comment on attachment 541477 [details] [diff] [review]
upload 3

Please request sr if you have API changes for me to look at.
Attachment #541477 - Flags: review?(dtownsend)
Whiteboard: [minotaur]
Attached patch upload 4 (obsolete) — Splinter Review
Updates to GCLI r=fitzgen
Attachment #541477 - Attachment is obsolete: true
Should have added: this patch update does not change any APIs
Attached patch upload 5 (obsolete) — Splinter Review
This patch has been reviewed in the various pull requests in the GCLI project (See https://github.com/mozilla/gcli/pulls click 'Closed')

As previously agreed, this patch does not change any APIs.
Attachment #551025 - Attachment is obsolete: true
Blocks: 656666
Whiteboard: [minotaur]
Attached patch upload 6 (obsolete) — Splinter Review
No changed APIs.

I'm probably going to remove the add/removeCommand functions and making them internal until we've got more experience with how commands work. It's not that they have changed much in themselves, but more the expectation that commands will need more support to be properly useful.
Attachment #555696 - Attachment is obsolete: true
Attached patch upload 7 (obsolete) — Splinter Review
Minor changes to go with latest patch to 665666
Attachment #561865 - Attachment is obsolete: true
Attached patch upload 8 (obsolete) — Splinter Review
No changed API
Attachment #562060 - Attachment is obsolete: true
Depends on: 684958
Attached patch upload 9 (obsolete) — Splinter Review
No changed API.
Reviewed in Github by robcee, mihai, fitgen, mikeratclife, jwalker
Attachment #562856 - Attachment is obsolete: true
Comment on attachment 563883 [details] [diff] [review]
upload 9

Review of attachment 563883 [details] [diff] [review]:
-----------------------------------------------------------------

A quick drive by comment. See below.

(thanks to Victor Porof!)

::: browser/devtools/webconsole/gcli.jsm
@@ +169,5 @@
> +  function fmt(aStr, aMaxLen, aMinLen, aOptions) {
> +    if (aMinLen == undefined) {
> +      aMinLen = aMaxLen;
> +    }
> +    if (aStr == null) {

These two checks need to use === or typeof.

@@ +268,5 @@
> +    if (aThing == null) {
> +      return "null\n";
> +    }
> +
> +    if (aThing == undefined) {

Same as above.
(In reply to Mihai Sucan [:msucan] from comment #33)
> Comment on attachment 563883 [details] [diff] [review] [diff] [details] [review]
> upload 9
> 
> Review of attachment 563883 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> A quick drive by comment. See below.
> 
> (thanks to Victor Porof!)
> 
> ::: browser/devtools/webconsole/gcli.jsm
> @@ +169,5 @@
> > +  function fmt(aStr, aMaxLen, aMinLen, aOptions) {
> > +    if (aMinLen == undefined) {
> > +      aMinLen = aMaxLen;
> > +    }
> > +    if (aStr == null) {
> 
> These two checks need to use === or typeof.

Why?
I think that (aMinLen == undefined) perhaps should be (aMinLen == null) in case someone passes null in, but that sounds like an edge case.


> @@ +268,5 @@
> > +    if (aThing == null) {
> > +      return "null\n";
> > +    }
> > +
> > +    if (aThing == undefined) {
> 
> Same as above.

I'm thinking they should both be ===. But this perhaps depends on your answer to #1.
Thanks for the comment.
> 
> Why?
> I think that (aMinLen == undefined) perhaps should be (aMinLen == null) in
> case someone passes null in, but that sounds like an edge case.
>

Because undefined == null.
(In reply to Victor Porof from comment #35)
> > 
> > Why?
> > I think that (aMinLen == undefined) perhaps should be (aMinLen == null) in
> > case someone passes null in, but that sounds like an edge case.
> >
> 
> Because undefined == null.

Right, but we're detecting a missing parameter here, in which case it will be undefined. I'm not sure we care what happens when someone passes null in, but my guess is that we should treat is as though it was undefined.

What can you see going wrong?
(In reply to Joe Walker from comment #36)
> Right, but we're detecting a missing parameter here, in which case it will
> be undefined. I'm not sure we care what happens when someone passes null in,
> but my guess is that we should treat is as though it was undefined.
> 
> What can you see going wrong?

Detecting missing parameters with == undefined is risky. Think of this case: you have the object

myObject = {
  someProperty: null
}

and you pass myObject.someProperty as a param to the function. This doesn't mean a missing parameter, but a param with a null value, which is incorrectly interpreted as undefined. I think it's a good practice to make the testing more thorough, even if some scenarios seem less likely than others, by using "undefined" === typeof param and param === null.

> 
> > @@ +268,5 @@
> > > +    if (aThing == null) {
> > > +      return "null\n";
> > > +    }
> > > +
> > > +    if (aThing == undefined) {
> > 
> > Same as above.
> 
> I'm thinking they should both be ===. But this perhaps depends on your
> answer to #1.

In this case, if aThing is passed as undefined, the return value of the function is "null\n" and the second if will never have a true condition.
(In reply to Victor Porof from comment #37)
> (In reply to Joe Walker from comment #36)
> > Right, but we're detecting a missing parameter here, in which case it will
> > be undefined. I'm not sure we care what happens when someone passes null in,
> > but my guess is that we should treat is as though it was undefined.
> > 
> > What can you see going wrong?
> 
> Detecting missing parameters with == undefined is risky. Think of this case:
> you have the object
> 
> myObject = {
>   someProperty: null
> }
> 
> and you pass myObject.someProperty as a param to the function. This doesn't
> mean a missing parameter, but a param with a null value, which is
> incorrectly interpreted as undefined. I think it's a good practice to make
> the testing more thorough, even if some scenarios seem less likely than
> others, by using "undefined" === typeof param and param === null.

There are 2 issues here: == vs === and the case in question.

Normally I prefer ===, however the Mozilla way is to generally use ==.
The argument seems to be "you need to understand the types anyway, so === doesn't fix all cases." Personally I'm unconvinced, so you should ask msucan to defend it rather than me! However my thoughts here aren't particularly relevant. == is the moz way in general.

To the argument in question, if someone was constructing an object with a minLen property to pass to this function, then I would take 'null' to mean there isn't a minLen value, and to use the default. Likewise if someone wanted to pass an options object to fmt() and yet use the default minLen, then I would expect them to explicitly use null.
In short, I see null and undefined as part of the contract - they both mean 'use the default'. Perhaps the bug is that I should document this.


> > > @@ +268,5 @@
> > > > +    if (aThing == null) {
> > > > +      return "null\n";
> > > > +    }
> > > > +
> > > > +    if (aThing == undefined) {
> > > 
> > > Same as above.
> > 
> > I'm thinking they should both be ===. But this perhaps depends on your
> > answer to #1.
> 
> In this case, if aThing is passed as undefined, the return value of the
> function is "null\n" and the second if will never have a true condition.

Right, so we agree that this should be ===.
(In reply to Joe Walker from comment #38)
> Normally I prefer ===, however the Mozilla way is to generally use ==.
> The argument seems to be "you need to understand the types anyway, so ===
> doesn't fix all cases." Personally I'm unconvinced, so you should ask msucan
> to defend it rather than me! However my thoughts here aren't particularly
> relevant. == is the moz way in general.

The general way in Mozilla's codebase is to use == when you don't need strict comparisons. In your case the code needs it. There's no strict rule against strict equality checks. :)
(In reply to Mihai Sucan [:msucan] from comment #39)
> (In reply to Joe Walker from comment #38)
> > Normally I prefer ===, however the Mozilla way is to generally use ==.
> > The argument seems to be "you need to understand the types anyway, so ===
> > doesn't fix all cases." Personally I'm unconvinced, so you should ask msucan
> > to defend it rather than me! However my thoughts here aren't particularly
> > relevant. == is the moz way in general.
> 
> The general way in Mozilla's codebase is to use == when you don't need
> strict comparisons. In your case the code needs it. There's no strict rule
> against strict equality checks. :)

Hence my careful use of the word 'generally'.

Also my code does not need strict checks here. In fact making it strict would make it harder to use. I want to use the default (i.e. aMaxLen) when aMinLen is either null or undefined.
(In reply to Joe Walker from comment #40)
> (In reply to Mihai Sucan [:msucan] from comment #39)
> > (In reply to Joe Walker from comment #38)
> > > Normally I prefer ===, however the Mozilla way is to generally use ==.
> > > The argument seems to be "you need to understand the types anyway, so ===
> > > doesn't fix all cases." Personally I'm unconvinced, so you should ask msucan
> > > to defend it rather than me! However my thoughts here aren't particularly
> > > relevant. == is the moz way in general.
> > 
> > The general way in Mozilla's codebase is to use == when you don't need
> > strict comparisons. In your case the code needs it. There's no strict rule
> > against strict equality checks. :)
> 
> Hence my careful use of the word 'generally'.
> 
> Also my code does not need strict checks here. In fact making it strict
> would make it harder to use. I want to use the default (i.e. aMaxLen) when
> aMinLen is either null or undefined.

For the fmt function, if aMinLen being equal to 0 doesn't require any special treatment, you could just check for !aMinLen to avoid making the code more complicated. However, for the second case with the log function, using === is necessary when distinguishing null and undefined values.
(In reply to Victor Porof from comment #41)
> (In reply to Joe Walker from comment #40)
> > (In reply to Mihai Sucan [:msucan] from comment #39)
> > > (In reply to Joe Walker from comment #38)
> > > > Normally I prefer ===, however the Mozilla way is to generally use ==.
> > > > The argument seems to be "you need to understand the types anyway, so ===
> > > > doesn't fix all cases." Personally I'm unconvinced, so you should ask msucan
> > > > to defend it rather than me! However my thoughts here aren't particularly
> > > > relevant. == is the moz way in general.
> > > 
> > > The general way in Mozilla's codebase is to use == when you don't need
> > > strict comparisons. In your case the code needs it. There's no strict rule
> > > against strict equality checks. :)
> > 
> > Hence my careful use of the word 'generally'.
> > 
> > Also my code does not need strict checks here. In fact making it strict
> > would make it harder to use. I want to use the default (i.e. aMaxLen) when
> > aMinLen is either null or undefined.
> 
> For the fmt function, if aMinLen being equal to 0 doesn't require any
> special treatment, you could just check for !aMinLen to avoid making the
> code more complicated.

But I'd rather an exception if someone passed in "".

> However, for the second case with the log function,
> using === is necessary when distinguishing null and undefined values.

Which I've done, as noted above (twice!)

Let's also remember that this is only a debug function.
Thanks.
Attached patch upload 10 (obsolete) — Splinter Review
Attachment #563883 - Attachment is obsolete: true
Attached patch upload 11Splinter Review
Alter localization string in line with bug 687851
No API change
Attachment #565603 - Attachment is obsolete: true
Attachment #566523 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9247eee791fd
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: