Refactoring into a public facing facing SkDebugger class first pass.

Refactoring into a public facing facing SkDebugger class first pass.

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

[DerekS, 2012-08-06 17:40:48]

http://codereview.appspot.com/6450096/diff/2001/debugger/SkDebugger.h
File debugger/SkDebugger.h (right):

http://codereview.appspot.com/6450096/diff/2001/debugger/SkDebugger.h#newcode26
debugger/SkDebugger.h:26: void draw(SkCanvas* canvas) {
is the plan to keep this around?

http://codereview.appspot.com/6450096/diff/2001/debugger/SkDebugger.h#newcode32
debugger/SkDebugger.h:32: void breakAt(int index);
seem like you'll need a clear() method to remove breakpoints

http://codereview.appspot.com/6450096/diff/2001/debugger/SkDebugger.h#newcode37
debugger/SkDebugger.h:37: void setTargetCanvas();
shouldn't this take a canvas?

http://codereview.appspot.com/6450096/diff/2001/debugger/SkDebugger.h#newcode48
debugger/SkDebugger.h:48: return fDebugCanvas->getDrawCommandsAsStrings();
doesn't need to be in this CL, but let's not use std::vector in our public API

http://codereview.appspot.com/6450096/diff/2001/debugger/debuggermain.cpp
File debugger/debuggermain.cpp (right):

http://codereview.appspot.com/6450096/diff/2001/debugger/debuggermain.cpp#new...
debugger/debuggermain.cpp:7: */
not sure what changed in this file.

[chudy, 2012-08-06 18:50:24]

Addressed patch 1 comments

http://codereview.appspot.com/6450096/diff/2001/debugger/SkDebugger.h
File debugger/SkDebugger.h (right):

http://codereview.appspot.com/6450096/diff/2001/debugger/SkDebugger.h#newcode26
debugger/SkDebugger.h:26: void draw(SkCanvas* canvas) {
On 2012/08/06 17:40:48, DerekS wrote:
> is the plan to keep this around?

I believe so? We talked about setting a target canvas but I do not see a benefit
in setting and unsetting a target, especially when you are drawing to more than
1 target at a time.

http://codereview.appspot.com/6450096/diff/2001/debugger/SkDebugger.h#newcode32
debugger/SkDebugger.h:32: void breakAt(int index);
On 2012/08/06 17:40:48, DerekS wrote:
> seem like you'll need a clear() method to remove breakpoints

I haven't decided whether to store the array of breakpoints here like we talked
about, or simply call setIndex and draw on the next breakpoint we want like we
talked about on friday. For now I have removed this method stub.

http://codereview.appspot.com/6450096/diff/2001/debugger/SkDebugger.h#newcode37
debugger/SkDebugger.h:37: void setTargetCanvas();
On 2012/08/06 17:40:48, DerekS wrote:
> shouldn't this take a canvas?

Will be removing this function.

http://codereview.appspot.com/6450096/diff/2001/debugger/SkDebugger.h#newcode48
debugger/SkDebugger.h:48: return fDebugCanvas->getDrawCommandsAsStrings();
On 2012/08/06 17:40:48, DerekS wrote:
> doesn't need to be in this CL, but let's not use std::vector in our public API

Added a todo.

http://codereview.appspot.com/6450096/diff/2001/debugger/debuggermain.cpp
File debugger/debuggermain.cpp (right):

http://codereview.appspot.com/6450096/diff/2001/debugger/debuggermain.cpp#new...
debugger/debuggermain.cpp:7: */
On 2012/08/06 17:40:48, DerekS wrote:
> not sure what changed in this file.

This file doesn't exist outside of this CL. Not sure why it isn't all green.

[DerekS, 2012-08-07 13:43:26]

do you want me to review the CL again or are you still making changes?

[chudy, 2012-08-07 13:45:06]

I'm making changes in a seperate CL on top of this one

On Tue, Aug 7, 2012 at 9:43 AM,  <djsollen@google.com> wrote:
>
> do you want me to review the CL again or are you still making changes?
>
> http://codereview.appspot.com/6450096/



-- 
Karol Chudy
Phone: (203) 898-4233
chudy@google.com

[DerekS, 2012-08-07 15:28:42]

lgtm with one nit

http://codereview.appspot.com/6450096/diff/9001/debugger/SkDebugger.cpp
File debugger/SkDebugger.cpp (right):

http://codereview.appspot.com/6450096/diff/9001/debugger/SkDebugger.cpp#newco...
debugger/SkDebugger.cpp:27: SkSafeUnref(fPicture);
if you follow my comment below you won't need this as the wrapper will do it for
you.

http://codereview.appspot.com/6450096/diff/9001/debugger/SkDebugger.cpp#newco...
debugger/SkDebugger.cpp:36: fPicture->ref();
to avoid this we can define fPicture as follows which will take care of the ref
counting

SkRefCnt_SafeAssign(...)

[chudy, 2012-08-07 16:13:32]

Comment fixed. Committed as revision 4986. Thanks for the review

http://codereview.appspot.com/6450096/diff/9001/debugger/SkDebugger.cpp
File debugger/SkDebugger.cpp (right):

http://codereview.appspot.com/6450096/diff/9001/debugger/SkDebugger.cpp#newco...
debugger/SkDebugger.cpp:27: SkSafeUnref(fPicture);
On 2012/08/07 15:28:42, DerekS wrote:
> if you follow my comment below you won't need this as the wrapper will do it
for
> you.

Done.

http://codereview.appspot.com/6450096/diff/9001/debugger/SkDebugger.cpp#newco...
debugger/SkDebugger.cpp:36: fPicture->ref();
On 2012/08/07 15:28:42, DerekS wrote:
> to avoid this we can define fPicture as follows which will take care of the
ref
> counting
> 
> SkRefCnt_SafeAssign(...)

Done.