Added SkDevice onAttachToCanvas & onDetachFromCanvas methods

Here is how officially handing the clip stack to the devices when they are attached would look. Note that I took the next steps of removing the clip stack from the prepareForDeviceDraw and gainFocus calls and moved the pixel lock\unlock into the two new methods.

[robertphillips, 2012-07-12 15:56:32]

http://codereview.appspot.com/6348100/diff/1/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

http://codereview.appspot.com/6348100/diff/1/src/core/SkCanvas.cpp#newcode80
src/core/SkCanvas.cpp:80: 
Pros: ensures the clip stack is passed to the device when a DeviceCM is
constructed (rather than the less reliable option of having an onAttachToCanvas
call after the ctor at all construction sites)

Cons: passing clip stack into ctor seems odd

[reed1, 2012-07-12 16:08:07]

1. do we still want a clipstack ptr in SkDraw?
2. can we add debugging, at least in SkDevice, to assert that onAttach has been
called (checking on other methods like gainFocus)
3. do we call onAttach for the root device (and onUnattach)?

http://codereview.appspot.com/6348100/diff/1/include/core/SkDevice.h
File include/core/SkDevice.h (right):

http://codereview.appspot.com/6348100/diff/1/include/core/SkDevice.h#newcode141
include/core/SkDevice.h:141: virtual void onAttachToCanvas(SkClipStack*
clipStack) {
this->methodName()

[reed1, 2012-07-12 16:08:37]

4. should we pass SkCanvas* to onAttach? (and not pass it on gainFocus)?

[Steve VanDeBogart, 2012-07-12 17:43:22]

http://codereview.appspot.com/6348100/diff/1/include/core/SkDevice.h
File include/core/SkDevice.h (right):

http://codereview.appspot.com/6348100/diff/1/include/core/SkDevice.h#newcode140
include/core/SkDevice.h:140: 
drive-by comment: A comment about when this pair of methods is called would be
helpful.

[robertphillips, 2012-07-12 18:09:04]

1) I left that in there b.c. SkPDFDevice seems to use it quite a bit (e.g., in
drawBitmap & drawSprite). SkPDFDevice should probably grab its own copy of the
clip stack in onAttach & then we could remove it from the draw. Thoughts?

2) I have added a check in gainFocus but don't know where else it should go

3) Yep - the only way a device can become the root is through the DeviceCM ctor
or the setDevice call. The only ways it can be removed from the device chain is
through setDevice or ~DeviceCM. Basically these are the only three places where
the DeviceCM's fDevice field is set and that forms the basis for when fTopLayer
can be altered.

4) Done - I think this makes the onAttach more credible instead of just a way to
get the clipStack to the device

http://codereview.appspot.com/6348100/diff/1/include/core/SkDevice.h
File include/core/SkDevice.h (right):

http://codereview.appspot.com/6348100/diff/1/include/core/SkDevice.h#newcode140
include/core/SkDevice.h:140: 
On 2012/07/12 17:43:22, Steve VanDeBogart wrote:
> drive-by comment: A comment about when this pair of methods is called would be
> helpful.

Done.

