779415 - The WebConsole autocompletion doesn't deal with non-objects (numbers, strings)

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Thaddee Tyl [:espadrine]]

1. Type the following in the WebConsole.

> window.a = 'hi';
> window.a.

2. Type <TAB>. (The "no results" overlay appears.)

The autocompletion doesn't take non-objects into account.
Unfortunately, a lot of values that can be autocompleted are not objects.

It should autocomplete on the constructor's prototype.

All major competitors autocomplete on non-objects.

Comment 1

[Thaddee Tyl [:espadrine]]

Attachment: Autocompletion of non-objects

This patch adds autocompletion of non-object elements to the WebConsole.

Note: I use `typeof aObj != "object"` rather than `instanceof Object` only because bug 774753 makes Object not be the same object as window.Object.

Comment 2

[Thaddee Tyl [:espadrine]]

Attachment: Autocompletion of non-objects, better check for non-objects

Actually, the previous patch had some bad behaviour,
wherein it would assume that some values which are not objects were objects,
and that some objects were not objects.

> window instanceof Object  // false
> typeof window == "object"  // true

> Object instanceof Object  // true
> typeof Object == "object"  // false

Yes, those are insane edge-cases. What is the point of "typeof" again? ☺

Comment 3

[Mihai Sucan [:msucan]]

Comment on attachment 647888 [details] [diff] [review]
Autocompletion of non-objects, better check for non-objects

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

This is cool. I like this. Thanks!

Please also add a test.

::: browser/devtools/webconsole/WebConsoleUtils.jsm
@@ +959,5 @@
> +  if (aObj == null) { return {}; }
> +  try {
> +    Object.getPrototypeOf(aObj);
> +  } catch(e) {
> +    aObj = aObj.constructor.prototype;

Can we always rely that obj.constructor.prototype works?

Also, wouldn't it work to just do if (!aObj.prototype) { aObj = aObj.constructor.prototype; } ? Maybe wrapped in a try-catch.

Comment 4

[Thaddee Tyl [:espadrine]]

obj.constructor.prototype always work.

`obj.constructor` is the function that was run to create the object,
which all objects have as per ES3.1 [15.2.4.1].

In turn, all constructors are functions,
and all functions are required to have a prototype [15.3.4.1].

On the other hand, all objects are not required to have a prototype.
There are objects that do not have a `prototype` property,
notably window (which is indeed an Object).
Using the form you suggest would make us lose all the properties we can gather
on those objects.

Finally, I'd like to add that the best way to do this is still to use
`if (!(aObj instanceof Object)) …`, but we cannot use it because of bug 774753.

I'll make a few tests! :)

Comment 5

[Thaddee Tyl [:espadrine]]

Attachment: Autocompletion of non-objects, with tests

Added a test to ensure that we indeed complete non-objects.

Comment 6

[Mihai Sucan [:msucan]]

Comment on attachment 649061 [details] [diff] [review]
Autocompletion of non-objects, with tests

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

Thanks!

Giving r+, but I'd note the try-catch looks a bit weird for me. I don't have an idea for something "better", so let's not worry. :)

Can you please push the patch to the try server?

::: browser/devtools/webconsole/WebConsoleUtils.jsm
@@ +966,4 @@
>   * Filter those properties by name.
>   * Take only a certain number of those.
>   *
> + * @param any obj

s/any obj/mixed aObj/

@@ +971,2 @@
>   *
>   * @param string matchProp

s/match/aMatch/

Comment 7

[Thaddee Tyl [:espadrine]]

Attachment: Patch for bugs 779415, 776106, 780564

As discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=776106#c6,
this patch includes the changes from:

- bug 776106: autocompletion on arrays includes numbers,
- bug 780564: performance improvements in dynamic property lookup.

Comment 8

[Thaddee Tyl [:espadrine]]

The patch in question has been submitted to the try server with great success:
https://tbpl.mozilla.org/?tree=Try&rev=8836cf4607f9

Comment 9

[Mihai Sucan [:msucan]]

Comment on attachment 649427 [details] [diff] [review]
Patch for bugs 779415, 776106, 780564

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

This looks good! Thanks for the try push.

I do have three requests:

- the while loop in getMatchedProps() is deeply nested and hard to read (for me). Can you please break early and not nest things too much? Like if prop.indexOf(match) != 0 { continue; } and if (!(prop in ownNames)) { continue; } ... etc. 

- also, completeAfter is never used. Shouldn't we remove it? We can add it back, later, when it's actually used.

- lastly, please add a test for arrays autocomplete so we make sure that the numbers do not show up.

Thanks!

Comment 10

[Thaddee Tyl [:espadrine]]

Attachment: [in-fx-team] Patch for bugs 779415, 776106, 780564

In this new patch, I followed your advice from the previous comment.

Comment 11

[Mihai Sucan [:msucan]]

Comment on attachment 650467 [details] [diff] [review]
[in-fx-team] Patch for bugs 779415, 776106, 780564

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

This looks good. r+!

If you have a green try push with the new test, I can land the patch. Please let me know here or on IRC and I'll land the patch.

::: browser/devtools/webconsole/WebConsoleUtils.jsm
@@ +976,5 @@
>   *
>   * @return object
>   *         Object whose keys are all accessible properties on the object.
>   */
> +function getMatchedProps(aObj, aOptions = {matchProp: ""})

Is this change of arguments needed anymore?

Comment 12

[Thaddee Tyl [:espadrine]]

The try push is at https://tbpl.mozilla.org/?tree=Try&rev=93553ed362d1.

I'd like to keep the aOptions argument because it clearly allows
extensibility.

Comment 13

[Mihai Sucan [:msucan]]

Comment on attachment 650467 [details] [diff] [review]
[in-fx-team] Patch for bugs 779415, 776106, 780564

Landed:
https://hg.mozilla.org/integration/fx-team/rev/9fcf6f5cb2c1

Thank you for your improvements!

Comment 14

[Thaddee Tyl [:espadrine]]

Thanks a lot!

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/9fcf6f5cb2c1