Created attachment 183130 [details] Patch

Created attachment 183153 [details] Patch

Created attachment 183330 [details] Patch

Created attachment 183335 [details] Patch

Please put it up for review when it applies :)

Created attachment 184659 [details] Patch

Created attachment 184704 [details] Patch

(In reply to comment #5) > Please put it up for review when it applies :) Now could you review? :)

Comment on attachment 184704 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184704&action=review I'm fine with this, needs a WK2 owner to review; they might want more explanations... > Source/WebKit2/ChangeLog:15 > + LayerTreeRenderer difficultly uses m_pendingSyncBackingStores to set a backing > + store to TextureMapperLayer, although LayerTreeRenderer easily sets a direct > + composited image to TextureMapperLayer. It is because LayerTreeRenderer directly > + set a backing store to TextureMapperLayer, instead of GraphicsLayerTextureMapper. > + > + This patch makes LayerTreeRenderer set a backing store to GraphicsLayerTextureMapper > + and then GraphicsLayerTextureMapper will set a backing store to TextureMapperLayer > + like other members. Allow me to propose an alternative wording for the explanation :) Instead of queuing the setting of backing stores in LayerTreeRenderer, and then setting them directly to TextureMapperLayer, we allow GraphicsLayerTextureMapper's existing queuing mechanism to handle that. Instead of a m_pendingSyncBackingStores queue, we have a m_backingStores queue which can be applied much more easily to the layer tree.

Created attachment 185117 [details] Patch

(In reply to comment #9) > (From update of attachment 184704 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184704&action=review > > I'm fine with this, needs a WK2 owner to review; they might want more explanations... Thank you for your favor as well as review! I'll ask WK2 owner. Now this bug depends on Bug 105787.

Created attachment 185169 [details] Patch

Comment on attachment 185169 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185169&action=review I am okay with this for WebKit2, with comments. Noam, your call for the r+. > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h:159 > - RefPtr<TextureMapperBackingStore> m_backingStore; > + TextureMapperBackingStore* m_backingStore; Just a comment: For this, you must have a strong guarantee that TextureMapperBackingStore always outlive TextureMapperLayer. Here the guarantee is m_releasedCoordinatedBackingStores. I think the ownership is too convoluted. > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:414 > + CoordinatedBackingStore* backingStore = 0; > + BackingStoreMap::iterator it = m_backingStores.find(graphicsLayer); > + if (it != m_backingStores.end()) > backingStore = it->value.get(); > return backingStore; This is what HashMap::get() is for.

(In reply to comment #13) > (From update of attachment 185169 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=185169&action=review > > I am okay with this for WebKit2, with comments. > > Noam, your call for the r+. Thank you for review! > > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h:159 > > - RefPtr<TextureMapperBackingStore> m_backingStore; > > + TextureMapperBackingStore* m_backingStore; > > Just a comment: For this, you must have a strong guarantee that TextureMapperBackingStore always outlive TextureMapperLayer. > Here the guarantee is m_releasedCoordinatedBackingStores. I think the ownership is too convoluted. m_releasedCoordinatedBackingStores is already existing and this bug uses it. I agree that m_releasedCoordinatedBackingStores is convoluted, so I will remove this twisted lifecycle management after Bug 103854. It is because Bug 103854 will remove the time gap between setting platform layer and flushing layer scene info. > > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:414 > > + CoordinatedBackingStore* backingStore = 0; > > + BackingStoreMap::iterator it = m_backingStores.find(graphicsLayer); > > + if (it != m_backingStores.end()) > > backingStore = it->value.get(); > > return backingStore; > > This is what HashMap::get() is for. Oh, I had put lots of above redundant code. someday I'll get rid of all. Thank you.

Created attachment 185396 [details] Patch

(In reply to comment #13) > (From update of attachment 185169 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=185169&action=review > > I am okay with this for WebKit2, with comments. > > Noam, your call for the r+. Noam, could you review this bug?

Comment on attachment 185396 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185396&action=review > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:420 > + if (m_backingStores.get(graphicsLayer)) To test if a key exists in a HashMap/HashSet, the correct thing to do is HashMap::contains(). > Source/WebKit2/UIProcess/CoordinatedGraphics/LayerTreeRenderer.cpp:435 > + if (!m_backingStores.get(graphicsLayer)) > return; > - } > > - if (!layer->backingStore()) > - return; // The layer has no backing store (and no pending addition). > - > - ASSERT(!m_pendingSyncBackingStores.contains(layer)); > - m_pendingSyncBackingStores.add(layer, 0); > + m_releasedCoordinatedBackingStores.append(m_backingStores.take(graphicsLayer)); > + toGraphicsLayerTextureMapper(graphicsLayer)->setBackingStore(0); This particular code should be using iterators. Otherwise you are doing get() twice.

Created attachment 185400 [details] Patch

(In reply to comment #17) > (From update of attachment 185396 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=185396&action=review > To test if a key exists in a HashMap/HashSet, the correct thing to do is HashMap::contains(). > This particular code should be using iterators. Otherwise you are doing get() twice. Oh, you are right! Done. (In reply to comment #13) > (From update of attachment 185169 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=185169&action=review > Just a comment: For this, you must have a strong guarantee that TextureMapperBackingStore always outlive TextureMapperLayer. > Here the guarantee is m_releasedCoordinatedBackingStores. I think the ownership is too convoluted. After rethinking, I understand this patch does not need to use m_releasedCoordinatedBackingStores. TextureMapperLayer still have RefPtr<TextureMapperBackingStore>. As I mentioned, m_releasedImageBackings is still alive and I'll kill in Bug 108296.

Comment on attachment 185400 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185400&action=review Some nitpick in the changelog, otherwise good to go. Also seems that you have addressed all of Benjamin's concerns. > Source/WebCore/ChangeLog:12 > + because of dead code now. because of dead code now -> because they are no longer used.

Created attachment 185407 [details] Patch for landing

Comment on attachment 185407 [details] Patch for landing Clearing flags on attachment: 185407 Committed r141232: <http://trac.webkit.org/changeset/141232>

All reviewed patches have been landed. Closing bug.