[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 bugzilla-daemon at webkit.org
Tue Dec 19 16:30:59 PST 2017

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

Attachment 329701: Patch


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

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


> 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

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

> LayoutTests/inspector/model/remote-object-api.html:85
> +	       let object = await

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.

More information about the webkit-reviews mailing list