Bug 101929 – Web Inspector: Sources: Scope Variables: add "Add to watch" menu item.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED INVALID

Problem: When user looks at Scope Variables and finds one he is interested in, there would be easy way to add this variable to watches.
Solution: Add "Add to watch" context menu item to Scope Variables items.

Created attachment 173624 [details]
Patch

Created attachment 173625 [details]
Patch

Created attachment 173630 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 173630 [details]
Patch

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

> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:175
> +    _onContextMenu: function(event) {

_onContextMenu -> _onNameContextMenu ?

> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:193
> +        for (var i = 1; i < path.length; ++i) {

There may also be local variables like [[boundThis]] and <exception> which we shouldn't allow to add to watches.

[Yury Semikhatsky]

(In reply to comment #4)
> > Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:193
> > +        for (var i = 1; i < path.length; ++i) {
> 
> There may also be local variables like [[boundThis]] and <exception> which we shouldn't allow to add to watches.

Sorry, the patch adding [[boundThis]] has not landed yet. It is https://bugs.webkit.org/show_bug.cgi?id=100021

[PhistucK]

Eustas seems not to work on this anymore. May I take this?

Yury, do you have any solution in mind for the internal properties issue?

I hacked something up - it works, but it looks pretty hacky, but maybe this is the right way?
ObjectPropertiesSection.js:486 -
                 internalProperties[i].parentObject = value;
+                internalProperties[i].internal = true;
                 treeElement.appendChild(new treeElement.treeOutline.section.treeElementConstructor(internalProperties[i]));

ScopeChainSidebarPane.js -
Prepending to populateContextMenu (this patch adds it) -
        if (this.property.internal)
            return;

And when creating the exception object -
                             var exceptionObject = /** @type {RuntimeAgent.RemoteObject} */ (exception);
+                            var exceptionRemoteObject = new WebInspector.RemoteObjectProperty("<exception>", + WebInspector.RemoteObject.fromPayload(exceptionObject));
+                            exceptionRemoteObject.internal = true;
+                            extraProperties.push(exceptionRemoteObject);
-                            extraProperties.push(new WebInspector.RemoteObjectProperty("<exception>", + WebInspector.RemoteObject.fromPayload(exceptionObject)));

I did not want to distinguish <exception> (or any other) by name, because any variable/property can have any name.

[Eugene Klyuchnikov]

(In reply to comment #6)
> Eustas seems not to work on this anymore. May I take this?
> 
> Yury, do you have any solution in mind for the internal properties issue?

Of course you can take this.

Please make sure that items inside long arrays work well (in scope tree they could be wrapped to "groups").

It could be better is you attach patch so we could see the full picture =)

[PhistucK]

Oh, right. I forgot about this feature... :)
I might work on it later today.
I am adding what I currently have (I took your patch and just added the final touches to it) for now.

By the way, what should I add in the ChangeLog for this kind of situations (someone writes a patch, abandons/leaves it and I take their code and add to it)? How should that be handled? I would not want to take any credit (for anything, but especially for code I did not write).

[PhistucK]

Created attachment 194806 [details]
Add to watch for scope variables

Based on the work of Eustas.
Note that I have not checked whether property groups work correctly yet.

[PhistucK]

Oops. *Eustas = Eugene. Sorry!

[Eugene Klyuchnikov]

(In reply to comment #8)
> Oh, right. I forgot about this feature... :)
> I might work on it later today.
> I am adding what I currently have (I took your patch and just added the final touches to it) for now.
> 
> By the way, what should I add in the ChangeLog for this kind of situations (someone writes a patch, abandons/leaves it and I take their code and add to it)? How should that be handled? I would not want to take any credit (for anything, but especially for code I did not write).

Usually Changelog entry is autogenerated.

You should take a look at http://trac.webkit.org/wiki/UsingGitWithWebKit

[Yury Semikhatsky]

Comment on attachment 194806 [details]
Add to watch for scope variables

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

> Source/WebCore/inspector/front-end/ObjectPropertiesSection.js:487
> +                internalProperties[i].internal = true;

I'm fine with having 'internal' marker on properties but I'd like it to be determined at the construction time of the property. For this particular place I think 'internal' property should be set in RemoteObject.prototype._getProperties - in the handler of the protocol response where we already have separate lists of properties and internalProperties.

[PhistucK]

@Eugene Klyuchnikov -
I am using SVN and I only checked out the ChangeLog and the inspector/front-end directory, so nothing will be automatically generated.
What is the proper procedure for taking the code of someone else? do I add a "Patch by"? a "Patch based on work by"? do I not mention the original author at all?

@Yury Semikhatsky -
I agree. Done.

[PhistucK]

Created attachment 194879 [details]
Add to watch for scope variables, including support for getters and setters.

The property.internal = true was moved to RemoteObject and I added handling for getters and setters.

[Eugene Klyuchnikov]

> I am using SVN and I only checked out the ChangeLog and the inspector/front-end directory, so nothing will be automatically generated.

Hmm. Strange. Usually Changelog entry is generated when I upload patch first time...

> What is the proper procedure for taking the code of someone else? do I add a "Patch by"? a "Patch based on work by"? do I not mention the original author at all?

I'm not sure. I think no special mentions required until other is said =)

[Eugene Klyuchnikov]

Comment on attachment 194879 [details]
Add to watch for scope variables, including support for getters and setters.

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

> Source/WebCore/inspector/front-end/RemoteObject.js:203
> +            var result = [], property, i, remoteObject;

IMHO declarations should go on other line than declaration and assignment.
It is not said about this case in style guide, but it will be easier to read.

> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:147
> +        this.nameElement.addEventListener("contextmenu", this._onContextMenu.bind(this), false);

As Yuri suggested earlier it could be more clear with "_onNameContextMenu" than with "_onContextMenu"

> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:182
> +        var path = [], property, name;

Split declarations from declaration and assignment (see above).

[Build Bot]

Comment on attachment 194879 [details]
Add to watch for scope variables, including support for getters and setters.

Attachment 194879 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17255489

New failing tests:
inspector/runtime/runtime-getProperties.html
fast/repaint/japanese-rl-selection-repaint-in-regions.html

[Build Bot]

Created attachment 195010 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[WebKit Review Bot]

Comment on attachment 194879 [details]
Add to watch for scope variables, including support for getters and setters.

Attachment 194879 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17216789

New failing tests:
inspector/runtime/runtime-getProperties.html

[WebKit Review Bot]

Created attachment 195015 [details]
Archive of layout-test-results from gce-cr-linux-04 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[PhistucK]

Comment on attachment 194879 [details]
Add to watch for scope variables, including support for getters and setters.

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

>> Source/WebCore/inspector/front-end/RemoteObject.js:203
>> +            var result = [], property, i, remoteObject;
> 
> IMHO declarations should go on other line than declaration and assignment.
> It is not said about this case in style guide, but it will be easier to read.

Done.

>> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:147
>> +        this.nameElement.addEventListener("contextmenu", this._onContextMenu.bind(this), false);
> 
> As Yuri suggested earlier it could be more clear with "_onNameContextMenu" than with "_onContextMenu"

Missed that, thank you. Done.

>> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:182
>> +        var path = [], property, name;
> 
> Split declarations from declaration and assignment (see above).

Done.

[PhistucK]

Created attachment 195068 [details]
Add to watch for scope variables, including some length caching, a layout test fix and ChangeLog entries

[Build Bot]

Comment on attachment 195068 [details]
Add to watch for scope variables, including some length caching, a layout test fix and ChangeLog entries

Attachment 195068 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17332025

[PhistucK]

Created attachment 195852 [details]
Add to watch for scope variables - as of r147277

Any ObjectPropertiesSection.js changes were dropped since they were only nits (more performant, but, still, unrelated).

Can anyone look at this patch?
Thank you!

[Eugene Klyuchnikov]

Comment on attachment 195852 [details]
Add to watch for scope variables - as of r147277

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

> Source/WebCore/inspector/front-end/RemoteObject.js:222
> +                    if (property.get || property.set) {

I'd rather use else-if to extra depth level.

> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:30
> + *

remove empty line

> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:178
> +    _pane: function() {

Please add new line before "{"

> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:191
> +    _onNameContextMenu: function(event) {

Ditto.

> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:217
> +        var characterRegExp = /^[_a-z][_a-z0-9]*$/i,

Please use 3 var statements instead of "," operator.
May be use "const" statement?

[PhistucK]

Comment on attachment 195852 [details]
Add to watch for scope variables - as of r147277

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

Please, see my replies and my two new comments.

>> Source/WebCore/inspector/front-end/RemoteObject.js:222
>> +                    if (property.get || property.set) {
> 
> I'd rather use else-if to extra depth level.

Where? how?
if (property.get)
else if (property.set)?
Both of them reside on the same original RemoteObject, if I understand correctly, so that would be wrong...

Perhaps I am missing something?

>> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:30
>> + *
> 
> remove empty line

It seems appropriate to separate the class/prototype chain from the variable, it makes it more readable.
But, done.

> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:167
> +        this._pane()._expandedProperties[this.propertyIdentifier] = true;

By the way, does that seem fine to you?
The original code did that, too, but, still - we are accessing a private member of an object from another object.
Perhaps make expandedProperties public?
(I assume _name means it is a private member)

> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:172
> +        delete this._pane()._expandedProperties[this.propertyIdentifier];

Same here.

>> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:178
>> +    _pane: function() {
> 
> Please add new line before "{"

Done.

>> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:191
>> +    _onNameContextMenu: function(event) {
> 
> Ditto.

Done.

>> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:217
>> +        var characterRegExp = /^[_a-z][_a-z0-9]*$/i,
> 
> Please use 3 var statements instead of "," operator.
> May be use "const" statement?

Used 3 var statements.
V8 (at least) is not optimizing const at the moment, I would rather not use it for now.

[Eugene Klyuchnikov]

Comment on attachment 195852 [details]
Add to watch for scope variables - as of r147277

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

>>> Source/WebCore/inspector/front-end/RemoteObject.js:222
>>> +                    if (property.get || property.set) {
>> 
>> I'd rather use else-if to extra depth level.
> 
> Where? how?
> if (property.get)
> else if (property.set)?
> Both of them reside on the same original RemoteObject, if I understand correctly, so that would be wrong...
> 
> Perhaps I am missing something?

May be:

if (property.get)
if (property.set)
if (!property.get && !property.set)

>>> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:30
>>> + *
>> 
>> remove empty line
> 
> It seems appropriate to separate the class/prototype chain from the variable, it makes it more readable.
> But, done.

We do not place comments to JSDocs, only taglets.
Taglets should be alphabetically sorted (by tag name).
So, readability is achieved the other way =)

>> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:167
>> +        this._pane()._expandedProperties[this.propertyIdentifier] = true;
> 
> By the way, does that seem fine to you?
> The original code did that, too, but, still - we are accessing a private member of an object from another object.
> Perhaps make expandedProperties public?
> (I assume _name means it is a private member)

"_name" doesn't mean private in common sense. It means that this name shouldn't be accessed from outside of file.

So when one renames or somehow else refactors member, then he knows if it is enough to look through the file, or he must search through all scripts.

One exclusion is: some tests refer to "private" members. That is not very good,... but test will care about themselves =)

Extraction of _pane makes it more type-clear.

>>> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:217
>>> +        var characterRegExp = /^[_a-z][_a-z0-9]*$/i,
>> 
>> Please use 3 var statements instead of "," operator.
>> May be use "const" statement?
> 
> Used 3 var statements.
> V8 (at least) is not optimizing const at the moment, I would rather not use it for now.

I know. Even worse - in some cases const could be slower than var.
But this is about readability: we show that those values shouldn't be changed.

[PhistucK]

Comment on attachment 195852 [details]
Add to watch for scope variables - as of r147277

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

One last comment needs addressing (the const issue) and I will upload a new patch.

>>>> Source/WebCore/inspector/front-end/RemoteObject.js:222
>>>> +                    if (property.get || property.set) {
>>> 
>>> I'd rather use else-if to extra depth level.
>> 
>> Where? how?
>> if (property.get)
>> else if (property.set)?
>> Both of them reside on the same original RemoteObject, if I understand correctly, so that would be wrong...
>> 
>> Perhaps I am missing something?
> 
> May be:
> 
> if (property.get)
> if (property.set)
> if (!property.get && !property.set)

Done.

>>>> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:30
>>>> + *
>>> 
>>> remove empty line
>> 
>> It seems appropriate to separate the class/prototype chain from the variable, it makes it more readable.
>> But, done.
> 
> We do not place comments to JSDocs, only taglets.
> Taglets should be alphabetically sorted (by tag name).
> So, readability is achieved the other way =)

Done.

>>> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:167
>>> +        this._pane()._expandedProperties[this.propertyIdentifier] = true;
>> 
>> By the way, does that seem fine to you?
>> The original code did that, too, but, still - we are accessing a private member of an object from another object.
>> Perhaps make expandedProperties public?
>> (I assume _name means it is a private member)
> 
> "_name" doesn't mean private in common sense. It means that this name shouldn't be accessed from outside of file.
> 
> So when one renames or somehow else refactors member, then he knows if it is enough to look through the file, or he must search through all scripts.
> 
> One exclusion is: some tests refer to "private" members. That is not very good,... but test will care about themselves =)
> 
> Extraction of _pane makes it more type-clear.

Got it.

>>>> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:217
>>>> +        var characterRegExp = /^[_a-z][_a-z0-9]*$/i,
>>> 
>>> Please use 3 var statements instead of "," operator.
>>> May be use "const" statement?
>> 
>> Used 3 var statements.
>> V8 (at least) is not optimizing const at the moment, I would rather not use it for now.
> 
> I know. Even worse - in some cases const could be slower than var.
> But this is about readability: we show that those values shouldn't be changed.

How about just adding a JSDoc taglet instead?
/** @const */

[Eugene Klyuchnikov]

Comment on attachment 195852 [details]
Add to watch for scope variables - as of r147277

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

>>>>> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:217
>>>>> +        var characterRegExp = /^[_a-z][_a-z0-9]*$/i,
>>>> 
>>>> Please use 3 var statements instead of "," operator.
>>>> May be use "const" statement?
>>> 
>>> Used 3 var statements.
>>> V8 (at least) is not optimizing const at the moment, I would rather not use it for now.
>> 
>> I know. Even worse - in some cases const could be slower than var.
>> But this is about readability: we show that those values shouldn't be changed.
> 
> How about just adding a JSDoc taglet instead?
> /** @const */

That seems OK to me, but reviewers doesn't like it =(
So it is still better to use const or var without annotation.

[PhistucK]

Comment on attachment 195852 [details]
Add to watch for scope variables - as of r147277

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

>>>>>> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:217
>>>>>> +        var characterRegExp = /^[_a-z][_a-z0-9]*$/i,
>>>>> 
>>>>> Please use 3 var statements instead of "," operator.
>>>>> May be use "const" statement?
>>>> 
>>>> Used 3 var statements.
>>>> V8 (at least) is not optimizing const at the moment, I would rather not use it for now.
>>> 
>>> I know. Even worse - in some cases const could be slower than var.
>>> But this is about readability: we show that those values shouldn't be changed.
>> 
>> How about just adding a JSDoc taglet instead?
>> /** @const */
> 
> That seems OK to me, but reviewers doesn't like it =(
> So it is still better to use const or var without annotation.

I added const per your original request, but I am really uncomfortable with this. This is non standard JavaScript (as of now, anyway).
Done.

[PhistucK]

Created attachment 196173 [details]
Add to watch for scope variables - as of r147441

Addressed review comments.

[Pavel Feldman]

Comment on attachment 196173 [details]
Add to watch for scope variables - as of r147441

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

Overall, please try to only change things that are necessary for the change in behavior you are implementing. Also, you should not rely upon tree element structure from outside that tree.

> Source/WebCore/inspector/front-end/RemoteObject.js:217
> +            var i, length, property, remoteObject;

We don't declare things ahead, please inline these.

> Source/WebCore/inspector/front-end/RemoteObject.js:219
> +            if (properties) {

Why has this changed? Please revert.

> Source/WebCore/inspector/front-end/RemoteObject.js:222
> +                    if (!property.get && !property.set)

Ditto.

> Source/WebCore/inspector/front-end/RemoteObject.js:226
> +                        remoteObject.getter = true;

It is better to define ._isGetter and have isGetter() function for testing it.

> Source/WebCore/inspector/front-end/RemoteObject.js:240
> +                for (i = 0, length = internalProperties.length; i < length; i++) {

These changes don't really make things faster, but rather make them less readable.

> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:202
> +        for (var node = this; node; node = node.parent) {

You should not rely upon node structure managed elsewhere.

> Source/WebCore/inspector/front-end/ScopeChainSidebarPane.js:211
> +                        name = name.substring(4);

You should not rely upon formatting that is managed elsewhere.

[PhistucK]

Just for the sake of completeness, this was done about five years later -
https://chromium.googlesource.com/chromium/src/+/3fae2c4abeb22b18d486a29f74cbafcce5668b8c