[webkit-reviews] review denied: [Bug 190440] WebKit Inspector: Expose Server Timing Response Headers in Network Tab : [Attachment 352052] 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 352052: Patch

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




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