Initial QuadTree implementation

Initial QuadTree implementation

In an effort to find a faster bounding box hierarchy than the R-Tree, a QuadTree has been implemented here.
For now, the QuadTree construction is generally faster than the R-Tree and the queries are a bit slower, so overall, SKP local tests showed QuadTree performance similar to the R-Tree performance.

Tests and bench are included in this cl.

At this point, I'd like to be able to commit this in order to more easily use the bots to test multiple configurations and a larger number of SKPs. The R-Tree BBH is still used by default so this change shouldn't affect chromium.

BUG=skia:

Committed: http://code.google.com/p/skia/source/detail?r=13282

[reed1, 2014-01-24 19:15:44]

sgtm -- do we not already have decent tests for RTree / GridTree? Could those
just be tweaked to loop around and try out all the different subclasses, instead
of creating a new file?

[fmalita_google_do_not_use, 2014-01-24 22:25:00]

It would be nice to also hook this into SampleApp at some point ('B' shortcut
currently switches between no-BBH/R-tree/TileGrid), to be able to
micro-benchmark local SKPs in continuous/tiled repaint mode.

[sugoi, 2014-01-27 13:37:43]

- Merged RTree and QuadTree tests
- Added QuadTree to SampleApp

[Justin Novosad, 2014-01-28 21:26:13]

https://codereview.chromium.org/131343011/diff/30002/src/core/SkQuadTree.cpp
File src/core/SkQuadTree.cpp (right):

