[webkit-reviews] review granted: [Bug 195642] Web Inspector: Network - HAR Import : [Attachment 364570] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 195642: Web Inspector: Network - HAR Import
https://bugs.webkit.org/show_bug.cgi?id=195642

Attachment 364570: [PATCH] Proposed Fix

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




--- Comment #14 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 364570
  --> https://bugs.webkit.org/attachment.cgi?id=364570
[PATCH] Proposed Fix

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

r=me

> LayoutTests/http/tests/inspector/network/har/resources/empty.har:10
> +	   "startedDateTime": "2017-10-23T01:55:52.694Z",

Why was this changed from the previous patch. Does it have an effect on tests?

> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:76
> +	   this._type = WI.Resource.typeFromMIMEType(this._mimeType);

This should get set by the `super` call, assuming you've passed in `mimeType`.

> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:100
> +    static fromHAREntry(entry, archiveStartWalltime = NaN)

Having this default to `NaN` means that it will force `requestSentTimestamp` to
be `NaN` even if `requestSentWalltime` is valid.  I'd either expect an error
(and `return null;`) in that case or for us to either require
`archiveStartWalltime` to not be `NaN` (and error if it is) or for it to
default to `0` when not provided.

> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:108
> +	   let {request, response, startedDateTime, timings} = entry;
> +	   let requestSentWalltime =
WI.HARBuilder.dateFromHARDate(startedDateTime) / 1000;
> +	   let requestSentTimestamp = requestSentWalltime -
archiveStartWalltime;

Should we add checks/logging/returns if any of these values isn't property
formatted in the imported JSON?  I think we should have a followup to examine
the entire import "flow" and add checks with errors/bails where needed to
ensure that we never throw an exception.

> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:131
> +	       let priorToRequestStart = timings.send / 1000;
> +	       let requestStartToResponseStart = timings.wait / 1000;
> +	       let responseStartToResponseEnd = timings.receive / 1000;

I'd inline these since they are only used once.

> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:140
> +		   accumulation += (timings.blocked / 1000);

Style: please remove the parenthesis, as they are unnecessary.

> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:144
> +		   accumulation += (timings.dns / 1000);

Ditto (>140).

> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:150
> +		   accumulation += (timings.connect / 1000);

Ditto (>140).