The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 17

Status

()

Firefox
Developer Tools: Console
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: espadrine, Assigned: espadrine)

Tracking

Trunk
Firefox 17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
Assignee: nobody → thaddee.tyl
Attachment #647873 - Flags: review?(mihai.sucan)
(Assignee)

Comment 2

5 years ago
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? ☺
Attachment #647873 - Attachment is obsolete: true
Attachment #647873 - Flags: review?(mihai.sucan)
Attachment #647888 - Flags: review?(mihai.sucan)
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.
Attachment #647888 - Flags: review?(mihai.sucan)
(Assignee)

Comment 4

5 years ago
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! :)
(Assignee)

Comment 5

5 years ago
Created attachment 649061 [details] [diff] [review]
Autocompletion of non-objects, with tests

Added a test to ensure that we indeed complete non-objects.
Attachment #647888 - Attachment is obsolete: true
Attachment #649061 - Flags: review?(mihai.sucan)
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/
Attachment #649061 - Flags: review?(mihai.sucan) → review+
(Assignee)

Comment 7

5 years ago
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.
Attachment #649061 - Attachment is obsolete: true
Attachment #649427 - Flags: review?(mihai.sucan)
(Assignee)

Comment 8

5 years ago
The patch in question has been submitted to the try server with great success:
https://tbpl.mozilla.org/?tree=Try&rev=8836cf4607f9
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!
Attachment #649427 - Flags: review?(mihai.sucan)
(Assignee)

Comment 10

5 years ago
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.
Attachment #649427 - Attachment is obsolete: true
Attachment #650467 - Flags: review?(mihai.sucan)
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?
Attachment #650467 - Flags: review?(mihai.sucan) → review+
(Assignee)

Comment 12

5 years ago
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 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!
Attachment #650467 - Attachment description: Patch for bugs 779415, 776106, 780564 → [in-fx-team] Patch for bugs 779415, 776106, 780564
Attachment #650467 - Flags: checkin+

Updated

5 years ago
Status: NEW → ASSIGNED
Whiteboard: [fixed-in-fx-team]
Version: unspecified → Trunk
(Assignee)

Comment 14

5 years ago
Thanks a lot!
https://hg.mozilla.org/mozilla-central/rev/9fcf6f5cb2c1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 17
You need to log in before you can comment on or make changes to this bug.