1023233 - GCLI help command is erroring

Status: RESOLVED FIXED

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

Comment 1

[Joe Walker [:jwalker]]

Error: Not implemented
Stack trace:
Type.prototype.getSpec@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/types/types.js:954:3
Parameter.prototype.toJson@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/commands/commands.js:298:5
Command.prototype.toJson/json.params<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/commands/commands.js:171:47
Command.prototype.toJson@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/commands/commands.js:171:5
getMatchingCommands/commands<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/commands/help.js:152:5
getMatchingCommands@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/commands/help.js:151:3
exports.items<.exec@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/commands/help.js:232:9
Requisition.prototype.exec/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/cli.js:2057:9
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:214:9
Task_spawn@resource://gre/modules/Task.jsm:139:5
exports.exec@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/host.js:71:3
Requisition.prototype.exec@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/cli.js:2058:1
Inputter.prototype._handleReturn@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/mozui/inputter.js:555:5
Inputter.prototype.handleKeyUp@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/mozui/inputter.js:454:5
Inputter.prototype.onKeyUp@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/mozui/inputter.js:426:3

Comment 2

[Joe Walker [:jwalker]]

Attachment: 0004-Bug-1023233-Add-getSpec-to-union-type-f-gl-r-mratcli.patch

I split the GCLI changes in this patch out, so you could easily understand it on github [1].

The changes that don't come through in the pull request are:

* browser.ini - Just adding the new test file (browser_gcli_union.js)
* inject.js - Nit s/'/"/ and rename the types property to alternatives to
  match the pull request

I'll ask Mike for a review when you're done. Thanks!

[1]: https://github.com/joewalker/gcli/pull/71

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8439956 [details] [diff] [review]
0004-Bug-1023233-Add-getSpec-to-union-type-f-gl-r-mratcli.patch

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

This looks good. Really appreciated the github PR breakdown.

browser_cmd_inject.js also needs to be updated with the union changes.
Error log:
chrome://mochitests/content/browser/browser/devtools/commandline/test/browser_cmd_inject.js | status (for 'inject j') - Got VALID, expected ERROR
chrome://mochitests/content/browser/browser/devtools/commandline/test/browser_cmd_inject.js | markup (for 'inject j') - Got VVVVVVVV, expected VVVVVVVI
chrome://mochitests/content/browser/browser/devtools/commandline/test/browser_cmd_inject.js | hints (for 'inject j') - Got , expected Query

::: browser/devtools/commandline/test/browser_gcli_union.js
@@ +84,5 @@
> +  item: 'converter',
> +  from: 'json',
> +  to: 'view',
> +  exec: function(json, context) {
> +    var html = JSON.stringify(json, null, '&nbsp;').replace('\n', '</br>');

This test doesn't seem to pass because of how the view is formed.
I got XML parsing error for an undefined entity for &nbsp;, which was fixed by using ' '.
<br/> should be used here. Otherwise, a parsing error occurs because of mismatch opening and closing tags with <div>.

This will work:
var html = JSON.stringify(json, null, ' ').replace('\n', '<br/>');

@@ +110,5 @@
> +      setup:    'unioncommand',
> +      check: {
> +        input:  'unioncommand',
> +        markup: 'VVVVVVVVVVVV',
> +        hints:              ' <first>',

<first> is misaligned

::: toolkit/devtools/gcli/source/lib/gcli/types/union.js
@@ +63,4 @@
>  
>        var onError = function(i) {
> +        if (i >= this.alternatives.length) {
> +          if (bestIncomplete != null) {

if (bestIncomplete) would work here.

@@ +94,3 @@
>              }
> +
> +            if (bestIncomplete == null &&

same as above !bestIncomplete

@@ +97,5 @@
> +                conversion.getStatus() === Status.INCOMPLETE) {
> +              bestIncomplete = conversion;
> +            }
> +
> +            return onError(i);

Since we don't return the bestIncomplete immediately and continue to try and find a valid conversion, we will eventually hit the most general cases, which will provide a valid status. For selections, this means we lose out on the predictions because we aren't returning incomplete conversions, and no autocompletion hints will be displayed. I think we might want to have a special case for selection here to display predictions. This also explains the error for browser_cmd_inject.

Comment 4

[Joe Walker [:jwalker]]

(In reply to Gabriel Luong (:gl) from comment #3)
> Comment on attachment 8439956 [details] [diff] [review]
> 0004-Bug-1023233-Add-getSpec-to-union-type-f-gl-r-mratcli.patch
> 
> Review of attachment 8439956 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good. Really appreciated the github PR breakdown.
> 
> browser_cmd_inject.js also needs to be updated with the union changes.
> Error log:
> chrome://mochitests/content/browser/browser/devtools/commandline/test/
> browser_cmd_inject.js | status (for 'inject j') - Got VALID, expected ERROR
> chrome://mochitests/content/browser/browser/devtools/commandline/test/
> browser_cmd_inject.js | markup (for 'inject j') - Got VVVVVVVV, expected
> VVVVVVVI
> chrome://mochitests/content/browser/browser/devtools/commandline/test/
> browser_cmd_inject.js | hints (for 'inject j') - Got , expected Query
> 
> ::: browser/devtools/commandline/test/browser_gcli_union.js
> @@ +84,5 @@
> > +  item: 'converter',
> > +  from: 'json',
> > +  to: 'view',
> > +  exec: function(json, context) {
> > +    var html = JSON.stringify(json, null, '&nbsp;').replace('\n', '</br>');
> 
> This test doesn't seem to pass because of how the view is formed.
> I got XML parsing error for an undefined entity for &nbsp;, which was fixed
> by using ' '.

The problem was that the XML parsing function that we use returns a Node, which means we can't have multiple top level things. I changed it to '<pre>' + html + '</pre>' to fix this.

> <br/> should be used here. Otherwise, a parsing error occurs because of
> mismatch opening and closing tags with <div>.

Fixed, thanks.

> @@ +110,5 @@
> > +      setup:    'unioncommand',
> > +      check: {
> > +        input:  'unioncommand',
> > +        markup: 'VVVVVVVVVVVV',
> > +        hints:              ' <first>',
> 
> <first> is misaligned

It's actually deliberate - I lined up the starts of setup/input/markup so you could see that they are the same length, and the hints (which come in the command line after the input) is lined up with the end of the input.


> ::: toolkit/devtools/gcli/source/lib/gcli/types/union.js
> @@ +63,4 @@
> >  
> >        var onError = function(i) {
> > +        if (i >= this.alternatives.length) {
> > +          if (bestIncomplete != null) {
> 
> if (bestIncomplete) would work here.

Because of it's history, GCLI has some minor code differences in coding conventions. This is one - to avoid unexpected type conversions we generally stick to ===, except for null/undefined checks that use ==null.

> @@ +97,5 @@
> > +                conversion.getStatus() === Status.INCOMPLETE) {
> > +              bestIncomplete = conversion;
> > +            }
> > +
> > +            return onError(i);
> 
> Since we don't return the bestIncomplete immediately and continue to try and
> find a valid conversion, we will eventually hit the most general cases,
> which will provide a valid status. For selections, this means we lose out on
> the predictions because we aren't returning incomplete conversions, and no
> autocompletion hints will be displayed. I think we might want to have a
> special case for selection here to display predictions. This also explains
> the error for browser_cmd_inject.

And I later realized that the algorithm was the cause of further bugs. If a custom URL was close enough to one of the built-in libraries that the fuzzy matcher suggested it as a prediction then the argument would be marked as incomplete, even though it was actually valid.

So we need to check all the alternatives before we decide which to use, which also means we can provide a unified set of predictions.

Comment 5

[Joe Walker [:jwalker]]

Attachment: 0001-Bug-1023233-Add-getSpec-to-union-type-f-gl-r-mratcli.patch

Changes since PR71 broken down here: https://github.com/joewalker/gcli/pull/72
Try: https://tbpl.mozilla.org/?tree=Try&rev=d7e28919deaf

Comment 6

[Joe Walker [:jwalker]]

(In reply to Joe Walker [:jwalker] from comment #4)
> (In reply to Gabriel Luong (:gl) from comment #3)
> ...
> > > +    var html = JSON.stringify(json, null, '&nbsp;').replace('\n', '</br>');
> > 
> > This test doesn't seem to pass because of how the view is formed.
> > I got XML parsing error for an undefined entity for &nbsp;, which was fixed
> > by using ' '.
> 
> The problem was that the XML parsing function that we use returns a Node,
> which means we can't have multiple top level things. I changed it to '<pre>'
> + html + '</pre>' to fix this.

We were both right, of course. I used &#160; because I didn't want repeated ' ' to be merged.

Comment 7

[Joe Walker [:jwalker]]

Attachment: 0001-Bug-1023233-Add-getSpec-to-union-type-f-gl-r-mratcli.patch

Unit test fixes. Pushed update commits to https://github.com/joewalker/gcli/pull/72. Also fixed tests in browser_gcli_union.js

Comment 8

[Joe Walker [:jwalker]]

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

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8442703 [details] [diff] [review]
0001-Bug-1023233-Add-getSpec-to-union-type-f-gl-r-mratcli.patch

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

Looking great. I really like the unified predictions. 

See below for more feedback.

::: browser/devtools/commandline/test/browser.ini
@@ +102,4 @@
>  [browser_gcli_tokenize.js]
>  [browser_gcli_tooltip.js]
>  [browser_gcli_types.js]
> +[browser_gcli_union.js]

Missing browser_gcli_url.js

::: browser/devtools/commandline/test/browser_gcli_url.js
@@ +70,5 @@
> +        hints:              ' -> http://example/',
> +        predictions: [
> +          'http://example/',
> +          'https://example/',
> +          'http://localhost:9999/example'

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/commandline/test/browser_gcli_url.js | array length: predictions (for 'urlc example') - Got 2, expected 3

'http://localhost:9999/example' is not predictions. This is probably because it is not running in the context of a node server.

::: toolkit/devtools/gcli/source/lib/gcli/commands/test.js
@@ +21,4 @@
>  var examiner = require('../testharness/examiner');
>  var stati = require('../testharness/status').stati;
>  var helpers = require('../test/helpers');
> +var cli = require('../cli');

The changes in toolkit/devtools/gcli/source/lib/gcli/commands/test.js were actually not in the PR. (In case it should be)

::: toolkit/devtools/gcli/source/lib/gcli/types/union.js
@@ +77,2 @@
>  
> +          indexLoop:

Interesting TIL!

::: toolkit/devtools/gcli/source/lib/gcli/types/url.js
@@ +59,5 @@
> +              // Ignore
> +            }
> +          }.bind(this));
> +
> +          if (context.environment.window) {

Could use a comment here that states a localized href is being created

::: toolkit/devtools/gcli/source/lib/gcli/util/host.js
@@ +74,5 @@
>  /**
> + * The URL API is new enough that we need specific platform help
> + */
> +exports.createUrl = function(uristr, base) {
> +  return require("sdk/url").URL(uristr, base);

The require might be better as a global since we call createUrl a number of times as the user types a url for inject, but should be okay locally since the module gets cached in the loader.

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1023233.patch

Updated the patch with browser.ini entry and fixed browser_gcli_url.js test case.
try: https://tbpl.mozilla.org/?tree=Try&rev=5bddf983ecbe

Diff patch coming.

Comment 11

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1023233-diff.patch

Comment 12

[Joe Walker [:jwalker]]

(In reply to Gabriel Luong (:gl) from comment #9)
> Comment on attachment 8442703 [details] [diff] [review]
> 0001-Bug-1023233-Add-getSpec-to-union-type-f-gl-r-mratcli.patch
> 
> Review of attachment 8442703 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking great. I really like the unified predictions. 
> 
> See below for more feedback.
> 
> ::: browser/devtools/commandline/test/browser.ini
> @@ +102,4 @@
> >  [browser_gcli_tokenize.js]
> >  [browser_gcli_tooltip.js]
> >  [browser_gcli_types.js]
> > +[browser_gcli_union.js]
> 
> Missing browser_gcli_url.js
> 
> ::: browser/devtools/commandline/test/browser_gcli_url.js
> @@ +70,5 @@
> > +        hints:              ' -> http://example/',
> > +        predictions: [
> > +          'http://example/',
> > +          'https://example/',
> > +          'http://localhost:9999/example'
> 
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/devtools/commandline/test/
> browser_gcli_url.js | array length: predictions (for 'urlc example') - Got
> 2, expected 3
> 
> 'http://localhost:9999/example' is not predictions. This is probably because
> it is not running in the context of a node server.

I'm a bit mystified by these, the fix for the latter was even in the PR, and both were in a later try. Anyway thanks for finding them.

> ::: toolkit/devtools/gcli/source/lib/gcli/commands/test.js
> @@ +21,4 @@
> >  var examiner = require('../testharness/examiner');
> >  var stati = require('../testharness/status').stati;
> >  var helpers = require('../test/helpers');
> > +var cli = require('../cli');
> 
> The changes in toolkit/devtools/gcli/source/lib/gcli/commands/test.js were
> actually not in the PR. (In case it should be)

No, the test command isn't in Firefox.

> ::: toolkit/devtools/gcli/source/lib/gcli/types/url.js
> @@ +59,5 @@
> > +              // Ignore
> > +            }
> > +          }.bind(this));
> > +
> > +          if (context.environment.window) {
> 
> Could use a comment here that states a localized href is being created

Done.

> ::: toolkit/devtools/gcli/source/lib/gcli/util/host.js
> @@ +74,5 @@
> >  /**
> > + * The URL API is new enough that we need specific platform help
> > + */
> > +exports.createUrl = function(uristr, base) {
> > +  return require("sdk/url").URL(uristr, base);
> 
> The require might be better as a global since we call createUrl a number of
> times as the user types a url for inject, but should be okay locally since
> the module gets cached in the loader.

Done.

Comment 13

[Joe Walker [:jwalker]]

Attachment: 0001-Bug-1023233-Add-getSpec-to-union-type-f-gl-r-mratcli.patch

Mike - Gabriel has already given this a very thorough check, I suggest you adjust the time you spend on it accordingly.

Comment 14

[Joe Walker [:jwalker]]

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

Comment 15

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=5def913879a7
https://hg.mozilla.org/integration/fx-team/rev/3988c3177983

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3988c3177983