947612 - VariablesView `Editable#_onCleanup` is not called when it should be

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P1)

Description

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

Regression after bug 808371.

STR:
1. Open debugger on http://htmlpad.org/debugger/
2. Add a watch expression (ex: "window")
3. Double click the expression in the variables view
4. Press RETURN

The value hides.
https://bugzilla.mozilla.org/show_bug.cgi?id=808371#c46

Comment 1

[Brandon Benvie [:benvie]]

Regression from bug 808371 probably. I'll look into it.

Comment 2

[Brandon Benvie [:benvie]]

Attachment: variables-view-onCleanup.patch

This patch should fix the issue. `_onCleanup` was not being called at the appropriate time.

Comment 4

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

Comment on attachment 8347512 [details] [diff] [review]
variables-view-onCleanup.patch

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

Please add a test.

Comment 5

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

Any progress with the test Benvie? This bug makes it really hard to work with watch expressions in the debugger.

Comment 6

[Brandon Benvie [:benvie]]

Sorry about that! This fell off my radar with all the promise stuff. I'll work on this today.

Comment 7

[Brandon Benvie [:benvie]]

Attachment: variables-view-onCleanup.patch

Adds a test.

Comment 8

[Brandon Benvie [:benvie]]

https://tbpl.mozilla.org/?tree=Try&rev=3c57a630f841

Comment 9

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

Comment on attachment 8361296 [details] [diff] [review]
variables-view-onCleanup.patch

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

r+ with the comments addressed and a green try.

::: browser/devtools/debugger/test/browser_dbg_variables-view-edit-cancel.js
@@ +8,5 @@
> +
> +const TAB_URL = EXAMPLE_URL + "doc_watch-expressions.html";
> +
> +
> +let {console} = Cu.import("resource://gre/modules/devtools/Console.jsm", {});

Remove this if not needed.

@@ +16,5 @@
> +    let win = panel.panelWin;
> +    let vars = win.DebuggerView.Variables;
> +
> +    win.DebuggerView.WatchExpressions.addExpression("this");
> +    executeSoon(() => debuggee.ermahgerd());

Please document the reason for this executeSoon.

Comment 10

[Brandon Benvie [:benvie]]

Attachment: variables-view-onCleanup.patch

Remove leftover debug line and add a comment explaining executeSoon.

Comment 11

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

Comment on attachment 8361308 [details] [diff] [review]
variables-view-onCleanup.patch

Try is all orange :(

Comment 12

[Brandon Benvie [:benvie]]

Attachment: variables-view-onCleanup.patch

EditableNameAndValue no longer has an _onCleanup hook, this fixes it.

Comment 13

[Brandon Benvie [:benvie]]

Comment on attachment 8361827 [details] [diff] [review]
variables-view-onCleanup.patch

Adding back r+ with fix.

Comment 14

[Brandon Benvie [:benvie]]

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

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

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