[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 #113630|review?                     |review-
               Flag|                            |




--- Comment #7 from Martin Robinson <mrobinson at webkit.org>  2011-11-09 14:08:37 PST ---
(From update of attachment 113630)
View in context: https://bugs.webkit.org/attachment.cgi?id=113630&action=review

This looks pretty close, but I have a few more comments. Sorry for the delay in reviewing.

> Source/WebCore/ChangeLog:12
> +        Added TileCairo and TiledBackingStoreBackend files needed by Tiled Backing Store implementation that uses cairo
> +        library eg. EFL and Gtk WebKit port.
> +        This patch contains some parts of rebased and updated code from 45423 bug. Originals authors of the code are:
> +        Julien Chaffraix  <jchaffraix at codeaurora.org>, Zaheer Ahmad  <zahmad at codeaurora.org>,
> +        Joone Hur  <joone.hur at collabora.co.uk>

In the future please try to align these lines into a paragraph.

> Source/WebCore/ChangeLog:35
> +        * platform/graphics/cairo/RefPtrCairo.cpp:
> +        (WTF::refIfNotNull):
> +        (WTF::derefIfNotNull):
> +        * platform/graphics/cairo/RefPtrCairo.h:
> +        * platform/graphics/cairo/TileCairo.cpp: Added.
> +        (WebCore::TileCairo::TileCairo):
> +        (WebCore::TileCairo::~TileCairo):
> +        (WebCore::TileCairo::isDirty):
> +        (WebCore::TileCairo::isReadyToPaint):
> +        (WebCore::TileCairo::invalidate):
> +        (WebCore::TileCairo::updateBackBuffer):
> +        (WebCore::TileCairo::swapBackBufferToFront):
> +        (WebCore::TileCairo::paint):
> +        (WebCore::TileCairo::resize):
> +        * platform/graphics/cairo/TileCairo.h: Added.
> +        (WebCore::TileCairo::create):
> +        (WebCore::TileCairo::coordinate):
> +        (WebCore::TileCairo::rect):
> +        * platform/graphics/cairo/TiledBackingStoreBackendCairo.cpp: Added.
> +        (WebCore::TiledBackingStoreBackend::createTile):
> +        (WebCore::checkeredSurface):
> +        (WebCore::TiledBackingStoreBackend::paintCheckerPattern):

It's a good idea to fill in these as well. Take a look at other ChangeLog entries for an example.

> Source/WebCore/platform/graphics/cairo/RefPtrCairo.cpp:116
> -
>  #endif
> -
>  }

It seems these lines were removed accidentally.

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

Extra space after '='

> Source/WebCore/platform/graphics/cairo/TileCairo.h:3
> +/*
> + Copyright (C) 2011 Samsung Electronics
> +

Doesn't it make sense for this file to have the same license as TileCairo.cpp? I think it's pretty weird to give them different licenses randomly. I'm unsure exactly what legal problems this could pose, but it's best to make them the same.

> Source/WebCore/platform/graphics/cairo/TileCairo.h:39
> +    static PassRefPtr<Tile> create(TiledBackingStore* backingStore, const Coordinate& tileCoordinate) { return adoptRef(new TileCairo(backingStore, tileCoordinate)); }

Please reformat this to be across multiple lines. It's quite a long line right now.

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

My issue with this is that it's really hard to tell just by looking at this lines what the colors are. I think they should be renamed to reflect what colors they are.

> Source/WebCore/platform/graphics/cairo/TiledBackingStoreBackendCairo.cpp:68
> +    for (unsigned y = 0; y < checkerSize; y += checkerSize / 2) {
> +        bool alternate = y % checkerSize;
> +        for (unsigned x = 0; x < checkerSize; x += checkerSize / 2) {
> +            if (alternate)
> +                setSourceRGBAFromColor(cr.get(), checkerColor1);
> +            else
> +                setSourceRGBAFromColor(cr.get(), checkerColor2);
> +            cairo_rectangle(cr.get(), x, y, checkerSize / 2, checkerSize / 2);
> +            cairo_fill(cr.get());
> +            alternate = !alternate;
> +        }
> +    }

I think it will be much more readable to unroll this loop:

unsigned halfCheckerSize = checkerSize / 2;
cairo_rectangle(cr.get(), 0, 0, halfCheckerSize, halfCheckerSize);
cairo_rectangle(cr.get(), halfCheckerSize, halfCheckerSize, halfCheckerSize, halfCheckerSize);
setSourceRGBAFromColor(cr.get(), checkerColor1);
cairo_fill(cr.get());

cairo_rectangle(cr.get(), halfCheckerSize, 0, halfCheckerSize, halfCheckerSize);
cairo_rectangle(cr.get(), 0, halfCheckerSize, halfCheckerSize, halfCheckerSize);
setSourceRGBAFromColor(cr.get(), checkerColor2);
cairo_fill(cr.get());

It's even fewer lines!

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