1164195 - $x should accept the resultType argument

Status: RESOLVED FIXED

(DevTools :: Console, enhancement)

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:

As per http://getfirebug.com/wiki/index.php/%24x , $x() can take a third argument that is the result type.
$x is a command that returns an array of nodes matching an XPath expression. It uses the document.evaluate() function to do so:
- here: https://github.com/mozilla/gecko-dev/blob/dd620e38c01c12b306dc26e19a851cfbd5b7996c/toolkit/devtools/webconsole/utils.js#L1666-L1680
- documentation of document.evaluate: https://developer.mozilla.org/en-US/docs/Web/API/Document/evaluate

TODO:
1. change the definition of the method in order to accept a third argument which has to be the result type
2. if the result type is not provided, the default value remains |Ci.nsIDOMXPathResult.ANY_TYPE|

Florent

Comment 1

[Sushrut Bhalla]

Hi,
Could you please assign this bug to me?
Thanks,
Sushrut

Comment 2

[Florent Fayolle]

Hi, Brian, could you assign this bug to Sushrut please?

Sushrut, don't hesitate to start working on this bug. If you have any questions, you can ask your here or on IRC:
* server: irc.mozilla.org
* channel: #devtools

My nickname is "florent", I am rather available the week-end or the evening in France (17:00 - 21:00 GMT) during the work days.

Florent

Comment 3

[Vijendra Singh]

Attachment: addedThirdArgumentSupport.patch

Hi Brian! I'm so sorry that I am submitting this patch (my second one!) after you assigned it to Sushrut. But I saw it a few days ago and started working on it. If it's ok, I request you to please review my patch.

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8614766 [details] [diff] [review]
addedThirdArgumentSupport.patch

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

::: toolkit/devtools/webconsole/utils.js
@@ +1663,5 @@
>   * Runs an xPath query and returns all matched nodes.
>   *
>   * @param string aXPath
>   *        xPath search query to execute.
>   * @param [optional] nsIDOMNode aContext

We need to add the new parameter to this header comment.  It should also list the accepted values as found in this table: http://getfirebug.com/wiki/index.php/%24x#resultType

