Bug 80121

Created attachment 129847 [details] patch

Comment on attachment 129847 [details] patch 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:30 > +static const int defaultMemoryLimit = 64 * 1024 * 1024; // Bytes Comment is a bit short... Maybe // Measured in bytes. > 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 > Source/WebCore/platform/graphics/blackberry/TextureCacheCompositingThread.h:77 > + // The following methods are called by Texture class. Is this comment really relevant?

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

Created attachment 130084 [details] updated patch

Comment on attachment 130084 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=130084&action=review Looks good, please have a look at my remarks before landing, maybe Leo can set cq+ once that is done. > Source/WebCore/platform/graphics/blackberry/Texture.cpp:83 > + bool subImage = (tile.size() == m_size); Could just be bool subImage = tile.size() == m_size; > Source/WebCore/platform/graphics/blackberry/Texture.cpp:91 > + IntSize yeOldeSize = size; yeOldeSize sounds a bit silly, can just be oldSize. > Source/WebCore/platform/graphics/blackberry/Texture.h:24 > +#include "IntRect.h" Looks like it can be a forward class reference. > Source/WebCore/platform/graphics/blackberry/TextureCacheCompositingThread.h:29 > +#include <memory> Nothing in that file seems needed here?

(In reply to comment #5) > (From update of attachment 130084 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=130084&action=review > > > Source/WebCore/platform/graphics/blackberry/Texture.cpp:83 > > + bool subImage = (tile.size() == m_size); > > Could just be bool subImage = tile.size() == m_size; > > > Source/WebCore/platform/graphics/blackberry/Texture.cpp:91 > > + IntSize yeOldeSize = size; > > yeOldeSize sounds a bit silly, can just be oldSize. > > > Source/WebCore/platform/graphics/blackberry/Texture.h:24 > > +#include "IntRect.h" > > Looks like it can be a forward class reference. > > > Source/WebCore/platform/graphics/blackberry/TextureCacheCompositingThread.h:29 > > +#include <memory> > > Nothing in that file seems needed here? Thank you very much! Will fix these before landing.

Created attachment 130545 [details] patch-for-landing

Comment on attachment 130545 [details] patch-for-landing Clearing flags on attachment: 130545 Committed r110040: <http://trac.webkit.org/changeset/110040>

All reviewed patches have been landed. Closing bug.