[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 #352052|review?                     |review-
              Flags|                            |

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

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

Nice! This parsing looks great to me. I provided a few feedback comments but nothing serious.

r- only because it just needs tests.

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:40
> +    static parseHeaders(valueString = "")

Lets put this somewhere nearby:

    // https://w3c.github.io/server-timing/#the-server-timing-header-field

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:74
> +        function consumeQuotedString() {

I found this pretty confusing to follow with the splits. This appears to be doing 2 things:

    1. Loop along a string to find a trailing DQUOTE that was not escaped.
    2. Unescaping characters preceded by an escape character.

I think it would be clearer written as a loop that does those two things:

    function consumeQuotedString() {
        // https://tools.ietf.org/html/rfc7230#section-3.2.6
        console.assert(valueString.charAt(0) === "\"");

        // Consume the leading quote.
        valueString = valueString.substring(1);

        let unescapedValueString = "";
        for (let i = 0; i < valueString.length; ++i) {
            let char = valueString.charAt(i);
            switch (char) {
            case "\\":
                // Escaped character.
                ++i;
                if (i < valueString.length)
                    break;
                unescapedValueString += valueString.charAt(i);
                break;
            case "\"":
                // Trailing quote.
                valueString = valueString.substring(i + 1);
                return unescapedValueString;
            default:
                unescapedValueString += char;
                break;
            }
        }

        // No trailing quote found. Consume the entire string to complete parsing.
        valueString = "";
        return null;
    }

The efficiency of this is a little worse, but negligibly so for the strings we are likely to have.  I find this much easier to follow. What do you think?

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:94
> +                console.assert(result[2].charAt(0) === "\\");

As written, this would assert if given an invalid string since result[2] could be the empty string. For example:

    valueString = '"te\\"st';
    consumeQuotedString();

In the second loop value string is "st" which would result in, result[2] being the empty string. This would behave just fine it would just inadvertently assert.

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:135
> +                if (parseParameter) {
> +                    // paramName is valid.
> +                    parseParameter.call(this, entry, paramValue);
> +                }

It could be that this is a new parameter we don't know about yet, but I like that this code is trying to be future proof/compatible. Also the Function.prototype.call is unnecessary here since we don't use `this`. How about:

    if (parseParameter)
        parseParameter(entry, paramValue);
    else
        console.warn("Unknown Server-Timing parameter:", paramName, paramValue)

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:146
> +    static getParserForParameter(paramName) {

Lets make this another inline function inside of parseHeaders. It doesn't seem to warrant being another static function.

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:149
> +            return function(entry, paramValue) {

Nit: I'd probably swap these parameters to be `function(value, entry)`. Since this primarily handles the value, and optionally updates the entry.

> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:189
> +

Style: Remove this extraneous line.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:180
> +            this._appendHeaderRow(WI.UIString("Server Timing"));

Nit: Add a colon to this UIString => "Server Timing:"

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:185
> +            for (let entry of serverTiming) {
> +              let {name, duration, description} = entry;

Style: Indentation should be 4 spaces.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:191
> +

Style: Remove this extraneous line.

-- 
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/20181011/0ffa96ab/attachment.html>