Ensure that if FrameView::isScrollable() is false, the assoc WebLayer is, too.

Ensure that if FrameView::isScrollable() is false, the assoc WebLayer is, too.

Currently, we ignore main thread scrolling reasons when
FrameView::isScrollable() returns false. Unfortunately, this can be the case
even if the main frame would indeed respond to scrolls on the impl thread.
This can happen when the main frame is overflow: hidden. This should never be.
This CL builds on the existing machinery in ScrollingCoordinator to detect when
the main frame's scrollability is dirty to ensure that its associated WebLayer's
scrollability is correct.

R=shawnsingh@chromium.org
BUG=328672
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164629

[Ian Vollick, 2013-12-17 03:35:54]

PTAL

[shawnsingh, 2013-12-17 09:04:47]

On 2013/12/17 03:35:54, Ian Vollick wrote:
> PTAL

I like the code, but I'm still uncertain about that this is the way to fix the
issue - should we just be always sending mainThreadScrollingReasons, regardless
of scrollability?  Why do we want to not-send-mainThreadScrollingReasons to the
impl thread in the first place?

[shawnsingh, 2013-12-17 09:05:11]

oh - previous comment + this comment is my first pass on reviewing

https://codereview.chromium.org/103583007/diff/1/Source/core/page/scrolling/S...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/103583007/diff/1/Source/core/page/scrolling/S...
Source/core/page/scrolling/ScrollingCoordinator.cpp:626: // We may be ignoring
main thread scrolling reasons if the main thread claims it cannot be scrolled.
Check for this case.
misplaced comment?  or should this be labeled as a FIXME? or am I just confused?

[Ian Vollick, 2013-12-17 15:39:50]

On 2013/12/17 09:04:47, shawnsingh wrote:
> On 2013/12/17 03:35:54, Ian Vollick wrote:
> > PTAL
> 
> I like the code, but I'm still uncertain about that this is the way to fix the
> issue - should we just be always sending mainThreadScrollingReasons,
regardless
> of scrollability?  Why do we want to not-send-mainThreadScrollingReasons to
the
> impl thread in the first place?

It's a fair question. I'll do my best to summarize how we got to where were are
now. Originally, I'd taught cc to recognize a main frame vs non-main-frame
scroll
and to disregard the main frame scroll reasons if the main frame couldn't be
scrolled. This was a decent-sized bit of code in cc. aelias@ suggested that we
instead determine if the main frame couldn't be scrolled in blink and just not
set
reasons in that case. This was a much simpler change, so I went that route.

There was a problem with FrameView::isScrollable being stale and the reasons not
updating, but that's since been resolved.

The only problem, currently, is that occasionally, the main frame will report
that
it is not scrollable only because it has no scrollbars. If it were not for the
lack
of scrollbars, it would have been scrollable by cc. If we don't send the
scroll-on-main-thread reasons in this case, cc will indeed scroll this layer. To
cope with this, I thought it would be simple to only disregard the main thread
scrolling reasons if it was truly impossible for cc to scroll the layer rather
than
to teach cc special rules about overflow:hidden blinkisms. There has been some
churn in cc recently regarding overflow:hidden, and I didn't want to wade into
that
or make it more complex. Adding sadrul@, who's knows more about that and can
hopefully chime in if this is all wrong :)

Some extra information that could be useful here:
 - The main thread scrolling reason here is a non-layer viewport constrained
object.
 - The reason we can't scroll is that the 'html' element is styled with
overflow:hidden.

[shawnsingh, 2013-12-17 18:49:05]

OK thanks for the explanation =)

> The only problem, currently, is that occasionally, the main frame will report
> that
> it is not scrollable only because it has no scrollbars. If it were not for the
> lack
> of scrollbars, it would have been scrollable by cc. If we don't send the
> scroll-on-main-thread reasons in this case, cc will indeed scroll this layer.
To
> cope with this, I thought it would be simple to only disregard the main thread
> scrolling reasons if it was truly impossible for cc to scroll the layer rather
> than
> to teach cc special rules about overflow:hidden blinkisms.

Aha -- this is the part that feels awkward to me... in some twisted distilled
sense, this patch is trying to create a second, redundant-but-different notion
of what it means to be scrollable, and using that to force cc/ to defer to the
main thread when it thinks something is scrollable but blink knows it's not. 
(whew... that was a mouthful)

Why would we not just set WebLayer::setScrollable(false) correctly, instead?

If there's some awkward pinch-zoom / mobile viewport complications, I would
argue that there needs to be a translation layer somewhere at a different level
of abstraction (after scrolling coordinator, before cc::layer) that knows how to
resolve the fact that scrollingcoordinator doesn't consider those details.  Or
maybe we need to make scrolling coordinator aware of those viewport details. 
But I think it's risky to create two different semantics for the notion of
"scrollable".

[Ian Vollick, 2013-12-18 21:09:04]

