Touch overflow scrolling elements (-webkit-overflow-scrolling: touch) should be split into layers and registered with the ScrollCoordinator so they can be scrolled efficiently without repainting the contents.
Are you merging the code from iOS5 branch into trunk? Or actually re-working it or working from scratch?
(In reply to comment #1) > Are you merging the code from iOS5 branch into trunk? Or actually re-working it or working from scratch? I am asking because I have done that for the BlackBerry port myself, but it was not a clean backport...
It's really more of a reimplementation than a straight port. I agree that the iOS5 branch isn't really usable as such.
Created attachment 146645 [details] Patch First version with the main pieces of functionality in place. Does not calculate per-layer non-fast scrollable regions, but that could be left for a follow-up patch.
Comment on attachment 146645 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146645&action=review > Source/WebCore/ChangeLog:12 > + Touch overflow scrolling elements should be split into layers > + https://bugs.webkit.org/show_bug.cgi?id=78862 > + > + Reviewed by NOBODY (OOPS!). > + > + When a regular overflow element is scrolled, we repaint the entire area > + it occupies in the view from scratch. Since this painting is done in the > + main thread, it can be blocked by, e.g., JavaScript execution, further > + limiting performance. > + I think it is valid and fair to mention that it was based on the original iOS5 branch implementation. I know it is a re-work, but it had a starting point.. > Source/WebCore/rendering/RenderLayer.cpp:4862 > + if (hasOverflow && isVisibleToHitTest > +#if ENABLE(OVERFLOW_SCROLLING) > + && !hasAcceleratedScrolling() > +#endif > + ) the blackberry port relies on all of the scrollable elements being tracked by the FrameView, in order to for example just skip a bunch of logic if there is no scrollable element (other than the main frame).No matter if it has "accelerated scrolling" or not. Could you explain why this change specifically?
Comment on attachment 146645 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146645&action=review Something I failed to mention earlier is that I'm still working on layout tests for these changes. >> Source/WebCore/ChangeLog:12 >> + > > I think it is valid and fair to mention that it was based on the original iOS5 branch implementation. I know it is a re-work, but it had a starting point.. You are absolutely right. I did not mean to pass this off as original work. >> Source/WebCore/rendering/RenderLayer.cpp:4862 >> + ) > > the blackberry port relies on all of the scrollable elements being tracked by the FrameView, in order to for example just skip a bunch of logic if there is no scrollable element (other than the main frame).No matter if it has "accelerated scrolling" or not. > > Could you explain why this change specifically? My motivation was to avoid the scrollable area from becoming part of the non-fast scrollable region (see ScrollingCoordinator.cpp) which would prevent it from being independently scrolled by the threaded compositor. Completely removing the scrollable layer from the scrollable set is a bit drastic, so perhaps it would be better to move this logic to ScrollingCoordinator instead.
Created attachment 147387 [details] Patch Updated to match new interface from bug 73350. Did not fix FrameView scrollable are registry yet.
Created attachment 147879 [details] Patch - Keep all ScrollableAreas -- fast scrollable or not -- in the FrameView's scrollable area set. - Add layout tests to verify graphics layer configuration and stacking context behavior with overflow:hidden.
The test coverage for the prepainting part of this patch isn't as good as I'd like yet. I'm thinking of somehow capturing the GraphicsLayer invalidations from DRT.
Created attachment 148135 [details] Patch - Added new test to verify scrolling tree using the framework from bug 89216.
Who should review this patch? @jamesr?
Created attachment 148562 [details] Patch - Cleaned up most of the #ifdef spaghetti. - Added test to verify scroll content layer alignment with borders. - Added test to check outline fallback.
Created attachment 148604 [details] Patch - Added overflow clipping test. Does not cover all the cases yet but it's a start. - Removed scrollingTreeAsText() test for now.
Comment on attachment 148604 [details] Patch I have some review feedback, but need a few days to look at the patch. Please wait for my feedback.
Sami: please cc: me on any future bugs that involve changes to RenderLayerCompositor/RenderLayerBacking/GraphicsLayer. Thanks.
Comment on attachment 148604 [details] Patch I plan to upsteam some iOS code that has a better implementation in RenderLayerBacking etc. Please give me a few days to do that.
(In reply to comment #16) > I plan to upsteam some iOS code that has a better implementation in RenderLayerBacking etc. Please give me a few days to do that. Okay, thanks Simon!
> I plan to upsteam some iOS code that has a better implementation in RenderLayerBacking etc. Please give me a few days to do that. @smfr: Did this end up happening? This patch accounts for a moderate amount of the remaining chromium-android fork and I'd like to make sure we don't stall out here. Thanks!
Sorry I haven't had time yet.
> Sorry I haven't had time yet. Would it be appropriate to proceed with this patch and then plan to merge with your work when you have time to contribute it? I don't fully understand the relation between the patches, so it's hard for me to know what the best course of action is here.
Unfortunately my patch will directly conflict with some of these changes (and addresses various issues that I think the current patch has). I'll try to get to it early next week.
(In reply to comment #21) > Unfortunately my patch will directly conflict with some of these changes (and addresses various issues that I think the current patch has). I'll try to get to it early next week. Thanks for the update. Is there another bug open for your patch, or were you going to land it against this one?
I'll do it via bug 91117.
Thanks Simon.
> I'll try to get to it early next week. Simon, I'm sorry to keep nagging you about this issue, but you've been blocking this patch for upwards of a month now. This bug is blocking us from upstreaming the chromium-android port and is one of the larger diffs left in WebCore, as described in <http://lists.webkit.org/pipermail/webkit-dev/2012-July/021516.html>. How do we make progress here?
This is top of my list to do after some internal stuff.
> This is top of my list to do after some internal stuff. Thanks Simon.
(In reply to comment #27) > > This is top of my list to do after some internal stuff. > > Thanks Simon. Just to make it clearer to me, once bug 91117 is done, there should still work to be done here?
ENABLE(OVERFLOW_SCROLLING) is a really bad name for this flag. overflows already scroll. It should have been ENABLE(COMPOSITED_OVERFLOW_SCROLLING) or something.
Also, I think it's fine to only protected the CSS parser code with #if ENABLE(OVERFLOW_SCROLLING). That just needs to hide the new CSS property. The rest of the code doesn't need #ifdefs.
(In reply to comment #28) > (In reply to comment #27) > > > This is top of my list to do after some internal stuff. > > > > Thanks Simon. > > Just to make it clearer to me, once bug 91117 is done, there should still work to be done here? Ok, given the patch Simon uploaded to bug 91117, the answer seems 'yes'. But it also seems it overlaps or even overwrites most of the changes the patch in this bug does in RenderLayer and RenderLayerBacking ...
The original iOS5 branch also had a related setting that I found useful, and maybe we should consider having them here: void setAlwaysUseAcceleratedOverflowScroll(bool flag) bool alwaysUseAcceleratedOverflowScroll() const
(In reply to comment #31) > Ok, given the patch Simon uploaded to bug 91117, the answer seems 'yes'. But it also seems it overlaps or even overwrites most of the changes the patch in this bug does in RenderLayer and RenderLayerBacking ... Right, as far as I can tell the following things are still needed on top of bug 91117: - Letting ScrollingCoordinator know about scrollable layers. - Avoiding repainting layer contents after scrolling. - Possibly avoiding overflow clipping when calculating paint rects (e.g., RenderObject::computeRectForRepaint() in this patch).
(In reply to comment #32) > The original iOS5 branch also had a related setting that I found useful, and maybe we should consider having them here: > > void setAlwaysUseAcceleratedOverflowScroll(bool flag) > bool alwaysUseAcceleratedOverflowScroll() const I don't think you want those. The idea is they make every overflow:scroll use composting and create stacking context, which can break web pages.
(In reply to comment #33) > (In reply to comment #31) > > Ok, given the patch Simon uploaded to bug 91117, the answer seems 'yes'. But it also seems it overlaps or even overwrites most of the changes the patch in this bug does in RenderLayer and RenderLayerBacking ... > > Right, as far as I can tell the following things are still needed on top of bug 91117: > > - Letting ScrollingCoordinator know about scrollable layers. > - Avoiding repainting layer contents after scrolling. Yes on these two. > - Possibly avoiding overflow clipping when calculating paint rects (e.g., RenderObject::computeRectForRepaint() in this patch). I don't think you need any changes here; the repaintContainner logic should take case of this.
(In reply to comment #34) > (In reply to comment #32) > > The original iOS5 branch also had a related setting that I found useful, and maybe we should consider having them here: > > > > void setAlwaysUseAcceleratedOverflowScroll(bool flag) > > bool alwaysUseAcceleratedOverflowScroll() const > > I don't think you want those. The idea is they make every overflow:scroll use composting and create stacking context, which can break web pages. would you have example of broken web pages with that overly enabled?
Created attachment 154694 [details] Patch Work-in-progress patch against 91117 - Register scrolling layers with ScrollingCoordinator. - Implement bounds origin in Chromium compositor. - Keep scrolling contents layer under overflow controls. - Don't reparent overflow controls inside scrolling content. - Promote composited scrolling layers to composited layers. - Include composited scrolling layers in memory usage estimate. - Avoid repaints when scrolling (not working yet).
Comment on attachment 154694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154694&action=review > Source/WebCore/platform/graphics/chromium/LayerChromium.h:120 > + const FloatPoint& boundsOrigin() const { return m_boundsOrigin; } > + void setBoundsOrigin(const FloatPoint& origin) { m_boundsOrigin = origin; } What does bounds origin mean? Forgive my skepticism about adding another member, but are you sure this translation can't be combined into one of position, anchor, transform, sublayerTransform, or scrollPosition at the LayerChromium level?
(In reply to comment #38) > What does bounds origin mean? > > Forgive my skepticism about adding another member, but are you sure this translation can't be combined into one of position, anchor, transform, sublayerTransform, or scrollPosition at the LayerChromium level? As far as I can tell it could be baked into sublayerTransform, so I don't think the compositor actually needs to care about it. I'm with you on not adding any more degrees of freedom to the layer transforms.
(In reply to comment #35) > > - Possibly avoiding overflow clipping when calculating paint rects (e.g., RenderObject::computeRectForRepaint() in this patch). > > I don't think you need any changes here; the repaintContainner logic should take case of this. I tried to make this work but looks like the repaints are still getting clipped to the visible part of the overflow contents. For example: 1. Make an overflow div with lots of text. 2. Select a part of the text at the beginning and scroll to the end. 3. Deselect and scroll back to the beginning. => The text is still selected, because the deselect repaint got clipped. I don't quite understand how this should work without turning off the overflow clipping in RenderObject et al. Did I miss something?
(In reply to comment #40) > I don't quite understand how this should work without turning off the overflow clipping in RenderObject et al. Did I miss something? For what it's worth, Chromium explicitly doesn't clip repaint rects for frame scrolling so that we can know which parts of the layer are invalidated even if they're not visible and don't have to invalidate on scroll. See FrameView::clipsRepaints().
(In reply to comment #38) > (From update of attachment 154694 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=154694&action=review > > > Source/WebCore/platform/graphics/chromium/LayerChromium.h:120 > > + const FloatPoint& boundsOrigin() const { return m_boundsOrigin; } > > + void setBoundsOrigin(const FloatPoint& origin) { m_boundsOrigin = origin; } > > What does bounds origin mean? > > Forgive my skepticism about adding another member, but are you sure this translation can't be combined into one of position, anchor, transform, sublayerTransform, or scrollPosition at the LayerChromium level? We used boundsOrigin because it plays nicely with UIScrollViews on iOS.
(In reply to comment #42) > (In reply to comment #38) > > (From update of attachment 154694 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=154694&action=review > > > > > Source/WebCore/platform/graphics/chromium/LayerChromium.h:120 > > > + const FloatPoint& boundsOrigin() const { return m_boundsOrigin; } > > > + void setBoundsOrigin(const FloatPoint& origin) { m_boundsOrigin = origin; } > > > > What does bounds origin mean? > > > > Forgive my skepticism about adding another member, but are you sure this translation can't be combined into one of position, anchor, transform, sublayerTransform, or scrollPosition at the LayerChromium level? > > We used boundsOrigin because it plays nicely with UIScrollViews on iOS. I don't understand what UIScrollViews has to do with adding a new property to LayerChromium.
Created attachment 157739 [details] Diff with 91117 This patch set removes boundsOrigin from LayerChromium and below and instead bakes the value into the sublayerTransform.
Created attachment 157740 [details] Patch including 91117 This attachment also contains code from 91117 (so that it can apply for the ews bots).
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 157740 [details] Patch including 91117 Attachment 157740 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13479010
(In reply to comment #44) > Created an attachment (id=157739) [details] > Diff with 91117 > > This patch set removes boundsOrigin from LayerChromium and below and instead > bakes the value into the sublayerTransform. How does that work if the overflow element also has perspective style?
Created attachment 157805 [details] Patch Adding missing file: WebScrollDelegate.h.
Comment on attachment 157805 [details] Patch Attachment 157805 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13464933 New failing tests: fast/css/resize-corner-tracking-transformed.html compositing/geometry/clip.html compositing/overflow/overflow-scroll.html
Created attachment 157848 [details] Archive of layout-test-results from gce-cr-linux-06 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-06 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
The layout test failures appear to be due to 91117, so this should still be safe to review.
Comment on attachment 157805 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157805&action=review > Source/WebKit/chromium/features.gypi:182 > + 'ENABLE_OVERFLOW_SCROLLING=1', I don't think we want to enable this unconditionally just yet -- see, e.g., http://code.google.com/p/chromium/issues/detail?id=128325 for the kinds of problems it can cause.
Comment on attachment 157805 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157805&action=review > Source/WebCore/rendering/RenderLayerBacking.cpp:673 > + // FIXME: Should use DontSetNeedsDisplay to avoid repainting the entire layer. > + m_scrollingContentsLayer->setOffsetFromRenderer(scrollingContentsOffset, GraphicsLayer::SetNeedsDisplay); why the fixme instead of using it right way?
(In reply to comment #54) > > + // FIXME: Should use DontSetNeedsDisplay to avoid repainting the entire layer. > > + m_scrollingContentsLayer->setOffsetFromRenderer(scrollingContentsOffset, GraphicsLayer::SetNeedsDisplay); > > why the fixme instead of using it right way? The problem is that with the current patch, doing this produces a lot of rendering corruption which we need to fix first. I think we'll want to get bug 91117 landed first to provide the scrolling layer infrastructure and then use this bug to make prepainting et al work.
(In reply to comment #55) > (In reply to comment #54) > > > + // FIXME: Should use DontSetNeedsDisplay to avoid repainting the entire layer. > > > + m_scrollingContentsLayer->setOffsetFromRenderer(scrollingContentsOffset, GraphicsLayer::SetNeedsDisplay); > > > > why the fixme instead of using it right way? > > The problem is that with the current patch, doing this produces a lot of rendering corruption which we need to fix first. I think we'll want to get bug 91117 landed first to provide the scrolling layer infrastructure and then use this bug to make prepainting et al work. 91117 has landed now - what's next for this patch?
Comment on attachment 157805 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157805&action=review This needs a rebase now that 91117 has landed >> Source/WebCore/rendering/RenderLayerBacking.cpp:673 >> + m_scrollingContentsLayer->setOffsetFromRenderer(scrollingContentsOffset, GraphicsLayer::SetNeedsDisplay); > > why the fixme instead of using it right way? Now that bug 91117 has landed I'd like to see an attempt at this.
(In reply to comment #57) > > why the fixme instead of using it right way? > > Now that bug 91117 has landed I'd like to see an attempt at this. Agreed, I'm on it. The way I see it there are two things we need: telling ScrollingCoordinator about the scrolling layers and implementing prepainting so that we don't need to fully invalidate the layers as they scroll. I've updated the title to better match the scope -- please let me know if want this bug split further.
(In reply to comment #58) > (In reply to comment #57) > > > why the fixme instead of using it right way? > > > > Now that bug 91117 has landed I'd like to see an attempt at this. > > Agreed, I'm on it. The way I see it there are two things we need: telling ScrollingCoordinator about the scrolling layers and implementing prepainting so that we don't need to fully invalidate the layers as they scroll. > > I've updated the title to better match the scope -- please let me know if want this bug split further. It's hard to say when to split. I think you should definitely split cross-platform changes from chromium-only changes, and then split as things get unwieldy.
(In reply to comment #59) > It's hard to say when to split. I think you should definitely split cross-platform changes from chromium-only changes, and then split as things get unwieldy. Ok. I'll use this bug for the registration part since it's (mostly) Chromium and will open another bug for prepainting.
Created attachment 161523 [details] Patch
Comment on attachment 161523 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161523&action=review > Source/Platform/ChangeLog:3 > + Register scrolling layers with ScrollingCoordinator this is an ~80% chromium-specific change with some changes in cross-platform code like RenderLayerBacking/etc. Please split the chromium parts out into a different patch so it can be reviewed separately. > Source/Platform/chromium/public/WebLayer.h:177 > + virtual void setScrollDelegate(WebLayerScrollDelegate*) = 0; document this please - in particular what the rules are for the lifetime of the passed in pointer (outlives the WebLayer, I'm guessing?) and whether you can have multiple (I'm guessing no?) > Source/Platform/chromium/public/WebLayerScrollDelegate.h:32 > +class WebLayerScrollDelegate { s/Delegate/Client/, please. we shouldn't be using 'delegate' in the WebKit API > Source/Platform/chromium/public/WebLayerScrollDelegate.h:35 > +}; declared d'tor please (protected most likely since you shouldn't call delete on a client ptr) > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:33 > +#include "LayerChromium.h" this is a really bad dependency - you can't depend on LayerChromium outside of the compositor implementation > Source/WebCore/platform/ScrollableArea.h:173 > + virtual bool usesCompositedScrolling() const { return false; } why is this outside of USE(ACCELERATED_COMPOSITING) ?
Comment on attachment 161523 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161523&action=review >> Source/Platform/ChangeLog:3 >> + Register scrolling layers with ScrollingCoordinator > > this is an ~80% chromium-specific change with some changes in cross-platform code like RenderLayerBacking/etc. Please split the chromium parts out into a different patch so it can be reviewed separately. Sure, here's the Chromium part: https://bugs.webkit.org/show_bug.cgi?id=95679 >> Source/Platform/chromium/public/WebLayer.h:177 >> + virtual void setScrollDelegate(WebLayerScrollDelegate*) = 0; > > document this please - in particular what the rules are for the lifetime of the passed in pointer (outlives the WebLayer, I'm guessing?) and whether you can have multiple (I'm guessing no?) Good idea. I've added a comment block. >> Source/Platform/chromium/public/WebLayerScrollDelegate.h:32 >> +class WebLayerScrollDelegate { > > s/Delegate/Client/, please. we shouldn't be using 'delegate' in the WebKit API Done. FWIW, this was modeled after WebAnimationDelegate. >> Source/Platform/chromium/public/WebLayerScrollDelegate.h:35 >> +}; > > declared d'tor please (protected most likely since you shouldn't call delete on a client ptr) Done. >> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:33 >> +#include "LayerChromium.h" > > this is a really bad dependency - you can't depend on LayerChromium outside of the compositor implementation Sorry, I added this include by mistake; there isn't an actual dependency here. >> Source/WebCore/platform/ScrollableArea.h:173 >> + virtual bool usesCompositedScrolling() const { return false; } > > why is this outside of USE(ACCELERATED_COMPOSITING) ? This used to be so because RenderLayer exposes usesCompositedScrolling() both with and without USE(ACCELERATED_COMPOSITING). I've now rearranged things so that RenderLayer uses OVERRIDE for usesCompositedScrolling() based on the build configuration.
Created attachment 161924 [details] Patch Cross-platform patch.
Comment on attachment 161924 [details] Patch R=me
Comment on attachment 161924 [details] Patch Clearing flags on attachment: 161924 Committed r127480: <http://trac.webkit.org/changeset/127480>
All reviewed patches have been landed. Closing bug.
(In reply to comment #57) > (From update of attachment 157805 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=157805&action=review > > This needs a rebase now that 91117 has landed > > >> Source/WebCore/rendering/RenderLayerBacking.cpp:673 > >> + m_scrollingContentsLayer->setOffsetFromRenderer(scrollingContentsOffset, GraphicsLayer::SetNeedsDisplay); > > > > why the fixme instead of using it right way? > > Now that bug 91117 has landed I'd like to see an attempt at this. If you referred to an attempt to fix the FIXME I pointed out above, if did not happen afaik.
(In reply to comment #68) > If you referred to an attempt to fix the FIXME I pointed out above, if did not happen afaik. That's right, I've opened https://bugs.webkit.org/show_bug.cgi?id=96087 to track that.