| Summary: | [BlackBerry] Upstream classes that handle layer tiling | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Robin Cao <robin.webkit> | ||||||||||
| Component: | WebKit BlackBerry | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | arvid2.nilsson, charles.wei, leo.yang, levin+threading, manyoso, rakuco, rwlbuis, tonikitoo, webkit.review.bot | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Other | ||||||||||||
| OS: | Other | ||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 73119 | ||||||||||||
| Attachments: |
|
||||||||||||
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. |
Classes that handle layer tiling: LayerTile.cpp LayerTileData.h LayerTile.h LayerTileIndex.h LayerTiler.cpp LayerTiler.h