[Devtools] Handle network sorting of grouped nodes properly

[Devtools] Handle network sorting of grouped nodes properly

This patch changes the logic of how grouped nodes are sorted to use first
known request in a group; however, sorting by name will first sort nodes by
weather they are grouped then sort them by name. This will ensure that
group nodes are together and non-group nodes are together but sorrted within
their partition.

R=dgozman,luoe,caseq
BUG=664704
Review-Url: https://codereview.chromium.org/2648853002
Cr-Commit-Position: refs/heads/master@{#446168}
Committed: https://chromium.googlesource.com/chromium/src/+/f70665f72a40d378c7bda5fc3aa6aec33dc93919

[allada, 2017-01-20 23:15:44]

PTL

[dgozman, 2017-01-23 20:32:40]

https://codereview.chromium.org/2648853002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js
(right):

https://codereview.chromium.org/2648853002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:165:
requests.reduce((a, b) => (!b || (a && a.issueTime() < b.issueTime())) ? a : b);
Let's do a for-loop. We can even remove .map() above.

https://codereview.chromium.org/2648853002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:197:
return aGroupNode ? 1 : -1;
This puts groups after non-groups. We should do the opposite.

[allada, 2017-01-24 22:54:08]

PTaL

https://codereview.chromium.org/2648853002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js
(right):

https://codereview.chromium.org/2648853002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:165:
requests.reduce((a, b) => (!b || (a && a.issueTime() < b.issueTime())) ? a : b);
On 2017/01/23 20:32:40, dgozman wrote:
> Let's do a for-loop. We can even remove .map() above.

Done.

https://codereview.chromium.org/2648853002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:197:
return aGroupNode ? 1 : -1;
On 2017/01/23 20:32:40, dgozman wrote:
> This puts groups after non-groups. We should do the opposite.

I believe users want to see their resources before grouped resources. Chris and
I talked about this, we are going to iterate over the details, but we did agree
that we do not want to mix groups and non-groups.

[luoe, 2017-01-24 23:56:55]

Looks good.  Is there a test for sorting with groups?

https://codereview.chromium.org/2648853002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js
(right):

https://codereview.chromium.org/2648853002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:159:
if (request || !this.hasChildren()) {
Do we need the !this.hasChildren() check?  This makes sense if you want to
return early, but nodes with no request can still be handled by the for loop
below.

https://codereview.chromium.org/2648853002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:209:
bName = bGroupNode.displayName();
NetworkNodes don't have displayNames, but NetworkGroupNodes do.  Closure may not
catch this, but perhaps adding casts to a/bGroupNode might still be worthwhile.

https://codereview.chromium.org/2648853002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:220:
return aRequest.indentityCompare(bRequest);
nit: "identity"?

[allada, 2017-01-25 00:12:59]

PTaL

https://codereview.chromium.org/2648853002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js
(right):

https://codereview.chromium.org/2648853002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:159:
if (request || !this.hasChildren()) {
On 2017/01/24 23:56:54, luoe wrote:
> Do we need the !this.hasChildren() check?  This makes sense if you want to
> return early, but nodes with no request can still be handled by the for loop
> below.

In the future it's very likely nodes may have a request() and children or no
children. This is to ensure we always give the parent the advantage.

https://codereview.chromium.org/2648853002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:209:
bName = bGroupNode.displayName();
On 2017/01/24 23:56:54, luoe wrote:
> NetworkNodes don't have displayNames, but NetworkGroupNodes do.  Closure may
not
> catch this, but perhaps adding casts to a/bGroupNode might still be
worthwhile.

Correct, that's why it uses "asGroupNode()" it ensures it's using it as a
NetworkGroupNode.

https://codereview.chromium.org/2648853002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkDataGridNode.js:220:
return aRequest.indentityCompare(bRequest);
On 2017/01/24 23:56:54, luoe wrote:
> nit: "identity"?

Yes, but not relevant to this patch.

[luoe, 2017-01-25 00:17:41]

Acknowledged, lgtm

[allada, 2017-01-25 00:25:04]

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

[commit-bot: I haz the power, 2017-01-25 00:25:50]

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-01-25 03:19:22]

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

[commit-bot: I haz the power, 2017-01-25 03:19:23]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-01-25 23:59:17]

lgtm

[allada, 2017-01-26 00:03:47]

The CQ bit was checked by allada@chromium.org

[allada, 2017-01-26 00:03:48]

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

[commit-bot: I haz the power, 2017-01-26 00:04:25]

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-01-26 00:10:15]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485389027987970,
"parent_rev": "b0986378bcdb6e01f0cdb1dc5a828a7873cda144", "commit_rev":
"f70665f72a40d378c7bda5fc3aa6aec33dc93919"}

[commit-bot: I haz the power, 2017-01-26 00:10:50]

Description was changed from

==========
[Devtools] Handle network sorting of grouped nodes properly

This patch changes the logic of how grouped nodes are sorted to use first
known request in a group; however, sorting by name will first sort nodes by
weather they are grouped then sort them by name. This will ensure that
group nodes are together and non-group nodes are together but sorrted within
their partition.

R=dgozman,luoe,caseq
BUG=664704
==========

to

==========
[Devtools] Handle network sorting of grouped nodes properly

This patch changes the logic of how grouped nodes are sorted to use first
known request in a group; however, sorting by name will first sort nodes by
weather they are grouped then sort them by name. This will ensure that
group nodes are together and non-group nodes are together but sorrted within
their partition.

R=dgozman,luoe,caseq
BUG=664704

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

[commit-bot: I haz the power, 2017-01-26 00:10:51]

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