Bug 147904 – Web Inspector: Watch Expressions

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* SUMMARY
Add support for Watch Expressions.

* NOTES
- Chrome and Firefox have some form of Watch Expressions
- Examples of users using watch expressions:
  - <https://remysharp.com/2013/11/27/using-watches-in-my-devtools-workflow>
  - <http://albertlee.azurewebsites.net/using-watch-tools-in-chrome-dev-tools-to-improve-your-debugging/>

[Joseph Pecoraro]

<rdar://problem/11545484>

[Joseph Pecoraro]

Created attachment 258763 [details]
[IMAGE] Watch Expressions Section with Expressions

[Joseph Pecoraro]

Created attachment 258764 [details]
[IMAGE] Watch Expressions Section without Expressions

[Joseph Pecoraro]

Created attachment 258765 [details]
[IMAGE] Adding a Watch Expression

[Joseph Pecoraro]

Created attachment 258766 [details]
[IMAGE] Remove an Individual Watch Expression

[Joseph Pecoraro]

Created attachment 258768 [details]
[PATCH] Proposed Fix

First draft. This keeps the sidebar name "Scope Chain" that may need to change. Firefox/Chrome call their sidebar "Variables".

[Joseph Pecoraro]

Comment on attachment 258768 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=258768&action=review

> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:41
> +        refreshAllWatchExpressionButton.role = "button";

This is not the correct way to set the attribute. I need to use elem.setAttribute("role", "button")

[Devin Rousso]

Comment on attachment 258768 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=258768&action=review

> Source/WebInspectorUI/UserInterface/Views/Popover.js:92
> +    update(shouldAnimate=true)

I personally think that for default values we should put spaces around the equals sign.  So "shouldAnimate = true, ..."

> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:168
> +        for (let i = 0; i < scopeChain.length; ++i) {

NIT: I think you could use "for (let scope of scopeChain)"

> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:326
> +        editorElement.classList.add(WebInspector.SyntaxHighlightedStyleClassName);

NIT: you can make these two classList.add on one line.

[Timothy Hatcher]

Comment on attachment 258768 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=258768&action=review

>> Source/WebInspectorUI/UserInterface/Views/Popover.js:92
>> +    update(shouldAnimate=true)
> 
> I personally think that for default values we should put spaces around the equals sign.  So "shouldAnimate = true, ..."

I agree.

> Source/WebInspectorUI/UserInterface/Views/Popover.js:154
> +            if (!this._element.contains(event.target) && !event.target.enclosingNodeOrSelfWithClass(WebInspector.Popover.IgnoreClassName))

This IgnoreClassName is vague. IgnoreAutoDismissPopoverClassName? Maybe a Symbol and a DOM ancestor walk would be better?

> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.css:64
> +    width: 198px; /* NOTE: Fixed value, manually tuned to .watch-expression-editor width */

calc(100% - 2px)?

> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.css:69
> +    width: 198px; /* NOTE: Fixed value, manually tuned to .watch-expression-editor width */

Ditto.

>> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:41
>> +        refreshAllWatchExpressionButton.role = "button";
> 
> This is not the correct way to set the attribute. I need to use elem.setAttribute("role", "button")

Sadly.

> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:90
> -        return !!this.callFrame;
> +        return true;

So scope chain is always available now?

> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:335
> +        this._codeMirror = CodeMirror(editorElement, {
> +            lineWrapping: true,
> +            mode: "text/javascript",
> +            indentWithTabs: true,
> +            indentUnit: 4,
> +            matchBrackets: true,
> +            value: "",
> +        });

We should make this a helper in CodeMirrorAdditions sometime, so we don't keep copying the config all over.

> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:408
> +        contextMenu.appendItem(WebInspector.UIString("Remove Watch Expression"), function() {

Kind of a bummer this is the only way to remove a watch expression. Maybe we need an inline icon too? Or row section for expressions so the delete key would work?

[Blaze Burg]

Comment on attachment 258768 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=258768&action=review

> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:72
> +    static fakeRemoteObject()

Why call it 'fake', if it is only used for keeping watch expressions updated? I would call this the watchRemoteObject or something like that.

>>> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:41
>>> +        refreshAllWatchExpressionButton.role = "button";
>> 
>> This is not the correct way to set the attribute. I need to use elem.setAttribute("role", "button")
> 
> Sadly.

I dunno, sometimes I appreciate setAttribute, because it's clearly modifying a DOM element not a setter on an inspector object.

> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:245
> +            return null;

No good, we can't return null on one exit and a Promise on another. Maybe Promise.resolve(), since there's nothing to do when watches are empty?

> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:249
> +        WebInspector.runtimeManager.removeEventListener(WebInspector.RuntimeManager.Event.DidEvaluate, this.needsRefresh, this);

This seems a little weird, like it might race or just disable updates if there happens to be an error between now and when all promises resolve. Can we take a different approach, like selectively ignoring evaluations from the WatchExpressionsObjectGroup since we manually update in that case when everything's evaluated? It seems less error-prone.

> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:278
> +            // Restore updates for evaluations.

It might be wise to add a catch block here where we also add back the event listener. Otherwise, if any promise rejects then updates will not work.

> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:298
> +        watchExpressions.remove(expression, true);

Because 'Array.remove' uses string strict equality (i.e, looking at characters rather than comparing pointers), this will remove the wrong expression in this test case:


1. foo
2. bar
3. foo
4. baz

If the user deletes 3, then 1 will get deleted instead. We should pass the index instead of the expression.

>> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:408
>> +        contextMenu.appendItem(WebInspector.UIString("Remove Watch Expression"), function() {
> 
> Kind of a bummer this is the only way to remove a watch expression. Maybe we need an inline icon too? Or row section for expressions so the delete key would work?

I would prefer selection + delete key to adding inline icons. There isn't much room, and it will unalign results with other object trees.

[Devin Rousso]

(In reply to comment #10)
> >> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:408
> >> +        contextMenu.appendItem(WebInspector.UIString("Remove Watch Expression"), function() {
> > 
> > Kind of a bummer this is the only way to remove a watch expression. Maybe we need an inline icon too? Or row section for expressions so the delete key would work?
> 
> I would prefer selection + delete key to adding inline icons. There isn't
> much room, and it will unalign results with other object trees.

I think that putting this in a Context Menu might be the way to go, since regularly clicking on a watched object will expand it (similar to object previews in the console).  Also, having some sort of highlighting system may break from the current style of the debugger sidebar since nothing else has a highlight on select.

[Joseph Pecoraro]

Comment on attachment 258768 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=258768&action=review

asdfsadfadsfads

>> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:72
>> +    static fakeRemoteObject()
> 
> Why call it 'fake', if it is only used for keeping watch expressions updated? I would call this the watchRemoteObject or something like that.

Anyone that wants to create a fake remote object needs only use this to get the benefits of all of the RemoteObject behavior below (isFakeObject behavior). There is only one occurrence right now, maybe in ScopeChainDetailsSidebar I should call it the "watchRemoteObject" instead of fakeRemoteObject, but I think a generic name applies here.

>> Source/WebInspectorUI/UserInterface/Views/Popover.js:154
>> +            if (!this._element.contains(event.target) && !event.target.enclosingNodeOrSelfWithClass(WebInspector.Popover.IgnoreClassName))
> 
> This IgnoreClassName is vague. IgnoreAutoDismissPopoverClassName? Maybe a Symbol and a DOM ancestor walk would be better?

I think a class name works well because you can see it in the DOM Tree.

>> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.css:64
>> +    width: 198px; /* NOTE: Fixed value, manually tuned to .watch-expression-editor width */
> 
> calc(100% - 2px)?

Good idea, I'll try it!

>> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:90
>> +        return true;
> 
> So scope chain is always available now?

Yes, because the Watch Expressions section is always available.

>> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:245
>> +            return null;
> 
> No good, we can't return null on one exit and a Promise on another. Maybe Promise.resolve(), since there's nothing to do when watches are empty?

In practice this works fine with Promise.all:

    Promise.all([null, 1, Promise.resolve(null), Promise.resolve(1)]).then(function(results) { console.log(results); });
    // => [null, 1, null, 1]

>> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:249
>> +        WebInspector.runtimeManager.removeEventListener(WebInspector.RuntimeManager.Event.DidEvaluate, this.needsRefresh, this);
> 
> This seems a little weird, like it might race or just disable updates if there happens to be an error between now and when all promises resolve. Can we take a different approach, like selectively ignoring evaluations from the WatchExpressionsObjectGroup since we manually update in that case when everything's evaluated? It seems less error-prone.

Kk

>> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:278
>> +            // Restore updates for evaluations.
> 
> It might be wise to add a catch block here where we also add back the event listener. Otherwise, if any promise rejects then updates will not work.

Yeah, that seems reasonable.

>> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:298
>> +        watchExpressions.remove(expression, true);
> 
> Because 'Array.remove' uses string strict equality (i.e, looking at characters rather than comparing pointers), this will remove the wrong expression in this test case:
> 
> 
> 1. foo
> 2. bar
> 3. foo
> 4. baz
> 
> If the user deletes 3, then 1 will get deleted instead. We should pass the index instead of the expression.

I started with that, and then went the simpler route. Though you are right, this doesn't matter in practice. The WatchExpressions are sorted in the object tree. So these would appear as:

    1. bar
    2. bar
    3. foo
    4. foo

To the user, deleting one of the "foo"s doesn't matter which. We aren't necessarily guaranteeing any order of execution or when they get evaluated, so if a watch expression has sideaffects users are most likely going to run into issues anyways.

>>> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:408
>>> +        contextMenu.appendItem(WebInspector.UIString("Remove Watch Expression"), function() {
>> 
>> Kind of a bummer this is the only way to remove a watch expression. Maybe we need an inline icon too? Or row section for expressions so the delete key would work?
> 
> I would prefer selection + delete key to adding inline icons. There isn't much room, and it will unalign results with other object trees.

There is the Trash icon to delete all, so this is not the only way.

[Timothy Hatcher]

Fair enough on seeing the class name in the DOM. The class name still doesn't say what the popover will ignore. It sounds like the popover will be blocked from showing instead of ignoring dismiss.

[Joseph Pecoraro]

(In reply to comment #13)
> Fair enough on seeing the class name in the DOM. The class name still
> doesn't say what the popover will ignore. It sounds like the popover will be
> blocked from showing instead of ignoring dismiss.

I agree, I used your suggestion and re-named it:

    WebInspector.Popover.IgnoreAutoDismissClassName = "popover-ignore-auto-dismiss";

[Joseph Pecoraro]

Created attachment 258907 [details]
[PATCH] Proposed Fix - 2

Addressed review comments. Since enough changed putting up for review again.

[Blaze Burg]

Comment on attachment 258907 [details]
[PATCH] Proposed Fix - 2

View in context: https://bugs.webkit.org/attachment.cgi?id=258907&action=review

Looks ready, except it would be great to get a little test added in, even if in another patch. I set cq- to remind you about the ChangeLog.

> Source/WebInspectorUI/ChangeLog:105
> [phone]  Joseph Pecoraro  <[e-mail]>

Double changelog.

> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:72
> +    static fakeRemoteObject()

I would prefer createFakeRemoteObject() to be clear it's not a singleton. Especially if we could confuse multiple instances. But there is only one call site for now, so.. up to you.

> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:-222
> -        if (!this._objectId || this._isSymbol()) {

It would be good to add a test that covers behavior of fake remote objects. The setup could be similar to remote-object-get-properties.html, except it's mostly testing the WebInspector.RemoteObject API instead of the injected script source.

> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:314
> +        console.assert(!this._isFakeObject());

Shouldn't you add fake objects to the early exit that immediately proceeds this? You can keep the assertion if you like.. but the error should be sent to the callback either way.

> Source/WebInspectorUI/UserInterface/Views/ObjectTreeBaseTreeElement.js:193
> +        var resolvedValue = this.resolvedValue();

let

> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:113
> +            let [watchExpressionsSection, callFrameSections] = sections;

Does this._generateCallFrameSection generate multiple sections? Singular/plural doesn't match here.

> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:118
> +                clearTimeout(timeout);

At first I was like "oh no, this won't work!". But then I tried it out, and there is no TDZ because we would have always set up the timer before calling this. Cool!

[Joseph Pecoraro]

Comment on attachment 258907 [details]
[PATCH] Proposed Fix - 2

View in context: https://bugs.webkit.org/attachment.cgi?id=258907&action=review

>> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:72
>> +    static fakeRemoteObject()
> 
> I would prefer createFakeRemoteObject() to be clear it's not a singleton. Especially if we could confuse multiple instances. But there is only one call site for now, so.. up to you.

Great idea! Done.

>> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:314
>> +        console.assert(!this._isFakeObject());
> 
> Shouldn't you add fake objects to the early exit that immediately proceeds this? You can keep the assertion if you like.. but the error should be sent to the callback either way.

Yep, I don't know why I went with this approach =(.

>> Source/WebInspectorUI/UserInterface/Views/ScopeChainDetailsSidebarPanel.js:113
>> +            let [watchExpressionsSection, callFrameSections] = sections;
> 
> Does this._generateCallFrameSection generate multiple sections? Singular/plural doesn't match here.

It does generate multiple sections. It generates one section per-Scope.

[Joseph Pecoraro]

http://trac.webkit.org/changeset/188407