Bug 104990

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.