[webkit-reviews] review granted: [Bug 56749] Enable skia gpu rendering for content layers : [Attachment 86530] proposed patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted Alok Priyadarshi
<alokp at chromium.org>'s request for review:
Bug 56749: Enable skia gpu rendering for content layers
https://bugs.webkit.org/show_bug.cgi?id=56749

Attachment 86530: proposed patch
https://bugs.webkit.org/attachment.cgi?id=86530&action=review

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

Overall looks good, but some things need addressing.

Please remember to add this flag to DumpRenderTree as well as chrome (see
--enable-accelerated-compositing as one example) so we can run the layout tests
in this configuration.

> Source/WebCore/ChangeLog:8
> +	   https://bugs.webkit.org/show_bug.cgi?id=56749
> +
> +	   * platform/graphics/chromium/ContentLayerChromium.cpp:

You should try to explain a patch in the ChangeLog.  For example, on this patch
it'd be really nice to understand which macros this behavior is tied to, how it
works at a high level, and what bits are unfinished (for example keeping m_fbo
on the ContentLayerChromium is bad, so explain why it's done that way and what
the plan to improve it is).

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:344
> +	   GraphicsContext3D* context = layerRendererContext();
> +	   m_contentsTexture = LayerTexture::create(context,
layerRenderer()->textureManager());

not sure why you are putting 'context' in a local here - it doesn't seem to buy
much.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.h:68
> +    bool isContentsTextureValid(const IntSize&) const;
> +    bool reserveContentsTexture(const IntSize&);

should these go in the private section? the current subclass
(ImageLayerChromium) isn't calling these functions from what I can tell.