$x should accept the resultType argument

RESOLVED FIXED in Firefox 67

Status

enhancement
RESOLVED FIXED
4 years ago
2 months ago

People

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

Tracking

(Blocks 1 bug, {dev-doc-needed, good-first-bug})

unspecified
Firefox 67

Firefox Tracking Flags

(firefox67 fixed)

Details

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

Attachments

(2 attachments, 8 obsolete attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Updated

4 years ago
Component: Untriaged → Developer Tools: Console
Whiteboard: [good-first-bug][firebug-gap]

Comment 1

4 years ago
Hi,
Could you please assign this bug to me?
Thanks,
Sushrut
(Reporter)

Comment 2

4 years ago
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
Flags: needinfo?(bgrinstead)
Assignee: nobody → sushrutbhalla
Flags: needinfo?(bgrinstead)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Comment 3

4 years ago
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.
Flags: needinfo?(bgrinstead)
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.
(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.
Assignee: sushrutbhalla → vijendrasingh161
Mentor: bgrinstead, fayolle-florent
Flags: needinfo?(bgrinstead)

Comment 6

4 years ago
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.
Attachment #8614766 - Attachment is obsolete: true
Attachment #8615127 - Flags: review?(bgrinstead)
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`
Attachment #8615127 - Flags: review?(bgrinstead) → feedback+
Posted file test_jsterm_xpath.html (obsolete) —
Starter for the test case, as promised in Comment 7.  Just needs an implementation for checkResultType.
Whiteboard: [good-first-bug][firebug-gap] → [good first bug][firebug-gap]
No updates in many months, unassigning.
Assignee: vijendrasingh161 → nobody
Status: ASSIGNED → NEW

Comment 10

4 years ago
Posted patch Bug1164195.patch (obsolete) — Splinter Review
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.
Flags: needinfo?(bgrinstead)
Attachment #8685769 - Flags: feedback?(bgrinstead)
Posted patch xpath-diff.patch (obsolete) — Splinter Review
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.
Flags: needinfo?(bgrinstead)
Comment on attachment 8685769 [details] [diff] [review]
Bug1164195.patch

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

Looks generally good, see attached test fixes
Attachment #8685769 - Flags: feedback?(bgrinstead) → feedback+

Comment 13

4 years ago
Posted patch Bug1164195.patch (obsolete) — Splinter Review
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 ?
Attachment #8685769 - Attachment is obsolete: true
Flags: needinfo?(bgrinstead)
Attachment #8686132 - Flags: feedback?(bgrinstead)
(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.
Flags: needinfo?(bgrinstead)

Comment 15

4 years ago
Understood.

Is my patch looks OK for merge or do I need to change it more?
Assignee: nobody → alexey.novak.mail
Status: NEW → ASSIGNED
Attachment #8615127 - Attachment is obsolete: true
Attachment #8615501 - Attachment is obsolete: true
Attachment #8686080 - Attachment is obsolete: true
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.
Keywords: dev-doc-needed
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).
Attachment #8686132 - Flags: feedback?(bgrinstead) → feedback+

Comment 18

4 years ago
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.
Severity: normal → enhancement
Version: 41 Branch → unspecified
Duplicate of this bug: 1227040

Comment 20

4 years ago
Sorry I had a deliverable to meet at work last week. I will try to resolve this bug this week.
[good first bug] whiteboard -> keyword mass change
Keywords: good-first-bug
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
Assignee: alexey.novak.mail → nobody
Status: ASSIGNED → NEW
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.
Flags: needinfo?(bgrinstead)
(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.
Flags: needinfo?(bgrinstead)
Posted patch 1164195-rebased.patch (obsolete) — Splinter Review
Went ahead and rebased the patch with the latest file path
Attachment #8686132 - Attachment is obsolete: true
With the patch applied, the test can be run via: `./mach mochitest devtools/shared/webconsole/test/test_jsterm_xpath.html`
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.
Pradeep, are you currently working on this ?
Flags: needinfo?(pradeepgangwar39)
No, I am currently not involved with this. :)
Flags: needinfo?(pradeepgangwar39)

Comment 30

a year ago
Is this still available? I would like to work on this bug.
(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

a year ago
Hello.

Has this file been moved?

devtools/shared/webconsole/test/test_jsterm_xpath.html

Thanks.

Updated

11 months ago
Product: Firefox → DevTools

Comment 33

8 months ago
Hi! I'm new in the community. Can I work on this?
Flags: needinfo?(bgrinstead)
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.
Assignee: nobody → aanchal138
Flags: needinfo?(bgrinstead)
I rebased the existing patch, so you should be able to apply it on your mozilla-central repo.
Attachment #8915715 - Attachment is obsolete: true

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!

Flags: needinfo?(aanchal120btcse16)

Comment 37

3 months ago

(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!

Updated

3 months ago
Flags: needinfo?(aanchal120btcse16)

no worries :)

Assignee: aanchal120btcse16 → nobody
(Assignee)

Comment 39

3 months ago

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

(Assignee)

Comment 40

3 months ago

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

Comment 41

3 months ago

(In reply to Nicolas Chevobbe from comment #38)

no worries :)

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

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!

Assignee: nobody → dhruvibutti9477
Mentor: bgrinstead, fayolle-florent → nchevobbe
Status: NEW → ASSIGNED
(Assignee)

Comment 43

3 months ago

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?

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

(Assignee)

Comment 45

3 months ago

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

(Assignee)

Comment 46

3 months ago

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

(Assignee)

Comment 47

3 months ago

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

(Assignee)

Comment 48

3 months ago

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

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

Comment 49

2 months ago
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/52e8296f4d36
Add a resultType param to $x console helper. .

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

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

Flags: needinfo?(dhruvibutti9477)

clearing needinfo as I requested the backout

Flags: needinfo?(dhruvibutti9477)
Attachment #9047262 - Attachment is obsolete: true
(Assignee)

Comment 52

2 months ago

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

2 months ago
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fcdfe2be33a5
Add a resultType param to $x console helper. r=nchevobbe.

Comment 54

2 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
You need to log in before you can comment on or make changes to this bug.