clt debugger

clt debugger
Committed: https://code.google.com/p/skia/source/detail?r=4404

[chudy, 2012-06-01 18:08:27]

First CLT

[chudy, 2012-06-01 19:26:34]

Updated and got rid of first patch set. Lots of useless includes and comments
removed + junk code. Also switched over to vectors from maps

[DerekS, 2012-06-01 20:14:29]

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDebugCanvas.cpp
File debugger/SkDebugCanvas.cpp (right):

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDebugCanvas.cpp#n...
debugger/SkDebugCanvas.cpp:3: * Copyright 2012 The Android Open Source Project
use the bsd license in these files instead of the android one.

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDebugCanvas.h
File debugger/SkDebugCanvas.h (right):

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDebugCanvas.h#new...
debugger/SkDebugCanvas.h:19: std::vector<SkDrawCommand*> commandVector;
move these to the bottom of the file and into a private section.

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDebugCanvas.h#new...
debugger/SkDebugCanvas.h:40: SkDebugCanvas() {};
our convention is that the constructor should be the first method in the public
section of the class

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDebugger.cpp
File debugger/SkDebugger.cpp (right):

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDebugger.cpp#newc...
debugger/SkDebugger.cpp:28: canvas->drawData(text, sizeof(text));
be careful about using sizeof as it measures the size of the data structure so
instead you want to use strlen

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDebugger.cpp#newc...
debugger/SkDebugger.cpp:32: * canvas->drawText("Picture", 8, SkIntToScalar(50),
SkIntToScalar(62), paint);
instead of 8 do the following...

