Bug 108990 - [CoordinatedGraphics] Use ScrollingCoordinator to track fixed layers
Summary: [CoordinatedGraphics] Use ScrollingCoordinator to track fixed layers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Marcelo de Oliveira Filho
URL:
Keywords:
Depends on: 109431
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-05 16:27 PST by Caio Marcelo de Oliveira Filho
Modified: 2013-02-11 07:09 PST (History)
20 users (show)

See Also:


Attachments
Patch (15.55 KB, patch)
2013-02-05 16:30 PST, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (16.91 KB, patch)
2013-02-06 05:50 PST, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (20.51 KB, patch)
2013-02-06 10:35 PST, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (20.84 KB, patch)
2013-02-07 06:26 PST, Caio Marcelo de Oliveira Filho
noam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Marcelo de Oliveira Filho 2013-02-05 16:27:28 PST
[CoordinatedGraphics] Use ScrollingCoordinator to track fixed layers
Comment 1 Caio Marcelo de Oliveira Filho 2013-02-05 16:30:14 PST
Created attachment 186723 [details]
Patch
Comment 2 Caio Marcelo de Oliveira Filho 2013-02-05 16:31:44 PST
I'll elaborate the ChangeLog tomorrow.
Comment 3 EFL EWS Bot 2013-02-05 17:35:05 PST
Comment on attachment 186723 [details]
Patch

Attachment 186723 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16388038
Comment 4 Dongseong Hwang 2013-02-06 02:22:40 PST
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.
Comment 5 Caio Marcelo de Oliveira Filho 2013-02-06 05:50:28 PST
Created attachment 186837 [details]
Patch
Comment 6 Kenneth Rohde Christiansen 2013-02-06 05:54:27 PST
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 7 Caio Marcelo de Oliveira Filho 2013-02-06 05:57:02 PST
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 8 Caio Marcelo de Oliveira Filho 2013-02-06 05:58:00 PST
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 9 Allan Sandfeld Jensen 2013-02-06 06:14:34 PST
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 10 Jesus Sanchez-Palencia 2013-02-06 06:16:01 PST
Comment on attachment 186837 [details]
Patch

Caio asked me to remove r? because the patch is missing buildsystem stuff for GTK and EFL.
Comment 11 EFL EWS Bot 2013-02-06 07:14:38 PST
Comment on attachment 186837 [details]
Patch

Attachment 186837 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16388323
Comment 12 Noam Rosenthal 2013-02-06 07:51:29 PST
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?
Comment 13 Caio Marcelo de Oliveira Filho 2013-02-06 10:35:52 PST
Created attachment 186876 [details]
Patch
Comment 14 Caio Marcelo de Oliveira Filho 2013-02-06 10:44:14 PST
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 15 Kenneth Rohde Christiansen 2013-02-06 10:45:06 PST
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?
Comment 16 Kenneth Rohde Christiansen 2013-02-06 10:45:52 PST
Simon Fraser, could you review this? thanks
Comment 17 Rafael Brandao 2013-02-06 10:50:46 PST
(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 18 Simon Fraser (smfr) 2013-02-06 10:52:42 PST
Comment on attachment 186876 [details]
Patch

I'm not a CoordinatedGraphics expert but this seems fine.
Comment 19 Rafael Brandao 2013-02-06 10:53:21 PST
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. :-)
Comment 20 Kenneth Rohde Christiansen 2013-02-06 11:00:05 PST
(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 21 Simon Fraser (smfr) 2013-02-06 11:03:40 PST
Comment on attachment 186876 [details]
Patch

r=me on the webkit2 changes.
Comment 22 Noam Rosenthal 2013-02-06 12:51:48 PST
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
Comment 23 Allan Sandfeld Jensen 2013-02-07 02:44:57 PST
(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.
Comment 24 Jocelyn Turcotte 2013-02-07 02:56:15 PST
(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
Comment 25 Noam Rosenthal 2013-02-07 03:49:27 PST
(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.
Comment 26 Caio Marcelo de Oliveira Filho 2013-02-07 06:26:57 PST
Created attachment 187087 [details]
Patch
Comment 27 Caio Marcelo de Oliveira Filho 2013-02-07 06:51:06 PST
Committed r142112: <http://trac.webkit.org/changeset/142112>
Comment 28 Julien Brianceau 2013-02-07 07:37:57 PST
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)
Comment 29 Simon Fraser (smfr) 2013-02-07 08:19:03 PST
(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?
Comment 30 Noam Rosenthal 2013-02-07 08:23:46 PST
> I'd prefer scrolling/coordinatedgraphics. We don't use the "gfx" abomination anywhere else, do we?
That's what we ended up using.
Comment 31 Gavin Peters 2013-02-07 09:33:47 PST
Reverted r142112 for reason:

New scrolling test crashes on Lion.

Committed r142141: <http://trac.webkit.org/changeset/142141>
Comment 33 Allan Sandfeld Jensen 2013-02-07 09:44:13 PST
(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.
Comment 34 Gavin Peters 2013-02-07 09:45:14 PST
If you're certain of that, I'm happy to reland it, and create a bug in Chromium for fixing the test. Thanks.

Thanks.
Comment 35 Noam Rosenthal 2013-02-07 09:47:00 PST
(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.
Comment 36 Gavin Peters 2013-02-07 09:53:29 PST
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>
Comment 37 Noam Rosenthal 2013-02-07 09:54:44 PST
(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.
Comment 38 Gavin Peters 2013-02-07 09:56:02 PST
I'm going to file the Chromium bug.

Thanks for the test case.  :D
Comment 39 Chris Dumez 2013-02-11 07:06:57 PST
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