Summary: | [CoordinatedGraphics] Use ScrollingCoordinator to track fixed layers | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Caio Marcelo de Oliveira Filho <cmarcelo> | ||||||||||
Component: | New Bugs | Assignee: | Caio Marcelo de Oliveira Filho <cmarcelo> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | allan.jensen, andersca, anilsson, cdumez, dongseong.hwang, gavinp, gyuyoung.kim, jamesr, jbriance, jturcotte, kenneth, luiz, mikhail.pozdnyakov, noam, rafael.lobo, rakuco, simon.fraser, tonikitoo, webkit.review.bot, zeno | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 109431 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Caio Marcelo de Oliveira Filho
2013-02-05 16:27:28 PST
Created attachment 186723 [details]
Patch
I'll elaborate the ChangeLog tomorrow. Comment on attachment 186723 [details] Patch Attachment 186723 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16388038 Comment on attachment 186723 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186723&action=review Great improvement! > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:-519 > - Great! where is client that uses ScrollingCoordinatorCoordinatedGraphics? is this patch WIP yet? > Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:76 > + webPage->corePage()->settings()->setScrollingCoordinatorEnabled(true); There are many settings related to CG in WebPage::setUseFixedLayout, although I'm not sure it is right. Created attachment 186837 [details]
Patch
Comment on attachment 186837 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186837&action=review > Source/WebCore/ChangeLog:19 > + not the other way round. > + > + * Target.pri: info abuot testing > Source/WebKit2/ChangeLog:8 > + WebCore keeps ScrollingCoordinator up-to-date about whether Layers are fixed or not, so we layers* > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:471 > + if (m_fixedToViewport == isFixed) > + return; > + m_fixedToViewport = isFixed; newline after return is common to make is more obvious to spot > Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:77 > #if USE(COORDINATED_GRAPHICS) > m_alwaysUseCompositing = true; > + webPage->corePage()->settings()->setScrollingCoordinatorEnabled(true); > #endif I am not sure this is the right place. We do similar things together with construction of our view or when setfixedlayout is turned on. Comment on attachment 186723 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186723&action=review Thanks for looking into the patch. >> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:-519 >> - > > Great! > > where is client that uses ScrollingCoordinatorCoordinatedGraphics? is this patch WIP yet? Note sure what was your question. If it was about where's the code that calls ScrollingCoordinator, I've explained a bit in the ChangeLog: RenderLayerBacking currently informs the ScrollingCoordinator about changes to its GraphicsLayers. >> Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:76 >> + webPage->corePage()->settings()->setScrollingCoordinatorEnabled(true); > > There are many settings related to CG in WebPage::setUseFixedLayout, although I'm not sure it is right. In the updated patch added a check for accelerated compositing for fixed layers in the code for ScrollingCoordinator so that we don't send updates if we are not using fixed layout. Adding a check here would be not enough since at this point we not necessarily set the FixedLayout option, we would have to hook into that. Comment on attachment 186837 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186837&action=review >> Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:77 >> #endif > > I am not sure this is the right place. We do similar things together with construction of our view or when setfixedlayout is turned on. See my comment to the other review. Comment on attachment 186837 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186837&action=review > Source/WebCore/page/scrolling/coordinated/ScrollingCoordinatorCoordinatedGraphics.cpp:28 > + You either need to also wrap this file in USE(COORDINATED_GRAPHICS), or only include it on build level when it is enabled. > Source/WebCore/page/scrolling/coordinated/ScrollingCoordinatorCoordinatedGraphics.h:40 > +#include "ScrollingCoordinator.h" > + > +namespace WebCore { > + > +class ScrollingCoordinatorCoordinatedGraphics : public ScrollingCoordinator { > +public: > + explicit ScrollingCoordinatorCoordinatedGraphics(Page*); > + > + virtual void setLayerIsFixedToContainerLayer(GraphicsLayer*, bool); > +}; > + > +} // namespace WebCore #if USE(COORDINATED_GRAPHICS) Comment on attachment 186837 [details]
Patch
Caio asked me to remove r? because the patch is missing buildsystem stuff for GTK and EFL.
Comment on attachment 186837 [details] Patch Attachment 186837 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16388323 Comment on attachment 186837 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186837&action=review > Source/WebCore/ChangeLog:25 > + (WebCore::ScrollingCoordinatorCoordinatedGraphics::ScrollingCoordinatorCoordinatedGraphics): The name is very verbose... How about ScrollingCoordinatorCGfx? Created attachment 186876 [details]
Patch
Comment on attachment 186876 [details]
Patch
Fixing some issues pointed by Allan and Kenneth. Added a manual test.
I think CGfx could still be confusing with CG. The current name is consistent with the USE() flag.
Comment on attachment 186876 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186876&action=review > Source/WebCore/ChangeLog:15 > + The new code also works in new situations where the previous was broken: if a layer changed > + from being fixed to not fixed (but still kept as a layer for other reasons), the layer will sticky position you mean? Simon Fraser, could you review this? thanks (In reply to comment #15) > (From update of attachment 186876 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186876&action=review > > > Source/WebCore/ChangeLog:15 > > + The new code also works in new situations where the previous was broken: if a layer changed > > + from being fixed to not fixed (but still kept as a layer for other reasons), the layer will > > sticky position you mean? No, the manual test he is adding shows a test case where we currently fails to track whether a layer is fixed or not. Once we start with a fixed layer, if javascript changes its fixed state to false, we've never changed this back on CoordinatedGraphics, besides it really happened on WebProcess. Using ScrollingCoordinator will make us avoid cases like this one. Comment on attachment 186876 [details]
Patch
I'm not a CoordinatedGraphics expert but this seems fine.
Also, I think we should not worry about sticky right now. Let's move forward and properly support fixed layers on CoordinatedGraphics as a first step. :-) (In reply to comment #18) > (From update of attachment 186876 [details]) > I'm not a CoordinatedGraphics expert but this seems fine. Can you sign off on the WebKit2 changes, so that one of our reviewers (Noam?) can review it? Comment on attachment 186876 [details]
Patch
r=me on the webkit2 changes.
Comment on attachment 186876 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186876&action=review > Source/WebCore/ChangeLog:19 > + Testing was done with the existing manual tests that make use of "position:fixed". Please explain here why this cannot be tested with a pixel test. > Source/WebCore/CMakeLists.txt:47 > + "${WEBCORE_DIR}/page/scrolling/coordinated" Let's use scrolling/CoordinatedGraphics (In reply to comment #22) > (From update of attachment 186876 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186876&action=review > > > Source/WebCore/ChangeLog:19 > > + Testing was done with the existing manual tests that make use of "position:fixed". > > Please explain here why this cannot be tested with a pixel test. > > > Source/WebCore/CMakeLists.txt:47 > > + "${WEBCORE_DIR}/page/scrolling/coordinated" > > Let's use scrolling/CoordinatedGraphics It is 'coordinated' under WebCore/platform/graphics, and lower-case seems to be the norm for port directory names under WebCore. (In reply to comment #23) > (In reply to comment #22) > > (From update of attachment 186876 [details] [details]) > > > Source/WebCore/CMakeLists.txt:47 > > > + "${WEBCORE_DIR}/page/scrolling/coordinated" > > > > Let's use scrolling/CoordinatedGraphics > > It is 'coordinated' under WebCore/platform/graphics, and lower-case seems to be the norm for port directory names under WebCore. "scrolling/coordinated" is ambiguous given that the whole thing is called ScrollingCoordinator. Suggestions: scrolling/coordinatedgraphics or scrolling/coordgfx (In reply to comment #24) > (In reply to comment #23) > > (In reply to comment #22) > > > (From update of attachment 186876 [details] [details] [details]) > > > > Source/WebCore/CMakeLists.txt:47 > > > > + "${WEBCORE_DIR}/page/scrolling/coordinated" > > > > > > Let's use scrolling/CoordinatedGraphics > > > > It is 'coordinated' under WebCore/platform/graphics, and lower-case seems to be the norm for port directory names under WebCore. > > "scrolling/coordinated" is ambiguous given that the whole thing is called ScrollingCoordinator. > > Suggestions: scrolling/coordinatedgraphics or scrolling/coordgfx scrolling/coordgfx should be fine. Created attachment 187087 [details]
Patch
Committed r142112: <http://trac.webkit.org/changeset/142112> This changeset breaks our build slave: http://build.webkit.org/builders/Qt%20Linux%20SH4%20Release/builds/19475 I think the files added in Target.pri are not OpenGL dependant and should not be in conditionnal use?(3D_GRAPHICS) (In reply to comment #25) > (In reply to comment #24) > > (In reply to comment #23) > > > (In reply to comment #22) > > > > (From update of attachment 186876 [details] [details] [details] [details]) > > > > > Source/WebCore/CMakeLists.txt:47 > > > > > + "${WEBCORE_DIR}/page/scrolling/coordinated" > > > > > > > > Let's use scrolling/CoordinatedGraphics > > > > > > It is 'coordinated' under WebCore/platform/graphics, and lower-case seems to be the norm for port directory names under WebCore. > > > > "scrolling/coordinated" is ambiguous given that the whole thing is called ScrollingCoordinator. > > > > Suggestions: scrolling/coordinatedgraphics or scrolling/coordgfx > > scrolling/coordgfx should be fine. I'd prefer scrolling/coordinatedgraphics. We don't use the "gfx" abomination anywhere else, do we? > I'd prefer scrolling/coordinatedgraphics. We don't use the "gfx" abomination anywhere else, do we?
That's what we ended up using.
Reverted r142112 for reason: New scrolling test crashes on Lion. Committed r142141: <http://trac.webkit.org/changeset/142141> (In reply to comment #31) > Reverted r142112 for reason: > > New scrolling test crashes on Lion. > > Committed r142141: <http://trac.webkit.org/changeset/142141> The patch does not touch Chromium or Mac code in any way. If the new test crashes, it is a pre-existing bug in your code, and you should skip the test until you fix the issue. If you're certain of that, I'm happy to reland it, and create a bug in Chromium for fixing the test. Thanks. Thanks. (In reply to comment #34) > If you're certain of that, I'm happy to reland it, and create a bug in Chromium for fixing the test. Thanks. > > Thanks. Yes, we're certain of that. The code in the patch touches Qt/EFL only. Reverted r142141 for reason: Reland r142112, will update Chromium expectations and create a Chromium bug instead for the crash. Committed r142143: <http://trac.webkit.org/changeset/142143> (In reply to comment #36) > Reverted r142141 for reason: > > Reland r142112, will update Chromium expectations and create a Chromium bug instead for the crash. > > Committed r142143: <http://trac.webkit.org/changeset/142143> Thank you for responding proactively, Gavin. I'm going to file the Chromium bug. Thanks for the test case. :D This patches appears to be causing a few regressions on WK2 EFL: compositing/iframes/iframe-composited-scrolling.html compositing/layer-creation/fixed-position-in-view-dynamic.html compositing/layer-creation/fixed-position-out-of-view-dynamic.html compositing/layer-creation/fixed-position-out-of-view-scroll-reason.html compositing/layer-creation/no-compositing-for-fixed-position-under-transform.html The diff looks like: -PASS +FAIL: Has viewport constrained objects without supporting fixed layers |