const char* text = "Picture";
canvas->drawText(text, strlen(text),...

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDebugger.h
File debugger/SkDebugger.h (right):

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDebugger.h#newcode19
debugger/SkDebugger.h:19: SkDebugger() {}
constructor and destructor always come first.

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDebugger.h#newcode21
debugger/SkDebugger.h:21: virtual ~SkDebugger() {}
if no one is going to override this class then you don't need to make this
virtual.

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDrawCommand.cpp
File debugger/SkDrawCommand.cpp (right):

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDrawCommand.cpp#n...
debugger/SkDrawCommand.cpp:20: /** NOTE(chudy): Should never be called. */
if is is never to be called put SkDEBUGFAIL("error msg") as it will cause a
runtime assert to be fired

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDrawCommand.h
File debugger/SkDrawCommand.h (right):

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDrawCommand.h#new...
debugger/SkDrawCommand.h:21: DrawType drawtype;
the convention would be to make this variable protected. Additionally, member
variables are always done in camelCase with the letter 'f' appended to the
beginning...

DrawType fDrawType;

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDrawCommand.h#new...
debugger/SkDrawCommand.h:28: class Unused : public SkDrawCommand {
what is the unused command for?

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDrawCommand.h#new...
debugger/SkDrawCommand.h:44: SkColor color;
use the naming convention mentioned above and if you want the fields to be
public they should be listed after the methods.

https://codereview.appspot.com/6267043/diff/2001/gyp/debugger.gyp
File gyp/debugger.gyp (right):

https://codereview.appspot.com/6267043/diff/2001/gyp/debugger.gyp#newcode21
gyp/debugger.gyp:21: 'core.gyp:core',
you should only need the core.gyp dependency for now

https://codereview.appspot.com/6267043/diff/2001/gyp/debugger.gyp#newcode37
gyp/debugger.gyp:37: 'SubSystem': '2',
I don't think you need this setting

https://codereview.appspot.com/6267043/diff/2001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.appspot.com/6267043/diff/2001/include/core/SkPicture.h#new...
include/core/SkPicture.h:28: SkPictureRecord* fRecord;
revert this file back to it's original state.

https://codereview.appspot.com/6267043/diff/2001/src/core/SkPicturePlayback.h
File src/core/SkPicturePlayback.h (right):

https://codereview.appspot.com/6267043/diff/2001/src/core/SkPicturePlayback.h...
src/core/SkPicturePlayback.h:40: void getDrawCommands();
remove this

[chudy, 2012-06-01 21:24:14]

Fixed and applied comments throughout files

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDebugCanvas.cpp
File debugger/SkDebugCanvas.cpp (right):

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDebugCanvas.cpp#n...
debugger/SkDebugCanvas.cpp:3: * Copyright 2012 The Android Open Source Project
On 2012/06/01 20:14:29, DerekS wrote:
> use the bsd license in these files instead of the android one.

I went and copied a different comment from other files in the src directory,
this one says C 2012 Google. I hope this is what you mean since the android
licensing still mentions the BSD-style license

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDebugCanvas.h
File debugger/SkDebugCanvas.h (right):

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDebugCanvas.h#new...
debugger/SkDebugCanvas.h:19: std::vector<SkDrawCommand*> commandVector;
On 2012/06/01 20:14:29, DerekS wrote:
> move these to the bottom of the file and into a private section.

Done.

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDebugCanvas.h#new...
debugger/SkDebugCanvas.h:40: SkDebugCanvas() {};
On 2012/06/01 20:14:29, DerekS wrote:
> our convention is that the constructor should be the first method in the
public
> section of the class

Done.

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDebugger.cpp
File debugger/SkDebugger.cpp (right):

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDebugger.cpp#newc...
debugger/SkDebugger.cpp:28: canvas->drawData(text, sizeof(text));
On 2012/06/01 20:14:29, DerekS wrote:
> be careful about using sizeof as it measures the size of the data structure so
> instead you want to use strlen

Done.

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDebugger.cpp#newc...
debugger/SkDebugger.cpp:32: * canvas->drawText("Picture", 8, SkIntToScalar(50),
SkIntToScalar(62), paint);
On 2012/06/01 20:14:29, DerekS wrote:
> instead of 8 do the following...
> 
> const char* text = "Picture";
> canvas->drawText(text, strlen(text),...

Done.

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDebugger.h
File debugger/SkDebugger.h (right):

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDebugger.h#newcode19
debugger/SkDebugger.h:19: SkDebugger() {}
On 2012/06/01 20:14:29, DerekS wrote:
> constructor and destructor always come first.

Done.

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDebugger.h#newcode21
debugger/SkDebugger.h:21: virtual ~SkDebugger() {}
On 2012/06/01 20:14:29, DerekS wrote:
> if no one is going to override this class then you don't need to make this
> virtual.

Done.

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDrawCommand.cpp
File debugger/SkDrawCommand.cpp (right):

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDrawCommand.cpp#n...
debugger/SkDrawCommand.cpp:20: /** NOTE(chudy): Should never be called. */
On 2012/06/01 20:14:29, DerekS wrote:
> if is is never to be called put SkDEBUGFAIL("error msg") as it will cause a
> runtime assert to be fired

Done.

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDrawCommand.h
File debugger/SkDrawCommand.h (right):

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDrawCommand.h#new...
debugger/SkDrawCommand.h:21: DrawType drawtype;
On 2012/06/01 20:14:29, DerekS wrote:
> the convention would be to make this variable protected. Additionally, member
> variables are always done in camelCase with the letter 'f' appended to the
> beginning...
> 
> DrawType fDrawType;

Done.

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDrawCommand.h#new...
debugger/SkDrawCommand.h:28: class Unused : public SkDrawCommand {
On 2012/06/01 20:14:29, DerekS wrote:
> what is the unused command for?

It looks like its a enumerated type inside of SkPictureFlat.h along with all the
other types. Since SkPicture also seems to have a switch for the UNUSED drawType
I figured it'd be better safe than sorry to also include it. Should we get rid
of it?

https://codereview.appspot.com/6267043/diff/2001/debugger/SkDrawCommand.h#new...
debugger/SkDrawCommand.h:44: SkColor color;
On 2012/06/01 20:14:29, DerekS wrote:
> use the naming convention mentioned above and if you want the fields to be
> public they should be listed after the methods.

Done.

https://codereview.appspot.com/6267043/diff/2001/gyp/debugger.gyp
File gyp/debugger.gyp (right):

https://codereview.appspot.com/6267043/diff/2001/gyp/debugger.gyp#newcode21
gyp/debugger.gyp:21: 'core.gyp:core',
On 2012/06/01 20:14:29, DerekS wrote:
> you should only need the core.gyp dependency for now

Had to keep images (5 errors with exclusion) and ports (>100 errors with
exclusion). Got rid of rest.

https://codereview.appspot.com/6267043/diff/2001/gyp/debugger.gyp#newcode37
gyp/debugger.gyp:37: 'SubSystem': '2',
On 2012/06/01 20:14:29, DerekS wrote:
> I don't think you need this setting

Done.

https://codereview.appspot.com/6267043/diff/2001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.appspot.com/6267043/diff/2001/include/core/SkPicture.h#new...
include/core/SkPicture.h:28: SkPictureRecord* fRecord;
On 2012/06/01 20:14:29, DerekS wrote:
> revert this file back to it's original state.

Done.

https://codereview.appspot.com/6267043/diff/2001/src/core/SkPicturePlayback.h
File src/core/SkPicturePlayback.h (right):

https://codereview.appspot.com/6267043/diff/2001/src/core/SkPicturePlayback.h...
src/core/SkPicturePlayback.h:40: void getDrawCommands();
On 2012/06/01 20:14:29, DerekS wrote:
> remove this

Done.

[DerekS, 2012-06-04 13:56:50]

looks good for now, let's focus on the qt progress now so we can see the
results.

https://codereview.appspot.com/6267043/diff/5003/debugger/SkDebugCanvas.cpp
File debugger/SkDebugCanvas.cpp (right):

https://codereview.appspot.com/6267043/diff/5003/debugger/SkDebugCanvas.cpp#n...
debugger/SkDebugCanvas.cpp:14: return this->INHERITED::save(flags);
4 spaces for indentations.

[chudy, 2012-06-18 18:22:42]

Updated. Still some renaming to do in the ui_SkDebuggerUI.h file but I skimmed
through and did some refactoring in the rest of the files

[DerekS, 2012-06-18 20:01:18]

here is my initial set of comments.  I haven't made it through all the files yet
but this should give you a good idea of some of the changes that will be needed.

https://codereview.appspot.com/6267043/diff/23003/debugger/QT/SkQtWidget.h
File debugger/QT/SkQtWidget.h (right):

https://codereview.appspot.com/6267043/diff/23003/debugger/QT/SkQtWidget.h#ne...
debugger/QT/SkQtWidget.h:37: SkQtWidget(QWidget *parent = 0);
does it make sense to create a SkQtWidget with a NULL parent?

https://codereview.appspot.com/6267043/diff/23003/debugger/QT/ui_SkDebuggerUI.h
File debugger/QT/ui_SkDebuggerUI.h (right):

https://codereview.appspot.com/6267043/diff/23003/debugger/QT/ui_SkDebuggerUI...
debugger/QT/ui_SkDebuggerUI.h:37: QAction *fActionOpen;
do these need to be public?

https://codereview.appspot.com/6267043/diff/23003/debugger/QT/ui_SkDebuggerUI...
debugger/QT/ui_SkDebuggerUI.h:70: void setupUi(QMainWindow *SkDebuggerGUI) {
move the implementation into a cpp

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDebugCanvas.cpp
File debugger/SkDebugCanvas.cpp (right):

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDebugCanvas.cpp#...
debugger/SkDebugCanvas.cpp:16: fRestoreOffsetStack.setReserve(32);
shouldn't need this data structure at all

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDebugCanvas.h
File debugger/SkDebugCanvas.h (right):

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDebugCanvas.h#ne...
debugger/SkDebugCanvas.h:28: void addDrawCommand(SkDrawCommand* command);
should this be private?

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDebugCanvas.h#ne...
debugger/SkDebugCanvas.h:74:
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
limit this line to 80 characters.

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDebugger.cpp
File debugger/SkDebugger.cpp (right):

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDebugger.cpp#new...
debugger/SkDebugger.cpp:12: #include "SkDebugCanvas.h"
you shouldn't need most of these includes

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDebugger.h
File debugger/SkDebugger.h (right):

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDebugger.h#newco...
debugger/SkDebugger.h:14: class SkDebugger {
remove this file

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDrawCommand.cpp
File debugger/SkDrawCommand.cpp (right):

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDrawCommand.cpp#...
debugger/SkDrawCommand.cpp:20: Unused::Unused() {
Do we ever instantiate a class of this type?

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDrawCommand.cpp#...
debugger/SkDrawCommand.cpp:22: this->fDrawCommand = new std::string("UNUSED");
Why store the string for each command when you could store it once and either
reference it or just look it up each time.

static const char* getCommandString(DrawType type) {
  switch (type) {
    case UNUSED:
...
  }
}

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDrawCommand.h
File debugger/SkDrawCommand.h (right):

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:21: SkDrawCommand() { fInfo = new
std::vector<std::string>; };
move the constructor and destructor into the cpp

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:23: virtual std::string* ToString() { return
fDrawCommand; };
toString()

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:24: virtual const char* ToChar() {return
fDrawCommand->c_str(); };
toCString()

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:31: std::string *fDrawCommand;
std::string* -- the astrisk should follow the type.

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:35: class ToString {
what does this class do?

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:43: void execute(SkCanvas* canvas);
this and the other subclasses with this declaration should have the virtual
keyword.

virtual void execute...

https://codereview.appspot.com/6267043/diff/23003/debugger/SkObjectParser.h
File debugger/SkObjectParser.h (right):

https://codereview.appspot.com/6267043/diff/23003/debugger/SkObjectParser.h#n...
debugger/SkObjectParser.h:22: class SkObjectParser {
I think this is OK as a starter, but I wouldn't spend too much time on these as
I have a feeling this code will be reworked as we investigate making fields
editable.

We may want to consider using the JSON'ized objects for display and editing, but
I'll need to get that code checked in first.

https://codereview.appspot.com/6267043/diff/23003/debugger/SkObjectParser.h#n...
debugger/SkObjectParser.h:31: std::string bitmapToString(const SkBitmap&
bitmap);
make these functions static

https://codereview.appspot.com/6267043/diff/23003/debugger/moc.sh
File debugger/moc.sh (right):

https://codereview.appspot.com/6267043/diff/23003/debugger/moc.sh#newcode4
debugger/moc.sh:4:
SRC_DIR="/usr/local/google/home/chudy/trunk-linux/debugger/QT"
use the same trick that android_setup.sh does to get the directory of the
script.

https://codereview.appspot.com/6267043/diff/23003/gyp/debugger.gyp
File gyp/debugger.gyp (right):

https://codereview.appspot.com/6267043/diff/23003/gyp/debugger.gyp#newcode9
gyp/debugger.gyp:9: '../src/core', # needed to get SkConcaveToTriangle, maybe
this should be moved to include dir?
this is fine to have src/core here so you can remove the comment.

https://codereview.appspot.com/6267043/diff/23003/gyp/debugger.gyp#newcode13
gyp/debugger.gyp:13:
'/usr/local/google/home/chudy/tools/Qt/Desktop/Qt/4.8.1/gcc/include',
do you need all these Qt includes directories?

https://codereview.appspot.com/6267043/diff/23003/gyp/debugger.gyp#newcode69
gyp/debugger.gyp:69:
'/usr/local/google/home/chudy/tools/Qt/Desktop/Qt/4.8.1/gcc/lib',
do you need this since you reference the libraries by their absolute path?

https://codereview.appspot.com/6267043/diff/23003/gyp/debugger.gyp#newcode76
gyp/debugger.gyp:76: '-lGLU',
I doubt you need the openGL libs for now.

[chudy, 2012-06-20 23:11:17]

Spruced things up. Added a hacky function to allow the user to change
directories via opening a file in that same directory. Will probably exchange
with something else but wanted you and Mike to be able to open multiple files at
the same time on the mac version. My directory is just hardcoded in.

https://codereview.appspot.com/6267043/diff/23003/debugger/QT/SkQtWidget.h
File debugger/QT/SkQtWidget.h (right):

https://codereview.appspot.com/6267043/diff/23003/debugger/QT/SkQtWidget.h#ne...
debugger/QT/SkQtWidget.h:37: SkQtWidget(QWidget *parent = 0);
On 2012/06/18 20:01:18, DerekS wrote:
> does it make sense to create a SkQtWidget with a NULL parent?

Fixed, the options there now to do either or.

https://codereview.appspot.com/6267043/diff/23003/debugger/QT/ui_SkDebuggerUI.h
File debugger/QT/ui_SkDebuggerUI.h (right):

https://codereview.appspot.com/6267043/diff/23003/debugger/QT/ui_SkDebuggerUI...
debugger/QT/ui_SkDebuggerUI.h:37: QAction *fActionOpen;
On 2012/06/18 20:01:18, DerekS wrote:
> do these need to be public?

Moved these and all functions originally in this file into SkDebuggerGUI private

https://codereview.appspot.com/6267043/diff/23003/debugger/QT/ui_SkDebuggerUI...
debugger/QT/ui_SkDebuggerUI.h:70: void setupUi(QMainWindow *SkDebuggerGUI) {
On 2012/06/18 20:01:18, DerekS wrote:
> move the implementation into a cpp

Done.

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDebugCanvas.cpp
File debugger/SkDebugCanvas.cpp (right):

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDebugCanvas.cpp#...
debugger/SkDebugCanvas.cpp:16: fRestoreOffsetStack.setReserve(32);
On 2012/06/18 20:01:18, DerekS wrote:
> shouldn't need this data structure at all

Done. Removed along with all occurrences. This was back in the day when the
debug canvas would be both storing commands and being the surface they were
executed on. The stack was for saveLayer I believe.

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDebugCanvas.h
File debugger/SkDebugCanvas.h (right):

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDebugCanvas.h#ne...
debugger/SkDebugCanvas.h:28: void addDrawCommand(SkDrawCommand* command);
On 2012/06/18 20:01:18, DerekS wrote:
> should this be private?

Done.

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDebugCanvas.h#ne...
debugger/SkDebugCanvas.h:74:
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
On 2012/06/18 20:01:18, DerekS wrote:
> limit this line to 80 characters.

Done.

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDebugger.cpp
File debugger/SkDebugger.cpp (right):

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDebugger.cpp#new...
debugger/SkDebugger.cpp:12: #include "SkDebugCanvas.h"
On 2012/06/18 20:01:18, DerekS wrote:
> you shouldn't need most of these includes

Done.

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDebugger.h
File debugger/SkDebugger.h (right):

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDebugger.h#newco...
debugger/SkDebugger.h:14: class SkDebugger {
On 2012/06/18 20:01:18, DerekS wrote:
> remove this file

Done.

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDrawCommand.cpp
File debugger/SkDrawCommand.cpp (right):

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDrawCommand.cpp#...
debugger/SkDrawCommand.cpp:20: Unused::Unused() {
On 2012/06/18 20:01:18, DerekS wrote:
> Do we ever instantiate a class of this type?

Removed.

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDrawCommand.cpp#...
debugger/SkDrawCommand.cpp:22: this->fDrawCommand = new std::string("UNUSED");
On 2012/06/18 20:01:18, DerekS wrote:
> Why store the string for each command when you could store it once and either
> reference it or just look it up each time.
> 
> static const char* getCommandString(DrawType type) {
>   switch (type) {
>     case UNUSED:
> ...
>   }
> }

Done.

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDrawCommand.h
File debugger/SkDrawCommand.h (right):

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:21: SkDrawCommand() { fInfo = new
std::vector<std::string>; };
On 2012/06/18 20:01:18, DerekS wrote:
> move the constructor and destructor into the cpp

Done.

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:23: virtual std::string* ToString() { return
fDrawCommand; };
On 2012/06/18 20:01:18, DerekS wrote:
> toString()

Done.

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:24: virtual const char* ToChar() {return
fDrawCommand->c_str(); };
On 2012/06/18 20:01:18, DerekS wrote:
> toCString()

Done.

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:31: std::string *fDrawCommand;
On 2012/06/18 20:01:18, DerekS wrote:
> std::string* -- the astrisk should follow the type.

Done.

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:35: class ToString {
On 2012/06/18 20:01:18, DerekS wrote:
> what does this class do?

Removed. Was the old way I parsed the draw types.

https://codereview.appspot.com/6267043/diff/23003/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:43: void execute(SkCanvas* canvas);
On 2012/06/18 20:01:18, DerekS wrote:
> this and the other subclasses with this declaration should have the virtual
> keyword.
> 
> virtual void execute...

Done.

https://codereview.appspot.com/6267043/diff/23003/debugger/SkObjectParser.h
File debugger/SkObjectParser.h (right):

https://codereview.appspot.com/6267043/diff/23003/debugger/SkObjectParser.h#n...
debugger/SkObjectParser.h:31: std::string bitmapToString(const SkBitmap&
bitmap);
On 2012/06/18 20:01:18, DerekS wrote:
> make these functions static

Done.

https://codereview.appspot.com/6267043/diff/23003/debugger/moc.sh
File debugger/moc.sh (right):

https://codereview.appspot.com/6267043/diff/23003/debugger/moc.sh#newcode4
debugger/moc.sh:4:
SRC_DIR="/usr/local/google/home/chudy/trunk-linux/debugger/QT"
On 2012/06/18 20:01:18, DerekS wrote:
> use the same trick that android_setup.sh does to get the directory of the
> script.

Done.

https://codereview.appspot.com/6267043/diff/23003/gyp/debugger.gyp
File gyp/debugger.gyp (right):

https://codereview.appspot.com/6267043/diff/23003/gyp/debugger.gyp#newcode9
gyp/debugger.gyp:9: '../src/core', # needed to get SkConcaveToTriangle, maybe
this should be moved to include dir?
On 2012/06/18 20:01:18, DerekS wrote:
> this is fine to have src/core here so you can remove the comment.

Done.

https://codereview.appspot.com/6267043/diff/23003/gyp/debugger.gyp#newcode13
gyp/debugger.gyp:13:
'/usr/local/google/home/chudy/tools/Qt/Desktop/Qt/4.8.1/gcc/include',
On 2012/06/18 20:01:18, DerekS wrote:
> do you need all these Qt includes directories?

Nope, removed most of em.

https://codereview.appspot.com/6267043/diff/23003/gyp/debugger.gyp#newcode69
gyp/debugger.gyp:69:
'/usr/local/google/home/chudy/tools/Qt/Desktop/Qt/4.8.1/gcc/lib',
On 2012/06/18 20:01:18, DerekS wrote:
> do you need this since you reference the libraries by their absolute path?

I think we did for lGLU for openGL but removed for now

https://codereview.appspot.com/6267043/diff/23003/gyp/debugger.gyp#newcode76
gyp/debugger.gyp:76: '-lGLU',
On 2012/06/18 20:01:18, DerekS wrote:
> I doubt you need the openGL libs for now.

Done.

[DerekS, 2012-06-26 19:48:21]

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkDebuggerGUI.cpp
File debugger/QT/SkDebuggerGUI.cpp (right):

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkDebuggerGUI.c...
debugger/QT/SkDebuggerGUI.cpp:23: //    connect(fDirectoryWidget,
SIGNAL(itemActivated(QListWidgetItem*)), this, SLOT(loadFile(QListWidgetItem
*)));
remove this

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkDebuggerGUI.c...
debugger/QT/SkDebuggerGUI.cpp:75:
QPixmap("/usr/local/google/home/chudy/Icons/delete.png"));
remove all hardcoded images

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkDebuggerGUI.h
File debugger/QT/SkDebuggerGUI.h (right):

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkDebuggerGUI.h...
debugger/QT/SkDebuggerGUI.h:54: void initializeSkia();
why do we need to do this separately?

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkDebuggerGUI.h...
debugger/QT/SkDebuggerGUI.h:166: SkQtWidget* qtWidget;
fCanvasWidget

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkDebuggerGUI.h...
debugger/QT/SkDebuggerGUI.h:167: SkInspectorWidget* inspectorWidget;
fInspectorWidget

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkInspectorWidg...
File debugger/QT/SkInspectorWidget.h (right):

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkInspectorWidg...
debugger/QT/SkInspectorWidget.h:31: SkInspectorWidget(QWidget *parent = 0);
use NULL instead of 0

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkListWidget.h
File debugger/QT/SkListWidget.h (right):

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkListWidget.h#...
debugger/QT/SkListWidget.h:23: Constructs the list widget with the specified
parent for layout purposes.
clean up comments to match style in SkCanvas. Just be consistent.

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkListWidget.h#...
debugger/QT/SkListWidget.h:33: void paint ( QPainter * painter, const
QStyleOptionViewItem & option, const QModelIndex & index ) const;
paint(QPainter* ...

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkListWidget.h#...
debugger/QT/SkListWidget.h:38: QSize sizeHint (const QStyleOptionViewItem
&option, const QModelIndex &index) const;
same here.

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.cpp
File debugger/QT/SkQtWidget.cpp (right):

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.cpp#...
debugger/QT/SkQtWidget.cpp:32: return fDebugCanvas->getCommandInfoAt(i);
move into header.

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.cpp#...
debugger/QT/SkQtWidget.cpp:37: return fDebugCanvas->getQTStrings();
move into header

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.cpp#...
debugger/QT/SkQtWidget.cpp:61: /** NOTE(chudy): This was a test to determine if
the canvas size is accurately
comment style

/* dsfs
   dfsadfasdf 
 */

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.cpp#...
debugger/QT/SkQtWidget.cpp:72: /** NOTE(chudy): Forces Qt to refresh the widget
to reflect changes. */
remove the note

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.cpp#...
debugger/QT/SkQtWidget.cpp:77: fDebugCanvas->toggleCommand(index);
move into header.

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.cpp#...
debugger/QT/SkQtWidget.cpp:83: /** Note */
remove

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.cpp#...
debugger/QT/SkQtWidget.cpp:87: const QPoint po(1,1);
should be 0,0 and call it origin

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.h
File debugger/QT/SkQtWidget.h (right):

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.h#ne...
debugger/QT/SkQtWidget.h:22: /** \class SkQtWidget
rename this to SkCanvasWidget

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.h#ne...
debugger/QT/SkQtWidget.h:51: SkBitmap* getBitmap();
remove this if not used. If only used for bounds then expose a bounds function
instead of the bitmap

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.h#ne...
debugger/QT/SkQtWidget.h:57: std::vector<std::string>* getCurrentCommandInfo(int
i);
CommandInfo* getCommandAt(int i);

struct CommandInfo {
char** commands;
int commandCount;
};

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.h#ne...
debugger/QT/SkQtWidget.h:63: std::vector<std::string> getDrawCommands();
return a pointer

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.h#ne...
debugger/QT/SkQtWidget.h:68: TODO(chudy): Move to constructor
move into constructor and delete this method

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.h#ne...
debugger/QT/SkQtWidget.h:92: SkBitmap *fBitmap;
style nit: SkBitmap* fBitmap;

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDebugCanvas.h
File debugger/SkDebugCanvas.h (right):

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDebugCanvas.h#ne...
debugger/SkDebugCanvas.h:66: std::vector<std::string> getQTStrings();
This class shouldn't have any knowledge of QT even in the naming.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.cpp
File debugger/SkDrawCommand.cpp (right):

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.cpp#...
debugger/SkDrawCommand.cpp:14: #include <string>
move all system includes before the Skia includes

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.cpp#...
debugger/SkDrawCommand.cpp:27: free(fInfo);
when we stop storing standard strings we will need to free the contents of fInfo
as well.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.cpp#...
debugger/SkDrawCommand.cpp:32: case UNUSED: SkDebugf("DrawType UNUSED\n");
SkASSERT(0); break;
you can use SkDEBUGFAIL("DrawType UNUSED\n");

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.cpp#...
debugger/SkDrawCommand.cpp:67: SkASSERT(0);
same

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.cpp#...
debugger/SkDrawCommand.cpp:72: return getCommandString(fDrawType);
you can always inline these types of simple methods in the header.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.cpp#...
debugger/SkDrawCommand.cpp:98: this->fInfo->push_back(p.pathToString(path));
this->fInfo->push_back(SkObjectParser::pathToString(path));

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.cpp#...
debugger/SkDrawCommand.cpp:159: this->fInfo->push_back(p.scalarToString(left,
std::string("SkScalar left: ")));
cleanup ScalarToString to take const char* for the text

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.h
File debugger/SkDrawCommand.h (right):

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:20: bool fVisible;
this should be private with a getter/setter.

bool getVisibility() const { return fVisible; }

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:31: static const char* getCommandString(DrawType type);
capitalize it and seems like this should be private or at least protected.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:40: std::string* fDrawCommand;
it doesn't look like this variable is used anymore.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:41: std::vector<std::string>* fInfo;
why not store the std::vector object here instead of storing the ptr?

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:44: class ToString {
not sure what the point of this is?

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:53: virtual void execute(SkCanvas* canvas);
add SK_OVERRIDE to the end of all these execute commands

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:194: /* NOTE(chudy): DrawText is a predefined macro and
was breaking something
we could and probably should put all these draw commands in their own namespace.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.cpp
File debugger/SkObjectParser.cpp (right):

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.cpp...
debugger/SkObjectParser.cpp:16: std::string mBitmap;
this creates the string on the stack and then when you return it it creates
another string with the copy constructor.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.cpp...
debugger/SkObjectParser.cpp:22: std::string mBool("bool doAA: ");
since you know the max size of the string you can pre-allocate the space.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.cpp...
debugger/SkObjectParser.cpp:81: ss << matrix.get(2) << "), <br/>(";
are all the strings in html format?

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.cpp...
debugger/SkObjectParser.cpp:98: unsigned int color;
you can do this as SkColor color = paint.getColor();

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.cpp...
debugger/SkObjectParser.cpp:131: 
extra space

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.h
File debugger/SkObjectParser.h (right):

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.h#n...
debugger/SkObjectParser.h:14: #include "SkCanvas.h"
put all system includes at the top then the skia includes.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.h#n...
debugger/SkObjectParser.h:24: SkObjectParser();
if these do nothing then you don't need to define them.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.h#n...
debugger/SkObjectParser.h:31: static std::string bitmapToString(const SkBitmap&
bitmap);
static methods should begin with a capital letter.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.h#n...
debugger/SkObjectParser.h:37: static std::string boolToString(bool doAA);
we will want to change these to SkString

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.h#n...
debugger/SkObjectParser.h:45: static std::string intToString(int x, std::string
text);
seems like this would be better to take a const char* text

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.h#n...
debugger/SkObjectParser.h:100: static std::string
saveFlagsToString(SkCanvas::SaveFlags flags);
Going forward we may be able to share code with the work I'm doing for
flattenables.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.h#n...
debugger/SkObjectParser.h:114: static std::string textToString(const void*
text);
is this method really necessary?

https://codereview.appspot.com/6267043/diff/38001/debugger/moc.sh
File debugger/moc.sh (right):

https://codereview.appspot.com/6267043/diff/38001/debugger/moc.sh#newcode5
debugger/moc.sh:5: MOC="/usr/bin/moc"
can we test to see if the user has moc in their path? For example, we can run
'which moc' and if that returns no path then we can try to guess.

https://codereview.appspot.com/6267043/diff/38001/gyp/common_conditions.gypi
File gyp/common_conditions.gypi (right):

https://codereview.appspot.com/6267043/diff/38001/gyp/common_conditions.gypi#...
gyp/common_conditions.gypi:126: #'ARCHS': 'x86_64',
remove this

https://codereview.appspot.com/6267043/diff/38001/gyp/debugger.gyp
File gyp/debugger.gyp (right):

https://codereview.appspot.com/6267043/diff/38001/gyp/debugger.gyp#newcode6
gyp/debugger.gyp:6: 'mac_bundle' : 1,
should this be in the skia_os = mac conditions?

https://codereview.appspot.com/6267043/diff/38001/gyp/debugger.gyp#newcode11
gyp/debugger.gyp:11: '../include/effects',
you shouldn't need this as the dependency on effects.gyp:effects should pull it
in for you.

https://codereview.appspot.com/6267043/diff/38001/gyp/debugger.gyp#newcode70
gyp/debugger.gyp:70: 'C:/Qt/4.8.2/include',
I wonder if we can not assume C:/ as different users may have it mapped
differently. Not a big deal for this CL, but we may want to put a TODO here so
we don't forget about it.

[chudy, 2012-06-27 15:40:28]

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkDebuggerGUI.cpp
File debugger/QT/SkDebuggerGUI.cpp (right):

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkDebuggerGUI.c...
debugger/QT/SkDebuggerGUI.cpp:23: //    connect(fDirectoryWidget,
SIGNAL(itemActivated(QListWidgetItem*)), this, SLOT(loadFile(QListWidgetItem
*)));
On 2012/06/26 19:48:21, DerekS wrote:
> remove this

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkDebuggerGUI.h
File debugger/QT/SkDebuggerGUI.h (right):

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkDebuggerGUI.h...
debugger/QT/SkDebuggerGUI.h:54: void initializeSkia();
On 2012/06/26 19:48:21, DerekS wrote:
> why do we need to do this separately?

Refactored.

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkDebuggerGUI.h...
debugger/QT/SkDebuggerGUI.h:166: SkQtWidget* qtWidget;
On 2012/06/26 19:48:21, DerekS wrote:
> fCanvasWidget

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkDebuggerGUI.h...
debugger/QT/SkDebuggerGUI.h:167: SkInspectorWidget* inspectorWidget;
On 2012/06/26 19:48:21, DerekS wrote:
> fInspectorWidget

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkInspectorWidg...
File debugger/QT/SkInspectorWidget.h (right):

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkInspectorWidg...
debugger/QT/SkInspectorWidget.h:31: SkInspectorWidget(QWidget *parent = 0);
On 2012/06/26 19:48:21, DerekS wrote:
> use NULL instead of 0

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkListWidget.h
File debugger/QT/SkListWidget.h (right):

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkListWidget.h#...
debugger/QT/SkListWidget.h:23: Constructs the list widget with the specified
parent for layout purposes.
On 2012/06/26 19:48:21, DerekS wrote:
> clean up comments to match style in SkCanvas. Just be consistent.

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkListWidget.h#...
debugger/QT/SkListWidget.h:33: void paint ( QPainter * painter, const
QStyleOptionViewItem & option, const QModelIndex & index ) const;
On 2012/06/26 19:48:21, DerekS wrote:
> paint(QPainter* ...

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkListWidget.h#...
debugger/QT/SkListWidget.h:38: QSize sizeHint (const QStyleOptionViewItem
&option, const QModelIndex &index) const;
On 2012/06/26 19:48:21, DerekS wrote:
> same here.

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.cpp
File debugger/QT/SkQtWidget.cpp (right):

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.cpp#...
debugger/QT/SkQtWidget.cpp:32: return fDebugCanvas->getCommandInfoAt(i);
On 2012/06/26 19:48:21, DerekS wrote:
> move into header.

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.cpp#...
debugger/QT/SkQtWidget.cpp:37: return fDebugCanvas->getQTStrings();
On 2012/06/26 19:48:21, DerekS wrote:
> move into header

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.cpp#...
debugger/QT/SkQtWidget.cpp:61: /** NOTE(chudy): This was a test to determine if
the canvas size is accurately
On 2012/06/26 19:48:21, DerekS wrote:
> comment style
> 
> /* dsfs
>    dfsadfasdf 
>  */

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.cpp#...
debugger/QT/SkQtWidget.cpp:72: /** NOTE(chudy): Forces Qt to refresh the widget
to reflect changes. */
On 2012/06/26 19:48:21, DerekS wrote:
> remove the note

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.cpp#...
debugger/QT/SkQtWidget.cpp:77: fDebugCanvas->toggleCommand(index);
On 2012/06/26 19:48:21, DerekS wrote:
> move into header.

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.cpp#...
debugger/QT/SkQtWidget.cpp:83: /** Note */
On 2012/06/26 19:48:21, DerekS wrote:
> remove

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.cpp#...
debugger/QT/SkQtWidget.cpp:87: const QPoint po(1,1);
On 2012/06/26 19:48:21, DerekS wrote:
> should be 0,0 and call it origin

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.h
File debugger/QT/SkQtWidget.h (right):

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.h#ne...
debugger/QT/SkQtWidget.h:22: /** \class SkQtWidget
On 2012/06/26 19:48:21, DerekS wrote:
> rename this to SkCanvasWidget

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.h#ne...
debugger/QT/SkQtWidget.h:51: SkBitmap* getBitmap();
On 2012/06/26 19:48:21, DerekS wrote:
> remove this if not used. If only used for bounds then expose a bounds function
> instead of the bitmap

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.h#ne...
debugger/QT/SkQtWidget.h:57: std::vector<std::string>* getCurrentCommandInfo(int
i);
On 2012/06/26 19:48:21, DerekS wrote:
> CommandInfo* getCommandAt(int i);
> 
> struct CommandInfo {
> char** commands;
> int commandCount;
> }; 

Added todo.

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.h#ne...
debugger/QT/SkQtWidget.h:63: std::vector<std::string> getDrawCommands();
On 2012/06/26 19:48:21, DerekS wrote:
> return a pointer

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.h#ne...
debugger/QT/SkQtWidget.h:68: TODO(chudy): Move to constructor
On 2012/06/26 19:48:21, DerekS wrote:
> move into constructor and delete this method

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/QT/SkQtWidget.h#ne...
debugger/QT/SkQtWidget.h:92: SkBitmap *fBitmap;
On 2012/06/26 19:48:21, DerekS wrote:
> style nit: SkBitmap* fBitmap;

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDebugCanvas.h
File debugger/SkDebugCanvas.h (right):

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDebugCanvas.h#ne...
debugger/SkDebugCanvas.h:66: std::vector<std::string> getQTStrings();
On 2012/06/26 19:48:21, DerekS wrote:
> This class shouldn't have any knowledge of QT even in the naming.

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.cpp
File debugger/SkDrawCommand.cpp (right):

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.cpp#...
debugger/SkDrawCommand.cpp:14: #include <string>
On 2012/06/26 19:48:21, DerekS wrote:
> move all system includes before the Skia includes

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.cpp#...
debugger/SkDrawCommand.cpp:27: free(fInfo);
On 2012/06/26 19:48:21, DerekS wrote:
> when we stop storing standard strings we will need to free the contents of
fInfo
> as well.

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.cpp#...
debugger/SkDrawCommand.cpp:32: case UNUSED: SkDebugf("DrawType UNUSED\n");
SkASSERT(0); break;
On 2012/06/26 19:48:21, DerekS wrote:
> you can use SkDEBUGFAIL("DrawType UNUSED\n");

Replaced but it still wants the break; Not sure if intended or should return
NULL or...?

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.cpp#...
debugger/SkDrawCommand.cpp:67: SkASSERT(0);
On 2012/06/26 19:48:21, DerekS wrote:
> same

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.cpp#...
debugger/SkDrawCommand.cpp:72: return getCommandString(fDrawType);
On 2012/06/26 19:48:21, DerekS wrote:
> you can always inline these types of simple methods in the header.

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.cpp#...
debugger/SkDrawCommand.cpp:98: this->fInfo->push_back(p.pathToString(path));
On 2012/06/26 19:48:21, DerekS wrote:
> this->fInfo->push_back(SkObjectParser::pathToString(path));

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.cpp#...
debugger/SkDrawCommand.cpp:159: this->fInfo->push_back(p.scalarToString(left,
std::string("SkScalar left: ")));
On 2012/06/26 19:48:21, DerekS wrote:
> cleanup ScalarToString to take const char* for the text

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.h
File debugger/SkDrawCommand.h (right):

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:20: bool fVisible;
On 2012/06/26 19:48:21, DerekS wrote:
> this should be private with a getter/setter.
> 
> bool getVisibility() const { return fVisible; }

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:31: static const char* getCommandString(DrawType type);
On 2012/06/26 19:48:21, DerekS wrote:
> capitalize it and seems like this should be private or at least protected.

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:40: std::string* fDrawCommand;
On 2012/06/26 19:48:21, DerekS wrote:
> it doesn't look like this variable is used anymore.

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:41: std::vector<std::string>* fInfo;
On 2012/06/26 19:48:21, DerekS wrote:
> why not store the std::vector object here instead of storing the ptr?

Changed but I forget the point of this change? Is it so that we don't have to
worry about freeing it in the destructor?

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:44: class ToString {
On 2012/06/26 19:48:21, DerekS wrote:
> not sure what the point of this is?

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:53: virtual void execute(SkCanvas* canvas);
On 2012/06/26 19:48:21, DerekS wrote:
> add SK_OVERRIDE to the end of all these execute commands

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:194: /* NOTE(chudy): DrawText is a predefined macro and
was breaking something
On 2012/06/26 19:48:21, DerekS wrote:
> we could and probably should put all these draw commands in their own
namespace.

Changed comment to TODO

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.cpp
File debugger/SkObjectParser.cpp (right):

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.cpp...
debugger/SkObjectParser.cpp:16: std::string mBitmap;
On 2012/06/26 19:48:21, DerekS wrote:
> this creates the string on the stack and then when you return it it creates
> another string with the copy constructor.

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.cpp...
debugger/SkObjectParser.cpp:22: std::string mBool("bool doAA: ");
On 2012/06/26 19:48:21, DerekS wrote:
> since you know the max size of the string you can pre-allocate the space.

I tried looking this stuff up but it appears as though you can only
preinitialize a string of size_t n with a character you pass in. So for example
if you toss it std::string(16,"s") it'll create a string that looks like this:
"sssssssssssssss". I ended up doing something else that is efficient though

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.cpp...
debugger/SkObjectParser.cpp:81: ss << matrix.get(2) << "), <br/>(";
On 2012/06/26 19:48:21, DerekS wrote:
> are all the strings in html format?

Nope but they get parsed as html. Added a todo.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.cpp...
debugger/SkObjectParser.cpp:98: unsigned int color;
On 2012/06/26 19:48:21, DerekS wrote:
> you can do this as SkColor color = paint.getColor();

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.cpp...
debugger/SkObjectParser.cpp:131: 
On 2012/06/26 19:48:21, DerekS wrote:
> extra space

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.h
File debugger/SkObjectParser.h (right):

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.h#n...
debugger/SkObjectParser.h:14: #include "SkCanvas.h"
On 2012/06/26 19:48:21, DerekS wrote:
> put all system includes at the top then the skia includes.

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.h#n...
debugger/SkObjectParser.h:24: SkObjectParser();
On 2012/06/26 19:48:21, DerekS wrote:
> if these do nothing then you don't need to define them.

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.h#n...
debugger/SkObjectParser.h:31: static std::string bitmapToString(const SkBitmap&
bitmap);
On 2012/06/26 19:48:21, DerekS wrote:
> static methods should begin with a capital letter.

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.h#n...
debugger/SkObjectParser.h:37: static std::string boolToString(bool doAA);
On 2012/06/26 19:48:21, DerekS wrote:
> we will want to change these to SkString

Added todo.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.h#n...
debugger/SkObjectParser.h:45: static std::string intToString(int x, std::string
text);
On 2012/06/26 19:48:21, DerekS wrote:
> seems like this would be better to take a const char* text

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.h#n...
debugger/SkObjectParser.h:100: static std::string
saveFlagsToString(SkCanvas::SaveFlags flags);
On 2012/06/26 19:48:21, DerekS wrote:
> Going forward we may be able to share code with the work I'm doing for
> flattenables.

Done.

https://codereview.appspot.com/6267043/diff/38001/debugger/SkObjectParser.h#n...
debugger/SkObjectParser.h:114: static std::string textToString(const void*
text);
On 2012/06/26 19:48:21, DerekS wrote:
> is this method really necessary?

It's to append Text: to the beginning of the string. Even when we switch over to
char* we should still keep it.

https://codereview.appspot.com/6267043/diff/38001/debugger/moc.sh
File debugger/moc.sh (right):

https://codereview.appspot.com/6267043/diff/38001/debugger/moc.sh#newcode5
debugger/moc.sh:5: MOC="/usr/bin/moc"
On 2012/06/26 19:48:21, DerekS wrote:
> can we test to see if the user has moc in their path? For example, we can run
> 'which moc' and if that returns no path then we can try to guess.

This script only works on linux. Should we remove it entirely? At the moment its
the only one generating moc files.

https://codereview.appspot.com/6267043/diff/38001/gyp/common_conditions.gypi
File gyp/common_conditions.gypi (right):

https://codereview.appspot.com/6267043/diff/38001/gyp/common_conditions.gypi#...
gyp/common_conditions.gypi:126: #'ARCHS': 'x86_64',
On 2012/06/26 19:48:21, DerekS wrote:
> remove this

Done.

https://codereview.appspot.com/6267043/diff/38001/gyp/debugger.gyp
File gyp/debugger.gyp (right):

https://codereview.appspot.com/6267043/diff/38001/gyp/debugger.gyp#newcode6
gyp/debugger.gyp:6: 'mac_bundle' : 1,
On 2012/06/26 19:48:21, DerekS wrote:
> should this be in the skia_os = mac conditions?

Done.

https://codereview.appspot.com/6267043/diff/38001/gyp/debugger.gyp#newcode11
gyp/debugger.gyp:11: '../include/effects',
On 2012/06/26 19:48:21, DerekS wrote:
> you shouldn't need this as the dependency on effects.gyp:effects should pull
it
> in for you.

Done.

https://codereview.appspot.com/6267043/diff/38001/gyp/debugger.gyp#newcode70
gyp/debugger.gyp:70: 'C:/Qt/4.8.2/include',
On 2012/06/26 19:48:21, DerekS wrote:
> I wonder if we can not assume C:/ as different users may have it mapped
> differently. Not a big deal for this CL, but we may want to put a TODO here so
> we don't forget about it.

Added todo.

[DerekS, 2012-06-28 16:00:45]

https://codereview.appspot.com/6267043/diff/46001/debugger/QT/SkDebuggerGUI.cpp
File debugger/QT/SkDebuggerGUI.cpp (right):

https://codereview.appspot.com/6267043/diff/46001/debugger/QT/SkDebuggerGUI.c...
debugger/QT/SkDebuggerGUI.cpp:89:
QPixmap("/usr/local/google/home/chudy/Icons/delete.png"));
remove the local file dependencies

https://codereview.appspot.com/6267043/diff/46001/debugger/SkDebugCanvas.cpp
File debugger/SkDebugCanvas.cpp (right):

https://codereview.appspot.com/6267043/diff/46001/debugger/SkDebugCanvas.cpp#...
debugger/SkDebugCanvas.cpp:125: INHERITED::clear(color);
no need to call the inherited methods if this is going to be private.

https://codereview.appspot.com/6267043/diff/46001/debugger/SkDebugCanvas.h
File debugger/SkDebugCanvas.h (right):

https://codereview.appspot.com/6267043/diff/46001/debugger/SkDebugCanvas.h#ne...
debugger/SkDebugCanvas.h:160: /* NOTE(chudy): Is this necessary? Haven't crashed
yet.
not if you make this class private to only the SkDebugger class,

https://codereview.appspot.com/6267043/diff/46001/debugger/SkDebugCanvas.h#ne...
debugger/SkDebugCanvas.h:165: void reset();
no need to implement if this is going to be private

https://codereview.appspot.com/6267043/diff/46001/debugger/SkDebugCanvas.h#ne...
debugger/SkDebugCanvas.h:177: };
you can delete these variables if it is going to be private

https://codereview.appspot.com/6267043/diff/46001/debugger/SkDrawCommand.cpp
File debugger/SkDrawCommand.cpp (right):

https://codereview.appspot.com/6267043/diff/46001/debugger/SkDrawCommand.cpp#...
debugger/SkDrawCommand.cpp:294: SkScalar constX = xpos[0];
you shouldn't need to. The SkObjectParser functions should take const params

https://codereview.appspot.com/6267043/diff/46001/debugger/SkDrawCommand.h
File debugger/SkDrawCommand.h (right):

https://codereview.appspot.com/6267043/diff/46001/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:20: /* TODO(chudy): Implement destructor in children.
*/
change the TODO to not implement a destructor but to not have subclasses

https://codereview.appspot.com/6267043/diff/46001/debugger/SkObjectParser.cpp
File debugger/SkObjectParser.cpp (right):

https://codereview.appspot.com/6267043/diff/46001/debugger/SkObjectParser.cpp...
debugger/SkObjectParser.cpp:21: return mBool;
this can be simplified to return "bool doAA: True";

https://codereview.appspot.com/6267043/diff/46001/debugger/SkObjectParser.cpp...
debugger/SkObjectParser.cpp:70: std::string mPath;
why prefix the variable with m?

https://codereview.appspot.com/6267043/diff/46001/debugger/SkObjectParser.cpp...
debugger/SkObjectParser.cpp:143: 
extra space

[chudy, 2012-06-28 20:26:56]

Fixed all previous comments. Added stubs for matrix, clip and settings widget.
They are physically visible but disabled in the UI for now (non interactable)

https://codereview.appspot.com/6267043/diff/46001/debugger/SkDebugCanvas.cpp
File debugger/SkDebugCanvas.cpp (right):

https://codereview.appspot.com/6267043/diff/46001/debugger/SkDebugCanvas.cpp#...
debugger/SkDebugCanvas.cpp:125: INHERITED::clear(color);
On 2012/06/28 16:00:45, DerekS wrote:
> no need to call the inherited methods if this is going to be private.

Done.

https://codereview.appspot.com/6267043/diff/46001/debugger/SkDebugCanvas.h
File debugger/SkDebugCanvas.h (right):

https://codereview.appspot.com/6267043/diff/46001/debugger/SkDebugCanvas.h#ne...
debugger/SkDebugCanvas.h:160: /* NOTE(chudy): Is this necessary? Haven't crashed
yet.
On 2012/06/28 16:00:45, DerekS wrote:
> not if you make this class private to only the SkDebugger class,

Not sure what you mean here but I removed the comment.

https://codereview.appspot.com/6267043/diff/46001/debugger/SkDebugCanvas.h#ne...
debugger/SkDebugCanvas.h:165: void reset();
On 2012/06/28 16:00:45, DerekS wrote:
> no need to implement if this is going to be private

Removed completely.

https://codereview.appspot.com/6267043/diff/46001/debugger/SkDebugCanvas.h#ne...
debugger/SkDebugCanvas.h:177: };
On 2012/06/28 16:00:45, DerekS wrote:
> you can delete these variables if it is going to be private

Done.

https://codereview.appspot.com/6267043/diff/46001/debugger/SkDrawCommand.cpp
File debugger/SkDrawCommand.cpp (right):

https://codereview.appspot.com/6267043/diff/46001/debugger/SkDrawCommand.cpp#...
debugger/SkDrawCommand.cpp:294: SkScalar constX = xpos[0];
On 2012/06/28 16:00:45, DerekS wrote:
> you shouldn't need to. The SkObjectParser functions should take const params

Fixed. Still confused though with the implementation of xpos[]. Is it a pointer?
I don't think its an array since SkScalar typedefs to a float...

https://codereview.appspot.com/6267043/diff/46001/debugger/SkDrawCommand.h
File debugger/SkDrawCommand.h (right):

https://codereview.appspot.com/6267043/diff/46001/debugger/SkDrawCommand.h#ne...
debugger/SkDrawCommand.h:20: /* TODO(chudy): Implement destructor in children.
*/
On 2012/06/28 16:00:45, DerekS wrote:
> change the TODO to not implement a destructor but to not have subclasses

Done.

https://codereview.appspot.com/6267043/diff/46001/debugger/SkObjectParser.cpp
File debugger/SkObjectParser.cpp (right):

https://codereview.appspot.com/6267043/diff/46001/debugger/SkObjectParser.cpp...
debugger/SkObjectParser.cpp:21: return mBool;
On 2012/06/28 16:00:45, DerekS wrote:
> this can be simplified to return "bool doAA: True";

Done. +1

https://codereview.appspot.com/6267043/diff/46001/debugger/SkObjectParser.cpp...
debugger/SkObjectParser.cpp:70: std::string mPath;
On 2012/06/28 16:00:45, DerekS wrote:
> why prefix the variable with m?

Wanted to keep something consistent between most functions and the variables I'm
prefixing M to usually have the same name as the parameters in these functions.
Is there a style for variables that are not member variables?

https://codereview.appspot.com/6267043/diff/46001/debugger/SkObjectParser.cpp...
debugger/SkObjectParser.cpp:143: 
On 2012/06/28 16:00:45, DerekS wrote:
> extra space

Done.

[DerekS, 2012-06-29 12:57:49]

lgtm. Let's get this code in and start iterating over the TODO's and send out
some instructions for early adopters.

https://codereview.appspot.com/6267043/diff/46001/debugger/SkDrawCommand.cpp
File debugger/SkDrawCommand.cpp (right):

https://codereview.appspot.com/6267043/diff/46001/debugger/SkDrawCommand.cpp#...
debugger/SkDrawCommand.cpp:294: SkScalar constX = xpos[0];
On 2012/06/28 20:26:56, chudy wrote:
> On 2012/06/28 16:00:45, DerekS wrote:
> > you shouldn't need to. The SkObjectParser functions should take const params
> 
> Fixed. Still confused though with the implementation of xpos[]. Is it a
pointer?
> I don't think its an array since SkScalar typedefs to a float...

Yes xpos is just a pointer to an array of SkScalars.