[Webkit-unassigned] [Bug 71350] Added TileCairo and TiledBackingStoreBackendCairo files

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=71350


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #113287|review?                     |review-
               Flag|                            |




--- Comment #4 from Martin Robinson <mrobinson at webkit.org>  2011-11-02 09:45:57 PST ---
(From update of attachment 113287)
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!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.