https://codereview.chromium.org/131343011/diff/30002/src/core/SkQuadTree.cpp#...
src/core/SkQuadTree.cpp:13: class SkQuadTree::QuadTree {
I find SkQuadTree vs. QuadTree confusing.  Why is this separated into two
classes? I don<t see why we would need an internal class given that the header
is not public.

https://codereview.chromium.org/131343011/diff/30002/src/core/SkQuadTree.cpp#...
src/core/SkQuadTree.cpp:17: , fNorthWest(NULL)
We use top/left/right/bottom in skia.

https://codereview.chromium.org/131343011/diff/30002/src/core/SkQuadTree.cpp#...
src/core/SkQuadTree.cpp:39: const SkIRect& getBoundary() const { return
fBoundary; }
Bondary -> Bounds, to be consistent with rest of skia

https://codereview.chromium.org/131343011/diff/30002/src/core/SkQuadTree.h
File src/core/SkQuadTree.h (right):

https://codereview.chromium.org/131343011/diff/30002/src/core/SkQuadTree.h#ne...
src/core/SkQuadTree.h:65: virtual void rewindInserts() SK_OVERRIDE {}
This needs to be implemented, otherwise there will we bugs with peephole
optimizations.

[sugoi1, 2014-01-29 15:29:08]

https://codereview.chromium.org/131343011/diff/30002/src/core/SkQuadTree.cpp
File src/core/SkQuadTree.cpp (right):

https://codereview.chromium.org/131343011/diff/30002/src/core/SkQuadTree.cpp#...
src/core/SkQuadTree.cpp:13: class SkQuadTree::QuadTree {
On 2014/01/28 21:26:13, junov wrote:
> I find SkQuadTree vs. QuadTree confusing.  Why is this separated into two
> classes? I don<t see why we would need an internal class given that the header
> is not public.

Renamed to QuadTreeNode

https://codereview.chromium.org/131343011/diff/30002/src/core/SkQuadTree.cpp#...
src/core/SkQuadTree.cpp:17: , fNorthWest(NULL)
On 2014/01/28 21:26:13, junov wrote:
> We use top/left/right/bottom in skia.

Done.

https://codereview.chromium.org/131343011/diff/30002/src/core/SkQuadTree.cpp#...
src/core/SkQuadTree.cpp:39: const SkIRect& getBoundary() const { return
fBoundary; }
On 2014/01/28 21:26:13, junov wrote:
> Bondary -> Bounds, to be consistent with rest of skia

Done.

https://codereview.chromium.org/131343011/diff/30002/src/core/SkQuadTree.h
File src/core/SkQuadTree.h (right):

https://codereview.chromium.org/131343011/diff/30002/src/core/SkQuadTree.h#ne...
src/core/SkQuadTree.h:65: virtual void rewindInserts() SK_OVERRIDE {}
On 2014/01/28 21:26:13, junov wrote:
> This needs to be implemented, otherwise there will we bugs with peephole
> optimizations.

Done.

https://codereview.chromium.org/131343011/diff/90001/src/core/SkQuadTree.cpp
File src/core/SkQuadTree.cpp (right):

https://codereview.chromium.org/131343011/diff/90001/src/core/SkQuadTree.cpp#...
src/core/SkQuadTree.cpp:15: QuadTreeNode(const SkIRect& bounds)
Renamed QuadTree to QuadTreeNode

https://codereview.chromium.org/131343011/diff/90001/src/core/SkQuadTree.cpp#...
src/core/SkQuadTree.cpp:78: int depthBR = fBottomRight->getDepth(i);
Fixed an issue here (fNorthWest was use in every case, oops)

https://codereview.chromium.org/131343011/diff/90001/src/core/SkQuadTree.cpp#...
src/core/SkQuadTree.cpp:84: void rewindInserts(SkBBoxHierarchyClient* client) {
Implemented recursive rewindInserts for QuadTreeNode

https://codereview.chromium.org/131343011/diff/90001/src/core/SkQuadTree.cpp#...
src/core/SkQuadTree.cpp:159: SkIRect fBounds;
Renamed Boundary to Bounds

https://codereview.chromium.org/131343011/diff/90001/src/core/SkQuadTree.cpp#...
src/core/SkQuadTree.cpp:168: QuadTreeNode* fBottomRight;
Renamed North/South/West/East to Top/Bottom/Left/Right

https://codereview.chromium.org/131343011/diff/90001/src/core/SkQuadTree.h
File src/core/SkQuadTree.h (right):

https://codereview.chromium.org/131343011/diff/90001/src/core/SkQuadTree.h#ne...
src/core/SkQuadTree.h:82: QuadTreeNode* fRoot;
Renamed fQuadTree to fRoot, since it is effectively the root node of the quad
tree.

[Stephen White, 2014-01-29 16:14:58]

https://codereview.chromium.org/131343011/diff/90001/src/core/SkQuadTree.h
File src/core/SkQuadTree.h (right):

https://codereview.chromium.org/131343011/diff/90001/src/core/SkQuadTree.h#ne...
src/core/SkQuadTree.h:70: struct Data {
Nit: is it necessary to declare this in the header file? It doesn't seem to be
used here.

[sugoi1, 2014-01-29 16:18:51]

https://codereview.chromium.org/131343011/diff/90001/src/core/SkQuadTree.h
File src/core/SkQuadTree.h (right):

https://codereview.chromium.org/131343011/diff/90001/src/core/SkQuadTree.h#ne...
src/core/SkQuadTree.h:70: struct Data {
On 2014/01/29 16:14:59, Stephen White wrote:
> Nit: is it necessary to declare this in the header file? It doesn't seem to be
used here.

Done. You're right, it was used in my initial implementation, but it is no
longer necessary. Moved it to cpp file.

[reed1, 2014-01-29 16:20:43]

for now, can we move the new header to effects or utils or somewhere like that?
include/core is pretty visible, and should be reserved for very stable APIs, and
then only ones that other parts of the core require.

[sugoi1, 2014-01-29 16:30:15]

On 2014/01/29 16:20:43, reed1 wrote:
> for now, can we move the new header to effects or utils or somewhere like
that?
> include/core is pretty visible, and should be reserved for very stable APIs,
and
> then only ones that other parts of the core require.

Would src/core be acceptable ?

[reed1, 2014-01-29 16:37:48]

On 2014/01/29 16:30:15, sugoi1 wrote:
> On 2014/01/29 16:20:43, reed1 wrote:
> > for now, can we move the new header to effects or utils or somewhere like
> that?
> > include/core is pretty visible, and should be reserved for very stable APIs,
> and
> > then only ones that other parts of the core require.
> 
> Would src/core be acceptable ?

absolutely!

[sugoi1, 2014-01-29 16:45:05]

On 2014/01/29 16:37:48, reed1 wrote:
> On 2014/01/29 16:30:15, sugoi1 wrote:
> > On 2014/01/29 16:20:43, reed1 wrote:
> > > for now, can we move the new header to effects or utils or somewhere like
> > that?
> > > include/core is pretty visible, and should be reserved for very stable
APIs,
> > and
> > > then only ones that other parts of the core require.
> > 
> > Would src/core be acceptable ?
> 
> absolutely!

Great, done.

[reed1, 2014-01-29 16:58:27]

https://codereview.chromium.org/131343011/diff/150001/src/core/SkBBoxHierarchy.h
File src/core/SkBBoxHierarchy.h (right):

https://codereview.chromium.org/131343011/diff/150001/src/core/SkBBoxHierarch...
src/core/SkBBoxHierarchy.h:72: * Gets the depth of the tree structure
Questions for the dox:

What is this used for?
What does 0 mean, what does 1 mean?

https://codereview.chromium.org/131343011/diff/150001/src/core/SkRTree.h
File src/core/SkRTree.h (right):

https://codereview.chromium.org/131343011/diff/150001/src/core/SkRTree.h#newc...
src/core/SkRTree.h:88: virtual int getDepth() const SK_OVERRIDE { return
this->isEmpty() ? 0 : fRoot.fChild.subtree->fLevel + 1; }
line-wrap

[sugoi1, 2014-01-29 18:21:53]

https://codereview.chromium.org/131343011/diff/150001/src/core/SkBBoxHierarchy.h
File src/core/SkBBoxHierarchy.h (right):

https://codereview.chromium.org/131343011/diff/150001/src/core/SkBBoxHierarch...
src/core/SkBBoxHierarchy.h:72: * Gets the depth of the tree structure
On 2014/01/29 16:58:28, reed1 wrote:
> Questions for the dox:
> 
> What is this used for?
> What does 0 mean, what does 1 mean?

Done.

https://codereview.chromium.org/131343011/diff/150001/src/core/SkRTree.h
File src/core/SkRTree.h (right):

https://codereview.chromium.org/131343011/diff/150001/src/core/SkRTree.h#newc...
src/core/SkRTree.h:88: virtual int getDepth() const SK_OVERRIDE { return
this->isEmpty() ? 0 : fRoot.fChild.subtree->fLevel + 1; }
On 2014/01/29 16:58:28, reed1 wrote:
> line-wrap

Done.

[reed1, 2014-01-29 18:41:16]

https://codereview.chromium.org/131343011/diff/190001/src/core/SkTileGrid.h
File src/core/SkTileGrid.h (right):

https://codereview.chromium.org/131343011/diff/190001/src/core/SkTileGrid.h#n...
src/core/SkTileGrid.h:64: virtual int getCount() const SK_OVERRIDE;
Can we return -1 for getDepth, but non-zero for getCount?

[sugoi1, 2014-01-29 18:43:57]

https://codereview.chromium.org/131343011/diff/190001/src/core/SkTileGrid.h
File src/core/SkTileGrid.h (right):

https://codereview.chromium.org/131343011/diff/190001/src/core/SkTileGrid.h#n...
src/core/SkTileGrid.h:64: virtual int getCount() const SK_OVERRIDE;
On 2014/01/29 18:41:16, reed1 wrote:
> Can we return -1 for getDepth, but non-zero for getCount?

Sure. If the structure's not a tree (like a tile grid), then you do insert data,
which means getCount() is non 0, but it still returns -1 for getDepth(), because
it's not a tree.

[reed1, 2014-01-29 18:49:10]

On 2014/01/29 18:43:57, sugoi1 wrote:
> https://codereview.chromium.org/131343011/diff/190001/src/core/SkTileGrid.h
> File src/core/SkTileGrid.h (right):
> 
>
https://codereview.chromium.org/131343011/diff/190001/src/core/SkTileGrid.h#n...
> src/core/SkTileGrid.h:64: virtual int getCount() const SK_OVERRIDE;
> On 2014/01/29 18:41:16, reed1 wrote:
> > Can we return -1 for getDepth, but non-zero for getCount?
> 
> Sure. If the structure's not a tree (like a tile grid), then you do insert
data,
> which means getCount() is non 0, but it still returns -1 for getDepth(),
because
> it's not a tree.

Ah, ok. .. then I don't understand the use for getDepth. Can you augment the dox
to explain why a caller would want to know the depth? More generally, for both
getDepth and getCount, it would be great for the dox to relate these two values
to other APIs on the class.

[sugoi1, 2014-01-29 18:58:43]

On 2014/01/29 18:49:10, reed1 wrote:
> On 2014/01/29 18:43:57, sugoi1 wrote:
> > https://codereview.chromium.org/131343011/diff/190001/src/core/SkTileGrid.h
> > File src/core/SkTileGrid.h (right):
> > 
> >
>
https://codereview.chromium.org/131343011/diff/190001/src/core/SkTileGrid.h#n...
> > src/core/SkTileGrid.h:64: virtual int getCount() const SK_OVERRIDE;
> > On 2014/01/29 18:41:16, reed1 wrote:
> > > Can we return -1 for getDepth, but non-zero for getCount?
> > 
> > Sure. If the structure's not a tree (like a tile grid), then you do insert
> data,
> > which means getCount() is non 0, but it still returns -1 for getDepth(),
> because
> > it's not a tree.
> 
> Ah, ok. .. then I don't understand the use for getDepth. Can you augment the
dox
> to explain why a caller would want to know the depth? More generally, for both
> getDepth and getCount, it would be great for the dox to relate these two
values
> to other APIs on the class.

Ok, no problem. So getCount() gives you the number of "data" (void* in this
case) inserted inside the BBoxHierarchy (regardless of what type of
BBoxHierarchy we have).
getDepth() is currently only used for tests, to check if the depth we get is
within the expected range. I doubt anyone would want to call getDepth(), apart
from tests, since this is information about the underlying structure, which is
probably not generally useful to anyone.

[reed1, 2014-01-29 19:13:07]

On 2014/01/29 18:58:43, sugoi1 wrote:
> On 2014/01/29 18:49:10, reed1 wrote:
> > On 2014/01/29 18:43:57, sugoi1 wrote:
> > >
https://codereview.chromium.org/131343011/diff/190001/src/core/SkTileGrid.h
> > > File src/core/SkTileGrid.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/131343011/diff/190001/src/core/SkTileGrid.h#n...
> > > src/core/SkTileGrid.h:64: virtual int getCount() const SK_OVERRIDE;
> > > On 2014/01/29 18:41:16, reed1 wrote:
> > > > Can we return -1 for getDepth, but non-zero for getCount?
> > > 
> > > Sure. If the structure's not a tree (like a tile grid), then you do insert
> > data,
> > > which means getCount() is non 0, but it still returns -1 for getDepth(),
> > because
> > > it's not a tree.
> > 
> > Ah, ok. .. then I don't understand the use for getDepth. Can you augment the
> dox
> > to explain why a caller would want to know the depth? More generally, for
both
> > getDepth and getCount, it would be great for the dox to relate these two
> values
> > to other APIs on the class.
> 
> Ok, no problem. So getCount() gives you the number of "data" (void* in this
> case) inserted inside the BBoxHierarchy (regardless of what type of
> BBoxHierarchy we have).
> getDepth() is currently only used for tests, to check if the depth we get is
> within the expected range. I doubt anyone would want to call getDepth(), apart
> from tests, since this is information about the underlying structure, which is
> probably not generally useful to anyone.

Thanks. Please add a version of that to the dox in the baseclass.

[sugoi1, 2014-01-29 19:21:50]

On 2014/01/29 19:13:07, reed1 wrote:
> On 2014/01/29 18:58:43, sugoi1 wrote:
> > On 2014/01/29 18:49:10, reed1 wrote:
> > > On 2014/01/29 18:43:57, sugoi1 wrote:
> > > >
> https://codereview.chromium.org/131343011/diff/190001/src/core/SkTileGrid.h
> > > > File src/core/SkTileGrid.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/131343011/diff/190001/src/core/SkTileGrid.h#n...
> > > > src/core/SkTileGrid.h:64: virtual int getCount() const SK_OVERRIDE;
> > > > On 2014/01/29 18:41:16, reed1 wrote:
> > > > > Can we return -1 for getDepth, but non-zero for getCount?
> > > > 
> > > > Sure. If the structure's not a tree (like a tile grid), then you do
insert
> > > data,
> > > > which means getCount() is non 0, but it still returns -1 for getDepth(),
> > > because
> > > > it's not a tree.
> > > 
> > > Ah, ok. .. then I don't understand the use for getDepth. Can you augment
the
> > dox
> > > to explain why a caller would want to know the depth? More generally, for
> both
> > > getDepth and getCount, it would be great for the dox to relate these two
> > values
> > > to other APIs on the class.
> > 
> > Ok, no problem. So getCount() gives you the number of "data" (void* in this
> > case) inserted inside the BBoxHierarchy (regardless of what type of
> > BBoxHierarchy we have).
> > getDepth() is currently only used for tests, to check if the depth we get is
> > within the expected range. I doubt anyone would want to call getDepth(),
apart
> > from tests, since this is information about the underlying structure, which
is
> > probably not generally useful to anyone.
> 
> Thanks. Please add a version of that to the dox in the baseclass.

Done

[reed1, 2014-01-29 19:36:26]

interface lgtm -- deferring to others for the impl specifics.

[Justin Novosad, 2014-01-31 18:57:54]

lgtm with nits

https://codereview.chromium.org/131343011/diff/240001/src/core/SkQuadTree.cpp
File src/core/SkQuadTree.cpp (right):

https://codereview.chromium.org/131343011/diff/240001/src/core/SkQuadTree.cpp...
src/core/SkQuadTree.cpp:163: static const int QT_NODE_CAPACITY = 4;
Skia style is: enum { kFavoriteNumber = 7 };

https://codereview.chromium.org/131343011/diff/240001/src/core/SkQuadTreePict...
File src/core/SkQuadTreePicture.h (right):

https://codereview.chromium.org/131343011/diff/240001/src/core/SkQuadTreePict...
src/core/SkQuadTreePicture.h:21: class SkQuadTreePicture : public SkPicture {
Better put SK_API here, you'll need it.

[sugoi1, 2014-02-03 17:52:01]

The CQ bit was checked by sugoi@chromium.org

[commit-bot: I haz the power, 2014-02-03 17:52:07]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/sugoi@chromium.org/131343011/350001

[commit-bot: I haz the power, 2014-02-03 18:08:41]

Change committed as 13282

[commit-bot: I haz the power, 2014-02-03 18:08:42]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-03 18:09:55]

CQ bit was unchecked on CL. Ignoring.

[f(malita), 2014-02-03 18:33:55]

On 2014/02/03 18:08:41, I haz the power (commit-bot) wrote:
> Change committed as 13282

This broke the Mac 10.8 builds, PTAL:

http://108.170.219.164:10117/builders/Build-Mac10.8-Clang-x86-Debug/builds/29...

[10:23:40.180779]
/Users/chrome-bot/buildbot/skiabot-mac-10_8-compile-007/buildbot/third_party/chromium_buildbot/slave/Build-Mac10_8-Clang-x86-Debug/build/skia/samplecode/SamplePictFile.cpp:97:24:
error: comparison of constant 4 with expression of type 'PictFileView::BBoxType'
is always true [-Werror,-Wtautological-constant-out-of-range-compare]
[10:23:40.180885]         SkASSERT(fBBox < kBBoxTypeCount);
[10:23:40.181001]                  ~~~~~ ^ ~~~~~~~~~~~~~~
[10:23:40.181103] In file included from
/Users/chrome-bot/buildbot/skiabot-mac-10_8-compile-007/buildbot/third_party/chromium_buildbot/slave/Build-Mac10_8-Clang-x86-Debug/build/skia/samplecode/SamplePictFile.cpp:8:
[10:23:40.181202] In file included from ../samplecode/SampleCode.h:13:
[10:23:40.181300] In file included from ../include/core/SkColor.h:13:
[10:23:40.181398] In file included from ../include/core/SkScalar.h:11:
[10:23:40.181496] In file included from ../include/core/SkFixed.h:11:
[10:23:40.181594] ../include/core/SkTypes.h:96:55: note: expanded from macro
'SkASSERT'
[10:23:40.181691]     #define SkASSERT(cond)              SK_DEBUGBREAK(cond)
[10:23:40.181786]                                                       ^
[10:23:40.181884] In file included from
/Users/chrome-bot/buildbot/skiabot-mac-10_8-compile-007/buildbot/third_party/chromium_buildbot/slave/Build-Mac10_8-Clang-x86-Debug/build/skia/samplecode/SamplePictFile.cpp:8:
[10:23:40.181983] In file included from ../samplecode/SampleCode.h:13:
[10:23:40.182081] In file included from ../include/core/SkColor.h:13:
[10:23:40.182178] In file included from ../include/core/SkScalar.h:11:
[10:23:40.182275] In file included from ../include/core/SkFixed.h:11:
[10:23:40.182372] In file included from ../include/core/SkTypes.h:13:
[10:23:40.182472] ../include/core/SkPostConfig.h:172:44: note: expanded from
macro 'SK_DEBUGBREAK'
[10:23:40.182596] #      define SK_DEBUGBREAK(cond) do { if (cond) break; \
[10:23:40.182694]                                            ^
[10:23:40.182800] 1 error generated.

[sugoi1, 2014-02-03 18:36:59]

On 2014/02/03 18:33:55, Florin Malita wrote:
> On 2014/02/03 18:08:41, I haz the power (commit-bot) wrote:
> > Change committed as 13282
> 
> This broke the Mac 10.8 builds, PTAL:
> 
>
http://108.170.219.164:10117/builders/Build-Mac10.8-Clang-x86-Debug/builds/29...
> 
> [10:23:40.180779]
>
/Users/chrome-bot/buildbot/skiabot-mac-10_8-compile-007/buildbot/third_party/chromium_buildbot/slave/Build-Mac10_8-Clang-x86-Debug/build/skia/samplecode/SamplePictFile.cpp:97:24:
> error: comparison of constant 4 with expression of type
'PictFileView::BBoxType'
> is always true [-Werror,-Wtautological-constant-out-of-range-compare]
> [10:23:40.180885]         SkASSERT(fBBox < kBBoxTypeCount);
> [10:23:40.181001]                  ~~~~~ ^ ~~~~~~~~~~~~~~
> [10:23:40.181103] In file included from
>
/Users/chrome-bot/buildbot/skiabot-mac-10_8-compile-007/buildbot/third_party/chromium_buildbot/slave/Build-Mac10_8-Clang-x86-Debug/build/skia/samplecode/SamplePictFile.cpp:8:
> [10:23:40.181202] In file included from ../samplecode/SampleCode.h:13:
> [10:23:40.181300] In file included from ../include/core/SkColor.h:13:
> [10:23:40.181398] In file included from ../include/core/SkScalar.h:11:
> [10:23:40.181496] In file included from ../include/core/SkFixed.h:11:
> [10:23:40.181594] ../include/core/SkTypes.h:96:55: note: expanded from macro
> 'SkASSERT'
> [10:23:40.181691]     #define SkASSERT(cond)              SK_DEBUGBREAK(cond)
> [10:23:40.181786]                                                       ^
> [10:23:40.181884] In file included from
>
/Users/chrome-bot/buildbot/skiabot-mac-10_8-compile-007/buildbot/third_party/chromium_buildbot/slave/Build-Mac10_8-Clang-x86-Debug/build/skia/samplecode/SamplePictFile.cpp:8:
> [10:23:40.181983] In file included from ../samplecode/SampleCode.h:13:
> [10:23:40.182081] In file included from ../include/core/SkColor.h:13:
> [10:23:40.182178] In file included from ../include/core/SkScalar.h:11:
> [10:23:40.182275] In file included from ../include/core/SkFixed.h:11:
> [10:23:40.182372] In file included from ../include/core/SkTypes.h:13:
> [10:23:40.182472] ../include/core/SkPostConfig.h:172:44: note: expanded from
> macro 'SK_DEBUGBREAK'
> [10:23:40.182596] #      define SK_DEBUGBREAK(cond) do { if (cond) break; \
> [10:23:40.182694]                                            ^
> [10:23:40.182800] 1 error generated.

Yeah, I'm already looking at it. It's just the 'fBBox < kBBoxTypeCount'
condition that I need to change.

[sugoi1, 2014-02-03 18:48:05]

On 2014/02/03 18:36:59, sugoi1 wrote:
> On 2014/02/03 18:33:55, Florin Malita wrote:
> > On 2014/02/03 18:08:41, I haz the power (commit-bot) wrote:
> > > Change committed as 13282
> > 
> > This broke the Mac 10.8 builds, PTAL:
> > 
> >
>
http://108.170.219.164:10117/builders/Build-Mac10.8-Clang-x86-Debug/builds/29...
> > 
> > [10:23:40.180779]
> >
>
/Users/chrome-bot/buildbot/skiabot-mac-10_8-compile-007/buildbot/third_party/chromium_buildbot/slave/Build-Mac10_8-Clang-x86-Debug/build/skia/samplecode/SamplePictFile.cpp:97:24:
> > error: comparison of constant 4 with expression of type
> 'PictFileView::BBoxType'
> > is always true [-Werror,-Wtautological-constant-out-of-range-compare]
> > [10:23:40.180885]         SkASSERT(fBBox < kBBoxTypeCount);
> > [10:23:40.181001]                  ~~~~~ ^ ~~~~~~~~~~~~~~
> > [10:23:40.181103] In file included from
> >
>
/Users/chrome-bot/buildbot/skiabot-mac-10_8-compile-007/buildbot/third_party/chromium_buildbot/slave/Build-Mac10_8-Clang-x86-Debug/build/skia/samplecode/SamplePictFile.cpp:8:
> > [10:23:40.181202] In file included from ../samplecode/SampleCode.h:13:
> > [10:23:40.181300] In file included from ../include/core/SkColor.h:13:
> > [10:23:40.181398] In file included from ../include/core/SkScalar.h:11:
> > [10:23:40.181496] In file included from ../include/core/SkFixed.h:11:
> > [10:23:40.181594] ../include/core/SkTypes.h:96:55: note: expanded from macro
> > 'SkASSERT'
> > [10:23:40.181691]     #define SkASSERT(cond)             
SK_DEBUGBREAK(cond)
> > [10:23:40.181786]                                                       ^
> > [10:23:40.181884] In file included from
> >
>
/Users/chrome-bot/buildbot/skiabot-mac-10_8-compile-007/buildbot/third_party/chromium_buildbot/slave/Build-Mac10_8-Clang-x86-Debug/build/skia/samplecode/SamplePictFile.cpp:8:
> > [10:23:40.181983] In file included from ../samplecode/SampleCode.h:13:
> > [10:23:40.182081] In file included from ../include/core/SkColor.h:13:
> > [10:23:40.182178] In file included from ../include/core/SkScalar.h:11:
> > [10:23:40.182275] In file included from ../include/core/SkFixed.h:11:
> > [10:23:40.182372] In file included from ../include/core/SkTypes.h:13:
> > [10:23:40.182472] ../include/core/SkPostConfig.h:172:44: note: expanded from
> > macro 'SK_DEBUGBREAK'
> > [10:23:40.182596] #      define SK_DEBUGBREAK(cond) do { if (cond) break; \
> > [10:23:40.182694]                                            ^
> > [10:23:40.182800] 1 error generated.
> 
> Yeah, I'm already looking at it. It's just the 'fBBox < kBBoxTypeCount'
> condition that I need to change.

See https://codereview.chromium.org/149463009/

[tomhudson, 2014-02-24 16:48:37]

A bit of patch necromancy:

> The R-Tree BBH is still used by default

Skia uses RTree "by default" in SkPicture::createBBoxHierarchy(), but Chromium
is actually using TileGrids at runtime, right?

[sugoi, 2014-02-24 16:50:21]

On 2014/02/24 16:48:37, tomhudson wrote:
> A bit of patch necromancy:
> 
> > The R-Tree BBH is still used by default
> 
> Skia uses RTree "by default" in SkPicture::createBBoxHierarchy(), but Chromium
> is actually using TileGrids at runtime, right?

Right