http://codereview.appspot.com/6348100/diff/1/include/core/SkDevice.h#newcode141
include/core/SkDevice.h:141: virtual void onAttachToCanvas(SkClipStack*
clipStack) {
On 2012/07/12 16:08:07, reed1 wrote:
> this->methodName()

Done.

http://codereview.appspot.com/6348100/diff/1/include/utils/SkDeferredCanvas.h
File include/utils/SkDeferredCanvas.h (right):

http://codereview.appspot.com/6348100/diff/1/include/utils/SkDeferredCanvas.h...
include/utils/SkDeferredCanvas.h:223: {}
Options here were to delete this or make it call the base class' gainFocus.

[reed1, 2012-07-12 19:36:33]

PDF dude, what to you think about removing clipstack from SkDraw?

http://codereview.appspot.com/6348100/diff/4002/src/core/SkDevice.cpp
File src/core/SkDevice.cpp (left):

http://codereview.appspot.com/6348100/diff/4002/src/core/SkDevice.cpp#oldcode22
src/core/SkDevice.cpp:22: 
Can we add SkASSERT(fAttachedToCanvas) in some places.

e.g. in gainFocus?

http://codereview.appspot.com/6348100/diff/4002/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

http://codereview.appspot.com/6348100/diff/4002/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:350: 
// Canvas promises that this ptr is valid until onDetachFromCanvas is called

[robertphillips, 2012-07-12 19:42:17]

http://codereview.appspot.com/6348100/diff/4002/src/core/SkDevice.cpp
File src/core/SkDevice.cpp (left):

http://codereview.appspot.com/6348100/diff/4002/src/core/SkDevice.cpp#oldcode22
src/core/SkDevice.cpp:22: 
I already have it in gainFocus (in SkDevice.h) but did not know of any other
suitable places.

http://codereview.appspot.com/6348100/diff/4002/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

http://codereview.appspot.com/6348100/diff/4002/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:350: 
On 2012/07/12 19:36:33, reed1 wrote:
> // Canvas promises that this ptr is valid until onDetachFromCanvas is called

Done.

[Steve VanDeBogart, 2012-07-12 19:50:23]

On 2012/07/12 19:36:33, reed1 wrote:
> PDF dude, what to you think about removing clipstack from SkDraw?

If a device is always attached to a canvas during a draw operation, then I have
no preference between getting the clipstack from the SkDraw or the attached
canvas.

[reed1, 2012-07-12 19:50:37]

lgtm w/ question in SkDeferredCanvas.h

http://codereview.appspot.com/6348100/diff/9001/include/utils/SkDeferredCanvas.h
File include/utils/SkDeferredCanvas.h (left):

http://codereview.appspot.com/6348100/diff/9001/include/utils/SkDeferredCanva...
include/utils/SkDeferredCanvas.h:223: {}
Why is this deleted instead of just changing the parameters?

[reed1, 2012-07-12 19:51:45]

Robert,

Is updating SkPDFDevice workable in this CL, or should that be left for Steve in
another (to remove clipstack from SkDraw)?

[robertphillips, 2012-07-12 19:54:30]

http://codereview.appspot.com/6348100/diff/9001/include/utils/SkDeferredCanvas.h
File include/utils/SkDeferredCanvas.h (left):

http://codereview.appspot.com/6348100/diff/9001/include/utils/SkDeferredCanva...
include/utils/SkDeferredCanvas.h:223: {}
The options were delete it or make it call the base class (so the check for
attachment status occurs in debug). Would you prefer it here w/ the call to the
base?

[robertphillips, 2012-07-12 20:02:15]

http://codereview.appspot.com/6348100/diff/9001/include/utils/SkDeferredCanvas.h
File include/utils/SkDeferredCanvas.h (left):

http://codereview.appspot.com/6348100/diff/9001/include/utils/SkDeferredCanva...
include/utils/SkDeferredCanvas.h:223: {}
Justin - can you provide any insight into why gainFocus' call to the base class
was being suppressed here|

[robertphillips, 2012-07-12 20:23:57]

On 2012/07/12 19:51:45, reed1 wrote:
> Robert,
> 
> Is updating SkPDFDevice workable in this CL, or should that be left for Steve
in
> another (to remove clipstack from SkDraw)?

I looked into and it gets a bit hairy in the ContentEntry setup (it is actually
synthesizing a new SkClipStack) so I think Steve might be better suited to this
- and certainly in a different CL. One way to proceed is I could do the plumbing
to get the clipstack pointer into the SkPDFDevice and then Steve could take over
from there?

[Steve VanDeBogart, 2012-07-12 20:28:02]

On 2012/07/12 20:23:57, robertphillips wrote:
> On 2012/07/12 19:51:45, reed1 wrote:
> > Robert,
> > 
> > Is updating SkPDFDevice workable in this CL, or should that be left for
Steve
> in
> > another (to remove clipstack from SkDraw)?
> 
> I looked into and it gets a bit hairy in the ContentEntry setup (it is
actually
> synthesizing a new SkClipStack) so I think Steve might be better suited to
this
> - and certainly in a different CL. One way to proceed is I could do the
plumbing
> to get the clipstack pointer into the SkPDFDevice and then Steve could take
over
> from there?

Don't let that part scare you off.  The clip stack is technically optional in
the SkDraw.  When it's absent, I try to make one up as best I can.  I think we
can always get the clip stack from the canvas, so that that chunk of code just
goes away.

[reed1, 2012-07-12 20:44:12]

Lets share in the fun!

CL progression:

1. roberphilips -- add a field to SkPDFDevice to record the clipstack in
onAttach
2. vandebo -- uses that stached ptr where he wants to, and ignores the field in
SkDraw
3. reed -- remove the field in SkDraw

[junov1, 2012-07-12 20:53:01]

lgtm for SkDeferredCanvas

http://codereview.appspot.com/6348100/diff/9001/include/utils/SkDeferredCanvas.h
File include/utils/SkDeferredCanvas.h (left):

http://codereview.appspot.com/6348100/diff/9001/include/utils/SkDeferredCanva...
include/utils/SkDeferredCanvas.h:223: {}
On 2012/07/12 20:02:15, robertphillips wrote:
> Justin - can you provide any insight into why gainFocus' call to the base
class
> was being suppressed here|

Hmmm...  I think I misread it as pure virtual in SkDevice when I wrote this.  No
harm in getting rid of it since the base class implementation is a no-op
anyways.

[robertphillips, 2012-07-12 20:56:01]

Here are the SkPDFDevice changes.

http://codereview.appspot.com/6348100/diff/14002/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (left):

http://codereview.appspot.com/6348100/diff/14002/include/pdf/SkPDFDevice.h#ol...
include/pdf/SkPDFDevice.h:260: 
This class derives from Noncopyable

[Steve VanDeBogart, 2012-07-12 20:58:06]

On 2012/07/12 20:56:01, robertphillips wrote:
> Here are the SkPDFDevice changes.
> 
> http://codereview.appspot.com/6348100/diff/14002/include/pdf/SkPDFDevice.h
> File include/pdf/SkPDFDevice.h (left):
> 
>
http://codereview.appspot.com/6348100/diff/14002/include/pdf/SkPDFDevice.h#ol...
> include/pdf/SkPDFDevice.h:260: 
> This class derives from Noncopyable

LGTM

[robertphillips, 2012-07-13 15:36:45]

committed as r4598