[webkit-reviews] review denied: [Bug 190440] WebKit Inspector: Expose Server Timing Response Headers in Network Tab : [Attachment 352052] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Oct 11 16:46:01 PDT 2018
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.
More information about the webkit-reviews
mailing list