Bug 195734 - Web Inspector: Network - Toggle Between Live Activity and Imported HAR resource collections
Summary: Web Inspector: Network - Toggle Between Live Activity and Imported HAR resour...
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-13 21:03 PDT by Joseph Pecoraro
Modified: 2019-03-15 12:08 PDT (History)
4 users (show)

See Also:


Attachments
[IMAGE] Dark - Imported (1.22 MB, image/png)
2019-03-13 21:08 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Dark - Live (945.67 KB, image/png)
2019-03-13 21:08 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Light - Imported (1.16 MB, image/png)
2019-03-13 21:08 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Light - Live (891.19 KB, image/png)
2019-03-13 21:08 PDT, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (25.60 KB, patch)
2019-03-13 21:09 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (42.18 KB, patch)
2019-03-14 17:37 PDT, Joseph Pecoraro
drousso: review+
drousso: commit-queue-
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-13 21:03:30 PDT
Network - Toggle Between Live Activity and Imported HAR resource collections

Network Tab should let you toggle between the Live Activity which it is always recording and any Imported HARs.
Comment 1 Joseph Pecoraro 2019-03-13 21:08:31 PDT
Created attachment 364626 [details]
[IMAGE] Dark - Imported
Comment 2 Joseph Pecoraro 2019-03-13 21:08:40 PDT
Created attachment 364627 [details]
[IMAGE] Dark - Live
Comment 3 Joseph Pecoraro 2019-03-13 21:08:49 PDT
Created attachment 364628 [details]
[IMAGE] Light - Imported
Comment 4 Joseph Pecoraro 2019-03-13 21:08:58 PDT
Created attachment 364629 [details]
[IMAGE] Light - Live
Comment 5 Joseph Pecoraro 2019-03-13 21:09:11 PDT
Created attachment 364630 [details]
[PATCH] Proposed Fix
Comment 6 Joseph Pecoraro 2019-03-13 21:10:07 PDT
Note, the path component collection switch only appears once you've imported something. otherwise it is never shown at all.
Comment 7 Devin Rousso 2019-03-14 10:51:59 PDT
Comment on attachment 364630 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:43
> +        this._pendingUpdateMap = new Map;

NIT: this should be `_pendingUpdatesMap` to match `_pendingUpdates`.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:55
> +        this._collections = new Map;

This isn't used.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:56
> +        this._nextCollectionKey = 1;
> +        this._mainCollectionKey = this._nextCollectionKey++;
> +
> +        this._collections = new Map;
> +        this._activeCollectionKey = null;

Duplicate of above?

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:326
> +            this._pendingUpdate = [];

This should be `_pendingUpdates`.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:552
> +    get _entries() { return this._entriesMap.get(this._activeCollectionKey); }
> +    set _entries(x) { this._entriesMap.set(this._activeCollectionKey, x); }
> +    get _filteredEntries() { return this._filteredEntriesMap.get(this._activeCollectionKey); }
> +    set _filteredEntries(x) { this._filteredEntriesMap.set(this._activeCollectionKey, x); }
> +    get _pendingInsertions() { return this._pendingInsertionsMap.get(this._activeCollectionKey); }
> +    set _pendingInsertions(x) { this._pendingInsertionsMap.set(this._activeCollectionKey, x); }
> +    get _pendingUpdates() { return this._pendingUpdateMap.get(this._activeCollectionKey); }
> +    set _pendingUpdates(x) { this._pendingUpdateMap.set(this._activeCollectionKey, x); }
> +    get _waterfallStartTime() { return this._waterfallStartTimeMap.get(this._activeCollectionKey); }
> +    set _waterfallStartTime(x) { this._waterfallStartTimeMap.set(this._activeCollectionKey, x); }
> +    get _waterfallEndTime() { return this._waterfallEndTimeMap.get(this._activeCollectionKey); }
> +    set _waterfallEndTime(x) { this._waterfallEndTimeMap.set(this._activeCollectionKey, x); }

I understand that the idea behind this is to avoid making the diff worse, but by doing this we're probably going to incur a performance "hit", in that each time we call these it's doing another brand new `get`/`set`.  Many of the functions in `WI.NetworkTableContentView` were written on the assumption (which was previously true) that these were actual values, not getters for some value.  I think that given that issue, we should probably rework this to have a single `_collections` map that holds objects with all of these inside it, so that each function can just fetch the active collection once (or you could make a function/getter for it) and modify that object as they see fit.  This would let you avoid most of the indentation changes caused by `_runForMainCollection` as instead you'd only need to change `this._entries` (for example) to `collection.entries`.

constructor()
    this._collections = new Map;

_addCollection(key)
    this._collections.set(key, {
        entries: [],
        filteredEntries: [],
        pendingInsertions: [],
        pendingUpdates: [],
        waterfallStartTime: NaN,
        waterfallEndTime: NaN,
    });

get _activeCollection()
    return this._collections.get(this._activeCollectionKey);

