[webkit-reviews] review denied: [Bug 37440] [WINCE] Port tiled backing store : [Attachment 53156] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Kwang Yul Seo
<kwangyul.seo at gmail.com>'s request for review:
Bug 37440: [WINCE] Port tiled backing store
https://bugs.webkit.org/show_bug.cgi?id=37440

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

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +	   * platform/graphics/wince/TileWince.cpp: Added.

This file should be named TileWinCE.cpp (see bug 37287).

> +	   (WebCore::checkeredBitmap):
> +	   (WebCore::Tile::Tile):
> +	   (WebCore::Tile::~Tile):
> +	   (WebCore::Tile::isDirty):
> +	   (WebCore::Tile::isReadyToPaint):
> +	   (WebCore::Tile::invalidate):
> +	   (WebCore::enclosingRect):
> +	   (WebCore::getUpdateRects):
> +	   (WebCore::Tile::updateBackBuffer):
> +	   (WebCore::Tile::swapBackBufferToFront):
> +	   (WebCore::Tile::paint):
> +	   (WebCore::Tile::paintCheckerPattern):

Function-level comments are helpful to anyone trying to understand the patch.
That includes reviewers and people in the future who are trying to figure out
what the point of this patch was (if, e.g., it caused a regression or contained
a bug). It's especially good if the comments explain the "why" of the patch,
not just the "how".

> +static PassRefPtr<SharedBitmap> checkeredBitmap()
> +{
> +    static RefPtr<SharedBitmap> bitmap;
> +    if (!bitmap) {
> +	   bitmap = SharedBitmap::createInstance(true, checkerSize,
checkerSize, false);
> +	   unsigned key1, key2;
> +	   HDC dc = bitmap->getDC(&key1, &key2);
> +	   GraphicsContext context(dc);
> +
> +	   Color color1(checkerColor1);
> +	   Color color2(checkerColor2);
> +	   for (unsigned y = 0; y < checkerSize; y += checkerSize / 2) {
> +	       bool alternate = y % checkerSize;
> +	       for (unsigned x = 0; x < checkerSize; x += checkerSize / 2) {
> +		   context.fillRect(FloatRect(x, y, checkerSize / 2,
checkerSize / 2),
> +		       alternate ? color1 : color2, DeviceColorSpace);
> +		   alternate = !alternate;
> +	       }
> +	   }
> +
> +	   bitmap->releaseDC(dc, key1, key2);
> +    }
> +    return bitmap.release();
> +}

It looks like this function is trying to create the bitmap just once and then
return that single cached instance. But that's not the behavior it will have.
RefPtr::release nulls out the RefPtr, so if (!bitmap) will always be true.

I think instead you should change this function to return a bare pointer. It's
a also slightly nicer style to have two separate functions: a "create" function
that always allocates a new bitmap, and a getter that returns a single cached
instance. Like this:

static PassRefPtr<SharedBitmap> createCheckeredBitmap()
{
    RefPtr<SharedBitmap> bitmap = SharedBitmap::createInstance(...);
    // Draw the checker pattern
    return bitmap.release();
}

static SharedBitmap* checkeredBitmap()
{
    static SharedBitmap* bitmap = createCheckeredBitmap().releaseRef();
    return bitmap;
}

> +Tile::Tile(TiledBackingStore* backingStore, const Coordinate&
tileCoordinate)
> +    : m_backingStore(backingStore)
> +    , m_coordinate(tileCoordinate)
> +    , m_rect(m_backingStore->tileRectForCoordinate(tileCoordinate))
> +    , m_buffer(0)
> +    , m_backBuffer(0)

No need to initialize m_buffer or m_backBuffer here. RefPtr does this for you
automatically.

> +static IntRect enclosingRect(HRGN hrgn)
> +{
> +    RECT r;
> +    GetRgnBox(hrgn, &r);
> +    return r;
> +}
> +

This function is only called once. I'm not sure it's worth having a separate
function for this.

