[webkit-reviews] review denied: [Bug 100664] Web Inspector: adds isOwnProperty to remote protocol : [Attachment 171223] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Andrey Lushnikov
<lushnikov at google.com>'s request for review:
Bug 100664: Web Inspector: adds isOwnProperty to remote protocol
https://bugs.webkit.org/show_bug.cgi?id=100664

Attachment 171223: Patch
https://bugs.webkit.org/attachment.cgi?id=171223&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=171223&action=review


> Source/WebCore/ChangeLog:3
> +	   [Dev Tools] Slight enhancement of remote protocol

drop these two lines

> Source/WebCore/ChangeLog:12
> +	   * inspector/InjectedScriptSource.js:

Please explain what has changed and why.

> Source/WebCore/inspector/InjectedScriptSource.js:355
> +

please revert

> Source/WebCore/inspector/InjectedScriptSource.js:369
> +			       descriptors.push({ name: name, value:
object[name], writable: false, configurable: false, enumerable: false,
isOwnProperty: firstIteration});

isOwnProperty: o === object,

also, since this property is optional, you should only add it when it is true.
I.e. create an object and optionally add isOwnProperty there.

> LayoutTests/ChangeLog:3
> +	   [Dev Tools] Slight enhancement of remote protocol

drop these 2 lines

>
LayoutTests/inspector/runtime/runtime-getProperties-isOwnProperty-expected.txt:
3
> +property.name=bar isOwnProperty=true

==, surround with " "
property.name == bar

> LayoutTests/inspector/runtime/runtime-getProperties-isOwnProperty.html:8
> +window.b = {bar: "cde"};

{ bar: "cde" };

> LayoutTests/inspector/runtime/runtime-getProperties-isOwnProperty.html:14
> +    var fail = function(msg)

function fail(message)
{
    ....
}

> LayoutTests/inspector/runtime/runtime-getProperties-isOwnProperty.html:18
> +	   throw new Error("Error: " + msg);

drop this

> LayoutTests/inspector/runtime/runtime-getProperties-isOwnProperty.html:25
> +	   if (error) {

Do not use {} around single line blocks

> LayoutTests/inspector/runtime/runtime-getProperties-isOwnProperty.html:26
> +	       fail("Error on getting window.b property");

return fail() ...

... also, you don't need this branch. Let the test time out in case it fails.

> LayoutTests/inspector/runtime/runtime-getProperties-isOwnProperty.html:28
> +	       RuntimeAgent.getProperties(result.objectId, false, step2);

/* isOwnProperty */ false

> LayoutTests/inspector/runtime/runtime-getProperties-isOwnProperty.html:32
> +    function step2(error, properties) {

{ on the next line

> LayoutTests/inspector/runtime/runtime-getProperties-isOwnProperty.html:33
> +	   if (error) {

ditto

> LayoutTests/inspector/runtime/runtime-getProperties-isOwnProperty.html:36
> +	   if (!properties) {

ditto

> LayoutTests/inspector/runtime/runtime-getProperties-isOwnProperty.html:39
> +	   for (var i = 0; i < properties.length; i++) {

++i