[Devtools] Fixed scrolling in network panel

[Devtools] Fixed scrolling in network panel

This patch fixes a minor regression which caused the arrows to not allow
users to scroll when scroll was in stick to bottom mode.

R=dgozman,luoe
BUG=683813
Review-Url: https://codereview.chromium.org/2654913003
Cr-Commit-Position: refs/heads/master@{#447073}
Committed: https://chromium.googlesource.com/chromium/src/+/80341e502bc01bdab13c7e69c2d713150a9f833b

[allada, 2017-01-24 22:39:11]

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

[allada, 2017-01-24 22:39:20]

PTL

[commit-bot: I haz the power, 2017-01-24 22:40:06]

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 00:40:25]

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

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

Dry run: This issue passed the CQ dry run.

[luoe, 2017-01-25 17:07:06]

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

https://codereview.chromium.org/2654913003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:179:
scrollDifference = -3;
If we check just based on scrollDifference, we may apply this check even when
scrolling a small amount in the middle.  Maybe we only want to do this when
adding the difference would move it too close to the bottom?

var wouldBeCloseToBottom = dataGridScroller.scrollHeight -
(dataGridScroller.scrollTop - scrollDifference) <= 2;
if (wouldBeCloseToBottom) {
    if (scrollDifference < 0) // jump to bottom
    else if (scrolllDifference > 0) // use scrollDifference - 3
}

[allada, 2017-01-25 22:38:54]

PTaL

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

https://codereview.chromium.org/2654913003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/network/NetworkLogViewColumns.js:179:
scrollDifference = -3;
On 2017/01/25 17:07:06, luoe wrote:
> If we check just based on scrollDifference, we may apply this check even when
> scrolling a small amount in the middle.  Maybe we only want to do this when
> adding the difference would move it too close to the bottom?
> 
> var wouldBeCloseToBottom = dataGridScroller.scrollHeight -
> (dataGridScroller.scrollTop - scrollDifference) <= 2;
> if (wouldBeCloseToBottom) {
>     if (scrollDifference < 0) // jump to bottom
>     else if (scrolllDifference > 0) // use scrollDifference - 3
> }

Done.

[pfeldman, 2017-01-25 23:15:14]

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

[pfeldman, 2017-01-25 23:15:15]

https://codereview.chromium.org/2654913003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js
(right):

https://codereview.chromium.org/2654913003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js:107:
this._updateAnimationFrameId =
this.element.window().requestAnimationFrame(this._update.bind(this,
isFromUser));
Are we reusing stick-to-bottom semantics between console and network?

[allada, 2017-01-25 23:44:45]

https://codereview.chromium.org/2654913003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js
(right):

https://codereview.chromium.org/2654913003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js:107:
this._updateAnimationFrameId =
this.element.window().requestAnimationFrame(this._update.bind(this,
isFromUser));
On 2017/01/25 23:15:15, pfeldman wrote:
> Are we reusing stick-to-bottom semantics between console and network?

The only thing we are sharing that I know of is the
Element.prototype.isScrolledToBottom() function.

[pfeldman, 2017-01-26 00:37:56]

https://codereview.chromium.org/2654913003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js
(right):

https://codereview.chromium.org/2654913003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js:102:
this.element.window().cancelAnimationFrame(this._updateAnimationFrameId);
What if this._updateAnimationFrameId was not set?
Also, you are rescheduling with the bit set upon user gesture? That is
equivalent to flipping a bit, no need to cancel and re-schedule.

https://codereview.chromium.org/2654913003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js:186:
if (!isFromUser && this._stickToBottom && this._atBottom)
What if user is trying to make it stick to bottom?

[allada, 2017-01-26 19:05:54]

PTaL

https://codereview.chromium.org/2654913003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js
(right):

