Added OpenGL support to Debugger

GL Widget Alpha

[chudy, 2012-07-23 21:55:14]

I removed skia-review from this CL for now. I wanted to submit something so you
can see I've made some progress.

After a ton of tinkering it looks like I was able to smash openGL into this Qt
window. It has ALOT of bugs and doesn't work for every other skia picture (don't
know why). 

After sitting down and talking with both Brian and Rob we've decided the next
route of action is to hook up the stepping functionality of the debugger, the
deleting and saving to a skpicture to debug what's going on behind the scenes
with this GL stuff.

[DerekS, 2012-07-24 03:52:25]

just wanted to give you my quick thoughts on the matter. You can ignore them for
now and we can talk about it when you have time.

http://codereview.appspot.com/6422060/diff/1/debugger/QT/SkDebuggerGUI.cpp
File debugger/QT/SkDebuggerGUI.cpp (right):

http://codereview.appspot.com/6422060/diff/1/debugger/QT/SkDebuggerGUI.cpp#ne...
debugger/QT/SkDebuggerGUI.cpp:536: fCanvasWidget.loadPicture(fileName);
seems like we should rename fCanvasWidget to fRasterWidget. Even better we could
create a wrapper class that encapsulates them both and then the caller doesn't
care which one he gets.

http://codereview.appspot.com/6422060/diff/1/debugger/QT/SkGLWidget.h
File debugger/QT/SkGLWidget.h (right):

http://codereview.appspot.com/6422060/diff/1/debugger/QT/SkGLWidget.h#newcode32
debugger/QT/SkGLWidget.h:32: debugCanvas->drawTo(fCanvas,
debugCanvas->getSize(), &temp);
seems like this drawTo() should be in the paintGL method.  However to do that
you would need to store a pointer to the debug canvas in your class.  Would
storing the ptr be acceptable?

[chudy, 2012-07-24 13:58:58]

Replied to comments, CL isn't updated yet.

http://codereview.appspot.com/6422060/diff/1/debugger/QT/SkDebuggerGUI.cpp
File debugger/QT/SkDebuggerGUI.cpp (right):

http://codereview.appspot.com/6422060/diff/1/debugger/QT/SkDebuggerGUI.cpp#ne...
debugger/QT/SkDebuggerGUI.cpp:536: fCanvasWidget.loadPicture(fileName);
On 2012/07/24 03:52:25, DerekS wrote:
> seems like we should rename fCanvasWidget to fRasterWidget. Even better we
could
> create a wrapper class that encapsulates them both and then the caller doesn't
> care which one he gets.

Absolutely, today might finally be the day the great refactor has come

http://codereview.appspot.com/6422060/diff/1/debugger/QT/SkGLWidget.h
File debugger/QT/SkGLWidget.h (right):

http://codereview.appspot.com/6422060/diff/1/debugger/QT/SkGLWidget.h#newcode32
debugger/QT/SkGLWidget.h:32: debugCanvas->drawTo(fCanvas,
debugCanvas->getSize(), &temp);
On 2012/07/24 03:52:25, DerekS wrote:
> seems like this drawTo() should be in the paintGL method.  However to do that
> you would need to store a pointer to the debug canvas in your class.  Would
> storing the ptr be acceptable?

I've been thinking about this and am wondering if that would perhaps not slow
down performance? I am assuming paintGL is triggered over and over again and it
just displays whatever is drawn to that render target onto the screen.

The canvas only needs to be updated once per new command scrolling or picture
loading. To me that makes it seem like it would be more logical to keep it out
of paintGL since that would force redraws even when unnecessary?

[DerekS, 2012-07-24 14:55:18]

http://codereview.appspot.com/6422060/diff/1/debugger/QT/SkGLWidget.h
File debugger/QT/SkGLWidget.h (right):

