Expose Server Timing Response Headers in WebKit Inspector
Created attachment 351984 [details] Patch
Comment on attachment 351984 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351984&action=review This is great! Don't be alarmed by all of my comments, they are just on style and minor nits. Rebaseline this, give it tests and include a screenshot and it should be good to review! > Source/WebInspectorUI/UserInterface/Models/Resource.js:600 > + this._serverTimingEntries = > + WI.ServerTimingEntry.parseHeaders(this._responseHeaders.valueForCaseInsensitiveKey("Server-Timing")); Style: Keep this one line. We don't worry about line width in these cases its much much easier to read this as a single line. > Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:27 > +WI.ServerTimingEntry = class ServerTimingEntry We will definitely want to include layout tests for the parsing here. I'd recommend: LayoutTests/inspector/unit-tests/server-timing-entry.js You can model it after something like string-utilities.js. A sync test that tests the static methods. Something that looks like: function test() { let suite = InspectorTest.createSyncSuite("ServerTimingEntry"); suite.addTestCase({ name: "ServerTimingEntry.parseHeaders", description: "Parsing of Server-Timing headers." test() { InspectorTest.expectThat(...); ... return true; } }); suite.runTestCasesAndFinish(); } > Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:40 > + static parseHeaders(valueString = "") { Style: Put method opening/closing braces on their own line. So this would be: https://trac.webkit.org/wiki/WebInspectorCodingStyleGuide static parseHeaders(valueString = "") { ... } > Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:43 > + function trimLeadingWhiteSpace() { > + valueString = valueString.replace(/^\s*/, ""); > + } We have String.prototype.trimStart which would do same thing. valueString = valueString.trimStart(); > Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:47 > + if (valueString.charAt(0) !== char) { Style: No braces on a single line conditionals: https://webkit.org/code-style-guidelines/#braces > Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:54 > + function consumeToken() { Style: Put a newline between these inline functions to make them read as their own paragraph / concrete block of code. > Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:80 > + // split into two parts: > + // -everything before the first " or \ > + // -everything else Style: For comments we prefer complete sentences with capitals and periods if applicable. https://webkit.org/code-style-guidelines/#comments So something like: // Split into two parts: // - Everything before the first " or \ // - Everything else > Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:87 > + if (result[2].charAt(0) === "\"") { > + // we have found our closing " > + valueString = result[2].substring(1); // strip off everything after the closing " > + return value; // we are done here > + } Style: For comments we prefer complete sentences and annotations above the line instead of on the same line. So something like: // Found the closing quote. if (result[2].charAt(0) === "\"") { // Strip off everything after the closing quote. ... } > Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:98 > + const result = /([,;].*)/.exec(valueString); Style: We tend to use `let` for results of computations and `const` for anything that you would traditionally think of as a "compile time constant". So all of these `const` would become `let`. > Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:141 > + switch (paramName) { > + case "dur": Style: Case indentation should match the switch: https://webkit.org/code-style-guidelines/#indentation-case-label > Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:169 > + set duration(duration) { Style: Braces on their own line for these multi-line getters as well. Include a newline between them as well (compare to other Model classes). > Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:108 > + if (typeof duration !== "undefined") { Nit: Since we don't need to worry about backwards compatibility this can be: if (duration !== undefined) { ... } I'm assuming this is to allow zero as a valid value. If not, then you could just do: if (duration) { ... } > Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:113 > + block.classList.add("block", "response"); // TODO (cvazac) color Style: Avoid "TODO" and instead have a FIXME near by that is more descriptive, or includes a bugzilla link. For example: // FIXME: Provide unique colors for the different ServerTiming rows based on the label. Or: // FIXME: <https://webkit.org/b/123456> Give ServerTiming entries colors > Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:119 > + timeCell.textContent = WI.UIString("%.2fms").format(duration); Style: We should prefer `Number.secondsToMillisecondsString(duration)` over the "%.2fms" formatting unless there is an explicit reason this needs to be different. > Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:166 > + var emptyCell = this._appendEmptyRow().appendChild(document.createElement("td")); Style: `let` where possible (everywhere expect case blocks). > Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:175 > + let maxDuration = 0; > + serverTiming.forEach(({duration = 0}) => { > + maxDuration = Math.max(maxDuration, duration); > + }); Style: We tend to prefer a for..of loop for loops of this nature, maybe even Array.prototype.reduce. > Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:177 > + serverTiming.forEach(({name, duration, description}) => { Style: Same, a for..of loop would read a bit easier.
Created attachment 351989 [details] screenshot In the left column, we show the description (falling back to name). The right column is the duration (if specified) in ms.
Created attachment 351992 [details] Patch
Comment on attachment 351992 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351992&action=review > Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:176 > + this._appendHeaderRow(WI.UIString("Server Timing")); In the screenshot, each of the others have a colon. So I'd expect: WI.UIString("Server Timing:")
Created attachment 352052 [details] Patch
Thx for the review! This patch (https://bugs.webkit.org/attachment.cgi?id=352052&action=diff) addresses your feedback. Remaining TODOs: - tests - colors
Comment on attachment 352052 [details] 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.
Comment on attachment 352052 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352052&action=review > Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:178 > + let emptyCell = this._appendEmptyRow().appendChild(document.createElement("td")); > + emptyCell.colSpan = 3; > + emptyCell.appendChild(document.createElement("hr")); Oh, and it might be worth pulling this out into a helper. How about `this._appendDividerRow()`?
Comment on attachment 351984 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351984&action=review >> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:108 >> + if (typeof duration !== "undefined") { > > Nit: Since we don't need to worry about backwards compatibility this can be: > > if (duration !== undefined) { > ... > } > > I'm assuming this is to allow zero as a valid value. If not, then you could just do: > > if (duration) { > ... > } That's right, we need to support zeroes, I added a comment.
Created attachment 352123 [details] Patch
Comment on attachment 352052 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352052&action=review >> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:135 >> + } > > 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) AKAICT, `console.warn`s from here don't make it to the actual browser console, only to the command line. Is that what you want?
(In reply to cvazac from comment #12) > Comment on attachment 352052 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=352052&action=review > > >> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:135 > >> + } > > > > 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) > > AKAICT, `console.warn`s from here don't make it to the actual browser > console, only to the command line. Is that what you want? Yep that is what we want. The web content is not necessarily in error, it may be making use of a feature the frontend is not yet aware of. The console.warn can go to the system console or a inspector², which will be great because those of us developing on it might see it and then decide to add support for a possibly new Server-Timing feature. It may also be possible that we could turn this into a warning indicator on the Server-Timing row in the popover. That could have a tooltip or some description that says: ⚠️ Unknown Parameter: 'dur' Which would be useful to the developer to show they mis-typed 'dur' as 'duration' and that is why it is not exposed in the tools.
Created attachment 352135 [details] Patch
Comment on attachment 352135 [details] Patch Attachment 352135 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9547126 New failing tests: inspector/unit-tests/server-timing-entry.html
Created attachment 352138 [details] Archive of layout-test-results from ews103 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 352135 [details] Patch Attachment 352135 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9547157 New failing tests: inspector/unit-tests/server-timing-entry.html
Created attachment 352141 [details] Archive of layout-test-results from ews107 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 352135 [details] Patch Attachment 352135 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9547332 New failing tests: inspector/unit-tests/server-timing-entry.html
Created attachment 352143 [details] Archive of layout-test-results from ews116 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 352169 [details] Patch
Comment on attachment 352169 [details] 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?
Comment on attachment 352169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352169&action=review >> LayoutTests/inspector/unit-tests/server-timing-entry.html:91 >> + testServerTimingHeader("metric;desc=\\\\", [{"name":"metric","desc":""}]); > > Should the expected description here be two slashes? Were it double-quoted, yes. But the \ character isn't valid as a token, because it's not a valid tchar. Relevant spec bits from https://tools.ietf.org/html/rfc7230: token = 1*tchar tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA
Created attachment 352214 [details] Patch
Sites in the wild with server-timing entries: https://www.bbc.co.uk/iplayer https://ma.ttias.be/server-timings-chrome-devtools/ https://www.scalemates.com/ https://www.kohls.com/ My test site: https://server-timing.netlify.com/
Comment on attachment 352214 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352214&action=review r=me! > Source/WebInspectorUI/ChangeLog:9 > + * UserInterface/Main.html: You should regenerate the ChangeLog, this now has edits to new files like Test.html. > Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:184 > + this._appendHeaderRow(WI.UIString("Server Timing")); This lost the colon. Should we add it back? I only suggested it because it matches the other header rows. > LayoutTests/inspector/unit-tests/server-timing-entry.html:63 > + // spaces > + testServerTimingHeader(`metric ; `, [{"name":"metric"}]); Maybe have a test for leading whitespace. Seems like it would be stripped via `consumeToken()` and work just fine.
(In reply to cvazac from comment #23) > Comment on attachment 352169 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=352169&action=review > > >> LayoutTests/inspector/unit-tests/server-timing-entry.html:91 > >> + testServerTimingHeader("metric;desc=\\\\", [{"name":"metric","desc":""}]); > > > > Should the expected description here be two slashes? > > Were it double-quoted, yes. But the \ character isn't valid as a token, > because it's not a valid tchar. > Relevant spec bits from https://tools.ietf.org/html/rfc7230: > token = 1*tchar > tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." / > "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA Oh nice, that makes sense. Thanks!
Comment on attachment 352214 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352214&action=review This is awesome! One thing to note is that you're missing a ChangeLog for the LayoutTests changes :( > Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:180 > + if (!this._durationSet) { Is it necessary to prevent modification of this value once it's set? Typically, our style would be to `console.assert` for this, as it's more of a programmer's responsibility to control this. > Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:188 > + if (!this._descriptionSet) { Ditto (180). > Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:140 > + let { I would prefer if you separated this onto two lines. The reason we destructured `timingData` is because it's just a plain object, whereas `timingData` is an actual getter. I don't think we should be destructuring our model objects. let {startTime, redirectStart, redirectEnd, fetchStart, domainLookupStart, domainLookupEnd, connectStart, connectEnd, secureConnectionStart, requestStart, responseStart, responseEnd} = this._resource.timingData; let serverTiming = this._resource.serverTiming;
Comment on attachment 352214 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352214&action=review >> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:180 >> + if (!this._durationSet) { > > Is it necessary to prevent modification of this value once it's set? Typically, our style would be to `console.assert` for this, as it's more of a programmer's responsibility to control this. Per spec (https://w3c.github.io/server-timing/) "first-man" in wins, relevant bits: To avoid any possible ambiguity, individual server-timing-param-names SHOULD NOT appear multiple times within a server-timing-metric. If any server-timing-param-name is specified more than once, only the first instance is to be considered, even if the server-timing-param is incomplete or invalid. All subsequent occurrences MUST be ignored without signaling an error or otherwise altering the processing of the server-timing-metric. This is the only case in which the ordering of parameters within a server-timing-metric is considered to be significant.
Comment on attachment 352214 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352214&action=review >>> Source/WebInspectorUI/UserInterface/Models/ServerTimingEntry.js:180 >>> + if (!this._durationSet) { >> >> Is it necessary to prevent modification of this value once it's set? Typically, our style would be to `console.assert` for this, as it's more of a programmer's responsibility to control this. > > Per spec (https://w3c.github.io/server-timing/) "first-man" in wins, relevant bits: > To avoid any possible ambiguity, individual server-timing-param-names SHOULD NOT appear multiple times within a server-timing-metric. If any server-timing-param-name is specified more than once, only the first instance is to be considered, even if the server-timing-param is incomplete or invalid. All subsequent occurrences MUST be ignored without signaling an error or otherwise altering the processing of the server-timing-metric. This is the only case in which the ordering of parameters within a server-timing-metric is considered to be significant. In that case, use an early return: set duration(duration) { if (this._duration !== undefined) return; this._duration = duration; } You might also want to synthesize a console error to warn developers that "hey you included the same name twice".
Created attachment 352243 [details] Patch
Comment on attachment 352243 [details] Patch r=me! Do you need me to commit-queue this?
Comment on attachment 352243 [details] Patch Clearing flags on attachment: 352243 Committed r237151: <https://trac.webkit.org/changeset/237151>
All reviewed patches have been landed. Closing bug.
<rdar://problem/45288297>