https://codereview.chromium.org/2654913003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js:102:
this.element.window().cancelAnimationFrame(this._updateAnimationFrameId);
On 2017/01/26 00:37:56, pfeldman wrote:
> What if this._updateAnimationFrameId was not set?
> Also, you are rescheduling with the bit set upon user gesture? That is
> equivalent to flipping a bit, no need to cancel and re-schedule.

Aaah, forgot to add to if statement. I was contemplating flipping a bit but went
this way to contain most of the code within this function and not need anywhere
else to remember to reset the value. I changed it to use a bit flag, but I feel
keeping the complexity only in this function is better.

https://codereview.chromium.org/2654913003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js:186:
if (!isFromUser && this._stickToBottom && this._atBottom)
On 2017/01/26 00:37:56, pfeldman wrote:
> What if user is trying to make it stick to bottom?

I could rename it to "ignoreStickToBottom", but in this case a "isFromUser" is
only dispatched when a user scroll happens. If the user scrolls we never want it
to stick to bottom.

[pfeldman, 2017-01-27 18:39:29]

> I could rename it to "ignoreStickToBottom", but in this case a "isFromUser" is
> only dispatched when a user scroll happens. If the user scrolls we never want
it
> to stick to bottom.

When the user scrolls to bottom, we always want to turn into stick-to-bottom
mode.

[allada, 2017-01-27 19:57:38]

On 2017/01/27 18:39:29, pfeldman wrote:
> > I could rename it to "ignoreStickToBottom", but in this case a "isFromUser"
is
> > only dispatched when a user scroll happens. If the user scrolls we never
want
> it
> > to stick to bottom.
> 
> When the user scrolls to bottom, we always want to turn into stick-to-bottom
> mode.

Yes, it keeps that logic. This simply bypasses the check if it's stick to bottom
if user performs a scroll action.

[pfeldman, 2017-01-27 20:21:47]

lgtm

https://codereview.chromium.org/2654913003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js
(right):

https://codereview.chromium.org/2654913003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js:179:
this._updateIsFromUser = false;
It still has a race when previous _update is clearing the user gesture, but I
hope it is negligible.

[allada, 2017-01-27 21:32:17]

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

[commit-bot: I haz the power, 2017-01-27 21:32:49]

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

[allada, 2017-01-27 21:33:31]

https://codereview.chromium.org/2654913003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js
(right):

https://codereview.chromium.org/2654913003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/data_grid/ViewportDataGrid.js:179:
this._updateIsFromUser = false;
On 2017/01/27 20:21:47, pfeldman wrote:
> It still has a race when previous _update is clearing the user gesture, but I
> hope it is negligible.

Acknowledged.

[commit-bot: I haz the power, 2017-01-28 00:52:41]

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

[commit-bot: I haz the power, 2017-01-28 00:52:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[allada, 2017-01-30 18:48:50]

The CQ bit was checked by allada@chromium.org

[commit-bot: I haz the power, 2017-01-30 18:49:23]

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-30 21:37:30]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485802130509280,
"parent_rev": "726ab49809891d09d5d6f70324786c18a9f2cf3c", "commit_rev":
"80341e502bc01bdab13c7e69c2d713150a9f833b"}

[commit-bot: I haz the power, 2017-01-30 21:38:09]

Description was changed from

==========
[Devtools] Fixed scrolling in network panel

This patch fixes a minor regression which caused the arrows to not allow
users to scroll when scroll was in stick to bottom mode.

R=dgozman,luoe
BUG=683813
==========

to

==========
[Devtools] Fixed scrolling in network panel

This patch fixes a minor regression which caused the arrows to not allow
users to scroll when scroll was in stick to bottom mode.

R=dgozman,luoe
BUG=683813

Review-Url: https://codereview.chromium.org/2654913003
Cr-Commit-Position: refs/heads/master@{#447073}
Committed:
https://chromium.googlesource.com/chromium/src/+/80341e502bc01bdab13c7e69c2d7...
==========

[commit-bot: I haz the power, 2017-01-30 21:38:10]

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