[webkit-reviews] review denied: [Bug 79875] [BlackBerry] Upstream classes that handle layer tiling : [Attachment 130806] patch-v3

[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 79875: [BlackBerry] Upstream classes that handle layer tiling
https://bugs.webkit.org/show_bug.cgi?id=79875

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

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


Looks good, but needs one more round :)

> Source/WebCore/platform/graphics/blackberry/LayerTileIndex.h:68
> +template<> struct IntHash<WebCore::TileIndex> {

You probably don't need WebCore:: prefixes in this file.

> Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:153
> +	   dirtyRect = IntRect(IntPoint(), requiredTextureSize);

IntPoint::zero

> Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:159
> +    if (requiredTextureSize.isEmpty() || dirtyRect == IntRect(IntPoint(),
requiredTextureSize)) {

IntPoint::zero

> Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:648
> +IntSize LayerTiler::defaultTileSize()

It seems this is only used internally? If so you can make it a free, static
function.

> Source/WebCore/platform/graphics/blackberry/LayerTiler.h:104
> +	       : type(type)

This looks really confusing, can you add m_ to the member vars? This makes it
clear which is which.

> Source/WebCore/platform/graphics/blackberry/LayerTiler.h:122
> +	   static TextureJob setContents(const SkBitmap& contents, bool
isOpaque) { return TextureJob(SetContents, contents, IntRect(IntPoint(),
IntSize(contents.width(), contents.height())), isOpaque); }

IntPoint::zero