Bug 79875 – [BlackBerry] Upstream classes that handle layer tiling

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Robin Cao]

Classes that handle layer tiling:
    LayerTile.cpp
    LayerTileData.h
    LayerTile.h
    LayerTileIndex.h
    LayerTiler.cpp
    LayerTiler.h

[Robin Cao]

Created attachment 129676 [details]
patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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.

[Robin Cao]

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

[Robin Cao]

LayerTiler.cpp misses an include which is needed in debug build.
#include "LayerMessage.h"

Will add it before landing.

[Adam Treat]

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

[Adam Treat]

(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

[Robin Cao]

Created attachment 130087 [details]
patch v2

[WebKit Review Bot]

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.

[Robin Cao]

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

[Rob Buis]

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.

[Robin Cao]

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

[Robin Cao]

Created attachment 130806 [details]
patch-v3

[WebKit Review Bot]

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.

[Rob Buis]

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

[Robin Cao]

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

[Robin Cao]

Created attachment 131001 [details]
patch-v4

[WebKit Review Bot]

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.

[Rob Buis]

Comment on attachment 131001 [details]
patch-v4

LGTM.

[WebKit Review Bot]

Comment on attachment 131001 [details]
patch-v4

Clearing flags on attachment: 131001

Committed r110300: <http://trac.webkit.org/changeset/110300>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.