877300 - Add a basic DOM walker actor

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Dave Camp (:dcamp)]

Attachment: WIP 1

The attached patch adds some basic dom walking to the debugger server.

Comment 1

[Dave Camp (:dcamp)]

Attachment: v1

Comment 2

[Joe Walker [:jwalker]]

Comment on attachment 756910 [details] [diff] [review]
v1

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

I wonder if the confusion between helpers.js and helpers.js is too great.
See also /browser/devtoools/commandline/test/helpers.js, which is used from anything that tests commands.

::: toolkit/devtools/server/actors/inspector.js
@@ +36,5 @@
> +
> +const protocol = require("devtools/server/protocol");
> +const {Arg, Option, method, RetVal, types} = protocol;
> +const {LongStringActor, ShortLongString} = require("devtools/server/actors/string");
> +const promise = require("sdk/core/promise");

I think we spell this 'Promise' in the rest of devtools

@@ +52,5 @@
> +};
> +
> +// XXX: A poor man's makeInfallible until we move it out of transport.js
> +// Which should be very soon.
> +function makeInfallible(handler) {

I wonder if logExceptionWrapper would be clearer? We're not stopping anything from falling are we.

@@ +70,5 @@
> +    try {
> +      deferred.resolve(value);
> +    } catch(ex) {
> +      console.error(ex);
> +    }

WAT!
Is "deferred.resolve(value)" likely to except?
And if it does, is it going to throw armour piercing exceptions that need 2 layers of catches?

@@ +103,5 @@
> +    this.rawNode = node;
> +  },
> +
> +  toString: function() {
> +    return "[NodeActor for " + this.rawNode.toString() + "]";

document.body.toString() -> "[object HTMLBodyElement]"
so:
this would give us "[NodeActor for [object HTMLBodyElement]]"
I guess it's only debug. Just wondering if it's what you meant.

@@ +159,5 @@
> +    return new LongStringActor(this.conn, this.rawNode.nodeValue || "");
> +  }, {
> +    request: {},
> +    response: {
> +      value: RetVal("longstring")

Since this will work for short strings too, perhaps just 'value'?
Except that's special...?

@@ +208,5 @@
> +    this._classList = this.className.split(/\s+/);
> +    this._classList.contains = function(cls) {
> +      return this.indexOf(cls) > 0;
> +    }.bind(this._classList);
> +    return this._classList;

The classList API is add/remove/contains/toggle ?
http://davidwalsh.name/classlist

@@ +215,5 @@
> +  get hasChildren() this._form.numChildren > 0,
> +  get numChildren() this._form.numChildren,
> +
> +  get tagName() this.nodeType === Ci.nsIDOMNode.ELEMENT_NODE ? this.nodeName : null,
> +  get nodeValue() this._form.value,

I wonder if we should call this truncatedValue or something?

@@ +216,5 @@
> +  get numChildren() this._form.numChildren,
> +
> +  get tagName() this.nodeType === Ci.nsIDOMNode.ELEMENT_NODE ? this.nodeName : null,
> +  get nodeValue() this._form.value,
> +  get incompleteNodeValue() !!this._form.incompleteValue,

I think the API user is going to know ahead of time if they're happy with the truncated value or not, so asking them to check is annoying.

    var foo = nodeFront.truncatedValue;
or
    nodeFront.getNodeValue(...);

And we don't need incompleteNodeValue
?

@@ +330,5 @@
> +   * Get a range of the items from the node list.
> +   */
> +  items: method(function(start=0, end=this.nodeList.length) {
> +    return [this.walker._ref(item)
> +            for (item of Array.prototype.slice.call(this.nodeList, start, end))];

Do we have a policy on JS that's Mozilla specific?

@@ +345,5 @@
> +
> +/**
> + * Client side of a node list as returned by querySelectorAll()
> + */
> +var NodeListFront = exports.NodeLIstFront = protocol.FrontClass(NodeListActor, {

NodeLIstFront -> NodeListFront

@@ +517,5 @@
> +   *    `center`: If a node is specified, the given node will be as centered
> +   *       as possible in the list, given how close to the ends of the child
> +   *       list it is.  Mutually exclusive with `start`.
> +   *    `whatToShow`: A bitmask of node types that should be included.  See
> +   *       https://developer.mozilla.org/en-US/docs/Web/API/NodeFilter.

Nice!

@@ +567,5 @@
> +      backwardWalker.previousSibling();
> +      let backwardCount = Math.floor(maxNodes / 2);
> +      let backwardNodes = this._readBackward(backwardWalker, backwardCount);
> +      nodes = backwardNodes;
> +    }

Couldn't we do this by working out the index of the start/center node and then doing some math?
Not sure it's worth re-writing, and there's probably a good reason why it wasn't done that way to start with that I've not seen yet.

::: toolkit/devtools/server/main.js
@@ +533,5 @@
>        let prefix = aActor.actorPrefix;
>        if (typeof aActor == "function") {
>          prefix = aActor.prototype.actorPrefix;
>        }
> +      dump("trying to add actor " + aActor + " to conn: " + this + "\n");

I've typically avoided dump in live code. Is that not general?

::: toolkit/devtools/server/protocol.js
@@ +744,5 @@
> +      try {
> +        throw Error("here");
> +      } catch(ex) {
> +        dump(ex.stack);
> +      }

Probably want to get rid of this?
Also:
  console.trace();

Comment 3

[Dave Camp (:dcamp)]

(In reply to Joe Walker [:jwalker] from comment #2)
> Comment on attachment 756910 [details] [diff] [review]
> v1
> 
> Review of attachment 756910 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I wonder if the confusion between helpers.js and helpers.js is too great.
> See also /browser/devtoools/commandline/test/helpers.js, which is used from
> anything that tests commands.

You know, this is the second time generic test helpers have lost a naming fight with specific command line test helpers.  Maybe we should call those command-helpers.js?

But I guess I can find something else this time around.

> ::: toolkit/devtools/server/actors/inspector.js
> @@ +36,5 @@
> > +
> > +const protocol = require("devtools/server/protocol");
> > +const {Arg, Option, method, RetVal, types} = protocol;
> > +const {LongStringActor, ShortLongString} = require("devtools/server/actors/string");
> > +const promise = require("sdk/core/promise");
> 
> I think we spell this 'Promise' in the rest of devtools

We do, but lowercase is the standard for sdk requires.  I'd prefer to be eventually consistent with sdk practices down the road, but I could be convinced otherwise.

> @@ +52,5 @@
> > +// XXX: A poor man's makeInfallible until we move it out of transport.js
> > +// Which should be very soon.
> > +function makeInfallible(handler) {
> 
> I wonder if logExceptionWrapper would be clearer? We're not stopping
> anything from falling are we.

Yeah possibly - this function already exists (see bug 878319) but we can followup to change its name?

 
> @@ +70,5 @@
> > +    try {
> > +      deferred.resolve(value);
> > +    } catch(ex) {
> > +      console.error(ex);
> > +    }
> 
> WAT!

Yeah, that's probably unnecessary.

> @@ +103,5 @@
> > +    this.rawNode = node;
> > +  },
> > +
> > +  toString: function() {
> > +    return "[NodeActor for " + this.rawNode.toString() + "]";
> 
> document.body.toString() -> "[object HTMLBodyElement]"
> so:
> this would give us "[NodeActor for [object HTMLBodyElement]]"
> I guess it's only debug. Just wondering if it's what you meant.

That is what I want, but [NodeActor conn0.domnode01 for [object HTMLBodyElement]] might be preferable...

> @@ +159,5 @@
> > +    return new LongStringActor(this.conn, this.rawNode.nodeValue || "");
> > +  }, {
> > +    request: {},
> > +    response: {
> > +      value: RetVal("longstring")
> 
> Since this will work for short strings too, perhaps just 'value'?
> Except that's special...?

Not sure what you mean - the packet property is named "value" and it returns a LongStringActor (which is registered with the type system as 'longstring').

> @@ +208,5 @@
> > +    this._classList = this.className.split(/\s+/);
> > +    this._classList.contains = function(cls) {
> > +      return this.indexOf(cls) > 0;
> > +    }.bind(this._classList);
> > +    return this._classList;
> 
> The classList API is add/remove/contains/toggle ?
> http://davidwalsh.name/classlist

Yeah, but add/remove/toggle all mutate synchronously, which we can't do.  Will be adding changes in a followup patch.

> @@ +215,5 @@
> > +  get hasChildren() this._form.numChildren > 0,
> > +  get numChildren() this._form.numChildren,
> > +
> > +  get tagName() this.nodeType === Ci.nsIDOMNode.ELEMENT_NODE ? this.nodeName : null,
> > +  get nodeValue() this._form.value,
> 
> I wonder if we should call this truncatedValue or something?

Yeah, maybe truncatedValue (it's not always truncated, but...)

> @@ +216,5 @@
> > +  get numChildren() this._form.numChildren,
> > +
> > +  get tagName() this.nodeType === Ci.nsIDOMNode.ELEMENT_NODE ? this.nodeName : null,
> > +  get nodeValue() this._form.value,
> > +  get incompleteNodeValue() !!this._form.incompleteValue,
> 
> I think the API user is going to know ahead of time if they're happy with
> the truncated value or not, so asking them to check is annoying.
> 
>     var foo = nodeFront.truncatedValue;
> or
>     nodeFront.getNodeValue(...);
> 
> And we don't need incompleteNodeValue
> ?

They certainly can use that setup, but you still might want incompleteNodeValue for presentation purposes, right?  Maybe to elide the value if it's truncated?

> @@ +330,5 @@
> > +   * Get a range of the items from the node list.
> > +   */
> > +  items: method(function(start=0, end=this.nodeList.length) {
> > +    return [this.walker._ref(item)
> > +            for (item of Array.prototype.slice.call(this.nodeList, start, end))];
> 
> Do we have a policy on JS that's Mozilla specific?

Discussed on irc: Will try to use [...set] syntax when it works, and the (going to be changed for es6) array comprehension syntax elsewhere.

> @@ +567,5 @@
> > +      backwardWalker.previousSibling();
> > +      let backwardCount = Math.floor(maxNodes / 2);
> > +      let backwardNodes = this._readBackward(backwardWalker, backwardCount);
> > +      nodes = backwardNodes;
> > +    }
> 
> Couldn't we do this by working out the index of the start/center node and
> then doing some math?
> Not sure it's worth re-writing, and there's probably a good reason why it
> wasn't done that way to start with that I've not seen yet.

This is mostly pulled from the current MarkupView implementation.  It could be simpler, but I'd prefer a followup for that (in particular, I'd want to measure how perf is impacted from a clean slate, rather than letting it be hidden by perf changes in the remote inspector)

> ::: toolkit/devtools/server/main.js
> @@ +533,5 @@
> >        let prefix = aActor.actorPrefix;
> >        if (typeof aActor == "function") {
> >          prefix = aActor.prototype.actorPrefix;
> >        }
> > +      dump("trying to add actor " + aActor + " to conn: " + this + "\n");
> 
> I've typically avoided dump in live code. Is that not general?

That's bad patch submission, will clean up dumps but please always point them out :)

> 
> ::: toolkit/devtools/server/protocol.js
> @@ +744,5 @@
> > +      try {
> > +        throw Error("here");
> > +      } catch(ex) {
> > +        dump(ex.stack);
> > +      }
> 
> Probably want to get rid of this?
> Also:
>   console.trace();

Yeah console.trace is way better, thanks.

Comment 4

[Joe Walker [:jwalker]]

(In reply to Dave Camp (:dcamp) from comment #3)
> (In reply to Joe Walker [:jwalker] from comment #2)
> > Comment on attachment 756910 [details] [diff] [review]
> > v1
> > 
> > Review of attachment 756910 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I wonder if the confusion between helpers.js and helpers.js is too great.
> > See also /browser/devtoools/commandline/test/helpers.js, which is used from
> > anything that tests commands.
> 
> You know, this is the second time generic test helpers have lost a naming
> fight with specific command line test helpers.  Maybe we should call those
> command-helpers.js?
> 
> But I guess I can find something else this time around.

Yeah, although would the other 2 competitors have conflicted with each other?

(command)helpers.js does contain some genuinely general helpers. It's got the best addTab anywhere imho. But next time I've got the hood open there, I'll look at changing the name.

> > ::: toolkit/devtools/server/actors/inspector.js
> > @@ +36,5 @@
> > > +
> > > +const protocol = require("devtools/server/protocol");
> > > +const {Arg, Option, method, RetVal, types} = protocol;
> > > +const {LongStringActor, ShortLongString} = require("devtools/server/actors/string");
> > > +const promise = require("sdk/core/promise");
> > 
> > I think we spell this 'Promise' in the rest of devtools
> 
> We do, but lowercase is the standard for sdk requires.  I'd prefer to be
> eventually consistent with sdk practices down the road, but I could be
> convinced otherwise.

I was just thinking that 'promise' is far more sensible.
Added to thu meeting

> > @@ +52,5 @@
> > > +// XXX: A poor man's makeInfallible until we move it out of transport.js
> > > +// Which should be very soon.
> > > +function makeInfallible(handler) {
> > 
> > I wonder if logExceptionWrapper would be clearer? We're not stopping
> > anything from falling are we.
> 
> Yeah possibly - this function already exists (see bug 878319) but we can
> followup to change its name?

Yes, but I think we should only file a followup if we're actually going to fix it.

> > @@ +159,5 @@
> > > +    return new LongStringActor(this.conn, this.rawNode.nodeValue || "");
> > > +  }, {
> > > +    request: {},
> > > +    response: {
> > > +      value: RetVal("longstring")
> > 
> > Since this will work for short strings too, perhaps just 'value'?
> > Except that's special...?
> 
> Not sure what you mean - the packet property is named "value" and it returns
> a LongStringActor (which is registered with the type system as 'longstring').

Ah, sorry got the uses of the strings mixed up.
I should probably convert the GcliActor to protocol.js

> > @@ +208,5 @@
> > > +    this._classList = this.className.split(/\s+/);
> > > +    this._classList.contains = function(cls) {
> > > +      return this.indexOf(cls) > 0;
> > > +    }.bind(this._classList);
> > > +    return this._classList;
> > 
> > The classList API is add/remove/contains/toggle ?
> > http://davidwalsh.name/classlist
> 
> Yeah, but add/remove/toggle all mutate synchronously, which we can't do. 
> Will be adding changes in a followup patch.

OK, but we should not expose the array API?

> > @@ +216,5 @@
> > > +  get numChildren() this._form.numChildren,
> > > +
> > > +  get tagName() this.nodeType === Ci.nsIDOMNode.ELEMENT_NODE ? this.nodeName : null,
> > > +  get nodeValue() this._form.value,
> > > +  get incompleteNodeValue() !!this._form.incompleteValue,
> > 
> > I think the API user is going to know ahead of time if they're happy with
> > the truncated value or not, so asking them to check is annoying.
> > 
> >     var foo = nodeFront.truncatedValue;
> > or
> >     nodeFront.getNodeValue(...);
> > 
> > And we don't need incompleteNodeValue
> > ?
> 
> They certainly can use that setup, but you still might want
> incompleteNodeValue for presentation purposes, right?  Maybe to elide the
> value if it's truncated?

OK, good point.

> > @@ +567,5 @@
> > > +      backwardWalker.previousSibling();
> > > +      let backwardCount = Math.floor(maxNodes / 2);
> > > +      let backwardNodes = this._readBackward(backwardWalker, backwardCount);
> > > +      nodes = backwardNodes;
> > > +    }
> > 
> > Couldn't we do this by working out the index of the start/center node and
> > then doing some math?
> > Not sure it's worth re-writing, and there's probably a good reason why it
> > wasn't done that way to start with that I've not seen yet.
> 
> This is mostly pulled from the current MarkupView implementation.  It could
> be simpler, but I'd prefer a followup for that (in particular, I'd want to
> measure how perf is impacted from a clean slate, rather than letting it be
> hidden by perf changes in the remote inspector)

Followup is fine. Never is too.
I'd be surprised if it materially affected perf. It was more about future maintainability. I had a preconception about how the function was going to work, and an "um, ok, that's bizarre moment" wondering what I was missing. But doing work to avoid the same level of future potential work doesn't make much sense.

Comment 5

[Dave Camp (:dcamp)]

(In reply to Joe Walker [:jwalker] from comment #4)
> (In reply to Dave Camp (:dcamp) from comment #3)
> > (In reply to Joe Walker [:jwalker] from comment #2)
> > This is mostly pulled from the current MarkupView implementation.  It could
> > be simpler, but I'd prefer a followup for that (in particular, I'd want to
> > measure how perf is impacted from a clean slate, rather than letting it be
> > hidden by perf changes in the remote inspector)
> 
> Followup is fine. Never is too.
> I'd be surprised if it materially affected perf. It was more about future
> maintainability. I had a preconception about how the function was going to
> work, and an "um, ok, that's bizarre moment" wondering what I was missing.
> But doing work to avoid the same level of future potential work doesn't make
> much sense.

I remember why I did it this way now:  The walker filters out some nodes (depending on whatToShow and our hack to remove empty text nodes), using index math would either generate bad results if we did it on the childNodes array or require building an entire filtered array, which would be bad on large child lists.

Comment 6

[Dave Camp (:dcamp)]

(In reply to Joe Walker [:jwalker] from comment #4)
> (In reply to Dave Camp (:dcamp) from comment #3)
> > (In reply to Joe Walker [:jwalker] from comment #2)
> > > Comment on attachment 756910 [details] [diff] [review]
> > > v1
> > > 
> > > Review of attachment 756910 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > I wonder if the confusion between helpers.js and helpers.js is too great.
> > > See also /browser/devtoools/commandline/test/helpers.js, which is used from
> > > anything that tests commands.
> > 
> > You know, this is the second time generic test helpers have lost a naming
> > fight with specific command line test helpers.  Maybe we should call those
> > command-helpers.js?
> > 
> > But I guess I can find something else this time around.
> 
> Yeah, although would the other 2 competitors have conflicted with each other?

I think it's reasonable for the name 'helpers.js' to be used as a this-directory-only helper filename.  What's unreasonable here is letting a shared resource take up that name all over the tree.

But am renaming this to inspector-helpers.js.

Comment 7

[Dave Camp (:dcamp)]

(In reply to Dave Camp (:dcamp) from comment #3)
> 
> > @@ +52,5 @@
> > > +// XXX: A poor man's makeInfallible until we move it out of transport.js
> > > +// Which should be very soon.
> > > +function makeInfallible(handler) {
> > 
> > I wonder if logExceptionWrapper would be clearer? We're not stopping
> > anything from falling are we.
> 
> Yeah possibly - this function already exists (see bug 878319) but we can
> followup to change its name?

Filed bug 880380

> > this would give us "[NodeActor for [object HTMLBodyElement]]"
> > I guess it's only debug. Just wondering if it's what you meant.
> 
> That is what I want, but [NodeActor conn0.domnode01 for [object
> HTMLBodyElement]] might be preferable...

I made that change.

> > @@ +208,5 @@
> > > +    this._classList = this.className.split(/\s+/);
> > > +    this._classList.contains = function(cls) {
> > > +      return this.indexOf(cls) > 0;
> > > +    }.bind(this._classList);
> > > +    return this._classList;
> > 
> > The classList API is add/remove/contains/toggle ?
> > http://davidwalsh.name/classlist
> 
> Yeah, but add/remove/toggle all mutate synchronously, which we can't do. 
> Will be adding changes in a followup patch.

Y'know, we neither use nor test this API yet, so I just got rid of it for now, we can revisit in a followup.

> > I wonder if we should call this truncatedValue or something?
> 
> Yeah, maybe truncatedValue (it's not always truncated, but...)

I chose 'shortValue'.

Comment 8

[Dave Camp (:dcamp)]

Attachment: v2

Changes made as discussed above.

Comment 9

[Dave Camp (:dcamp)]

https://hg.mozilla.org/integration/fx-team/rev/a61daa400fe8

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

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