[webkit-reviews] review canceled: [Bug 195642] Web Inspector: Network - HAR Import : [Attachment 364450] [PATCH] Proposed Fix
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Mar 13 13:53:32 PDT 2019
Joseph Pecoraro <joepeck at webkit.org> has canceled 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 364450: [PATCH] Proposed Fix
https://bugs.webkit.org/attachment.cgi?id=364450&action=review
--- Comment #10 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 364450
--> https://bugs.webkit.org/attachment.cgi?id=364450
[PATCH] Proposed Fix
View in context: https://bugs.webkit.org/attachment.cgi?id=364450&action=review
>> Source/WebInspectorUI/ChangeLog:43
>> + loaded resources).
>
> I don't think this is the right approach for this. Hiding the current page's
network data is bound to cause confusion. Especially since it's not
indicated anywhere. Perhaps we can add a <select> next to the "[ ] Preserve
Log" that would appear when there are imported recordings? That way, it's
possible to toggle back and forth.
The two cases are mutually exclusive:
• If you're importing a HAR you probably don't care about the active page's
data.
• If you care about the active page's data you wouldn't import a HAR.
However I agree we should have some indication somewhere that we are viewing an
imported archive.
>> Source/WebInspectorUI/UserInterface/Controllers/HARBuilder.js:350
>> + return WI.Resource.ResponseSource.Other;
>
> I never understood the "distinction" in our style between using a `default`
vs returning outside the `switch`. Do you have any "criteria" for using one
over the other?
Yeahhh we don't really have one. I'll just make them consistent in this file.
Ideally we wouldn't have a default and could lint that the switch handles all
cases.
Also in this case I'll make then `console.warn` since it is user provided data,
we might be missing an unknown valid case or it could just be bad data.
>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:46
>> + this._localResourcesMap = new Map;
>
> This assumes that there is only ever one `WI.LocalResource` per url. Does
HAR explicitly only allow one resource per URL? This would be a good
opportunity to use a `Multimap` :)
Our entire Network stack makes that "assumption" right now. Technically nothing
uses this map right now anyways, I used it for testing.
No HAR doesn't restrict one resource per URL. Also the same HAR could be
imported multiple times.
>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:610
>> + localResourceForURL(url)
>
> Will this be used by a later patch?
No. I used this for debugging and in case we start using this in the future.
>> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:663
>> + let mainResourceSentWalltime =
WI.HARBuilder.dateFromHARDate(json.log.pages[0].startedDateTime) / 1000;
>
> We should probably have stricter checks since this is imported data (which
could be potentially anything). Nothing will get executed, but I'd like to
avoid situations where someone can load something into Web Inspector and cause
it to error :(
> ```
> if (!Array.isArray(json.log.entries) || !Array.isArray(json.log.pages) ||
!json.log.pages[0] || !json.log.pages[0].startedDateTime) {
> ```
> If you're feeling _really_ careful, we should also check that
`mainResourceSentWalltime` doesn't return `NaN` :)
Done.
>> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:50
>> + // NOTE: This directly overwrites WI.Resource properties.
>
> Rather than do this, can't we pass some of these values into the constructor
as an object? The constructor of `WI.Resource` (and `WI.SourceCode`) don't do
any other logic that isn't repeated here. Is this a "future-proofing" of
sorts, to make sure that if the constructor of `WI.Resource` changes this class
won't also be affected?
> ```
> super(request.url, {
> mimeType: response.mimeType ||
this._responseHeaders.valueForCaseInsensitiveKey("Content-Type") || null,
> requestMethod: request.method,
> requestHeaders: request.headers,
> requestData: request.data,
> requestSentTimestamp, request.timestamp,
> requestSentWalltime: request.walltime,
> });
> ```
Sure, I did this. It does seem a little weird, but does reduce duplicate work.
>> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:84
>> + this._cached = false; // FIXME: How should we denote cached? Assume
from response source?
>
> Is this not stored in the HAR? We already add "extra" keys/values (e.g.
`_fetchType`), so maybe we could do the same for `_failed` and `_cached`?
Correct, but I want this to work with existing HARs today and then we can
potentially extend our HAR to include additional properties.
There is a `cache` section of HAR that we do not implement.
>> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:94
>> + result[name] = value;
>
> This limits us to having only one header per "type", although it seems like
this is expected by `WI.Resource`.
Yep, we've had this limitation for a very long time.
>> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:102
>> + // FIXME: HAR does not include resource priority.
>
> Ditto (>83).
Same response. I want this to work with HARs today, and extend with WebKit
specific values later.
>> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:126
>> + let earlierToRequestStart = timings.send / 1000;
>
> NIT: how about `beforeRequestStart`, or even `priorToRequestStart`?
Prior sounds nice!
>> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:130
>> + // We don't allow zero for the start time, so bump it to
non-zero if it was.
>
> Is it possible for an inspector generated HAR to have a 0
`requestSentTimestamp`? If not, I'd consider this to be an early-return worthy
error (see >NetworkManager.js:663).
Yes, it is possible, and likely always the case for the for the initial main
resource, since that will be the pages[0].startedDateTime that we are using as
the `archiveStartWalltime`:
startedDateTime:
HARBuilder.date(WI.networkManager.mainFrame.mainResource.requestSentDate),
It is just unfortunate that we don't support timing.startTime being the number
0, and I'll file a bug about that.
>> Source/WebInspectorUI/UserInterface/Models/LocalResource.js:157
>> + if (connectDuration) {
>
> Why is it that `connectStart` is only being set if `blockedDuration` and/or
`dnsDuration` is set, but not if just `connectDuration` is set? If
`blockedDuration` and `dnsDuration` are both 0, but `connectDuration` is non-0,
shouldn't we still be setting `connectStart = accumulation` and `connectEnd =
accumulation + connectDuration`?
The 4 cases are (with any section prior to `send` being potentially -1 or 0).
|-------------------------------------|-------|
blocked send
|-------------------------|-----------|-------|
blocked connect send
|-----------------|-------|-----------|-------|
blocked dns connect send
|-----------------|-------------------|-------|
blocked dns send
I've simplified this to instead just perform accumulation work:
let accumulation = timing.startTime;
if (hasBlocked)
accumulation += (timings.blocked / 1000);
if (hasDNS) {
timing.domainLookupStart = accumulation;
accumulation += (timings.dns / 1000);
timing.domainLookupEnd = accumulation;
}
if (hasConnect) {
timing.connectStart = accumulation;
accumulation += (timings.connect / 1000);
timing.connectEnd = accumulation;
if (hasSecureConnect)
timing.secureConnectionStart = timing.connectEnd - (timings.ssl /
1000);
}
accumulation += priorToRequestStart;
timing.requestStart = accumulation;
...
This should better handle cases where a prior section was missing or empty and
make it more straightforward to read as you were suggesting.
>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:115
>> +
this._harImportNavigationItem.addEventListener(WI.ButtonNavigationItem.Event.Cl
icked, () => { this._importHAR(); });
>
> NIT: please put this on a new line, so it isn't confused with an implicit
return arrow function.
We follow this pattern in more than 50% of these, but I'll newline it.
More information about the webkit-reviews
mailing list