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.
I don't think our current code supports composited scrolling for iframes or anything other than the root, so the current behavior seems correct.
(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.
(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().
Created attachment 180410 [details] Patch
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
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.
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
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.
(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
Created attachment 181383 [details] Manual test case
(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.
Comment on attachment 180410 [details] Patch Doesn't work yet.
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.
(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.
(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.
@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 :)
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.
(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?
(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.
(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.
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).
Created attachment 181575 [details] Patch
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
Created attachment 181598 [details] Patch
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?
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.
(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.
Created attachment 181609 [details] Patch
Comment on attachment 181609 [details] Patch Still looks good. Want a CQ?
(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.
Created attachment 181610 [details] Patch
Comment on attachment 181610 [details] Patch Clearing flags on attachment: 181610 Committed r139024: <http://trac.webkit.org/changeset/139024>
All reviewed patches have been landed. Closing bug.