[webkit-reviews] review denied: [Bug 190440] WebKit Inspector: Expose Server Timing Response Headers in Network Tab : [Attachment 352169] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied cvazac at gmail.com's request for
review:
Bug 190440: WebKit Inspector: Expose Server Timing Response Headers in Network
Tab
https://bugs.webkit.org/show_bug.cgi?id=190440

Attachment 352169: Patch

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




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

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

Awesome tests! Just rebaseline the results (the console.warn output shows up in
it)

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:87
> +		       // Backslash character found, ignore it

Style: Comments are complete sentences that end in a period.

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:89
> +		       if (i < valueString.length)

Style: Given the comment inside of this there should be braces.

> LayoutTests/inspector/unit-tests/server-timing-entry-expected.txt:221
> +Testing response header: -->metric;desc=\"\<--
> +PASS: Parsed ServerTimingEntry count ?== expected results count.
> +PASS: expectEqual("metric", "metric")
> +PASS: expectEqual(undefined, undefined)
> +PASS: expectEqual("", "")

These tests are great but the output is a bit unfortunate!

> LayoutTests/inspector/unit-tests/server-timing-entry.html:20
> +		   InspectorTest.expectEqual(results.length,
expectedResults.length,
> +		       `Parsed ServerTimingEntry count ?== expected results
count.`);

This could provide the expected results count and it might be easier to read
immediately. For example:

    PASS: Parsed ServerTimingEntry count has expected results count of 0.
    PASS: Parsed ServerTimingEntry count has expected results count of 1.
    PASS: Parsed ServerTimingEntry count has expected results count of 2.

Instead of all having ?== and no known value.

> LayoutTests/inspector/unit-tests/server-timing-entry.html:31
> +		       InspectorTest.expectEqual(serverTimingEntry.name,
expectedResult.name);
> +		       InspectorTest.expectEqual(serverTimingEntry.duration,
expectedResult.dur);
> +		       InspectorTest.expectEqual(serverTimingEntry.description,
expectedResult.desc);

If you add a third parameter the output will be better. Something like:

    InspectorTest.expectEqual(serverTimingEntry.name, expectedResult.name,
`name is ${JSON.stringify(expectedResult.name)}`);
    InspectorTest.expectEqual(serverTimingEntry.duration, expectedResult.dur,
`duration is ${JSON.stringify(expectedResult.dur)}`);
    InspectorTest.expectEqual(serverTimingEntry.description,
expectedResult.desc, `description is ${JSON.stringify(expectedResult.desc)}`);

Should produce something slightly easier to read then things like "equals(42,
42)" above.

> LayoutTests/inspector/unit-tests/server-timing-entry.html:50
> +	       testServerTimingHeader("metric;desc=\"description\"",
[{"name":"metric","desc":"description"}]);

It may be easier for you to use template strings so that you don't have to
escape double quotes all the time:

    testServerTimingHeader(`metric;desc="description"`,
[{"name":"metric","desc":"description"}]);

This has the added bonus of syntax highlighting the test strings slightly
differently if your text editor supports template strings =).

> LayoutTests/inspector/unit-tests/server-timing-entry.html:91
> +	       testServerTimingHeader("metric;desc=\\\\",
[{"name":"metric","desc":""}]);

Should the expected description here be two slashes?