[webkit-reviews] review granted: [Bug 195655] Web Inspector: Network - HAR Export duplicates blocked/send time if there was no dns/connect block : [Attachment 364480] [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 195655: Web Inspector: Network - HAR Export duplicates blocked/send time if
there was no dns/connect block
https://bugs.webkit.org/show_bug.cgi?id=195655

Attachment 364480: [PATCH] Proposed Fix

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




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

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

r=me

> LayoutTests/http/tests/inspector/network/har/har-basic.html:22
> +	   return value;

Should we `value.toFixed(2)` on any number that's logged, or is the floating
point "error" more predictable than that?

> LayoutTests/http/tests/inspector/network/har/har-basic.html:97
> +	       const requestHeaders = {};
> +	       const responseHeaders = {};

Considering that these shadow the same variables above, I'd keep the
`requestHeaders` and `responseHeaders` variables where they were in the
HAR.Basic.FakeResource test case.

> LayoutTests/http/tests/inspector/network/har/har-basic.html:107
> +	       let resource1 = resourceWithTimingData({

Style: all of the `let` for each `resourceWithTImingData` should be `const`, as
the data/value doesn't change between executions.

> LayoutTests/http/tests/inspector/network/har/har-basic.html:158
> +	       let {log: {entries: [entry1, entry2, entry3]}} = har;

🤮, but I guess it works

> Source/WebInspectorUI/UserInterface/Controllers/HARBuilder.js:269
> +	       result.send = (domainLookupEnd || connectStart) ? (requestStart
- (connectEnd || domainLookupEnd || startTime)) * 1000 : 0;

NIT: I'd rather you use `connectStart` so that it matches the true case of the
ternary.  You should also flip the order around so that matches too.

As you mentioned in person, if the ternary is true, we're guaranteed to not
need the `startTime` as one of `connectEnd` or `domainLookupEnd` will be
truthy.
```
    result.send = (connectEnd || domainLookupEnd) ? (requestStart - (connectEnd
|| domainLookupEnd)) * 1000 : 0;
```

You could also pull out the `connectEnd || domainLookupEnd` into a local
temporary if you feel like it :P