952277 - Highlight and jump to nodes from the Debugger

Status: VERIFIED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Benoit Girard (:BenWa)]

+++ This bug was initially created as a clone of Bug #757866 +++
Similarly to bug 757866. I was debugging something like this:
window.getComputedStyle(divNode);

and I was getting the wrong value. It was very difficult to find this node in the inspector. It would be really nice if the debugger could switch tabs to the inspector and select that node.

Comment 1

[Patrick Brosset <:pbro>]

Same as in bug 757866, it makes sense to make this depend on bug 916443.
Indeed, highlighting and selecting a node from another tool will be a lot simpler once this patch lands.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Bug 918715 is about the same thing, but for the events pane. I expect this bug to take care of the variables view.

Comment 3

[Patrick Brosset <:pbro>]

You're right, if we manage to make it work from the VariablesView widget, then we'll get DOM highlighting and selection for free in all the places it's used!

I will try to start something, but may need help.

My plan so far:
- listen to mouseover events on DOM objects property values
- ask the toolbox's walker actor for a NodeFront for the hovered property
- with it, ask the highlighter for highlight!

I will take a look at the code first and come back with questions if any.

Comment 4

[Patrick Brosset <:pbro>]

I've made some progress. See a quick demo here: http://www.youtube.com/watch?v=XXiD7WX5aOI
I'll need to ask for review to someone from the debugger team because I'm not very comfortable with what I needed to do on the actor side.
Also, for some reasons, it only works for first level variables. If you, say, expand document.body and look at the childNodes property, each of the children below isn't going to be highlighter on hover.
This won't be very hard to fix though, I think it's just a problem with how the VariablesView instantiate sub trees.

Comment 5

[Benoit Girard (:BenWa)]

Nice, love it!

Comment 6

[Patrick Brosset <:pbro>]

Attachment: bug952277-highlight-nodes-debugger v1.patch

Here is a first patch for this bug.
Its job is to link DOM Nodes in the VariablesView widget to the inspector by doing 2 things:
- highlight on mouseover
- select node and switch to inspector on click.

I'd like to ask for review especially for the part in the WalkerActor.
Indeed, the VariablesView widget knows about valueGrip objects. These are sent by the server and correspond to Debugger.Object, wrapping actual objects part of the debuggee.
However, the inspector only deals with NodeActors, and these 2 things, although both representing objects in the debuggee, are not the same.
So, my idea was to implement a new method on the WalkerActor that would simply translate a valueGrip into a NodeActor.
And it does this by asking the connection for the ObjectActor represented by the ID in the grip, and from there, finds the Debugger.Object and executes its unsafeDereference() function to get the rawObject out of it.

Here is a list of things that were done in this patch:
- Moved the highlight/unhighlight node logic from markup-view to toolbox, so it can be used from anywhere
- Changed the variablesView so it can accept a "toolbox" property which, if set, is used to highlighter/select DOMNodes.
- Used this property in the webconsole, debugger, and debuggerTooltip's variablesviews.
- DOMNode properties in vview: mouseenter/leave shows/hide the highlighter
- The tooltip has been changed too
- DOMNode properties in vview: click doesn't edit the value anymore, but just select and switch to inspector.
- DOMNodes properties in vview: different CSS class applies now

In terms of tests, I fixed several that were impacted by a refactor of toolbox.js, and I added one in the webconsole to test this new feature.
I haven't added any for the debugger yet though (I'll probably need some help for that one).

Comment 7

[Patrick Brosset <:pbro>]

Attachment: bug952277-highlight-nodes-debugger v2.patch

Sorry, just correcting a debug comment I left in there and forgot about.
The patch is the same otherwise.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8361746 [details] [diff] [review]
bug952277-highlight-nodes-debugger v2.patch

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

This is sooo useful! I have a few suggestions for improvement, but my main concern is that variables or properties that are references to DOM nodes are no longer editable in the variables view. I think that is a regression that we need to fix. Maybe the right interaction would be to use double-click for viewing the node in the inspector, but I'm open to other ideas.

One bug I see is that hovering over a DOM node in variables view, then hovering over something else, and then hovering over the same DOM node again does not display the dashed border, only the infobar. Could be bug 961740. Something else that surprised me is that clicking on |document| doesn't select the <html> tag in the inspector, but I'm not sure if this is this an effect of this patch.

::: browser/devtools/debugger/debugger-panes.js
@@ +6,5 @@
>  
>  "use strict";
>  
> +Cu.import("resource:///modules/devtools/gDevTools.jsm");
> +

Nit: can you please move this to debugger-controller.js? It gets loaded first and we just put every dependency in it. Come to think of it, check out if it is actually needed first, because it may already be loaded by another dependency.

::: browser/devtools/debugger/debugger-view.js
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  "use strict";
>  
> +Cu.import("resource:///modules/devtools/gDevTools.jsm");
> +

This is redundant, all the code in the debugger frontend is loaded using script tags and share the same global.

