Summary: | Coordinated Graphics: Separate LayerTreeCoordinator into LayerTreeCoordinator and CompositingCoordinator | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gwang Yoon Hwang <yoon> | ||||||||||||||||||||||||||||||||
Component: | Layout and Rendering | Assignee: | 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
Gwang Yoon Hwang
2012-12-07 03:19:16 PST
Created attachment 178194 [details]
Patch
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 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? (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. (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. Created attachment 178339 [details] Patch rebase after r136976 Created attachment 178512 [details] Patch Fix typos, rebase after r137128 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.
Created attachment 178863 [details] Rebase after r137293 To remove ambiguity, rename CompositingCoordinatorMessageClient to CompositingCoordinatorCommands 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. (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. 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. (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. (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 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 Created attachment 182305 [details]
Patch
(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 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. (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 :) After #108294 and #111919, we almost complete to make a actor-model, I'll update this bug to match with current design. Created attachment 192909 [details]
Patch
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. Created attachment 193083 [details]
Patch
After discussion in IRC, I creates CompositingCoordinator::Client and modified CoordinatedLayerTree to inherit Client instead of CompositingCoordinator, to avoid multiple inheritance. Created attachment 203804 [details]
Patch
Created attachment 203920 [details]
Patch
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... Created attachment 204012 [details]
Patch
(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 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 Created attachment 204024 [details]
Patch
(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 on attachment 204024 [details] Patch Attachment 204024 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/715558 Created attachment 204029 [details]
Fixed typo
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 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 on attachment 204029 [details]
Fixed typo
r- based on Jocelyn's comments.
Created attachment 205472 [details]
Patch
(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 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 Created attachment 205548 [details]
Patch
(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. ;( WK2 changes look good. Comment on attachment 205548 [details] Patch Clearing flags on attachment: 205548 Committed r152074: <http://trac.webkit.org/changeset/152074> All reviewed patches have been landed. Closing bug. 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 Could you check it, please? (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. Re-opened since this is blocked by bug 118137 Created attachment 205715 [details]
Patch
(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 on attachment 205715 [details]
Patch
OK, hopefully this time the bots don't barf.
Comment on attachment 205715 [details] Patch Clearing flags on attachment: 205715 Committed r152183: <http://trac.webkit.org/changeset/152183> All reviewed patches have been landed. Closing bug. |