[Devtools] NetworkLogView now uses NetworkLog as source of truth.

[Devtools] NetworkLogView now uses NetworkLog as source of truth.

NetworkLogView will now use NetworkLog to control it's requests.
NetworkLog now will dispatch the needed events and the view will only
update when the NetworkLog has dispatched them.

This is in preporation of loading HAR files into network panel.

R=dgozman,pfeldman
BUG=374557
Review-Url: https://codereview.chromium.org/2914033002
Cr-Commit-Position: refs/heads/master@{#480728}
Committed: https://chromium.googlesource.com/chromium/src/+/ca6a9f1f759e79a3a08be1747c47ee9354db6cff

[allada, 2017-05-31 22:23:26]

Patchset #1 (id:1) has been deleted

[allada, 2017-05-31 22:23:31]

Patchset #1 (id:20001) has been deleted

[allada, 2017-05-31 22:23:36]

Patchset #1 (id:40001) has been deleted

[allada, 2017-05-31 22:23:47]

PTL

[dgozman, 2017-06-01 20:56:28]

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js
(left):

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:954: if
(!this._recording)
Who handles this?

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js
(right):

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:117:
NetworkLog.networkLog.addEventListener(NetworkLog.NetworkLog.Events.Reset,
this._reset.bind(this));
this._reset, this

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:834:
while (this._staleRequests.size) {
while (this._staleRequests.size) {
  var request = this._staleRequests.firstValue();
  this._staleRequests.delete(request);
  ...
}

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:900:
this.didRefreshForTest();
_didRefreshForTest

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:968: //
Pull all the redirects of the main request upon commit load.
You don't check that request is a main request.

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:980:
while (node && node.parent) {
Why do you remove all parents? This removes the group completely, right?

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js
(left):

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js:262:
this._reset();
Where did this go?

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js
(right):

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:251: if
(!Common.moduleSetting('network.preserve-log').get())
The setting should move to network_log module.

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:358:
reset() {
Why is this public?

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:360:
this._requestsByManagerAndId.forEach(map => map.clear());
Just clear the map itself instead?

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:361: var
networkManagers = new Set(
var networkManagers = SDK.targetManager.models(SDK.NetworkManager);

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:384:
this._mainRequest = mainRequest;
This is a struct, where all fields should be public.

[allada, 2017-06-02 01:02:44]

PTaL

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js
(left):

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:954: if
(!this._recording)
On 2017/06/01 20:56:27, dgozman wrote:
> Who handles this?

It's now in NetworkLog.

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js
(right):

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:117:
NetworkLog.networkLog.addEventListener(NetworkLog.NetworkLog.Events.Reset,
this._reset.bind(this));
On 2017/06/01 20:56:27, dgozman wrote:
> this._reset, this

Done.

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:834:
while (this._staleRequests.size) {
On 2017/06/01 20:56:27, dgozman wrote:
> while (this._staleRequests.size) {
>   var request = this._staleRequests.firstValue();
>   this._staleRequests.delete(request);
>   ...
> }

Done.

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:900:
this.didRefreshForTest();
On 2017/06/01 20:56:27, dgozman wrote:
> _didRefreshForTest

Done.

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:968: //
Pull all the redirects of the main request upon commit load.
On 2017/06/01 20:56:27, dgozman wrote:
> You don't check that request is a main request.

This comment was moved and git made a horrible diff here... I removed the
comment since the code is self explanatory.

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:980:
while (node && node.parent) {
On 2017/06/01 20:56:27, dgozman wrote:
> Why do you remove all parents? This removes the group completely, right?

good catch.

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js
(left):

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js:262:
this._reset();
On 2017/06/01 20:56:28, dgozman wrote:
> Where did this go?

It's in NetworkLog now and _onNetworkLogReset gets called when NetworkLog gets
cleared.

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js
(right):

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:251: if
(!Common.moduleSetting('network.preserve-log').get())
On 2017/06/01 20:56:28, dgozman wrote:
> The setting should move to network_log module.

Done.

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:358:
reset() {
On 2017/06/01 20:56:28, dgozman wrote:
> Why is this public?

NetworkPanel clears the log with a UI button.

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:360:
this._requestsByManagerAndId.forEach(map => map.clear());
On 2017/06/01 20:56:28, dgozman wrote:
> Just clear the map itself instead?

This is going away in a (soon) followup patch. But for now it needs to keep the
current targets as keys.

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:361: var
networkManagers = new Set(
On 2017/06/01 20:56:28, dgozman wrote:
> var networkManagers = SDK.targetManager.models(SDK.NetworkManager);

Done.

https://codereview.chromium.org/2914033002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:384:
this._mainRequest = mainRequest;
On 2017/06/01 20:56:28, dgozman wrote:
> This is a struct, where all fields should be public.

Done.

[dgozman, 2017-06-02 21:13:38]

https://codereview.chromium.org/2914033002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js
(right):

https://codereview.chromium.org/2914033002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:979:
parent.removeChild(node);
I think this will be slow on navigation, since we are going to remove one-by-one
and update DOM? Let's batch the removals?

https://codereview.chromium.org/2914033002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js
(right):

https://codereview.chromium.org/2914033002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:91:
setRecording(enabled) {
setIsRecording

https://codereview.chromium.org/2914033002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:286:
this._requests = filteredRequests;
this._requests = this._requests.filter(request => request.networkManager() !==
networkManager);

https://codereview.chromium.org/2914033002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:303:
this.dispatchEventToListeners(NetworkLog.NetworkLog.Events.RequestAdded,
request);
Let's not dispatch RequestRemoved and then RequestAdded for these requests, and
instead only dispatch removed for actually removed ones.

https://codereview.chromium.org/2914033002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:330: if
(!this._isRecording)
Instead of checking all the time, should we start/stop listening to events
instead?

[allada, 2017-06-03 00:27:41]

PTaL

https://codereview.chromium.org/2914033002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js
(right):

https://codereview.chromium.org/2914033002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:979:
parent.removeChild(node);
On 2017/06/02 21:13:37, dgozman wrote:
> I think this will be slow on navigation, since we are going to remove
one-by-one
> and update DOM? Let's batch the removals?

Done.

https://codereview.chromium.org/2914033002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js
(right):

https://codereview.chromium.org/2914033002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:91:
setRecording(enabled) {
On 2017/06/02 21:13:38, dgozman wrote:
> setIsRecording

Done.

https://codereview.chromium.org/2914033002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:286:
this._requests = filteredRequests;
On 2017/06/02 21:13:38, dgozman wrote:
> this._requests = this._requests.filter(request => request.networkManager() !==
> networkManager);

Done.

https://codereview.chromium.org/2914033002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:303:
this.dispatchEventToListeners(NetworkLog.NetworkLog.Events.RequestAdded,
request);
On 2017/06/02 21:13:38, dgozman wrote:
> Let's not dispatch RequestRemoved and then RequestAdded for these requests,
and
> instead only dispatch removed for actually removed ones.

Done.

https://codereview.chromium.org/2914033002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:330: if
(!this._isRecording)
On 2017/06/02 21:13:38, dgozman wrote:
> Instead of checking all the time, should we start/stop listening to events
> instead?

Done.

[allada, 2017-06-07 23:23:53]

The CQ bit was checked by allada@chromium.org to run a CQ dry run

[allada, 2017-06-07 23:24:00]

PTaL

[commit-bot: I haz the power, 2017-06-07 23:24:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[allada, 2017-06-07 23:37:37]

The CQ bit was checked by allada@chromium.org to run a CQ dry run

[allada, 2017-06-07 23:37:47]

Patchset #7 (id:180001) has been deleted

[commit-bot: I haz the power, 2017-06-07 23:38:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-08 01:21:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-08 01:21:09]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-06-08 18:25:17]

https://codereview.chromium.org/2914033002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js
(right):

https://codereview.chromium.org/2914033002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:990: if
(parent.children.length > 1)
This won't ever remove parents with 2 children, since you postpone the actual
removing until later.

[allada, 2017-06-08 21:06:51]

PTaL

https://codereview.chromium.org/2914033002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js
(right):

https://codereview.chromium.org/2914033002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:990: if
(parent.children.length > 1)
On 2017/06/08 18:25:17, dgozman wrote:
> This won't ever remove parents with 2 children, since you postpone the actual
> removing until later.

Good catch, I forgot to check if the children were already tagged to be removed.

[allada, 2017-06-08 21:09:44]

Patchset #8 (id:220001) has been deleted

[allada, 2017-06-08 21:22:41]

Patchset #9 (id:260001) has been deleted

[allada, 2017-06-08 21:22:47]

Patchset #8 (id:240001) has been deleted

[allada, 2017-06-08 21:24:25]

https://codereview.chromium.org/2914033002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network/NetworkFrameGrouper.js
(right):

https://codereview.chromium.org/2914033002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/NetworkFrameGrouper.js:14:
this._frameGroupNodeSymbol = Symbol('FrameGroupNode');
I needed to change this because a frame may now go away and we no longer clear
everything and now instead get notified about requests that were removed which
makes calling reset() difficult. To make it easier I let the garbage collector
deal with it.

[allada, 2017-06-08 21:24:25]

https://codereview.chromium.org/2914033002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network/NetworkFrameGrouper.js
(right):

https://codereview.chromium.org/2914033002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/NetworkFrameGrouper.js:14:
this._frameGroupNodeSymbol = Symbol('FrameGroupNode');
I needed to change this because a frame may now go away and we no longer clear
everything and now instead get notified about requests that were removed which
makes calling reset() difficult. To make it easier I let the garbage collector
deal with it.

[allada, 2017-06-08 22:30:17]

The CQ bit was checked by allada@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-08 22:30:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 01:04:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-09 01:04:39]

Dry run: This issue passed the CQ dry run.

[allada, 2017-06-12 21:59:19]

PTaL - This should have fixed service worker issues and also the overview
timeline issues. The top overview timeline should now reset and be in-sync now
with the waterfall.

[allada, 2017-06-14 18:42:34]

PTaL

[dgozman, 2017-06-14 22:21:48]

https://codereview.chromium.org/2914033002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js
(right):

https://codereview.chromium.org/2914033002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:515:
recordNode.textContent =
UI.shortcutRegistry.shortcutTitleForAction('network_log.toggle-recording');
This does not work.

https://codereview.chromium.org/2914033002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:968:
node[Network.NetworkLogView._isFilteredOutSymbol] = true;
Why filtered by default?

https://codereview.chromium.org/2914033002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:979:
_onRequestsRemoved(event) {
As discussed offline, let's recreate everything in next refresh.

https://codereview.chromium.org/2914033002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:987: if
(parent === rootNode || !parent.children.every(child =>
this._nodesToRemove.has(child)))
I think this is still O(n^2).

https://codereview.chromium.org/2914033002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js
(right):

https://codereview.chromium.org/2914033002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js:179:
this._preserveLogSetting = Common.moduleSetting('network_log.preserve-log');
This does not work, right?

https://codereview.chromium.org/2914033002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js
(right):

https://codereview.chromium.org/2914033002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:309:
this._addRequest(redirectRequest, idMap, currentPageLoad);
Let's just inline two lines from _addRequest instead?

https://codereview.chromium.org/2914033002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:410:
this.dispatchEventToListeners(NetworkLog.NetworkLog.Events.Reset);
I think we can issue RequestsRemoved with all requests.

[allada, 2017-06-15 00:19:48]

PTaL

https://codereview.chromium.org/2914033002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js
(right):

https://codereview.chromium.org/2914033002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:515:
recordNode.textContent =
UI.shortcutRegistry.shortcutTitleForAction('network_log.toggle-recording');
On 2017/06/14 22:21:47, dgozman wrote:
> This does not work.

Done.

https://codereview.chromium.org/2914033002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:968:
node[Network.NetworkLogView._isFilteredOutSymbol] = true;
On 2017/06/14 22:21:47, dgozman wrote:
> Why filtered by default?

This is what it was doing, I did not change the logic.

https://codereview.chromium.org/2914033002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:979:
_onRequestsRemoved(event) {
On 2017/06/14 22:21:47, dgozman wrote:
> As discussed offline, let's recreate everything in next refresh.

Done.

https://codereview.chromium.org/2914033002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:987: if
(parent === rootNode || !parent.children.every(child =>
this._nodesToRemove.has(child)))
On 2017/06/14 22:21:47, dgozman wrote:
> I think this is still O(n^2).

Acknowledged.

https://codereview.chromium.org/2914033002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js
(right):

https://codereview.chromium.org/2914033002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/NetworkPanel.js:179:
this._preserveLogSetting = Common.moduleSetting('network_log.preserve-log');
On 2017/06/14 22:21:47, dgozman wrote:
> This does not work, right?

Done.

https://codereview.chromium.org/2914033002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js
(right):

https://codereview.chromium.org/2914033002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:309:
this._addRequest(redirectRequest, idMap, currentPageLoad);
On 2017/06/14 22:21:47, dgozman wrote:
> Let's just inline two lines from _addRequest instead?

Done.

https://codereview.chromium.org/2914033002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:410:
this.dispatchEventToListeners(NetworkLog.NetworkLog.Events.Reset);
On 2017/06/14 22:21:47, dgozman wrote:
> I think we can issue RequestsRemoved with all requests.

Acknowledged.

[dgozman, 2017-06-15 20:24:10]

This looks much better!

https://codereview.chromium.org/2914033002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js
(right):

https://codereview.chromium.org/2914033002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:917:
_reset() {
Out of curiosity, how does this method trigger _refresh? I don't see us
scheduling it anywhere :)

https://codereview.chromium.org/2914033002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:970: //
/**
Remove?

https://codereview.chromium.org/2914033002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js
(left):

https://codereview.chromium.org/2914033002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:83:
return this._requests.find(request => request.url() === url) || null;
It was pretty good before :)

https://codereview.chromium.org/2914033002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js
(right):

https://codereview.chromium.org/2914033002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:310:
request[NetworkLog.NetworkLog._pageLoadForRequestSymbol] = currentPageLoad;
request -> redirectRequest?

https://codereview.chromium.org/2914033002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:311:
this._addRequest(redirectRequest, idMap, currentPageLoad);
Looks like this call does handle the page load and idMap already.

https://codereview.chromium.org/2914033002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:335: *
@param {!Map<string, !SDK.NetworkRequest>=} requestIdMap
remove =

https://codereview.chromium.org/2914033002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:336: *
@param {!NetworkLog.PageLoad=} pageLoad
?NetworkLog.PageLoad

https://codereview.chromium.org/2914033002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:339: if
(this._requestsSet.has(request))
How could this happen?

[allada, 2017-06-17 00:04:14]

PTaL

https://codereview.chromium.org/2914033002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js
(right):

https://codereview.chromium.org/2914033002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:917:
_reset() {
On 2017/06/15 20:24:09, dgozman wrote:
> Out of curiosity, how does this method trigger _refresh? I don't see us
> scheduling it anywhere :)

Looks like it never did call _refresh... I added a scheduleRefresh to the call,
but I believe it was not needed because it directly removes the children from
the datagrid.

https://codereview.chromium.org/2914033002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:970: //
/**
On 2017/06/15 20:24:09, dgozman wrote:
> Remove?

Done.

https://codereview.chromium.org/2914033002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js
(left):

https://codereview.chromium.org/2914033002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:83:
return this._requests.find(request => request.url() === url) || null;
On 2017/06/15 20:24:10, dgozman wrote:
> It was pretty good before :)

Ooops I think this was because I was debugging it. Done.

https://codereview.chromium.org/2914033002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js
(right):

https://codereview.chromium.org/2914033002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:310:
request[NetworkLog.NetworkLog._pageLoadForRequestSymbol] = currentPageLoad;
On 2017/06/15 20:24:09, dgozman wrote:
> request -> redirectRequest?

Done.

https://codereview.chromium.org/2914033002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:311:
this._addRequest(redirectRequest, idMap, currentPageLoad);
On 2017/06/15 20:24:10, dgozman wrote:
> Looks like this call does handle the page load and idMap already.

Done.

https://codereview.chromium.org/2914033002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:335: *
@param {!Map<string, !SDK.NetworkRequest>=} requestIdMap
On 2017/06/15 20:24:10, dgozman wrote:
> remove =

I need it to not update the map or pageload if the request is being preserved. I
also would like to have these optional for when import HAR, because we won't
have a manager and may not have a pageload.

https://codereview.chromium.org/2914033002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:336: *
@param {!NetworkLog.PageLoad=} pageLoad
On 2017/06/15 20:24:10, dgozman wrote:
> ?NetworkLog.PageLoad

ditto.

https://codereview.chromium.org/2914033002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:339: if
(this._requestsSet.has(request))
On 2017/06/15 20:24:09, dgozman wrote:
> How could this happen?
If preserve log is on it may try to add the navigation or redirect request
again.

[dgozman, 2017-06-17 00:12:56]

https://codereview.chromium.org/2914033002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js
(right):

https://codereview.chromium.org/2914033002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:335: *
@param {!Map<string, !SDK.NetworkRequest>=} requestIdMap
On 2017/06/17 00:04:13, allada wrote:
> On 2017/06/15 20:24:10, dgozman wrote:
> > remove =
> 
> I need it to not update the map or pageload if the request is being preserved.
I
> also would like to have these optional for when import HAR, because we won't
> have a manager and may not have a pageload.

Let's inline _addRequest everywhere.

[allada, 2017-06-19 20:48:04]

PTaL

[dgozman, 2017-06-19 23:08:30]

lgtm

https://codereview.chromium.org/2914033002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js
(right):

https://codereview.chromium.org/2914033002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:304:
requestsToAdd = requestsToAdd.concat(request.redirects);
requestsToAdd.pushAll(request.redirects)

[allada, 2017-06-20 02:35:02]

https://codereview.chromium.org/2914033002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js
(right):

https://codereview.chromium.org/2914033002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/network_log/NetworkLog.js:304:
requestsToAdd = requestsToAdd.concat(request.redirects);
On 2017/06/19 23:08:30, dgozman wrote:
> requestsToAdd.pushAll(request.redirects)

Done.

[allada, 2017-06-20 02:35:07]

The CQ bit was checked by allada@chromium.org

[allada, 2017-06-20 02:35:08]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2914033002/#ps440001
(title: "Merge branch 'master' into MOVE_NETWORK_STUFF_AROUND")

[commit-bot: I haz the power, 2017-06-20 02:35:34]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-20 04:17:04]

CQ is committing da patch.

Bot data: {"patchset_id": 440001, "attempt_start_ts": 1497926107962290,
"parent_rev": "e60d1d4ae98c4c916db70cb2f090f59e3ad76a51", "commit_rev":
"ca6a9f1f759e79a3a08be1747c47ee9354db6cff"}

[commit-bot: I haz the power, 2017-06-20 04:17:14]

Description was changed from

==========
[Devtools] NetworkLogView now uses NetworkLog as source of truth.

NetworkLogView will now use NetworkLog to control it's requests.
NetworkLog now will dispatch the needed events and the view will only
update when the NetworkLog has dispatched them.

This is in preporation of loading HAR files into network panel.

R=dgozman,pfeldman
BUG=374557
==========

to

==========
[Devtools] NetworkLogView now uses NetworkLog as source of truth.

NetworkLogView will now use NetworkLog to control it's requests.
NetworkLog now will dispatch the needed events and the view will only
update when the NetworkLog has dispatched them.

This is in preporation of loading HAR files into network panel.

R=dgozman,pfeldman
BUG=374557

Review-Url: https://codereview.chromium.org/2914033002
Cr-Commit-Position: refs/heads/master@{#480728}
Committed:
https://chromium.googlesource.com/chromium/src/+/ca6a9f1f759e79a3a08be1747c47...
==========

[commit-bot: I haz the power, 2017-06-20 04:17:16]

Committed patchset #16 (id:440001) as
https://chromium.googlesource.com/chromium/src/+/ca6a9f1f759e79a3a08be1747c47...