900114 - GCLI rollup for bugs 781856, 884015, 789884, 892901, 773188, 901602, 901598, 659269, 903852

Status: RESOLVED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Description

[Joe Walker [:jwalker]]

I fixed a number of small GCLI bugs and want to land them together to save time.

Comment 1

[Joe Walker [:jwalker]]

Changes broken out here: https://github.com/joewalker/gcli/pull/15

Comment 2

[Joe Walker [:jwalker]]

Attachment: rollup-900114.patch

Mike is slammed with reviews right now. So perhaps you could take a look at these fixes and we'll ask Mike to take your feedback into account when he does a review.

I'm very happy to introduce you to the GCLI code, so ping me if you'd like. There are docs: https://developer.mozilla.org/en-US/docs/Tools/GCLI and https://github.com/joewalker/gcli/blob/master/docs/index.md are good places to start.

The changes are broken down here: https://github.com/joewalker/gcli/pull/15

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 790254 [details] [diff] [review]
rollup-900114.patch

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

First round of feedback: I have pulled down the code, made some notes and asked questions where necessary.  I will need to spend more time digging into the design of the program to do another round, but I wanted to leave the first set of notes here.

One general note: there are a lot of `== null` or `!= null` checks in the patch.  I'm not sure if these are intentionally checking for all falsy values and the functionality relies on it.  Where it is not relying on that for functionality, it may be good to switch these to === checks to make it easier for the reader to reason about what the code is doing.

::: toolkit/devtools/gcli/gcli.jsm
@@ +2728,5 @@
> +
> +  var addParam = function(param) {
> +    var groupName = param.groupName || Parameter.DEFAULT_GROUP_NAME;
> +    this.params.push(param);
> +    if (this.paramGroups[groupName] == null) {

Should be if (!this.paramGroups.hasOwnProperty(groupName)) in case param.groupName to "toString" or something silly.

@@ +2925,5 @@
>   * in this command)
>   * @param name The name to check
>   */
>  Parameter.prototype.isKnownAs = function(name) {
> +  return (name === '--' + this.name) || (name == '-' + this.short);

Should use triple equals on second condition here for consistency

@@ +4981,3 @@
>      value = new Date();
>    }
> +  else if (arg.text.toLowerCase() === 'yesterday') {

setDate() seems to return an integer.  So this command outputs as so:

> "tsDate 2010-01-01 tomorrow"
> tsdate 2001-01-01
> Exec: tsdate d1=Mon Jan 01 2001 00:00:00 GMT-0600 (CST), d2=1376601506010

For consistency this should probably be:

  else if (arg.text.toLowerCase() === 'yesterday') {
    value = new Date()
    value.setDate(value.getDate() - 1);
  }
  else if (arg.text.toLowerCase() === 'tomorrow') {
    value = new Date();
    value.setDate(value.getDate() + 1);
  }

which leads to this result of the same command:

> "tsDate 2010-01-01 tomorrow"
> tsdate 2010-01-01 2013-08-15
> Exec: tsdate d1=Fri Jan 01 2010 00:00:00 GMT-0600 (CST), d2=Thu Aug 15 2013 16:14:20 GMT-0500 (CDT)

@@ +4987,4 @@
>      value = new Date().setDate(new Date().getDate() + 1);
>    }
>    else {
> +    // So now actual date parsing.

This JavaScript date stuff looks crazy.  I think it is begging for a test.  It is kind of tricky, because it seems to depend on the time zone of the computer running the commands, but a test could probably at least confirm that 2010-01-02 == 2010/01/02, and cover the case where a time zone is passed in.  This makes the assumption that the test would eventually run in a non UTC time zone (probably would on your CI server).

@@ +5002,5 @@
> +    // be in the local timezone.
> +
> +    // First, if the user explicitly includes a timezone, then we assume they
> +    // know what they are doing with timezones
> +    if (arg.text.indexOf('Z') !== -1) {

This may be an obvious question, but why Z?  What is the expected string format that includes the time zone in it?

@@ +6242,5 @@
>    return doc;
>  };
>  
>  /**
> + * Helper functions to be attached to the prototypes of NodeType and

Are these supposed to be highlighting nodes on the page?  Seems cool, and the code looks fine, but I'm not seeing it on the browser test page, when running `tse body`, for instance.

@@ +8119,5 @@
> +  assignment.conversion = conversion;
> +  assignment.conversion.assignment = assignment;
> +
> +  // Do nothing if the value is unchanged
> +  if (assignment.conversion.equals(oldConversion) ||

Conversion.equals implies Conversion.valueEquals, and valueEquals is protected from null parameters in this patch.  Should not need to check both, just check valueEquals as there will not be a situation where equals is true and valueEquals is false.

Comment 4

[Joe Walker [:jwalker]]

Excellent set of comments, thanks. Working on fixes.

Comment 5

[Joe Walker [:jwalker]]

Attachment: rollup-900114.patch v2

Mike is working on an update to our highlighter, so the node-list highlighting code won't work in Firefox yet. I raised bug 905626 to cover a known issue.

Comment 6

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Try&rev=9896f6574192

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 790800 [details] [diff] [review]
rollup-900114.patch v2

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

I have gone over the code again, and it looks good (I made one note to take a look at).  The patch will probably need to be rebuilt - I got 'failed to apply browser/devtools/commandline/test/helpers.js' after pulling down changes.

::: toolkit/devtools/gcli/gcli.jsm
@@ +8146,5 @@
> +  assignment.conversion = conversion;
> +  assignment.conversion.assignment = assignment;
> +
> +  // Do nothing if the conversion is unchanged
> +  if (assignment.conversion.equals(oldConversion)) {

I was suggesting to switch this to:

    if (assignment.conversion.valueEquals(oldConversion)) {
    
Switching it to just .equals changes the behavior from the initial patch because it requires both argEquals and valueEquals to be true.  Is this intentional?

Comment 8

[Joe Walker [:jwalker]]

(In reply to Brian Grinstead [:bgrins] from comment #7)
> Comment on attachment 790800 [details] [diff] [review]
> rollup-900114.patch v2
> 
> Review of attachment 790800 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have gone over the code again, and it looks good (I made one note to take
> a look at).  The patch will probably need to be rebuilt - I got 'failed to
> apply browser/devtools/commandline/test/helpers.js' after pulling down
> changes.
> 
> ::: toolkit/devtools/gcli/gcli.jsm
> @@ +8146,5 @@
> > +  assignment.conversion = conversion;
> > +  assignment.conversion.assignment = assignment;
> > +
> > +  // Do nothing if the conversion is unchanged
> > +  if (assignment.conversion.equals(oldConversion)) {
> 
> I was suggesting to switch this to:
> 
>     if (assignment.conversion.valueEquals(oldConversion)) {
>     
> Switching it to just .equals changes the behavior from the initial patch
> because it requires both argEquals and valueEquals to be true.  Is this
> intentional?

Discussed f-f and agreed that assignment.conversion.equals(oldConversion) is correct. The original code checked both valueEquals() and equals() were true, and since equals uses valueEquals(), we can have just equals().
Also all the unit tests pass, and I think this is well covered code.

Comment 9

[Joe Walker [:jwalker]]

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

Comment 10

[Joe Walker [:jwalker]]

Comment on attachment 790800 [details] [diff] [review]
rollup-900114.patch v2

Mike, this has had a *very* through review by Brian. Feel free to take that into account in your review.

Comment 11

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=3483a63fd3ca
https://hg.mozilla.org/integration/fx-team/rev/3483a63fd3ca

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3483a63fd3ca