::: browser/devtools/framework/toolbox.js
@@ +1180,5 @@
> +}
> +
> +ToolboxHighlighterUtils.prototype = {
> +  /**
> +   * Does the HighlighterActor exists on the server-side. This is used to know

Nit: "Indicates whether the highlighter actor exists on the server."

@@ +1333,5 @@
> +    } else {
> +      return promise.resolve();
> +    }
> +    // If not, no need to unhighlight as the older highlight method uses a
> +    // setTimeout to hide itself

Nit: moving the comment right before promise.resolve would make it clearer that it applies to the |else| branch.

::: browser/devtools/shared/widgets/Tooltip.js
@@ +490,5 @@
> +      // If a toolbox was provided, link it to the vview and make sure to hide
> +      // the tooltip if the toolbox selects another panel
> +      if (toolbox) {
> +        widget.toolbox = toolbox;
> +        toolbox.on("select", () => this.hide());

Wouldn't toolbox.once() be more appropriate in this case? Does the Tooltip remain alive after going to the inspector and back?

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +2132,5 @@
>   * @param object aDescriptor
>   *        The variable's descriptor.
>   */
>  function Variable(aScope, aName, aDescriptor) {
> +  this.scope = aScope;

Nit: if we're not exposing this, then make it private please.

@@ +2703,5 @@
> +
> +    // Listen to value mouseover/click events to highlight and jump
> +    this._valueLabel.addEventListener("mouseenter", this.highlightDomNode, false);
> +    this._valueLabel.addEventListener("mouseleave", this.unhighlightDomNode, false);
> +    this._valueLabel.addEventListener("mousedown", this.selectDomNodeAndLoadInspector, false);

I think these will cause the following warning:

[4413] WARNING: Please do not use mouseenter/leave events in chrome. They are slower than mouseover/out!: '!nsContentUtils::IsChromeDoc(d)', file /home/past/src/fx-team/dom/events/nsEventListenerManager.cpp, line 350

Can you use mouseover/out instead?

Also, are we removing these listeners anywhere? It doesn't look like we do.

@@ +2708,5 @@
> +
> +    this._linkedToInspector = true;
> +  },
> +
> +  selectDomNodeAndLoadInspector: function(event) {

Nit: openNodeInInspector seems shorter, but it's up to you.

@@ +2728,5 @@
> +
> +  highlightDomNode: function() {
> +    let toolbox = this._getToolbox();
> +    if (toolbox) {
> +      toolbox.highlighterUtils.highlightDomValueGrip(this._valueGrip);

Protocol requests are expensive for mobile debugging, so I think we should try to memoize the node front in the variables view for better reponsiveness. Since HighlighterUtils is a generic utility object, this doesn't look completely straightforward, but one idea is this:
- rename the method to highlightDomNode
- have it return the front object it retrieves from the grip
- make it accept either a grip or a front as a parameter and skip the conversion in the latter case

::: toolkit/devtools/server/actors/inspector.js
@@ +1929,5 @@
> +  /**
> +   * Given an ObjectActor value grip, commonly used in the debugger, webconsole
> +   * and variablesView, return the corresponding NodeActor
> +   */
> +  getNodeActorFromValueGrip: method(function(grip) {

You don't seem to need the entire grip, just the actor ID. Changing the method parameter to a string will cut down on unnecessary protocol chatter, which is more important for mobile devices.

With such a change, perhaps a more suitable name for the method would be getNodeActorFromObjectActor or even getNodeFromObject.

Comment 9

[Patrick Brosset <:pbro>]

Thanks Panos for the comprehensive review.
I'll address the points raised. Here are a few answers to some of them.

(In reply to Panos Astithas [:past] from comment #8)
> Comment on attachment 8361746 [details] [diff] [review]
> bug952277-highlight-nodes-debugger v2.patch
> 
> Review of attachment 8361746 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is sooo useful! I have a few suggestions for improvement, but my main
> concern is that variables or properties that are references to DOM nodes are
> no longer editable in the variables view. I think that is a regression that
> we need to fix. Maybe the right interaction would be to use double-click for
> viewing the node in the inspector, but I'm open to other ideas.
Yes, true, should be fixed. However, perhaps a "jump to inspector" icon would be better than having a click and a double-click event listeners that lead to very different things. 

> One bug I see is that hovering over a DOM node in variables view, then
> hovering over something else, and then hovering over the same DOM node again
> does not display the dashed border, only the infobar. Could be bug 961740.
This is bug 961740 indeed, which I fixed yesterday, so this problem will go away.

> Something else that surprised me is that clicking on |document| doesn't
> select the <html> tag in the inspector, but I'm not sure if this is this an
> effect of this patch.
Shouldn't |document.documentElement| select the <html> tag instead? I'll test if it does.

> ::: browser/devtools/debugger/debugger-panes.js
> @@ +6,5 @@
> >  
> >  "use strict";
> >  
> > +Cu.import("resource:///modules/devtools/gDevTools.jsm");
> > +
> 
> Nit: can you please move this to debugger-controller.js? It gets loaded
> first and we just put every dependency in it. Come to think of it, check out
> if it is actually needed first, because it may already be loaded by another
> dependency.
Ok, will check this.

> ::: browser/devtools/shared/widgets/Tooltip.js
> @@ +490,5 @@
> > +      // If a toolbox was provided, link it to the vview and make sure to hide
> > +      // the tooltip if the toolbox selects another panel
> > +      if (toolbox) {
> > +        widget.toolbox = toolbox;
> > +        toolbox.on("select", () => this.hide());
> 
> Wouldn't toolbox.once() be more appropriate in this case? Does the Tooltip
> remain alive after going to the inspector and back?
Well, this tooltip type has a "reuseCachedWidget" option which will return the same tooltip, so we do need the toolbox.on() here, but I should try and find a way to toolbox.off() when it's destroyed.

> @@ +2703,5 @@
> > +
> > +    // Listen to value mouseover/click events to highlight and jump
> > +    this._valueLabel.addEventListener("mouseenter", this.highlightDomNode, false);
> > +    this._valueLabel.addEventListener("mouseleave", this.unhighlightDomNode, false);
> > +    this._valueLabel.addEventListener("mousedown", this.selectDomNodeAndLoadInspector, false);
> 
> I think these will cause the following warning:
> 
> [4413] WARNING: Please do not use mouseenter/leave events in chrome. They
> are slower than mouseover/out!: '!nsContentUtils::IsChromeDoc(d)', file
> /home/past/src/fx-team/dom/events/nsEventListenerManager.cpp, line 350
> 
> Can you use mouseover/out instead?
> 
> Also, are we removing these listeners anywhere? It doesn't look like we do.
Ok, I didn't know, I will change to mouseover/out and make sure they are removed at some stage.

> @@ +2728,5 @@
> > +
> > +  highlightDomNode: function() {
> > +    let toolbox = this._getToolbox();
> > +    if (toolbox) {
> > +      toolbox.highlighterUtils.highlightDomValueGrip(this._valueGrip);
> 
> Protocol requests are expensive for mobile debugging, so I think we should
> try to memoize the node front in the variables view for better
> reponsiveness. Since HighlighterUtils is a generic utility object, this
> doesn't look completely straightforward, but one idea is this:
> - rename the method to highlightDomNode
> - have it return the front object it retrieves from the grip
> - make it accept either a grip or a front as a parameter and skip the
> conversion in the latter case
Good idea.

> ::: toolkit/devtools/server/actors/inspector.js
> @@ +1929,5 @@
> > +  /**
> > +   * Given an ObjectActor value grip, commonly used in the debugger, webconsole
> > +   * and variablesView, return the corresponding NodeActor
> > +   */
> > +  getNodeActorFromValueGrip: method(function(grip) {
> 
> You don't seem to need the entire grip, just the actor ID. Changing the
> method parameter to a string will cut down on unnecessary protocol chatter,
> which is more important for mobile devices.
> 
> With such a change, perhaps a more suitable name for the method would be
> getNodeActorFromObjectActor or even getNodeFromObject.
True indeed, only the actorID is required here, I'll change this.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Patrick Brosset [:pbrosset] from comment #9)
> Yes, true, should be fixed. However, perhaps a "jump to inspector" icon
> would be better than having a click and a double-click event listeners that
> lead to very different things. 

Indeed, that seems more discoverable and there is also the precedent of the "delete" icon in watch expressions that appears on hover (to cut down on visual clutter). Maybe a shrunk inspector panel icon?

> Shouldn't |document.documentElement| select the <html> tag instead? I'll
> test if it does.

Hmm, you have a point there. I don't know, maybe |document| deserves to be weird in the same way as |window| in that regard.

Comment 11

[Patrick Brosset <:pbro>]

Attachment: bug952277-highlight-nodes-debugger v3.patch

Here is an update.
See my comments inline below:

> This is sooo useful! I have a few suggestions for improvement, but my main
> concern is that variables or properties that are references to DOM nodes are
> no longer editable in the variables view. I think that is a regression that
> we need to fix. Maybe the right interaction would be to use double-click for
> viewing the node in the inspector, but I'm open to other ideas.
I added a new icon that appears on hover of the line (just like the delete icon).
The icon isn't nice, so I'll needinfo Darrin for something better, but I think it does the job. It's discoverable, it has a tooltip, and it doesn't break the edition feature.

> One bug I see is that hovering over a DOM node in variables view, then
> hovering over something else, and then hovering over the same DOM node again
> does not display the dashed border, only the infobar. Could be bug 961740.
Yep, bug 961740. Fixed.

> Something else that surprised me is that clicking on |document| doesn't
> select the <html> tag in the inspector, but I'm not sure if this is this an
> effect of this patch.
I agree, we might want to treat document as documentElement, so I added a couple of lines in walker.getNodeActorFromObjectActor to do just that.

> ::: browser/devtools/debugger/debugger-panes.js
> @@ +6,5 @@
> >  
> >  "use strict";
> >  
> > +Cu.import("resource:///modules/devtools/gDevTools.jsm");
> > +
> 
> Nit: can you please move this to debugger-controller.js? It gets loaded
> first and we just put every dependency in it. Come to think of it, check out
> if it is actually needed first, because it may already be loaded by another
> dependency.
I removed it altogether and it worked.

> ::: browser/devtools/debugger/debugger-view.js
> @@ +5,5 @@
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  "use strict";
> >  
> > +Cu.import("resource:///modules/devtools/gDevTools.jsm");
> > +
> 
> This is redundant, all the code in the debugger frontend is loaded using
> script tags and share the same global.
Done.

> ::: browser/devtools/framework/toolbox.js
> @@ +1180,5 @@
> > +}
> > +
> > +ToolboxHighlighterUtils.prototype = {
> > +  /**
> > +   * Does the HighlighterActor exists on the server-side. This is used to know
> 
> Nit: "Indicates whether the highlighter actor exists on the server."
Better wording. Done.

> @@ +1333,5 @@
> > +    } else {
> > +      return promise.resolve();
> > +    }
> > +    // If not, no need to unhighlight as the older highlight method uses a
> > +    // setTimeout to hide itself
> 
> Nit: moving the comment right before promise.resolve would make it clearer
> that it applies to the |else| branch.
Indeed, that's more logical. Done.

> ::: browser/devtools/shared/widgets/Tooltip.js
> @@ +490,5 @@
> > +      // If a toolbox was provided, link it to the vview and make sure to hide
> > +      // the tooltip if the toolbox selects another panel
> > +      if (toolbox) {
> > +        widget.toolbox = toolbox;
> > +        toolbox.on("select", () => this.hide());
> 
> Wouldn't toolbox.once() be more appropriate in this case? Does the Tooltip
> remain alive after going to the inspector and back?
As I said earlier, we can't do once() here, so what I did is I extracted this tooltip hiding logic at a more global level.
Now, tooltips can be configured to close on any event from any observable object. So it's a better feature, and it allows to cleanly get rid of the event on destroy.

> ::: browser/devtools/shared/widgets/VariablesView.jsm
> @@ +2132,5 @@
> >   * @param object aDescriptor
> >   *        The variable's descriptor.
> >   */
> >  function Variable(aScope, aName, aDescriptor) {
> > +  this.scope = aScope;
> 
> Nit: if we're not exposing this, then make it private please.
Done.

> @@ +2703,5 @@
> > +
> > +    // Listen to value mouseover/click events to highlight and jump
> > +    this._valueLabel.addEventListener("mouseenter", this.highlightDomNode, false);
> > +    this._valueLabel.addEventListener("mouseleave", this.unhighlightDomNode, false);
> > +    this._valueLabel.addEventListener("mousedown", this.selectDomNodeAndLoadInspector, false);
> 
> I think these will cause the following warning:
> 
> [4413] WARNING: Please do not use mouseenter/leave events in chrome. They
> are slower than mouseover/out!: '!nsContentUtils::IsChromeDoc(d)', file
> /home/past/src/fx-team/dom/events/nsEventListenerManager.cpp, line 350
> 
> Can you use mouseover/out instead?
> 
> Also, are we removing these listeners anywhere? It doesn't look like we do.
Done. For both things.

> @@ +2708,5 @@
> > +
> > +    this._linkedToInspector = true;
> > +  },
> > +
> > +  selectDomNodeAndLoadInspector: function(event) {
> 
> Nit: openNodeInInspector seems shorter, but it's up to you.
Agreed. Done.

> @@ +2728,5 @@
> > +
> > +  highlightDomNode: function() {
> > +    let toolbox = this._getToolbox();
> > +    if (toolbox) {
> > +      toolbox.highlighterUtils.highlightDomValueGrip(this._valueGrip);
> 
> Protocol requests are expensive for mobile debugging, so I think we should
> try to memoize the node front in the variables view for better
> reponsiveness. Since HighlighterUtils is a generic utility object, this
> doesn't look completely straightforward, but one idea is this:
> - rename the method to highlightDomNode
> - have it return the front object it retrieves from the grip
> - make it accept either a grip or a front as a parameter and skip the
> conversion in the latter case
Good point. What I ended up doing was simply storing the nodeFront at Variable level, so that next time, if it's already here, we just reuse it and save one round trip to resolve it from the grip.

> ::: toolkit/devtools/server/actors/inspector.js
> @@ +1929,5 @@
> > +  /**
> > +   * Given an ObjectActor value grip, commonly used in the debugger, webconsole
> > +   * and variablesView, return the corresponding NodeActor
> > +   */
> > +  getNodeActorFromValueGrip: method(function(grip) {
> 
> You don't seem to need the entire grip, just the actor ID. Changing the
> method parameter to a string will cut down on unnecessary protocol chatter,
> which is more important for mobile devices.
> 
> With such a change, perhaps a more suitable name for the method would be
> getNodeActorFromObjectActor or even getNodeFromObject.
Done. Now only sending the actorID string.

Comment 12

[Patrick Brosset <:pbro>]

Attachment: open-node-in-inspector.mov

Darrin, here's a quick video showing the new "open in inspector" icon in a variablesview widget.
So far it's only a resized down version of the inspector icon in the toolbox tab, so it's not great, and it doesn't have a :hover or :active state yet.

Do you think you could provide an icon that fits better with the other ones we have in this widget (delete and edit)?

Comment 13

[Girish Sharma [:Optimizer]]

My two cents on not introducing a new icon for the feature similar to inspecting DOM nodes. Why not use the Inspector button (the one along the other command buttons) ?

Comment 14

[Patrick Brosset <:pbro>]

(In reply to Girish Sharma [:Optimizer] from comment #13)
> My two cents on not introducing a new icon for the feature similar to
> inspecting DOM nodes. Why not use the Inspector button (the one along the
> other command buttons) ?

Good idea, it's even more discoverable I think.

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Girish Sharma [:Optimizer] from comment #13)
> My two cents on not introducing a new icon for the feature similar to
> inspecting DOM nodes. Why not use the Inspector button (the one along the
> other command buttons) ?

That was my first thought as well, but this is actually the highlighter button and hovering over the value in the variables view already activates the highlighter. I can imagine some people being confused about its purpose.

Comment 16

[Girish Sharma [:Optimizer]]

(In reply to Panos Astithas [:past] from comment #15)
> (In reply to Girish Sharma [:Optimizer] from comment #13)
> > My two cents on not introducing a new icon for the feature similar to
> > inspecting DOM nodes. Why not use the Inspector button (the one along the
> > other command buttons) ?
> 
> That was my first thought as well, but this is actually the highlighter
> button and hovering over the value in the variables view already activates
> the highlighter. I can imagine some people being confused about its purpose.

Hmm, that is also true.

Darrin gets the final call :)

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8363082 [details] [diff] [review]
bug952277-highlight-nodes-debugger v3.patch

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

I discovered one bug: hovering over a DOM node in variables view and then editing the value to something else (e.g. {}) will leave the highlighter on. r=me with that fixed.

::: browser/devtools/framework/toolbox.js
@@ +1237,5 @@
> +      } else {
> +        this.toolbox.walker.pick().then(node => {
> +          this.toolbox.selection.setNodeFront(node, "picker-node-picked");
> +          this.stopPicker();
> +        });

Define _onPickerNodePicked out of the |if| branch and reuse it here? Actually, why not define both it and _onPickerNodeHovered directly on the prototype instead of redefiinng them each time startPicker is called?

::: browser/devtools/shared/widgets/Tooltip.js
@@ +146,3 @@
>   *        - noAutoFocus {Boolean} Should the focus automatically go to the panel
> + *        when it opens. Defaults to true.
> + *        - closeOnEvent {object: {Object}, event: {String}} An observable

Wouldn't calling the property "emitter" clarify both its functionality and the fact that it's not a DOM event, compared to a generic "object" name? Might want to elaborate a bit on that in this comment, too.

::: browser/locales/en-US/chrome/browser/devtools/debugger.properties
@@ +262,5 @@
>  # in the variables list on a getter or setter which can be edited.
>  variablesEditButtonTooltip=Click to set value
>  
> +# LOCALIZATION NOTE (variablesEditableValueTooltip): The text that is displayed
> +# in the variables list on an DOMNode item.

Nit: this note needs updating as the tooltip appears on the icon now.

::: toolkit/devtools/server/actors/inspector.js
@@ +1926,5 @@
>      this.releaseNode(documentActor, { force: true });
> +  },
> +
> +  /**
> +   * Given an ObjectActor value grip (identified by its ID), commonly used in

Stale comment: s/value grip//

Comment 18

[Patrick Brosset <:pbro>]

Thanks Panos!

(In reply to Panos Astithas [:past] from comment #17)
> Comment on attachment 8363082 [details] [diff] [review]
> bug952277-highlight-nodes-debugger v3.patch
> 
> Review of attachment 8363082 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I discovered one bug: hovering over a DOM node in variables view and then
> editing the value to something else (e.g. {}) will leave the highlighter on.
> r=me with that fixed.
Bug fixed! Thanks for catching that. I'm now stoping the highlighting when the variable value is edited.

> ::: browser/devtools/framework/toolbox.js
> @@ +1237,5 @@
> > +      } else {
> > +        this.toolbox.walker.pick().then(node => {
> > +          this.toolbox.selection.setNodeFront(node, "picker-node-picked");
> > +          this.stopPicker();
> > +        });
> 
> Define _onPickerNodePicked out of the |if| branch and reuse it here?
> Actually, why not define both it and _onPickerNodeHovered directly on the
> prototype instead of redefiinng them each time startPicker is called?
You're right, it's better. Done.

> ::: browser/devtools/shared/widgets/Tooltip.js
> @@ +146,3 @@
> >   *        - noAutoFocus {Boolean} Should the focus automatically go to the panel
> > + *        when it opens. Defaults to true.
> > + *        - closeOnEvent {object: {Object}, event: {String}} An observable
> 
> Wouldn't calling the property "emitter" clarify both its functionality and
> the fact that it's not a DOM event, compared to a generic "object" name?
> Might want to elaborate a bit on that in this comment, too.
I agree, emitter is better, I've added some more comments for it too.

> ::: browser/locales/en-US/chrome/browser/devtools/debugger.properties
> @@ +262,5 @@
> >  # in the variables list on a getter or setter which can be edited.
> >  variablesEditButtonTooltip=Click to set value
> >  
> > +# LOCALIZATION NOTE (variablesEditableValueTooltip): The text that is displayed
> > +# in the variables list on an DOMNode item.
> 
> Nit: this note needs updating as the tooltip appears on the icon now.
Done.

> ::: toolkit/devtools/server/actors/inspector.js
> @@ +1926,5 @@
> >      this.releaseNode(documentActor, { force: true });
> > +  },
> > +
> > +  /**
> > +   * Given an ObjectActor value grip (identified by its ID), commonly used in
> 
> Stale comment: s/value grip//
Done.

Comment 19

[Patrick Brosset <:pbro>]

Attachment: bug952277-highlight-nodes-debugger v4.patch

v4.
Contains fixes as per Panos' comments (as described in my Comment 18)
Applying R+.

Comment 20

[Patrick Brosset <:pbro>]

Attachment: bug952277-debugger-test.patch

The main patch already contains a test for the webconsole, this one contains a new test for the debugger that checks the presence of DOMNodes in the variable bubble and checks the highlight on hover and select on click.

Victor, since you spent time cleaning up the debugger tests, getting a review from you would be appropriate.

Comment 21

[Patrick Brosset <:pbro>]

Attachment: bug952277-debugger-test v2.patch

Quick change, I forgot to remove an unused function from the test.

Comment 22

[Victor Porof [:vporof][:vp]]

Comment on attachment 8365089 [details] [diff] [review]
bug952277-debugger-test v2.patch

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

::: browser/devtools/debugger/test/browser.ini
@@ +64,5 @@
>    doc_step-out.html
>    doc_tracing-01.html
>    doc_watch-expressions.html
>    doc_with-frame.html
> +  doc_domnode-variables.html

Please keep this list sorted alphabetically.

::: browser/devtools/debugger/test/browser_dbg_variables-view-popup-10.js
@@ +38,5 @@
> +
> +    // Simulate mouseover on the property value
> +    EventUtils.sendMouseEvent({ type: "mouseover" }, property,
> +      property.ownerDocument.defaultView);
> +    yield once(toolbox, "node-highlight");

Does this always happen asynchronously? If not, you should do this the other way around:
> let highlighted = once(toolbox, "node-highlight");
> EventUtils.sendMouseEvent(...);
> yield highlighted;
to avoid oranges.

@@ +46,5 @@
> +    let button = property.parentNode.querySelector(".variables-view-open-inspector");
> +    ok(button, "The select-in-inspector button is present");
> +    EventUtils.sendMouseEvent({ type: "mousedown" }, button,
> +      button.ownerDocument.defaultView);
> +    yield once(toolbox, "inspector-selected");

Ditto.

@@ +49,5 @@
> +      button.ownerDocument.defaultView);
> +    yield once(toolbox, "inspector-selected");
> +    ok(true, "The inspector got selected when clicked on the select-in-inspector");
> +
> +    yield once(toolbox.getPanel("inspector"), "inspector-updated");

Ditto on events getting fired before being started listened for.

::: browser/devtools/debugger/test/doc_domnode-variables.html
@@ +8,5 @@
> +    <title>Debugger test page</title>
> +  </head>
> +
> +  <body>
> +    <div>Look at this DIV! Just look at it!</div>

Can't stop looking now .__.

Comment 23

[Victor Porof [:vporof][:vp]]

Comment on attachment 8365084 [details] [diff] [review]
bug952277-highlight-nodes-debugger v4.patch

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

I have a few questions :) Sorry for the drive-by.

::: browser/devtools/debugger/debugger-panes.js
@@ +1744,5 @@
> +    this._tooltip = new Tooltip(document, {
> +      closeOnEvent: {
> +        emitter: gDevTools.getToolbox(DebuggerController._target),
> +        event: "select"
> +      }

This is sexy. However, we're now having two separate mechanisms of closing this tooltip (see also _onMouseScroll and hideContents). Might prove to be hard to maintain in the long run.

Please choose one way and stick to it. Either remove closeOnEvent and add the event listener manually, or make the scroll event also a thing that closes the tooltip.

@@ +1898,5 @@
>            if (aType == "properties") {
>              window.emit(EVENTS.FETCHED_BUBBLE_PROPERTIES);
>            }
>          }
> +      }, gDevTools.getToolbox(DebuggerController._target));

Is gDevTools available in the debugger frontend? Had no idea. Is it also available in the Browser Debugger? 

DebuggerController._target isn't available in that case iirc, in which case the toolbox won't be defined. That might have changed since we started having the Browser Toolbox.

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +2133,5 @@
>   * @param object aDescriptor
>   *        The variable's descriptor.
>   */
>  function Variable(aScope, aName, aDescriptor) {
> +  this._scope = aScope;

Wouldn't this.ownerView work just as well? Why assign aScope to a this._scope too?

@@ +2660,5 @@
>      }
> +    if (this._openInspectorNode && this._linkedToInspector) {
> +      this._openInspectorNode.setAttribute("tooltiptext", this.ownerView.domNodeValueTooltip);
> +    }
> +    else if (this._valueLabel && ownerView.eval) {

If I'm reading this correctly, this means that you can't change the value of properties that happen to have a DOM node as a value. Why? All editable properties should be editable IMHO, no need to disallow in some cases. If this is a UX issue, we should do something about that instead of crippling some functionality.

@@ +2685,5 @@
> +      view = view.ownerView;
> +      toolbox = view.toolbox;
> +    }
> +
> +    return toolbox;

I think you could simply return this._variablesView.toolbox;

@@ +2688,5 @@
> +
> +    return toolbox;
> +  },
> +
> +  _isLinkableToInspector: function() {

Nit: please document this method.

@@ +2718,5 @@
> +
> +    this._linkedToInspector = true;
> +  },
> +
> +  openNodeInInspector: function(event) {

Nit: please document this, at least briefly :)

@@ +2730,5 @@
> +
> +        getNodeFront.then(nodeFront => {
> +          if (nodeFront) {
> +            let toolbox = this._getToolbox();
> +            toolbox.selectTool("inspector").then(() => {

May I suggest using Task.jsm to avoid such deep nesting with promises?

Task.spawn(function*() {
  ...
  yield asyncThing1();
  let resolvedValue = yield asyncThing2();
  yield asyncThing3();

}.bind(this));

@@ +2739,5 @@
> +      });
> +    }
> +  },
> +
> +  highlightDomNode: function() {

Nit: please document this.

@@ +2755,5 @@
> +      });
> +    }
> +  },
> +
> +  unhighlightDomNode: function() {

Nit: please document this.

Comment 24

[Patrick Brosset <:pbro>]

Attachment: bug952277-debugger-test v3.patch

Thanks Victor for the review. See my comments below:

(In reply to Victor Porof [:vp] from comment #22)
> Comment on attachment 8365089 [details] [diff] [review]
> bug952277-debugger-test v2.patch
> 
> Review of attachment 8365089 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/debugger/test/browser.ini
> @@ +64,5 @@
> >    doc_step-out.html
> >    doc_tracing-01.html
> >    doc_watch-expressions.html
> >    doc_with-frame.html
> > +  doc_domnode-variables.html
> 
> Please keep this list sorted alphabetically.
Done.

> ::: browser/devtools/debugger/test/browser_dbg_variables-view-popup-10.js
> @@ +38,5 @@
> > +
> > +    // Simulate mouseover on the property value
> > +    EventUtils.sendMouseEvent({ type: "mouseover" }, property,
> > +      property.ownerDocument.defaultView);
> > +    yield once(toolbox, "node-highlight");
> 
> Does this always happen asynchronously? If not, you should do this the other
> way around:
> > let highlighted = once(toolbox, "node-highlight");
> > EventUtils.sendMouseEvent(...);
> > yield highlighted;
> to avoid oranges.
You're right, I've moved this around.
Although this is always going to be asynchronous anyway.

> @@ +46,5 @@
> > +    let button = property.parentNode.querySelector(".variables-view-open-inspector");
> > +    ok(button, "The select-in-inspector button is present");
> > +    EventUtils.sendMouseEvent({ type: "mousedown" }, button,
> > +      button.ownerDocument.defaultView);
> > +    yield once(toolbox, "inspector-selected");
> 
> Ditto.
Done here too.

> @@ +49,5 @@
> > +      button.ownerDocument.defaultView);
> > +    yield once(toolbox, "inspector-selected");
> > +    ok(true, "The inspector got selected when clicked on the select-in-inspector");
> > +
> > +    yield once(toolbox.getPanel("inspector"), "inspector-updated");
> 
> Ditto on events getting fired before being started listened for.
Here though, I can't really do the same change as toolbox.getPanel("inspector") will return null before |yield once(toolbox, "inspector-selected");| is done.
So I can't start listening before.
Also, when the inspector is switched to the first time, it is loaded asynchronously, so this should never fail really.

Comment 25

[Victor Porof [:vporof][:vp]]

Comment on attachment 8367217 [details] [diff] [review]
bug952277-debugger-test v3.patch

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

Sure, just wanted to make sure you've considered stuff. Thanks for updating.

Comment 26

[Patrick Brosset <:pbro>]

Attachment: bug952277-highlight-nodes-debugger v5.patch

Thanks for the awesome feedback Victor. See my answers below.
(In reply to Victor Porof [:vp] from comment #23)
> Comment on attachment 8365084 [details] [diff] [review]
> bug952277-highlight-nodes-debugger v4.patch
> 
> Review of attachment 8365084 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have a few questions :) Sorry for the drive-by.
> 
> ::: browser/devtools/debugger/debugger-panes.js
> @@ +1744,5 @@
> > +    this._tooltip = new Tooltip(document, {
> > +      closeOnEvent: {
> > +        emitter: gDevTools.getToolbox(DebuggerController._target),
> > +        event: "select"
> > +      }
> 
> This is sexy. However, we're now having two separate mechanisms of closing
> this tooltip (see also _onMouseScroll and hideContents). Might prove to be
> hard to maintain in the long run.
> 
> Please choose one way and stick to it. Either remove closeOnEvent and add
> the event listener manually, or make the scroll event also a thing that
> closes the tooltip.
You're right, so what I've done is I changed closeOnEvent to be closeOnEvents instead. It is now an array of {emmiter, event} and emitters can either implement on/off or addEventListener/removeEventListener. This way I was able to move the _onMouseScroll logic to closeOnEvents

> @@ +1898,5 @@
> >            if (aType == "properties") {
> >              window.emit(EVENTS.FETCHED_BUBBLE_PROPERTIES);
> >            }
> >          }
> > +      }, gDevTools.getToolbox(DebuggerController._target));
> 
> Is gDevTools available in the debugger frontend? Had no idea. Is it also
> available in the Browser Debugger? 
> 
> DebuggerController._target isn't available in that case iirc, in which case
> the toolbox won't be defined. That might have changed since we started
> having the Browser Toolbox.
It is available indeed, but you're right, target doesn't seem to be available. So I've changed that part.
Since the debugger panel has a reference to the toolbox, it now just exposes it on the DebuggerController object so that I can get it where I need it.

> ::: browser/devtools/shared/widgets/VariablesView.jsm
> @@ +2133,5 @@
> >   * @param object aDescriptor
> >   *        The variable's descriptor.
> >   */
> >  function Variable(aScope, aName, aDescriptor) {
> > +  this._scope = aScope;
> 
> Wouldn't this.ownerView work just as well? Why assign aScope to a
> this._scope too?
Right, that works and is simpler. Thanks!

> @@ +2660,5 @@
> >      }
> > +    if (this._openInspectorNode && this._linkedToInspector) {
> > +      this._openInspectorNode.setAttribute("tooltiptext", this.ownerView.domNodeValueTooltip);
> > +    }
> > +    else if (this._valueLabel && ownerView.eval) {
> 
> If I'm reading this correctly, this means that you can't change the value of
> properties that happen to have a DOM node as a value. Why? All editable
> properties should be editable IMHO, no need to disallow in some cases. If
> this is a UX issue, we should do something about that instead of crippling
> some functionality.
No, editing properties still work, even if they're DOMNodes.
There was an extra else in here that shouldn't have been here. So only the tooltip was wrong. 
I've fixed it.

> @@ +2685,5 @@
> > +      view = view.ownerView;
> > +      toolbox = view.toolbox;
> > +    }
> > +
> > +    return toolbox;
> 
> I think you could simply return this._variablesView.toolbox;
Good catch, I didn't realize we had a ref to the parent variablesView.
What I've done now is that toolbox is a getter now so it simplifies a lot the places where I needed to check for it.

> @@ +2688,5 @@
> > +
> > +    return toolbox;
> > +  },
> > +
> > +  _isLinkableToInspector: function() {
> 
> Nit: please document this method.
Done

> @@ +2718,5 @@
> > +
> > +    this._linkedToInspector = true;
> > +  },
> > +
> > +  openNodeInInspector: function(event) {
> 
> Nit: please document this, at least briefly :)
Done

> @@ +2730,5 @@
> > +
> > +        getNodeFront.then(nodeFront => {
> > +          if (nodeFront) {
> > +            let toolbox = this._getToolbox();
> > +            toolbox.selectTool("inspector").then(() => {
> 
> May I suggest using Task.jsm to avoid such deep nesting with promises?
> 
> Task.spawn(function*() {
>   ...
>   yield asyncThing1();
>   let resolvedValue = yield asyncThing2();
>   yield asyncThing3();
> 
> }.bind(this));
Done. Looks better indeed. I will try and make a habit of using this for nested promises. Much better than going back to callback hell mode.
Also, I've added yield statements in there that make sure the inspector is fully loaded and operational, and the parent function also return the promise returned by Task.spawn, so that consumers can use it too.

> @@ +2739,5 @@
> > +      });
> > +    }
> > +  },
> > +
> > +  highlightDomNode: function() {
> 
> Nit: please document this.
Done

> @@ +2755,5 @@
> > +      });
> > +    }
> > +  },
> > +
> > +  unhighlightDomNode: function() {
> 
> Nit: please document this.
Done

Care for a last review?

Comment 27

[Patrick Brosset <:pbro>]

Also, first try build: https://tbpl.mozilla.org/?tree=Try&rev=af31a9194d01

Comment 28

[Victor Porof [:vporof][:vp]]

Comment on attachment 8367356 [details] [diff] [review]
bug952277-highlight-nodes-debugger v5.patch

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

Sweet! One small visual remains, the inspect button is basically invisible in the light theme, but I guess that will be addressed in bug 957117?

::: browser/devtools/shared/widgets/Tooltip.js
@@ +506,5 @@
>      objectActor,
>      viewOptions = {},
>      controllerOptions = {},
>      relayEvents = {},
> +    toolbox,

I think you should provide a null default value for the toolbox param. I didn't even know this is valid js :)

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +2731,5 @@
> +        if (!nodeFront) {
> +          nodeFront = yield this.toolbox.walker.getNodeActorFromObjectActor(this._valueGrip.actor);
> +        }
> +
> +

Nit: there are two newlines here.

@@ +2741,5 @@
> +          yield this.toolbox.selection.setNodeFront(nodeFront, "variables-view");
> +          yield inspectorReady.promise;
> +        }
> +      }.bind(this));
> +    }

How about returning a rejected promise when there's no toolbox available? You could do this at the top of this method, to even avoid an extra level of indentation! Value!

if (!this.toolbox) {
  return promise.reject(new Error("Toolbox not available"));
}

return Task.spawn(...)

Comment 29

[Patrick Brosset <:pbro>]

Attachment: bug952277-highlight-nodes-debugger v6.patch

Thanks Victor for the review.
(In reply to Victor Porof [:vp] from comment #28)
> Comment on attachment 8367356 [details] [diff] [review]
> bug952277-highlight-nodes-debugger v5.patch
> 
> Review of attachment 8367356 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sweet! One small visual remains, the inspect button is basically invisible
> in the light theme, but I guess that will be addressed in bug 957117?
> 
> ::: browser/devtools/shared/widgets/Tooltip.js
> @@ +506,5 @@
> >      objectActor,
> >      viewOptions = {},
> >      controllerOptions = {},
> >      relayEvents = {},
> > +    toolbox,
> 
> I think you should provide a null default value for the toolbox param. I
> didn't even know this is valid js :)
Done.
> ::: browser/devtools/shared/widgets/VariablesView.jsm
> @@ +2731,5 @@
> > +        if (!nodeFront) {
> > +          nodeFront = yield this.toolbox.walker.getNodeActorFromObjectActor(this._valueGrip.actor);
> > +        }
> > +
> > +
> 
> Nit: there are two newlines here.
Done.
> @@ +2741,5 @@
> > +          yield this.toolbox.selection.setNodeFront(nodeFront, "variables-view");
> > +          yield inspectorReady.promise;
> > +        }
> > +      }.bind(this));
> > +    }
> 
> How about returning a rejected promise when there's no toolbox available?
> You could do this at the top of this method, to even avoid an extra level of
> indentation! Value!
> 
> if (!this.toolbox) {
>   return promise.reject(new Error("Toolbox not available"));
> }
> 
> return Task.spawn(...)
Done!

Applying R+ since these are minor changes.

New ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=40a0fac13dfb

Comment 30

[Patrick Brosset <:pbro>]

Attachment: bug952277-debugger-test v4.patch

Simple rebase (someone has added a new test file that has the same name as the one I created).

Comment 31

[Patrick Brosset <:pbro>]

The new try looks pretty green so far so I'm almost ready to land the patches :)
Last remaining detail is the new inspector icon.
I've been talking to Darrin who provided me all new images for the inspector, delete, edit, lock icons!
I'm getting them in right now. Here's how it looks so far: https://dl.dropboxusercontent.com/u/714210/mozilla/all-states.png (I need to rework a bit the positioning of the edit icon which seems too close to the label).

Comment 32

[Patrick Brosset <:pbro>]

Attachment: all-states.png

Darrin, here are screenshots of all the new icons.

Comment 33

[Patrick Brosset <:pbro>]

Attachment: bug952277-highlight-nodes-debugger v7.patch

No code change. Only new icons provided by Darrin + corresponding CSS changes.

Brian, could you please take a quick look and let me know if the icons are at the right place, and referenced from the right theme css files. 
Note that there's only 1 version of each, since they work both on light and dark themes thanks to the way they've been designed.

Also, Darrin provided the 2x versions but I haven't put them in yet. Wondering how to go about doing this.

I'd rather do icon changes (if needed) in a separate bug I think.

Comment 34

[Brian Grinstead [:bgrins]]

Comment on attachment 8368547 [details] [diff] [review]
bug952277-highlight-nodes-debugger v7.patch

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

The only thing I'd say is that we should be moving these 4 icons into the ../shared/devtools/images folder instead of each OS folder (and updating the references in the jar.mn).  I wouldn't block this bug from landing to do that, but it would be nice to do since the existing 2 icons are changing in this case anyway.  Don't worry about @2x to land this feature - we can handle that with Bug 837188.

Comment 35

[Darrin Henein [:darrin]]

Attachment: icons v2

Comment 36

[Patrick Brosset <:pbro>]

Attachment: bug952277-highlight-nodes-debugger v8.patch

Icons v2 are in!
They look much nicer.
I've also taken Brian's feedback into consideration here, so:
- images are in the shared folder
- jar.mn files are updated
Also:
- when a line in the vview is selected, it turns blue-ish, which makes the hover state of the new icon not visible enough, this is now fixed by preserving the normal state on hover, when the line is focused
- the 2x image versions are checked-in the source, so that they can be used in bug 837188

Comment 37

[Patrick Brosset <:pbro>]

Attachment: new-vview-icons.mov

Darrin, here's what it looks like with the new icons in.

Comment 38

[Darrin Henein [:darrin]]

Attachment: icons v3

Comment 39

[Patrick Brosset <:pbro>]

Attachment: vview-new-icons.mov

v3 icons are in.
See the video and let me know if you're happy with the result.
Personally, I agree that the flat look works better.

Comment 40

[Darrin Henein [:darrin]]

Attachment: icons v4

Last one I promise!

Comment 41

[Girish Sharma [:Optimizer]]

(In reply to Darrin Henein [:darrin] from comment #40)
> Created attachment 8368743 [details]
> icons v4
> 
> Last one I promise!

:D

Comment 42

[Patrick Brosset <:pbro>]

Attachment: vview-new-icons.mov

Comment 43

[Darrin Henein [:darrin]]

Comment on attachment 8368750 [details]
vview-new-icons.mov

Looks great. I believe @2x is happening in another bug, but icons are attached here (icons v4). Thanks!

Comment 44

[Patrick Brosset <:pbro>]

Attachment: bug952277-highlight-nodes-debugger v9.patch

Just changed the icons and rebased.

Comment 45

[Patrick Brosset <:pbro>]

Attachment: bug952277-debugger-test v5.patch

Just rebased.

New ongoing try: https://tbpl.mozilla.org/?tree=Try&rev=d42cbe8a0163
Although this was only a rebase, so I don't expect anything crazy.

Comment 46

[Patrick Brosset <:pbro>]

Try is green.
Both patches are R+ (I've re-applied the R+ on the last versions since they were just rebases and icon changes).
The icon changes are ui-R+.
Checking this in now!

Comment 47

[Patrick Brosset <:pbro>]

Fixed in fx-team
https://hg.mozilla.org/integration/fx-team/rev/790594df4527
https://hg.mozilla.org/integration/fx-team/rev/c63c7b1d253d

Comment 48

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/790594df4527
https://hg.mozilla.org/mozilla-central/rev/c63c7b1d253d

Comment 49

[:Gijs (he/him)]

Yeah, so, landing the code with 'Australis' and the test without - not a good idea. Behold, a bright orange mochitest-bc run on all platforms, builds and whatnot:

https://tbpl.mozilla.org/?tree=Holly&rev=fc7df5471afe

because the code got backed out from holly - but the test didn't.

Patrick, is this really meant to not land on holly, ie is this dependent on Australis? Or did you just add that to bypass the commit hook?

Comment 50

[Patrick Brosset <:pbro>]

Sorry about that, these 2 patches are indeed meant to go together and I didn't know about "OVERRIDE HOOK".

Comment 51

[:Gijs (he/him)]

Per discussion with Patrick, relanded the code parts as

https://hg.mozilla.org/projects/holly/rev/ebd5fe2ead7e

on holly.

Comment 53

[Ioana (away)]

Verified as fixed on the latest Firefox 29 beta and Firefox 30 aurora builds on Mac OS X 10.9.2, Windows XP 32bit and Ubuntu 13.04 32bit.