fix state machine so we know simple only loops once, but we can call maprect in debug mode

fix state machine so we know simple only loops once, but we can call maprect in debug mode


Revert "Revert of r10943."

This reverts commit 9e83074cce521d3cc3b8b3a9b819a612a07d800a.

BUG=
R=tomhudson@google.com

Committed: https://code.google.com/p/skia/source/detail?r=10981

[tomhudson, 2013-08-28 14:27:50]

https://codereview.chromium.org/23618005/diff/3001/src/core/SkDeviceLooper.cpp
File src/core/SkDeviceLooper.cpp (right):

https://codereview.chromium.org/23618005/diff/3001/src/core/SkDeviceLooper.cp...
src/core/SkDeviceLooper.cpp:73: if (kPreSimple_State == fState) {
Weren't these next 3 lines already done in the constructor?
Why not go back to simple/complex/done states, make next(){simple} be a NOOP?

Ah, I suppose you can't do while(looper.next()) then AND retain your !done
asserts for the complex cases. Sigh.

https://codereview.chromium.org/23618005/diff/3001/src/core/SkDeviceLooper.h
File src/core/SkDeviceLooper.h (right):

https://codereview.chromium.org/23618005/diff/3001/src/core/SkDeviceLooper.h#...
src/core/SkDeviceLooper.h:16: *  Helper class to manage "tiling" are large
coordinate space into managable
Nit: are?

https://codereview.chromium.org/23618005/diff/3001/src/core/SkDeviceLooper.h#...
src/core/SkDeviceLooper.h:25: *  needs (at the moment, that is 4X, so the effect
"safe" range is +- 4K).
Nit: effect -> effective
Or just drop that word, and the quotes around safe.

https://codereview.chromium.org/23618005/diff/3001/src/core/SkDeviceLooper.h#...
src/core/SkDeviceLooper.h:50: bool next();
Even after reading the code I don't know what the return value of this function
means or how I should use it.

[reed1, 2013-08-28 14:38:33]

I will try a version that doesn't need pre/post...

https://codereview.chromium.org/23618005/diff/3001/src/core/SkDeviceLooper.cpp
File src/core/SkDeviceLooper.cpp (right):

https://codereview.chromium.org/23618005/diff/3001/src/core/SkDeviceLooper.cp...
src/core/SkDeviceLooper.cpp:73: if (kPreSimple_State == fState) {
On 2013/08/28 14:27:50, tomhudson wrote:
> Weren't these next 3 lines already done in the constructor?
> Why not go back to simple/complex/done states, make next(){simple} be a NOOP?
> 
> Ah, I suppose you can't do while(looper.next()) then AND retain your !done
> asserts for the complex cases. Sigh.

Right, given this iterator pattern (while (next()) do). We could use a diff
iterator pattern if need be.

https://codereview.chromium.org/23618005/diff/3001/src/core/SkDeviceLooper.h
File src/core/SkDeviceLooper.h (right):

https://codereview.chromium.org/23618005/diff/3001/src/core/SkDeviceLooper.h#...
src/core/SkDeviceLooper.h:16: *  Helper class to manage "tiling" are large
coordinate space into managable
On 2013/08/28 14:27:50, tomhudson wrote:
> Nit: are?

Done.

https://codereview.chromium.org/23618005/diff/3001/src/core/SkDeviceLooper.h#...
src/core/SkDeviceLooper.h:25: *  needs (at the moment, that is 4X, so the effect
"safe" range is +- 4K).
On 2013/08/28 14:27:50, tomhudson wrote:
> Nit: effect -> effective
> Or just drop that word, and the quotes around safe.

Done.

https://codereview.chromium.org/23618005/diff/3001/src/core/SkDeviceLooper.h#...
src/core/SkDeviceLooper.h:50: bool next();
On 2013/08/28 14:27:50, tomhudson wrote:
> Even after reading the code I don't know what the return value of this
function
> means or how I should use it.

Same as all of skia's interators that just use a next() method. But I definitely
need to document it.

[tomhudson, 2013-08-28 15:01:21]

lgtm

Thanks for improved documentation. Not worth further bikeshedding; I'd be happy
to use or maintain this code.

https://codereview.chromium.org/23618005/diff/11001/src/core/SkDeviceLooper.cpp
File src/core/SkDeviceLooper.cpp (right):

https://codereview.chromium.org/23618005/diff/11001/src/core/SkDeviceLooper.c...
src/core/SkDeviceLooper.cpp:17: // sentinels that next() has not yet bee called,
and so our mapper functions
Nit: bee -> been

https://codereview.chromium.org/23618005/diff/11001/src/core/SkDeviceLooper.h
File src/core/SkDeviceLooper.h (right):

https://codereview.chromium.org/23618005/diff/11001/src/core/SkDeviceLooper.h...
src/core/SkDeviceLooper.h:38: SkASSERT(kDone_State != fState);
SkASSERT(NULL != fCurrBitmap)?

https://codereview.chromium.org/23618005/diff/11001/src/core/SkDeviceLooper.h...
src/core/SkDeviceLooper.h:43: SkASSERT(kDone_State != fState);
SkASSERT(NULL != fCurrRC)?

[reed1, 2013-08-28 15:11:30]

https://codereview.chromium.org/23618005/diff/11001/src/core/SkDeviceLooper.cpp
File src/core/SkDeviceLooper.cpp (right):

https://codereview.chromium.org/23618005/diff/11001/src/core/SkDeviceLooper.c...
src/core/SkDeviceLooper.cpp:17: // sentinels that next() has not yet bee called,
and so our mapper functions
On 2013/08/28 15:01:21, tomhudson wrote:
> Nit: bee -> been

Done.

https://codereview.chromium.org/23618005/diff/11001/src/core/SkDeviceLooper.h
File src/core/SkDeviceLooper.h (right):

https://codereview.chromium.org/23618005/diff/11001/src/core/SkDeviceLooper.h...
src/core/SkDeviceLooper.h:38: SkASSERT(kDone_State != fState);
On 2013/08/28 15:01:21, tomhudson wrote:
> SkASSERT(NULL != fCurrBitmap)?

Done.

https://codereview.chromium.org/23618005/diff/11001/src/core/SkDeviceLooper.h...
src/core/SkDeviceLooper.h:43: SkASSERT(kDone_State != fState);
On 2013/08/28 15:01:21, tomhudson wrote:
> SkASSERT(NULL != fCurrRC)?

Done.

[reed1, 2013-08-28 15:11:51]

this has been a valuable review, thanks.

[reed1, 2013-08-28 15:23:26]

Committed patchset #5 manually as r10981 (presubmit successful).