[webkit-reviews] review denied: [Bug 69441] [chromium] Keep track of the paint rect in layers : [Attachment 110013] renamed repaint to update

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Shawn Singh
<shawnsingh at chromium.org>'s request for review:
Bug 69441: [chromium] Keep track of the paint rect in layers
https://bugs.webkit.org/show_bug.cgi?id=69441

Attachment 110013: renamed repaint to update
https://bugs.webkit.org/attachment.cgi?id=110013&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=110013&action=review


Getting close! You probably want to read the feedback bottom up as the
interesting comments are near the bottom of the patch.

> Source/WebCore/ChangeLog:4
> +	   Added a FloatRect to LayerChromium that tracks what region was
actually
> +	   updated in a layer.

bug titles should ideally describe what the patch is doing or what problem it's
solving, not how. also, it's very helpful to tag chromium-specific patches with
[chromium] so that people looking through the commit log for regressions on
other platforms (for example) can know that this patch only impacts chromium.
so maybe something like "[chromium] Track the updated portion of the layer in
updateCompositingResources()"

> Source/WebCore/ChangeLog:10
> +	   Tests will be added in a separate patch after the
> +	   GraphicsContext3D and allocator can be mocked.

FYI we can mock out GraphicsContext3D today I believe - take a look at
MockGraphicsContext3D and CompositorMockGraphicsContext3D in
WebKit/chromium/tests/.  TextureAllocator is pretty thin and designed to be
mocked easily if you're so inclined.  I'm not sure if there's all that much to
test here though since the value's not being used for anything.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:84
> +    m_actuallyUpdatedRect = FloatRect(0.0f, 0.0f, bounds().width(),
bounds().height());

Could be FloatRect(FloatPoint::zero(), bounds());

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:218
> +    FloatRect m_actuallyUpdatedRect;

sorry to rathole on the name but this still feels awkward.  why isn't this
m_updatedRect? is there another variable that we are trying to distinguish this
from with the 'actually' qualifier?

> Source/WebCore/platform/graphics/chromium/PluginLayerChromium.cpp:71
> +    m_actuallyUpdatedRect = FloatRect(0.0f, 0.0f, bounds().width(),
bounds().height());

could be written as:

FloatRect(FloatPoint::zero(), bounds());

> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:140
> +    m_actuallyUpdatedRect = FloatRect(0.0f, 0.0f, bounds().width(),
bounds().height());

same here - this could be written as FloatRect(FloatPoint::zero(), bounds());

> Source/WebCore/platform/graphics/chromium/WebGLLayerChromium.cpp:95
> +	   m_actuallyUpdatedRect = FloatRect(0.0f, 0.0f, bounds().width(),
bounds().height());

ditto re: FloatRect(). since this is repeated 4x should it be in the base class
to be the default behavior so that subclasses can override it with more
specific bounds if they have it? if we add a new layer type we'll have to
remember to add this incantation.

maybe one way to do this would be to have a virtual updatedRect() getter on
LayerChromium that returns this and override that in TiledLayerChromium with a
function that calculates the value it uses.