[webkit-reviews] review granted: [Bug 150005] Web Inspector: show redirect requests in Network and Timelines tabs : [Attachment 351837] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 150005: Web Inspector: show redirect requests in Network and Timelines tabs
https://bugs.webkit.org/show_bug.cgi?id=150005

Attachment 351837: Patch

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




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

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

r=me!

> Source/JavaScriptCore/inspector/protocol/Network.json:44
>		   { "name": "startTime", "type": "number", "description":
"Timing's startTime is a baseline in seconds, while the other numbers are ticks
in milliseconds relatively to this." },
> +		   { "name": "redirectStart", "type": "number", "description":
"Started redirect resolution." },
> +		   { "name": "redirectEnd", "type": "number", "description":
"Finished redirect resolution." },
> +		   { "name": "fetchStart", "type": "number", "description":
"Started fetching the resource." },

I think you need to add something to the description of these.

• The description of fetchStart should be like what startTime is now.
• The description of startTime/redirectStart/redirectEnd should point out that
they are in seconds, and that they are not milliseconds relative to something
else.

We will want to email our ITML friends about this change in required properties
on the Network.ResourceTiming object. I think they make use of it.

> Source/WebInspectorUI/UserInterface/Models/ResourceTimingData.js:81
> +	       redirectStart: payload.redirectStart,
> +	       redirectEnd: payload.redirectEnd,

If these are undefined (legacy backed) or zero (invalid value), should we just
make them NaN immediately?

    redirectStart: payload.redirectStart || NaN,

We may even want to assert that redirectStart/End are after startTime and
before fetchStart, otherwise fallback to NaN because its invalid data.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:217
> +.waterfall .block.filler{

Style: Space before brace.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:687
> +	       if (startTimestamp > lastEndTimestamp)
> +		   appendBlock(lastEndTimestamp, startTimestamp, "filler");
> +	       lastEndTimestamp = endTimestamp;

I figured we'd just make 1 filler block in the background from start to end and
draw all the other blocks on top of it. That seems simpler then creating a
bunch of filler blocks in the middle.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:727
>	   if (connectStart)
> -	       appendBlock(connectStart, connectEnd, "connect");
> +	       appendBlock(connectStart, secureConnectionStart || connectEnd,
"connect");

I think this one may be unnecessary since secureConnectionStart is in the
middle of connectStart/connectEnd. In ResourceTiming connectStart to connectEnd
includes the secure and leading time anyways.