1164210 - $$() should return a true Array

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Florent Fayolle]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150508094354

Steps to reproduce:

Firebug used to return a true array for the $$() command (http://getfirebug.com/wiki/index.php/$$ ). Currently, it returns a NodeList, which doesn't have the Array methods. So for example, you can't do $$("selector").forEach(function() {...}).

Currently implemented here: https://github.com/mozilla/gecko-dev/blob/dd620e38c01c12b306dc26e19a851cfbd5b7996c/toolkit/devtools/webconsole/utils.js#L1639-L1642

TODO:
- Copy the node list into a true Array. Note that to do so, you can't use the Array object as is, but rather use the Array of the window being debugged, like in this method: https://github.com/mozilla/gecko-dev/blob/dd620e38c01c12b306dc26e19a851cfbd5b7996c/toolkit/devtools/webconsole/utils.js#L1668

Florent

Comment 1

[John Pavlicek]

Is it ok if I try to take this one on?

Comment 2

[Florent Fayolle]

Is it OK for you, Jeff?

Florent

Comment 3

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Sounds good to me!

Comment 4

[Florent Fayolle]

Great!
@John: if you need help, you can ask on IRC: 
* server: irc.mozilla.org 
* channel: #devtools

Florent

Comment 5

[John Pavlicek]

Quick Mercurial question.  I asked on IRC and didn't get a response, nor did any web searches help with a solution.  I've made some changes, and tested them out, and now I want to create a patch.  When I tried "hg qnew"  I received the message "abort: working directory revision is not qtip."  How do I resolve this so that I can create a new patch?  I suspect this is, in some way due to me trying to reset to the original head location after the last bug I fixed.  Hopefully, I haven't messed it up too bad.

Thanks!

Comment 6

[Florent Fayolle]

Sorry, John, I haven't seen your message on IRC until your reply on this bug. Actually, I pay more attention to messages on #devtools than #developers (which is where you asked your question).

Can you please ping me on IRC please (my nickname is Florent)?

Florent

Comment 7

[Florent Fayolle]

Also does this link help?
http://andreaswuest.blogspot.fr/2009/11/what-to-do-when-your-mercurial-working.html

Florent

Comment 8

[John Pavlicek]

Attachment: Patch

Hopefully I did this correctly and selected the proper reviewer.

Comment 9

[Florent Fayolle]

Comment on attachment 8609768 [details] [diff] [review]
Patch

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

Great!

Note that I am not a reviewer normally and not an employee of the Mozilla foundation. So I just give you my feedback for information.

If you need an official reviewer for the WebConsole, you can ask :bgrins :).

Florent

::: toolkit/devtools/webconsole/utils.js
@@ +1644,5 @@
> +  let node;
> +
> +  for (var i = 0; i < results.length; ++i) {
> +    nodes.push(results[i]);
> +  }

Instead of looping, you can use |Array.from|:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/from

It allows you to create a true array from an array-like object:
 >>> Array.isArray(document.querySelectorAll('p'))
 false
 >>> let arr = Array.from(document.querySelector('p')); Array.isArray(arr);
 true

That's a slight improvement to be more concise and probably more optimized ;)

Comment 10

[John Pavlicek]

Comment on attachment 8609768 [details] [diff] [review]
Patch

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

Need a more official review.  Is this better as-is, or should I use the Array.from() method?  My hesitancy is due to the experimental nature of Array.from(), as per the documentation.

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8609768 [details] [diff] [review]
Patch

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

Hi, thanks for the patch.  We are using Array.from in other places in the tools without a problem, but there is a weird situation here that can happen that makes it safer to not use it.  The case is when we are debugging an older 'server' (aka an older version of Firefox).  In that case, `aOwner.window.wrappedJSObject.Array.from` may not be a function.  This is only for really old versions (like Firefox OS 1.3), but since the workaround is easy I'm OK with keeping it as-is.

Can you please add a test for this change?  Here is what would need to be done:

1) It will require making a new test, very similar to this one and in the same folder: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/webconsole/test/test_jsterm_last_result.html.  It could be called test_jsterm_queryselector.html.  I'll upload a simple version of this file to make it easier
2) Add an entry to this file with the file name right after test_jsterm_last_result.html: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/webconsole/test/chrome.ini#21
3) You can then run the test with `./mach mochitest toolkit/devtools/webconsole/test/test_jsterm_queryselector.html`

