Disable compositing asserts when gesture scrolling on main thread

Disable compositing asserts when gesture scrolling on main thread

These asserts get tripped when ScrollAnimator tries to determine if the
scroller needs to be scrolled on the main thread or on the impl thread.

Also moved the early-out if there's no FrameView on GestureScrollBegin
to all GestureScroll events since we shouldn't handle any of those if
there's no view.

BUG=625676
Committed: https://crrev.com/08ba06781913559370207cd5be4756c1d706fd14
Cr-Commit-Position: refs/heads/master@{#404922}

[bokan, 2016-07-04 21:04:25]

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

[commit-bot: I haz the power, 2016-07-04 21:04:34]

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

[bokan, 2016-07-04 21:05:01]

Description was changed from

==========
Make sure document is in a compositing clean state when scrolling.

We make sure the document has had its style and layout updated before processing
a scroll gesture but the scrolling code can query compositing state too. This
happens in the attached bug when ScrollAnimator starts an animation and has to
query compositing state to determine if the animation needs to run on the main
or compositor thread.

BUG=625676
==========

to

==========
Make sure document is in a compositing clean state when scrolling.

We make sure the document has had its style and layout updated before processing
a scroll gesture but the scrolling code can query compositing state too. This
happens in the attached bug when ScrollAnimator starts an animation and has to
query compositing state to determine if the animation needs to run on the main
or compositor thread.

BUG=625676
==========

[bokan, 2016-07-04 21:05:01]

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

[bokan, 2016-07-04 21:07:38]

Ian, is this a sensible thing to do? We touch compositing state as part of
scrolling so it seems like we should make sure we get to CompositingClean but
maybe the real answer is we shouldn't be touching compositing state from
scrolling? WDYT?

Also, no test since I'm not sure how to get it into this state. I could probably
do it with a unit test and manually putting it into LayoutClean or something
like that. Advice welcome.

[commit-bot: I haz the power, 2016-07-04 22:32:24]

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

[commit-bot: I haz the power, 2016-07-04 22:32:25]

Dry run: This issue passed the CQ dry run.

[bokan, 2016-07-06 16:01:50]

friendly ping :)

[Ian Vollick, 2016-07-06 17:39:18]

https://codereview.chromium.org/2120373002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/ScrollManager.cpp (right):

https://codereview.chromium.org/2120373002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/ScrollManager.cpp:181:
m_frame->view()->updateLifecycleToCompositingCleanPlusScrolling();
I'm worried this is overkill. I spoke with skobes offline and he mentioned that
using an assert disabler is the usual way of dealing with these issues until we
fix the more fundamental issue of fitting scroll properly into the document
lifecycle (eg:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...).
Could we use the same approach here?

[skobes, 2016-07-06 18:24:49]

To recap the offline conversation: I think scrolling has inherent dependencies
on compositing state at least until slimming paint v2.  If the only problem is
the assertion failure and you can't reason your way to a concrete bug caused by
the use of compositing state here, I'd rather suppress the assert than go
through contortions with the lifecycle.

On 2016/07/06 17:39:18, vollick wrote:
>
https://codereview.chromium.org/2120373002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/input/ScrollManager.cpp (right):
> 
>
https://codereview.chromium.org/2120373002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/input/ScrollManager.cpp:181:
> m_frame->view()->updateLifecycleToCompositingCleanPlusScrolling();
> I'm worried this is overkill. I spoke with skobes offline and he mentioned
that
> using an assert disabler is the usual way of dealing with these issues until
we
> fix the more fundamental issue of fitting scroll properly into the document
> lifecycle (eg:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...).
> Could we use the same approach here?

[bokan, 2016-07-06 19:59:18]

Description was changed from

==========
Make sure document is in a compositing clean state when scrolling.

We make sure the document has had its style and layout updated before processing
a scroll gesture but the scrolling code can query compositing state too. This
happens in the attached bug when ScrollAnimator starts an animation and has to
query compositing state to determine if the animation needs to run on the main
or compositor thread.

BUG=625676
==========

to

==========
Disable compositing asserts when gesture scrolling on main thread

These asserts get tripped when ScrollAnimator tries to determine if the
scroller needs to be scrolled on the main thread or on the impl thread.

Also moved the early-out if there's no FrameView on GestureScrollBegin
to all GestureScroll events since we shouldn't handle any of those if
there's no view.

BUG=625676
==========

[bokan, 2016-07-06 20:00:25]

Thanks both for the explanation, makes sense to me. Patch changed to use the
assert disabler, ptal.

[bokan, 2016-07-08 16:33:42]

ping

[bokan, 2016-07-12 16:42:56]

ping. vollick@ does this look ok to you?

[Ian Vollick, 2016-07-12 16:47:48]

On 2016/07/12 at 16:42:56, bokan wrote:
> ping. vollick@ does this look ok to you?

So sorry for the slow response. Yes, this seems reasonable, but please check
with skobes@ in case I'm missing something.

[bokan, 2016-07-12 16:48:50]

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

[bokan, 2016-07-12 16:49:01]

+skobes@, WDYT?

[skobes, 2016-07-12 18:21:41]

lgtm

[bokan, 2016-07-12 18:43:10]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2016-07-12 18:44:58]

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-07-12 21:14:59]

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

[commit-bot: I haz the power, 2016-07-12 21:15:01]

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

[bokan, 2016-07-12 21:20:59]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2016-07-12 21:22:34]

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-07-13 02:41:53]

Description was changed from

==========
Disable compositing asserts when gesture scrolling on main thread

These asserts get tripped when ScrollAnimator tries to determine if the
scroller needs to be scrolled on the main thread or on the impl thread.

Also moved the early-out if there's no FrameView on GestureScrollBegin
to all GestureScroll events since we shouldn't handle any of those if
there's no view.

BUG=625676
==========

to

==========
Disable compositing asserts when gesture scrolling on main thread

These asserts get tripped when ScrollAnimator tries to determine if the
scroller needs to be scrolled on the main thread or on the impl thread.

Also moved the early-out if there's no FrameView on GestureScrollBegin
to all GestureScroll events since we shouldn't handle any of those if
there's no view.

BUG=625676
==========

[commit-bot: I haz the power, 2016-07-13 02:41:55]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-13 02:42:25]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 02:43:29]

Description was changed from

==========
Disable compositing asserts when gesture scrolling on main thread

These asserts get tripped when ScrollAnimator tries to determine if the
scroller needs to be scrolled on the main thread or on the impl thread.

Also moved the early-out if there's no FrameView on GestureScrollBegin
to all GestureScroll events since we shouldn't handle any of those if
there's no view.

BUG=625676
==========

to

==========
Disable compositing asserts when gesture scrolling on main thread

These asserts get tripped when ScrollAnimator tries to determine if the
scroller needs to be scrolled on the main thread or on the impl thread.

Also moved the early-out if there's no FrameView on GestureScrollBegin
to all GestureScroll events since we shouldn't handle any of those if
there's no view.

BUG=625676

Committed: https://crrev.com/08ba06781913559370207cd5be4756c1d706fd14
Cr-Commit-Position: refs/heads/master@{#404922}
==========

[commit-bot: I haz the power, 2016-07-13 02:43:30]

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