686937 - Web Console doesn't autocomplete JSTerm helpers.

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Jesse Ruderman]

The web console has a function called "inspectstyle" but nothing is autocompleted if I start typing "ins...".

Comment 1

[Rob Campbell [:rc] (:robcee)]

still true!

Comment 2

[Rob Campbell [:rc] (:robcee)]

filter on GUNGNIR.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

I found a vict..., er, I mean volunteer, to work on this!

inspectstyle is long gone, but we have inspect, pprint and all the other goodies mentioned here:

https://developer.mozilla.org/en-US/docs/Tools/Web_Console/Helpers

Look here for the actual helper definitions:

http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/webconsole/WebConsoleUtils.jsm#1198

Comment 4

[Christos Stathis]

Attachment: First attempt. It appends helper names in the list of suggestions

With this patch, helper names that start with the console input will always be present in the suggestion list. This is not correct in all cases e.g. "window." will give "window.inspect" which is not correct. Except for the case of ".", all other cases should be ok.

Comment 5

[Christos Stathis]

Attachment: Fixed case of '.' and added tests

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Forgot to hg addremove?

Comment 7

[Christos Stathis]

yep. Anyway, I was going to upload another one to cover one more case that I thought of.

Comment 8

[Christos Stathis]

Attachment: Autocompletes JSTerm helper names

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 781103 [details] [diff] [review]
Autocompletes JSTerm helper names

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

Finally! This bug has been annoying me to no end and I finally found relief! Thank you! I have a few comments below that will require another iteration, but this is good stuff.

The patch breaks a chrome mochitest, test_jsterm.html, you need to update that. Chrome mochitests are similar to browser mochitests and you can run them all with:

mach mochitest-chrome toolkit/devtools/webconsole

You can also use the path to a single file if you want. Also, there are 3 instances of extraneous whitespace in the patch that you should remove. You may add this in your .hgrc to catch them when refreshing your patch:

commit.whitespace = hg export tip | (! grep -C 3 --color=always -n '^+.*[ ]$')

::: browser/devtools/webconsole/test/browser_webconsole_bug_686937_autocomplete_JSTerm_helpers.js
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Tests that the cached autocomplete results are used when the new
> +// completions are a subset  of the completions for the shorter prefix

This comment doesn't look accurate.

@@ +5,5 @@
> +
> +// Tests that the cached autocomplete results are used when the new
> +// completions are a subset  of the completions for the shorter prefix
> +
> +const TEST_URI = "data:text/html;charset=utf8,<p>test JSTerm Helpers autocomplete";

Could you use a page that redefines some of our helper functions and verify that autocompletion finds the helper function and not the overriden value? Something like this:

data:text/html;charset=utf8,<script type="application/javascript">window.print=null;</script><p>test JSTerm Helpers autocomplete

I know that Chrome always prefers the overrides, so I'd like to (a) make this more explicit, and (b) make sure that if we ever change our implementation to match theirs, this test will force us to fix autocompletion as well.

@@ +47,5 @@
> +  jsterm.complete(jsterm.COMPLETE_HINT_ONLY, testNext);
> +  yield;
> +
> +  newItems = popup.getItems().map(function(e) {return e.label;});
> +  ok(newItems.indexOf("inspect") < 0, "autocomplete results do not contain helper 'inspect'");

is(foo, -1, "blah") would provide better error messages in case of failure.

::: toolkit/devtools/server/actors/webconsole.js
@@ +627,5 @@
>      // TODO: Bug 842682 - use the debugger API for autocomplete in the Web
>      // Console, and provide suggestions from the selected debugger stack frame.
>      let result = JSPropertyProvider(this.window, aRequest.text,
>                                      aRequest.cursor) || {};
> +    let helpers = this._getJSTermHelpers(this.dbg.addDebuggee(this.window));

Adding the window as a debuggee will put the page in debug mode imposing a significant performance hit until removeDebuggee is called. The good news is that I think we can punt on fixing this until bug 842682 is done, and just keep ignoring the Debugger API for now. You would only have to copy the guts of _getJSTermHelpers here, initializing it with a plain object that only has a sandbox property.

@@ +630,5 @@
>                                      aRequest.cursor) || {};
> +    let helpers = this._getJSTermHelpers(this.dbg.addDebuggee(this.window));
> +    let matches = result.matches || [];
> +    
> +    let lastNonAlhaIsDot = /[.][a-zA-Z0-9]*$/.test(aRequest.text);

Is this Alha guy a friend of yours? Let me introduce you to my buddy Alpha!

