[webkit-reviews] review granted: [Bug 180831] Web Inspector: InspectorTest.evaluateInPage should unwrap primitive values by default : [Attachment 329388] Proposed Fix

Thu Dec 14 15:10:39 PST 2017

Joseph Pecoraro <joepeck at webkit.org> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 180831: Web Inspector: InspectorTest.evaluateInPage should unwrap primitive
values by default
https://bugs.webkit.org/show_bug.cgi?id=180831

Attachment 329388: Proposed Fix

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

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

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

r=me

> LayoutTests/inspector/console/command-line-api-copy.html:27
> +	       InspectorTest.evaluateInPage("pasteAndReturnString()", (error,
result) => {

Nit: Lets update the source string to be a template string:
`pasteAndReturnString()`.

>
LayoutTests/inspector/unit-tests/test-harness-evaluate-in-page-expected.txt:165
> +PASS: Expected and actual evaluation result type should be equal.
> +PASS: Expected and actual evaluation result subtype should be equal.

These lines are a bit vague, but I guess if it fails it outputs the two types?
I normally like to see the expected type in the output so I don't have to
search the test code for what it was.

> LayoutTests/inspector/unit-tests/test-harness-evaluate-in-page.html:1
> +<!doctype html>

Nice test!

> LayoutTests/inspector/unit-tests/test-harness-evaluate-in-page.html:10
> +	       input: "-42",

Style: I find it clearer when a string is intended to be code, to use template
strings. `-42` makes it just a little bit clearer that this will be evaluated
later on.

> LayoutTests/inspector/unit-tests/test-harness-evaluate-in-page.html:143
> +	       for (let {input, type, subtype, thrown} of complexCases) {
> +		   let result = await InspectorTest.evaluateInPage(input);

Technically this is serial (request, response, request, response, ...) but you
could do this all in parallel (request1, request2, ..., response1, response2,
...) like so:

    let results = await Promise.all(...complexCases.map((input) =>
InspectorTest.evaluateInPage(input))];
    for (let {input, type, subtype, thrown} of results)

But I don't think it matters if the debug bugs aren't timing out.

> LayoutTests/inspector/unit-tests/test-harness-evaluate-in-page.html:146
> +		   InspectorTest.expectThat(result instanceof WI.RemoteObject,
"Returned result should be a WI.RemoteObject.");

Maybe we should really have `expectInstanceOf` because we do this all over the
place.

> LayoutTests/inspector/unit-tests/test-harness-evaluate-in-page.html:150
> +		   InspectorTest.log("\n");

Nit: Just `InspectorTest.log("");` will put 1 newline between things instead of
2.