Main-thread scroll if we have fixed-pos children and overflow:hidden.

Main-thread scroll if we have fixed-pos children and overflow:hidden.

If we are overflow:hidden and have fixed-pos children we would transition
to impl-side scrolling. This was exposing another bug (304810)
where on impl-side where we do not correctly stop body elements from 
scrolling. This lead us to scroll elements on the impl-thread even 
if they need to be main-thread scrolled.

Here we do not short-circuit the main-thread requirements if
the frame is overflow:hidden. This is equivalent to our old
implementation, and lets gmail continue to be on impl-thread,
which is what abarth had originally changed that caused this issue.

BUG=411767
R=vollick@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184891

[Ian Vollick, 2014-11-05 16:40:05]

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

[Ian Vollick, 2014-11-05 16:40:07]

You'll certainly need a layout test for this. It would also help to cite the bug
that forces us to main thread scroll in this case.

I've got a few bikesheddy comments about naming/clarity, but it'd be great to
get skobes@'s opinion on this since he's most familiar with main frame scrolling
at this point.

https://codereview.chromium.org/699183004/diff/1/Source/core/frame/FrameView.cpp
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/699183004/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.cpp:2287: return NonScrollableFromSize;
NotScrollableNoOverflow

https://codereview.chromium.org/699183004/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.cpp:2293: return NonScrollableFromVisibility;
NotScrollableNotVisible

https://codereview.chromium.org/699183004/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.cpp:2300: return NonScrollableFromLayout;
NotScrollableExplicitlyDisabled

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

