[webkit-reviews] review granted: [Bug 180945] Web Inspector: add RemoteObject.fetchProperties and some basic tests for RemoteObject API : [Attachment 329701] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 180945: Web Inspector: add RemoteObject.fetchProperties and some basic
tests for RemoteObject API
https://bugs.webkit.org/show_bug.cgi?id=180945

Attachment 329701: Patch

https://bugs.webkit.org/attachment.cgi?id=329701&action=review




--- Comment #2 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 329701
  --> https://bugs.webkit.org/attachment.cgi?id=329701
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:519
> +	       let processResultForCallback = (error, result, wasThrown) => {

Nit: You could just inline this and it might read even better.

> LayoutTests/inspector/model/remote-object-api.html:15
> +    "resolved": Promise.resolve(666),
> +    "rejected": Promise.reject(new Error("I promised problems.")),

Nit: Perhaps a less contentious value.
Or remove these? They don't appear to be used, but do affect the page's output.

> LayoutTests/inspector/model/remote-object-api.html:25
> +	       let object = await
InspectorTest.evaluateInPage("window.testObject");

Nit: Use template strings for code. `window.testObject`, here and below.

> LayoutTests/inspector/model/remote-object-api.html:85
> +	       let object = await
InspectorTest.evaluateInPage("window.testObject");

Instead of doing this in each test, you can do this before
runTestCasesAndFinish. That would reduce the protocol messages almost in half
for this test.

> LayoutTests/inspector/model/remote-object-api.html:186
> +	       InspectorTest.expectEqual(name, "Favorites", `Fetched property
'name' should equal 'Favorites'`);
> +	       InspectorTest.expectEqual(size, 456, `Fetched property 'size'
should equal '456'`);
> +	       InspectorTest.expectThat(data instanceof WI.RemoteObject,
`Fetched property 'data' should be a WI.RemoteObject`);

Nit: End these messages with a period.