Bug 76740

Created attachment 123674 [details] Patch

Comment on attachment 123674 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123674&action=review > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:56 > + explicit UpdatableTile(PassOwnPtr<LayerTextureUpdater::Texture> texture) : m_partialUpdate(0), m_texture(texture) { } is 0 right for a bool? > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1301 > + // Number of textures used for draw should always be one. nit: two.

Created attachment 124567 [details] Patch

(In reply to comment #2) > (From update of attachment 123674 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123674&action=review > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:56 > > + explicit UpdatableTile(PassOwnPtr<LayerTextureUpdater::Texture> texture) : m_partialUpdate(0), m_texture(texture) { } > > is 0 right for a bool? fixed. > > > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1301 > > + // Number of textures used for draw should always be one. > > nit: two. fixed. Thanks!

Comment on attachment 123674 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123674&action=review > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:87 > So we don't forget: If the number of updates including partial updates in this batch exceeds the regular maximum number per-batch, early out to push all the partial updates off to a next batch.

Comment on attachment 124567 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124567&action=review > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:70 > IntRect m_updateRect; > + bool m_partialUpdate; It seems like this boolean is redundant to m_updateRect. m_updateRect is the not the full tile rect iff it's a partial update, yeah? > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:400 > + // Partial update doesn't apply to new textures. > + if (!tile->managedTexture()->isValid(m_tiler->tileSize(), m_textureFormat)) { > + partialUpdate = false; > + newTexture = false; > + } nit: Can you call newTexture something clearer? It's kind of non-intuitive to read this code and see the new texture case set newTexture to false.

Comment on attachment 124567 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124567&action=review >> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:70 >> + bool m_partialUpdate; > > It seems like this boolean is redundant to m_updateRect. m_updateRect is the not the full tile rect iff it's a partial update, yeah? maybe m_partialUpdate is a bad name? the LTH can apply restrictions to allow a limited number of partial updates to actually be done without double buffering. maybe m_doubleBufferedUpdate or something?

(In reply to comment #6) > (From update of attachment 124567 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124567&action=review > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:70 > > IntRect m_updateRect; > > + bool m_partialUpdate; > > It seems like this boolean is redundant to m_updateRect. m_updateRect is the not the full tile rect iff it's a partial update, yeah? No, it's only a partial update if requestPartialTextureUpdate() returned true. I think Dana is right, this variable needs a better name. > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:400 > > + // Partial update doesn't apply to new textures. > > + if (!tile->managedTexture()->isValid(m_tiler->tileSize(), m_textureFormat)) { > > + partialUpdate = false; > > + newTexture = false; > > + } > > nit: Can you call newTexture something clearer? It's kind of non-intuitive to read this code and see the new texture case set newTexture to false. Will do.

Created attachment 124625 [details] Patch

Latest patch makes sure partial updates are done separately. I'll fix the m_partialUpdate variable name tomorrow. In general this patch would be much cleaner if https://bugs.webkit.org/show_bug.cgi?id=72752 landed prior to it.

I don't think landing https://bugs.webkit.org/show_bug.cgi?id=72752 first is a good idea. https://bugs.webkit.org/show_bug.cgi?id=72752 will regress performance on the platforms where we need incremental uploads the most.

View in context: https://bugs.webkit.org/attachment.cgi?id=124625&action=review > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:89 > + if (m_partialEntries.size() && index != m_entryIndex) What about letting partial updates go on if they fit inside count? So 5-1-4 could become 5-5 at least?

Created attachment 124969 [details] Patch

(In reply to comment #12) > View in context: https://bugs.webkit.org/attachment.cgi?id=124625&action=review > > > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:89 > > + if (m_partialEntries.size() && index != m_entryIndex) > > What about letting partial updates go on if they fit inside count? So 5-1-4 could become 5-5 at least? Fixed in latest patch. Also added a unit test to verify this behavior. I kept the m_partialUpdate variable name and instead restructured the code and added some comments to TiledLayerChromium.cpp which I hope makes things clear.

(In reply to comment #11) > I don't think landing https://bugs.webkit.org/show_bug.cgi?id=72752 first is a good idea. https://bugs.webkit.org/show_bug.cgi?id=72752 will regress performance on the platforms where we need incremental uploads the most. That's fine. I'll make sure to clean this up after 72752 lands.

Created attachment 125950 [details] Patch

Created attachment 125969 [details] Patch Remove 'using namespace std' from header and use std::numeric_limits::max() instead of ~0

Comment on attachment 125969 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125969&action=review This looks good, but I'd really like to see some of the stuff in TiledLayerChromium::prepareToUpdateTiles() split out. Lots of nits but content looks good. > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:56 > + explicit UpdatableTile(PassOwnPtr<LayerTextureUpdater::Texture> texture) : m_partialUpdate(false), m_texture(texture) { } one statement per line. expand this initialization out. > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:380 > + bool partialUpdate = false; this is a lot of lines of code to add to an already large and hard-to-follow function. can you break any of this out into a helper? > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:384 > + // For tiles with valid textures, new textures need to be the excessive line wrapping here makes this comment hard for me to read > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:400 > + ASSERT(layerTreeHost()); this ASSERT() isn't super valuable - we can never enter this function with a null LTH > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:405 > + // TODO(reveman): Decide if partial update should be allowed webkit style is FIXME, not TODO(name) > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:417 > + ASSERT(layerTreeHost()); ASSERT() not helpful > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:418 > + ASSERT(tile->managedTexture()->isValid(m_tiler->tileSize(), m_textureFormat)); this is just ASSERT()ing on logic 20 lines up. maybe if this was a helper function it'd be easier to structure the code so that things like this are more obvious? > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:63 > + Texture(FakeLayerTextureUpdater* layer, PassOwnPtr<ManagedTexture> texture) : LayerTextureUpdater::Texture(texture), m_layer(layer) { } one statement per line, so expand initialization > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:72 > + FakeLayerTextureUpdater() : m_prepareCount(0), m_updateCount(0) { } expand

Thanks for looking at this. (In reply to comment #18) > (From update of attachment 125969 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125969&action=review > > This looks good, but I'd really like to see some of the stuff in TiledLayerChromium::prepareToUpdateTiles() split out. Lots of nits but content looks good. > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:56 > > + explicit UpdatableTile(PassOwnPtr<LayerTextureUpdater::Texture> texture) : m_partialUpdate(false), m_texture(texture) { } > > one statement per line. expand this initialization out. fixed. > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:380 > > + bool partialUpdate = false; > > this is a lot of lines of code to add to an already large and hard-to-follow function. can you break any of this out into a helper? let me know what you think about the updated patch. > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:384 > > + // For tiles with valid textures, new textures need to be > > the excessive line wrapping here makes this comment hard for me to read I know there's no 80 column max in the webkit style but I can't make myself do that :) The new patch shouldn't have this problem. > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:400 > > + ASSERT(layerTreeHost()); > > this ASSERT() isn't super valuable - we can never enter this function with a null LTH TiledLayerChromiumTest.pushDirtyTiles causes this to be entered with a null LTH somehow. Maybe just improper use by the unit test but I left the layerTreeHost() checks in the patch for now.. > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:405 > > + // TODO(reveman): Decide if partial update should be allowed > > webkit style is FIXME, not TODO(name) fixed. > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:417 > > + ASSERT(layerTreeHost()); > > ASSERT() not helpful removed from latest patch. > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:418 > > + ASSERT(tile->managedTexture()->isValid(m_tiler->tileSize(), m_textureFormat)); > > this is just ASSERT()ing on logic 20 lines up. maybe if this was a helper function it'd be easier to structure the code so that things like this are more obvious? removed from latest patch. > > > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:63 > > + Texture(FakeLayerTextureUpdater* layer, PassOwnPtr<ManagedTexture> texture) : LayerTextureUpdater::Texture(texture), m_layer(layer) { } > > one statement per line, so expand initialization fixed. > > > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:72 > > + FakeLayerTextureUpdater() : m_prepareCount(0), m_updateCount(0) { } > > expand fixed.

Created attachment 126152 [details] Patch

Comment on attachment 126152 [details] Patch I like :)

Comment on attachment 126152 [details] Patch OK cool, the small functions make that bit a lot easier to read IMO.

Comment on attachment 126152 [details] Patch Clearing flags on attachment: 126152 Committed r107177: <http://trac.webkit.org/changeset/107177>

All reviewed patches have been landed. Closing bug.