[webkit-reviews] review denied: [Bug 71350] Added TileCairo and TiledBackingStoreBackendCairo files : [Attachment 113630] Updated

[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 113630: Updated
https://bugs.webkit.org/attachment.cgi?id=113630&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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!