Only clear tooltip message on a user scroll or compositor scroll.

Only clear tooltip message on a user scroll or compositor scroll.

This behavior isn't well defined, but it seems reasonable that the
tooltip should only be dismissed by a user-initiated scroll.

BUG=677252
R=tdresser@chromium.org,skobes@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/e80f3cb67769187ddc37c8867648cae883de47b3
Cr-Commit-Position: refs/heads/master@{#441544}

[szager1, 2016-12-30 00:37:16]

Description was changed from

==========
Only clear tooltip message on a user scroll or compositor scroll.

This behavior isn't well defined, but it seems reasonable that the
tooltip should only be dismissed by a user-initiated scroll.

BUG=677252
R=tdresser@chromium.org,skobes@chromium.org
==========

to

==========
Only clear tooltip message on a user scroll or compositor scroll.

This behavior isn't well defined, but it seems reasonable that the
tooltip should only be dismissed by a user-initiated scroll.

BUG=677252
R=tdresser@chromium.org,skobes@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[szager1, 2016-12-30 00:52:06]

Add test, handle non-RLS frame scroll case.

[szager1, 2016-12-30 03:12:19]

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

[commit-bot: I haz the power, 2016-12-30 03:12:27]

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, 2016-12-30 04:57:36]

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

[commit-bot: I haz the power, 2016-12-30 04:57:37]

Dry run: This issue passed the CQ dry run.

[tdresser, 2017-01-03 13:56:32]

LGTM, with minor nit.

https://codereview.chromium.org/2603063002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2603063002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3820: if (Page* page =
frame().page())
Moving the assignment to within the conditional makes this harder to read IMO.

https://codereview.chromium.org/2603063002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/2603063002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:417: if (Page*
page = frame->page())
Why move this assignment into the conditional?

[szager1, 2017-01-03 20:24:22]

PTAL -- I refined it so that frame scroll always dismiss the tooltip, but
non-frame scrolls only dismiss the tooltip if the scroll was user-initiated.

https://codereview.chromium.org/2603063002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/2603063002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:417: if (Page*
page = frame->page())
On 2017/01/03 13:56:32, tdresser wrote:
> Why move this assignment into the conditional?

I have no opinion on this.  I restored the previous syntax.

[tdresser, 2017-01-03 21:05:51]

Why should the root frame always dismiss the tooltip?

[szager1, 2017-01-03 21:28:34]

On 2017/01/03 21:05:51, tdresser wrote:
> Why should the root frame always dismiss the tooltip?

I thought it might be a good idea to make the most targeted possible fix for
this; the bug refers specifically to a non-root scroller causing the tooltip to
disappear.

Since you made the original fix here, do you have an opinion?  Should
programmatic frame scrolls leave the tooltip intact?

[tdresser, 2017-01-03 21:59:31]

I'd prefer we weren't inconsistent between root frame and non-root frame
scrolls.

Unless we have evidence to suggest that these should be handled differently,
let's handle them the same way.

[szager1, 2017-01-03 22:54:49]

On 2017/01/03 21:59:31, tdresser wrote:
> I'd prefer we weren't inconsistent between root frame and non-root frame
> scrolls.
> 
> Unless we have evidence to suggest that these should be handled differently,
> let's handle them the same way.

OK, I restored the change to frame scroll behavior; it now matches non-frame
scrolls.  Also added a test for PaintLayerScrollableArea.

[tdresser, 2017-01-04 18:13:49]

LGTM

[szager1, 2017-01-04 18:37:41]

The CQ bit was checked by szager@chromium.org

[commit-bot: I haz the power, 2017-01-04 18:38:01]

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-04 18:46:16]

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

[commit-bot: I haz the power, 2017-01-04 18:46:17]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[szager1, 2017-01-04 21:17:04]

Description was changed from

==========
Only clear tooltip message on a user scroll or compositor scroll.

This behavior isn't well defined, but it seems reasonable that the
tooltip should only be dismissed by a user-initiated scroll.

BUG=677252
R=tdresser@chromium.org,skobes@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Only clear tooltip message on a user scroll or compositor scroll.

This behavior isn't well defined, but it seems reasonable that the
tooltip should only be dismissed by a user-initiated scroll.

BUG=677252
R=tdresser@chromium.org,skobes@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[szager1, 2017-01-04 21:17:04]

szager@chromium.org changed reviewers:
+ bokan@chromium.org

[szager1, 2017-01-04 21:17:26]

bokan@, would you mind reviewing this since skobes is OOO?

[bokan, 2017-01-04 23:53:09]

lgtm

https://codereview.chromium.org/2603063002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/2603063002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:417: if (Page*
page = frame->page())
On 2017/01/03 20:24:22, szager1 wrote:
> On 2017/01/03 13:56:32, tdresser wrote:
> > Why move this assignment into the conditional?
> 
> I have no opinion on this.  I restored the previous syntax.

No need to change anything again, just FYI, but I seem to recall Blink style at
one point encouraging this (though I can't find it now) and there's lots of
precedent in existing code. The advantage is that it limits the scope on a
variable to where its used.

[szager1, 2017-01-04 23:57:08]

The CQ bit was checked by szager@chromium.org

[commit-bot: I haz the power, 2017-01-04 23:57:28]

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-05 01:24:56]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483574228720140,
"parent_rev": "fd3854336be4aa491bc3316da49d65498ed40992", "commit_rev":
"2db041d9a05334a75178db8f0cd27995b272c658"}

[commit-bot: I haz the power, 2017-01-05 01:25:46]

Description was changed from

==========
Only clear tooltip message on a user scroll or compositor scroll.

This behavior isn't well defined, but it seems reasonable that the
tooltip should only be dismissed by a user-initiated scroll.

BUG=677252
R=tdresser@chromium.org,skobes@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Only clear tooltip message on a user scroll or compositor scroll.

This behavior isn't well defined, but it seems reasonable that the
tooltip should only be dismissed by a user-initiated scroll.

BUG=677252
R=tdresser@chromium.org,skobes@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2603063002
==========

[commit-bot: I haz the power, 2017-01-05 01:25:48]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2017-01-05 01:30:04]

Description was changed from

==========
Only clear tooltip message on a user scroll or compositor scroll.

This behavior isn't well defined, but it seems reasonable that the
tooltip should only be dismissed by a user-initiated scroll.

BUG=677252
R=tdresser@chromium.org,skobes@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2603063002
==========

to

==========
Only clear tooltip message on a user scroll or compositor scroll.

This behavior isn't well defined, but it seems reasonable that the
tooltip should only be dismissed by a user-initiated scroll.

BUG=677252
R=tdresser@chromium.org,skobes@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/e80f3cb67769187ddc37c8867648cae883de47b3
Cr-Commit-Position: refs/heads/master@{#441544}
==========

[commit-bot: I haz the power, 2017-01-05 01:30:05]

Patchset 5 (id:??) landed as
https://crrev.com/e80f3cb67769187ddc37c8867648cae883de47b3
Cr-Commit-Position: refs/heads/master@{#441544}