[Devtools] Canvas network timeline experiment establishes scrolling

[Devtools] Canvas network timeline experiment establishes scrolling

This patch fixes the scrolling in canvas timeline experiment.

R=lushnikov,dgozman
BUG=653738
Committed: https://crrev.com/365eb6fa4e31505842375fa762e228eee8ef8183
Cr-Commit-Position: refs/heads/master@{#425236}

[allada, 2016-10-11 21:08:08]

Patchset #1 (id:1) has been deleted

[allada, 2016-10-11 21:25:58]

Patchset #1 (id:20001) has been deleted

[allada, 2016-10-11 21:35:48]

Patchset #1 (id:40001) has been deleted

[allada, 2016-10-11 21:38:35]

Patchset #1 (id:60001) has been deleted

[allada, 2016-10-11 21:38:55]

PTL

[allada, 2016-10-11 21:39:54]

Description was changed from

==========
[Devtools] Canvas timeline experiment scroll fixes

This patch fixes the scrolling in canvas timeline experiment.


R=lushnikov,dgozman
BUG=653738
==========

to

==========
[Devtools] Canvas network timeline experiment scroll fixes

This patch fixes the scrolling in canvas timeline experiment.


R=lushnikov,dgozman
BUG=653738
==========

[dgozman, 2016-10-11 21:53:40]

Description is strange. I find this patch not "fixing" scrolling, but rather
establishing it.

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

https://codereview.chromium.org/2409313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkTimelineColumn.js:73:
_onMouseWheel: function(event)
Annotate please.

https://codereview.chromium.org/2409313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkTimelineColumn.js:75:
this._vScrollElement.scrollTop -= event.wheelDeltaY;
Won't this cause _onScroll itself?

https://codereview.chromium.org/2409313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkTimelineColumn.js:79:
_onScroll: function(event)
Annotate please.

https://codereview.chromium.org/2409313003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui_lazy/DataGrid.js (right):

https://codereview.chromium.org/2409313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui_lazy/DataGrid.js:158:
PaddingChanged: Symbol("PaddingChanged")
HeightChanged or something like that? As a user of DataGrid, I don't know what
this means, especially since there is no direct method which affects this
padding.

Should we actually move this to ViewportDataGrid instead?

https://codereview.chromium.org/2409313003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui_lazy/ViewportDataGrid.js
(right):

https://codereview.chromium.org/2409313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui_lazy/ViewportDataGrid.js:21: //
This is not in setScrollContainer because mouse wheel only should be bound to
content and not another scroller.
I don't get this.

https://codereview.chromium.org/2409313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui_lazy/ViewportDataGrid.js:49:
Updated: Symbol("Updated")
Not very descriptive name.

https://codereview.chromium.org/2409313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui_lazy/ViewportDataGrid.js:60:
this._scrollContainer.addEventListener("scroll", this._onScrollBound, true);
Why capture?

[allada, 2016-10-12 00:10:50]

Description was changed from

==========
[Devtools] Canvas network timeline experiment scroll fixes

This patch fixes the scrolling in canvas timeline experiment.


R=lushnikov,dgozman
BUG=653738
==========

to

==========
[Devtools] Canvas network timeline experiment establishes scrolling

This patch fixes the scrolling in canvas timeline experiment.


R=lushnikov,dgozman
BUG=653738
==========

[allada, 2016-10-12 00:33:09]

PTaL

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

https://codereview.chromium.org/2409313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkTimelineColumn.js:73:
_onMouseWheel: function(event)
On 2016/10/11 21:53:39, dgozman wrote:
> Annotate please.

Done.

https://codereview.chromium.org/2409313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkTimelineColumn.js:75:
this._vScrollElement.scrollTop -= event.wheelDeltaY;
On 2016/10/11 21:53:39, dgozman wrote:
> Won't this cause _onScroll itself?

Yes, I can remove this part:
"this._dataGridScrollContainer.scrollTop = this._vScrollElement.scrollTop;"

However, if I do, I risk the chance of scrolling lagging. I am trying to keep
scrolling in sync as much as possible.

https://codereview.chromium.org/2409313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/network/NetworkTimelineColumn.js:79:
_onScroll: function(event)
On 2016/10/11 21:53:39, dgozman wrote:
> Annotate please.

Done.

https://codereview.chromium.org/2409313003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui_lazy/DataGrid.js (right):

https://codereview.chromium.org/2409313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui_lazy/DataGrid.js:158:
PaddingChanged: Symbol("PaddingChanged")
On 2016/10/11 21:53:39, dgozman wrote:
> HeightChanged or something like that? As a user of DataGrid, I don't know what
> this means, especially since there is no direct method which affects this
> padding.
> 
> Should we actually move this to ViewportDataGrid instead?

There's a strange correlation between DataGrid and ViewportDataGrid. Some of the
code that viewport uses to manage it's viewport lives in datagrid and some of it
lives in it's own code. There are two clients of setVerticalPadding() one is
ViewportDataGrid the other is HeapSnapshotDataGrid.

I am fine renaming this, but I do not feel "HeightChanged" is appropriate
because the height is changing, but it can change without this being invoked.
"PaddingChanged" is what is happening and just happens to be all I need out of
it because it will never add padding if there is no need for a scrollbar.

https://codereview.chromium.org/2409313003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui_lazy/ViewportDataGrid.js
(right):

https://codereview.chromium.org/2409313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui_lazy/ViewportDataGrid.js:21: //
This is not in setScrollContainer because mouse wheel only should be bound to
content and not another scroller.
On 2016/10/11 21:53:40, dgozman wrote:
> I don't get this.

Better?

https://codereview.chromium.org/2409313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui_lazy/ViewportDataGrid.js:49:
Updated: Symbol("Updated")
On 2016/10/11 21:53:39, dgozman wrote:
> Not very descriptive name.

Done.

https://codereview.chromium.org/2409313003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui_lazy/ViewportDataGrid.js:60:
this._scrollContainer.addEventListener("scroll", this._onScrollBound, true);
On 2016/10/11 21:53:39, dgozman wrote:
> Why capture?

It needs it for stick to bottom and a few other things. I just need the ability
to bind it to something else, so I created a "Bound" holder so I can maintain a
reference to the bound function for tieing and untieing.

[dgozman, 2016-10-13 22:57:55]

dgozman@chromium.org changed reviewers:
+ caseq@chromium.org
- lushnikov@chromium.org

[dgozman, 2016-10-13 22:57:56]

Looks good in general, but I'd like caseq@ to take a look at {Viewport}DataGrid.

[caseq, 2016-10-14 00:44:30]

datagrids lgtm!

[allada, 2016-10-14 00:45:26]

The CQ bit was checked by allada@chromium.org

[commit-bot: I haz the power, 2016-10-14 00:45:51]

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, 2016-10-14 02:04:36]

