DevTools: group by frame, not product in the network panel.

DevTools: group by frame, not product in the network panel.

Review-Url: https://codereview.chromium.org/2876983002
Cr-Commit-Position: refs/heads/master@{#471961}
Committed: https://chromium.googlesource.com/chromium/src/+/9bb9db9d916f1c395b86844de7893f0c43a00fd1

[pfeldman, 2017-05-11 23:29:39]

pfeldman@chromium.org changed reviewers:
+ allada@chromium.org

[allada, 2017-05-11 23:58:37]

Can we also please add a better description and a screenshot or two?

https://codereview.chromium.org/2876983002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js
(left):

https://codereview.chromium.org/2876983002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:376:
columnConfig = this._addCustomHeader(setting.title, columnId);
This should still be needed.

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

https://codereview.chromium.org/2876983002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:535:
if (hoveredNode instanceof Network.FrameGroupNode) {
Lets only have the popover for the name column.

https://codereview.chromium.org/2876983002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:535:
if (hoveredNode instanceof Network.FrameGroupNode) {
Also, can we add a TODO to not use instanceof and instead expose a better api
for this?

https://codereview.chromium.org/2876983002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/network/networkPanel.css
(right):

https://codereview.chromium.org/2876983002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/network/networkPanel.css:128:
.data-grid tr td:first-child {
These should all go into networkLogView.css

https://codereview.chromium.org/2876983002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/network/networkPanel.css:128:
.data-grid tr td:first-child {
This should be something like:

.network-log-grid.data-grid tr td:first-child

[pfeldman, 2017-05-12 23:16:20]

https://codereview.chromium.org/2876983002/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js
(left):

https://codereview.chromium.org/2876983002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:376:
columnConfig = this._addCustomHeader(setting.title, columnId);
On 2017/05/11 23:58:36, allada wrote:
> This should still be needed.

What does it do? I noticed that it only breaks things, never fixes anything.

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

https://codereview.chromium.org/2876983002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:535:
if (hoveredNode instanceof Network.FrameGroupNode) {
On 2017/05/11 23:58:36, allada wrote:
> Lets only have the popover for the name column.

I'll have a new patch for the rest...

[pfeldman, 2017-05-12 23:52:36]

pfeldman@chromium.org changed reviewers:
+ dgozman@chromium.org

[pfeldman, 2017-05-12 23:52:37]

PTAL

[dgozman, 2017-05-13 00:32:37]

lgtm

https://codereview.chromium.org/2876983002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/NetworkFrameGrouper.js
(right):

https://codereview.chromium.org/2876983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkFrameGrouper.js:67:
return new Common.ParsedURL(this._frame.url).domain() || this._frame.name ||
'<iframe>';
Check for isValid().

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

https://codereview.chromium.org/2876983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:124:
Common.moduleSetting('network.group-by-frame').addChangeListener(() =>
this._updateGroupByFrame());
(..., this._updateGroupByFrame, this)

https://codereview.chromium.org/2876983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:129:
this.contentElement.classList.toggle('network-group-by-frame', value);
Is this class ever used?

https://codereview.chromium.org/2876983002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/module.json (right):

https://codereview.chromium.org/2876983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/module.json:53: "tags":
"network, frame, group",
"experiment": "...."

[allada, 2017-05-13 01:10:06]

Update description and title and it's lgtm.

[pfeldman, 2017-05-15 14:53:03]

https://codereview.chromium.org/2876983002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/NetworkFrameGrouper.js
(right):

https://codereview.chromium.org/2876983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkFrameGrouper.js:67:
return new Common.ParsedURL(this._frame.url).domain() || this._frame.name ||
'<iframe>';
On 2017/05/13 00:32:34, dgozman wrote:
> Check for isValid().

This is iframe navigated url, it is always valid.

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

https://codereview.chromium.org/2876983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:124:
Common.moduleSetting('network.group-by-frame').addChangeListener(() =>
this._updateGroupByFrame());
I am now using these, because I think performance-wise they are same and
readability-wise, it is better.

https://codereview.chromium.org/2876983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogView.js:129:
this.contentElement.classList.toggle('network-group-by-frame', value);
On 2017/05/13 00:32:34, dgozman wrote:
> Is this class ever used?

Done.

https://codereview.chromium.org/2876983002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/network/module.json (right):

https://codereview.chromium.org/2876983002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/module.json:53: "tags":
"network, frame, group",
On 2017/05/13 00:32:35, dgozman wrote:
> "experiment": "...."

There is none!

[pfeldman, 2017-05-15 20:14:36]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2017-05-15 20:14:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, allada@chromium.org
Link to the patchset: https://codereview.chromium.org/2876983002/#ps80001
(title: "review comments")

[commit-bot: I haz the power, 2017-05-15 20:15: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-05-15 21:27:56]

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

[commit-bot: I haz the power, 2017-05-15 21:27:57]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[allada, 2017-05-15 21:38:01]

The CQ bit was checked by allada@chromium.org

[commit-bot: I haz the power, 2017-05-15 21:39:04]

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-05-16 00:52:50]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494884281037700,
"parent_rev": "abb90a5c3d4a074c35fb3b60fdeddda93aaef865", "commit_rev":
"9bb9db9d916f1c395b86844de7893f0c43a00fd1"}

[commit-bot: I haz the power, 2017-05-16 00:53:00]

Description was changed from

==========
DevTools: group by frame, not product in the network panel.
==========

to

==========
DevTools: group by frame, not product in the network panel.

Review-Url: https://codereview.chromium.org/2876983002
Cr-Commit-Position: refs/heads/master@{#471961}
Committed:
https://chromium.googlesource.com/chromium/src/+/9bb9db9d916f1c395b86844de789...
==========

[commit-bot: I haz the power, 2017-05-16 00:53:01]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/9bb9db9d916f1c395b86844de789...