Currently, Web Process does not send a layer creation message to UI Process, so LayerTreeHost creates GraphicsLayer lazily. It causes redundant if statements to check if GraphicsLayer is created. This patch manages the lifecycle of layer explicitly, and it increases readability. This is in preparation for refactoring TextureMapper to work in an actor model (http://webkit.org/b/103854).
Created attachment 179412 [details] Patch
ping. Could you review this bug?
Comment on attachment 179412 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179412&action=review > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:353 > + OwnPtr<WebCore::GraphicsLayer> newLayer = GraphicsLayer::create(0, this); What does 0 stand for? > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:592 > + m_rootLayer = GraphicsLayer::create(0, this); What does 0 stand for? > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.h:179 > + Vector<CoordinatedLayerID> m_attachedLayers; > Vector<CoordinatedLayerID> m_detachedLayers; Let's rename to m_layersToCreate and m_layersToDelete
Comment on attachment 179412 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179412&action=review >> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:592 >> + m_rootLayer = GraphicsLayer::create(0, this); > > What does 0 stand for? 0 stands for absence of factory. in GraphicsLayerTextureMapper. PassOwnPtr<GraphicsLayer> GraphicsLayer::create(GraphicsLayerFactory* factory, GraphicsLayerClient* client) { if (!factory) return adoptPtr(new GraphicsLayerTextureMapper(client)); return factory->createGraphicsLayer(client); } I agree that it is hard to read. I'll change adoptPtr(new GraphicsLayerTextureMapper(this)) or GraphicsLayer::create(this) if possible (I need to investigate what is s_graphicsLayerFactory) PassOwnPtr<GraphicsLayer> GraphicsLayer::create(GraphicsLayerClient* client) { if (s_graphicsLayerFactory) return (*s_graphicsLayerFactory)(client); return adoptPtr(new GraphicsLayerTextureMapper(client)); } >> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.h:179 >> Vector<CoordinatedLayerID> m_detachedLayers; > > Let's rename to m_layersToCreate and m_layersToDelete Ok, I'll change!
(In reply to comment #4) > (From update of attachment 179412 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=179412&action=review > > >> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:592 > >> + m_rootLayer = GraphicsLayer::create(0, this); > > > > What does 0 stand for? > > 0 stands for absence of factory. > > in GraphicsLayerTextureMapper. > PassOwnPtr<GraphicsLayer> GraphicsLayer::create(GraphicsLayerFactory* factory, GraphicsLayerClient* client) > { > if (!factory) > return adoptPtr(new GraphicsLayerTextureMapper(client)); > > return factory->createGraphicsLayer(client); > } > > I agree that it is hard to read. > > I'll change adoptPtr(new GraphicsLayerTextureMapper(this)) or GraphicsLayer::create(this) if possible (I need to investigate what is s_graphicsLayerFactory) > It's enough to put (0 /* factory */, ...)
Created attachment 179837 [details] Patch
This bug depends on Bug 103959 physically, not logically.
Created attachment 179858 [details] Patch
Comment on attachment 179858 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179858&action=review I am a little hesitant to add new IPC messages and overhead just for code readability. I would prefer it if we made sure setLayerInfo is always the first command called. > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:330 > +void CoordinatedLayerTreeHost::createCompositingLayers() > +{ > + if (m_layersToCreate.isEmpty()) > + return; > + > + for (size_t i = 0; i < m_layersToCreate.size(); ++i) > + m_webPage->send(Messages::CoordinatedLayerTreeHostProxy::CreateCompositingLayer(m_layersToCreate[i])); > + m_layersToCreate.clear(); > + m_shouldSyncFrame = true; > +} > + > +void CoordinatedLayerTreeHost::deleteCompositingLayers() > +{ > + if (m_layersToDelete.isEmpty()) > + return; > + > + if (m_isPurging) { > + m_layersToDelete.clear(); > + return; > + } > + > + for (size_t i = 0; i < m_layersToDelete.size(); ++i) > + m_webPage->send(Messages::CoordinatedLayerTreeHostProxy::DeleteCompositingLayer(m_layersToDelete[i])); > + m_layersToDelete.clear(); > + m_shouldSyncFrame = true; If a layer gets created and deleted in the same cycle, shouldn't we simply remove it from m_layersToCreate?
(In reply to comment #9) > (From update of attachment 179858 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=179858&action=review > > I am a little hesitant to add new IPC messages and overhead just for code readability. I would prefer it if we made sure setLayerInfo is always the first command called. Thank you for review. All are reasonable concerns :) I think creating Layer in setLayerInfo is one good solution. but there are a bit tricky problem. 1. It is weird that setLayerInfo is prior to setRootCompositingLayer. 2. when we change CoordinatedGraphicsLayer::flushCompositingStateForThisLayer, we always concern it. We already have a method prior to syncLayerState() although syncImageBacking() does not send any messages. I think it is a bit burden for future changes if we create layer by piggyback of setLayerInfo. void CoordinatedGraphicsLayer::flushCompositingStateForThisLayerOnly() { // Sets the values. computePixelAlignment(m_adjustedPosition, m_adjustedSize, m_adjustedAnchorPoint, m_pixelAlignmentOffset); syncImageBacking(); syncLayerState(); ... } In addition, IMHO createCompositingLayer message is so light that we should not make premature optimization. > If a layer gets created and deleted in the same cycle, shouldn't we simply remove it from m_layersToCreate? That's good idea, but there is some tricky issues, too. For example, following seq is possible in the same cycle. - createCompositingLayer - setLayerInfo - deleteCompositingLayer If we skip createCompositingLayer, LayerTreeRenderer can receive setLayerInfo without creating layer. I'm sure we can handle it but I don't want to add complex algorithm to optimize it.
(In reply to comment #10) > > If a layer gets created and deleted in the same cycle, shouldn't we simply remove it from m_layersToCreate? > > That's good idea, but there is some tricky issues, too. > > For example, following seq is possible in the same cycle. > - createCompositingLayer > - setLayerInfo > - deleteCompositingLayer > > If we skip createCompositingLayer, LayerTreeRenderer can receive setLayerInfo without creating layer. > I'm sure we can handle it but I don't want to add complex algorithm to optimize it. After rethinking about it, I think your suggestion is great. m_layersToDelete stores destructed layer so the layer cannet send any messages. If a layer gets created and deleted in the same cycle, we can simply remove it from m_layersToCreate.
Created attachment 180485 [details] Patch
Comment on attachment 180485 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180485&action=review > Source/WebKit2/ChangeLog:12 > + Currently, Web Process does not send a layer creation message to UI Process, so > + LayerTreeRenderer creates GraphicsLayer lazily. We need to deliberatively decide > + when we can create layer lazily, and it is hard to understand the lifecycle of > + GraphicsLayer. This patch manages the lifecycle of layer explicitly, and it > + increases readability. Send explicit commands to the UI process to create/delete compositing layers, instead of having the UI process decide lazily when to create them. Avoid creating a compositing layer at all if it was deleted in the same cycle. > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:321 > + layer->setReplicatedByLayer((layerInfo.replica != InvalidCoordinatedLayerID) ? layerByID(layerInfo.replica) : 0); > + layer->setMaskLayer((layerInfo.mask != InvalidCoordinatedLayerID) ? layerByID(layerInfo.mask) : 0); This is really verbose. I liked the old way better. > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.h:126 > + WebCore::GraphicsLayer* layerByID(CoordinatedLayerID id) > + { > + ASSERT(m_layers.contains(id)); > + ASSERT(id != InvalidCoordinatedLayerID); > + return m_layers.get(id); Why are you changing this function? It's not more readable.
Comment on attachment 180485 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180485&action=review >> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:321 >> + layer->setMaskLayer((layerInfo.mask != InvalidCoordinatedLayerID) ? layerByID(layerInfo.mask) : 0); > > This is really verbose. I liked the old way better. It is because I want to restrict the pre-condition of layerByID(). Please see below comment. >> Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.h:126 >> + return m_layers.get(id); > > Why are you changing this function? It's not more readable. I want to restrict the strict condition for layerByID(). What I mean that caller of layerByID(id) ensures id is not InvalidCoordinatedLayerID and m_layers contains id. If so, we can remove redundant two ASSERT in all sites. ASSERT(id); <- removable GraphicsLayer* layer = layerByID(id) ASSERT(layer); <- removable I think more strict, more clear.
Created attachment 180719 [details] Patch
(In reply to comment #13) > (From update of attachment 180485 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180485&action=review > > Send explicit commands to the UI process to create/delete compositing layers, instead of having the UI process decide lazily when to create them. > Avoid creating a compositing layer at all if it was deleted in the same cycle. Done. Thank you for rephrasing :) > > > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:321 > > + layer->setReplicatedByLayer((layerInfo.replica != InvalidCoordinatedLayerID) ? layerByID(layerInfo.replica) : 0); > > + layer->setMaskLayer((layerInfo.mask != InvalidCoordinatedLayerID) ? layerByID(layerInfo.mask) : 0); > > This is really verbose. I liked the old way better. I added layerByIDIfPossible() to preserve readability at this spot.
Comment on attachment 180719 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180719&action=review LGTM, but let's rename to getLayerByIDIfExists > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:351 > +GraphicsLayer* LayerTreeRenderer::layerByIDIfPossible(CoordinatedLayerID id) getLayerByIDIfExists
Created attachment 180720 [details] Patch
(In reply to comment #17) > (From update of attachment 180719 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180719&action=review > > LGTM, but let's rename to getLayerByIDIfExists Thx for review :) > > getLayerByIDIfExists Done.
Comment on attachment 180720 [details] Patch Clearing flags on attachment: 180720 Committed r138468: <http://trac.webkit.org/changeset/138468>
All reviewed patches have been landed. Closing bug.