Coordinated Graphics: Separate LayerTreeCoordinator into LayerTreeCoordinator and CompositingCoordinator

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Gwang Yoon Hwang]

The LayerTreeCoordinator has too many responsibilities. It implements
LayerTreeHost, GraphicsLayerClient, CoordinatedGraphicsLayerClient,
CoordinatedImageBacking::Coordinator, UpdateAtlasClient,
GraphicsLayerFactory and WebCustomFilterProgramProxyClient.

This refactoring reduces the responsibilities of LayerTreeCoordinator.

By creates a new class called CompositingCoordinator, which takes the 
responsibility of scheduling compositing task in WebProcess,
LayerTreeCoordinator acts like simple LayerTreeHost.

By extracting interfaces dealing with LayerTreeCoordinatorProxy to
CompositingCoordinatorMessageClient, CompositingCoordinator is decoupled
from IPC handling.

WebCompositingCoordinatorMessageClient implements IPC handling for 
CompositingCoordinator and LayerTreeCoordinator.
WebCustomFilterProgramProxyClient is stands for IPC handling, so
WebCompositingCoordiantorMessageClient implements this interface, too.

[Gwang Yoon Hwang]

Created attachment 178194 [details]
Patch

[Dongseong Hwang]

Comment on attachment 178194 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178194&action=review
great work! some nits.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinatorClient.h:84
> +class CompositingCoordinatorClient {

CompositingCoordinator already has a Page instance. I think CompositingCoordinator can do some works by itself.
Perhaps this client is not necessary.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:57
> +    m_compositingCoordinator = CompositingCoordinator::create(m_webPage->corePage(), this, m_compositingCoordinatorMessageClient.get());

I think LayerTreeCoordinator should delegate ownership of WebCompositingCoordinatorMessageClient to CompositingCoordinator.
LayerTreeCoordinator sometimes uses m_compositingCoordinatorMessageClient directly, but it means WebCompositingCoordinatorMessageClient plays a role of LayerTreeCoordinator.
I think you want to get together all communication mechanism of CompositingCoordinator into WebCompositingCoordinatorMessageClient, not them of LayerTreeCoordinator.

I mean LayerTreeCoordinator should communicate its own message by itself, not via WebCompositingCoordinatorMessageClient, and LayerTreeCoordinator does not need to own WebCompositingCoordinatorMessageClient after passing it to CompositingCoordinator.

[Dongseong Hwang]

Comment on attachment 178194 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178194&action=review

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/WebCompositingCoordinatorMessageClient.cpp:70
> +    m_webPage->send(Messages::LayerTreeCoordinatorProxy::SetRootCompositingLayer(id));

WebCompositingCoordinatorMessageClient sends messages to LayerTreeCoordinatorProxy, not WebCompositingCoordinatorMessageClientProxy(?).
And LayerTreeCoordinatorProxy sends messages to LayerTreeCoordinator and LayerTreeCoordinator redirects to CompositingCoordinator.
It means refactoring is not complete.
Do you make it in next bugs?

[Gwang Yoon Hwang]

(In reply to comment #2)
> (From update of attachment 178194 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178194&action=review
> great work! some nits.
> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinatorClient.h:84
> > +class CompositingCoordinatorClient {
> 
> CompositingCoordinator already has a Page instance. I think CompositingCoordinator can do some works by itself.
> Perhaps this client is not necessary.

Page is not enough to remove CompositingCoordinatorClient.

For example : CompositingCoordinatorClient::layoutIfNeeded.

In the WebKit1 case, page->mainFrame()->view()->updateLayoutAndStyleIfNeededRecursive() is sufficient.
But in the WebKit2 case, we need to consider m_underlayPage in WebKit::WebPage.
That's why I delegate this to Client.

Other methods in this Client has dependencies to WebKit::WebPage, not only WebCore::Page.

> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:57
> > +    m_compositingCoordinator = CompositingCoordinator::create(m_webPage->corePage(), this, m_compositingCoordinatorMessageClient.get());
> 
> I think LayerTreeCoordinator should delegate ownership of WebCompositingCoordinatorMessageClient to CompositingCoordinator.
> LayerTreeCoordinator sometimes uses m_compositingCoordinatorMessageClient directly, but it means WebCompositingCoordinatorMessageClient plays a role of LayerTreeCoordinator.
> I think you want to get together all communication mechanism of CompositingCoordinator into WebCompositingCoordinatorMessageClient, not them of LayerTreeCoordinator.
> 
> I mean LayerTreeCoordinator should communicate its own message by itself, not via WebCompositingCoordinatorMessageClient, and LayerTreeCoordinator does not need to own WebCompositingCoordinatorMessageClient after passing it to CompositingCoordinator.

CompositingCoordinatorMessageClient is made for interface to send messages to renderer.

And WebCompositingCoordinatorMessageClient implements interface from above, and extra messages from LayerTreeCoordinator. WebCompositingCoordinatorMessageClient has a duty to send IPC message, and manage data types just for IPC communication. So I think it is natural.

[Gwang Yoon Hwang]

(In reply to comment #3)
> (From update of attachment 178194 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178194&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/WebCompositingCoordinatorMessageClient.cpp:70
> > +    m_webPage->send(Messages::LayerTreeCoordinatorProxy::SetRootCompositingLayer(id));
> 
> WebCompositingCoordinatorMessageClient sends messages to LayerTreeCoordinatorProxy, not WebCompositingCoordinatorMessageClientProxy(?).
> And LayerTreeCoordinatorProxy sends messages to LayerTreeCoordinator and LayerTreeCoordinator redirects to CompositingCoordinator.
> It means refactoring is not complete.
> Do you make it in next bugs?

Relationships between LayerTreeCoordinator <-> LayerTreeCoordinatorProxy is maintained. LayerTreeCoordinator just uses CompositingCooridnator and its clients to manage resources and send messages. WebCompositingCoordinatorMessageClient is not replacement of LayerTreeCoordinator.

[Gwang Yoon Hwang]

Created attachment 178339 [details]
Patch

rebase after r136976

[Gwang Yoon Hwang]

Created attachment 178512 [details]
Patch

Fix typos, rebase after r137128

[Noam Rosenthal]

Comment on attachment 178512 [details]
Patch

This is quite messy... 
I'd rather if we divided to 2 classes:
1. common with threaded
2. WebKit2 only. This would include the non-composited layer, messaging, WebPage access etc.

[Gwang Yoon Hwang]

Created attachment 178863 [details]
Rebase after r137293

To remove ambiguity, rename CompositingCoordinatorMessageClient to CompositingCoordinatorCommands

[Gwang Yoon Hwang]

After a discussion in IRC, I update the patch.

CoordinatedLayerTreeHost: It has a responsibility to mediate between LayerTreeHost and CompositingCoordinator. It uses CompositingCoordiator to fulfill LayerTreeHost's requirements, and manage states dedicated on WebKit2 (forceRepaint and notifyAfterScheduledLayerFlush).
It also provides a way for CompositingCoordinator to use WebKit::WebPage via implementing CompositingCoordinatorClient, because there are unremoveable dependencies between CompositingCoordinator and WebKit::WebPage (eg. m_underlayPage in WebKit::WebPage). We can remove this dependencies afterwards.

CompositingCoordinatorCommands: This interface defines several methods used by CompositingCoordinator to send commands to renderer. Methods of this interface are gathered from CoordiantedLayerTreeHostProxyMessages.

WebCompositingCoordinatorCommands: It is a concrete implemention of CompositingCoordinatorCommands. WebCompositingCoordinatorCommands manages the lifecycle of IPC specific datatypes and sends IPC message.

1. I think it is natual to create WebCompositingCoordinatorCommands in this refactoring

2. By introducing WebCompositingCoordinatorCommands, we can separate the IPC specific commands from the CoordinatedLayerTreeHost. Without WebCompositingCoordinatorCommands, 

3. It is ambiguous if we insert CompositingCoordinatorCommands::setRootCompositingLayer into CoordinatedLayerTreeHost.
Because of LayerTreeHost::setRootCompositingLayer, we need to rename CompositingCoordinatorCommands::setRootCompositingLayer. But methods of CompositingCommands are matched with CoordinatedLayerTreeHostProxyMessages, it is a little bit hard to change.

[Gwang Yoon Hwang]

(In reply to comment #8)
> (From update of attachment 178512 [details])
> This is quite messy... 
> I'd rather if we divided to 2 classes:
> 1. common with threaded
> 2. WebKit2 only. This would include the non-composited layer, messaging, WebPage access etc.

It think WebCompositingCoordinatorCommands helps to make more understandable code. On CompositingCoordinator side of view, It uses CompositingCoordinatorClient to access the non-composited layer and WebPage. And to send commands to renderer, It uses CompositingCoordinatorCommands.

If we implement CompositingCoordinatorCommands inside of CoordinatedLayerTreeHost, the call sequence of LayerTreeHost can make several ping/pongs between CompositingCoordinator and CoordinatedLayerTreeHost in a single method call. I think it makes hard to read code.

[Dongseong Hwang]

I read IRC discussion with noamr and ryumiel. your opinions are very reasonable. I want to stack a bit idea on your opinions.

I prefer keeping three class: CoordinatedLayerTreeHost, CompositingCoordinator and WebCompositingCoordinatorOperations (extends CompositingCoordinatorOperations)

The role description is
CoordinatedLayerTreeHost : LayerTreeHost for Coordinated Graphics
CompositingCoordinator : Managing the lifecycle, root layer and etc. It will be reused in both WK1 and WK2.
CompositingCoordinatorOperations : Operations to communicate with LayerTreeRenderer.
WebCompositingCoordinatorOperations : IPC-based CompositingCoordinatorOperations implementaion.

CoordinatedLayerTreeHost can extend CompositingCoordinatorOperations, but it makes CoordinatedLayerTreeHost play many roles. I prefer that CoordinatedLayerTreeHost has a minimal role: LayerTreeHost.

If ryumiel choose this suggestion, ryumiel need to move some code from WebCompositingCoordinatorOperations to CoordinatedLayerTreeHost. CoordinatedLayerTreeHost should send messages to UI Process by itself. I think CoordinatedLayerTreeHost does not need to have a WebCompositingCoordinatorOperations member. After setting WebCompositingCoordinatorOperations to CompositingCoordinator, CoordinatedLayerTreeHost should not use WebCompositingCoordinatorOperations anymore.

[Noam Rosenthal]

(In reply to comment #12)
> I read IRC discussion with noamr and ryumiel. your opinions are very reasonable. I want to stack a bit idea on your opinions.
> 
> I prefer keeping three class: CoordinatedLayerTreeHost, CompositingCoordinator and WebCompositingCoordinatorOperations (extends CompositingCoordinatorOperations)
> 
> The role description is
> CoordinatedLayerTreeHost : LayerTreeHost for Coordinated Graphics
> CompositingCoordinator : Managing the lifecycle, root layer and etc. It will be reused in both WK1 and WK2.
> CompositingCoordinatorOperations : Operations to communicate with LayerTreeRenderer.
> WebCompositingCoordinatorOperations : IPC-based CompositingCoordinatorOperations implementaion.

My issue is mainly with CompositingCoordinatorOperations. I don't like that it's a bunch of virtual functions; instead it should be a list of operations, with a type and parameters, that we encode/decode in WebKit2.
So, CoordinatedLayerTreeHost and CompositingCoordinator are pretty clear,
but I would have CoordinatedLayerTreeHost deal with e.g. converting a CoordinatedSurface inside CompositingCoordinatorOperations and converting it to a handle, rather than have an additional WebCompositingCoordinatorOperations class for that.

[Dongseong Hwang]

(In reply to comment #13)
> (In reply to comment #12)
> > I read IRC discussion with noamr and ryumiel. your opinions are very reasonable. I want to stack a bit idea on your opinions.
> > 
> > I prefer keeping three class: CoordinatedLayerTreeHost, CompositingCoordinator and WebCompositingCoordinatorOperations (extends CompositingCoordinatorOperations)
> > 
> > The role description is
> > CoordinatedLayerTreeHost : LayerTreeHost for Coordinated Graphics
> > CompositingCoordinator : Managing the lifecycle, root layer and etc. It will be reused in both WK1 and WK2.
> > CompositingCoordinatorOperations : Operations to communicate with LayerTreeRenderer.
> > WebCompositingCoordinatorOperations : IPC-based CompositingCoordinatorOperations implementaion.
> 
> My issue is mainly with CompositingCoordinatorOperations. I don't like that it's a bunch of virtual functions; instead it should be a list of operations, with a type and parameters, that we encode/decode in WebKit2.
> So, CoordinatedLayerTreeHost and CompositingCoordinator are pretty clear,
> but I would have CoordinatedLayerTreeHost deal with e.g. converting a CoordinatedSurface inside CompositingCoordinatorOperations and converting it to a handle, rather than have an additional WebCompositingCoordinatorOperations class for that.

do you mean that you want template polymorphism instead of class polymorphism. When it comes to how to implement, I does not have concrete idea yet.

However, if we find a solution, we cannot use template polymorphism, because we need to switch WK1's CompositingCoordinatorOperations and WK2's CompositingCoordinatorOperations in runtime, not compile time.
It is because we must compile both implementation together: WK1 will use WebCore's CompositingCoordinatorOperations while WK2 will use WebCompositingCoordinatorOperations.
I think virtual causes some overhead, but I cannot find more feasible solution.

[Noam Rosenthal]

Comment on attachment 178863 [details]
Rebase after r137293

View in context: https://bugs.webkit.org/attachment.cgi?id=178863&action=review

This change takes the code in a direction I don't agree with.
r- until we reach a concensus.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinatorCommands.h:82
> +class CompositingCoordinatorCommands {
> +public:
> +    virtual void setRootCompositingLayer(CoordinatedLayerID) = 0;
> +    virtual void setCompositingLayerState(CoordinatedLayerID, const CoordinatedLayerInfo&) = 0;
> +    virtual void setCompositingLayerChildren(CoordinatedLayerID, const Vector<CoordinatedLayerID>&) = 0;
> +#if ENABLE(CSS_FILTERS)
> +    virtual void setCompositingLayerFilters(CoordinatedLayerID, const WebCore::FilterOperations&) = 0;
> +#endif
> +    virtual void deleteCompositingLayer(CoordinatedLayerID) = 0;
> +
> +    virtual void createTileForLayer(CoordinatedLayerID, uint32_t tileID, const WebCore::IntRect&, const SurfaceUpdateInfo&) = 0;
> +    virtual void updateTileForLayer(CoordinatedLayerID, uint32_t tileID, const WebCore::IntRect&, const SurfaceUpdateInfo&) = 0;
> +    virtual void removeTileForLayer(CoordinatedLayerID, uint32_t tileID) = 0;
> +
> +    virtual void createUpdateAtlas(uint32_t atlasID, const WebCoordinatedSurface::Handle&) = 0;
> +    virtual void removeUpdateAtlas(uint32_t atlasID) = 0;
> +
> +    virtual void createImageBacking(CoordinatedImageBackingID) = 0;
> +    virtual void updateImageBacking(CoordinatedImageBackingID, const WebCoordinatedSurface::Handle&) = 0;
> +    virtual void clearImageBackingContents(CoordinatedImageBackingID) = 0;
> +    virtual void removeImageBacking(CoordinatedImageBackingID) = 0;
> +
> +    virtual void didRenderFrame(const WebCore::IntSize& contentsSize, const WebCore::IntRect& coveredRect) = 0;
> +    virtual void didChangeScrollPosition(const WebCore::FloatPoint& position) = 0;
> +
> +#if USE(GRAPHICS_SURFACE)
> +    virtual void createCanvas(CoordinatedLayerID, const WebCore::IntSize&, const WebCore::GraphicsSurfaceToken&) = 0;
> +    virtual void syncCanvas(CoordinatedLayerID, uint32_t frontBuffer) = 0;
> +    virtual void destroyCanvas(CoordinatedLayerID) = 0;
> +#endif
> +
> +    virtual void setLayerAnimations(CoordinatedLayerID, const WebCore::GraphicsLayerAnimations&) = 0;
> +    virtual void setAnimationsLocked(bool) = 0;
> +};

I'd rather have these as operations; e.g.
CompositingOperations::RemoveTileForLayer::create(CoordinatedLayerID, uint32_t)
with a deepCopy function for threading.

and then 
WebCompositingOperations::encode/decode

[Gwang Yoon Hwang]

Created attachment 182305 [details]
Patch

[Gwang Yoon Hwang]

(In reply to comment #15)
> I'd rather have these as operations; e.g.
> CompositingOperations::RemoveTileForLayer::create(CoordinatedLayerID, uint32_t)
> with a deepCopy function for threading.
> 
> and then 
> WebCompositingOperations::encode/decode

I modified CoordinatedLayerTreeHost to inherit CompositingCoordinator and made some changes for namings.

But I could not apply your suggestion about CompositingOperations. Could you elaborate your idea on #15?

Thanks in advance.

[Noam Rosenthal]

Comment on attachment 182305 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182305&action=review

This is not really what I meant...
If you don't mind, I can take charge of this patch as it's quite a major refactor that I feel I need to be more involved in.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:234
> +void CoordinatedLayerTreeHost::purgeBackingStores()
> +{
> +    CompositingCoordinator::purgeBackingStores();

Spaghetti code.

[Dongseong Hwang]

(In reply to comment #18)
> (From update of attachment 182305 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182305&action=review
> 
> This is not really what I meant...
> If you don't mind, I can take charge of this patch as it's quite a major refactor that I feel I need to be more involved in.

I posted prototype actor model of TextureMapperLayer in Bug 103854.
I wish the patch would help you make complete actor model! feel free to use the patch :)

[Gwang Yoon Hwang]

After #108294 and #111919, we almost complete to make a actor-model, I'll update this bug to match with current design.

[Gwang Yoon Hwang]

Created attachment 192909 [details]
Patch

[Gwang Yoon Hwang]

Now we can have unified messages after this bug, https://bugs.webkit.org/show_bug.cgi?id=111919, I think we can go forward with this bug.

So, I updated CompositingCoordinator / CoordinatedLayerTreeHost pair.

Changes are:

1. Now CompositingCoordinator manages the "resources" for CoordinatedGraphics. Such as layers, updateAtlases, and coordinated surfaces.

2. CoordinatedLayerTreeHost, which inherits CompositingCoordinator, implements scheduling.


I think it becomes much cleaner, but I need your opinion.

[Gwang Yoon Hwang]

Created attachment 193083 [details]
Patch

[Gwang Yoon Hwang]

After discussion in IRC, I creates CompositingCoordinator::Client and modified CoordinatedLayerTree to inherit Client instead of CompositingCoordinator, to avoid multiple inheritance.

[Gwang Yoon Hwang]

Created attachment 203804 [details]
Patch

[Gwang Yoon Hwang]

Created attachment 203920 [details]
Patch

[Noam Rosenthal]

Comment on attachment 203920 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203920&action=review

> Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp:100
> +    m_client->flushCompositingStatePeripheralLayers();

This should be called something like didFlushRootLayer(); The peripheral layers stuff should be known only to CoordinatedLTH.

> Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp:176
> +    m_client->prepareCustomFilterProxiesIfNeeded(state);

This should be called didSyncLayerState. The client can decide whether this is about custom filters or something else.

> Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.h:70
> +    class Client {
> +    public:
> +        virtual void flushCompositingStatePeripheralLayers() = 0;
> +        virtual void scheduleLayerFlushRequired() = 0;
> +        virtual void commitCoordinatedGraphicsState(CoordinatedGraphicsState&) = 0;
> +        virtual void paintContents(const GraphicsLayer*, GraphicsContext&, const IntRect& clipRect) = 0;
> +#if ENABLE(CSS_SHADERS)
> +        virtual void prepareCustomFilterProxiesIfNeeded(CoordinatedGraphicsLayerState&) = 0;
> +#endif
> +    };

This client is too specific.
The CompositingCoordinator::Client class should respond to events that compositing coordinator knows about, otherwise it looks like an artificial separation of classes...

[Gwang Yoon Hwang]

Created attachment 204012 [details]
Patch

[Gwang Yoon Hwang]

(In reply to comment #27)
> (From update of attachment 203920 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203920&action=review
> 
> > Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp:100
> > +    m_client->flushCompositingStatePeripheralLayers();
> 
> This should be called something like didFlushRootLayer(); The peripheral layers stuff should be known only to CoordinatedLTH.
> 
> > Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp:176
> > +    m_client->prepareCustomFilterProxiesIfNeeded(state);
> 
> This should be called didSyncLayerState. The client can decide whether this is about custom filters or something else.
> 

Yes, I love the way you suggests.

> 
> This client is too specific.
> The CompositingCoordinator::Client class should respond to events that compositing coordinator knows about, otherwise it looks like an artificial separation of classes...

 I made didFlushRootLayer and didSyncLayerState, and a Client(CoordinatedLTH) do what it need. 
I think that makes CompositingCoordinator::Client more generic.

[Noam Rosenthal]

Comment on attachment 204012 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204012&action=review

Sorry for the naming nitpicks... :)

> Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp:175
> +    m_client->didSyncLayerState(state);

Actually maybe this should be called willSyncLayerState. Otherwise it's a bit puzzling when it changes the state...

> Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.h:64
> +        virtual void didSyncLayerState(CoordinatedGraphicsLayerState&) = 0;

willSyncLayerState. It's a bit puzzling to have "did" that modifies something...

> Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.h:65
> +        virtual void scheduleLayerFlushRequired() = 0;

notifyFlushRequired()

> Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.h:66
> +        virtual void commitCoordinatedGraphicsState(CoordinatedGraphicsState&) = 0;

commitSceneState()

> Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.h:67
> +        virtual void paintContents(const GraphicsLayer*, GraphicsContext&, const IntRect& clipRect) = 0;

paintLayerContents

[Gwang Yoon Hwang]

Created attachment 204024 [details]
Patch

[Gwang Yoon Hwang]

(In reply to comment #30)
> (From update of attachment 204012 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204012&action=review
> 
> Sorry for the naming nitpicks... :)
> 
> > Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp:175
> > +    m_client->didSyncLayerState(state);
> 
> Actually maybe this should be called willSyncLayerState. Otherwise it's a bit puzzling when it changes the state...
> 
> > Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.h:64
> > +        virtual void didSyncLayerState(CoordinatedGraphicsLayerState&) = 0;
> 
> willSyncLayerState. It's a bit puzzling to have "did" that modifies something...

Yes, I agree. Fixed.

> 
> > Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.h:65
> > +        virtual void scheduleLayerFlushRequired() = 0;
> 
> notifyFlushRequired()
>

Ditto.
 
> > Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.h:66
> > +        virtual void commitCoordinatedGraphicsState(CoordinatedGraphicsState&) = 0;
> 
> commitSceneState()
>

Ditto. 
> > Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.h:67
> > +        virtual void paintContents(const GraphicsLayer*, GraphicsContext&, const IntRect& clipRect) = 0;
> 
> paintLayerContents
Ditto.

[Early Warning System Bot]

Comment on attachment 204024 [details]
Patch

Attachment 204024 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/715558

[Gwang Yoon Hwang]

Created attachment 204029 [details]
Fixed typo

[Noam Rosenthal]

I'm ok with this, but I would also like Jocelyn to take a look.
Furthermore, it requires a WK2 owner review because of the change to LayerTreeHost.h (though the change is rather trivial).

[Jocelyn Turcotte]

Comment on attachment 204029 [details]
Fixed typo

View in context: https://bugs.webkit.org/attachment.cgi?id=204029&action=review

I can't see anything wrong with the patch, here are just a couple of picky comments.

> Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp:122
> +        m_client->commitSceneState(m_state);
> +
> +        clearPendingStateChanges();
> +        m_isWaitingForRenderer = true;

It feels to me that m_isWaitingForRenderer is part of the "async state management" responsibility and would fit better to CoordinatedLayerTreeHost, so I would keep it outside. But you probably know better what feature you'll need in CompositingCoordinator, so just use this as food for thoughts.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:83
> +    m_coordinator = CompositingCoordinator::create(webPage->corePage(), this);
>  
> -    CoordinatedSurface::setFactory(createCoordinatedSurface);
> +    m_coordinator->createRootLayer(m_webPage->size());

Please consistently use either webPage or m_webPage (webPage would be easier to follow here).

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:84
> +    m_layerTreeContext.coordinatedLayerID = toCoordinatedGraphicsLayer(rootLayer())->id();

Directly using m_coordinator->rootLayer() would give more context here.
Actually, unless I'm missing the point, I think that CoordinatedLayerTreeHost::rootLayer() is more harmful than helpful everywhere I see it used, I would avoid it.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:325
>  void CoordinatedLayerTreeHost::syncDisplayState()

syncDisplayState deals with WebCore stuff, essential steps like layouting and servicing scripted animations. I think it fits more into CompositingCoordinator's responsibility.
You'll need this kind of logic later on in WebKit1 anyway and it shouldn't be dupplicated if possible.

[Noam Rosenthal]

Comment on attachment 204029 [details]
Fixed typo

r- based on Jocelyn's comments.

[Gwang Yoon Hwang]

Created attachment 205472 [details]
Patch

[Gwang Yoon Hwang]

(In reply to comment #36)
> (From update of attachment 204029 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204029&action=review
> 
> I can't see anything wrong with the patch, here are just a couple of picky comments.
> 

Sorry for late.

> > Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp:122
> > +        m_client->commitSceneState(m_state);
> > +
> > +        clearPendingStateChanges();
> > +        m_isWaitingForRenderer = true;
> 
> It feels to me that m_isWaitingForRenderer is part of the "async state management" responsibility and would fit better to CoordinatedLayerTreeHost, so I would keep it outside. But you probably know better what feature you'll need in CompositingCoordinator, so just use this as food for thoughts.
> 
I agreed your comment about m_isWaitingForRenderer. By moving it to CoordinatedLayerTreeHost, I could remove some redundant codes.


> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:83
> > +    m_coordinator = CompositingCoordinator::create(webPage->corePage(), this);
> >  
> > -    CoordinatedSurface::setFactory(createCoordinatedSurface);
> > +    m_coordinator->createRootLayer(m_webPage->size());
> 
> Please consistently use either webPage or m_webPage (webPage would be easier to follow here).
> 
Fixed. :)

> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:84
> > +    m_layerTreeContext.coordinatedLayerID = toCoordinatedGraphicsLayer(rootLayer())->id();
> 
> Directly using m_coordinator->rootLayer() would give more context here.
> Actually, unless I'm missing the point, I think that CoordinatedLayerTreeHost::rootLayer() is more harmful than helpful everywhere I see it used, I would avoid it.
> 
Modified to use m_coordinator->rootLayer().

> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:325
> >  void CoordinatedLayerTreeHost::syncDisplayState()
> 
> syncDisplayState deals with WebCore stuff, essential steps like layouting and servicing scripted animations. I think it fits more into CompositingCoordinator's responsibility.
> You'll need this kind of logic later on in WebKit1 anyway and it shouldn't be dupplicated if possible.

I placed syncDisplayState to CompositingCoordinator since it deals with WebKit2::WebPage::layoutIfNeeded. It relayouts not only main frame, but also main frame of underlayPage which is a specific to WebKit2.

If Qt, EFL, and Gtk do not cares about underlayPage now, this can be CompositingCoordinator. 

Anyway, I posted the patch that moves syncDisplayState to CompositingCoordinator, and It seems much cleaner. After this patch we can get another idea to handle underlayPage.

[Noam Rosenthal]

Comment on attachment 205472 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205472&action=review

> Source/WebCore/ChangeLog:21
> +        CoordinatedLayerTreeHost now takes a part for scheduling and IPC
> +        specific parts.

CoordinatedLayerTreeHost is responsible only for the scheduling and IPC-specific stuff, which are relevant only for WebKit2.

> Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp:196
> +

Remove line

> Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp:198
> +

Remove line

[Gwang Yoon Hwang]

Created attachment 205548 [details]
Patch

[Gwang Yoon Hwang]

(In reply to comment #40)
> (From update of attachment 205472 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205472&action=review
> 
> > Source/WebCore/ChangeLog:21
> > +        CoordinatedLayerTreeHost now takes a part for scheduling and IPC
> > +        specific parts.
> 
> CoordinatedLayerTreeHost is responsible only for the scheduling and IPC-specific stuff, which are relevant only for WebKit2.

Thanks for suggestion. Fixed.
> 
> > Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp:196
> > +
> 
> Remove line
> 
Removed

> > Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp:198
> > +
> 
> Remove line

Removed.


I think I've reset review flag by accidentally. ;(

[Anders Carlsson]

WK2 changes look good.

[WebKit Commit Bot]

Comment on attachment 205548 [details]
Patch

Clearing flags on attachment: 205548

Committed r152074: <http://trac.webkit.org/changeset/152074>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ádám Kallai]

This patch caused lots of crash on Qt Wk2 bots:

http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r152093%20(36414)/results.html

and API tests are failing with crash too:

http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release%20-%20Qt5.1%20-%20WebKit2/builds/238/steps/API%20tests/logs/stdio

Lots of layout tests became flaky on Qt 5.1 Wk2 bot after it:

http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release%20-%20Qt5.1%20-%20WebKit2/r152075%20(238)/results.html

[Ádám Kallai]

Could you check it, please?

[Gwang Yoon Hwang]

(In reply to comment #47)
> Could you check it, please?

Currently, I didn't bring my laptop ;(

I didn't check api test but layout test was passed in my pc.
 I don't have idea about it right now. 

I can start investgate about it 7 hours later.
If you want, plz revert it for now.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 118137

[Gwang Yoon Hwang]

Created attachment 205715 [details]
Patch

[Gwang Yoon Hwang]

(In reply to comment #50)
> Created an attachment (id=205715) [details]
> Patch

I've omitted synchronization check in CoordinatedLayerTreeHost::forceRepaint. That causes crashes in API test of WebKitQt.

[Noam Rosenthal]

Comment on attachment 205715 [details]
Patch

OK, hopefully this time the bots don't barf.

[WebKit Commit Bot]

Comment on attachment 205715 [details]
Patch

Clearing flags on attachment: 205715

Committed r152183: <http://trac.webkit.org/changeset/152183>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.