Bug 195655 - Web Inspector: Network - HAR Export duplicates blocked/send time if there was no dns/connect block
Summary: Web Inspector: Network - HAR Export duplicates blocked/send time if there was...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-12 17:28 PDT by Joseph Pecoraro
Modified: 2019-03-13 12:06 PDT (History)
4 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (10.41 KB, patch)
2019-03-12 17:33 PDT, Joseph Pecoraro
drousso: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2019-03-12 17:28:32 PDT
Network - HAR Export duplicates blocked/send time if there was no dns/connect block

Steps to Reproduce:
1. Inspect <http://bogojoker.com/shell/>
2. Show Network tab
3. Reload (ignoring caches)
4. Export HAR
  => Some resources that were stalled but no DNS will double count their "queuing time" as both the blocked and send:

Example:

    "timings": {
 >    "blocked": 579.9859762191772,
      "dns": -1,
      "connect": -1,
      "ssl": -1,
 >    "send": 579.9859762191772,
      "wait": 41.41700267791748,
      "receive": 59.1130256652832
    },

Note:
• One of these should win.
• I think it makes the most sense for blocked to be the large value and send to be 0 here.
Comment 1 Radar WebKit Bug Importer 2019-03-12 17:32:24 PDT
<rdar://problem/48831152>
Comment 2 Joseph Pecoraro 2019-03-12 17:33:40 PDT
Created attachment 364480 [details]
[PATCH] Proposed Fix
Comment 3 Devin Rousso 2019-03-12 17:56:18 PDT
Comment on attachment 364480 [details]
[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
Comment 4 Joseph Pecoraro 2019-03-13 11:57:51 PDT
Comment on attachment 364480 [details]
[PATCH] Proposed Fix

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

>> 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?

The floating point error is regular, so I'd rather just log the entire number.

>> LayoutTests/http/tests/inspector/network/har/har-basic.html:97
>> +            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.

Yeah, this was before I filtered them out entirely! I'll do that.

>> 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

Tweaked this a little.
Comment 5 Joseph Pecoraro 2019-03-13 12:06:24 PDT
https://trac.webkit.org/r242896