Bug 108990 – [CoordinatedGraphics] Use ScrollingCoordinator to track fixed layers

• Component: New Bugs
• Status: RESOLVED FIXED

[Caio Marcelo de Oliveira Filho]

[CoordinatedGraphics] Use ScrollingCoordinator to track fixed layers

[Caio Marcelo de Oliveira Filho]

Created attachment 186723 [details]
Patch

[Caio Marcelo de Oliveira Filho]

I'll elaborate the ChangeLog tomorrow.

[EFL EWS Bot]

Comment on attachment 186723 [details]
Patch

Attachment 186723 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16388038

[Dongseong Hwang]

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.

[Caio Marcelo de Oliveira Filho]

Created attachment 186837 [details]
Patch

[Kenneth Rohde Christiansen]

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.

[Caio Marcelo de Oliveira Filho]

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.

[Caio Marcelo de Oliveira Filho]

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.

[Allan Sandfeld Jensen]

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)

[Jesus Sanchez-Palencia]

Comment on attachment 186837 [details]
Patch

Caio asked me to remove r? because the patch is missing buildsystem stuff for GTK and EFL.

[EFL EWS Bot]

Comment on attachment 186837 [details]
Patch

Attachment 186837 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16388323

[Noam Rosenthal]

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?

[Caio Marcelo de Oliveira Filho]

Created attachment 186876 [details]
Patch

[Caio Marcelo de Oliveira Filho]

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.

[Kenneth Rohde Christiansen]

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?

[Kenneth Rohde Christiansen]

Simon Fraser, could you review this? thanks

[Rafael Brandao]

(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.

[Simon Fraser (smfr)]

Comment on attachment 186876 [details]
Patch

I'm not a CoordinatedGraphics expert but this seems fine.

[Rafael Brandao]

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. :-)

[Kenneth Rohde Christiansen]

(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?

[Simon Fraser (smfr)]

Comment on attachment 186876 [details]
Patch

r=me on the webkit2 changes.

[Noam Rosenthal]

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

[Allan Sandfeld Jensen]

(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.

[Jocelyn Turcotte]

(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

[Noam Rosenthal]

(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.

[Caio Marcelo de Oliveira Filho]

Created attachment 187087 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Committed r142112: <http://trac.webkit.org/changeset/142112>

[Julien Brianceau]

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)

[Simon Fraser (smfr)]

(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?

[Noam Rosenthal]

> I'd prefer scrolling/coordinatedgraphics. We don't use the "gfx" abomination anywhere else, do we?
That's what we ended up using.

[Gavin Peters]

Reverted r142112 for reason:

New scrolling test crashes on Lion.

Committed r142141: <http://trac.webkit.org/changeset/142141>

[Gavin Peters]

See http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=scrollingcoordinator%2Fnon-fast-scrollable-region-transformed-iframe.html

[Allan Sandfeld Jensen]

(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.

[Gavin Peters]

If you're certain of that, I'm happy to reland it, and create a bug in Chromium for fixing the test. Thanks.

Thanks.

[Noam Rosenthal]

(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.

[Gavin Peters]

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>

[Noam Rosenthal]

(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.

[Gavin Peters]

I'm going to file the Chromium bug.

Thanks for the test case.  :D

[Chris Dumez]

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