[DevTools] Make NetworkLog a singleton, and not an SDKModel

[DevTools] Make NetworkLog a singleton, and not an SDKModel

NetworkLog is a presentation model, which keeps track of interesting
network requests (between page loads actually) for our UI.
It is also created in TargetManager which is a layering violation.

This patch makes it a separate entity which can even be extracted
out of SDK.

BUG=none

Review-Url: https://codereview.chromium.org/2749043005
Cr-Commit-Position: refs/heads/master@{#457663}
Committed: https://chromium.googlesource.com/chromium/src/+/e8fb18a01d4fdfaa6c3007df47bb7d2ba709afed

[dgozman, 2017-03-16 19:00:53]

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

[dgozman, 2017-03-16 19:01:38]

dgozman@chromium.org changed reviewers:
+ allada@chromium.org, caseq@chromium.org

[dgozman, 2017-03-16 19:01:39]

Could you please take a look at this speculative patch? WDYT?

[commit-bot: I haz the power, 2017-03-16 19:01:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[caseq, 2017-03-16 20:27:47]

https://codereview.chromium.org/2749043005/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js (right):

https://codereview.chromium.org/2749043005/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:83: for (var i =
0; i < this._requests.length; ++i) {
return this._requests.find(request => request.url() === url)

https://codereview.chromium.org/2749043005/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:102: return
this._requests.filter(request => request.target() === target);
return Array.from(this._requestsByTargetAndId.get(target) || [])

https://codereview.chromium.org/2749043005/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:112: if
(this._requests[i].target() === target && this._requests[i].url() === url)
Iterate over this._requestsByTargetAndId.get(target)?

https://codereview.chromium.org/2749043005/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:186: if
(logRequest.target() !== request.target())
ditto

https://codereview.chromium.org/2749043005/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:262:
this._currentPageLoad.set(target, currentPageLoad);
Why can we have more than one?

[dgozman, 2017-03-16 20:51:21]

https://codereview.chromium.org/2749043005/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js (right):

https://codereview.chromium.org/2749043005/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:83: for (var i =
0; i < this._requests.length; ++i) {
On 2017/03/16 20:27:47, caseq wrote:
> return this._requests.find(request => request.url() === url)

Done.

https://codereview.chromium.org/2749043005/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:102: return
this._requests.filter(request => request.target() === target);
On 2017/03/16 20:27:47, caseq wrote:
> return Array.from(this._requestsByTargetAndId.get(target) || [])

Done.

https://codereview.chromium.org/2749043005/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:112: if
(this._requests[i].target() === target && this._requests[i].url() === url)
On 2017/03/16 20:27:47, caseq wrote:
> Iterate over this._requestsByTargetAndId.get(target)?

Done.

https://codereview.chromium.org/2749043005/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:186: if
(logRequest.target() !== request.target())
On 2017/03/16 20:27:47, caseq wrote:
> ditto

Done.

https://codereview.chromium.org/2749043005/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:262:
this._currentPageLoad.set(target, currentPageLoad);
On 2017/03/16 20:27:47, caseq wrote:
> Why can we have more than one?

One per target in case of multiple targets?

[dgozman, 2017-03-16 20:51:37]

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

[commit-bot: I haz the power, 2017-03-16 20:52:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[caseq, 2017-03-16 20:53:35]

lgtm

[allada, 2017-03-16 21:58:01]

https://codereview.chromium.org/2749043005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js (right):

https://codereview.chromium.org/2749043005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:37:
this._requests = [];
It looks like we are only using this._requests to search for urls and to return
an array of all requests. In a followup lets make this a map of: Map<url,
!SDK.NetworkRequest>. Then when we need to get the underlying array extract the
values and then it is O(1) to search for URLs [which right now is O(n)].

https://codereview.chromium.org/2749043005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:38: /** @type
{!Map<!SDK.Target, !Map<string, !SDK.NetworkRequest>>} */
Lets follow up with adding a new map type for multi-key maps. eg:
!MultiKeyMap<!SDK.Target, string, !SDK.NetworkRequest>

https://codereview.chromium.org/2749043005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:99: return map ?
Array.from(map.values()) : [];
We should be using: map.valuesArray() instead.

https://codereview.chromium.org/2749043005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:209: while
(checkRequest) {
Can we this instead:
do {
 ...
} while (checkRequest);

Because we always know we will run the loop one time and makes it more explicit.

https://codereview.chromium.org/2749043005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:242: var
mainFrame = /** type {!SDK.ResourceTreeFrame} */ (event.data);
Doctype is wrong. Missing "@".

https://codereview.chromium.org/2749043005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:257:
this._requestsByTargetAndId.get(target).set(request.requestId(), request);
We are forcing a lookup on the map on every request here when we know the map is
always the same.

Lets define the map as a local variable on line 247 and set it, then use the map
created there on this line. This will also relieve some confusion like "'target"
might be changing here, so it's looking up the target" when in reality target
never changes.

https://codereview.chromium.org/2749043005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:290:
pageLoad.contentLoadTime = event.data;
Lets document this below in SDK.PageLoad of what type it is.

https://codereview.chromium.org/2749043005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:300:
pageLoad.loadTime = event.data;
dido.

https://codereview.chromium.org/2749043005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:315: *
@unrestricted
We should be able to remove unrestricted if we do above items.

https://codereview.chromium.org/2749043005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js (right):

https://codereview.chromium.org/2749043005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:247:
target.model(SDK.NetworkManager);
I don't like this, I'd like to also like to follow up with a "registerModel" or
"createModelIfNeeded". This is not very descriptive on what's happening.

[caseq, 2017-03-16 22:13:11]

On 2017/03/16 21:58:01, allada wrote:
>
https://codereview.chromium.org/2749043005/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:99: return map
?
> Array.from(map.values()) : [];
> We should be using: map.valuesArray() instead.

Not really. Check out where and how Map.prototype.valuesArray() is defined :-)

[commit-bot: I haz the power, 2017-03-16 22:16:17]

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

[commit-bot: I haz the power, 2017-03-16 22:16:19]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-03-17 00:51:04]

https://codereview.chromium.org/2749043005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js (right):

https://codereview.chromium.org/2749043005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:209: while
(checkRequest) {
On 2017/03/16 21:58:00, allada wrote:
> Can we this instead:
> do {
>  ...
> } while (checkRequest);
> 
> Because we always know we will run the loop one time and makes it more
explicit.

Done.

https://codereview.chromium.org/2749043005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:242: var
mainFrame = /** type {!SDK.ResourceTreeFrame} */ (event.data);
On 2017/03/16 21:58:01, allada wrote:
> Doctype is wrong. Missing "@".

Done.

https://codereview.chromium.org/2749043005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:257:
this._requestsByTargetAndId.get(target).set(request.requestId(), request);
On 2017/03/16 21:58:00, allada wrote:
> We are forcing a lookup on the map on every request here when we know the map
is
> always the same.
> 
> Lets define the map as a local variable on line 247 and set it, then use the
map
> created there on this line. This will also relieve some confusion like
"'target"
> might be changing here, so it's looking up the target" when in reality target
> never changes.

Done.

https://codereview.chromium.org/2749043005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:290:
pageLoad.contentLoadTime = event.data;
On 2017/03/16 21:58:00, allada wrote:
> Lets document this below in SDK.PageLoad of what type it is.

Done.

https://codereview.chromium.org/2749043005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:300:
pageLoad.loadTime = event.data;
On 2017/03/16 21:58:01, allada wrote:
> dido.

Done.

https://codereview.chromium.org/2749043005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkLog.js:315: *
@unrestricted
On 2017/03/16 21:58:01, allada wrote:
> We should be able to remove unrestricted if we do above items.

Done.

[dgozman, 2017-03-17 00:51:09]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2017-03-17 00:51:10]

The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org
Link to the patchset: https://codereview.chromium.org/2749043005/#ps40001
(title: "review comments addressed")

[commit-bot: I haz the power, 2017-03-17 00:51:37]

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-03-17 02:37:36]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489711869710170,
"parent_rev": "cd936213338a5b35605840b9325f6eca5479bbf7", "commit_rev":
"e8fb18a01d4fdfaa6c3007df47bb7d2ba709afed"}

[commit-bot: I haz the power, 2017-03-17 02:38:21]

Description was changed from

==========
[DevTools] Make NetworkLog a singleton, and not an SDKModel

NetworkLog is a presentation model, which keeps track of interesting
network requests (between page loads actually) for our UI.
It is also created in TargetManager which is a layering violation.

This patch makes it a separate entity which can even be extracted
out of SDK.

BUG=none
==========

to

==========
[DevTools] Make NetworkLog a singleton, and not an SDKModel

NetworkLog is a presentation model, which keeps track of interesting
network requests (between page loads actually) for our UI.
It is also created in TargetManager which is a layering violation.

This patch makes it a separate entity which can even be extracted
out of SDK.

BUG=none

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

[commit-bot: I haz the power, 2017-03-17 02:38:22]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/e8fb18a01d4fdfaa6c3007df47bb...