Currently, LayerTreeRenderer uses GraphicsLayerTextureMapper to use TextureMapperLayer, because TextureMapperLayer does not have actor model API. If refactoring TextureMapper to work in an actor model, Coodinated Graphics does not depend on GraphicsLayerTextureMapper, so we can change features more easily. After complete, LayerTreeRenderer and GraphicsLayerTextureMapper use TextureMapperLayer in the same way: actor model.
The following document explains what this meta bug will change. https://docs.google.com/document/pub?id=1u1Oif9lDmVt_MkJLxaEYbTlOuZWVJcKrM1vAxAvFrZk
Created attachment 182686 [details] Not for review : prototype actor model. post this patch to help noam's actor model implementation. This implementation changes only TextureMapperLayer API so Coordinated Graphics needs to manage similar message passing PODs. It means it is not completed patch, and noam will complete :) Note, 1. after actor model implementation, we don't need LayerTreeRenderer::m_pendingSyncBackingStores. 2. after actor model implementation, it is hard to use getters of TextureMapperLayer.
I think part of it you should already prepare, if you don't mind :) The part where we do all the syncing in GraphicsLayerTextureMapper instead of in TextureMapperLayer. If you want to prepare that and upstream I can do the other part where we use PODs instead of functional.
Comment on attachment 182686 [details] Not for review : prototype actor model. (In reply to comment #3) > I think part of it you should already prepare, if you don't mind :) > The part where we do all the syncing in GraphicsLayerTextureMapper instead of in TextureMapperLayer. If you want to prepare that and upstream I can do the other part where we use PODs instead of functional. I want the fastest way. I don't want you to wait me. Feel free to use this patch. If we can work in parallel, I can complete this patch if you want. Which is preferable? :) View in context: https://bugs.webkit.org/attachment.cgi?id=182686&action=review > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:490 > + btw, I'm not sure which granularity is optimal. Coarse granularity makes POD big and reduces the number of dispatching message. Fine granularity makes POD small and increases the number of dispatching message. We need to find sweet point. IMO CoordinatedGraphics::syncLayerState is a bit coarse. but below is a bit fine (e.g. ContentsRectChange, MasksToBoundsChange and etc.) Do you have good solution? Or will you correct the granularity if I land this patch with this problem? 491 if (m_changeMask & ContentsRectChange) 492 m_layer->appendChange(bind(&TextureMapperLayer::setContentsRect, m_layer.get(), contentsRect())); 493 494 if (m_changeMask & MasksToBoundsChange) 495 m_layer->appendChange(bind(&TextureMapperLayer::setMasksToBounds, m_layer.get(), masksToBounds())); 496 497 if (m_changeMask & DrawsContentChange) 498 m_layer->appendChange(bind(&TextureMapperLayer::setDrawsContent, m_layer.get(), drawsContent()));
(In reply to comment #3) > I think part of it you should already prepare, if you don't mind :) > The part where we do all the syncing in GraphicsLayerTextureMapper instead of in TextureMapperLayer. If you want to prepare that and upstream I can do the other part where we use PODs instead of functional. I'm working on completing this patch. If you have other opinion, let me know :)
Actually, I think you should go ahead full speed with your approach. Using POD types in the middle would just create unnecessary copies. No'am
(In reply to comment #6) > Actually, I think you should go ahead full speed with your approach. Using POD types in the middle would just create unnecessary copies. > No'am Yes, I run.
Created attachment 183420 [details] Not for review : prototype CoordinatedTask to reduce CoreIPC messages. This patch's purpose shows how to reduce CoreIPC messages. If we choose this mechanism instead of CoreIPC Message, when adding new api (e.g. background color), we don't need to change LayerTreeCoordinator and LayerTreeCoordinatorProxy. new Task will be passed via existing LayerTreeCoordinator::flushLayerChanges(). I apply CoordinatedTask to only SyncLayerState and SetLayerChildren for explanation.
Comment on attachment 183420 [details] Not for review : prototype CoordinatedTask to reduce CoreIPC messages. View in context: https://bugs.webkit.org/attachment.cgi?id=183420&action=review > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:835 > + encoder << static_cast<const CoordinatdSyncLayerStateTask*>(task)->m_layerID; spelling issue... Coordinat*E*d
Seems like the main classes of coordinated graphics (CoordinatedGraphicsLayer, CoordinatedBackingStore, LayerTreeRenderer, CoordinatedSurface, CoordinatedTile, UpdateAtlas, AreaAllocator) no longer depend on anything from WebKit2. Dongsung, Do you think it's time to move them to WebCore/platform/graphics/texmap? This would ease up reviews and would leave only IPC-related code in the WebKit2 directory.
(In reply to comment #10) > Seems like the main classes of coordinated graphics (CoordinatedGraphicsLayer, CoordinatedBackingStore, LayerTreeRenderer, CoordinatedSurface, CoordinatedTile, UpdateAtlas, AreaAllocator) no longer depend on anything from WebKit2. > Dongsung, Do you think it's time to move them to WebCore/platform/graphics/texmap? Yes, I think so. how about WebCore/platform/graphics/texmap/CoordinatedGraphics? > This would ease up reviews and would leave only IPC-related code in the WebKit2 directory. Ok, I'll create patch ASAP!
(In reply to comment #11) > (In reply to comment #10) > > Seems like the main classes of coordinated graphics (CoordinatedGraphicsLayer, CoordinatedBackingStore, LayerTreeRenderer, CoordinatedSurface, CoordinatedTile, UpdateAtlas, AreaAllocator) no longer depend on anything from WebKit2. > > Dongsung, Do you think it's time to move them to WebCore/platform/graphics/texmap? > Yes, I think so. > how about WebCore/platform/graphics/texmap/CoordinatedGraphics? platform/graphics/texmap/coordinated?
(In reply to comment #12) > platform/graphics/texmap/coordinated? Ok
Created attachment 184963 [details] WIP: Refactoring TextureMapper to work in an actor model.
Comment on attachment 184963 [details] WIP: Refactoring TextureMapper to work in an actor model. Not for review patch I submit this patch to listen feedback. I need to test more and I'll refactor this patch after moving WebCore refactoring.
Comment on attachment 184963 [details] WIP: Refactoring TextureMapper to work in an actor model. View in context: https://bugs.webkit.org/attachment.cgi?id=184963&action=review > Source/WebKit2/Scripts/webkit2/messages.py:401 > + 'WebKit::CoordinatedOperationHolder': ['"CoordinatedOperation.h"'], I'd prefer to have WebCoordinatedOperation, with its own decode/encode > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedOperation.h:87 > + virtual void execute(LayerTreeRenderer*) = 0; I don't think this is needed. I'd just have LayerTreeRenderer::handleCoordinatedGraphicsOperation(operation) with a switch statement. > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedOperation.h:88 > + virtual TaskType type() const = 0; OperationType getOperationType() > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedOperation.h:99 > +// To pass operation via CoreIPC. > +class CoordinatedOperationHolder { > +public: > + RefPtr<CoordinatedOperation> operation; > +}; I would wrap it with WebKit::WebCoordinatedOperation, and then we can move this to WebCore and don't need a "Holder". > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedOperation.h:263 > + uint32_t m_atlasID; > + RefPtr<CoordinatedSurface> m_surface; Don't use m_ for public members.
Seeing your solution, I think it's artificial to call enqueue and commit and maintain a queue in the UI process. I think your previous solution of sending a vector of operations and enqueuing them in the web process is more succinct. This way you also don't need a special "holder" class, all you need is a CoordinatedGraphicsOperations class that has a Vector<RefPtr<CoordinatedGraphicsOperation> > internally.
(In reply to comment #17) > This way you also don't need a special "holder" class, all you need is a CoordinatedGraphicsOperations class that has a Vector<RefPtr<CoordinatedGraphicsOperation> > internally. Yeah, I felt like this when implementing :)
Created attachment 185213 [details] WIP: Refactoring TextureMapper to work in an actor model.
Comment on attachment 185213 [details] WIP: Refactoring TextureMapper to work in an actor model. I posted the current implementation. It runs well. I'll rebase after completing Bug 108149. I need feedback :)
(In reply to comment #20) > (From update of attachment 185213 [details]) > I posted the current implementation. > It runs well. > I'll rebase after completing Bug 108149. > > I need feedback :) It seems CoordinatedOperations.h is missing from the patch.
btw CoordinatedOperation should maybe be CoordinatedGraphicsOperation.
Comment on attachment 185213 [details] WIP: Refactoring TextureMapper to work in an actor model. View in context: https://bugs.webkit.org/attachment.cgi?id=185213&action=review > Source/WebKit2/ChangeLog:29 > + (WebKit::CoordinatedSyncLayerStateOperation::create): Could you suggest this op name? CoordinatedGraphicsSyncLayerStateOperation is a bit long.
(In reply to comment #23) > (From update of attachment 185213 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=185213&action=review > > > Source/WebKit2/ChangeLog:29 > > + (WebKit::CoordinatedSyncLayerStateOperation::create): > > Could you suggest this op name? > CoordinatedGraphicsSyncLayerStateOperation is a bit long. I think it's ok to have them as inner classes, like CoordinatedGraphicsOperation::SetLayerAnimations
Comment on attachment 185213 [details] WIP: Refactoring TextureMapper to work in an actor model. View in context: https://bugs.webkit.org/attachment.cgi?id=185213&action=review > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:460 > m_animationsLocked = true; > - m_webPage->send(Messages::CoordinatedLayerTreeHostProxy::SetAnimationsLocked(true)); > + enqueueCoordinatedOperation(CoordinatedSetAnimationsLockedOperation::create(true)); This doesn't make sense. lockAnimations is not a coordinated graphics operation - it should happen immediately if at all. Though I think we should just remove this feature, it's not really tested and I'm not sure if it helps anything.
Created attachment 186013 [details] Patch