Do not use composited scrolling when accelerated compositing turned off.
Created attachment 163145 [details] Patch At present, some pages turn off accelerated compositing (tracing, inspector). This causes a problem if we continue to act as if compostited scrolling is active when it really isn't.
Comment on attachment 163145 [details] Patch I think the check is fine, just one nit. View in context: https://bugs.webkit.org/attachment.cgi?id=163145&action=review > Source/WebCore/rendering/RenderLayer.cpp:1573 > + // Accelerated compositing may be forced off on some pages, so make sure we're really accelerated. I think this comment is a bit misleading as it can be thought of applying to the entire line below; I suggest you change it to something that includes all the conditions, e.g., "Only use composited scrolling if accelerated compositing is enabled and there is something to scroll in the layer." Alternatively, you could split out the hasAcceleratedCompositing() condition.
(In reply to comment #2) > (From update of attachment 163145 [details]) > I think the check is fine, just one nit. > > View in context: https://bugs.webkit.org/attachment.cgi?id=163145&action=review > > > Source/WebCore/rendering/RenderLayer.cpp:1573 > > + // Accelerated compositing may be forced off on some pages, so make sure we're really accelerated. > > I think this comment is a bit misleading as it can be thought of applying to the entire line below; I suggest you change it to something that includes all the conditions, e.g., "Only use composited scrolling if accelerated compositing is enabled and there is something to scroll in the layer." > > Alternatively, you could split out the hasAcceleratedCompositing() condition. Agreed. I had sort of thought the same thing, but not decided how to act on it yet :-)
Comment on attachment 163145 [details] Patch Are you saying you want to ignore the -webkit-overflow-touch: setting completely if compositing is disabled? That'll also change the stacking order, which seems wrong. We should try to avoid changing the rendering of the page due to things like accelerated compositing being on/off except where it's inevitable (i.e. some effect we can only do on a GPU). I don't get why you are setting this - ENABLE(OVERFLOW_SCROLLING) isn't set on chromium.
(In reply to comment #4) > (From update of attachment 163145 [details]) > Are you saying you want to ignore the -webkit-overflow-touch: setting completely if compositing is disabled? That'll also change the stacking order, which seems wrong. We should try to avoid changing the rendering of the page due to things like accelerated compositing being on/off except where it's inevitable (i.e. some effect we can only do on a GPU). > > I don't get why you are setting this - ENABLE(OVERFLOW_SCROLLING) isn't set on chromium. It's not a problem, yet. But Ian's patch to promote overflow layers to use composited scrolling will run into this (https://bugs.webkit.org/show_bug.cgi?id=94743). In particular, without this check, the call to usesCompositedScrolling() in RenderLayer::scrollTo() will try to bypass painting, which causes problems if accelerated compositing isn't actually on. The check to compositor()->hasAcceleratedCompositing() could move to RenderLayer::scrollTo(), though in that case the name "usesCompositedScrolling()" is misleading because the function will return true even if accelerated compositing is off.
(In reply to comment #4) > That'll also change the stacking order, which seems wrong. We should try to avoid changing the rendering of the page due to things like accelerated compositing being on/off except where it's inevitable (i.e. some effect we can only do on a GPU). I don't think this should break stacking context creation since that's done by StyleResolver only based on the CSS. This function already disables composited scrolling if there isn't anything to scroll, and platform/chromium/compositing/overflow/overflow-scrolling-touch-stacking-context.html checks that the stacking context is still there.
(In reply to comment #6) > (In reply to comment #4) > > That'll also change the stacking order, which seems wrong. We should try to avoid changing the rendering of the page due to things like accelerated compositing being on/off except where it's inevitable (i.e. some effect we can only do on a GPU). > > I don't think this should break stacking context creation since that's done by StyleResolver only based on the CSS. This function already disables composited scrolling if there isn't anything to scroll, and platform/chromium/compositing/overflow/overflow-scrolling-touch-stacking-context.html checks that the stacking context is still there. Just realized I left r? on when I uploaded this, didn't mean to ... it isn't ready for "prime time" just yet (and in reality may need to go in *after* Ian's patch to facilitate layout testing). Sorry about that.