[webkit-reviews] review denied: [Bug 80121] [BlackBerry] Upstream Texture and TextureCache : [Attachment 129847] patch

[bugzilla-daemon at webkit.org]

Rob Buis <rwlbuis at gmail.com> has denied Robin Cao
<robin.cao at torchmobile.com.cn>'s request for review:
Bug 80121: [BlackBerry] Upstream Texture and TextureCache
https://bugs.webkit.org/show_bug.cgi?id=80121

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

------- Additional Comments from Rob Buis <rwlbuis at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=129847&action=review


Looks good overall, but can be improved some more.

> Source/WebCore/platform/graphics/blackberry/Texture.h:24
> +#include "Color.h"

A forward reference could be enough.

> Source/WebCore/platform/graphics/blackberry/Texture.h:28
> +#include <SkBitmap.h>

Ditto.

> Source/WebCore/platform/graphics/blackberry/Texture.h:50
> +    }

I think that this would be clearer with an enum. So you could do
Texture::create(ColorTexture|NormalTexture) or something and only need one
create.

> Source/WebCore/platform/graphics/blackberry/Texture.h:78
> +    // The following method is only called by our dear friend.

Ha ha :) Not the best of comments, I think you can remove it without losing
anything.

> Source/WebCore/platform/graphics/blackberry/Texture.h:79
> +    friend class TextureCacheCompositingThread;

Usually this is put just under private.

>
Source/WebCore/platform/graphics/blackberry/TextureCacheCompositingThread.cpp:3
0
> +static const int defaultMemoryLimit = 64 * 1024 * 1024; // Bytes

Comment is a bit short... Maybe // Measured in bytes.

>
Source/WebCore/platform/graphics/blackberry/TextureCacheCompositingThread.cpp:9
7
> +    int delta = (texture->width() * texture->height() - oldSize.width() *
oldSize.height()) * texture->bytesPerPixel();

bytesPerPixel is staic, so could as well use Texturee: bytesPerPixel() to make
it consistent

>
Source/WebCore/platform/graphics/blackberry/TextureCacheCompositingThread.h:77
> +    // The following methods are called by Texture class.

Is this comment really relevant?