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

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




--- Comment #2 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 351984
  --> https://bugs.webkit.org/attachment.cgi?id=351984
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.valueForCaseInsensitive
Key("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.