[webkit-reviews] review granted: [Bug 56749] Enable skia gpu rendering for content layers : [Attachment 86530] proposed patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Mar 22 15:56:41 PDT 2011
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.
More information about the webkit-reviews
mailing list