727429 - Add support for watch expressions to the debugger

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

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

The remote debugging protocol implementation already has the required functionality (clientEvaluate packet, ThreadClient.prototype.eval method), we just need to provide a UI for it. Using the same mechanism as in bug 723071 I think makes sense. We just need to decide what this will be.

Comment 1

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

This will essentially use the same implementation pattern in bug 740825.

Comment 2

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

Attachment: v1

Works! Needs test.

Comment 3

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

Attachment: v2

Fixed a few behavioral things. I should be finished with the test sometime tomorrow, but in the meantime some feedback is appreciated.

Comment 4

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

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

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

ok, lookin' good.

::: browser/devtools/debugger/debugger-controller.js
@@ +492,5 @@
>        }
>      }
>  
> +    // Watch expressions are evaluated in the context of the top level frame,
> +    // and the results and displayed in th variables view.

some funky commenting here.

@@ +495,5 @@
> +    // Watch expressions are evaluated in the context of the top level frame,
> +    // and the results and displayed in th variables view.
> +    if (this.currentWatchExpressions) {
> +      // Evaluation causes the stack frames to be cleared and active thread to
> +      // pause, sending a 'clientEvaluated' packed and adding the frames again.

s/packed/packet/

@@ +508,5 @@
> +      // If an error was thrown during the evaluation of the watch expressions,
> +      // then at least one expressions' syntax was malformed.
> +      if (this.currentEvaluation.throw) {
> +        DebuggerView.WatchExpressions.removeLastExpression();
> +        DebuggerView.WatchExpressions.saveExpressions();

I'm having a thought...

::: browser/devtools/debugger/debugger-panes.js
@@ +933,5 @@
> +create({ constructor: WatchExpressionsView, proto: MenuContainer.prototype }, {
> +  /**
> +   * Initialization function, called when the debugger is started.
> +   */
> +  initialize: function DVSF_initialize() {

DVSF == Debugger View Stack Frame? Might want to use DVWE or something for these signatures.

@@ +1018,5 @@
> +   *        The index used to identify the watch expression.
> +   * @return string
> +   *         The watch expression string.
> +   */
> +  getExpression: function DVSF_getExpression(aIndex) {

I think we can safely ditch the "Expression" part of these names. We're in a WatchExpressions view so it feels a bit redundant.

What do you think? Does that make this API too vague?

Also, get, add, remove, removeLast sound like a pretty solid base API for most of our panes in here. Might be able to move them higher up in the prototype chain.

::: browser/devtools/debugger/debugger-view.js
@@ +717,5 @@
> +   * @param boolean aVisibleFlag
> +   *        Specifies the intended visibility.
> +   */
> +  toggleContents: function DVMC_toggleContents(aVisibleFlag) {
> +    for (let [, item] of this._itemsByElement) {

ok. _itemsByElement is storing tuples, yes?

Comment 5

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #4)
> Comment on attachment 681532 [details] [diff] [review]
> v2
> 
> Review of attachment 681532 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ok, lookin' good.
> 

Thanks for the feedback! I take it that the functionality and UI/UX feels right? 

> @@ +508,5 @@
> > +      // If an error was thrown during the evaluation of the watch expressions,
> > +      // then at least one expressions' syntax was malformed.
> > +      if (this.currentEvaluation.throw) {
> > +        DebuggerView.WatchExpressions.removeLastExpression();
> > +        DebuggerView.WatchExpressions.saveExpressions();
> 
> I'm having a thought...
> 

removeLast and save?

> ::: browser/devtools/debugger/debugger-panes.js
> @@ +933,5 @@
> > +create({ constructor: WatchExpressionsView, proto: MenuContainer.prototype }, {
> > +  /**
> > +   * Initialization function, called when the debugger is started.
> > +   */
> > +  initialize: function DVSF_initialize() {
> 
> DVSF == Debugger View Stack Frame? Might want to use DVWE or something for
> these signatures.
> 

Yup. Copy paste matrix glitch.

> @@ +1018,5 @@
> > +   *        The index used to identify the watch expression.
> > +   * @return string
> > +   *         The watch expression string.
> > +   */
> > +  getExpression: function DVSF_getExpression(aIndex) {
> 
> I think we can safely ditch the "Expression" part of these names. We're in a
> WatchExpressions view so it feels a bit redundant.
> 
> What do you think? Does that make this API too vague?
> 

Hmm, I agree. Since most of the calls already have the Foo.Bar signature on the left, we're probably safe.

> Also, get, add, remove, removeLast sound like a pretty solid base API for
> most of our panes in here. Might be able to move them higher up in the
> prototype chain.
> 

This I also agree to a point. However, the common implementation for 'adding stuff' is already there, the push(), commit(), remove(), empty() etc. methods, all part of the parent prototype. 

The biggie here is that, for example, addExpression behaves slightly different from addBreakpoint. Same goes for getExpression vs getBreakpoint, and so on, all of these expecting different params (to make it more accessible to play with the data). It's probably too much of a burden on different menu container implementations to abstract a common API.

> ::: browser/devtools/debugger/debugger-view.js
> @@ +717,5 @@
> > +   * @param boolean aVisibleFlag
> > +   *        Specifies the intended visibility.
> > +   */
> > +  toggleContents: function DVMC_toggleContents(aVisibleFlag) {
> > +    for (let [, item] of this._itemsByElement) {
> 
> ok. _itemsByElement is storing tuples, yes?

Yes. [nsIDOMNode, MenuItem].

Comment 6

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

Attachment: v3

Better handling strings as expressions. Multiple statements in the same expression also supported.

Comment 7

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

Note: watch expressions only work in the context of the topmost scope. Working on it.

Comment 8

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

Comment on attachment 682468 [details] [diff] [review]
v3

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

::: browser/devtools/debugger/debugger-controller.js
@@ +921,5 @@
> +      this.queuedWatchExpressions =
> +        this.currentWatchExpressions = "[" + aList.map(function(str)
> +          "(function() {" +
> +            "try { return eval(\"" + str.replace(/"/g, "\\$&") + "\"); }" +
> +            "catch(e) { return undefined; }" +

Shouldn't you stringify the error message here instead? That way we can display the error message for expressions that throw.

Comment 9

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

(In reply to Panos Astithas [:past] from comment #8)
> Comment on attachment 682468 [details] [diff] [review]
> v3
> 
> Review of attachment 682468 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/debugger/debugger-controller.js
> @@ +921,5 @@
> > +      this.queuedWatchExpressions =
> > +        this.currentWatchExpressions = "[" + aList.map(function(str)
> > +          "(function() {" +
> > +            "try { return eval(\"" + str.replace(/"/g, "\\$&") + "\"); }" +
> > +            "catch(e) { return undefined; }" +
> 
> Shouldn't you stringify the error message here instead? That way we can
> display the error message for expressions that throw.

Yeah, I was just worried that the error message would be confused with the evaluation result, but I think I can tweak the variables view to show these in red or something.

Comment 10

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

You could also return the error object and check the type in the client.

Comment 11

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

Attachment: v4

Addressed comments and added 2 tests.

Comment 12

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

(In reply to Panos Astithas [:past] from comment #10)
> You could also return the error object and check the type in the client.

I tried that, however, the error object didn't contain the name and message, only the __proto__, for which both of them were empty string. I just return e.name + ': ' + e.message now and it feels good enough.

Comment 13

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

(In reply to Victor Porof [:vp] from comment #12)
> (In reply to Panos Astithas [:past] from comment #10)
> > You could also return the error object and check the type in the client.
> 
> I tried that, however, the error object didn't contain the name and message,
> only the __proto__, for which both of them were empty string. I just return
> e.name + ': ' + e.message now and it feels good enough.

Can you file a bug about that? We have the same problem when breaking on exceptions, since most of the time (always?) I don't see these properties in the variables view either.

Perhaps returning the result of JSON.parse(JSON.stringify(error)) would work in both cases.

Comment 14

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

(In reply to Panos Astithas [:past] from comment #13)
> (In reply to Victor Porof [:vp] from comment #12)
> > (In reply to Panos Astithas [:past] from comment #10)
> > > You could also return the error object and check the type in the client.
> > 
> > I tried that, however, the error object didn't contain the name and message,
> > only the __proto__, for which both of them were empty string. I just return
> > e.name + ': ' + e.message now and it feels good enough.
> 
> Can you file a bug about that? We have the same problem when breaking on
> exceptions, since most of the time (always?) I don't see these properties in
> the variables view either.
> 
> Perhaps returning the result of JSON.parse(JSON.stringify(error)) would work
> in both cases.

Sure thing.

Comment 15

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

Attachment: v4.1

Whoops, pressing the delete button on a watch expression caused all of them to be deleted sequentially :)

Comment 16

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

(In reply to Panos Astithas [:past] from comment #13)

Filed bug 812764.

Comment 17

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

Green. https://tbpl.mozilla.org/?tree=Try&rev=e1f85abdecd0

Comment 18

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

Comment on attachment 682740 [details] [diff] [review]
v4.1

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

not a lot of comments. Nothing deal-breaking in any of this, though I would like to try using the defaults for fonts and see some localized strings for those separators. r+ with those and a try run.

::: browser/devtools/debugger/debugger-view.js
@@ +10,5 @@
>  const PANES_APPEARANCE_DELAY = 50; // ms
>  const BREAKPOINT_LINE_TOOLTIP_MAX_LENGTH = 1000; // chars
>  const BREAKPOINT_CONDITIONAL_POPUP_POSITION = "after_start";
>  const BREAKPOINT_CONDITIONAL_POPUP_OFFSET = 50; // px
> +const WATCH_EXPRESSIONS_EVALUATION_SEPARATOR = " →";

this should probably be localizeable string.

::: browser/devtools/debugger/test/browser_dbg_bug727429_watch-expressions-02.js
@@ +61,5 @@
> +              test6(function() {
> +                test7(function() {
> +                  test8(function() {
> +                    test9(function() {
> +                      finishTest();

top of the pyramid!

::: browser/devtools/shared/VariablesView.jsm
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  "use strict";
>  
>  const LAZY_EMPTY_DELAY = 150; // ms
> +const SEPARATOR_LABEL = ":";

should probably be localizeable.

::: browser/themes/gnomestripe/devtools/debugger.css
@@ +219,5 @@
> +  background: url("chrome://browser/skin/devtools/commandline.png") 0px 4px no-repeat;
> +}
> +
> +.dbg-expression-input {
> +  font: 9pt monospace;

we don't use pts for font sizes in our code.

I'd recommend using
font-family: monospace;
font-size: inherit; /* inherit browser's default monospace font size */

to be consistent with what we're doing in orion.css.

Comment 19

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

(and I should've known there was a successful try run already)

Comment 20

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

Filed bug 812811.

Comment 21

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

And also bug 812814.

Comment 22

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

Addressed comments and landed:
https://hg.mozilla.org/integration/fx-team/rev/34a9d08a82c

Comment 23

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

The separator label should preserve the leading whitespace character (followup):
https://hg.mozilla.org/integration/fx-team/rev/8a4f312b964b

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/34a9d08a82c4
https://hg.mozilla.org/mozilla-central/rev/8a4f312b964b