@@ +1667,5 @@
>   * @param [optional] nsIDOMNode aContext
>   *        Context to run the xPath query on. Uses window.document if not set.
>   * @return array of nsIDOMNode
>   */
> +WebConsoleCommands._registerOriginal("$x", function JSTH_$x(aOwner, aXPath, aContext, aResultType)

You can use the default parameter feature to make this change without needing to change the function body.

You can set `aResultType = Ci.nsIDOMXPathResult.ANY_TYPE` here in the argument list and it will fall back to the correct value if nothing is passed in.

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Vijendra Singh from comment #3)
> Hi Brian! I'm so sorry that I am submitting this patch (my second one!)
> after you assigned it to Sushrut. But I saw it a few days ago and started
> working on it. If it's ok, I request you to please review my patch.

Alright.  Sushrut, sorry for the inconvenience - please take a look at other good first bugs for another one to work on: https://wiki.mozilla.org/DevTools/GetInvolved#Mentored_and_Good_First_Bugs, and let us know if you need help finding one.

Comment 6

[Vijendra Singh]

Attachment: addedThirdArgument-Edited.patch

Thanks for all you help! I added the parameter to the header comment and used the default parameter feature as suggested. Please have a look at it again.

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8615127 [details] [diff] [review]
addedThirdArgument-Edited.patch

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

Looks great!

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_xpath.html.  I'll upload a simple version of this file to make it easier.
2) Add an entry to the chrome.ini 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_xpath.html`

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: test_jsterm_xpath.html

Starter for the test case, as promised in Comment 7.  Just needs an implementation for checkResultType.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

No updates in many months, unassigning.

Comment 10

[Alexey Novak]

Attachment: Bug1164195.patch

Hi,

I saw that there were no updates in many months for this bug so I would love to finish it.
Attaching an update patch with tests.

Although I'm slightly confused with these tests. It is either I do not understand something about $x() or I'm doing something wrong. After reading documentation http://getfirebug.com/wiki/index.php/%24x I tried to select '//p' or './/p' inside of my test but it always gives me empty results back but it works with '*'. Ok sure... but it does not look like 3rd argument has an effect on anything. I tried to put couple of different values like: 'nodes' or 'node' and still get the same result back.

Comment 11

[Brian Grinstead [:bgrins]]

Attachment: xpath-diff.patch

Here's a diff you can apply to Attachment 8685769 [details] [diff] that should get the test working.  I believe it was attaching to the test harness document before - now it opens a new HTML page and attaches to that.  I also updated some of the assertions to target specific elements.

If you need extra debugging in the test, you can throw a debugger; statement somewhere and run  `./mach mochitest devtools/shared/webconsole/test/test_jsterm_xpath.html --jsdebugger` which will let you pause.

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8685769 [details] [diff] [review]
Bug1164195.patch

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

Looks generally good, see attached test fixes

Comment 13

[Alexey Novak]

Attachment: Bug1164195.patch

Oh wow. Thanks for the patch and explanations. It probably would take me a long time to find what are those changes to make my test to work.
Also it gives me a better idea on how to use |basicResultCheck()| as well as how to debug mochitests.

I made the changes in the patch. Please let me know if I need to add some more tests.

p.s.
A tiny question about JS syntax I saw in your patch file

items: [
        { class: "HTMLSpanElement" },
        { class: "HTMLParagraphElement" },
      ]

usually JSHint/JSLint would complain that there is a comma after the last element in an array. I assume it is a fair game in the Moz codebase ?

Comment 14

[Brian Grinstead [:bgrins]]

(In reply to Alexey Novak from comment #13)
> A tiny question about JS syntax I saw in your patch file
> 
> items: [
>         { class: "HTMLSpanElement" },
>         { class: "HTMLParagraphElement" },
>       ]
> 
> usually JSHint/JSLint would complain that there is a comma after the last
> element in an array. I assume it is a fair game in the Moz codebase ?

I prefer this format since it better preserves diff when appending a new object to the array.  I believe our linter is configured to be OK with this pattern.

Comment 15

[Alexey Novak]

Understood.

Is my patch looks OK for merge or do I need to change it more?

Comment 16

[Brian Grinstead [:bgrins]]

We are in the process of adding support for the resultType argument to the $x helper.  Documentation for that argument: https://developer.mozilla.org/en-US/docs/Web/API/Document/evaluate#Result_types.

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8686132 [details] [diff] [review]
Bug1164195.patch

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

::: devtools/shared/webconsole/test/test_jsterm_xpath.html
@@ +23,5 @@
> +function startTest() {
> +  removeEventListener("load", startTest);
> +  attachConsoleToTab([], state => {
> +    gState = state;
> +    let tests = [checkBasic,checkContext,checkResultType];

Before landing I'd like to see a test case that uses another result type.  For instance, I'm seeing an issue with the patch applied.  STR:

Open a new tab: data:text/html,<button>hi</button>
Open console
Run: document.evaluate('count(//BUTTON)', document, null, XPathResult.NUMBER_TYPE, null)
Run: $x('count(//BUTTON)', document, XPathResult.NUMBER_TYPE)

When running with $x I get `TypeError: The expression cannot be converted to return the specified type`

I see the same error with XPathResult.FIRST_ORDERED_NODE_TYPE, although it works with UNORDERED_NODE_ITERATOR_TYPE.  I guess it's only working when the results set is able to be iterated, so we may need a fix in utils.js for this (and a test case here for it).

Comment 18

[Alexey Novak]

Yes. Let's land something working and useful. I will take a look into the problem and think how to deal with it.

Also I tried to run in my Nightly:
$x('count(//BUTTON)', document, XPathResult.UNORDERED_NODE_ITERATOR_TYPE)
$x('count(//BUTTON)', document, XPathResult.FIRST_ORDERED_NODE_TYPE)

but I get |TypeError: The expression cannot be converted to return the specified type.| for both executions. I'm not entirely sure how UNORDERED_NODE_ITERATOR_TYPE worked for you.

Comment 20

[Alexey Novak]

Sorry I had a deliverable to meet at work last week. I will try to resolve this bug this week.

Comment 21

[Patrick Brosset <:pbro>]

[good first bug] whiteboard -> keyword mass change

Comment 22

[Sebastian Zartner [:sebo]]

As Alexey wasn't active on Bugzilla for a long time, I assume it's safe to unassign him to give others the chance to pick this up.

Sebastian

Comment 23

[Pradeep Gangwar [:pradeepgangwar]]

I am a new one here looking to contribute to devtools in firefox.Since this is categorized as Good First Bug, I will try my best to solve this out. Please guide me how should i proceed further with solving this bug. I have built mozilla-central repo on my computer.

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to Pradeep Gangwar [:Pradeep] from comment #23)
> I am a new one here looking to contribute to devtools in firefox.Since this
> is categorized as Good First Bug, I will try my best to solve this out.
> Please guide me how should i proceed further with solving this bug. I have
> built mozilla-central repo on my computer.

The current patch attached to the bug is a good start, but the file at devtools/shared/webconsole/utils.js has since moved to devtools/server/actors/webconsole/utils.js. See Comments 17 and 18 for the next steps, let me know if you need help getting it running.

Comment 25

[Brian Grinstead [:bgrins]]

Attachment: 1164195-rebased.patch

Went ahead and rebased the patch with the latest file path

Comment 26

[Brian Grinstead [:bgrins]]

With the patch applied, the test can be run via: `./mach mochitest devtools/shared/webconsole/test/test_jsterm_xpath.html`

Comment 27

[Avedis Zeitounilian]

Can this bug be assigned to me? Im a student at Seneca College, learning how to fix bugs in open source projects and I think this is a good starting bug.

Comment 28

[Nicolas Chevobbe [:nchevobbe]]

Pradeep, are you currently working on this ?

Comment 29

[Pradeep Gangwar [:pradeepgangwar]]

No, I am currently not involved with this. :)

Comment 30

[George Lam]

Is this still available? I would like to work on this bug.

Comment 31

[Brian Grinstead [:bgrins]]

(In reply to George Lam from comment #30)
> Is this still available? I would like to work on this bug.

Yes, it is. The attachment may need rebasing but it should be a good starting place, along with Comment 17. Let us know if you have questions.

Comment 32

[George Lam]

Hello.

Has this file been moved?

devtools/shared/webconsole/test/test_jsterm_xpath.html

Thanks.

Comment 33

[Aanchal Agarwal]

Hi! I'm new in the community. Can I work on this?

Comment 34

[Nicolas Chevobbe [:nchevobbe]]

Hello Aanchal,

I'll assign the bug to you.
Since you're new, I suggest you to read https://docs.firefox-dev.tools/getting-started/ to know how to setup the work environment. Since the patch only needs JS change you can use Artifact builds which are way faster to work with (with a decent connection) than regular ones.

Comment 35

[Nicolas Chevobbe [:nchevobbe]]

Attachment: bug_1164195.patch

I rebased the existing patch, so you should be able to apply it on your mozilla-central repo.

Comment 36

[Nicolas Chevobbe [:nchevobbe]]

Hello Aanchal,

Sorry I didn't check on you earlier. Is there anything that blocked you here? Not having enough time is a totally valid and fine answer!

Could you tell me if you plan to work on this? If not, I'll unassign the bug so someone else can pick it up.
Thanks!

Comment 37

[Aanchal Agarwal]

(In reply to Nicolas Chevobbe from comment #36)

Hello Aanchal,

Sorry I didn't check on you earlier. Is there anything that blocked you here? Not having enough time is a totally valid and fine answer!

Could you tell me if you plan to work on this? If not, I'll unassign the bug so someone else can pick it up.
Thanks!

Hello,

I'm really sorry I didn't have enough time. It would be better if someone else picks this up as I'm not working on this currently.
Thanks!

Comment 38

[Nicolas Chevobbe [:nchevobbe]]

no worries :)

Comment 39

[Dhruvi Butti]

Hey, I am an outreachy applicant and I wanted to take up this issue.
Could you please assign me this issue?

Comment 40

[Dhruvi Butti]

Hey,
Can anyone help me with the first todo?
Thanks,
Dhruvi

Comment 41

[Getty Orawo]

(In reply to Nicolas Chevobbe from comment #38)

no worries :)

Would love to take this up. What protocol do I go through?

Comment 42

[Nicolas Chevobbe [:nchevobbe]]

Hello Dhruvi Butti, I'm going to assign you the bug.

You can have a look at http://docs.firefox-dev.tools/getting-started/ to setup the work environment. Make sure to use artifact builds when asked to as it's much faster.
Please feel free to ask any question, here or on our Slack.

Thanks!

Comment 43

[Dhruvi Butti]

Hey, I am not able to install arc. I tried all the steps provided here https://secure.phabricator.com/book/phabricator/article/arcanist_quick_start/ but I am getting the same error "bash: arc: command not found" again and again. I already have PHP and git installed and even the $PATH defined.
Could you please help me here?

Comment 44

[Nicolas Chevobbe [:nchevobbe]]

Hello Dhruvi. Can you try to install https://github.com/mozilla-conduit/review ? That's what I use to push patches to phabricator.

Comment 45

[Dhruvi Butti]

Attachment: Bug 1164195 - Add a resultType param to $x console helper. r=nchevobbe.

A new param is added to the function which has a default type.

Comment 46

[Dhruvi Butti]

Hey! Thanks for the help. Please review and tell me if missed something.

Comment 47

[Dhruvi Butti]

I did update it! How do I send it here again?

Comment 48

[Dhruvi Butti]

Wait I have to do some more changes. Sorry for the inconvenience.
Thanks,
Dhruvi

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/52e8296f4d36
Add a resultType param to $x console helper. .

Comment 50

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 52e8296f4d36 (Bug 1164195) as requested by nchevobbe for missing the reviewer information.

Backout: https://hg.mozilla.org/integration/autoland/rev/c4a4baf5067986533afa57e15a251b5d7f7f3bac

Comment 51

[Nicolas Chevobbe [:nchevobbe]]

clearing needinfo as I requested the backout

Comment 52

[Dhruvi Butti]

Attachment: Bug 1164195 - Add a resultType param to $x console helper. r=nchevobbe.

This makes it possible to pass a third parameter which is a XPathResult constant.
Test cases are added to ensure this works as expected.

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fcdfe2be33a5
Add a resultType param to $x console helper. r=nchevobbe.

Comment 54

[Oana Pop-Rus]

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

Comment 55

[Sebastian Zartner [:sebo]]

Coming back late to this, I wonder whether it was considered to implement this the way it was implemented in Firebug and actively decided against it.

For comparison, in Firebug you could use strings instead of the XPathResult constants, see https://github.com/firebug/firebug/blob/master/extension/content/firebug/console/commandLineAPI.js#L75-L96 and http://web.archive.org/web/20171027120636/https://getfirebug.com/wiki/index.php/$x#resultType. The reason I implemented it this way back then was that it's easier to remember and write the strings instead of the constants.

Nicolas, do you think it's worth changing the parameter to take strings instead of the constants or additionally to them? Or do you think we should keep the functionality as is?

Sebastian

Comment 56

[Nicolas Chevobbe [:nchevobbe]]

I didn't check the Firebug implementation before, so I didn't know about the string usage.
We can probably file a follow up bug to implement it