[Webkit-unassigned] [Bug 190440] WebKit Inspector: Expose Server Timing Response Headers in Network Tab

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=190440

Joseph Pecoraro <joepeck at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #352169|review?                     |review-
              Flags|                            |

--- 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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20181012/96047483/attachment.html>