> Why would we not just set WebLayer::setScrollable(false) correctly, instead?

I was worried about other cc code (like pinch-zooming) would bust if we did
this,
but wjmaclean assured me that this would be fine, so yeah, let's do this! I've
updated the CL subject and description and uploaded a (much tinier) patch to
accomplish this. WDYT?

[shawnsingh, 2013-12-18 23:43:35]

LGTM

[Ian Vollick, 2013-12-19 17:07:17]

+jamesr for Source/web/tests

[jamesr, 2013-12-19 22:02:33]

Can we move the test file to live next to ScrollingCoordinator.cpp?  OWNERS for
a piece of code should also be able to approve test changes.

[Ian Vollick, 2013-12-21 03:00:42]

On 2013/12/19 22:02:33, jamesr wrote:
> Can we move the test file to live next to ScrollingCoordinator.cpp?  OWNERS
for
> a piece of code should also be able to approve test changes.

Would you be ok with me just moving the test for ScrollingCoordinator.cpp? I
started moving all the tests in web/tests/, but the yak is looking pretty hairy.

[Ian Vollick, 2013-12-21 05:07:22]

On 2013/12/21 03:00:42, Ian Vollick wrote:
> On 2013/12/19 22:02:33, jamesr wrote:
> > Can we move the test file to live next to ScrollingCoordinator.cpp?  OWNERS
> for
> > a piece of code should also be able to approve test changes.
> 
> Would you be ok with me just moving the test for ScrollingCoordinator.cpp? I
> started moving all the tests in web/tests/, but the yak is looking pretty
hairy.

Even moving just the ScrollingCoordinator unit test could be tricky. The problem
is that it depends on web/tests/URLTestHelpers.h and
web/tests/FrameTestHelpers.h. I don't see an easy way of resolving the layering
violation. Maybe we could create a unit test support facility somewhere in
public/ that could be consumed in core?

Do you have any advice, James?

[Ian Vollick, 2014-01-06 19:59:04]

On 2013/12/21 05:07:22, Ian Vollick wrote:
> On 2013/12/21 03:00:42, Ian Vollick wrote:
> > On 2013/12/19 22:02:33, jamesr wrote:
> > > Can we move the test file to live next to ScrollingCoordinator.cpp? 
OWNERS
> > for
> > > a piece of code should also be able to approve test changes.
> > 
> > Would you be ok with me just moving the test for ScrollingCoordinator.cpp? I
> > started moving all the tests in web/tests/, but the yak is looking pretty
> hairy.
> 
> Even moving just the ScrollingCoordinator unit test could be tricky. The
problem
> is that it depends on web/tests/URLTestHelpers.h and
> web/tests/FrameTestHelpers.h. I don't see an easy way of resolving the
layering
> violation. Maybe we could create a unit test support facility somewhere in
> public/ that could be consumed in core?
> 
> Do you have any advice, James?

ping.

[jamesr, 2014-01-06 23:46:01]

Ah, the issue is ScrollingCoordinatorTest.cpp isn't actually a unit test but an
integration test that depends on other things. Oh well.

https://codereview.chromium.org/103583007/diff/40001/Source/web/tests/Scrolli...
File Source/web/tests/ScrollingCoordinatorChromiumTest.cpp (right):

https://codereview.chromium.org/103583007/diff/40001/Source/web/tests/Scrolli...
Source/web/tests/ScrollingCoordinatorChromiumTest.cpp:147: // not scrollable,
then the rootScrollLayer shouldn't be scrollable either.
can you add a case for what this was originally testing?

[Ian Vollick, 2014-01-07 19:20:32]

On 2014/01/06 23:46:01, jamesr wrote:
> Ah, the issue is ScrollingCoordinatorTest.cpp isn't actually a unit test but
an
> integration test that depends on other things. Oh well.
> 
>
https://codereview.chromium.org/103583007/diff/40001/Source/web/tests/Scrolli...
> File Source/web/tests/ScrollingCoordinatorChromiumTest.cpp (right):
> 
>
https://codereview.chromium.org/103583007/diff/40001/Source/web/tests/Scrolli...
> Source/web/tests/ScrollingCoordinatorChromiumTest.cpp:147: // not scrollable,
> then the rootScrollLayer shouldn't be scrollable either.
> can you add a case for what this was originally testing?

Done. I've got a 'fast-scrolls-by-default' test that covers the case where the
main frame is scrollable.

[jamesr, 2014-01-07 21:01:53]

lgtm

[commit-bot: I haz the power, 2014-01-07 21:17:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/103583007/190001

[commit-bot: I haz the power, 2014-01-07 23:31:02]

Change committed as 164629

[Dirk Pranke, 2014-01-08 02:55:23]

It looks like this change broke a couple of layout tests:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...

More importantly, we have a vague hope that it also broke some android java unit
tests:

http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28dbg...

I'm going to try and speculatively roll it out to see if the Android bot greens
up.