> +static void getUpdateRects(HRGN region, Vector<IntRect>& rects)
> +{
> +    const int rectThreshold = 10;
> +    const float wastedSpaceThreshold = 0.75f;
> +
> +    IntRect dirtyRect(enclosingRect(region));
> +
> +    DWORD regionDataSize = GetRegionData(region, sizeof(RGNDATA), 0);
> +    if (!regionDataSize) {
> +	   rects.append(dirtyRect);
> +	   return;
> +    }
> +
> +    Vector<unsigned char> buffer(regionDataSize);
> +    RGNDATA* regionData = reinterpret_cast<RGNDATA*>(buffer.data());
> +    GetRegionData(region, regionDataSize, regionData);
> +    if (regionData->rdh.nCount > rectThreshold) {
> +	   rects.append(dirtyRect);
> +	   return;
> +    }
> +
> +    double singlePixels = 0.0;
> +    unsigned i;
> +    RECT* rect;
> +    for (i = 0, rect = reinterpret_cast<RECT*>(regionData->Buffer); i <
regionData->rdh.nCount; i++, rect++)
> +	   singlePixels += (rect->right - rect->left) * (rect->bottom -
rect->top);
> +
> +    double unionPixels = dirtyRect.width() * dirtyRect.height();
> +    double wastedSpace = 1.0 - (singlePixels / unionPixels);
> +    if (wastedSpace <= wastedSpaceThreshold) {
> +	   rects.append(dirtyRect);
> +	   return;
> +    }
> +
> +    for (i = 0, rect = reinterpret_cast<RECT*>(regionData->Buffer); i <
regionData->rdh.nCount; i++, rect++)
> +	   rects.append(*rect);
> +}

This looks to be a copy of the function of the same name in
WebKit/win/WebView.cpp. Can we share the code instead of duplicating it?

> +void Tile::updateBackBuffer()
> +{
> +    if (m_buffer && !isDirty())
> +	   return;
> +
> +    if (!m_backBuffer) {
> +	   if (!m_buffer)
> +	       m_backBuffer = SharedBitmap::createInstance(
> +		       true, m_backingStore->m_tileSize.width(),
m_backingStore->m_tileSize.height(), false);

We normally would just put this all on one line. I think you should do that
here.

It would be nice if there were an overload of SharedBitmap::createInstance that
took a const IntSize&. It would also be nice if it used enums instead of
booleans, as I have no idea what "true" and "false" mean here.

> +	   else {
> +	       // Currently all buffers are updated synchronously at the same
time so there is no real need
> +	       // to have separate back and front buffers. Just use the
existing buffer.

If there's no need for two buffers, why do we have them?

> +	       m_backBuffer = m_buffer;
> +	       m_buffer = 0;

You can do this more efficiently like this:

m_backBuffer = m_buffer.release();

> +    HRGN nullRegion = CreateRectRgn(0, 0, 0, 0);
> +    m_dirtyRegion.set(nullRegion);

No need for the local nullRegion variable.

> +    int size = dirtyRects.size();
> +    for (int n = 0; n < size; ++n)  {

It's a little better to use size_t instead of int when iterating over a Vector.


> +void Tile::swapBackBufferToFront()
> +{
> +    if (!m_backBuffer)
> +	   return;
> +    m_buffer = m_backBuffer;
> +    m_backBuffer = 0;
> +}

You can do this more efficiently like this:

m_buffer = m_backBuffer.release();

> +void Tile::paint(GraphicsContext* context, const IntRect& rect)
> +{
> +    if (!m_buffer)
> +	   return;
> +
> +    IntRect target = intersection(rect, m_rect);
> +    IntRect source((target.x() - m_rect.x()),
> +		      (target.y() - m_rect.y()),

No need for the parentheses around these expressions. I'm also not sure all the
newlines are making this more readable.

> +void Tile::paintCheckerPattern(GraphicsContext* context, const FloatRect&
target)
> +{
> +    FloatRect tileRectIn(0, 0, checkerSize, checkerSize);
> +    context->drawBitmapPattern(checkeredBitmap().get(),
> +				  tileRectIn,
> +				  AffineTransform(),
> +				  FloatPoint(0, 0),

You can just say FloatPoint()

> +				  CompositeCopy,
> +				  target,
> +				  IntSize(checkerSize, checkerSize));

Is there no way to get the size from checkeredBitmap? It seems unfortunate to
have to use the constants again here.

Again, I don't think all the newlines are helping here.