Description was changed from

==========
[Devtools] Canvas network timeline experiment establishes scrolling

This patch fixes the scrolling in canvas timeline experiment.


R=lushnikov,dgozman
BUG=653738
==========

to

==========
[Devtools] Canvas network timeline experiment establishes scrolling

This patch fixes the scrolling in canvas timeline experiment.


R=lushnikov,dgozman
BUG=653738
==========

[commit-bot: I haz the power, 2016-10-14 02:04:37]

Committed patchset #3 (id:120001)

[commit-bot: I haz the power, 2016-10-14 02:07:49]

Description was changed from

==========
[Devtools] Canvas network timeline experiment establishes scrolling

This patch fixes the scrolling in canvas timeline experiment.


R=lushnikov,dgozman
BUG=653738
==========

to

==========
[Devtools] Canvas network timeline experiment establishes scrolling

This patch fixes the scrolling in canvas timeline experiment.

R=lushnikov,dgozman
BUG=653738

Committed: https://crrev.com/365eb6fa4e31505842375fa762e228eee8ef8183
Cr-Commit-Position: refs/heads/master@{#425236}
==========

[commit-bot: I haz the power, 2016-10-14 02:07:51]

Patchset 3 (id:??) landed as
https://crrev.com/365eb6fa4e31505842375fa762e228eee8ef8183
Cr-Commit-Position: refs/heads/master@{#425236}