[webkit-reviews] review denied: [Bug 108899] Coordinated Graphics : Refactor CoordinatedSurface to provide beginPaint and endPaint : [Attachment 187823] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Jae Hyun Park
<jae.park at company100.net>'s request for review:
Bug 108899: Coordinated Graphics : Refactor CoordinatedSurface to provide
beginPaint and endPaint
https://bugs.webkit.org/show_bug.cgi?id=108899

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=187823&action=review


r- because of two very fishy things:
-m_graphicsContext is defined on CoordinatedSurface?
-Why is the context own by the CoordinatedSurface and not by the
GraphicsSurface? The asymmetry is not clear to me.

First the first, I think the ChangeLog should explain it if it is correct. For
the second, some context here is enough, I just don't know much about
CoordinatedGraphics.

>
Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cp
p:794
> +GraphicsContext* CoordinatedGraphicsLayer::beginContentUpdate(const IntSize&
size, uint32_t& atlas, IntPoint& offset)

atlas being an uint32_t, shouldn't the argument be named atlasID?

>
Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h:
74
> +    virtual GraphicsContext* beginContentUpdate(const IntSize&,
CoordinatedSurface::Flags, uint32_t& atlasID, IntPoint&) = 0;

It is okay to have the IntPoint named here.
We don't put the attribute names when they are obvious.

Depending on the kind of API you are making, it should not have one output by
return and 2 by passing arguments.
I don't know what are the arguments for so I let you check that.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedSurface.h:61
> +    OwnPtr<GraphicsContext> m_graphicsContext;

Why is this protected of CoordinatedSurface?
CoordinatedSurface defines an interface, it should not force the way it is
implemented.

> Source/WebKit2/Shared/CoordinatedGraphics/WebCoordinatedSurface.cpp:109
> -    if (isBackedByGraphicsSurface())
> -	   return m_graphicsSurface->beginPaint(rect, 0 /* Write without
retaining pixels*/);
> +    if (isBackedByGraphicsSurface()) {
> +	   m_graphicsContext = m_graphicsSurface->beginPaint(rect, 0 /* Write
without retaining pixels*/);

The 0 + comment are mysterious.