Move SkRecordComputeLayers and CollectLayers into SkRecordDraw.cpp

Move SkRecordComputeLayers and CollectLayers into SkRecordDraw.cpp

Rather then exposing parts of FillBounds (as in Expose FillBounds to allow GrPictureUtils::CollectLayers to be layered on top of it - https://codereview.chromium.org/698643002/), this CL moves CollectLayers into SkRecordDraw.cpp to accomplish the layering.

Committed: https://skia.googlesource.com/skia/+/4e8e3421aa919a82eb1dd287fecbd079f5a320b4

[robertphillips, 2014-11-11 20:59:08]

robertphillips@google.com changed reviewers:
+ mtklein@google.com

[robertphillips, 2014-11-11 21:00:44]

robertphillips@google.com changed reviewers:
+ robertphillips@google.com

[mtklein, 2014-11-11 21:17:30]

Nits only, though I'd like to get a handle on what's up with the test change
before we land this.

https://codereview.chromium.org/716913003/diff/1/src/core/SkRecordDraw.cpp
File src/core/SkRecordDraw.cpp (right):

https://codereview.chromium.org/716913003/diff/1/src/core/SkRecordDraw.cpp#ne...
src/core/SkRecordDraw.cpp:149: FillBounds(const SkRect& cullRect, const
SkRecord& record, SkBBoxHierarchy* bbh)
If we can get away with it, let's just pass bbh as an argument to cleanUp()s
instead of threading it through the ctors.

https://codereview.chromium.org/716913003/diff/1/src/core/SkRecordDraw.cpp#ne...
src/core/SkRecordDraw.cpp:555: const unsigned int fNumRecords;
We usually just write unsigned.

https://codereview.chromium.org/716913003/diff/1/src/core/SkRecordDraw.cpp#ne...
src/core/SkRecordDraw.cpp:592: fFillBounds.cleanUp();
Does the ordering of cleanUp() and the popSaveLayerInfo() loop matter here? 
Might be nice to note it, and if it does a little bit about why.

https://codereview.chromium.org/716913003/diff/1/src/core/SkRecordDraw.cpp#ne...
src/core/SkRecordDraw.cpp:605: class SaveLayerInfo {
some might rewrite "class ... { public: " -> "struct ... { "   :)

https://codereview.chromium.org/716913003/diff/1/src/core/SkRecordDraw.cpp#ne...
src/core/SkRecordDraw.cpp:633: SkPicture::AccelData::Key key =
GrAccelData::ComputeAccelDataKey();
Just out of curiosity, how much of GrAccelData is really Gr-only?  Is it
possible we can rejuggle the #ifdef guards a little and make this facility
generally available, even if only used by Ganesh?  It'd be nice to be able to
build and test this stuff on any machine.

https://codereview.chromium.org/716913003/diff/1/tests/PictureTest.cpp
File tests/PictureTest.cpp (right):

https://codereview.chromium.org/716913003/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:1021: REPORTER_ASSERT(reporter, kWidth/2.0 ==
info1.fBounds.width() &&
What happened here?

[robertphillips, 2014-11-12 13:46:26]

https://codereview.chromium.org/716913003/diff/1/src/core/SkRecordDraw.cpp
File src/core/SkRecordDraw.cpp (right):

https://codereview.chromium.org/716913003/diff/1/src/core/SkRecordDraw.cpp#ne...
src/core/SkRecordDraw.cpp:149: FillBounds(const SkRect& cullRect, const
SkRecord& record, SkBBoxHierarchy* bbh)
On 2014/11/11 21:17:30, mtklein wrote:
> If we can get away with it, let's just pass bbh as an argument to cleanUp()s
> instead of threading it through the ctors.

Done.

https://codereview.chromium.org/716913003/diff/1/src/core/SkRecordDraw.cpp#ne...
src/core/SkRecordDraw.cpp:555: const unsigned int fNumRecords;
On 2014/11/11 21:17:30, mtklein wrote:
> We usually just write unsigned.

Done.

https://codereview.chromium.org/716913003/diff/1/src/core/SkRecordDraw.cpp#ne...
src/core/SkRecordDraw.cpp:592: fFillBounds.cleanUp();
On 2014/11/11 21:17:30, mtklein wrote:
> Does the ordering of cleanUp() and the popSaveLayerInfo() loop matter here? 
> Might be nice to note it, and if it does a little bit about why.

Done.

https://codereview.chromium.org/716913003/diff/1/src/core/SkRecordDraw.cpp#ne...
src/core/SkRecordDraw.cpp:605: class SaveLayerInfo {
On 2014/11/11 21:17:30, mtklein wrote:
> some might rewrite "class ... { public: " -> "struct ... { "   :)

Done.

https://codereview.chromium.org/716913003/diff/1/src/core/SkRecordDraw.cpp#ne...
src/core/SkRecordDraw.cpp:633: SkPicture::AccelData::Key key =
GrAccelData::ComputeAccelDataKey();
On 2014/11/11 21:17:30, mtklein wrote:
> Just out of curiosity, how much of GrAccelData is really Gr-only?  Is it
> possible we can rejuggle the #ifdef guards a little and make this facility
> generally available, even if only used by Ganesh?  It'd be nice to be able to
> build and test this stuff on any machine.

I think that's a good idea but would prefer to delay it to a later CL.

https://codereview.chromium.org/716913003/diff/1/tests/PictureTest.cpp
File tests/PictureTest.cpp (right):

https://codereview.chromium.org/716913003/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:1021: REPORTER_ASSERT(reporter, kWidth/2.0 ==
info1.fBounds.width() &&
On 2014/11/11 21:17:30, mtklein wrote:
> What happened here?

The CollectLayers code was based on a version of FillBounds prior to (Shrink
saveLayer device bounds when it supplies an explicit bounds and has a complex
paint - https://codereview.chromium.org/696763002/) so this CL also changes how
the bounding boxes are computed in the CollectLayers case. Note that this case
here is exactly the one mentioned in the shrinking patch's description so this
is an expected result of this CL.

[mtklein, 2014-11-12 14:18:37]

lgtm

[robertphillips, 2014-11-12 14:33:19]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-11-12 14:34:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/716913003/20001

[commit-bot: I haz the power, 2014-11-12 14:34:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-12 14:34:23]

Presubmit check for 716913003-20001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Since the CL is editing public API, you must have an LGTM from one of:
('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org',
'bsalomon@google.com', 'djsollen@chromium.org', 'djsollen@google.com')

Presubmit checks took 1.7s to calculate.

[robertphillips, 2014-11-12 14:35:29]

robertphillips@google.com changed reviewers:
+ bsalomon@google.com

[bsalomon, 2014-11-12 14:37:47]

On 2014/11/12 14:35:38, robertphillips wrote:

lgtm

[robertphillips, 2014-11-12 14:37:59]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-11-12 14:38:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/716913003/20001

[commit-bot: I haz the power, 2014-11-12 14:46:12]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/4e8e3421aa919a82eb1dd287fecbd079f5a320b4