Closed Bug 1164210 Opened 5 years ago Closed 5 years ago

$$() should return a true Array

Categories

(DevTools :: Console, defect)

38 Branch
defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: fayolle-florent, Assigned: sr71pav, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good-first-bug][firebug-gap])

Attachments

(2 files, 3 obsolete files)

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
Component: Untriaged → Developer Tools: Console
Whiteboard: [good-first-bug][firebug-gap]
Is it ok if I try to take this one on?
Is it OK for you, Jeff?

Florent
Flags: needinfo?(jgriffiths)
Sounds good to me!
Flags: needinfo?(jgriffiths)
Great!
@John: if you need help, you can ask on IRC: 
* server: irc.mozilla.org 
* channel: #devtools

Florent
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!
Flags: needinfo?(fayolle-florent)
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
Flags: needinfo?(fayolle-florent)
Attached patch Patch (obsolete) — Splinter Review
Hopefully I did this correctly and selected the proper reviewer.
Attachment #8609768 - Flags: review?(fayolle-florent)
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 ;)
Attachment #8609768 - Flags: review?(fayolle-florent) → review+
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.
Attachment #8609768 - Flags: review?(bgrinstead)
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
Attachment #8609768 - Flags: review?(bgrinstead) → feedback+
A simple testcase that can be added to toolkit/devtools/webconsole/test
Assignee: nobody → sr71pav
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Mentor: bgrinstead, fayolle-florent
(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.
No problem...I can do Array.from.  Should I do the fix and the test in separate patches or in a single patch?
Flags: needinfo?(bgrinstead)
(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
Flags: needinfo?(bgrinstead)
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?
Flags: needinfo?(bgrinstead)
(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.
Flags: needinfo?(bgrinstead)
(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
Attached patch b1164210.patch (obsolete) — Splinter Review
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?
Attachment #8609768 - Attachment is obsolete: true
Flags: needinfo?(bgrinstead)
Attachment #8616315 - Flags: review?(bgrinstead)
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;
Attachment #8616315 - Flags: review?(bgrinstead) → feedback+
Feel free to upload with just the test change and we can handle the invalid input as another bug if you'd like
Flags: needinfo?(bgrinstead)
> 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
Flags: needinfo?(bgrinstead)
Attached patch b1164210.patch (obsolete) — Splinter Review
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?
Attachment #8616315 - Attachment is obsolete: true
Attachment #8616472 - Flags: review?(bgrinstead)
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
(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.
Flags: needinfo?(bgrinstead)
(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 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
Attachment #8616472 - Flags: review?(bgrinstead) → review+
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.
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`
Flags: needinfo?(sr71pav)
Attached patch b1164210.patchSplinter Review
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.
Attachment #8616472 - Attachment is obsolete: true
Flags: needinfo?(sr71pav)
Attachment #8616951 - Flags: review?(bgrinstead)
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
Attachment #8616951 - Flags: review?(bgrinstead) → review+
New try push (old one got bit by some infrastructure issues): https://treeherder.mozilla.org/#/jobs?repo=try&revision=64330b57f2a6
(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!
Try push looks good
Keywords: checkin-needed
(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
https://hg.mozilla.org/mozilla-central/rev/c71df86eb2f7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.