[webkit-reviews] review denied: [Bug 71350] Added TileCairo and TiledBackingStoreBackendCairo files : [Attachment 113287] Initial implementation

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Tomasz Morawski
<t.morawski at samsung.com>'s request for review:
Bug 71350: Added TileCairo and TiledBackingStoreBackendCairo files
https://bugs.webkit.org/show_bug.cgi?id=71350

Attachment 113287: Initial implementation
https://bugs.webkit.org/attachment.cgi?id=113287&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=113287&action=review


> Source/WebCore/platform/graphics/cairo/OwnPtrCairo.cpp:56
> +template <> void deleteOwnedPtr<cairo_region_t>(cairo_region_t* ptr)
> +{
> +    if (ptr)
> +	   cairo_region_destroy(ptr);
> +}

cairo_region_t should not be wrapped in an OwnPtr because it's reference
counted. Instead you should use RefPtrCairo.

> Source/WebCore/platform/graphics/cairo/OwnPtrCairo.cpp:62
> +template <> void deleteOwnedPtr<cairo_rectangle_t>(cairo_rectangle_t* ptr)
> +{
> +    if (ptr)
> +	   free(ptr);
> +}

I think you can aovid this, because the default implementation calls delete.

> Source/WebCore/platform/graphics/cairo/TileCairo.cpp:78
> +    OwnPtr<cairo_rectangle_int_t> rects = adoptPtr(new
cairo_rectangle_int_t[rectCount]);

It's probably better here to use OwnFastMallocPtr and fastAlloc.

> Source/WebCore/platform/graphics/cairo/TileCairo.cpp:91
> +	   m_buffer = adoptRef(cairo_image_surface_create(CAIRO_FORMAT_ARGB32,
m_backingStore->tileSize().width(), m_backingStore->tileSize().height()));

This line is pretty long. I'd prefer it shorter, at least.

m_buffer = adoptRef(cairo_image_surface_create(CAIRO_FORMAT_ARGB32,
					      
m_backingStore->tileSize().width(),
					      
m_backingStore->tileSize().height()));

> Source/WebCore/platform/graphics/cairo/TileCairo.cpp:126
> +    PlatformContextCairo* platformContext = context->platformContext();
> +    cairo_t* cr = platformContext->cr();

No need to cache platformContext here. You can just do:
cairo_t* cr =  context->platformContext()->cr();

> Source/WebCore/platform/graphics/cairo/TileCairo.cpp:127
> +    cairo_set_source_surface(cr, m_buffer.get(), target.x() - source.x(),
target.y() - source.y());

Okay. 

The result is target.x() - source.x()
source.x() = target.x() - m_rect.x()
Therefore the result is target.x() - target.x() + m_rect.x()

Am I missing something here or can you just write m_rect.x() and m_rect.y() and
get rid of source altogether?

> Source/WebCore/platform/graphics/cairo/TiledBackingStoreBackendCairo.cpp:46
> +static const unsigned checkerColor1 = 0xff555555;
> +static const unsigned checkerColor2 = 0xffaaaaaa;

What colors are these?

> Source/WebCore/platform/graphics/cairo/TiledBackingStoreBackendCairo.cpp:63
> +	       if (alternate)
> +		   setSourceRGBAFromColor(cr.get(), checkerColor1);
> +	       else
> +		   setSourceRGBAFromColor(cr.get(), checkerColor2);

I think it's much clearer to call cairo_set_source_rgba directly here!