[webkit-reviews] review denied: [Bug 101929] Web Inspector: Sources: Scope Variables: add "Add to watch" menu item. : [Attachment 196173] Add to watch for scope variables - as of r147441

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied PhistucK
<phistuck at chromium.org>'s request for review:
Bug 101929: Web Inspector: Sources: Scope Variables: add "Add to watch" menu
item.
https://bugs.webkit.org/show_bug.cgi?id=101929

Attachment 196173: Add to watch for scope variables - as of r147441
https://bugs.webkit.org/attachment.cgi?id=196173&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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.