The existing logic should be extended to allow creating hardware compositing layers for scrollable (I)Frames, DIVS, and others. A new compositing trigger should be added to turn on/off this feature, and layers should only be created if the object is both scrollable and has actual overflow.
Created attachment 83890 [details] Patch
Can we have some tests?
Comment on attachment 83890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=83890&action=review > Source/WebCore/rendering/RenderLayer.cpp:3816 > + || isComposited(); Why do you need to change this logic? It doesn't seem like this patch should change this logic. > Source/WebCore/rendering/RenderLayerCompositor.cpp:-1206 > - return m_hasAcceleratedCompositing && layer->isSelfPaintingLayer(); Why do you need to change this? > Source/WebCore/rendering/RenderLayerCompositor.cpp:1305 > + FrameView* frameView = m_renderView->frameView(); > + // Some sites use tiny iframes to load hidden content, so don't composite those. > + if (frameView->layoutWidth() <= 1 || frameView->layoutHeight() <= 1) > + return false; This feels awkward - is not redundant with the check at 1309-1310? > Source/WebCore/rendering/RenderLayerCompositor.h:244 > // Whether a running transition or animation enforces the need for a compositing layer. > + bool requiresCompositingForScrollableOverflow(RenderObject*) const; This comment seems out of date now.
Comment on attachment 83890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=83890&action=review >> Source/WebCore/rendering/RenderLayer.cpp:3816 >> + || isComposited(); > > Why do you need to change this logic? It doesn't seem like this patch should change this logic. From what I understand isSelfPaintingLayer() decides whether a layer paints itself or some parent does it and is used in a bunch of places that deal with painting and hit testing. Especially with the latter I'm concerned that we need to give the correct answer here. Doesn't a layer have to be self-painting anyway, if we are using hw compositing and it got its own compositing layer? So pre-existing cases should not be affected. The thing here is that the pre-existing cases rely on things known before layout only, and the decision can be made more statically based on type and style. For the more general 'blocks that overflow' case, it seemed to work best to force the decision from requiresCompositingLayerForScrollableOverflow() and then just make sure isSelfPainting() layer gives good answers if we happened to make the layer composited. >> Source/WebCore/rendering/RenderLayerCompositor.cpp:-1206 >> - return m_hasAcceleratedCompositing && layer->isSelfPaintingLayer(); > > Why do you need to change this? This function is used in needsToBeComposited() and other places to see if the layer could be composited. It doesn't mean it necessarily will (the requiresCompositing*() functions will determine if it actually will be). Usually the check to create compositing layers is done before layout. If we set m_compositingDependsOnGeometry we will revisit things after layout. Since we don't know until after layout whether we really want to composite it, we have to be permissive here and then make the final decision after layout. Also I only want to set m_compositingDependsOnGeometry if there is a reasonable chance (based on style known before layout, i.e. overflow : 'scroll' or 'auto') that we might want to create a layer. >> Source/WebCore/rendering/RenderLayerCompositor.cpp:1305 >> + return false; > > This feels awkward - is not redundant with the check at 1309-1310? Actually, the check below uses contentSize, while the layout size would be the visible size. If a site uses a hidden (e.g. 1x1 iframe) to preload flash or so, I think the content size would have the full content dimensions, while the visible/layout size is 1x1. >> Source/WebCore/rendering/RenderLayerCompositor.h:244 >> + bool requiresCompositingForScrollableOverflow(RenderObject*) const; > > This comment seems out of date now. Oops, will move this back down.
Created attachment 84664 [details] Patch
Created attachment 84667 [details] Patch
I have made the following changes: * added layout tests with expected output (graphics layer tree). I had problems adding a runtime property to LayoutTestController::overridePreference(), because RenderLayerCompositor() caches m_compositingTriggers when it is first created so triggering a change from Javascript won't propagate properly. Therefore I have marked the tests as SKIPPED for now. * In RenderLayerCompositor::requiresCompositingForScrollableOverflow() I have moved the root layer check up so that we don't always set m_compositingDependsOnGeometry. * I have overwritten canBeProgramaticallyScrolled() for RenderView so that it correctly returns false now for frames and iframes with the 'scrolling' attribute set to 'no', and we don't create an unwanted layer.
Created attachment 84668 [details] Patch
Fixed typo "should create" -> "should not create" in overflow-iframe-layer.html.
Created attachment 84672 [details] Patch
Made the expected result reflect the typo fix.
Comment on attachment 84672 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84672&action=review > LayoutTests/platform/chromium/test_expectations.txt:3111 > +// Needs ScrollableOverflow compositing trigger. This test should be enabled on platforms allowing > +// this trigger. > +BUGWK55257 GPU SKIP : platform/chromium/compositing/layer-creation/overflow-block-layer.html = PASS > +BUGWK55257 GPU SKIP : platform/chromium/compositing/layer-creation/overflow-iframe-layer.html = PASS In chromium we don't SKIP tests unless they cause the test harness to crash in some way. Instead set the appropriate expectation ( = TEXT or whatnot). > Source/WebCore/rendering/RenderLayerCompositor.cpp:1292 > + bool isRootLayer = box->layer()->isRootLayer(); > + if (isRootLayer && !m_renderView->document()->frame()->tree()->parent()) { > + // Do not create a backing for root layers, these are handled differently. I'm a bit confused here - why do we need the two checks? Does isRootLayer not indicate whether the layer is a root layer? > Source/WebCore/rendering/RenderLayerCompositor.cpp:1305 > + // Some sites use tiny iframes to load hidden content, so don't composite those. > + if (frameView->layoutWidth() <= 1 || frameView->layoutHeight() <= 1) This still feels redundant with the check at 1310. If we don't want to make compositing layers for small things, we should just do the size check once IMO. > Source/WebCore/rendering/RenderView.cpp:818 > +bool RenderView::canBeProgramaticallyScrolled(bool) const > +{ > + if (node() && node()->isDocumentNode()) { > + // Handle scrolling="no" style for (i)frames. > + HTMLFrameOwnerElement* ownerElement = document()->ownerElement(); > + if (ownerElement && (ownerElement->hasTagName(HTMLNames::iframeTag) || ownerElement->hasTagName(HTMLNames::frameTag))) { > + HTMLFrameElementBase* frame = static_cast<HTMLFrameElementBase*>(ownerElement); > + if (frame->scrollingMode() == ScrollbarAlwaysOff) > + return false; > + } > + } > + > + return RenderBox::canBeProgramaticallyScrolled(false); > +} Supporting scrolling="no" on an iframe is a separate issue - please file another bug on this and attach this code + the corresponding test(s) to that bug.
(In reply to comment #7) > I have made the following changes: > > * added layout tests with expected output (graphics layer tree). > I had problems adding a runtime property to LayoutTestController::overridePreference(), because RenderLayerCompositor() caches m_compositingTriggers when it is first created so triggering a change from Javascript won't propagate properly. Therefore I have marked the tests as SKIPPED for now. Take a look at RenderLayerCompositor::cacheAcceleratedCompositingFlags() and its caller - I think the problem you are having has been solved. > > * In RenderLayerCompositor::requiresCompositingForScrollableOverflow() I have moved the root layer check up so that we don't always set m_compositingDependsOnGeometry. > > * I have overwritten canBeProgramaticallyScrolled() for RenderView so that it correctly returns false now for frames and iframes with the 'scrolling' attribute set to 'no', and we don't create an unwanted layer. This is nifty, but should be done in a separate bug.
Comment on attachment 84672 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84672&action=review >> Source/WebCore/rendering/RenderLayerCompositor.cpp:1292 >> + // Do not create a backing for root layers, these are handled differently. > > I'm a bit confused here - why do we need the two checks? Does isRootLayer not indicate whether the layer is a root layer? Actually isRootLayer() returns 'true' for RenderViews that belong to a frame or iframe also: bool isRootLayer() const { return renderer()->isRenderView(); } And we don't want to bail out for those.
Created attachment 84676 [details] Patch
> In chromium we don't SKIP tests unless they cause the test harness to crash in some way. Instead set the appropriate expectation ( = TEXT or whatnot). Fixed. > I'm a bit confused here - why do we need the two checks? Does isRootLayer not indicate whether the layer is a root layer? > > + if (frameView->layoutWidth() <= 1 || frameView->layoutHeight() <= 1) see comment above, unfortunately isRootLayer() is misleading and just returns whether it's a RenderView, so could also be an (i)frame. > > This still feels redundant with the check at 1310. If we don't want to make compositing layers for small things, we should just do the size check once IMO. Removed. > Supporting scrolling="no" on an iframe is a separate issue - please file another bug on this and attach this code + the corresponding test(s) to that bug. Removed, will file a separate bug.
https://bugs.webkit.org/show_bug.cgi?id=55737 filed for the nonscrollable iframes.
Created attachment 84779 [details] Patch
Removed the iframe 'scrolling=no' testcase... let's allow a layer to be created here for now.
Kinda blown away that I wasn't cc'd on this bug.
Comment on attachment 84779 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84779&action=review > Source/WebCore/rendering/RenderLayer.cpp:3816 > + || renderer()->isRenderIFrame() > + || isComposited(); You can't do this; the test becomes circular. See RenderLayerCompositor::canBeComposited(). Non-self-painting layers are used for overflow, so your changes here will break rendering in some cases. > Source/WebCore/rendering/RenderLayerCompositor.cpp:1230 > - return m_hasAcceleratedCompositing && layer->isSelfPaintingLayer(); > + if (!m_hasAcceleratedCompositing) > + return false; > + > + bool canBeComposited = layer->isSelfPaintingLayer(); > + > + if ((m_compositingTriggers & ChromeClient::ScrollableOverflowTrigger) > + && layer->renderer()->isRenderBlock() && !layer->renderer()->isTextControl()) { > + // Only do a loose check now, as this function will be queried before layout. > + // Whether there is actual overflow will be determined after layout > + // by requiresCompositingForScrollableOverflow(). > + canBeComposited |= toRenderBox(layer->renderer())->scrollsOverflow(); > + } > + > + return canBeComposited; This seems wrong.
Created attachment 84784 [details] Testcase Compositing for overflow is very hard, because overflow does not create stacking context. Try the attached test case with your changes.
Simon, thanks for your feedback and apologies for missing to CC you on this bug, my bad! (In reply to comment #21) > (From update of attachment 84779 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84779&action=review > > > Source/WebCore/rendering/RenderLayer.cpp:3816 > > + || renderer()->isRenderIFrame() > > + || isComposited(); > > You can't do this; the test becomes circular. See RenderLayerCompositor::canBeComposited(). > But isComposited() != canBeComposited(). The problem I had is that the more generic elements that I want to composite (like overflowing DIVs) are currently not self-painting. Also I didn't want to make all kinds of elements self-painting, if they will not be composited. Since I really only know until after layout, I did the inverse logic here of saying that if we eventually made the layer composited (because it is scrollable and does overflow), we should now be returning true from isSelfPaintingLayer(). In other words, I added isComposited() to the logic here to make sure we return true for 'self-painting' after we have decided to give this layers its own graphics layer in requiresCompositingForScrollableOverflow() (without duplicating the logic). > Non-self-painting layers are used for overflow, so your changes here will break rendering in some cases. > Wouldn't RenderLayers that have a GraphicsLayer (i.e. are composited) always be self-painting? I'm probably missing something, can you elaborate on what would break? On the other hand, so what would happen if I created a composited layer that ends up returning 'false' from isSelfPaintingLayer()? This seems to be used quite a bit including in hit testing logic. > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1230 > > - return m_hasAcceleratedCompositing && layer->isSelfPaintingLayer(); > > + if (!m_hasAcceleratedCompositing) > > + return false; > > + > > + bool canBeComposited = layer->isSelfPaintingLayer(); > > + > > + if ((m_compositingTriggers & ChromeClient::ScrollableOverflowTrigger) > > + && layer->renderer()->isRenderBlock() && !layer->renderer()->isTextControl()) { > > + // Only do a loose check now, as this function will be queried before layout. > > + // Whether there is actual overflow will be determined after layout > > + // by requiresCompositingForScrollableOverflow(). > > + canBeComposited |= toRenderBox(layer->renderer())->scrollsOverflow(); > > + } > > + > > + return canBeComposited; > > This seems wrong. This change is basically related to the one above. If I don't want to make all sorts of render block elements self-painting, I'd have to somehow avoid to bail out from needsToBeComposited()->canBeComposited() early, because the layer is not self-painting. I thought this change would be safe, because it only says whether a layer can theoretically be composited, and it seems like all RenderBlocks would be able to do that, and also because it doesn't mean that we will actually create a compositing layer - the latter would only happen if requiresCompositingLayer() or some other explicit logic would dictate the need for that.
(In reply to comment #22) > Created an attachment (id=84784) [details] > Testcase > > Compositing for overflow is very hard, because overflow does not create stacking context. Try the attached test case with your changes. Thanks for the testcase! I don't seem to see a problem with this particular case. In Chrome, the results look same with and without my changes. All 3 nodes (container, interleaved, inner) are part of the same (root) stacking context. With compositing, we create 3 sibling layers in the expected order (container, interleaved, inner from back to front), with the 'inner' div layer having a clipping layer inserted.
(In reply to comment #23) > Simon, thanks for your feedback and apologies for missing to CC you on this bug, my bad! > > (In reply to comment #21) > > (From update of attachment 84779 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=84779&action=review > > > > > Source/WebCore/rendering/RenderLayer.cpp:3816 > > > + || renderer()->isRenderIFrame() > > > + || isComposited(); > > > > You can't do this; the test becomes circular. See RenderLayerCompositor::canBeComposited(). > > > > But isComposited() != canBeComposited(). The problem I had is that the more generic elements that I want to composite (like overflowing DIVs) are currently not self-painting. In the current design, non-self-painting layers cannot be composited. Changing whether a layer is self-painting will change rendering. Non self-painting layers are used for overflow (they hold a clip). > Also I didn't want to make all kinds of elements self-painting, if they will not be composited. Since I really only know until after layout, I did the inverse logic here of saying that if we eventually made the layer composited (because it is scrollable and does overflow), we should now be returning true from isSelfPaintingLayer(). > > In other words, I added isComposited() to the logic here to make sure we return true for 'self-painting' after we have decided to give this layers its own graphics layer in requiresCompositingForScrollableOverflow() (without duplicating the logic). > > > > Non-self-painting layers are used for overflow, so your changes here will break rendering in some cases. > > > > Wouldn't RenderLayers that have a GraphicsLayer (i.e. are composited) always be self-painting? I'm probably missing something, can you elaborate on what would break? Compositing logic should never change whether something is self-painting. We've chosen to make some replaced elements self-painting so we can accelerate them (video, plugins etc), but doing so for container elements like divs will break the web. > > On the other hand, so what would happen if I created a composited layer that ends up returning 'false' from isSelfPaintingLayer()? This seems to be used quite a bit including in hit testing logic. It don't render any content, and probably invalidates some assumptions that the compositing code makes.
Simon, without touching canBeComposited() and isSelfPaintingLayer() my proposed change allows for compositing (i)frames that overflow. Assuming we would want to allow overflowing divs to be composited into their own layers also, what would be the right way to implement it? I was wondering if you were suggesting that even in order to achieve this goal we should not make such layers self-painting, or were you rather suggesting that it is risky/experimental to start forcing such elements through the self-painting codepaths? Also, about the testcase: Were you worried that with my change we would create an unwanted stacking context at the 'container' node?
(In reply to comment #26) > Simon, > > without touching canBeComposited() and isSelfPaintingLayer() my proposed change allows for compositing (i)frames that overflow. Please use separate patches for (i)frames and overflow: scroll divs. I think they share little code. > Assuming we would want to allow overflowing divs to be composited into their own layers also, what would be the right way to implement it? I'm not sure. One approach is break the web, and cause overflow to create stacking context. This was suggested to the CSS-WG a couple of years ago, and rejected. Otherwise, we have to be very careful to allow compositing for overflow without breaking things. I'd have to probably just try it to figure out if it's possible. One approach is to make a series of testcases similar to mine, where it matters that overflow does not create a SC. > Also, about the testcase: Were you worried that with my change we would create an unwanted stacking context at the 'container' node? I was worried that your changes would alter the rendering of the test.
Created attachment 85000 [details] Patch
(In reply to comment #27) > (In reply to comment #26) > > Simon, > > > > without touching canBeComposited() and isSelfPaintingLayer() my proposed change allows for compositing (i)frames that overflow. > > Please use separate patches for (i)frames and overflow: scroll divs. I think they share little code. > Done. I have undone changes to canBeComposited() and isSelfPaintingLayer(). Also, I removed the div testcase. See latest patch.
Comment on attachment 85000 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=85000&action=review > Source/WebCore/rendering/RenderLayerCompositor.cpp:1262 > +bool RenderLayerCompositor::requiresCompositingForScrollableOverflow(RenderObject* renderer) const > +{ > + if (!(m_compositingTriggers & ChromeClient::ScrollableOverflowTrigger)) > + return false; > + > + if (!renderer->isRenderBlock() || renderer->isTextControl()) > + return false; > + > + RenderBox* box = toRenderBox(renderer); I think you should treat iframes and overflow:scroll totally separately, rather than mushing "scrollable areas" together.
Comment on attachment 85000 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=85000&action=review Does the bug title still accurately describe what this patch does? > Source/WebCore/rendering/RenderLayerCompositor.cpp:1264 > + if (!box->canBeProgramaticallyScrolled(false)) It seems like this isn't quite the right test - at least to start it sounds like you are more interested in boxes that can be scrolled by user interaction, not just by javascript manipulating scrollTop/scrollLeft. There's also a canBeScrolledAndHasScrollableArea() helper function on RenderBox that encapsulates this and the scrollWidth/Height vs clientWidth/Height checks you are doing on lines 1289-1290. > Source/WebCore/rendering/RenderLayerCompositor.cpp:1282 > + if (contentBox.height() * contentBox.width() <= 1) This deserves a comment and a test.
Comment on attachment 85000 [details] Patch r- based on the new focus for this bug.
Created attachment 85016 [details] Patch
Simon, how does this look to you? (In reply to comment #33) > Created an attachment (id=85016) [details] > Patch
Comment on attachment 85016 [details] Patch LGTM
Comment on attachment 85016 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=85016&action=review > Source/WebCore/rendering/RenderLayerCompositor.cpp:1259 > +bool RenderLayerCompositor::requiresCompositingForScrollableFrame(RenderObject* renderer) const > +{ > + if (!(m_compositingTriggers & ChromeClient::ScrollableFrameTrigger)) > + return false; > + > + if (!renderer->isRenderView()) Given the phrasing works, I'd expect this to be called on the renderer in the parent document (the RenderIFrame or whatever), and affect compositing in the parent document. However, that's not what you're doing. So I think it's wrong to add a requiresCompositingForScrollableFrame(), and wasteful to call requiresCompositingForScrollableFrame() for every renderer in the document when it would only ever apply to the root. I think you should just do the root compositing check in one place. > Source/WebCore/rendering/RenderLayerCompositor.cpp:1289 > + return (view->scrollWidth() != view->clientWidth()) > + || (view->scrollHeight() != view->clientHeight()); What will cause compositing to be re-evaluated when the contents of the iframe grow such that it becomes scrollable?
Simon, I picked this up again today and looked at how this would work differently with respect to what you suggested when we chatted the other day. With respect to your question to what causes the compositing to be re-evaluated when e.g. the iframe size changes: With my attached patch, the compositing layers will always be updated after layout (even if we previously were not compositing, because I set 'm_compositingDependendsOnGeometry'). About handling the overflow from the RenderIFrame: needsToBeComposited() is currently not called for RenderIFrames, because RenderIFrames usually don't have a RenderLayer. They only create a RenderLayer it if the child is compositing. That's why it plays nicely with the attached change. Also it seems like the overflow would have to be detected by the child in one form or another anyway, because it has to happen after the child FrameView has completed layout. About using 'RenderLayerCompositor::m_forceCompositing', that seems to work fine too. However, not sure what a good place to force it would be. Somewhere at the end of FrameView::layout() is called? It could probably go into FrameView::layout before updateCompositingLayers() is called after the actual layout, so that if the document changes and does not overflow anymore we can disable m_forceCompositing again (compositing will still have to be updated because we don't know if 'force' was the only reason to turn it on). Otherwise, the graphics layers created by m_forceCompositing vs. using needsToBeComposited() for the RenderView's layer seem to be the same. Speaking of which, I don't understand the '(inCompositingMode() && layer->isRootLayer())' part in needsToBeComposited(). Why does the root layer need a backing when it already creates a 'root platform layer' (the latter being what seems to be used in compositing)?
(In reply to comment #37) > With respect to your question to what causes the compositing to be re-evaluated when e.g. the iframe size changes: With my attached patch, the compositing layers will always be updated after layout (even if we previously were not compositing, because I set 'm_compositingDependendsOnGeometry'). But the parent document many not see any layout when the iframe contents change. > About handling the overflow from the RenderIFrame: needsToBeComposited() is currently not called for RenderIFrames, because RenderIFrames usually don't have a RenderLayer. They only create a RenderLayer it if the child is compositing. That's why it plays nicely with the attached change. That's not true. <iframe style="position: relative"> will create a RenderIFrame with a RenderLayer. > Also it seems like the overflow would have to be detected by the child in one form or another anyway, because it has to happen after the child FrameView has completed layout. > > About using 'RenderLayerCompositor::m_forceCompositing', that seems to work fine too. However, not sure what a good place to force it would be. Somewhere at the end of FrameView::layout() is called? It could probably go into FrameView::layout before updateCompositingLayers() is called after the actual layout, so that if the document changes and does not overflow anymore we can disable m_forceCompositing again (compositing will still have to be updated because we don't know if 'force' was the only reason to turn it on). Something like that. > Otherwise, the graphics layers created by m_forceCompositing vs. using needsToBeComposited() for the RenderView's layer seem to be the same. > Speaking of which, I don't understand the '(inCompositingMode() && layer->isRootLayer())' part in needsToBeComposited(). Why does the root layer need a backing when it already creates a 'root platform layer' (the latter being what seems to be used in compositing)? m_rootPlatformLayer is the layer handed off to the native layer-hosting code. The root RenderLayer's GraphicsLayer (which is often a layer with no backing) exists to host other compositing layers in the document, and is required to get the geometry right.
Created attachment 89651 [details] Patch
> > Also it seems like the overflow would have to be detected by the child in one form or another anyway, because it has to happen after the child FrameView has completed layout. > > > > About using 'RenderLayerCompositor::m_forceCompositing', that seems to work fine too. However, not sure what a good place to force it would be. Somewhere at the end of FrameView::layout() is called? It could probably go into FrameView::layout before updateCompositingLayers() is called after the actual layout, so that if the document changes and does not overflow anymore we can disable m_forceCompositing again (compositing will still have to be updated because we don't know if 'force' was the only reason to turn it on). > > Something like that. > Ok, see new patch. I have tried to add tests for entering and leaving compositing when the iframe content changes, but can someone tell me if there is a way to do this that is not conceptually flaky and ugly with respect to the setTimeout() (which I copied from other tests)?
Simon, how does the new patch look? Thanks.
Comment on attachment 89651 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89651&action=review Still not a big fan of this change. > Source/WebCore/page/FrameView.cpp:948 > + if (root->view()->document()->frame()->tree()->parent()) There has to be a better way than root->view()->document()->frame()->tree()->parent() > Source/WebCore/page/FrameView.cpp:949 > + compositor->setForceCompositingMode(compositor->requiresCompositingForScrollableFrame(view) ? true : false); No need for foo ? true : false. Also, this could be just one method on the compositor: compositor->forceCompostingForScrollableFrame(). So you want scrollable frames to composite even if nothing else is triggering composting mode? Why not just always use compositing? > Source/WebCore/page/Settings.h:313 > + void setAcceleratedCompositingForScrollableFrameEnabled(bool); > + bool acceleratedCompositingForScrollableFrameEnabled() const { return m_acceleratedCompositingForScrollableFrameEnabled; } I'd name these acceleratedCompositingForScrollableFrame_s_Enabled I don't really like the fact that you're adding a setting. Will this get toggled at runtime? Can it just be a compile-time thing? > Source/WebCore/rendering/RenderLayerCompositor.cpp:1278 > + if (!ownerElement || (!ownerElement->hasTagName(iframeTag) && !ownerElement->hasTagName(frameTag))) > + return false; What about <object src="foo.html">?
Comment on attachment 89651 [details] Patch Simon's comments sound like an r- to me.
Created attachment 100879 [details] Patch
> > Source/WebCore/page/FrameView.cpp:948 > > + if (root->view()->document()->frame()->tree()->parent()) > > There has to be a better way than root->view()->document()->frame()->tree()->parent() Done. > > > Source/WebCore/page/FrameView.cpp:949 > > + compositor->setForceCompositingMode(compositor->requiresCompositingForScrollableFrame(view) ? true : false); > > No need for foo ? true : false. > Done. > Also, this could be just one method on the compositor: compositor->forceCompostingForScrollableFrame(). > Done. > So you want scrollable frames to composite even if nothing else is triggering composting mode? Why not just always use compositing? Yes, I wanted it separate. Also the 'force compositing' setting only forces it on the root layer but not on frame/child compositors. > > > Source/WebCore/page/Settings.h:313 > > + void setAcceleratedCompositingForScrollableFrameEnabled(bool); > > + bool acceleratedCompositingForScrollableFrameEnabled() const { return m_acceleratedCompositingForScrollableFrameEnabled; } > > I'd name these acceleratedCompositingForScrollableFrame_s_Enabled > Done. > I don't really like the fact that you're adding a setting. Will this get toggled at runtime? Can it just be a compile-time thing? > At runtime would be nice to be able to support a cmdline flag. Also, I tried to mimick the other compositing triggers which have similar settings. > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1278 > > + if (!ownerElement || (!ownerElement->hasTagName(iframeTag) && !ownerElement->hasTagName(frameTag))) > > + return false; > > What about <object src="foo.html">? Maybe handle separately?
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:1289 > > + return (view->scrollWidth() != view->clientWidth()) > > + || (view->scrollHeight() != view->clientHeight()); > > What will cause compositing to be re-evaluated when the contents of the iframe grow such that it becomes scrollable? I think this is a valid point. How are you handling this?
(In reply to comment #46) > > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1289 > > > + return (view->scrollWidth() != view->clientWidth()) > > > + || (view->scrollHeight() != view->clientHeight()); > > > > What will cause compositing to be re-evaluated when the contents of the iframe grow such that it becomes scrollable? > > I think this is a valid point. How are you handling this? The evaluation is always performed after layout in FrameView.cpp, followed by a call to updateCompositingLayers(). When we were already compositing, and the iframe is not scrollable anymore, we will do a hierarchical/recursive update as is (which means we will leave it if there is no other reason left to be compositing). When we were not compositing, and the iframe became scrollable, we will force compositing on this frame and enableCompositingMode() from updateCompositingLayers(). Also see attached test cases for enter-compositing and leave-compositing.
Comment on attachment 100879 [details] Patch Attachment 100879 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9092113 New failing tests: editing/pasteboard/paste-noscript.html fast/repaint/line-flow-with-floats-2.html fast/events/page-visibility-iframe-move-test.html fast/dom/shadow/activeelement-should-be-shadowhost.html fast/frames/iframe-reparenting.html fast/frames/sandboxed-iframe-attribute-parsing.html fast/dom/HTMLDocument/active-element-frames.html fast/forms/input-number-large-padding.html fast/forms/input-spinbutton-capturing.html fast/loader/grandparent-completion-starts-redirect.html http/tests/misc/drag-not-loaded-image.html fast/events/pageshow-pagehide.html fast/events/gc-freeze-with-attribute-listeners.html fast/frames/iframe-no-src-set-location.html fast/repaint/line-flow-with-floats-1.html fast/repaint/line-flow-with-floats-3.html fast/repaint/fixed-move-after-keyboard-scroll.html fast/frames/sandboxed-iframe-navigation-allowed.html fast/forms/input-number-events.html http/tests/inspector/console-resource-errors.html
Created attachment 100937 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 100879 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100879&action=review > Source/WebCore/page/ChromeClient.h:268 > + ScrollableFrameTrigger = 1 << 5, Ports other than chromium set m_compositingTriggers = AllTriggers, meaning that they will also get composited iframes. Do we want that? > Source/WebCore/rendering/RenderLayerCompositor.cpp:1351 > +bool RenderLayerCompositor::requiresCompositingForScrollableFrame(RenderView* view) const Doesn't this method need to be called from requiresCompositingLayer()? If not, what is it that guarantees that the iframe gets its own backing? > Source/WebCore/rendering/RenderLayerCompositor.cpp:1907 > +void RenderLayerCompositor::forceCompostingForScrollableFrame(RenderView* view) Typo: forceCompos_i_tingForScrollableFrame
Daniel's out for a little bit, so I'm going to pick up this patch and see if I can get it to a landable state.
(In reply to comment #50) > (From update of attachment 100879 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=100879&action=review > > > Source/WebCore/page/ChromeClient.h:268 > > + ScrollableFrameTrigger = 1 << 5, > > Ports other than chromium set m_compositingTriggers = AllTriggers, meaning that they will also get composited iframes. Do we want that? Grep says this is just the Qt port. I'll CC noamr to see if ChromeClientQt should be adjusted or if this behavior is ok. > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1351 > > +bool RenderLayerCompositor::requiresCompositingForScrollableFrame(RenderView* view) const > > Doesn't this method need to be called from requiresCompositingLayer()? If not, what is it that guarantees that the iframe gets its own backing? requiresCompositingLayer is a check for if a layer needs to be composited. This function checks if a view needs to be composited. It gets a backing via the force compositing mode flag which triggers enableCompositingMode() during updateCompositedLayers(), which in turn calls ensureRootLayer().
> > Ports other than chromium set m_compositingTriggers = AllTriggers, meaning that they will also get composited iframes. Do we want that? > > Grep says this is just the Qt port. I'll CC noamr to see if ChromeClientQt should be adjusted or if this behavior is ok. For now it's ok. We can always change our triggers if we have to. Is there anything specific we need to add in GraphicsLayer to support composited IFrames? My assumption was that the changes are mainly in the DOM<->RenderTree domain.
(In reply to comment #53) > Is there anything specific we need to add in GraphicsLayer to support composited IFrames? My assumption was that the changes are mainly in the DOM<->RenderTree domain. IFrames can already become composited if there's another compositing trigger inside that frame. So, no extra changes in GraphicsLayer should be needed; this is just a change about when that compositing behavior is triggered.
Comment on attachment 100879 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100879&action=review >>> Source/WebCore/page/ChromeClient.h:268 >>> + ScrollableFrameTrigger = 1 << 5, >> >> Ports other than chromium set m_compositingTriggers = AllTriggers, meaning that they will also get composited iframes. Do we want that? > > Grep says this is just the Qt port. I'll CC noamr to see if ChromeClientQt should be adjusted or if this behavior is ok. The value for m_compositingTriggers is set by calling allowedCompositingTriggers() on the ChromeClient. The default implementation used by all other ports returns AllTriggers: http://www.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/page/ChromeClient.h&exact_package=chromium&q=m_compositingtriggers&type=cs&l=274 Chromium overrides this method but I don't believe other ports (including Safari) do.
(In reply to comment #55) > Chromium overrides this method but I don't believe other ports (including Safari) do. Hmm. Thanks for pointing that out. I'm not sure that I agree with the idea that a iframe with overflow on an otherwise non-composited page should *trigger* compositing. If the page is already composited, then it makes sense to require compositing for the iframe. If this were how it behaved, I'd have no problem turning this on by default for other ports. Given that, what we'd really want is to be able to conditionally require compositing on these iframes. (Please correct me if I'm wrong here, but) in my understanding of how layout and computing compositing requirements works, we may not know when we're looking at an subframe whether or not the main frame is going to be composited or not, because some later part of the page might be the compositing trigger. I'm not sure that there's a good way to solve this. Needing to do layout again to get to a stable result seems like a bad approach. jamesr suggested the other day that maybe instead of a setting we should just tie this to the force compositing flag. I'm coming back around to that idea. This would sidestep the problem of other ports and would provide a much more definitive trigger for when to composite these subframes.
Created attachment 102845 [details] Patch
(In reply to comment #57) > Created an attachment (id=102845) [details] > Patch Here's something along the lines of my previous comment, where the trigger has been removed and the force compositing mode flag is used instead. I also cleaned up the requiresCompositingForScrollableFrame function to use functions that existed elsewhere rather than duplicating code. I also pushed everything into updateCompositingLayers rather than having FrameView call a function to set an internal RLC flag. As the tests don't apply with force compositing mode, I removed them. I continue to be unhappy that force compositing mode can't be tested in DRT and would love to add this to layoutTestController in a future patch.
Created attachment 102849 [details] Patch
(In reply to comment #59) > Created an attachment (id=102849) [details] > Patch Edited to also add a scrollbar check. Subframes without scrollbars don't get composited, as they seem less likely to be scrolled.
Comment on attachment 102849 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102849&action=review This is looking nice and simple now! > Source/WebCore/rendering/RenderLayerCompositor.cpp:1381 > + // Don't go into compositing mode if height or width are zero, or size is 1x1. Watch indent.
Comment on attachment 102849 [details] Patch Needs tests.
(In reply to comment #62) > (From update of attachment 102849 [details]) > Needs tests. In that case, I'll need to add support to LayoutTestController so that the force compositing mode flag can be turned on dynamically in DumpRenderTree. As it stands, this patch is untestable as-is other than via manual testing.
You should think about using window.internal of that kind of thing.
Created attachment 103311 [details] Patch
(In reply to comment #65) > Created an attachment (id=103311) [details] > Patch Now with tests (which depend on functionality from bug 65777).
Created attachment 103385 [details] Patch
(In reply to comment #67) > Created an attachment (id=103385) [details] > Patch Same patch, just giving ews another shot now that it applies cleanly against ToT.
(In reply to comment #68) > (In reply to comment #67) > > Created an attachment (id=103385) [details] [details] > > Patch > > Same patch, just giving ews another shot now that it applies cleanly against ToT. smfr, jamesr: ping?
Comment on attachment 103385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103385&action=review Looks pretty good, except the 1x1 stuff. > Source/WebCore/rendering/RenderLayerCompositor.cpp:1388 > + // Don't go into compositing mode if height or width are zero, or size is 1x1. > + // (This is sometimes used to load flash in a hidden iframe). > + IntRect contentBox = view->contentBoxRect(); > + if (contentBox.height() * contentBox.width() <= 1) > + return false; If we're going to have this code, we should have tests for it. The comment is also incorrect - a 1x1 frame will not make us 'go into compositing mode', since this behavior is behind --force-compositing-mode. I'd honestly prefer that we just delete this unless there's some compelling reason to leave it around, 1x1 composited frames should not be expensive for us.
Created attachment 103522 [details] Patch
(In reply to comment #70) > (From update of attachment 103385 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=103385&action=review > > Looks pretty good, except the 1x1 stuff. > > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1388 > > + // Don't go into compositing mode if height or width are zero, or size is 1x1. > > + // (This is sometimes used to load flash in a hidden iframe). > > + IntRect contentBox = view->contentBoxRect(); > > + if (contentBox.height() * contentBox.width() <= 1) > > + return false; > > If we're going to have this code, we should have tests for it. > > The comment is also incorrect - a 1x1 frame will not make us 'go into compositing mode', since this behavior is behind --force-compositing-mode. I'd honestly prefer that we just delete this unless there's some compelling reason to leave it around, 1x1 composited frames should not be expensive for us. Removed. Thanks for pointing that out. (This last patch also merges where force compositing mode is set so that subframe and main frames assign its value in the same block of code.)
Comment on attachment 103522 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103522&action=review > Source/WebCore/rendering/RenderLayerCompositor.cpp:1377 > + ScrollView* scrollView = m_renderView->frameView(); > + if (!scrollView->verticalScrollbar() && !scrollView->horizontalScrollbar()) > + return false; > + > + return view->canBeScrolledAndHasScrollableArea(); This is still a little funky. view == m_renderView here always, right? Why are we checking for scrollbars on the FrameView and checking for scroll overflow on the view? Suggestions: Don't pass a RenderView* as a parameter, just use m_renderView. Also figure out if you really need to check for scrollbars existing and for canBeScrolled..(), since the latter will check for the case where there's no overflow.
(In reply to comment #73) > Also figure out if you really need to check for scrollbars existing and for canBeScrolled..(), since the latter will check for the case where there's no overflow. This needed a test anyway.
Created attachment 103533 [details] Patch
(In reply to comment #75) > Created an attachment (id=103533) [details] > Patch The scrollbar check is required if we want to not composite overflow:hidden iframes (which I think is reasonable). I can't think of any case where canBeScrolledEtc() would be true but scrollbars would be false. The function is now simplified even more and I added a test for overflow:hidden.
Comment on attachment 103533 [details] Patch Cool, R=me.
Committed r92873: <http://trac.webkit.org/changeset/92873>