Bug 104360

Summary: Coordinated Graphics: Separate LayerTreeCoordinator into LayerTreeCoordinator and CompositingCoordinator
Product: WebKit Reporter: Gwang Yoon Hwang <yoon>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, andersca, cmarcelo, commit-queue, d.nomiyama, dongseong.hwang, gyuyoung.kim, gyuyoung.kim, jaepark, jturcotte, kadam, kalyan.kondapally, luiz, menard, noam, rakuco, skyul, webkit-ews, webkit.review.bot, zeno
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 111919, 118137    
Bug Blocks: 102994    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Rebase after r137293
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
webkit-ews: commit-queue-
Fixed typo
none
Patch
none
Patch
none
Patch none

Description Gwang Yoon Hwang 2012-12-07 03:19:16 PST
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.
Comment 1 Gwang Yoon Hwang 2012-12-07 03:59:42 PST
Created attachment 178194 [details]
Patch
Comment 2 Dongseong Hwang 2012-12-07 20:08:19 PST
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.
Comment 3 Dongseong Hwang 2012-12-07 20:12:30 PST
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?
Comment 4 Gwang Yoon Hwang 2012-12-07 21:37:22 PST
(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.
Comment 5 Gwang Yoon Hwang 2012-12-07 21:46:53 PST
(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.
Comment 6 Gwang Yoon Hwang 2012-12-07 22:57:41 PST
Created attachment 178339 [details]
Patch

rebase after r136976
Comment 7 Gwang Yoon Hwang 2012-12-10 04:10:12 PST
Created attachment 178512 [details]
Patch

Fix typos, rebase after r137128
Comment 8 Noam Rosenthal 2012-12-11 13:23:21 PST
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.
Comment 9 Gwang Yoon Hwang 2012-12-11 13:29:07 PST
Created attachment 178863 [details]
Rebase after r137293

To remove ambiguity, rename CompositingCoordinatorMessageClient to CompositingCoordinatorCommands
Comment 10 Gwang Yoon Hwang 2012-12-11 13:34:45 PST
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.
Comment 11 Gwang Yoon Hwang 2012-12-11 13:48:57 PST
(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.
Comment 12 Dongseong Hwang 2012-12-11 16:52:02 PST
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.
Comment 13 Noam Rosenthal 2012-12-11 22:23:30 PST
(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.
Comment 14 Dongseong Hwang 2012-12-11 23:24:54 PST
(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.
Comment 15 Noam Rosenthal 2012-12-13 07:25:36 PST
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
Comment 16 Gwang Yoon Hwang 2013-01-11 02:44:55 PST
Created attachment 182305 [details]
Patch
Comment 17 Gwang Yoon Hwang 2013-01-11 02:50:16 PST
(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.
Comment 18 Noam Rosenthal 2013-01-13 07:45:32 PST
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.
Comment 19 Dongseong Hwang 2013-01-14 19:59:52 PST
(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 :)
Comment 20 Gwang Yoon Hwang 2013-03-09 00:56:46 PST
After #108294 and #111919, we almost complete to make a actor-model, I'll update this bug to match with current design.
Comment 21 Gwang Yoon Hwang 2013-03-13 06:01:04 PDT
Created attachment 192909 [details]
Patch
Comment 22 Gwang Yoon Hwang 2013-03-13 06:20:13 PDT
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.
Comment 23 Gwang Yoon Hwang 2013-03-14 01:01:32 PDT
Created attachment 193083 [details]
Patch
Comment 24 Gwang Yoon Hwang 2013-03-14 01:07:57 PDT
After discussion in IRC, I creates CompositingCoordinator::Client and modified CoordinatedLayerTree to inherit Client instead of CompositingCoordinator, to avoid multiple inheritance.
Comment 25 Gwang Yoon Hwang 2013-06-05 04:41:13 PDT
Created attachment 203804 [details]
Patch
Comment 26 Gwang Yoon Hwang 2013-06-06 03:51:48 PDT
Created attachment 203920 [details]
Patch
Comment 27 Noam Rosenthal 2013-06-06 07:04:54 PDT
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...
Comment 28 Gwang Yoon Hwang 2013-06-07 01:01:18 PDT
Created attachment 204012 [details]
Patch
Comment 29 Gwang Yoon Hwang 2013-06-07 01:07:29 PDT
(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.
Comment 30 Noam Rosenthal 2013-06-07 02:46:13 PDT
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
Comment 31 Gwang Yoon Hwang 2013-06-07 04:59:31 PDT
Created attachment 204024 [details]
Patch
Comment 32 Gwang Yoon Hwang 2013-06-07 05:02:21 PDT
(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.
Comment 33 Early Warning System Bot 2013-06-07 05:11:30 PDT
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
Comment 34 Gwang Yoon Hwang 2013-06-07 05:23:47 PDT
Created attachment 204029 [details]
Fixed typo
Comment 35 Noam Rosenthal 2013-06-07 05:34:49 PDT
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).
Comment 36 Jocelyn Turcotte 2013-06-10 03:35:03 PDT
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.
Comment 37 Noam Rosenthal 2013-06-25 00:29:48 PDT
Comment on attachment 204029 [details]
Fixed typo

r- based on Jocelyn's comments.
Comment 38 Gwang Yoon Hwang 2013-06-26 05:03:00 PDT
Created attachment 205472 [details]
Patch
Comment 39 Gwang Yoon Hwang 2013-06-26 05:14:21 PDT
(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.
Comment 40 Noam Rosenthal 2013-06-26 09:22:35 PDT
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
Comment 41 Gwang Yoon Hwang 2013-06-26 17:49:49 PDT
Created attachment 205548 [details]
Patch
Comment 42 Gwang Yoon Hwang 2013-06-26 17:54:06 PDT
(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. ;(
Comment 43 Anders Carlsson 2013-06-26 18:17:31 PDT
WK2 changes look good.
Comment 44 WebKit Commit Bot 2013-06-26 19:50:46 PDT
Comment on attachment 205548 [details]
Patch

Clearing flags on attachment: 205548

Committed r152074: <http://trac.webkit.org/changeset/152074>
Comment 45 WebKit Commit Bot 2013-06-26 19:50:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 47 Ádám Kallai 2013-06-27 07:07:46 PDT
Could you check it, please?
Comment 48 Gwang Yoon Hwang 2013-06-27 07:28:38 PDT
(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.
Comment 49 WebKit Commit Bot 2013-06-27 08:06:13 PDT
Re-opened since this is blocked by bug 118137
Comment 50 Gwang Yoon Hwang 2013-06-28 08:08:43 PDT
Created attachment 205715 [details]
Patch
Comment 51 Gwang Yoon Hwang 2013-06-28 08:11:05 PDT
(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.
Comment 52 Noam Rosenthal 2013-06-28 11:48:59 PDT
Comment on attachment 205715 [details]
Patch

OK, hopefully this time the bots don't barf.
Comment 53 WebKit Commit Bot 2013-06-28 12:10:29 PDT
Comment on attachment 205715 [details]
Patch

Clearing flags on attachment: 205715

Committed r152183: <http://trac.webkit.org/changeset/152183>
Comment 54 WebKit Commit Bot 2013-06-28 12:10:39 PDT
All reviewed patches have been landed.  Closing bug.