1032855 - Add rep for XPathResult display in the Console

Status: NEW

(DevTools :: Shared Components, defect, P3)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

When executing xpath query in the Console panel, the actual list of nodes is not displayed.

An example:
document.evaluate('//div', document.body, null, XPathResult.ANY_TYPE, null)

Shows:
XPathResult { resultType: 4, invalidIteratorState: false }

It should rather work similarly to document.querySelectorAll.

A example:
document.querySelectorAll("div")

Shows:
NodeList [ <div#content>, <div#test>, <div#hidden.myClass.hidden> ]


The list of nodes should be sent in the XPathResult RDP packet in a form of a preview (just like for other objects), so e.g. even the Variables View can properly show the result value.

This is also how Firebug does it.


Honza

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Joe, another potential Firebug gap...

Honza

Comment 2

[Joe Walker [:jwalker]]

Is XPath still important? I'm tempted to think that it's not a very important gap...

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I don't know how to determine whether it's important or not.
It's just an existing content API.

Honza

Comment 4

[Joe Walker [:jwalker]]

There was a recent thread on dev-platform "Is XPath still a thing?". Summary:

Eric Shepherd wrote:
> I think I know the answer to this, but want to confirm: is XPath a going
> concern?

Anne van Kesteren wrote:
> I don't think we should actively recommend it. We're maintaining the
> existing code, but are not upgrading our level of support or anything.
> And if we could get rid of the existing code, we would.

David Burns wrote:
> XPath is still a going concern from where I stand. Web Testing people,
> who use Selenium WebDriver, use XPath extensively since they struggle
> to get to have testable documents.

Andreas Tolfsen wrote:
> Consider something like
>
>    //table/tr/td[contains(., "foo")/../td[2]/input
>
> which will find the table cell containing the text "foo", then find
> the second cell in the same row with an input element.
>
> A few other limitations I've been informed of are:
>
> - Complex conditional statements such as looking up all classes of A,
>   but not A combined with B or A with C.
>
> - Subqueries such as //li[.//a], meaning all li elements that have a
>   as a child or descendant.  Although possibly this is addressed by !
>   in CSS4?

So in summary, I think we should see how many Selenium folks shout, since as far as I can see, that's 99% of the use-case for XPath right now.

CC: David Burns so he can start the shouting!

The question is not "Is XPath useful?", but "Should we support XPathResults in the console as high priority?".

Comment 5

[David Burns :automatedtester]

The Selenium user base, from my straw poll, use Firepath extensively and looking at AMO[1] there is still quite a lot of activity on this. I think dropping the Selenium community (which includes webdev and web testing) will cause a lot of harm to us (and this is purely opinion)

[1] https://addons.mozilla.org/en-US/firefox/addon/firepath/statistics/?last=30

Comment 6

[Aidan Harding]

Hi,

I'd like to help with Mozilla and a friend of mine ([e-mail]) suggested that this might be a good first bug.

Any objections to me working on it?

Comment 7

[Patrick Brosset <:pbro>]

No objections at all, in fact that'd be great!

Since this is your first bug, you should first make sure you have the dev environment in place: https://wiki.mozilla.org/DevTools/GetInvolved#Hacking

And once done, you'll need some guidance on how to fix the bug. msucan would be the best to help you here, but he's not available at the moment.

I can give some information about the client-side code for this as I worked on a similar bug to output NodeList nicely in the console, but I don't know that much about the server-side part.

For info, all of the devtools is split in 2: the "client-side" which is the toolbox itself, with all the panel, and the "server-side" which is the part that runs on the debugged device or tab. It is split this way so that it's possible to debug a remote mobile device for example.

Please do get started with the dev environment and build process and then ask for help on the code part.

Comment 8

[Aidan Harding]

Hi Patrick,

Thanks!

I've already managed to sort out the environment and poke at the nightly build via xcode debugger (I'm on OS X). So, I think I'm good there. 

From that DevTools Wiki and wandering around the source, I could see the split going on. Any help you could give on where to look more precisely would be helpful. Or actually, maybe the bug number that you worked on before would be useful. Your patch would probably tell me a lot about what happens generally inside the console.

