[webkit-reviews] review granted: [Bug 195734] Web Inspector: Network - Toggle Between Live Activity and Imported HAR resource collections : [Attachment 364731] [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 195734: Web Inspector: Network - Toggle Between Live Activity and Imported
HAR resource collections
https://bugs.webkit.org/show_bug.cgi?id=195734

Attachment 364731: [PATCH] Proposed Fix

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




--- Comment #9 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 364731
  --> https://bugs.webkit.org/attachment.cgi?id=364731
[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 :(