Bug 104950 – Add a setting to enable composited scrolling for frames

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Xianzhu Wang]

When enableAcceleratedCompositingForScrollableFrames, we also need to setScrollable(true) to enable composited scrolling for iframes. Otherwise Chromium compositor will still use the scroll layer of the main page and fall back to slow scrolling path because of the nonFastScrollableRegion (which eventually should not include iframes and overflow:scroll areas) of the main scroll layer. The patch to this bug will only address setScrollable, not nonFastScrollableRegion.

[James Robinson]

I don't think our current code supports composited scrolling for iframes or anything other than the root, so the current behavior seems correct.

[Xianzhu Wang]

(In reply to comment #1)
> I don't think our current code supports composited scrolling for iframes or anything other than the root, so the current behavior seems correct.

The purpose of this bug is to enable composited scrolling for iframes for better performance. To achieve that we need to fix any blocking issues. Is the solution proposed in Description the right way to go?

Just saw that the patch of bug 97357 just let RenderLayer::usesCompositedScrolling() return true to enable composited scrolling for -webkit-overflow-scrolling:touch. I'm wondering how it works.

[Xianzhu Wang]

(In reply to comment #2)
> Just saw that the patch of bug 97357 just let RenderLayer::usesCompositedScrolling() return true to enable composited scrolling for -webkit-overflow-scrolling:touch. I'm wondering how it works.

Never mind. Found updateScrollingLayers in RenderLayerBacking::updateGraphicsLayerConfigurations().

[Xianzhu Wang]

Created attachment 180410 [details]
Patch

[WebKit Review Bot]

Comment on attachment 180410 [details]
Patch

Attachment 180410 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15451191

New failing tests:
platform/chromium/virtual/softwarecompositing/iframes/iframe-composited-scrolling.html
compositing/iframes/iframe-composited-scrolling.html

[Antonio Gomes]

I plan to enable compositing for inner frames ports that do not use non-force-compositing as well. I do not think it overlaps with this work though.

[Build Bot]

Comment on attachment 180410 [details]
Patch

Attachment 180410 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15458253

New failing tests:
compositing/iframes/iframe-composited-scrolling.html

[James Robinson]

I think there's a bit more work to do before we can turn usesCompositedScrolling on for subframes.  For instance from ScrollingCoordinator.cpp:

bool ScrollingCoordinator::coordinatesScrollingForFrameView(FrameView* frameView) const
{
    ASSERT(isMainThread());
    ASSERT(m_page);

    // We currently only handle the main frame.
    if (frameView->frame() != m_page->mainFrame())
        return false;



One consequence is that scrollbar layers don't update properly for subframes.  There are probably other cases as well.

[Xianzhu Wang]

(In reply to comment #8)
> I think there's a bit more work to do before we can turn usesCompositedScrolling on for subframes.  For instance from ScrollingCoordinator.cpp:
> 
> bool ScrollingCoordinator::coordinatesScrollingForFrameView(FrameView* frameView) const
> {
>     ASSERT(isMainThread());
>     ASSERT(m_page);
> 
>     // We currently only handle the main frame.
>     if (frameView->frame() != m_page->mainFrame())
>         return false;
> 
> 
> 
> One consequence is that scrollbar layers don't update properly for subframes.  There are probably other cases as well.

I noticed this, while I thought most parts of ScrollingCoordinator were for main frame only and sub-frame scrolling could be handled by the compositor directly given that ScrollingCoordinatorChromium sets the scrollable flag and scrollableArea for the layer. Ran a test (will attach) on chromium-android and it worked well (including the scrollbars and fixed position element in the sub-frame) with the patch. I think this is just like how composited scrolling for overflow:scroll works because the layer tree for a sub-frame is similar to that of an overflow-scroll element. Am I missing anything?

Did a smoke test of enabling ScrollingCoordinator::coordinatesScrollingForFrameView() for sub-frames and it didn't work: no scrolling, no scroll bar and sometimes crashed IN

[Xianzhu Wang]

Created attachment 181383 [details]
Manual test case

[James Robinson]

(In reply to comment #9)
> (In reply to comment #8)
> > I think there's a bit more work to do before we can turn usesCompositedScrolling on for subframes.  For instance from ScrollingCoordinator.cpp:
> > 
> > bool ScrollingCoordinator::coordinatesScrollingForFrameView(FrameView* frameView) const
> > {
> >     ASSERT(isMainThread());
> >     ASSERT(m_page);
> > 
> >     // We currently only handle the main frame.
> >     if (frameView->frame() != m_page->mainFrame())
> >         return false;
> > 
> > 
> > 
> > One consequence is that scrollbar layers don't update properly for subframes.  There are probably other cases as well.
> 
> I noticed this, while I thought most parts of ScrollingCoordinator were for main frame only and sub-frame scrolling could be handled by the compositor directly given that ScrollingCoordinatorChromium sets the scrollable flag and scrollableArea for the layer. Ran a test (will attach) on chromium-android and it worked well (including the scrollbars and fixed position element in the sub-frame) with the patch. I think this is just like how composited scrolling for overflow:scroll works because the layer tree for a sub-frame is similar to that of an overflow-scroll element. Am I missing anything?
>

As far as I understand, composited scrolling (in the usesCompositedScrolling() sense) for overflow:scroll doesn't work right now either.

With this patch, running on desktop linux with "--enable-threaded-compositing --force-compositing-mode --enable-accelerated-scrollable-frames" on this page http://webstuff.nfshost.com/scrolling/iframe_scrolls.html the iframe's contents scroll on the thread but the scrollbars do not, which looks pretty bad.
 
> Did a smoke test of enabling ScrollingCoordinator::coordinatesScrollingForFrameView() for sub-frames and it didn't work: no scrolling, no scroll bar and sometimes crashed IN

Right.  I imagine there's some amount of work needed to get this working for either the iframe or overflow:scroll case.

[James Robinson]

Comment on attachment 180410 [details]
Patch

Doesn't work yet.

[Xianzhu Wang]

Is overflow:scroll in the same status (composited scrolling, main-thread scrollbar update)? Actually we have enabled composited scrolling for overflow:scroll in chromium-android dev.

[James Robinson]

(In reply to comment #13)
> Is overflow:scroll in the same status (composited scrolling, main-thread scrollbar update)? Actually we have enabled composited scrolling for overflow:scroll in chromium-android dev.

I don't know what the status of scrollbars for overflow:scroll elements on android is, but if they had scrollbars I do not think they would work as expected.  There just isn't any code that would implement that logic.

[Sami Kyöstilä]

(In reply to comment #14)
> I don't know what the status of scrollbars for overflow:scroll elements on android is, but if they had scrollbars I do not think they would work as expected.  There just isn't any code that would implement that logic.

Composited overflow:scroll on Android and elsewhere currently uses main-thread scrollbars which have a number of problems (not scrolling on the compositor thread, not fading out, changing size depending on the page scale). I've opened bug 106215.

[Xianzhu Wang]

@jamesr, are there issues other than the main-thread scrollbar of enabling composited scrolling for frames? If no, would you agree to enable it just like what we did for overflo:scroll? It is controlled by the switch and might be only enabled on android at first.

In addition, some of the main-thread scrollbar issues seem to also exist without enabling composited scrolling for frames and overflow:scroll, so enabling it seems not to make things worse, while it will make scrolling faster :)

[James Robinson]

The cross-platform support isn't in place, so the cross-platform changes in this patch (like FrameView) are definitely wrong and cannot land.  If you want to consider enabling this feature just for chromium-android despite its current bugs, the way to do that would be to add a setting that can be toggled by different ports and platforms.

[Xianzhu Wang]

(In reply to comment #17)
> The cross-platform support isn't in place, so the cross-platform changes in this patch (like FrameView) are definitely wrong and cannot land.  If you want to consider enabling this feature just for chromium-android despite its current bugs, the way to do that would be to add a setting that can be toggled by different ports and platforms.

I was thinking acceleratedCompositingForScrollableFrames is the setting. For now it seems to be disabled for all platforms. ForceCompositingMode of a sub-frame will be set to true if and only if the setting is enabled and the frame is scrollable. Do we need another setting?

[James Robinson]

(In reply to comment #18)
> (In reply to comment #17)
> > The cross-platform support isn't in place, so the cross-platform changes in this patch (like FrameView) are definitely wrong and cannot land.  If you want to consider enabling this feature just for chromium-android despite its current bugs, the way to do that would be to add a setting that can be toggled by different ports and platforms.
> 
> I was thinking acceleratedCompositingForScrollableFrames is the setting. For now it seems to be disabled for all platforms. ForceCompositingMode of a sub-frame will be set to true if and only if the setting is enabled and the frame is scrollable. Do we need another setting?

That flag controls compositing, not scrolling.

[Xianzhu Wang]

(In reply to comment #19)
> (In reply to comment #18)
> > I was thinking acceleratedCompositingForScrollableFrames is the setting. For now it seems to be disabled for all platforms. ForceCompositingMode of a sub-frame will be set to true if and only if the setting is enabled and the frame is scrollable. Do we need another setting?
> 
> That flag controls compositing, not scrolling.

I thought the reason to composite a scrollable frame was for scrolling only, just like that acceleratedCompositingForOverflowScrollEnabled is for composited scrolling of overflow:scroll.

I'd add another setting but am still wondering what the other reasons to composite scrollable (but not non-scrollable) frames are.

[James Robinson]

Performance - so you don't have to repaint the full frame every time it scrolls.

If composited scrolling was feature complete then it might make sense to combine the two into one setting, but so long as composited scrolling for frames doesn't fully work it cannot be combined with a feature that does work correctly (compositing iframes that can scroll).

[Xianzhu Wang]

Created attachment 181575 [details]
Patch

[James Robinson]

Comment on attachment 181575 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181575&action=review

> Source/WebCore/ChangeLog:3
> +        [Chromium] Enable composited scrolling for frames

This touches cross-platform code, so it shouldn't have the [Chromium] tag, and it doesn't enable anything by itself - it adds a setting.

> LayoutTests/platform/chromium/compositing/force-compositing-mode/iframe-composited-scrolling.html:1
> +<html>

add <!DOCTYPE html> before this line so the test doesn't run in quirks mode

[Xianzhu Wang]

Created attachment 181598 [details]
Patch

[James Robinson]

Comment on attachment 181598 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181598&action=review

Move the test to a cross-platform location (if it is indeed cross platform) then r=me.  If it isn't, what's the issue?

> LayoutTests/ChangeLog:11
> +        * platform/chromium/compositing/force-compositing-mode/iframe-composited-scrolling-expected.txt: Added.
> +        * platform/chromium/compositing/force-compositing-mode/iframe-composited-scrolling.html: Added.

this test isn't chromium-specific in any way, is it?

[Xianzhu Wang]

Comment on attachment 181575 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181575&action=review

>> Source/WebCore/ChangeLog:3
>> +        [Chromium] Enable composited scrolling for frames
> 
> This touches cross-platform code, so it shouldn't have the [Chromium] tag, and it doesn't enable anything by itself - it adds a setting.

Done.

I still put the layout test under platform/chromium because all other force-compositing-mode tests are at here. If necessary, I think we can move them all together out of platform/chromium in a separate bug.

>> LayoutTests/platform/chromium/compositing/force-compositing-mode/iframe-composited-scrolling.html:1
>> +<html>
> 
> add <!DOCTYPE html> before this line so the test doesn't run in quirks mode

Done.

[James Robinson]

(In reply to comment #26)
> (From update of attachment 181575 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181575&action=review
> 
> >> Source/WebCore/ChangeLog:3
> >> +        [Chromium] Enable composited scrolling for frames
> > 
> > This touches cross-platform code, so it shouldn't have the [Chromium] tag, and it doesn't enable anything by itself - it adds a setting.
> 
> Done.
> 
> I still put the layout test under platform/chromium because all other force-compositing-mode tests are at here. If necessary, I think we can move them all together out of platform/chromium in a separate bug.

Those seem wrong as well, then.  No reason to continue the badness.

[Xianzhu Wang]

Created attachment 181609 [details]
Patch

[James Robinson]

Comment on attachment 181609 [details]
Patch

Still looks good. Want a CQ?

[Xianzhu Wang]

(In reply to comment #29)
> (From update of attachment 181609 [details])
> Still looks good. Want a CQ?

There are some tiny issues in ChangeLog (extra chromium/ in the path). Updating. Will CQ by myself when it's uploaded.

[Xianzhu Wang]

Created attachment 181610 [details]
Patch

[WebKit Review Bot]

Comment on attachment 181610 [details]
Patch

Clearing flags on attachment: 181610

Committed r139024: <http://trac.webkit.org/changeset/139024>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.