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

Status: VERIFIED FIXED

(DevTools :: Console, enhancement, P4)

Description

[Sebastian Zartner [:sebo]]

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

Comment 1

[Sebastian Zartner [:sebo]]

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

Comment 2

[Seifeddine Besbes]

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

Comment 3

[Sebastian Zartner [:sebo]]

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

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 5

[parshva.barbhaya]

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

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

(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 :)

Comment 7

[parshva.barbhaya]

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

Comment 8

[Seifeddine Besbes]

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

Comment 9

[Seifeddine Besbes]

Attachment: Bug 1602591 - Change result type parameter of $x to accept strings. r=nchevobbe

Comment 10

[Seifeddine Besbes]

Attachment: Bug 1602591 - Change result type parameter of $x to accept strings. r=nchevobbe

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05a2fd32061f
Change result type parameter of $x to accept strings. r=nchevobbe

Comment 12

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/05a2fd32061f

Comment 13

[Sebastian Zartner [:sebo]]

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