@@ +634,5 @@
> +    let lastNonAlhaIsDot = /[.][a-zA-Z0-9]*$/.test(aRequest.text);
> +    if (!lastNonAlhaIsDot) {
> +      let helperNames = Object.getOwnPropertyNames(helpers.sandbox);
> +
> +      matches = matches.concat(helperNames.filter(function(n) {return n.startsWith(result.matchProp)})).sort();

If you are going to sort the matches here, remove the sorting from JSPropertyProvider so we don't waste time sorting there.

Also, inlining the filter function like this is kinda weird from the POV of our coding style. If you want to be hip and macho like the rest of us, you may use the latest arrow function syntax from ES6:

matches = matches.concat(helperNames.filter(n => n.startsWith(result.matchProp))).sort();

Comment 10

[Christos Stathis]

(In reply to Panos Astithas [:past] from comment #9)
> Comment on attachment 781103 [details] [diff] [review]
> Autocompletes JSTerm helper names
> 
> Review of attachment 781103 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Finally! This bug has been annoying me to no end and I finally found relief!
> Thank you! I have a few comments below that will require another iteration,
> but this is good stuff.
> 
> The patch breaks a chrome mochitest, test_jsterm.html, you need to update
> that. Chrome mochitests are similar to browser mochitests and you can run
> them all with:
> 
> mach mochitest-chrome toolkit/devtools/webconsole
> 

Fixed.

> You can also use the path to a single file if you want. Also, there are 3
> instances of extraneous whitespace in the patch that you should remove. You
> may add this in your .hgrc to catch them when refreshing your patch:
> 
> commit.whitespace = hg export tip | (! grep -C 3 --color=always -n '^+.*[
> ]$')
> 

Fixed.

> :::
> browser/devtools/webconsole/test/
> browser_webconsole_bug_686937_autocomplete_JSTerm_helpers.js
> @@ +3,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +// Tests that the cached autocomplete results are used when the new
> > +// completions are a subset  of the completions for the shorter prefix
> 
> This comment doesn't look accurate.

Fixed.

> 
> @@ +5,5 @@
> > +
> > +// Tests that the cached autocomplete results are used when the new
> > +// completions are a subset  of the completions for the shorter prefix
> > +
> > +const TEST_URI = "data:text/html;charset=utf8,<p>test JSTerm Helpers autocomplete";
> 
> Could you use a page that redefines some of our helper functions and verify
> that autocompletion finds the helper function and not the overriden value?
> Something like this:
> 
> data:text/html;charset=utf8,<script
> type="application/javascript">window.print=null;</script><p>test JSTerm
> Helpers autocomplete
> 
> I know that Chrome always prefers the overrides, so I'd like to (a) make
> this more explicit, and (b) make sure that if we ever change our
> implementation to match theirs, this test will force us to fix
> autocompletion as well.

As far as I understand, the client just receives a list of suggestions without any knowledge of where they came from. So if a page overrides a helper name, the autocomplete should continue to suggest the same name without knowing if it is the helper or the overriden name. Whether the console will evaluate it as a helper or something else is a different matter. This test is probably more suitable for the console evaluation functionality.

> 
> @@ +47,5 @@
> > +  jsterm.complete(jsterm.COMPLETE_HINT_ONLY, testNext);
> > +  yield;
> > +
> > +  newItems = popup.getItems().map(function(e) {return e.label;});
> > +  ok(newItems.indexOf("inspect") < 0, "autocomplete results do not contain helper 'inspect'");
> 
> is(foo, -1, "blah") would provide better error messages in case of failure.

Fixed

> 
> ::: toolkit/devtools/server/actors/webconsole.js
> @@ +627,5 @@
> >      // TODO: Bug 842682 - use the debugger API for autocomplete in the Web
> >      // Console, and provide suggestions from the selected debugger stack frame.
> >      let result = JSPropertyProvider(this.window, aRequest.text,
> >                                      aRequest.cursor) || {};
> > +    let helpers = this._getJSTermHelpers(this.dbg.addDebuggee(this.window));
> 
> Adding the window as a debuggee will put the page in debug mode imposing a
> significant performance hit until removeDebuggee is called. The good news is
> that I think we can punt on fixing this until bug 842682 is done, and just
> keep ignoring the Debugger API for now. You would only have to copy the guts
> of _getJSTermHelpers here, initializing it with a plain object that only has
> a sandbox property.

Fixed

> 
> @@ +630,5 @@
> >                                      aRequest.cursor) || {};
> > +    let helpers = this._getJSTermHelpers(this.dbg.addDebuggee(this.window));
> > +    let matches = result.matches || [];
> > +    
> > +    let lastNonAlhaIsDot = /[.][a-zA-Z0-9]*$/.test(aRequest.text);
> 
> Is this Alha guy a friend of yours? Let me introduce you to my buddy Alpha!

Fixed.

> 
> @@ +634,5 @@
> > +    let lastNonAlhaIsDot = /[.][a-zA-Z0-9]*$/.test(aRequest.text);
> > +    if (!lastNonAlhaIsDot) {
> > +      let helperNames = Object.getOwnPropertyNames(helpers.sandbox);
> > +
> > +      matches = matches.concat(helperNames.filter(function(n) {return n.startsWith(result.matchProp)})).sort();
> 
> If you are going to sort the matches here, remove the sorting from
> JSPropertyProvider so we don't waste time sorting there.

Fixed.

> 
> Also, inlining the filter function like this is kinda weird from the POV of
> our coding style. If you want to be hip and macho like the rest of us, you
> may use the latest arrow function syntax from ES6:
> 
> matches = matches.concat(helperNames.filter(n =>
> n.startsWith(result.matchProp))).sort();

Now I am hip and macho, too!

Comment 11

[Christos Stathis]

Attachment: Autocompletes JSTerm helper names.

Addressed all issues in comment #10

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 782184 [details] [diff] [review]
Autocompletes JSTerm helper names.

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

There is one remaining instance of redundant whitespace in the test and a couple of typos in comments, but I'll take care of them before I check it in.

Thanks for the patch!

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

Landed:
https://hg.mozilla.org/integration/fx-team/rev/5458a907d1a5

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/mozilla-central/rev/5458a907d1a5

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

Mihai observed that calling JSTermHelpers() for every autocompletion request is needlessly expensive, as we regenerate the helper object from scratch. We should just cache it after creation, and I think bug 842682 will be a good opportunity to fix both this and the other issue I mentioned in comment 9.