[webkit-reviews] review denied: [Bug 76740] [Chromium] Avoid unnecessary full tile updates without breaking atomicity of commits. : [Attachment 125969] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied David Reveman
<reveman at chromium.org>'s request for review:
Bug 76740: [Chromium] Avoid unnecessary full tile updates without breaking
atomicity of commits.
https://bugs.webkit.org/show_bug.cgi?id=76740

Attachment 125969: Patch
https://bugs.webkit.org/attachment.cgi?id=125969&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
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