If the bug isn't the right place to get help of this nature, I'm happy to take it to email/IRC/wherever.

Comment 9

[Patrick Brosset <:pbro>]

Awesome!
The bug is fine to ask for help. You'd probably get faster responses on mozilla IRC #devtools though.

Bug 843004 is where most of the work to make objects look nicer in the console output happened.
Bug 757866 is where DOM Nodes were made more interactive in the console output (hovering over them would highlight them in the page and clicking on an icon next to them would jump to the inspector and select them).

I guess we want the result of xpath queries to look somewhat like NodeList outputs, so looking at the code we have to format DOMNodes should be a good starting point.
To my knowledge, the server-side code is in http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webconsole.js and the client-side code in http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/
In particular, http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/console-output.js is where logs are formatted (look for Widgets.ObjectRenderers).

My opinion on this bug is that something like:
|document.evaluate('//div', document.body, null, XPathResult.ANY_TYPE, null)|
should not only result in a list of nodes in the console.
I think the XPathResult object (i.e. |XPathResult { resultType: 4, invalidIteratorState: false }|) should still be there because that's what the function returns, but maybe next to it on the same line, we should also be displaying the list of nodes.

Or maybe, the output should look something like:
XPathResult [ <div#content>, <div#test>, <div#hidden.myClass.hidden> ]
and clicking on the word XPathResult would bring up the sidebar and show the object properties.

Comment 10

[Aidan Harding]

A few thoughts/questions on this:

I'm not sure that return types of ORDERED_NODE_ITERATOR_TYPE and UNORDERED_NODE_ITERATOR_TYPE ought to display their nodes. If I asked for something in iterator format rather than snapshot, it would probably be because I didn't want all the data at once. 

I wanted to reference the XPathResult constants in my ObjectRenderer, but it is not defined at that point. Is there a recommended solution to that? I saw the use of 

const {Cc, Ci, Cu} = require("chrome");

...

if (this._message instanceof Ci.nsIDOMNode)

for DOM nodes, maybe I should do something similar, but I couldn't see the wood for the trees on that one.

Finally, the message that gets to the ObjectRenderer does seem to have enough information to display the nodes on snapshot types. If you were working with an XPathResult, you would call snapshotItem() on the result. But the message to the renderer doesn't seem to have a reference to the XPathResult. I dug around, but not found where that message is actually created, so I couldn't see how to pass any extra information over.

Help!

Comment 11

[Aidan Harding]

Actually, I've found the DebuggerServer.ObjectActorPreviewers.Object so I can pass over the relevant information to the ObjectRenderer. I have something that does display the nodes for a snapshot type of result.

Any input on whether or not the iterator results should display a list of nodes, and how I should properly refer to XPathResult constants would be appreciated, though.

Comment 12

[Patrick Brosset <:pbro>]

(In reply to Aidan Harding from comment #11)
> Actually, I've found the DebuggerServer.ObjectActorPreviewers.Object so I
> can pass over the relevant information to the ObjectRenderer. I have
> something that does display the nodes for a snapshot type of result.
Great! Please do share a patch here (even if WIP) so it's easier to discuss on technical issues you're facing.
> Any input on whether or not the iterator results should display a list of
> nodes, and how I should properly refer to XPathResult constants would be
> appreciated, though.
Ci.nsIDOMXPathResult should help! Also, I agree with you about not previewing nodes returned in iterators.
Note that the matching node for resultType FIRST_ORDERED_NODE_TYPE and ANY_UNORDERED_NODE_TYPE is already previewed because it comes as a property (singleNodeValue) of the returned object.
So, if I'm not mistaken, we only want to preview returned nodes for UNORDERED_NODE_SNAPSHOT_TYPE and ORDERED_NODE_SNAPSHOT_TYPE result types.

Comment 13

[Aidan Harding]

I've attached a patch as WIP, since I'm stuck.

What I want to do is add a new property to the preview of the XPathResult: snapshotItems. That would be the array of nodes in the snapshot.

But there seems to be something clever that I need to do with makeDebuggeeValue and createValueGrip. I couldn't really see how that works from looking at the other ObjectActorPreviewers. I've got something that I thought would do it in the patch, but it doesn't come through correctly.

Any help there greatly appreciated...

Comment 14

[Aidan Harding]

Attachment: xpath-console-output.patch

Comment 15

[Patrick Brosset <:pbro>]

Flagging myself as NI? so I remember to take a look at this later. I should have some time later today or tomorrow.

Comment 16

[Aidan Harding]

Attachment: xpath-console-output2.patch

This patch has the virtue of producing the required output

Comment 17

[Patrick Brosset <:pbro>]

(In reply to Aidan Harding from comment #16)
> Created attachment 8522432 [details] [diff] [review]
> xpath-console-output2.patch
> 
> This patch has the virtue of producing the required output
Bugzilla sees this patch as empty.

Comment 18

[Patrick Brosset <:pbro>]

Damn, I didn't intend to clear the NI?

Comment 19

[Patrick Brosset <:pbro>]

Comment on attachment 8521355 [details] [diff] [review]
xpath-console-output.patch

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

Nice, this almost works.
My main comment is: reusing code, both on the server and client.
The object you're trying to preview here is like other objects we already can preview, so there's a lot code around we can reuse.

The server-side part looks pretty good, I just have a few comments about simplification, and putting code in common in a new function.
The client-side part looks too complex compared to what it could be if you were reusing some of the code we have to format objects.

Other than that, this is the right approach, you're on the right track.
Thanks for this patch!

::: browser/devtools/webconsole/console-output.js
@@ +1178,5 @@
>      if (preview && preview.kind) {
>        widget = Widgets.ObjectRenderers.byKind[preview.kind];
>      }
>  
> +    if (!widget || objectActor.class == "XPathResult" || (widget.canRender && !widget.canRender(objectActor))) {

Why is this needed?
It appears the code has changed since and this part doesn't apply anymore. Furthermore, I tested without that change at all, and the new renderer you added still got called, so I'm thinking this is unneeded.

@@ +3091,5 @@
> +Widgets.ObjectRenderers.add({
> +  byClass: "XPathResult",
> +
> +  render: function()
> +  {

This function looks like a lot of code for something that's already handled elsewhere in this file.
What we really want to do here is format the object like any other standard object (because that's what it is) + in some cases, if there are snapshotItems, display an array-like list of DOM nodes, right?
So we need to make sure we reuse as much of the existing code as possible.

If you look at the Widgets.ObjectRenderers Object kind render method, I think we should basically just copy paste it here, and then add a part to render the list of snapshot items. Something like this should work:

  render: function()
  {
    let { ownProperties, safeGetterValues } = this.objectActor.preview;
    if ((!ownProperties && !safeGetterValues) || this.options.concise) {
      this._renderConciseObject();
      return;
    }

    this._renderObjectPrefix();
    this._renderObjectProperties(this.element, false);
    this._renderObjectSuffix();

    let {preview} = this.objectActor;
    if (!preview.snapshotItems) {
      return;
    }

    this._text(" snapshot: ");

    let isFirst = true;
    let renderSeparator = () => !isFirst && this._text(", ");
    for (let item of preview.snapshotItems) {
      renderSeparator();
      isFirst = false;
      let elem = this.message._renderValueGrip(item, { concise: true });
      this.element.appendChild(elem);
    }
  }

@@ +3116,5 @@
> +      9 : "FIRST_ORDERED_NODE_TYPE"
> +      };
> +    for(let c in XPATH_CONSTANTS) { 
> +      XPATH_CONSTANTS[XPATH_CONSTANTS[c]] = c; 
> +    }

Why do you want to test the resultType here? I think just checking if the preview has snapshotItems is enough to decide whether or not you want to preview them.

@@ +3149,5 @@
> +      valueElem = this.message._renderValueGrip(resultType, { concise: true });
> +    } else {
> +      valueElem = this.el("span.console-string", namedConstantValue);
> +      canShowResult = true;
> +    }

This part looks a bit complex to me (took me some time to understand what you were wanting to do here).
Why not just use this part only:
valueElem = this.message._renderValueGrip(resultType, { concise: true });
and get rid of namedConstantValue and the whole if/else?

In fact, while we're at it, we might as well remove the whole thing (from let resultTypeKey = "resultType"; to here), and let the for loop below handle the preview of the object. 
This would simplify things a lot.

Also, canShowResult seems unused.

@@ +3155,5 @@
> +    container.appendChild(valueElem);
> +
> +    for (let key of Object.keys(safeGetterValues || {})) {
> +
> +      if(key != resultTypeKey) {

Based on my previous comment, this line should be removed.

@@ +3164,5 @@
> +
> +    this._text(" }");
> +
> +    if(resultType == XPATH_CONSTANTS.UNORDERED_NODE_SNAPSHOT_TYPE
> +        || resultType == XPATH_CONSTANTS.ORDERED_NODE_SNAPSHOT_TYPE) {

Based on my comment above, this can be replaced by a check for preview.snapshotItems

@@ +3165,5 @@
> +    this._text(" }");
> +
> +    if(resultType == XPATH_CONSTANTS.UNORDERED_NODE_SNAPSHOT_TYPE
> +        || resultType == XPATH_CONSTANTS.ORDERED_NODE_SNAPSHOT_TYPE) {
> +      this._text(" snapshot: ");

This should localized. There's a localization utility available in this file. Search for 'l10n.getStr'. 
Actual string values go here: browser/locales/en-US/chrome/browser/devtools/webconsole.properties

::: toolkit/devtools/server/actors/script.js
@@ +4168,5 @@
> +      ownProperties: Object.create(null),
> +    };
> +
> +    // TODO: get these constants from where they are defined
> +    

nit: trailing whitespaces.

@@ +4169,5 @@
> +    };
> +
> +    // TODO: get these constants from where they are defined
> +    
> +    if(aRawObj.resultType == 6 || aRawObj.resultType == 7) {

You can use aRawObj.ORDERED_NODE_SNAPSHOT_TYPE and aRawObj.UNORDERED_NODE_SNAPSHOT_TYPE

@@ +4170,5 @@
> +
> +    // TODO: get these constants from where they are defined
> +    
> +    if(aRawObj.resultType == 6 || aRawObj.resultType == 7) {
> +      let snapshotItems = new Array();

let snapshotItems = [];

@@ +4172,5 @@
> +    
> +    if(aRawObj.resultType == 6 || aRawObj.resultType == 7) {
> +      let snapshotItems = new Array();
> +
> +      for(let i=0; i < aRawObj.snapshotLength; i++) {

nit: Please use spaces around the = sign and after for:
for (let i = 0; i < aRawObj.snapshotLength; i ++) {

Also, to reduce protocol traffic, we usually cap list of previewed objects to OBJECT_PREVIEW_MAX_ITEMS.
So something like this instead:

for (let i = 0; i < aRawObj.snapshotLength && i < OBJECT_PREVIEW_MAX_ITEMS; i ++) {

@@ +4173,5 @@
> +    if(aRawObj.resultType == 6 || aRawObj.resultType == 7) {
> +      let snapshotItems = new Array();
> +
> +      for(let i=0; i < aRawObj.snapshotLength; i++) {
> +        let thisItem = aRawObj.snapshotItem(i);

I think the name |item| is fine here instead of |thisItem|

@@ +4180,5 @@
> +      }
> +      let debuggeeSnapshotItems = makeDebuggeeValueIfNeeded(aObjectActor.threadActor, snapshotItems);
> +      let gripSnapshotItems = aObjectActor.threadActor.createValueGrip(debuggeeSnapshotItems);
> +
> +      preview.snapshotItems = gripSnapshotItems;

I think it will work better if you replace the last 3 lines with just:

preview.snapshotItems = snapshotItems;

@@ +4183,5 @@
> +
> +      preview.snapshotItems = gripSnapshotItems;
> +    }
> +
> +    try {

The rest of this function is the same as the GenericObject function below, which is fine, because that's what we want.
But we need to remove the duplicated code though: can you define a function that does what the GenericObject function outside of th DebuggerServer.ObjectActorPreviewers.Object object, so that its available at the module scope, and then make GenericObject and XPathResult call this new function.
XPathResult would then on top add its snapshotItems to the preview.

Comment 20

[Aidan Harding]

Patrick - just wanted to quickly say thanks for the feedback... and that I'm snowed under in the day job, but I haven't run away from this bug :)