[Webkit-unassigned] [Bug 80121] [BlackBerry] Upstream Texture and TextureCache

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=80121





--- Comment #3 from Robin Cao <robin.cao at torchmobile.com.cn>  2012-03-05 02:07:44 PST ---
(In reply to comment #2)
> (From update of attachment 129847 [details])
> 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.
> 

Fixed.

> > 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.
> 

I have merge those two functions into one: Texture::create(bool isColor = false)

> > 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.
> 

Removed.

> > Source/WebCore/platform/graphics/blackberry/Texture.h:79
> > +    friend class TextureCacheCompositingThread;
> 
> Usually this is put just under private.
> 

Fixed.

> > Source/WebCore/platform/graphics/blackberry/TextureCacheCompositingThread.cpp:30
> > +static const int defaultMemoryLimit = 64 * 1024 * 1024; // Bytes
> 
> Comment is a bit short... Maybe // Measured in bytes.
> 

Fixed.

> > Source/WebCore/platform/graphics/blackberry/TextureCacheCompositingThread.cpp:97
> > +    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
> 
Good catch, fixed.

> > Source/WebCore/platform/graphics/blackberry/TextureCacheCompositingThread.h:77
> > +    // The following methods are called by Texture class.
> 
> Is this comment really relevant?

I agree this can be removed.

Will update the patch soon.

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