_updateWaterfallTimelineRuler()
    let collection = this._activeCollection;

    if (isNaN(collection.waterfallStartTime) || startTimestamp < collection.waterfallStartTime)
        collection.waterfallStartTime = startTimestamp;

    if (isNaN(collection.waterfallEndTime) || endTimestamp > collection.waterfallEndTime)
        collection.waterfallEndTime = endTimestamp;

...
Comment 8 Joseph Pecoraro 2019-03-14 17:37:20 PDT
Created attachment 364731 [details]
[PATCH] Proposed Fix

I agree the `this._collections` set approach is much cleaner. This also addresses a few subtle bugs.
Comment 9 Devin Rousso 2019-03-14 22:27:06 PDT
Comment on attachment 364731 [details]
[PATCH] Proposed Fix

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

r=me, awesome work!

I did discover an interesting bug while testing this though :(

# STEPS TO REPRODUCE:
1. inspect <https://webkit.org>
2. go to the Network tab
3. clear the Network table
4. reload the page
5. navigate to <https://apple.com>
 => Export button is disabled (`_canExportHAR` returns `false` because `WI.networkManager.mainFrame.mainResource.requestSendDate` is falsy)

> Source/WebInspectorUI/ChangeLog:57
> +        Introduce the concept of collections that can be swapped between

Grammar: the word "between" is awkward.  Perhaps just use "in/out"?

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:537
> +        this._collections.add(collection);

Given that we're the ones who are creating these objects, is there a reason to not just use an array?  There's no need to worry about uniqueness since each time we add, we're adding a brand new object.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:597
> +        this._hideDetailView();

Rather than hide the details view, should we remember it per-collection, so that switching back/forth between different collections doesn't require you to reselect a row?  I can imagine a use case where someone has a HAR export of a bug (e.g. some cookie is set wrong) and has imported it to try to "diff" it against the live page.  Having to reselect the particular entry in question each time you switch collections would be extremely annoying.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1293
> +        if (!this._isShowingMainCollection())
> +            return false;

Why don't we want to allow the export of an imported HAR?  It seems unnecessarily strict to only allow exporting of the main activity data.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1333
> +        let entries = collection.entries;

I'd inline this.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1674
> +        let isMain = currentCollection === this._mainCollection;

NIT: this should be renamed to `wasNamed` to match how the callers use it (or rename `isMain` to match this function).

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:2051
> +

Style: unnecessary newline.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:2061
> +        for (let entry of this._activeCollection.entries)
>              this._checkURLFilterAgainstResource(entry.resource);

This needs to be guarded by a `if (this._hasURLFilter())` .

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:2129
> +            let displayName = WI.UIString("Imported - %s").format(result.filename);

Missing localized string :(
Comment 10 Joseph Pecoraro 2019-03-15 11:32:09 PDT
Comment on attachment 364731 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:537
>> +        this._collections.add(collection);
> 
> Given that we're the ones who are creating these objects, is there a reason to not just use an array?  There's no need to worry about uniqueness since each time we add, we're adding a brand new object.

No reason, other then assertions. I'll switch.

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:597
>> +        this._hideDetailView();
> 
> Rather than hide the details view, should we remember it per-collection, so that switching back/forth between different collections doesn't require you to reselect a row?  I can imagine a use case where someone has a HAR export of a bug (e.g. some cookie is set wrong) and has imported it to try to "diff" it against the live page.  Having to reselect the particular entry in question each time you switch collections would be extremely annoying.

That is a reasonable idea. I'll leave that for a follow-up if it comes up in practice, I'd rather people have two windows side-by side in that case.

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1293
>> +            return false;
> 
> Why don't we want to allow the export of an imported HAR?  It seems unnecessarily strict to only allow exporting of the main activity data.

The filename of the imported HAR seems sufficient to me. Having Export disabled is another indicator to the user that they are viewing non-live data.

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:2061
>>              this._checkURLFilterAgainstResource(entry.resource);
> 
> This needs to be guarded by a `if (this._hasURLFilter())` .

Good catch.

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:2129
>> +            let displayName = WI.UIString("Imported - %s").format(result.filename);
> 
> Missing localized string :(

Yeah, this is already in another patch up for review and I didn't want to conflict. I re-generate before landing to have the latest.
Comment 11 Joseph Pecoraro 2019-03-15 11:37:56 PDT
> I did discover an interesting bug while testing this though :(
> 
> # STEPS TO REPRODUCE:
> 1. inspect <https://webkit.org>
> 2. go to the Network tab
> 3. clear the Network table
> 4. reload the page
> 5. navigate to <https://apple.com>
>  => Export button is disabled (`_canExportHAR` returns `false` because
> `WI.networkManager.mainFrame.mainResource.requestSendDate` is falsy)

I suspect this is because PSON. We are likely missing details about the main resource load on a cross origin navigation.
Comment 12 Joseph Pecoraro 2019-03-15 12:03:15 PDT
<https://trac.webkit.org/changeset/243004/webkit>
Comment 13 Radar WebKit Bug Importer 2019-03-15 12:08:00 PDT
<rdar://problem/48933409>