https://codereview.chromium.org/699183004/diff/1/Source/core/page/scrolling/S...
Source/core/page/scrolling/ScrollingCoordinator.cpp:952: &&
hasVisibleSlowRepaintViewportConstrainedObjects(frameView)) {
I think this could be made clearer with some well named temporaries.

const bool mayBeScrolledByInput = scrollingReasons == FrameView::Scrollable;
const bool mayBeScrolledByScript = scrollingReasons ==
FrameView::NotScrollableExplicitlyDisabled;
const bool canScroll = mayBeScrolledByInput || mayBeScrolledByScript;

if (canScroll && hasBlahBlah(...) { ... }

[awoloszyn, 2014-11-05 19:25:11]

Added comments, and a layout test.

https://codereview.chromium.org/699183004/diff/1/Source/core/frame/FrameView.cpp
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/699183004/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.cpp:2287: return NonScrollableFromSize;
On 2014/11/05 16:40:06, vollick wrote:
> NotScrollableNoOverflow

Done.

https://codereview.chromium.org/699183004/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.cpp:2293: return NonScrollableFromVisibility;
On 2014/11/05 16:40:06, vollick wrote:
> NotScrollableNotVisible

Done.

https://codereview.chromium.org/699183004/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.cpp:2300: return NonScrollableFromLayout;
On 2014/11/05 16:40:06, vollick wrote:
> NotScrollableExplicitlyDisabled

Done.

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

https://codereview.chromium.org/699183004/diff/1/Source/core/page/scrolling/S...
Source/core/page/scrolling/ScrollingCoordinator.cpp:952: &&
hasVisibleSlowRepaintViewportConstrainedObjects(frameView)) {
On 2014/11/05 16:40:06, vollick wrote:
> I think this could be made clearer with some well named temporaries.
> 
> const bool mayBeScrolledByInput = scrollingReasons == FrameView::Scrollable;
> const bool mayBeScrolledByScript = scrollingReasons ==
> FrameView::NotScrollableExplicitlyDisabled;
> const bool canScroll = mayBeScrolledByInput || mayBeScrolledByScript;
> 
> if (canScroll && hasBlahBlah(...) { ... }

Done.

[skobes, 2014-11-05 21:05:22]

Let me see if I understand this correctly...

- scrollable frames with fixed-pos children need to be scrolled on the main
thread (due to http://crbug.com/304810?),

- this change causes frames with overflow:hidden to be considered "scrollable"
for purposes of triggering main-thread scrolling,

- and this is needed because overflow:hidden frames can still be scrolled from
script.

Is that an accurate summary?

How hard would it be to fix impl-thread scrolling to handle this case correctly?

https://codereview.chromium.org/699183004/diff/20001/Source/core/page/scrolli...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/699183004/diff/20001/Source/core/page/scrolli...
Source/core/page/scrolling/ScrollingCoordinator.cpp:951: const bool
mayBeScrolledByScript = (scrollingReasons ==
Anything scrollable by input is also scrollable by script, so this might be
clearer as:

const bool mayBeScrolledByScript = mayBeScrolledByInput || (scrollingReasons ==
    FrameView::NotScrollableExplicitlyDisabled);

Then you can use mayBeScrolledByScript in place of canScroll in the check below.

https://codereview.chromium.org/699183004/diff/20001/Source/core/page/scrolli...
Source/core/page/scrolling/ScrollingCoordinator.cpp:955: // overflow:hidden
elements scroll on the fast path, so we should not let
Nit: I think this comment should say "...scroll on the compositor thread...". 
Doesn't fast path vs. slow path refer to whether we repaint?  So a main-thread
scroll can still be "fast path" if the layer is composited.

[awoloszyn, 2014-11-05 21:33:36]

On 2014/11/05 21:05:22, skobes wrote:
> Let me see if I understand this correctly...
> 
> - scrollable frames with fixed-pos children need to be scrolled on the main
> thread (due to http://crbug.com/304810),
> 
> - this change causes frames with overflow:hidden to be considered "scrollable"
> for purposes of triggering main-thread scrolling,
> 
> - and this is needed because overflow:hidden frames can still be scrolled from
> script.
> 
> Is that an accurate summary?
> 
That is an accurate summary. A bit more background is that abarth has recently
moved a lot of the code around,
and in doing so had taken gmail off of the impl-side scrolling path. His fix was
to move everything that is not
"scrollable" to the impl-thread, even if it would have been stuck on the main
thread due to fixed-pos elements.
This caused an issue when we actually do scroll when overflow:hidden is set. 

> How hard would it be to fix impl-thread scrolling to handle this case
correctly?
> 
I started by looking into the issue, but it looks like the fix would be fairly
complicated and require plumbing 
more information through from the main thread to the impl-thread. Even then I am
not entirely confident that it would
work as expected, especially in time for the branch, since this is a
release-blocker.


>
https://codereview.chromium.org/699183004/diff/20001/Source/core/page/scrolli...
> File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):
> 
>
https://codereview.chromium.org/699183004/diff/20001/Source/core/page/scrolli...
> Source/core/page/scrolling/ScrollingCoordinator.cpp:951: const bool
> mayBeScrolledByScript = (scrollingReasons ==
> Anything scrollable by input is also scrollable by script, so this might be
> clearer as:
> 
> const bool mayBeScrolledByScript = mayBeScrolledByInput || (scrollingReasons
==
>     FrameView::NotScrollableExplicitlyDisabled);
> 
> Then you can use mayBeScrolledByScript in place of canScroll in the check
below.
> 
>
https://codereview.chromium.org/699183004/diff/20001/Source/core/page/scrolli...
> Source/core/page/scrolling/ScrollingCoordinator.cpp:955: // overflow:hidden
> elements scroll on the fast path, so we should not let
> Nit: I think this comment should say "...scroll on the compositor thread...". 
> Doesn't fast path vs. slow path refer to whether we repaint?  So a main-thread
> scroll can still be "fast path" if the layer is composited.

[awoloszyn, 2014-11-05 21:33:44]

https://codereview.chromium.org/699183004/diff/20001/Source/core/page/scrolli...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/699183004/diff/20001/Source/core/page/scrolli...
Source/core/page/scrolling/ScrollingCoordinator.cpp:951: const bool
mayBeScrolledByScript = (scrollingReasons ==
On 2014/11/05 21:05:22, skobes wrote:
> Anything scrollable by input is also scrollable by script, so this might be
> clearer as:
> 
> const bool mayBeScrolledByScript = mayBeScrolledByInput || (scrollingReasons
==
>     FrameView::NotScrollableExplicitlyDisabled);
> 
> Then you can use mayBeScrolledByScript in place of canScroll in the check
below.

Done.

https://codereview.chromium.org/699183004/diff/20001/Source/core/page/scrolli...
Source/core/page/scrolling/ScrollingCoordinator.cpp:955: // overflow:hidden
elements scroll on the fast path, so we should not let
On 2014/11/05 21:05:22, skobes wrote:
> Nit: I think this comment should say "...scroll on the compositor thread...". 
> Doesn't fast path vs. slow path refer to whether we repaint?  So a main-thread
> scroll can still be "fast path" if the layer is composited.

Done.

[skobes, 2014-11-05 22:02:56]

lgtm

[Ian Vollick, 2014-11-06 00:28:22]

On 2014/11/05 22:02:56, skobes wrote:
> lgtm

lgtm2

[Ian Vollick, 2014-11-06 00:28:26]

The CQ bit was checked by vollick@chromium.org

[commit-bot: I haz the power, 2014-11-06 00:29:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/699183004/40001

[commit-bot: I haz the power, 2014-11-06 00:33:05]

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

[commit-bot: I haz the power, 2014-11-06 00:33:07]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[Ian Vollick, 2014-11-06 00:34:30]

On 2014/11/06 00:33:07, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   blink_presubmit on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

lgtm

[Ian Vollick, 2014-11-06 00:34:36]

The CQ bit was checked by vollick@chromium.org

[commit-bot: I haz the power, 2014-11-06 00:35:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/699183004/40001

[commit-bot: I haz the power, 2014-11-06 01:44:18]

Committed patchset #3 (id:40001) as 184891