Closed Bug 1602591 Opened 5 years ago Closed 4 years ago

Change result type parameter of $x to accept strings instead or additionally to XPathResult constants

Categories

(DevTools :: Console, enhancement, P4)

enhancement

Tracking

(firefox74 verified)

VERIFIED FIXED
Firefox 74
Tracking Status
firefox74 --- verified

People

(Reporter: sebo, Assigned: seifeddinebesbes, Mentored)

References

Details

(Keywords: dev-doc-complete, good-first-bug, Whiteboard: [lang=js])

Attachments

(1 file, 1 obsolete file)

In bug 1164195 the support for a parameter for the $x() command was implemented that allows to specify the variable type of the returned value of the function.

This feature was implemented using the XPathResult constants.

This implementation is different to the one of Firebug, though, which used strings instead of those constants. The reason behind this was to make it easy for users to specify those values and to avoid having to remember XPathResult API and its constants.

The allowed values according to Firebug's implementation should be "number", "string", "bool" (could be "boolean", though), "node", and "nodes".

The implementation in the Firefox DevTools should follow this and provide the possibility to use those strings instead of the constants. This might be as a replacement for or in addition to them.

Sebastian

The implementation should be relatively easy, so I marked this as good-first-bug and added myself as mentor.

The related code is here:

https://searchfox.org/mozilla-central/rev/23d4bffcad365e68d2d45776017056b76ca9a968/devtools/server/actors/webconsole/utils.js#445-469

For reference, here's the original Firebug issue for that functionality and the related commit:
https://code.google.com/archive/p/fbug/issues/18
https://github.com/firebug/firebug/commit/3b3147ea3a2d568bc3d111815da7d23ceeeb8bb9

From what I saw in the code right now, the DevTools implementation differs a bit from the one in Firebug. As the DevTools implementation currently uses the XPathResult constants directly, it implements all of them. Firebug restricted them internally to XPathResult.NUMBER_TYPE, XPathResult.STRING_TYPE, XPathResult.BOOLEAN_TYPE, XPathResult.FIRST_ORDERED_NODE_TYPE, and XPathResult.UNORDERED_NODE_ITERATOR_TYPE (mapped by the strings mentioned previously).

So there are two questions to be clarified before implementing this:

  1. Should the implementation allow both constants and strings to be used?
  2. Should the same strings as in Firebug be used and should they map to the same constants?

I slightly tend to only support strings because function overloading causes complexity (in the code, in usage, and documentation), even when it means to change existing behavior and using the XPathResult API allows autocompleting the values. Though I'd be ok if we decide to support both.
As the implementor of that function in Firebug, I am open in regard of the naming of the strings and their mappings or add more to cover other the other constants. I even guess the mapping between "nodes" and XPathResult.UNORDERED_NODE_ITERATOR_TYPE might rather have been XPathResult.ORDERED_NODE_SNAPSHOT_TYPE, but that needs to be tested out.

Nicolas, any thoughts on those two questions?

Sebastian

Mentor: sebastianzartner
Flags: needinfo?(nchevobbe)
Keywords: good-first-bug
Whiteboard: [lang=js]

Hello Sebastian,

I am a new contributor and i want to start with this implementation as my first "good-first-bug".
it's possible ?

Seif

Flags: needinfo?(sebastianzartner)

Hi Seif,

sure, you can work on this bug. Though note that there are two open questions regarding the implementation (see comment 1). So, you may already set up your development environment, try to change the code a bit and build Firefox, but you should wait until those two questions are clarified before creating the actual patch.

See https://firefox-dev.tools/ and especially https://docs.firefox-dev.tools/ for more info on how to set up the development environment and how to contribute and let me know if you need any help.

Sebastian

Flags: needinfo?(sebastianzartner)

Nicolas, any thoughts on those two questions?

not really. I'm not sure this is widely used, so let's stick to what Firebug was doing

Flags: needinfo?(nchevobbe)
Priority: -- → P4

Hello Seif, any progress on this issue? If not, I would love to contribute and claim this issue.

(In reply to parshva.barbhaya from comment #5)

Hello Seif, any progress on this issue? If not, I would love to contribute and claim this issue.

let's wait for ~1 week, and if we don't here back from them, I'll assign the bug to you :)

Flags: needinfo?(seifeddinebesbes)

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #6)

let's wait for ~1 week, and if we don't here back from them, I'll assign the bug to you :)

Thank you for the prompt response.

Hello Nicolas, Parshva,

I am sorry to take this long to respond, I have been a little busy these past few weeks, but now I am free and I will try to submit a fix in two days max.

Seif

Flags: needinfo?(seifeddinebesbes)
Assignee: nobody → seifeddinebesbes
Status: NEW → ASSIGNED
Attachment #9122592 - Attachment is obsolete: true
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05a2fd32061f
Change result type parameter of $x to accept strings. r=nchevobbe
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 74

I just realized I didn't give the changes a try so far. So I did it now in 74.0b3 and Nightly 75.0a1 (2020-02-15) and it works as expected.

Thank you very much for the patch, Seif!

Sebastian

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: