[webkit-reviews] review denied: [Bug 47016] [chromium] GPU compositor cannot handle large layers : [Attachment 70201] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Adrienne Walker
<enne at google.com>'s request for review:
Bug 47016: [chromium] GPU compositor cannot handle large layers
https://bugs.webkit.org/show_bug.cgi?id=47016

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

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

Needs a touch more cleanup.

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:130
> +    , m_allocatedTextureSize(0, 0)
> +    , m_skipsDraw(false)
> +    , m_largeLayerDrawRect(0, 0, 0, 0)
> +    , m_largeLayerDirtyRect(0, 0, 0, 0)

The default constructor should do the right thing for all of these except for
m_skipsDraw.  No need to list them out.

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:149
> +bool ContentLayerChromium::largeLayer() const

This function name isn't very clear - it actually looks more like a getter for
some large layer member variable.  Maybe something like
requiresClippedUpdateRect() or something?

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:155
> +void ContentLayerChromium::calculateLargeLayerViewRect(IntRect* dirtyRect,
> +							  IntRect* drawRect)
const

nit: don't line wrap this

WebKit typically uses reference parameters for out params

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:164
> +    // 1) The minimal texture space rectangle to be uploaded, returned in
> +    // dirtyRect.
> +    // 2) The content rect-relative rectangle to draw this texture in,
returned
> +    // in drawRect.

nit: this is over-wrapped. There's not a firm column limit in WebKit but this
would be more readable if (1) and (2) were each only on one line.

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:175
> +    const IntSize layerPos(transform.m41() - m_bounds.width() / 2,
> +			      transform.m42() - m_bounds.height() / 2);
> +    // Transform the contentRect into the space of the layer.
> +    IntRect contentRectInLayerSpace(IntPoint(0, 0) - layerPos,
> +				       contentRect.size());

I don't fully grok the math here, but it seems like the negation in
contentRectInLayerSpace's initialization could be done in the layerPos
calculation and that layerPos is really an IntPoint and not an IntSize.  Or in
other words something like:

const IntPoint layerPos(m_bounds.width() - transform.m41() / 2,
m_bounds.height() - transform.m42() / 2);
IntRect contentRectInLayerSpace(layerPos, contentRect.size());

> WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:375
> +	   transform.setM41(m_largeLayerDrawRect.center().x());
> +	   transform.setM42(m_largeLayerDrawRect.center().y());

is this just doing a translate?  If so please use the appropriate translate()
function on TransformationMatrix to be clearer

> WebCore/platform/graphics/chromium/ContentLayerChromium.h:95
> +    bool largeLayer() const;
> +    void calculateLargeLayerViewRect(IntRect* dirtyRect,
> +					IntRect* drawRect) const;
>  
>      unsigned m_contentsTexture;
>      IntSize m_allocatedTextureSize;
>      bool m_skipsDraw;
> -
> +    IntRect m_largeLayerDrawRect;
> +    IntRect m_largeLayerDirtyRect;

It looks like the newly added functions/members should all be private: except
for largeLayer()