::: toolkit/devtools/webconsole/utils.js
@@ +1640,5 @@
>  {
> +  let nodes = new aOwner.window.wrappedJSObject.Array();
> +  let results = aOwner.window.document.querySelectorAll(aSelector);
> +
> +  let node;

This variable is unused and can be removed

Comment 12

[Brian Grinstead [:bgrins]]

Attachment: test_jsterm_queryselector.html

A simple testcase that can be added to toolkit/devtools/webconsole/test

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #11)
> Hi, thanks for the patch.  We are using Array.from in other places in the
> tools without a problem, but there is a weird situation here that can happen
> that makes it safer to not use it.  The case is when we are debugging an
> older 'server' (aka an older version of Firefox).  In that case,
> `aOwner.window.wrappedJSObject.Array.from` may not be a function.  This is
> only for really old versions (like Firefox OS 1.3), but since the workaround
> is easy I'm OK with keeping it as-is.

I was wrong about this - the command registration is happening on the server side so this is not a concern.  I think it'd be nice to update the patch to use `aOwner.window.wrappedJSObject.Array.from` and save a few lines of code.

Comment 14

[John Pavlicek]

No problem...I can do Array.from.  Should I do the fix and the test in separate patches or in a single patch?

Comment 15

[Brian Grinstead [:bgrins]]

(In reply to John Pavlicek from comment #14)
> No problem...I can do Array.from.  Should I do the fix and the test in
> separate patches or in a single patch?

Single patch would be great

Comment 16

[John Pavlicek]

I've been spending the last couple days trying to read through the test that you supplied as well as a couple others just so that I could better understand what's happening in the file. I think that I have it figured out, at least to a basic level.  What other tests should be added?  Do I need to check the "undefined" result case?

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to John Pavlicek from comment #16)
> I've been spending the last couple days trying to read through the test that
> you supplied as well as a couple others just so that I could better
> understand what's happening in the file. I think that I have it figured out,
> at least to a basic level.  What other tests should be added?  Do I need to
> check the "undefined" result case?

It'd be great to have a case that checks $$("foobar") and $$() to make sure there is still an array returned.  But I'd be happy with any basic test coverage for this feature, since once it's in place it's easy to expand if we add new features or need to fix any bugs.

Comment 18

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #17)
> (In reply to John Pavlicek from comment #16)
> > I've been spending the last couple days trying to read through the test that
> > you supplied as well as a couple others just so that I could better
> > understand what's happening in the file. I think that I have it figured out,
> > at least to a basic level.  What other tests should be added?  Do I need to
> > check the "undefined" result case?
> 
> It'd be great to have a case that checks $$("foobar") and $$() to make sure
> there is still an array returned.  But I'd be happy with any basic test
> coverage for this feature, since once it's in place it's easy to expand if
> we add new features or need to fix any bugs.

In fact, if you just integrate the test case I attached that should be a fine start

Comment 19

[John Pavlicek]

Attachment: b1164210.patch

I did add the test for $$("foobar") and that seems to work, returning an empty Array.  I tried adding the test for $$(""), but this does not return an Array, but returns "undefined" instead.  Looking at what the document.querySelectorAll("") returns, it actually returns a SyntaxError.  How should I handle this both in the utils.js and the test case?

Comment 20

[Brian Grinstead [:bgrins]]

Comment on attachment 8616315 [details] [diff] [review]
b1164210.patch

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

Looks good, just a minor note for the test.  If you want to add support for $$("") here, feel free (I've left a comment with how we could possibly handle that).  Otherwise that could be done in a follow up bug.

::: toolkit/devtools/webconsole/test/test_jsterm_queryselector.html
@@ +32,5 @@
> +  info ("$$ returns an array");
> +  let response = yield evaluateJS("$$('body')");
> +  basicResultCheck(response, "$$('body')", {
> +    type: "object",
> +    class: "Array"

I *think* you can add something like this to the result check to make sure the length is right (for this, and for the other where it should be 0)

preview: {
  length: 1
}

::: toolkit/devtools/webconsole/utils.js
@@ +1637,5 @@
>   *         Returns the result of document.querySelectorAll(aSelector).
>   */
>  WebConsoleCommands._registerOriginal("$$", function JSTH_$$(aOwner, aSelector)
>  {
> +  let results = aOwner.window.document.querySelectorAll(aSelector);

Regarding passing an invalid selector to qSA, we could do something like this:

let nodes = [];

try {
  let results = aOwner.window.document.querySelectorAll(aSelector);
  nodes = aOwner.window.wrappedJSObject.Array.from(results);
} catch(e) {}

return nodes;

Comment 21

[Brian Grinstead [:bgrins]]

Feel free to upload with just the test change and we can handle the invalid input as another bug if you'd like

Comment 22

[Florent Fayolle]

> Regarding passing an invalid selector to qSA, we could do something like this:

That's an excellent question. I wonder if it shouldn't print an exception in the console, for these reasons:
- to reflect the behavior of querySelectorAll
- to expressively mean that the selector is invalid or unsupported yet, thus that the user can't get any result anyway and anywhere (vs. the selector doesn't match anything)

FWIW, that's the behavior of Firebug and the Chrome DevTools.

Maybe that could be part of another bug.

What do you think Brian?

Florent

Comment 23

[John Pavlicek]

Attachment: b1164210.patch

I updated the tests  to add the "length" check.  Based on the discussion, I'm anticipating a follow-up bug to address the exception handling, so I haven't touched any of that.

Also, I just noticed the note at the top of the page when I submit the file about MozReview.  Should I be trying to use that, instead, or just keep with this form, for now?

Comment 24

[Florent Fayolle]

Nice!

> Also, I just noticed the note at the top of the page when I submit the file about MozReview.
> Should I be trying to use that, instead, or just keep with this form, for now?

I am interested in that question too.

Florent

Comment 25

[Brian Grinstead [:bgrins]]

(In reply to fayolle-florent from comment #22)
> > Regarding passing an invalid selector to qSA, we could do something like this:
> 
> That's an excellent question. I wonder if it shouldn't print an exception in
> the console, for these reasons:
> - to reflect the behavior of querySelectorAll
> - to expressively mean that the selector is invalid or unsupported yet, thus
> that the user can't get any result anyway and anywhere (vs. the selector
> doesn't match anything)
> 
> FWIW, that's the behavior of Firebug and the Chrome DevTools.
> 
> Maybe that could be part of another bug.

Sure, we could make it throw an exception in that case.  In fact, I'm sure it is throwing right now but being caught and ignored somewhere.

Comment 26

[Brian Grinstead [:bgrins]]

(In reply to John Pavlicek from comment #23)
> Also, I just noticed the note at the top of the page when I submit the file
> about MozReview.  Should I be trying to use that, instead, or just keep with
> this form, for now?

Both are fine options - since you already have the workflow down for this method that's a big advantage (i.e. if you want to grab another bug you will be able to jump right in without needing to learn more process).  FWIW I'm still using the 'old' way for the most part also.

Comment 27

[Brian Grinstead [:bgrins]]

Comment on attachment 8616472 [details] [diff] [review]
b1164210.patch

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

Looks good to me - here's a push to the test server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ba95db9745f

Comment 28

[Brian Grinstead [:bgrins]]

John, at this point we just wait for the tests to finish running on the test server, and then I'll mark checkin-needed if it all looks good.  Otherwise I'll let you know if there are any issues.  Thanks for the patch!

If you are interested in taking on another bug, I just saw Bug 1159725 go by, which would be a great improvement for the markup view.

Comment 29

[Brian Grinstead [:bgrins]]

John, looks like there is a timeout on browser/devtools/webconsole/test/browser_console_native_getters.js with the patch applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ba95db9745f.

I think it's because of this line, which is expecting a NodeList (when it should now be an array): https://dxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/test/browser_console_native_getters.js#80.

You can test it locally with this command: `./mach test browser/devtools/webconsole/test/browser_console_native_getters.js`

Comment 30

[John Pavlicek]

Attachment: b1164210.patch

I fixed the broken test.  It does appear that it was as simple as changing it to expect an Array instead of a NodeList.

Brian, thanks for the suggestion on the other bug.  I'll go apply to take that one on.

Comment 31

[Brian Grinstead [:bgrins]]

Comment on attachment 8616951 [details] [diff] [review]
b1164210.patch

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

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=453c222c0388

Comment 32

[Brian Grinstead [:bgrins]]

New try push (old one got bit by some infrastructure issues): https://treeherder.mozilla.org/#/jobs?repo=try&revision=64330b57f2a6

Comment 33

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to fayolle-florent from comment #22)
> > Regarding passing an invalid selector to qSA, we could do something like this:
> 
> That's an excellent question. I wonder if it shouldn't print an exception in
> the console, for these reasons:
> - to reflect the behavior of querySelectorAll
> - to expressively mean that the selector is invalid or unsupported yet, thus
> that the user can't get any result anyway and anywhere (vs. the selector
> doesn't match anything)
> 
> FWIW, that's the behavior of Firebug and the Chrome DevTools.

I think their approach is correct, fyi - I don't think hiding a real user input error is ever correct.

> Maybe that could be part of another bug.

+1!

Comment 34

[Brian Grinstead [:bgrins]]

Try push looks good

Comment 35

[Brian Grinstead [:bgrins]]

(In reply to Jeff Griffiths (:canuckistani) from comment #33)
> (In reply to fayolle-florent from comment #22)
> > > Regarding passing an invalid selector to qSA, we could do something like this:
> > 
> > That's an excellent question. I wonder if it shouldn't print an exception in
> > the console, for these reasons:
> > - to reflect the behavior of querySelectorAll
> > - to expressively mean that the selector is invalid or unsupported yet, thus
> > that the user can't get any result anyway and anywhere (vs. the selector
> > doesn't match anything)
> > 
> > FWIW, that's the behavior of Firebug and the Chrome DevTools.
> 
> I think their approach is correct, fyi - I don't think hiding a real user
> input error is ever correct.
> 
> > Maybe that could be part of another bug.
> 
> +1!

Filed Bug 1173385 for this

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c71df86eb2f7

Comment 37

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

https://hg.mozilla.org/mozilla-central/rev/c71df86eb2f7