Bug 195734 – Web Inspector: Network - Toggle Between Live Activity and Imported HAR resource collections

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 364626 [details]
[IMAGE] Dark - Imported

[Joseph Pecoraro]

Created attachment 364627 [details]
[IMAGE] Dark - Live

[Joseph Pecoraro]

Created attachment 364628 [details]
[IMAGE] Light - Imported

[Joseph Pecoraro]

Created attachment 364629 [details]
[IMAGE] Light - Live

[Joseph Pecoraro]

Created attachment 364630 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Note, the path component collection switch only appears once you've imported something. otherwise it is never shown at all.

[Devin Rousso]

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;

...

[Joseph Pecoraro]

Created attachment 364731 [details]
[PATCH] Proposed Fix

I agree the `this._collections` set approach is much cleaner. This also addresses a few subtle bugs.

[Devin Rousso]

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

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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

[Joseph Pecoraro]

<https://trac.webkit.org/changeset/243004/webkit>

[Radar WebKit Bug Importer]

<rdar://problem/48933409>