Bug 79875

Created attachment 129676 [details] patch

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Attachment 129676 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1 Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:160: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:177: This { should be at the end of the previous line [whitespace/braces] [4] WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitDefines.h" Total errors found: 2 in 142 files If any of these errors are false positives, please file a bug against check-webkit-style.

The style error is false positives, a bug has been filed against check-webkit-style: https://bugs.webkit.org/show_bug.cgi?id=80018

LayerTiler.cpp misses an include which is needed in debug build. #include "LayerMessage.h" Will add it before landing.

Note, this originally had a piece that contained a hunk against LayerTiler.cpp. I've split it up so that is now committed to master_33. That should be included when you redo this Robin. https://bugs.webkit.org/show_bug.cgi?id=80161

(In reply to comment #6) > Note, this originally had a piece that contained a hunk against LayerTiler.cpp. I've split it up so that is now committed to master_33. That should be included when you redo this Robin. https://bugs.webkit.org/show_bug.cgi?id=80161 See webkit/098ee04bed1a7e97ddf7bbeb7389b53a3f98e0ec for a patch that touches LayerTiler.cpp

Created attachment 130087 [details] patch v2

Attachment 130087 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:161: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:178: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 2 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

The style error is false positive, see https://bugs.webkit.org/show_bug.cgi?id=79875#c4

Comment on attachment 130087 [details] patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=130087&action=review Looks good, can be cleaned up some more. > Source/WebCore/platform/graphics/blackberry/LayerTile.h:25 > +#include "IntRect.h" Can be just forward references. > Source/WebCore/platform/graphics/blackberry/LayerTileData.h:26 > +class LayerTileData { Not sure if this tiny class deserves its own file. Could we put it in LayerTile.h? > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:153 > + requiredTextureSize = IntSize(m_layer->contents()->width(), m_layer->contents()->height()); m_layer->contents()->size() should give IntSize too. > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:430 > + } I'd prefer: if (result.second) // Successfully added the new job. return; > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:527 > + FloatRect rect(-d, -d, 2*d, 2*d); Watch spacing 2*d > Source/WebCore/platform/graphics/blackberry/LayerTiler.h:41 > +class TransformationMatrix; Not used? Please check all.

(In reply to comment #11) > (From update of attachment 130087 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=130087&action=review > > Looks good, can be cleaned up some more. > > > Source/WebCore/platform/graphics/blackberry/LayerTile.h:25 > > +#include "IntRect.h" > > Can be just forward references. > Fixed. > > Source/WebCore/platform/graphics/blackberry/LayerTileData.h:26 > > +class LayerTileData { > > Not sure if this tiny class deserves its own file. Could we put it in LayerTile.h? > Fixed > > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:153 > > + requiredTextureSize = IntSize(m_layer->contents()->width(), m_layer->contents()->height()); > > m_layer->contents()->size() should give IntSize too. > Good catch, fixed. > > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:430 > > + } > > I'd prefer: > if (result.second) // Successfully added the new job. > return; > > > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:527 > > + FloatRect rect(-d, -d, 2*d, 2*d); > > Watch spacing 2*d > Fixed. > > Source/WebCore/platform/graphics/blackberry/LayerTiler.h:41 > > +class TransformationMatrix; > > Not used? Please check all. All unnecessary includes and forward declarations are removed.

Created attachment 130806 [details] patch-v3

Attachment 130806 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:160: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:177: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 2 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 130806 [details] patch-v3 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

(In reply to comment #15) > (From update of attachment 130806 [details]) > 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. > I removed 4 WebCore:: prefixes, the other WebCore:: prefixes in the WTF namespace can not be omitted. > > 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 > Fixed. > > 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. > Good point, fixed. > > 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. > My bad, I should have found this in the first round. All member variables have been prefixed now. > > 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 Done.

Created attachment 131001 [details] patch-v4

Attachment 131001 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:167: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:184: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 2 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 131001 [details] patch-v4 LGTM.

Comment on attachment 131001 [details] patch-v4 Clearing flags on attachment: 131001 Committed r110300: <http://trac.webkit.org/changeset/110300>

All reviewed patches have been landed. Closing bug.