[webkit-reviews] review denied: [Bug 71388] [Chromium] Add support for painting into an SkPicture and then rasterizing into tile-sized chunks : [Attachment 114120] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied David Reveman
<reveman at chromium.org>'s request for review:
Bug 71388: [Chromium] Add support for painting into an SkPicture and then
rasterizing into tile-sized chunks
https://bugs.webkit.org/show_bug.cgi?id=71388

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

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


Lots of feedback of the nitpicky variety - no big changes requested. Overall
this looks great.

>
Source/WebCore/platform/graphics/chromium/BitmapCanvasLayerTextureUpdater.h:47
> +	   virtual void prepareRect(const IntRect& sourceRect) { }

i'm not sure why here and in other files you declare an empty override - should
this function just not be declare pure virtual in the base class if some
significant portion of subclasses will just override it with an empty
implementation.

if you really do want to provide this override, either omit the parameter name
or comment it out with /* */ so we don't get unused variable warnings

>
Source/WebCore/platform/graphics/chromium/BitmapCanvasLayerTextureUpdater.h:56
> +    static PassRefPtr<BitmapCanvasLayerTextureUpdater>
create(PassOwnPtr<LayerPainterChromium>, bool useMapTexSubImage);

you also should declare a virtual d'tor for this class and put its definition
in the .cpp. Since this class is virtual and its base class has a virtual
d'tor, this classes d'tor is virtual whether you declare it or not and if you
do not declare it c++ will happily define a d'tor in the header. This sucks
because the d'tor is large (since it has to invoke the d'tor for members and
the base d'tor) and since it's virtual it is cannot be inlined, so it just
bloats up the object size of every translation unit including this header and
makes the linker do more work and use more memory dealing with lots of
duplicate symbols.

> Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.h:42
> +    explicit CanvasLayerTextureUpdater(PassOwnPtr<LayerPainterChromium>);

declare d'tor here in the header, define in the .cpp please.

> Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:73
> +    virtual Texture* createTexture(TextureManager*) = 0;

it appears that all callers of this function stuff the return value into a
RefPtr<>. If that is the case then it would be better for this function to
return a PassRefPtr<> to reduce the odds of some caller forgetting to put the
pointer in the correct wrapper.

in general any function in WebKit that returns a pointer and is transferring
some notion of ownership should return a PassOwnPtr or PassRefPtr - raw pointer
return types are reserved for when ownership isn't being transferred (for
example returning a pointer to a member that is owned exclusively by the
object)

> Source/WebCore/platform/graphics/chromium/SkiaCanvasLayerTextureUpdater.h:42
> +class SkiaCanvasLayerTextureUpdater : public CanvasLayerTextureUpdater {

i think this would be better called SkPictureCanvasLayerTextureUpdater since
other layer types also use skia in different ways

> Source/WebCore/platform/graphics/chromium/SkiaCanvasLayerTextureUpdater.h:44
> +    explicit
SkiaCanvasLayerTextureUpdater(PassOwnPtr<LayerPainterChromium>);

a public raw c'tor is weird - normally we only do this if we want to
instantiate instances of this class on the stack or as member variables of
other classes, otherwise we use a static create() of the correct smart pointer
type. It seems other CanvasLayerTextureUpdater subclasses expose a create() so
I'm not sure why this one is different.

please declare a virtual d'tor here and define it in the .cpp

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:54
> +    explicit UpdatableTile(PassRefPtr<LayerTextureUpdater::Texture> tex) :
m_tex(tex) { }

nit: this isn't your fault, but we don't typically abbreviate words like
'texture' to 'tex' even in one-liners like this. can you fix this since you're
already here?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:64
> +    // Page-space rectangle that needs to be updated.

"page" is an ambiguous word to use here - what page? the page the layer is in?
Do you mean content space?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:67
> +    RefPtr<LayerTextureUpdater::Texture> m_tex;

similarly this should be m_texture, not m_tex