812814 - Add a way to edit or remove watch expressions while the debugger is paused

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

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

When paused, there's no way to remove watch expressions, because they're part of the variables view. We could either use a customizable context menu for variables/properties, or the ability to show the [x] button on certain elements.

Comment 1

[Luke Wagner [:luke]]

Hi, I just ran into this with a webdev.  In our case, we were using watched expressions to evaluate arbitrary expressions which meant we were accumulating a bunch of watched expressions that we couldn't clear out.  (Is there a better way to interactively evaluate JS?)

Comment 2

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

Currently, this is the only supported way of evaluating JS when execution is paused. We plan to make the web console and scratchpad usable in such cases, too, but we are not there, yet. We should have this bug fixed in the next few weeks.

Comment 3

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

(In reply to Luke Wagner [:luke] from comment #1)
> Hi, I just ran into this with a webdev.  In our case, we were using watched
> expressions to evaluate arbitrary expressions which meant we were
> accumulating a bunch of watched expressions that we couldn't clear out.  (Is
> there a better way to interactively evaluate JS?)

I wonder, would a "Persist watch expressions" pref (defaults to true, exposed as an option in the gear menu) be helpful? I imagine there are plenty of people that use watch expressions (not the webconsole) solely for evaluating arbitrary js. Of course, when we'll have other means to evaluate code in the context of the current frame, then that pref may be redundant, but I'm thinking about *habits* here.

Comment 4

[Luke Wagner [:luke]]

That's a good question.  In Visual Studio's debugger, there is the "immediate" window (whose purpose would be filled by the web console, iiuc) which is separate from the watch window.  I have also seen devs who didn't know about the immediate window constantly adding watch expressionst.  Perhaps then a "fix" would be to make it easier to discover that you can interactively evaluate JS from the debugger UI.  I'm not a UI guy, but one idea that comes to mind is that, to the right of the "Add watch expression" text (in the same box) you have a link "Evaluate JS" that, when clicked, just opens up the web console.

Comment 5

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

Attachment: v1

Works, needs a test.

Comment 6

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

Attachment: v2

Fixed tests, added tooltips, polished etc.

Comment 7

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

Attachment: v3

Added test, going through try.

Comment 8

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

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

Comment 9

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

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

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

Very nice! My only gripe is that editing the result of a watch expression is allowed, and even though the edit will be reverted after pressing ENTER, it can be confusing to a developer.

Also, it would be nice if we could get a version of this patch without the string changes for Aurora.

::: browser/devtools/debugger/debugger-panes.js
@@ +944,5 @@
>   */
>  function WatchExpressionsView() {
>    dumpn("WatchExpressionsView was instantiated");
>    MenuContainer.call(this);
> +  this.switchExpression = this.switchExpression.bind(this);

Nit: I'd go for changeExpression myself.

::: browser/devtools/debugger/test/browser_dbg_propertyview-edit-watch.js
@@ +14,5 @@
> +var gVars = null;
> +
> +requestLongerTimeout(2);
> +
> +function test() {

Can you add one line describing this test please?

::: browser/devtools/shared/VariablesView.jsm
@@ +640,5 @@
>    set twisty(aFlag) aFlag ? this.showArrow() : this.hideArrow(),
>  
>    /**
> +   * Specifies if editing variable or property names is allowed.
> +   * This flag applies non-recursively on the current scope.

s/on/to/g

Comment 10

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

(In reply to Panos Astithas [:past] from comment #9)
> Comment on attachment 684940 [details] [diff] [review]
> v3
> 
> Review of attachment 684940 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Very nice! My only gripe is that editing the result of a watch expression is
> allowed, and even though the edit will be reverted after pressing ENTER, it
> can be confusing to a developer.
> 

Well, maybe. The changes aren't always reverted. For example, go to http://htmlpad.org/debugger/ and add the following watch expressions: "arguments[0]" and "aArg". Click me, and when on the first debugger statement, change the evaluation value of arguments[0] to be "42" or whatever. This change persists in the whole watch expressions scope, and in all the other scopes.

So my hypothesis is that there may be some remote use cases in which editing the watch expressions evaluation result may be useful (albeit confusing in others). If not, then my backup argument is that copying the result isn't possible without the textbox :) What do you think?

> Also, it would be nice if we could get a version of this patch without the
> string changes for Aurora.
> 

I was just about to suggest that.

Comment 11

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

(In reply to Victor Porof [:vp] from comment #10)
> (In reply to Panos Astithas [:past] from comment #9)
> > Very nice! My only gripe is that editing the result of a watch expression is
> > allowed, and even though the edit will be reverted after pressing ENTER, it
> > can be confusing to a developer.
> > 
> 
> Well, maybe. The changes aren't always reverted. For example, go to
> http://htmlpad.org/debugger/ and add the following watch expressions:
> "arguments[0]" and "aArg". Click me, and when on the first debugger
> statement, change the evaluation value of arguments[0] to be "42" or
> whatever. This change persists in the whole watch expressions scope, and in
> all the other scopes.
> 
> So my hypothesis is that there may be some remote use cases in which editing
> the watch expressions evaluation result may be useful (albeit confusing in
> others). If not, then my backup argument is that copying the result isn't
> possible without the textbox :) What do you think?

Good points, agreed.

Comment 12

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

Attachment: v3.1 [in-fx-team]

Addressed comments.

Comment 13

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

Attachment: v3.1 [aurora:no-string-changes]

Comment 14

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

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

Comment 15

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

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

Comment 16

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

Comment on attachment 685164 [details] [diff] [review]
v3.1 [aurora:no-string-changes]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Watch expressions can't be modified or removed while the debugger is paused. It sucks :)
User impact if declined: Almost none, this patch affects a developer tool, however it's a brand new feature that is likely to get some attention.
Testing completed (on m-c, etc.): fx-team and m-c
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: None with this patch

Comment 17

[Alex Keybl [:akeybl]]

Comment on attachment 685164 [details] [diff] [review]
v3.1 [aurora:no-string-changes]

[Triage Comment]
Low risk fix for a new dev feature. Approving for Aurora 19.

Comment 18

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/c515d6ec8985