725235 - In the debugger, hovering over a variable or property in the source editor could show a details bubble

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

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

It would be nice if mouse is hovering over a variable or property in the source editor inside the debugger UI would show a bubble containing information like value, child properties etc., even if said variable/property isn't currently specifically added in a inspect/watch variables list.

Determining if we're over a var would require two things:
* text-under-cursor method for the source editor, or some way of determining this
* deciding if the text represents a field for which we can get additional info

Comment 1

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

Marking as P3 mainly because I assume it will need some SourceEditor work to come first.

Comment 2

[Mihai Sucan [:msucan]]

Panos, that's true.

Orion provides us with mouseover/move/out events and from those we can determine the line and character under the mouse pointer, from that we can determine the token/variable/object the user points to, and with enough data from the debugger, we can display a tooltip with any info we want.

In the source editor I need to expose the mouse events and we should have a utility method that retrieves the token at the given offset. What do you think? Shall I open two bug reports for these two source editor "features"? I also think we need a method to convert from mouse coordinates to line numbers and offsets in the source code.

Comment 3

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

(In reply to Mihai Sucan [:msucan] from comment #2)
> 
> In the source editor I need to expose the mouse events and we should have a
> utility method that retrieves the token at the given offset. What do you
> think? Shall I open two bug reports for these two source editor "features"?

That makes perfect sense to me.

> I also think we need a method to convert from mouse coordinates to line
> numbers and offsets in the source code.

Yay goody goody!

Comment 4

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

(In reply to Victor Porof from comment #3)
> (In reply to Mihai Sucan [:msucan] from comment #2)
> > 
> > In the source editor I need to expose the mouse events and we should have a
> > utility method that retrieves the token at the given offset. What do you
> > think? Shall I open two bug reports for these two source editor "features"?
> 
> That makes perfect sense to me.

Agreed.

Comment 5

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

Until the mouse API is finished, I can work on the bubbles :)

Comment 6

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

Suppose we have:

let interesting = 12;
// This is a very interesting comment.

Hovering over "interesting" in the comment shouldn't show a bubble. We need a pretty good way of telling what's analyzable and what not.

Comment 7

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

Filed bug 751844.

Comment 8

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

Bug 732442 serves the same purpose I think as bug 751844.

Comment 9

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

Attachment: v1

Positioning would've been much easier if bug 619887 was fixed.
We can still get away with it, however, if the necessary mouse API bits in the source editor are added.

Comment 10

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

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

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

Looking good!

::: browser/devtools/debugger/debugger-view.js
@@ +1877,5 @@
>  /**
> + * A popup containing information about a variable or property.
> + */
> +function VariableDetailsPopup() {
> +  this._onSourceEditorClick = this._onSourceEditorClick.bind(this);

Use a different name for the bound method. See bug 749628 comment 23.

@@ +1937,5 @@
> +   * Initialization function, called when the debugger is initialized.
> +   */
> +  initialize: function VDP_initialize() {
> +    this._anchor = DebuggerView.editor.parentElement;
> +    this._anchor.addEventListener("click", this._onSourceEditorClick, true);

I assume having to click instead of hover is temporary until dependent bugs are fixed?

Comment 11

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

(In reply to Panos Astithas [:past] from comment #10)
> Comment on attachment 621039 [details] [diff] [review]
> v1
> 
> Review of attachment 621039 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good!
> 
> ::: browser/devtools/debugger/debugger-view.js
> @@ +1877,5 @@
> >  /**
> > + * A popup containing information about a variable or property.
> > + */
> > +function VariableDetailsPopup() {
> > +  this._onSourceEditorClick = this._onSourceEditorClick.bind(this);
> 
> Use a different name for the bound method. See bug 749628 comment 23.
> 

Ugh :(
That would be so inconsistent with everything that's in the debugger codebase.

> @@ +1937,5 @@
> > +   * Initialization function, called when the debugger is initialized.
> > +   */
> > +  initialize: function VDP_initialize() {
> > +    this._anchor = DebuggerView.editor.parentElement;
> > +    this._anchor.addEventListener("click", this._onSourceEditorClick, true);
> 
> I assume having to click instead of hover is temporary until dependent bugs
> are fixed?

Yes.

Comment 12

[Rob Campbell [:rc] (:robcee)]

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

do we need an actual <panel> here or can this be accomplished with a tooltiptext attribute on the appropriate target node?

Comment 13

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #12)
> Comment on attachment 621039 [details] [diff] [review]
> v1
> 
> do we need an actual <panel> here or can this be accomplished with a
> tooltiptext attribute on the appropriate target node?

I think we want the ability to move the popup wherever we want (aka heavy mouse action). Does tooltiptext work that way?

Comment 14

[Rob Campbell [:rc] (:robcee)]

(In reply to Victor Porof from comment #13)
> (In reply to Rob Campbell [:rc] (:robcee) from comment #12)
> > Comment on attachment 621039 [details] [diff] [review]
> > v1
> > 
> > do we need an actual <panel> here or can this be accomplished with a
> > tooltiptext attribute on the appropriate target node?
> 
> I think we want the ability to move the popup wherever we want (aka heavy
> mouse action). Does tooltiptext work that way?

no, it's anchored near the pointer, iirc and pretty transient (disappears on mousemove). If you want finer control you'll need a panel. Carry on!

Comment 15

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

Just making a note that we'll need to expose an orion private API in order to make this happen: editor._view.getLocationAtOffset, but this bug it's a bit low priority for now and I'll get to it later (it will also be useful in bug 717366).

Comment 16

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

From recent discussions it seems that the Parser API is here to stay. Let's use it for this bug.

Also, I'll try to just use tooltips for the popup. If you think about it, there's no need for draggable windows.

Comment 17

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

(In reply to Victor Porof [:vp] from comment #16)
> From recent discussions it seems that the Parser API is here to stay. Let's
> use it for this bug.
> 
> Also, I'll try to just use tooltips for the popup. If you think about it,
> there's no need for draggable windows.

Note: this doesn't mean the dependencies have changed.

Comment 19

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

Dave was asking me about this yesterday.

Comment 20

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

Ok, bumping priority and starting work on this again.

Comment 21

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

This will depend on the CodeMirror migration.

Comment 22

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

Attachment: teaser

Comment 23

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

Attachment: dbg-parser-cleanup.patch

Some long overdue cleanup. I'll probably file a different bug for this.

Comment 24

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

Attachment: dbg-hover.patch

WIP patch implementing the popups and the parser mechanism for getting identifiers etc.

Comment 25

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

Attachment: dbg-hover.patch

Works, no tests.

Comment 26

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

Attachment: v1

Fixed typo and an existing failing test.
Panos, mind taking a look at this while I'm finishing writing some new tests?

Comment 27

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

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

Sorry for the drive-by, but I just had a couple questions:

::: browser/devtools/debugger/debugger-panes.js
@@ +1240,5 @@
>  
>  /**
> + * Functions handling the variables bubble UI.
> + */
> +function VariableBubbleView() {

Why is this in debugger-panes when it isn't in one of the two panes? Shouldn't this be in -view? Or (aside/follow up) can we just put every view in its own file instead of having semi-arbitrary boundaries based on where things happen to be located in the UI (please)?

@@ +1384,5 @@
> +    };
> +
> +    // Evaluate the identifier in the current stack frame and show the
> +    // results in a VariablesView inspection popup.
> +    this._widget.eval(nodeInfo.evalString, 0, FRAME_META.IDENTIFIER_EVAL)

Can't we get at this through the current frame's environment instead of via an eval? That way you wouldn't have to muck about with types of frames, and hiding meta frames, etc etc. This might also let you just find the word the mouse is hovering over, and then check if it is in the environment chain, instead of doing all this parsing here as well.

Comment 28

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #27)
> Comment on attachment 814858 [details] [diff] [review]
> v1
> 
> Review of attachment 814858 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the drive-by, but I just had a couple questions:
>

Drive-by always welcome from you Nick :)
 
> ::: browser/devtools/debugger/debugger-panes.js
> @@ +1240,5 @@
> >  
> >  /**
> > + * Functions handling the variables bubble UI.
> > + */
> > +function VariableBubbleView() {
> 
> Why is this in debugger-panes when it isn't in one of the two panes?
> Shouldn't this be in -view? Or (aside/follow up) can we just put every view
> in its own file instead of having semi-arbitrary boundaries based on where
> things happen to be located in the UI (please)?
> 

I honestly don't care about it so much. Since the editor is, well, a pane, I thought this makes sense to be in debugger-panes.js, but I wholeheartedly agree with you that it's a semi-randomly argumented decision. debugger-view.js might make more sense. Having every tiny UI widget in its own file might be bad for performance, because disk activity.

> @@ +1384,5 @@
> > +    };
> > +
> > +    // Evaluate the identifier in the current stack frame and show the
> > +    // results in a VariablesView inspection popup.
> > +    this._widget.eval(nodeInfo.evalString, 0, FRAME_META.IDENTIFIER_EVAL)
> 
> Can't we get at this through the current frame's environment instead of via
> an eval? That way you wouldn't have to muck about with types of frames, and
> hiding meta frames, etc etc. This might also let you just find the word the
> mouse is hovering over, and then check if it is in the environment chain,
> instead of doing all this parsing here as well.

Of course we can, but that would be severely limiting for a number of reasons:
1. a function environment only contains the variables and arguments, obviously no properties etc.
2. 'with' scopes are a pain to work with.
3. I found that limiting the bubble to only show top-level variables is extremely annoying.

My approach also handles hovering "baz" in foo.bar.baz, in which case it shows the value for "baz". Using the environment in this case would require several protocol round-trips instead of just one.

I also handle hovering "baz" in var foo = { bar: { baz: ... } }, which would be impossible using only the environments. I've also found that handling such cases can very useful.

Comment 29

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

I didn't break anything: https://tbpl.mozilla.org/?tree=Try&rev=881ab19824b5

Comment 30

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

(In reply to Victor Porof [:vp] from comment #28)
> > 
> > Can't we get at this through the current frame's environment instead of via
> > an eval? That way you wouldn't have to muck about with types of frames, and
> > hiding meta frames, etc etc. This might also let you just find the word the
> > mouse is hovering over, and then check if it is in the environment chain,
> > instead of doing all this parsing here as well.
> 
> Of course we can, but that would be severely limiting for a number of
> reasons:
> 1. a function environment only contains the variables and arguments,
> obviously no properties etc.
> 2. 'with' scopes are a pain to work with.
> 3. I found that limiting the bubble to only show top-level variables is
> extremely annoying.
> 
> My approach also handles hovering "baz" in foo.bar.baz, in which case it
> shows the value for "baz". Using the environment in this case would require
> several protocol round-trips instead of just one.
> 
> I also handle hovering "baz" in var foo = { bar: { baz: ... } }, which would
> be impossible using only the environments. I've also found that handling
> such cases can very useful.

PS: I also handle this.foo, which again, needs an eval to work properly.

Comment 31

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Ah, good stuff, thanks for the explanation.

Comment 32

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

Attachment: v2

Fixed a few dumb things, like the whole panel disappearing if a tooltip was would appear inside it. Fun.

Sorry about the double r? past.

Comment 33

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

Why the embedded search field in the popup? Popups are by their nature transient, so I wouldn't think anyone would expect to be able to work on them beyond quick glancing. Is this feature present in some other tool that I'm not aware of?

Comment 34

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

(In reply to Panos Astithas [:past] from comment #33)
> Why the embedded search field in the popup? Popups are by their nature
> transient, so I wouldn't think anyone would expect to be able to work on
> them beyond quick glancing. Is this feature present in some other tool that
> I'm not aware of?

Largely I agree, however panels behave a little bit differently than transient popups though, in that they don't automatically hide, unless there's an explicit user action that triggers this. Since one might hover "window" and get a large list of variables, it made sense to me to include the search box, because it can come in handy. No other tool has this afaik, but then again, they don't have the variable searching capabilities that our debugger offers anyway.

Comment 35

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

Attachment: Screenshot of the problem on linux

Still reviewing, but I came across this weird bug on Linux. Notice the error message in the terminal that may be responsible for the rendering glitch.

Comment 36

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

Wooohooo I love panels! :) Thanks for finding that, I fixed it.

Comment 37

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

Whoa, that was fast!

Regarding the search field, I understand the extra capabilities it provides, but I don't think someone who wants to examine the window object (or a TypedArray, or whatever giant object you can think of) will start looking for it in the editor, in order to hover the cursor over it. Elegant design is always a tradeoff between simplicity and additional features and in this case it looks to me like we're going this way:

https://www.evernote.com/shard/s312/sh/e90fdfd2-c9fd-46f7-8783-485b574e65e7/ae953707ba43857c9e55c9829fdca4fd

But I'm not going to pretend that my opinion is the only one that counts :-)

Comment 38

[Julien Wajsberg [:julienw]]

I actually like the searching ability, and I understand it could be then reused in a lot of other places.

Yet it could be in another panel, accessed when clicking the object somehow, as I'm afraid that showing this in the overlay would be too small in space for big objects ?

Comment 39

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

Comment on attachment 815433 [details] [diff] [review]
v2

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

This is coming along nicely! Besides tests, this only needs attention to a couple of issues I mention below and it should be good to go. 

One thing that I find suboptimal is that object properties look editable, even though modifying them doesn't work as in the variables view. We should either make editing work (preferably) or make them read-only.

::: browser/devtools/debugger/debugger-controller.js
@@ +72,5 @@
>    OPTIONS_POPUP_HIDDEN: "Debugger:OptionsPopupHidden",
>  };
>  
> +// Descriptions for what a stack frame represents after the debugger pauses.
> +const FRAME_META = {

Nit: I would have called this FRAME_TYPE, but it's up to you.

@@ +885,2 @@
>        this.activeThread.eval(frame.actor, aExpression);
> +      this.activeThread.addOneTimeListener("paused", (aEvent, aPacket) => {

This seems backwards, adding the listener after calling eval risks missing the event if the time god decides to have fun.

::: browser/devtools/debugger/debugger-panes.js
@@ +1385,5 @@
> +
> +    // Evaluate the identifier in the current stack frame and show the
> +    // results in a VariablesView inspection popup.
> +    let { evalString } = nodeInfo;
> +    this._widget.eval(evalString, 0, FRAME_META.IDENTIFIER_EVAL)

Why isn't this using the currently selected frame instead? Users will no doubt come to expect this.

There is a nice use case in this script:

http://htmlpad.org/debugger-bfcache/

After pausing at the debugger statement, both the top-most and the bottom-most frame have an |a| variable, but with different values. We could easily fix the display of the bottom-most frame's |a| when that frame is selected, if we just used the selected frame in the eval call.

There is a harder problem in displaying the right value when hovering over the |a| in load(), even when the top-most frame is selected. This would require that we figure out whether the enclosing function appears on the stack and if so, evaluate the identifier on the first of those frames (so inspecting the value in a recursive function would work).

I think between the parser and the protocol's frame list we should have all the necessary data to make this work.

::: browser/devtools/debugger/debugger.css
@@ +12,5 @@
>  
> +/* Variable bubble view */
> +
> +#variable-bubble-panel > .panel-arrowcontainer > .panel-arrowcontent {
> +  overflow: hidden;

This makes it hard to inspect long string values, wouldn't "overflow-x: scroll; overflow-y: hidden;" work?

::: browser/devtools/shared/Parser.jsm
@@ +189,5 @@
>          // need to be updated. If an exception is thrown here, file a bug.
>          log("syntax tree", e);
>        }
>      }
> +    // this._cache.set(requestId, results);

Why is this commented?

@@ +431,5 @@
>     *        The column number to check.
>     * @return boolean
>     *         True if the line and column is contained in the node's bounds.
>     */
> +  nodeContainsBounds: function(aNode, aLine, aColumn) {

Nit: I would call this nodeContainsPoint, nodeContainsCursor or something similar.

Comment 40

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

(In reply to Julien Wajsberg [:julienw] from comment #38)
> I actually like the searching ability, and I understand it could be then
> reused in a lot of other places.
> 
> Yet it could be in another panel, accessed when clicking the object somehow,
> as I'm afraid that showing this in the overlay would be too small in space
> for big objects ?

You can already filter variables either by using the '*' prefix in the toolbar's search box, or by enabling the optional variables search field from the gear menu. These are the places where I expect more elaborate object inspection will be performed, due to their larger size and non-ephemeral state.

Comment 41

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

(In reply to Panos Astithas [:past] from comment #37)
> 
> https://www.evernote.com/shard/s312/sh/e90fdfd2-c9fd-46f7-8783-485b574e65e7/
> ae953707ba43857c9e55c9829fdca4fd
> 
> But I'm not going to pretend that my opinion is the only one that counts :-)

What would you say about reusing the "Show variables filter box" option in the gear menu for the popup as well? It's off by default, so if someone would really require this functionality, they can easily turn it on.

Comment 42

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

Yes, that sounds like a good compromise!

Comment 43

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

> One thing that I find suboptimal is that object properties look editable,
> even though modifying them doesn't work as in the variables view. We should
> either make editing work (preferably) or make them read-only.
> 

Agreed, I forgot to add a proper eval function or something else is going on. They should be indeed editable.

> ::: browser/devtools/debugger/debugger-controller.js
> @@ +72,5 @@
> >    OPTIONS_POPUP_HIDDEN: "Debugger:OptionsPopupHidden",
> >  };
> >  
> > +// Descriptions for what a stack frame represents after the debugger pauses.
> > +const FRAME_META = {
> 
> Nit: I would have called this FRAME_TYPE, but it's up to you.
> 

Sure!

> @@ +885,2 @@
> >        this.activeThread.eval(frame.actor, aExpression);
> > +      this.activeThread.addOneTimeListener("paused", (aEvent, aPacket) => {
> 
> This seems backwards, adding the listener after calling eval risks missing
> the event if the time god decides to have fun.
> 
> ::: browser/devtools/debugger/debugger-panes.js
> @@ +1385,5 @@
> > +
> > +    // Evaluate the identifier in the current stack frame and show the
> > +    // results in a VariablesView inspection popup.
> > +    let { evalString } = nodeInfo;
> > +    this._widget.eval(evalString, 0, FRAME_META.IDENTIFIER_EVAL)
> 
> Why isn't this using the currently selected frame instead? Users will no
> doubt come to expect this.
> 
> There is a nice use case in this script:
> 
> http://htmlpad.org/debugger-bfcache/
> 
> After pausing at the debugger statement, both the top-most and the
> bottom-most frame have an |a| variable, but with different values. We could
> easily fix the display of the bottom-most frame's |a| when that frame is
> selected, if we just used the selected frame in the eval call.
> 
> There is a harder problem in displaying the right value when hovering over
> the |a| in load(), even when the top-most frame is selected. This would
> require that we figure out whether the enclosing function appears on the
> stack and if so, evaluate the identifier on the first of those frames (so
> inspecting the value in a recursive function would work).
> 
> I think between the parser and the protocol's frame list we should have all
> the necessary data to make this work.
> 

Figuring out the correct frame to eval in is a bit tricky, as you may suspect. You can't use the naive approach and count block or function environments upwards from the identifier, because that obviously doesn't work for non-nested calls. That is, "n lexical scopes upwards != n actual scope upwards".

In principle finding the enclosing function's name would be ok-ish, as long as the enclosing function has a name, in which case we just need to pick the correct scope based on that. However, this gets harder with "with" scopes or anonymous functions, where the parser API doesn't expose the "inferred name", an algorithm that might be a bit hard to duplicate, if not already public somehow.

I just tested if other browsers actually try and be smart about this, and it seems that nobody bothers to find a variable's value outside the current scope. This is, of course, something we can innovate with, but I'd love to talk to the people responsible with the reflection API to see if there's an easier way of retrieving the inferred function name.

Would it be ok for this to be a followup? What do you think?

> ::: browser/devtools/debugger/debugger.css
> @@ +12,5 @@
> >  
> > +/* Variable bubble view */
> > +
> > +#variable-bubble-panel > .panel-arrowcontainer > .panel-arrowcontent {
> > +  overflow: hidden;
> 
> This makes it hard to inspect long string values, wouldn't "overflow-x:
> scroll; overflow-y: hidden;" work?
> 

That would render the F S N and lock icons invisible in some cases, but maybe that's better, albeit a bit awkward? I don't know. What do you think?

> ::: browser/devtools/shared/Parser.jsm
> @@ +189,5 @@
> >          // need to be updated. If an exception is thrown here, file a bug.
> >          log("syntax tree", e);
> >        }
> >      }
> > +    // this._cache.set(requestId, results);
> 
> Why is this commented?
> 

Debugging. I'll uncomment it in the next patch.

> @@ +431,5 @@
> >     *        The column number to check.
> >     * @return boolean
> >     *         True if the line and column is contained in the node's bounds.
> >     */
> > +  nodeContainsBounds: function(aNode, aLine, aColumn) {
> 
> Nit: I would call this nodeContainsPoint, nodeContainsCursor or something
> similar.

nodeContainsCursor does sound better!

Comment 44

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

Attachment: v3

Fixed rendering issue on Linux, addressed comments, polished a bunch of other things and added some tests.

Comment 45

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=28a734e6c789

Comment 46

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

(In reply to Victor Porof [:vp] from comment #43)
> > ::: browser/devtools/debugger/debugger-panes.js
> > @@ +1385,5 @@
> > > +
> > > +    // Evaluate the identifier in the current stack frame and show the
> > > +    // results in a VariablesView inspection popup.
> > > +    let { evalString } = nodeInfo;
> > > +    this._widget.eval(evalString, 0, FRAME_META.IDENTIFIER_EVAL)
> > 
> > Why isn't this using the currently selected frame instead? Users will no
> > doubt come to expect this.
> > 
> > There is a nice use case in this script:
> > 
> > http://htmlpad.org/debugger-bfcache/
> > 
> > After pausing at the debugger statement, both the top-most and the
> > bottom-most frame have an |a| variable, but with different values. We could
> > easily fix the display of the bottom-most frame's |a| when that frame is
> > selected, if we just used the selected frame in the eval call.
> > 
> > There is a harder problem in displaying the right value when hovering over
> > the |a| in load(), even when the top-most frame is selected. This would
> > require that we figure out whether the enclosing function appears on the
> > stack and if so, evaluate the identifier on the first of those frames (so
> > inspecting the value in a recursive function would work).
> > 
> > I think between the parser and the protocol's frame list we should have all
> > the necessary data to make this work.
> > 
> 
> Figuring out the correct frame to eval in is a bit tricky, as you may
> suspect. You can't use the naive approach and count block or function
> environments upwards from the identifier, because that obviously doesn't
> work for non-nested calls. That is, "n lexical scopes upwards != n actual
> scope upwards".
> 
> In principle finding the enclosing function's name would be ok-ish, as long
> as the enclosing function has a name, in which case we just need to pick the
> correct scope based on that. However, this gets harder with "with" scopes or
> anonymous functions, where the parser API doesn't expose the "inferred
> name", an algorithm that might be a bit hard to duplicate, if not already
> public somehow.
> 
> I just tested if other browsers actually try and be smart about this, and it
> seems that nobody bothers to find a variable's value outside the current
> scope. This is, of course, something we can innovate with, but I'd love to
> talk to the people responsible with the reflection API to see if there's an
> easier way of retrieving the inferred function name.
> 
> Would it be ok for this to be a followup? What do you think?

I assume you mean the second case is hard, right? Always using the currently selected frame in eval() should be straightforward.

I'm OK with doing the second case in a followup, but let me describe the simple algorithm I have in mind:

1. Figure out the enclosing function of the current identifier, as you already do.
2. Check if this function is one of the call frames on the stack, by looking at frames[i].environment.function.{url,line,displayName} (the response from the "frames" protocol request) and comparing to what you have from the parser about the function.
3. If you have a match, pass that frame to eval(), otherwise default to frame 0.

> > ::: browser/devtools/debugger/debugger.css
> > @@ +12,5 @@
> > >  
> > > +/* Variable bubble view */
> > > +
> > > +#variable-bubble-panel > .panel-arrowcontainer > .panel-arrowcontent {
> > > +  overflow: hidden;
> > 
> > This makes it hard to inspect long string values, wouldn't "overflow-x:
> > scroll; overflow-y: hidden;" work?
> > 
> 
> That would render the F S N and lock icons invisible in some cases, but
> maybe that's better, albeit a bit awkward? I don't know. What do you think?

They are already invisible for the long properties that are cut off, although this is not the common case. Alternatively, can we use tooltips for the values instead of adding a horizontal scrollbar? That's what chrome does and it seems OK, too.

Comment 47

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

(In reply to Panos Astithas [:past] from comment #46)
> 
> I assume you mean the second case is hard, right? Always using the currently
> selected frame in eval() should be straightforward.
> 

Yes.

> I'm OK with doing the second case in a followup, but let me describe the
> simple algorithm I have in mind:
> 
> 1. Figure out the enclosing function of the current identifier, as you
> already do.
> 2. Check if this function is one of the call frames on the stack, by looking
> at frames[i].environment.function.{url,line,displayName} (the response from
> the "frames" protocol request) and comparing to what you have from the
> parser about the function.
> 3. If you have a match, pass that frame to eval(), otherwise default to
> frame 0.
> 

We're on the same page here, except that not all functions have a name, in which case the displayName cannot be easily retrieved via the parser API (it's not exposed, and duplicating the algorithm can be tedious and incompatible in subtle ways).

Comment 48

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

(In reply to Victor Porof [:vp] from comment #47)
> We're on the same page here, except that not all functions have a name, in
> which case the displayName cannot be easily retrieved via the parser API
> (it's not exposed, and duplicating the algorithm can be tedious and
> incompatible in subtle ways).

Sure, but the name won't matter in most cases since we have location information for the function. When the situation is ambiguous we just fall back to the default frame 0.

Comment 49

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

(In reply to Panos Astithas [:past] from comment #48)
> 
> Sure, but the name won't matter in most cases since we have location
> information for the function. When the situation is ambiguous we just fall
> back to the default frame 0.

Played with this, and using only the location does actually work nicely for functions, where once can compare the definition locations from the parser vs. what the protocol returns (which is also the definition location). No need for function names.

Block scopes and with scopes require more special care, since they don't inherently have a "declaration" location, but certain bounds in which they are in effect. Comparing the location (that the protocol returns) with those bounds might work. One would need to find the innermost such scope for which the stack frame location is contained in the bounds retrieved by the parser API.

Comment 50

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

Attachment: v4

Rebased on top of bug 919709. Not fun.

Comment 51

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

Attachment: v5

Rebased yet again...

Comment 52

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

Attachment: v6

Moar rebasing, and fixing a number of things I observed while playing with this:
* after opening a bubble, inspecting variables in side pane didn't work anymore ("no such actor"s)
* evaluating an identifier in a not-topmost frame would switch the editor location back to the topmost frame, leaving an orphaned bubble lying around
* some other things I can't remember but I'm gland they're fixed

Panos, when you get the time, can you please play around with this in Linux for a bit and let me know if I missed anything, ux-wise?

Comment 54

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

Attachment: Screenshot of the error

I could only spot 3 issues while playing with it, this looks very good!

I got an error that is visible in the screenshot, when hovering over the identifier that was in a part of the source that hadn't executed yet. I've opened the file in view source, so that you could see the line number.

The second issue was that when scrolling away from the highlighted line (where execution is paused) and then trying to inspect a variable, would case the bubble to flash briefly and the editor to scroll back to the highlighted line.

The third issue is the chopping of long stings that we've discussed before and the fact that editing values doesn't work.

Another issue is that the bubble background doesn't follow the selected theme, which is more obvious in Ubuntu with its default dark tooltip background, but I guess that will be fixed when the VariablesView is properly themed.

Comment 55

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

(In reply to Panos Astithas [:past] from comment #54)
> Created attachment 825806 [details]
> Screenshot of the error
> 
> I could only spot 3 issues while playing with it, this looks very good!
> 
> I got an error that is visible in the screenshot, when hovering over the
> identifier that was in a part of the source that hadn't executed yet. I've
> opened the file in view source, so that you could see the line number.
> 

I think this is ok? The evaluation is throwing, in which case the popup isn't displayed, and the seemingly spooky error is shown in the console. I would vote for not showing the popup in such cases (versus displaying "undefined"). Making the logged error in the console less spooky is a thing to consider.

Comment 56

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

(In reply to Panos Astithas [:past] from comment #54)
> 
> The second issue was that when scrolling away from the highlighted line
> (where execution is paused) and then trying to inspect a variable, would
> case the bubble to flash briefly and the editor to scroll back to the
> highlighted line.
> 

Now that's a great catch! Thanks.

Comment 57

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

Attachment: v7

Fixes a few things:
* now able to edit values within the popup.
* the popup doesn't flicker and then get hidden when inspecting a variable far away from the current frame location

I don't know what to do about chopping the long strings... I tried throwing CSS at it and nothing happens. Will try some more.

Comment 58

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

(In reply to Victor Porof [:vp] from comment #55)
> I think this is ok? The evaluation is throwing, in which case the popup
> isn't displayed, and the seemingly spooky error is shown in the console. I
> would vote for not showing the popup in such cases (versus displaying
> "undefined"). Making the logged error in the console less spooky is a thing
> to consider.

Can we avoid treating this case as an error? I can almost see the bug reports flowing in about the error message :-)

(In reply to Victor Porof [:vp] from comment #57)
> I don't know what to do about chopping the long strings... I tried throwing
> CSS at it and nothing happens. Will try some more.

How about using the tooltip for the full contents in this case? Can we add an ellipsis to better convey the fact that it's not just a bug?

Comment 59

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

(In reply to Panos Astithas [:past] from comment #58)
> 
> Can we avoid treating this case as an error? I can almost see the bug
> reports flowing in about the error message :-)
> 

You bet.

Comment 60

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

Attachment: v8

LONG STRINGS ARE NOW DISPLAYED PROPERLY. I win XUL, you lose.

I also made Tooltip.js aware of variable views, so I'm using that instead of homebrewing panels in the debugger. Patrick, please take a look at those changes when you get the time :)

Next step: finish those tests.

Comment 61

[Patrick Brosset <:pbro>]

Comment on attachment 827997 [details] [diff] [review]
v8

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

::: browser/devtools/debugger/debugger-panes.js
@@ +1288,5 @@
> +
> +    this._tooltip.defaultPosition = EDITOR_VARIABLE_POPUP_POSITION;
> +    this._tooltip.defaultShowDelay = EDITOR_VARIABLE_HOVER_DELAY;
> +
> +    this._tooltip.panel.addEventListener("popuphiding", this._onPopupHiding);

Do you specifically need to listen for "popuphiding"? If "popuphidden" is enough, then you could do `this._tooltip.on("hidden", this._onPopupHidden);` instead.
Otherwise, I think it would be good to add both popuphiding and popupshowing as observable events of the Tooltip class.

@@ +1429,5 @@
> +    let anchor = editor.getCoordsFromPosition(identifierCenter);
> +
> +    this._tooltip.defaultOffsetX = anchor.left + EDITOR_VARIABLE_POPUP_OFFSET_X;
> +    this._tooltip.defaultOffsetY = anchor.top + EDITOR_VARIABLE_POPUP_OFFSET_Y;
> +    this._tooltip.show(this._editorContainer);

It seems to me that tooltip.startTogglingOnHover(baseNode, targetNodeCb) is exactly what you would need for this patch. What made you choose to call show() manually instead?
I see you have a lot of logic to execute on mouseover, to find identifiers at x/y coordinates.
To make the tooltip class more useful, perhaps when we execute the targetNodeCb function we should be passing not only the target node but also the x/y coordinates, or simply the mouseover event.

::: browser/devtools/shared/widgets/Tooltip.js
@@ +133,5 @@
> +   */
> +  get hidden()
> +    this.panel.state == "closed" ||
> +    this.panel.state == "hiding",
> +

Good point, I was in the process of adding the same function in bug 889638, except I didn't know about the panel's states. This version is better.
Although I would probably not have used a getter for this and would have named the function isHidden instead. Getters make me think there will be a setter.

@@ +192,5 @@
>     * @param {Number} showDelay
>     *        An optional delay that will be observed before showing the tooltip.
>     *        Defaults to 750ms
>     */
> +  startTogglingOnHover: function(baseNode, targetNodeCb, showDelay = this.defaultShowDelay) {

I have added something really similar in bug 931845 which should land anytime now.
So if and when you rebase, you'll have to merge that detail in.
In my version, I have made the defaultShowDelay a const in the module, but I guess your version is better, so please keep yours when/if you do merge.

@@ +310,5 @@
>    },
>  
>    /**
> +   * Fille the tooltip with an variables view, inspecting an object via its
> +   * cooresponding object actor, as sepcified in the remote debugging protocol.

Nits: couple of misspelled words: s/Fille/Fills and s/sepcified/specified

Comment 62

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

(In reply to Patrick Brosset from comment #61)

> 
> Do you specifically need to listen for "popuphiding"? If "popuphidden" is
> enough, then you could do `this._tooltip.on("hidden", this._onPopupHidden);`
> instead.
> Otherwise, I think it would be good to add both popuphiding and popupshowing
> as observable events of the Tooltip class.
> 

It's better to clear the state on popuphiding. Otherwise, stuff in the editor will hang around for a second or so while the panel fades away.

> @@ +1429,5 @@
> > +    let anchor = editor.getCoordsFromPosition(identifierCenter);
> > +
> > +    this._tooltip.defaultOffsetX = anchor.left + EDITOR_VARIABLE_POPUP_OFFSET_X;
> > +    this._tooltip.defaultOffsetY = anchor.top + EDITOR_VARIABLE_POPUP_OFFSET_Y;
> > +    this._tooltip.show(this._editorContainer);
> 
> It seems to me that tooltip.startTogglingOnHover(baseNode, targetNodeCb) is
> exactly what you would need for this patch. What made you choose to call
> show() manually instead?

I don't have (and shouldn't have) access to the text nodes in the editor, just the line/column ranges. Moreover, I need to hide the panel on precise events, like scrolling, so using show/hide seemed much easier.

> Good point, I was in the process of adding the same function in bug 889638,
> except I didn't know about the panel's states. This version is better.
> Although I would probably not have used a getter for this and would have
> named the function isHidden instead. Getters make me think there will be a
> setter.
> 

Done.

> I have added something really similar in bug 931845 which should land
> anytime now.
> So if and when you rebase, you'll have to merge that detail in.
> In my version, I have made the defaultShowDelay a const in the module, but I
> guess your version is better, so please keep yours when/if you do merge.
> 

Cool!

> @@ +310,5 @@
> >    },
> >  
> >    /**
> > +   * Fille the tooltip with an variables view, inspecting an object via its
> > +   * cooresponding object actor, as sepcified in the remote debugging protocol.
> 
> Nits: couple of misspelled words: s/Fille/Fills and s/sepcified/specified

Yeah, saw that after submitting the patch. Fixed.

Thanks!

Comment 63

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

(In reply to Patrick Brosset from comment #61)
> 
> Do you specifically need to listen for "popuphiding"? If "popuphidden" is
> enough, then you could do `this._tooltip.on("hidden", this._onPopupHidden);`
> instead.

In which patch are these events implemented? (they're not there yet afaict).

Comment 64

[Patrick Brosset <:pbro>]

You're right, I thought they were, but I added them in the color picker tooltip bug I'm working on and that hasn't landed yet. So don't bother with this.

Comment 65

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

Attachment: v9

Rebased again.

Comment 66

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

Attachment: v10

Rebased again and added a shitload of tests.
More to come.

Comment 67

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

Attachment: v11

Hello.

Comment 68

[Rob Campbell [:rc] (:robcee)]

good grief.

Comment 69

[Rob Campbell [:rc] (:robcee)]

some preliminary comments:

1. This is cool.
2. Slight delay opening the bubble.
3. Surprised to see it working across the whole file, even though foreign block-local variables are "undefined" (i.e., local variables in different scopes, functions).

minor rebasing in editor.js required post source editor update. I did it myself!

Comment 70

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #69)
> 2. Slight delay opening the bubble.

Do you think it should be longer or shorter? Ideally we should accidental popups.

> 3. Surprised to see it working across the whole file, even though foreign
> block-local variables are "undefined" (i.e., local variables in different
> scopes, functions).

Yup, see comment 43 and replies. Followup work, tricky but possible.

> 
> minor rebasing in editor.js required post source editor update. I did it
> myself!

Comment 71

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 8336008 [details] [diff] [review]
v11

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

lgtm.

::: browser/devtools/debugger/debugger-controller.js
@@ +758,5 @@
>     */
>    _onBlackBoxChange: function() {
>      if (this.activeThread.state == "paused") {
> +      // Hack to avoid selecting the topmost frame after blackboxing a source.
> +      this.currentFrameDepth = NaN;

really?

could you at least enclose this in a 

// XXX HAAAAACKK

comment?

@@ +918,5 @@
> +        } else {
> +          deferred.reject(new Error("The execution was abruptly terminated."));
> +        }
> +      } else {
> +        deferred.reject(new Error("Active thread paused unexpectedly."));

should these strings be localized?

Looking around at some of the other internal error messages we're throwing, probably doesn't make sense to try localizing them. Also, probably going to be very difficult for someone to translate a lot of these without understanding the code.

::: browser/devtools/debugger/debugger-view.js
@@ +27,5 @@
>  const SEARCH_FUNCTION_FLAG = "@";
>  const SEARCH_TOKEN_FLAG = "#";
>  const SEARCH_LINE_FLAG = ":";
>  const SEARCH_VARIABLE_FLAG = "*";
> +const EDITOR_VARIABLE_HOVER_DELAY = 500; // ms

bit slow. try 300 or 250?

::: browser/devtools/shared/widgets/Tooltip.js
@@ +193,5 @@
>  
>  Tooltip.prototype = {
>    defaultPosition: "before_start",
> +  defaultOffsetX: 0, // px
> +  defaultOffsetY: 0, // px

do we need units for 0? I guess we change these values later. NVM!

@@ +375,5 @@
>     * @param {node} content
>     *        A node that can be appended in the tooltip XUL element
>     */
>    set content(content) {
> +    if (this.content == content) {

this is a DOM node, right? Can use === here?

@@ +404,4 @@
>     */
> +  setTextContent: function(messages,
> +    messagesClass = "default-tooltip-simple-text-colors",
> +    containerClass = "default-tooltip-simple-text-colors") {

this indentation and brace style confused me. But that's just me. Not you.

@@ +443,5 @@
> +    objectActor,
> +    viewOptions = {},
> +    controllerOptions = {},
> +    relayEvents = {},
> +    reuseCachedWidget = true) {

and again. What is this, K&R?

Comment 72

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #71)
> 
> @@ +443,5 @@
> > +    objectActor,
> > +    viewOptions = {},
> > +    controllerOptions = {},
> > +    relayEvents = {},
> > +    reuseCachedWidget = true) {
> 
> and again. What is this, K&R?

Suggestions welcome.

Comment 73

[Julien Wajsberg [:julienw]]

Hey,

is there a try build somewhere ? I'd like to try the patch to feel how the delay is. I would even lower it to 100ms but I don't know how the popup is so this might be in the way of the user.

Another possibility (maybe for a follow-up patch) would be to show a simple title-like bubble containing a "toString" like value, constrained in its size (say: 10 or 15 characters max) immediately on hover.

Comment 74

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

Attachment: v12

Thanks for the review, Rob.

Rebased the patch on fx-team tip, addressed comments and fixed a bug where the topmost frame would't get reselected when stepping in, if a different frame was selected beforehand and a variable inspection popup was shown. Added more tests.

I'd still like to get more opinions on EDITOR_VARIABLE_HOVER_DELAY. Builds will be available soon.

Comment 75

[Patrick Brosset <:pbro>]

> I'd still like to get more opinions on EDITOR_VARIABLE_HOVER_DELAY. Builds
> will be available soon.

In the css rule-view and markup-view, we show image tooltips after 50ms. It is short enough to feel immediate when you want to see it, still it's long enough to avoid the tooltip from poping up when you just happened to hover over the element on your way somewhere else.

If there's some time-consuming server-side processing to do here, I don't know if 50ms would be feasible, but that would be my suggestion.

I had a look at chrome devtools and it seems they have some pretty long delay (almost a second I'd say). I find this a bit frustrating because when you want to evaluate something, you want the result now.

Comment 76

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

(In reply to Patrick Brosset from comment #75)
> > I'd still like to get more opinions on EDITOR_VARIABLE_HOVER_DELAY. Builds
> > will be available soon.
> 
> In the css rule-view and markup-view, we show image tooltips after 50ms. It
> is short enough to feel immediate when you want to see it, still it's long
> enough to avoid the tooltip from poping up when you just happened to hover
> over the element on your way somewhere else.

I don't think such a low delay is reasonable in this case, because contrary to the markup view the popup doesn't disappear on mousemove, as the user might want to expand some properties in it. So in case you accidentally cause a popup to appear while moving the cursor, you would have to hit ESC to dismiss it and then continue your movement, which will be frustrating.

The current value doesn't feel too slow for me, but I don't think I would mind something smaller either. Perhaps Rob's suggestion would feel exactly right.

Comment 77

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

I think 350 ms might be the sweet spot then? I accidentally triggered the popup a few times, that's why I felt like 500 ms is a better value.

FWIW, in Chrome's debugger it takes more than a second to show the popup.

Comment 78

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

(In reply to Julien Wajsberg [:julienw] from comment #73)
> Hey,
> 
> is there a try build somewhere ? I'd like to try the patch to feel how the
> delay is. I would even lower it to 100ms but I don't know how the popup is
> so this might be in the way of the user.
> 
> Another possibility (maybe for a follow-up patch) would be to show a simple
> title-like bubble containing a "toString" like value, constrained in its
> size (say: 10 or 15 characters max) immediately on hover.

Here are some builds: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-9c13c4d1b283/

Comment 79

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

Try is green: https://tbpl.mozilla.org/?tree=Try&rev=9c13c4d1b283

Comment 80

[Julien Wajsberg [:julienw]]

Hey,

first of all, thanks for the build !

here is my feedback, you can file follow-ups or change them here (or just ignore ;) ):

* the timeout feels actually too long

* When a popup is displayed, it's annoying to have to click to dismiss it before displaying another one. I think that as long as a user stayed a long time over another variable, the current popup should be dismissed and the new one be displayed

* when this is a compound expression (example: elements[i] while in a loop, so where i is existing), the popup is displaying "elements"; it would really help more to have elements[i] instead. By the way, in that case, hovering the "i" shows "undefined", which is surprising (when I hover the i in another place it shows the correct value)

* not sure the current popup when hovering on a function name is useful. Could be more useful to show the function.toSource() result.

Thanks for this, I'm really looking forward to use it :)

Comment 81

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

(In reply to Julien Wajsberg [:julienw] from comment #80)
> Hey,
> 
> first of all, thanks for the build !

Thanks for the feedback!

> * the timeout feels actually too long

Ok, will tone it down to 350 ms :)

> * When a popup is displayed, it's annoying to have to click to dismiss it
> before displaying another one. I think that as long as a user stayed a long
> time over another variable, the current popup should be dismissed and the
> new one be displayed
> 

I don't necessarily agree with this one. There's no way of accurately knowing when a user wants the popup hidden, and doing so automatically might be useful in some cases but annoying in others. I'd love to hear more opinions on this, but will file a followup.

> * when this is a compound expression (example: elements[i] while in a loop,
> so where i is existing), the popup is displaying "elements"; it would really
> help more to have elements[i] instead. By the way, in that case, hovering
> the "i" shows "undefined", which is surprising (when I hover the i in
> another place it shows the correct value)

Agreed, this would be helpful. Can you please share a snippet with the second problem (i being "undefined")? I can't reproduce after some mild testing, and as long as 'i' is defined it should work, but maybe there's some scenarios I can't think of.

> * not sure the current popup when hovering on a function name is useful.
> Could be more useful to show the function.toSource() result.

Also a good idea. But approaching this differently would be better (bug 786069). In that case, you'd have access to the function source both in the popup and the side pane.

Comment 82

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #81)

> > * When a popup is displayed, it's annoying to have to click to dismiss it
> > before displaying another one. I think that as long as a user stayed a long
> > time over another variable, the current popup should be dismissed and the
> > new one be displayed
> > 
> 
> I don't necessarily agree with this one. There's no way of accurately
> knowing when a user wants the popup hidden, and doing so automatically might
> be useful in some cases but annoying in others. I'd love to hear more
> opinions on this, but will file a followup.

While I agree that every user will have a different choice on this one, the most popular use case of these popups is to quickly have a look at the value of a variable in a glance, without much effort, while I am stopped at a breakpoint.

Thus I would prefer to have the popup behave like a hover-popup. Come while I hover, go away if I move away my mouse, come to the new location if I hover to a new location. If a user wants to do more with the popup's variables view, he can click on the popup to not make it go away.

Also, the hiding of the popup should be delayed, so that the user gets the time to go to the popup's content to click it. The popup should also not hide when the mouse is over the popup itself.

Comment 83

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

(In reply to Girish Sharma [:Optimizer] from comment #82)
> (In reply to Victor Porof [:vp] from comment #81)
> 

It seems there are mixed opinions about this, so I'll file a followup. Implementing the Right Behavior™ as expected by all parties is probably very delicate, but possible.

Comment 84

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

Filed bug 943341 and bug 943342.

Comment 85

[Julien Wajsberg [:julienw]]

(In reply to Victor Porof [:vp] from comment #81)

> 
> > * When a popup is displayed, it's annoying to have to click to dismiss it
> > before displaying another one. I think that as long as a user stayed a long
> > time over another variable, the current popup should be dismissed and the
> > new one be displayed
> > 
> 
> I don't necessarily agree with this one. There's no way of accurately
> knowing when a user wants the popup hidden, and doing so automatically might
> be useful in some cases but annoying in others. I'd love to hear more
> opinions on this, but will file a followup.

I'd say, if the user wants the popup to stay, he can keep the mouse inside the popup or out of the debugger window.

But rightly that's only my opinion and my way of working here so it would be useful to hear other opinions.

BTW Chrome is doing like I'm suggesting ;) (and I think all graphical debuggers (eg Eclipse) do this too)

> 
> > * when this is a compound expression (example: elements[i] while in a loop,
> > so where i is existing), the popup is displaying "elements"; it would really
> > help more to have elements[i] instead. By the way, in that case, hovering
> > the "i" shows "undefined", which is surprising (when I hover the i in
> > another place it shows the correct value)
> 
> Agreed, this would be helpful. Can you please share a snippet with the
> second problem (i being "undefined")? I can't reproduce after some mild
> testing, and as long as 'i' is defined it should work, but maybe there's
> some scenarios I can't think of.

I get it, it happens when the var i is "scoped" in a for loop, for example with this code:

    for (var i = 0, l = elements.length; i < l; i++) {
      console.log(elements[i]);
    }

(elements is a NodeList here)

Hovering on "i" inside "elements[i]" brings "undefined" but hovering inside the for parenthesis is working fine.

Comment 86

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

(In reply to Julien Wajsberg [:julienw] from comment #85)
> (In reply to Victor Porof [:vp] from comment #81)
> 
> BTW Chrome is doing like I'm suggesting ;) (and I think all graphical
> debuggers (eg Eclipse) do this too)
> 

Hmm, if there's a de-facto norm on how these popups behave, then yeah, we should probably follow it. See bug 943341.

Comment 87

[Julien Wajsberg [:julienw]]

Other ideas around this:
* how about adding a "watch" button in the popup, which would add the variable in the watched list?
* how about highlighting the var that will be inspected in the popup, but doing this as soon sa the var is hovered ?

Comment 88

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

(In reply to Julien Wajsberg [:julienw] from comment #87)

Sounds interesting, please file bugs and mark them as depending on this one.

Comment 89

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

Comment on attachment 8338306 [details] [diff] [review]
v12

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

::: browser/themes/osx/devtools/debugger.css
@@ +152,5 @@
> +.devtools-tooltip-simple-text.token-undefined,
> +.devtools-tooltip-simple-text.token-null {
> +  text-align: center;
> +  color: #666 !important; /* Override the theme's color. */
> +}

Heh, all of this needs to be moved to the theme files after bug 941799.

Comment 90

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

Comment on attachment 8338306 [details] [diff] [review]
v12

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

I'm happy!

::: browser/devtools/debugger/debugger-panes.js
@@ +1471,5 @@
> +   * The mousemove listener for the source editor.
> +   */
> +  _onMouseMove: function({ clientX: x, clientY: y }) {
> +    // Prevent the variable inspection popup from showing when the thread client
> +    // is paused, or while a popup is already visible.

s/is paused/is not paused/

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +927,5 @@
>    /**
>     * Gets the parent node holding this view.
>     * @return nsIDOMNode
>     */
> +  get boxObject() this._list.boxObject.QueryInterface(Ci.nsIScrollBoxObject),

SpiderMonkey-specific expression alert! (I'm sure you opted for consistency in the file.)

::: browser/devtools/sourceeditor/editor.js
@@ +531,5 @@
> +   * Mark a range of text inside the two {line, ch} bounds. Since the range may
> +   * be modified, for example, when typing text, this method returns a function
> +   * that can be used to remove the mark.
> +   */
> +  markText: function(from, to, className = "marked-text") {

Overriding the more versatile function with a simpler one is questionable, even if it ends up being cleaner for the only current use in our codebase. I'm not going to stop you, but we may have to revert this decision at some point.

Comment 91

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

(In reply to Panos Astithas [:past] from comment #90)
> Comment on attachment 8338306 [details] [diff] [review]

> @@ +531,5 @@
> > +   * Mark a range of text inside the two {line, ch} bounds. Since the range may
> > +   * be modified, for example, when typing text, this method returns a function
> > +   * that can be used to remove the mark.
> > +   */
> > +  markText: function(from, to, className = "marked-text") {
> 
> Overriding the more versatile function with a simpler one is questionable,
> even if it ends up being cleaner for the only current use in our codebase.
> I'm not going to stop you, but we may have to revert this decision at some
> point.

Agreed. However, Anton's plan with the Editor wrapper was to expose as little CodeMirror functionality as possible, to make an eventual (please no) transition easier. Since we're likely not going to need the plethora of methods available to "marks" (in the CodeMirror terminology), this simple function felt like a good approach.

Comment 92

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

FIXED IN FX-TEAM!
https://hg.mozilla.org/integration/fx-team/rev/9a1e06298854

Comment 93

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9a1e06298854

Comment 94

[Patrick Brosset <:pbro>]

(In reply to Victor Porof [:vporof][:vp] from comment #81)
> > * When a popup is displayed, it's annoying to have to click to dismiss it
> > before displaying another one. I think that as long as a user stayed a long
> > time over another variable, the current popup should be dismissed and the
> > new one be displayed
> > 
> 
> I don't necessarily agree with this one. There's no way of accurately
> knowing when a user wants the popup hidden, and doing so automatically might
> be useful in some cases but annoying in others. I'd love to hear more
> opinions on this, but will file a followup.
I know this is an old discussion, but after a long time using the debugger and the variables popup, I still find this annoying most of the times, so I thought I would report it here and maybe we can get a discussion started to eventually end up on a bug being filed.
I think:
- it takes too long for the popup to appear
- it should go away when you move your mouse out of the expression but:
  - it should do so only after a delay that allows the user to quickly move back the mouse to the expression if needed
  - it shouldn't occur if the mouse goes over the bubble itself.
I think this is pretty much what Chrome does. Also, beside my point of view, I've also had several reports of our implementation being a little annoying.
I'd like to hear what other people think about this.

Comment 95

[Karl Dubost💡 :karlcow]

To follow up on Patrick's comment:

Today, I was trying/expecting a bit of this when hovering a string in the debugger. Let's say this piece of code:

   n.target.setPlaybackRate(parseFloat(o));

when hovering "o" I would love to have the current value, and probably where the value was defined before in the code. So it would be cool to have an info bubble which could be extended in the future with more data. Giving a set of information and to make these data clickable when it's possible. Like for example, to go up the ladder for finding where does it come from, adjust the value when it's a number or a string. etc.

Comment 96

[Patrick Brosset <:pbro>]

(In reply to Karl Dubost :karlcow from comment #95)
>    n.target.setPlaybackRate(parseFloat(o));
> 
> when hovering "o" I would love to have the current value, and probably where
> the value was defined before in the code. So it would be cool to have an
> info bubble which could be extended in the future with more data. Giving a
> set of information and to make these data clickable when it's possible. Like
> for example, to go up the ladder for finding where does it come from, adjust
> the value when it's a number or a string. etc.
This sounds like a new bug you should file.

Comment 97

[Jim Blandy :jimb]

(In reply to Karl Dubost :karlcow from comment #95)
> where the value was defined before in the code.

FWIW: I've been using a C++ debugger that does deep dark magic to provide this kind of information, and it's invaluable. It's also extraordinarily difficult to obtain without using huge amounts of memory.

We don't have any plans to tackle this at the moment.