Last Comment Bug 779415 - The WebConsole autocompletion doesn't deal with non-objects (numbers, strings)
: The WebConsole autocompletion doesn't deal with non-objects (numbers, strings)
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Thaddee Tyl [:espadrine]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-31 23:14 PDT by Thaddee Tyl [:espadrine]
Modified: 2012-08-13 21:20 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Autocompletion of non-objects (1.22 KB, patch)
2012-08-01 00:51 PDT, Thaddee Tyl [:espadrine]
no flags Details | Diff | Review
Autocompletion of non-objects, better check for non-objects (1.25 KB, patch)
2012-08-01 02:12 PDT, Thaddee Tyl [:espadrine]
no flags Details | Diff | Review
Autocompletion of non-objects, with tests (1.96 KB, patch)
2012-08-04 21:05 PDT, Thaddee Tyl [:espadrine]
mihai.sucan: review+
Details | Diff | Review
Patch for bugs 779415, 776106, 780564 (4.93 KB, patch)
2012-08-06 14:57 PDT, Thaddee Tyl [:espadrine]
no flags Details | Diff | Review
[in-fx-team] Patch for bugs 779415, 776106, 780564 (5.53 KB, patch)
2012-08-09 01:44 PDT, Thaddee Tyl [:espadrine]
mihai.sucan: review+
mihai.sucan: checkin+
Details | Diff | Review

Description Thaddee Tyl [:espadrine] 2012-07-31 23:14:43 PDT
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] 2012-08-01 00:51:48 PDT
Created attachment 647873 [details] [diff] [review]
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] 2012-08-01 02:12:22 PDT
Created attachment 647888 [details] [diff] [review]
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] 2012-08-01 12:28:57 PDT
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] 2012-08-02 01:03:12 PDT
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] 2012-08-04 21:05:51 PDT
Created attachment 649061 [details] [diff] [review]
Autocompletion of non-objects, with tests

Added a test to ensure that we indeed complete non-objects.
Comment 6 Mihai Sucan [:msucan] 2012-08-06 11:33:15 PDT
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] 2012-08-06 14:57:46 PDT
Created attachment 649427 [details] [diff] [review]
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] 2012-08-07 00:16:19 PDT
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] 2012-08-08 09:36:43 PDT
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] 2012-08-09 01:44:59 PDT
Created attachment 650467 [details] [diff] [review]
[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] 2012-08-10 08:27:52 PDT
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] 2012-08-10 12:27:28 PDT
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] 2012-08-13 08:56:41 PDT
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] 2012-08-13 09:42:52 PDT
Thanks a lot!
Comment 15 Tim Taubert [:ttaubert] 2012-08-13 21:20:58 PDT
https://hg.mozilla.org/mozilla-central/rev/9fcf6f5cb2c1

Note You need to log in before you can comment on or make changes to this bug.