http://codereview.appspot.com/6422060/diff/1/debugger/QT/SkGLWidget.h#newcode32
debugger/QT/SkGLWidget.h:32: debugCanvas->drawTo(fCanvas,
debugCanvas->getSize(), &temp);
My understanding of paintGL() is that is only called when you notify the widget
that you want it to change it's contents by calling updateGL().  In most
platforms behave this way and to constantly animate you set some draw flag or
just continuously call updateGL().

On 2012/07/24 13:58:58, chudy wrote:
> On 2012/07/24 03:52:25, DerekS wrote:
> > seems like this drawTo() should be in the paintGL method.  However to do
that
> > you would need to store a pointer to the debug canvas in your class.  Would
> > storing the ptr be acceptable?
> 
> I've been thinking about this and am wondering if that would perhaps not slow
> down performance? I am assuming paintGL is triggered over and over again and
it
> just displays whatever is drawn to that render target onto the screen.
> 
> The canvas only needs to be updated once per new command scrolling or picture
> loading. To me that makes it seem like it would be more logical to keep it out
> of paintGL since that would force redraws even when unnecessary?

[chudy, 2012-07-25 19:27:43]

Everything seems to be working

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

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkDebuggerGUI.cp...
debugger/QT/SkDebuggerGUI.cpp:159: void SkDebuggerGUI::actionGLWidget(bool
setHidden) {
Change setHidden var name to isToggled. Makes more sense.

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkDebuggerGUI.cp...
debugger/QT/SkDebuggerGUI.cpp:183: void SkDebuggerGUI::actionRasterWidget(bool
setHidden) {
Here as well.

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkDebuggerGUI.cp...
debugger/QT/SkDebuggerGUI.cpp:264: std::vector<std::string> *v =
fCanvasWidget.getCurrentCommandInfo(
rename v

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkDebuggerGUI.cp...
debugger/QT/SkDebuggerGUI.cpp:422: //            QSizePolicy::Expanding);
remove commented out code

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkDebuggerGUI.cp...
debugger/QT/SkDebuggerGUI.cpp:537: //   
fGLWidget.drawToGLWidget(fCanvasWidget.getCurrentDebugCanvas());
remove commented code

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

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkDebuggerGUI.h#...
debugger/QT/SkDebuggerGUI.h:96: void actionGLWidget(bool setHidden);
Change to isToggled or isOn as before. Refers to state of checkbox

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkDebuggerGUI.h#...
debugger/QT/SkDebuggerGUI.h:112: void actionRasterWidget(bool setHidden);
same here

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkDebuggerGUI.h#...
debugger/QT/SkDebuggerGUI.h:218: //    SkRasterWidget fRasterWidget;
remove commented code

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkGLWidget.cpp
File debugger/QT/SkGLWidget.cpp (right):

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkGLWidget.cpp#n...
debugger/QT/SkGLWidget.cpp:67: fDebugCanvas->drawTo(&canvas, fIndex+1, NULL);
Refactor debug canvas so that it doesn't take a bitmap as an arguement

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkSettingsWidget...
File debugger/QT/SkSettingsWidget.cpp (right):

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkSettingsWidget...
debugger/QT/SkSettingsWidget.cpp:23: , fCanvasLayout(&fCanvasFrame)
See if these can't be instantiated without parent widgets and added
programatically. Will make it easier to follow

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkSettingsWidget.h
File debugger/QT/SkSettingsWidget.h (right):

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkSettingsWidget...
debugger/QT/SkSettingsWidget.h:43: return &fGLBox;
Rename to GLCheckBox for sake of preserving naming convention, same with
rasterbox

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/moc_SkCanvasWidg...
File debugger/QT/moc_SkCanvasWidget.cpp (right):

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/moc_SkCanvasWidg...
debugger/QT/moc_SkCanvasWidget.cpp:6: **
Remove this file.

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/moc_SkInspectorW...
File debugger/QT/moc_SkInspectorWidget.cpp (right):

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/moc_SkInspectorW...
debugger/QT/moc_SkInspectorWidget.cpp:6: **
Remove this file.

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/moc_SkRasterWidg...
File debugger/QT/moc_SkRasterWidget.cpp (right):

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/moc_SkRasterWidg...
debugger/QT/moc_SkRasterWidget.cpp:6: **
Remove this file and associated slots with it in the file. Remove moc file from
debugger.gyp. The slots / signals should be exclusively in SkCanvasWidget

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/moc_SkSettingsWi...
File debugger/QT/moc_SkSettingsWidget.cpp (right):

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/moc_SkSettingsWi...
debugger/QT/moc_SkSettingsWidget.cpp:6: **
Remove this file

https://codereview.appspot.com/6422060/diff/7001/debugger/moc.sh
File debugger/moc.sh (right):

https://codereview.appspot.com/6422060/diff/7001/debugger/moc.sh#newcode11
debugger/moc.sh:11: $MOC $SRC_DIR/SkRasterWidget.h -o
$SRC_DIR/moc_SkRasterWidget.cpp
Remove raster widget from this

https://codereview.appspot.com/6422060/diff/7001/gyp/debugger.gyp
File gyp/debugger.gyp (right):

https://codereview.appspot.com/6422060/diff/7001/gyp/debugger.gyp#newcode21
gyp/debugger.gyp:21: '../debugger/QT/moc_SkRasterWidget.cpp',
remove raster widget

[DerekS, 2012-07-25 19:49:38]

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkInspectorWidget.h
File debugger/QT/SkInspectorWidget.h (right):

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkInspectorWidge...
debugger/QT/SkInspectorWidget.h:58: void setMatrix(SkMatrix matrix);
why not pass them by reference?

[chudy, 2012-07-25 20:39:58]

Addressed comments

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

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkDebuggerGUI.cp...
debugger/QT/SkDebuggerGUI.cpp:159: void SkDebuggerGUI::actionGLWidget(bool
setHidden) {
On 2012/07/25 19:27:43, chudy wrote:
> Change setHidden var name to isToggled. Makes more sense.

Done.

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkDebuggerGUI.cp...
debugger/QT/SkDebuggerGUI.cpp:183: void SkDebuggerGUI::actionRasterWidget(bool
setHidden) {
On 2012/07/25 19:27:43, chudy wrote:
> Here as well.

Done.

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkDebuggerGUI.cp...
debugger/QT/SkDebuggerGUI.cpp:264: std::vector<std::string> *v =
fCanvasWidget.getCurrentCommandInfo(
On 2012/07/25 19:27:43, chudy wrote:
> rename v 

Done.

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkDebuggerGUI.cp...
debugger/QT/SkDebuggerGUI.cpp:422: //            QSizePolicy::Expanding);
On 2012/07/25 19:27:43, chudy wrote:
> remove commented out code

Done.

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkDebuggerGUI.cp...
debugger/QT/SkDebuggerGUI.cpp:537: //   
fGLWidget.drawToGLWidget(fCanvasWidget.getCurrentDebugCanvas());
On 2012/07/25 19:27:43, chudy wrote:
> remove commented code

Done.

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

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkDebuggerGUI.h#...
debugger/QT/SkDebuggerGUI.h:96: void actionGLWidget(bool setHidden);
On 2012/07/25 19:27:43, chudy wrote:
> Change to isToggled or isOn as before. Refers to state of checkbox

Done.

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkDebuggerGUI.h#...
debugger/QT/SkDebuggerGUI.h:112: void actionRasterWidget(bool setHidden);
On 2012/07/25 19:27:43, chudy wrote:
> same here

Done.

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkDebuggerGUI.h#...
debugger/QT/SkDebuggerGUI.h:218: //    SkRasterWidget fRasterWidget;
On 2012/07/25 19:27:43, chudy wrote:
> remove commented code

Done.

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkGLWidget.cpp
File debugger/QT/SkGLWidget.cpp (right):

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkGLWidget.cpp#n...
debugger/QT/SkGLWidget.cpp:67: fDebugCanvas->drawTo(&canvas, fIndex+1, NULL);
On 2012/07/25 19:27:43, chudy wrote:
> Refactor debug canvas so that it doesn't take a bitmap as an arguement

Added a TODO

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkInspectorWidget.h
File debugger/QT/SkInspectorWidget.h (right):

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkInspectorWidge...
debugger/QT/SkInspectorWidget.h:58: void setMatrix(SkMatrix matrix);
On 2012/07/25 19:49:38, DerekS wrote:
> why not pass them by reference?

Done.

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkSettingsWidget...
File debugger/QT/SkSettingsWidget.cpp (right):

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkSettingsWidget...
debugger/QT/SkSettingsWidget.cpp:23: , fCanvasLayout(&fCanvasFrame)
On 2012/07/25 19:27:43, chudy wrote:
> See if these can't be instantiated without parent widgets and added
> programatically. Will make it easier to follow

Added TODO.

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkSettingsWidget.h
File debugger/QT/SkSettingsWidget.h (right):

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/SkSettingsWidget...
debugger/QT/SkSettingsWidget.h:43: return &fGLBox;
On 2012/07/25 19:27:43, chudy wrote:
> Rename to GLCheckBox for sake of preserving naming convention, same with
> rasterbox

Done.

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/moc_SkCanvasWidg...
File debugger/QT/moc_SkCanvasWidget.cpp (right):

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/moc_SkCanvasWidg...
debugger/QT/moc_SkCanvasWidget.cpp:6: **
On 2012/07/25 19:27:43, chudy wrote:
> Remove this file.

Done.

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/moc_SkInspectorW...
File debugger/QT/moc_SkInspectorWidget.cpp (right):

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/moc_SkInspectorW...
debugger/QT/moc_SkInspectorWidget.cpp:6: **
On 2012/07/25 19:27:43, chudy wrote:
> Remove this file.

Done.

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/moc_SkRasterWidg...
File debugger/QT/moc_SkRasterWidget.cpp (right):

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/moc_SkRasterWidg...
debugger/QT/moc_SkRasterWidget.cpp:6: **
On 2012/07/25 19:27:43, chudy wrote:
> Remove this file and associated slots with it in the file. Remove moc file
from
> debugger.gyp. The slots / signals should be exclusively in SkCanvasWidget

Done.

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/moc_SkSettingsWi...
File debugger/QT/moc_SkSettingsWidget.cpp (right):

https://codereview.appspot.com/6422060/diff/7001/debugger/QT/moc_SkSettingsWi...
debugger/QT/moc_SkSettingsWidget.cpp:6: **
On 2012/07/25 19:27:43, chudy wrote:
> Remove this file

Done.

https://codereview.appspot.com/6422060/diff/7001/debugger/moc.sh
File debugger/moc.sh (right):

https://codereview.appspot.com/6422060/diff/7001/debugger/moc.sh#newcode11
debugger/moc.sh:11: $MOC $SRC_DIR/SkRasterWidget.h -o
$SRC_DIR/moc_SkRasterWidget.cpp
On 2012/07/25 19:27:43, chudy wrote:
> Remove raster widget from this

Done.

https://codereview.appspot.com/6422060/diff/7001/gyp/debugger.gyp
File gyp/debugger.gyp (right):

https://codereview.appspot.com/6422060/diff/7001/gyp/debugger.gyp#newcode21
gyp/debugger.gyp:21: '../debugger/QT/moc_SkRasterWidget.cpp',
On 2012/07/25 19:27:43, chudy wrote:
> remove raster widget

Done.

[DerekS, 2012-07-25 20:51:03]

I didn't have time to look at SkCanvasWidget but here are my comments on the
GLWidget.

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkGLWidget.cpp
File debugger/QT/SkGLWidget.cpp (right):

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkGLWidget.cpp#n...
debugger/QT/SkGLWidget.cpp:19: fIndex = 0;
this should be in the constructor

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkGLWidget.cpp#n...
debugger/QT/SkGLWidget.cpp:20: fDebugCanvas = new SkDebugCanvas();
this should be in the constructor but initialize it to null.

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkGLWidget.cpp#n...
debugger/QT/SkGLWidget.cpp:21: fCurIntf = GrGLCreateNativeInterface();
you should unref this in your destructor.

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkGLWidget.cpp#n...
debugger/QT/SkGLWidget.cpp:22: fCurContext =
GrContext::Create(kOpenGL_Shaders_GrEngine, (GrPlatform3DContext) fCurIntf);
you should unref this in the destructor.

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkGLWidget.cpp#n...
debugger/QT/SkGLWidget.cpp:24: GrPlatformRenderTargetDesc desc;
move the construction of this into a private function so you can share it with
resizeGL()

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkGLWidget.cpp#n...
debugger/QT/SkGLWidget.cpp:34: fCurRenderTarget =
fCurContext->createPlatformRenderTarget(desc);
you don't need to store this as a member variable.

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkGLWidget.cpp#n...
debugger/QT/SkGLWidget.cpp:35: fGpuDevice = new SkGpuDevice(fCurContext,
fCurRenderTarget);
you should unref this in the destructor.

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkGLWidget.cpp#n...
debugger/QT/SkGLWidget.cpp:54: delete fGpuDevice;
don't call delete but instead use SkSafeUnref(fGpuDevice)

[DerekS, 2012-07-26 13:55:55]

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkCanvasWidget.cpp
File debugger/QT/SkCanvasWidget.cpp (right):

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkCanvasWidget.c...
debugger/QT/SkCanvasWidget.cpp:91: fGLWidget.setScale(fScaleFactor);
why not just pass the scale/translate as part of the drawTo method to the raster
or gl widget?

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkCanvasWidget.h
File debugger/QT/SkCanvasWidget.h (right):

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkCanvasWidget.h...
debugger/QT/SkCanvasWidget.h:57: fRasterWidget.setHidden(hidden);
use an enum to list all the possible widget types. Then have one setter for all
types to control visibility.

enum WidgetType {
    kRaster8888_Type = 1 << 0,
    kGPU_Type        = 1 << 1
};

void setWidgetVisibility(WidgetType type, bool isHidden);

[chudy, 2012-07-26 14:57:22]

Addressed comments in two different patches. First one got the GL Widget, second
one added enums to the canvas widget.

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkCanvasWidget.cpp
File debugger/QT/SkCanvasWidget.cpp (right):

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkCanvasWidget.c...
debugger/QT/SkCanvasWidget.cpp:91: fGLWidget.setScale(fScaleFactor);
On 2012/07/26 13:55:55, DerekS wrote:
> why not just pass the scale/translate as part of the drawTo method to the
raster
> or gl widget?

Potentially a lot of wasted calls. If someone is just scrolling through a list
of commands or directory its setting the translate and scale over and over again
when it doesn't need to. 

I was going to shove all of these in an updateTransform function however that
will work with our enum. Not sure if there's any point to either of these ways
other than to reuse code

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkCanvasWidget.h
File debugger/QT/SkCanvasWidget.h (right):

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkCanvasWidget.h...
debugger/QT/SkCanvasWidget.h:57: fRasterWidget.setHidden(hidden);
On 2012/07/26 13:55:55, DerekS wrote:
> use an enum to list all the possible widget types. Then have one setter for
all
> types to control visibility.
> 
> enum WidgetType {
>     kRaster8888_Type = 1 << 0,
>     kGPU_Type        = 1 << 1
> };
> 
> void setWidgetVisibility(WidgetType type, bool isHidden);

Done. Is there a standard as to which way to do enums? You did bit shifting, but
there's also 0x01, 0x02, etc and just = 1, =2, etc

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkGLWidget.cpp
File debugger/QT/SkGLWidget.cpp (right):

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkGLWidget.cpp#n...
debugger/QT/SkGLWidget.cpp:19: fIndex = 0;
On 2012/07/25 20:51:04, DerekS wrote:
> this should be in the constructor

Done.

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkGLWidget.cpp#n...
debugger/QT/SkGLWidget.cpp:20: fDebugCanvas = new SkDebugCanvas();
On 2012/07/25 20:51:04, DerekS wrote:
> this should be in the constructor but initialize it to null.

Done.

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkGLWidget.cpp#n...
debugger/QT/SkGLWidget.cpp:21: fCurIntf = GrGLCreateNativeInterface();
On 2012/07/25 20:51:04, DerekS wrote:
> you should unref this in your destructor.

Done.

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkGLWidget.cpp#n...
debugger/QT/SkGLWidget.cpp:22: fCurContext =
GrContext::Create(kOpenGL_Shaders_GrEngine, (GrPlatform3DContext) fCurIntf);
On 2012/07/25 20:51:04, DerekS wrote:
> you should unref this in the destructor.

Done.

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkGLWidget.cpp#n...
debugger/QT/SkGLWidget.cpp:24: GrPlatformRenderTargetDesc desc;
On 2012/07/25 20:51:04, DerekS wrote:
> move the construction of this into a private function so you can share it with
> resizeGL()

Done.

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkGLWidget.cpp#n...
debugger/QT/SkGLWidget.cpp:34: fCurRenderTarget =
fCurContext->createPlatformRenderTarget(desc);
On 2012/07/25 20:51:04, DerekS wrote:
> you don't need to store this as a member variable.

Done.

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkGLWidget.cpp#n...
debugger/QT/SkGLWidget.cpp:35: fGpuDevice = new SkGpuDevice(fCurContext,
fCurRenderTarget);
On 2012/07/25 20:51:04, DerekS wrote:
> you should unref this in the destructor.

Unref or delete? We created fGpuDevice with the new call

https://codereview.appspot.com/6422060/diff/9015/debugger/QT/SkGLWidget.cpp#n...
debugger/QT/SkGLWidget.cpp:54: delete fGpuDevice;
On 2012/07/25 20:51:04, DerekS wrote:
> don't call delete but instead use SkSafeUnref(fGpuDevice)

Done.

[DerekS, 2012-07-26 18:10:16]

http://codereview.appspot.com/6422060/diff/15014/debugger/QT/SkCanvasWidget.h
File debugger/QT/SkCanvasWidget.h (right):

http://codereview.appspot.com/6422060/diff/15014/debugger/QT/SkCanvasWidget.h...
debugger/QT/SkCanvasWidget.h:28: kRaster_8888 	= 1 << 0,
kRaster8888_WidgetType
kGPU_WidgetType

http://codereview.appspot.com/6422060/diff/15014/debugger/QT/SkCanvasWidget.h...
debugger/QT/SkCanvasWidget.h:29: kGPU_Type		= 1 << 1,
don't use tabs use spaces.

http://codereview.appspot.com/6422060/diff/15014/debugger/QT/SkCanvasWidget.h...
debugger/QT/SkCanvasWidget.h:71: void updateWidgetTransform(TransformType type);
who calls this publicly?

[chudy, 2012-07-26 19:22:06]

http://codereview.appspot.com/6422060/diff/15014/debugger/QT/SkCanvasWidget.h
File debugger/QT/SkCanvasWidget.h (right):

http://codereview.appspot.com/6422060/diff/15014/debugger/QT/SkCanvasWidget.h...
debugger/QT/SkCanvasWidget.h:28: kRaster_8888 	= 1 << 0,
On 2012/07/26 18:10:16, DerekS wrote:
> kRaster8888_WidgetType
> kGPU_WidgetType

Done.

http://codereview.appspot.com/6422060/diff/15014/debugger/QT/SkCanvasWidget.h...
debugger/QT/SkCanvasWidget.h:29: kGPU_Type		= 1 << 1,
On 2012/07/26 18:10:16, DerekS wrote:
> don't use tabs use spaces.

Done.

http://codereview.appspot.com/6422060/diff/15014/debugger/QT/SkCanvasWidget.h...
debugger/QT/SkCanvasWidget.h:71: void updateWidgetTransform(TransformType type);
On 2012/07/26 18:10:16, DerekS wrote:
> who calls this publicly?

Moved this and all other incorrectly labeled public functions into private

[DerekS, 2012-07-26 19:25:40]

small nits but otherwise lgtm

http://codereview.appspot.com/6422060/diff/21002/debugger/QT/SkCanvasWidget.h
File debugger/QT/SkCanvasWidget.h (right):

http://codereview.appspot.com/6422060/diff/21002/debugger/QT/SkCanvasWidget.h...
debugger/QT/SkCanvasWidget.h:29: kGPU_WidgetType    = 1 << 1,
align the equals sign

http://codereview.appspot.com/6422060/diff/21002/debugger/QT/SkCanvasWidget.h...
debugger/QT/SkCanvasWidget.h:34: kScale 		= 1 << 1,
fix spacing and make this enum private.

[chudy, 2012-07-26 19:30:25]

Sorry about that, turns out I'm not using a monospace font

http://codereview.appspot.com/6422060/diff/21002/debugger/QT/SkCanvasWidget.h
File debugger/QT/SkCanvasWidget.h (right):

http://codereview.appspot.com/6422060/diff/21002/debugger/QT/SkCanvasWidget.h...
debugger/QT/SkCanvasWidget.h:29: kGPU_WidgetType    = 1 << 1,
On 2012/07/26 19:25:40, DerekS wrote:
> align the equals sign

Done.

http://codereview.appspot.com/6422060/diff/21002/debugger/QT/SkCanvasWidget.h...
debugger/QT/SkCanvasWidget.h:34: kScale 		= 1 << 1,
On 2012/07/26 19:25:40, DerekS wrote:
> fix spacing and make this enum private.

Done.

[chudy, 2012-07-26 19:30:52]

Submitting, thanks for the review!


On Thu, Jul 26, 2012 at 3:30 PM, <chudy@google.com> wrote:

>
> Sorry about that, turns out I'm not using a monospace font
>
>
>
> http://codereview.appspot.com/**6422060/diff/21002/debugger/**
>
QT/SkCanvasWidget.h<http://codereview.appspot.com/6422060/diff/21002/debugger/QT/SkCanvasWidget.h>
> File debugger/QT/SkCanvasWidget.h (right):
>
> http://codereview.appspot.com/**6422060/diff/21002/debugger/**
>
QT/SkCanvasWidget.h#newcode29<http://codereview.appspot.com/6422060/diff/21002/debugger/QT/SkCanvasWidget.h#newcode29>
> debugger/QT/SkCanvasWidget.h:**29: kGPU_WidgetType    = 1 << 1,
> On 2012/07/26 19:25:40, DerekS wrote:
>
>> align the equals sign
>>
>
> Done.
>
>
> http://codereview.appspot.com/**6422060/diff/21002/debugger/**
>
QT/SkCanvasWidget.h#newcode34<http://codereview.appspot.com/6422060/diff/21002/debugger/QT/SkCanvasWidget.h#newcode34>
> debugger/QT/SkCanvasWidget.h:**34: kScale                 = 1 << 1,
> On 2012/07/26 19:25:40, DerekS wrote:
>
>> fix spacing and make this enum private.
>>
>
> Done.
>
>
http://codereview.appspot.com/**6422060/<http://codereview.appspot.com/6422060/>
>



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