Smallest possible desktop application that uses Skia to render stuff.

Smallest possible desktop application that uses Skia to render stuff.

Small Hello World smaple. Subclasses SkOSWindow.

Committed: http://code.google.com/p/skia/source/detail?r=9657

[caryclark, 2013-06-03 19:02:01]

https://codereview.chromium.org/16337012/diff/4002/Makefile
File Makefile (right):

https://codereview.chromium.org/16337012/diff/4002/Makefile#newcode47
Makefile:47: HelloSkia \
This probably doesn't need to change, except possibly to enable the app to be
built on Android. Let's omit this change for now.

https://codereview.chromium.org/16337012/diff/4002/gyp/HelloSkia.gyp
File gyp/HelloSkia.gyp (right):

https://codereview.chromium.org/16337012/diff/4002/gyp/HelloSkia.gyp#newcode8
gyp/HelloSkia.gyp:8: '../helloskia',
please move everything (except for this file) into experimental. You can put the
HelloSkia directory there.

https://codereview.chromium.org/16337012/diff/4002/gyp/most.gyp
File gyp/most.gyp (right):

https://codereview.chromium.org/16337012/diff/4002/gyp/most.gyp#newcode18
gyp/most.gyp:18: 'HelloSkia.gyp:HelloSkia',
Again, probably not needed -- let's omit this change for now

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp
File helloskia/HelloSkia.cpp (right):

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp#ne...
helloskia/HelloSkia.cpp:15: #if HELLOSKIA_USE_GPU
does the GPU version work yet?

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp#ne...
helloskia/HelloSkia.cpp:19: */
Why commented out? If this documentation, include a comment as to why it's here
*/

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp#ne...
helloskia/HelloSkia.cpp:25: SkOSWindow* create_sk_window(void* hwnd, int argc,
char** argv) {
static

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp#ne...
helloskia/HelloSkia.cpp:30: #ifdef SK_BUILD_FOR_MAC
I assume this is if SK_BUILD_FOR_ANDROID

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp#ne...
helloskia/HelloSkia.cpp:49: fBGColor = 0xffffffff;
Use SK_ColorWHITE

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp#ne...
helloskia/HelloSkia.cpp:50: fRotationAngle = 0.0;
Declare fRotationAngle as an SkScalar (if you have not done so already) then use
SkIntToScalar(#) to make the scalars floats or (in the future) doubles

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp#ne...
helloskia/HelloSkia.cpp:59: 
Use SK_ColorBLUE etc

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp#ne...
helloskia/HelloSkia.cpp:62: canvas->drawRect(rect, paint);
I assume you wanted to draw the rect aliased

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp#ne...
helloskia/HelloSkia.cpp:68: canvas->drawCircle(200,200,64,paint);
put spaces after the commas (to be consistent with your SkRect initialization)

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp#ne...
helloskia/HelloSkia.cpp:76: paint.setTextSize(SkIntToScalar(20));
Consider bracketing all SkScalars like this

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp#ne...
helloskia/HelloSkia.cpp:81: fRotationAngle += 0.2;
You can use SkFloatToScalar here

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.h
File helloskia/HelloSkia.h (right):

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.h#newc...
helloskia/HelloSkia.h:28: #endif
Looks great. By the way, have you activated trybots to see what happens on
various platforms? Choose a few that try 32/64, mac/windows, debug/release (but
keep your total count small so the trybots aren't hammered)

[sglez, 2013-06-03 20:24:07]

https://codereview.chromium.org/16337012/diff/4002/Makefile
File Makefile (right):

https://codereview.chromium.org/16337012/diff/4002/Makefile#newcode47
Makefile:47: HelloSkia \
On 2013/06/03 19:02:01, caryclark wrote:
> This probably doesn't need to change, except possibly to enable the app to be
> built on Android. Let's omit this change for now.

Done.

https://codereview.chromium.org/16337012/diff/4002/gyp/HelloSkia.gyp
File gyp/HelloSkia.gyp (right):

https://codereview.chromium.org/16337012/diff/4002/gyp/HelloSkia.gyp#newcode8
gyp/HelloSkia.gyp:8: '../helloskia',
On 2013/06/03 19:02:01, caryclark wrote:
> please move everything (except for this file) into experimental. You can put
the
> HelloSkia directory there.

Done.

https://codereview.chromium.org/16337012/diff/4002/gyp/most.gyp
File gyp/most.gyp (right):

https://codereview.chromium.org/16337012/diff/4002/gyp/most.gyp#newcode18
gyp/most.gyp:18: 'HelloSkia.gyp:HelloSkia',
On 2013/06/03 19:02:01, caryclark wrote:
> Again, probably not needed -- let's omit this change for now

Done.

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp
File helloskia/HelloSkia.cpp (right):

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp#ne...
helloskia/HelloSkia.cpp:15: #if HELLOSKIA_USE_GPU
On 2013/06/03 19:02:01, caryclark wrote:
> does the GPU version work yet?

No, I'm posting the software version for review before implementing that.

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp#ne...
helloskia/HelloSkia.cpp:19: */
On 2013/06/03 19:02:01, caryclark wrote:
> Why commented out? If this documentation, include a comment as to why it's
here
> */

This shouldn't be here, sorry about that.

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp#ne...
helloskia/HelloSkia.cpp:25: SkOSWindow* create_sk_window(void* hwnd, int argc,
char** argv) {
On 2013/06/03 19:02:01, caryclark wrote:
> static


These three functions need outside visibility since they are called from
skia_unix.cpp and skia_mac.cpp

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp#ne...
helloskia/HelloSkia.cpp:30: #ifdef SK_BUILD_FOR_MAC
On 2013/06/03 19:02:01, caryclark wrote:
> I assume this is if SK_BUILD_FOR_ANDROID

I had copied this from SampleApp. 

I just now deleted this check, since I assume that HelloSkia will always be a
Desktop app. SampleApp still looks like this, though

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp#ne...
helloskia/HelloSkia.cpp:49: fBGColor = 0xffffffff;
On 2013/06/03 19:02:01, caryclark wrote:
> Use SK_ColorWHITE

Done.

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp#ne...
helloskia/HelloSkia.cpp:50: fRotationAngle = 0.0;
On 2013/06/03 19:02:01, caryclark wrote:
> Declare fRotationAngle as an SkScalar (if you have not done so already) then
use
> SkIntToScalar(#) to make the scalars floats or (in the future) doubles

Done.

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp#ne...
helloskia/HelloSkia.cpp:59: 
On 2013/06/03 19:02:01, caryclark wrote:
> Use SK_ColorBLUE etc

Done.

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp#ne...
helloskia/HelloSkia.cpp:62: canvas->drawRect(rect, paint);
On 2013/06/03 19:02:01, caryclark wrote:
> I assume you wanted to draw the rect aliased

Yes, it was intentional.

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp#ne...
helloskia/HelloSkia.cpp:68: canvas->drawCircle(200,200,64,paint);
On 2013/06/03 19:02:01, caryclark wrote:
> put spaces after the commas (to be consistent with your SkRect initialization)

Done.

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp#ne...
helloskia/HelloSkia.cpp:81: fRotationAngle += 0.2;
On 2013/06/03 19:02:01, caryclark wrote:
> You can use SkFloatToScalar here

Done.

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.h
File helloskia/HelloSkia.h (right):

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.h#newc...
helloskia/HelloSkia.h:28: #endif
On 2013/06/03 19:02:01, caryclark wrote:
> Looks great. By the way, have you activated trybots to see what happens on
> various platforms? Choose a few that try 32/64, mac/windows, debug/release
(but
> keep your total count small so the trybots aren't hammered)

I will try that, are the docs up-to-date?

[caryclark, 2013-06-04 11:48:12]

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp
File helloskia/HelloSkia.cpp (right):

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp#ne...
helloskia/HelloSkia.cpp:25: SkOSWindow* create_sk_window(void* hwnd, int argc,
char** argv) {
Yes, you're right. I forgot the historic naming convention. In more recent code
xxx_xxx function names are local and therefore static. These functions,
application_init and application_term, got named before that convention was
solidified.

On 2013/06/03 20:24:07, sglez wrote:
> On 2013/06/03 19:02:01, caryclark wrote:
> > static
> 
> 
> These three functions need outside visibility since they are called from
> skia_unix.cpp and skia_mac.cpp

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.h
File helloskia/HelloSkia.h (right):

https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.h#newc...
helloskia/HelloSkia.h:28: #endif
Not sure -- bump Eric if they are not.
On 2013/06/03 20:24:08, sglez wrote:
> On 2013/06/03 19:02:01, caryclark wrote:
> > Looks great. By the way, have you activated trybots to see what happens on
> > various platforms? Choose a few that try 32/64, mac/windows, debug/release
> (but
> > keep your total count small so the trybots aren't hammered)
> 
> I will try that, are the docs up-to-date?

[sglez, 2013-06-04 22:59:36]

Cary, Here is an update to the CL, adding OpenGL support

On 2013/06/04 11:48:12, caryclark wrote:
> https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp
> File helloskia/HelloSkia.cpp (right):
> 
>
https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.cpp#ne...
> helloskia/HelloSkia.cpp:25: SkOSWindow* create_sk_window(void* hwnd, int argc,
> char** argv) {
> Yes, you're right. I forgot the historic naming convention. In more recent
code
> xxx_xxx function names are local and therefore static. These functions,
> application_init and application_term, got named before that convention was
> solidified.
> 
> On 2013/06/03 20:24:07, sglez wrote:
> > On 2013/06/03 19:02:01, caryclark wrote:
> > > static
> > 
> > 
> > These three functions need outside visibility since they are called from
> > skia_unix.cpp and skia_mac.cpp
> 
> https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.h
> File helloskia/HelloSkia.h (right):
> 
>
https://codereview.chromium.org/16337012/diff/4002/helloskia/HelloSkia.h#newc...
> helloskia/HelloSkia.h:28: #endif
> Not sure -- bump Eric if they are not.
> On 2013/06/03 20:24:08, sglez wrote:
> > On 2013/06/03 19:02:01, caryclark wrote:
> > > Looks great. By the way, have you activated trybots to see what happens on
> > > various platforms? Choose a few that try 32/64, mac/windows, debug/release
> > (but
> > > keep your total count small so the trybots aren't hammered)
> > 
> > I will try that, are the docs up-to-date?

[caryclark, 2013-06-05 11:55:33]

Also, I'd like to start using this to create a simple presentation app that
shows Skia examples. Let's figure out how to make this extendable without
requiring that I edit your core code.

https://codereview.chromium.org/16337012/diff/12001/experimental/HelloSkia/He...
File experimental/HelloSkia/HelloSkia.cpp (right):

https://codereview.chromium.org/16337012/diff/12001/experimental/HelloSkia/He...
experimental/HelloSkia/HelloSkia.cpp:65: SkDebugf("Not possible to create GL
backend. Defaulting to raster.");
this should change the state of the runtime switch
should it abort the setup function at this point?

https://codereview.chromium.org/16337012/diff/12001/experimental/HelloSkia/He...
experimental/HelloSkia/HelloSkia.cpp:94: #ifdef SK_HelloSkia_ENABLE_GPU
It looks like the only runtime decision is here (e.g., if gpu ...)

https://codereview.chromium.org/16337012/diff/12001/experimental/HelloSkia/He...
File experimental/HelloSkia/HelloSkia.h (right):

https://codereview.chromium.org/16337012/diff/12001/experimental/HelloSkia/He...
experimental/HelloSkia/HelloSkia.h:20: //#define SK_HelloSkia_ENABLE_GPU
I'd prefer a runtime switch to a compile time one.

https://codereview.chromium.org/16337012/diff/12001/experimental/HelloSkia/He...
experimental/HelloSkia/HelloSkia.h:46: #endif
remove indent

[sglez, 2013-06-05 16:15:31]

Did some refactoring. A presentation app would subclass BaseExample

https://codereview.chromium.org/16337012/diff/12001/experimental/HelloSkia/He...
File experimental/HelloSkia/HelloSkia.cpp (right):

https://codereview.chromium.org/16337012/diff/12001/experimental/HelloSkia/He...
experimental/HelloSkia/HelloSkia.cpp:65: SkDebugf("Not possible to create GL
backend. Defaulting to raster.");
On 2013/06/05 11:55:33, caryclark wrote:
> this should change the state of the runtime switch
> should it abort the setup function at this point?

Yes, this was incomplete. Thanks.

https://codereview.chromium.org/16337012/diff/22001/experimental/SkiaExamples...
File experimental/SkiaExamples/HelloSkiaExample.cpp (right):

https://codereview.chromium.org/16337012/diff/22001/experimental/SkiaExamples...
experimental/SkiaExamples/HelloSkiaExample.cpp:9: // runs on Linux and Mac.
Running buildbots on linux, mac and windows is next on my to-do list.

https://codereview.chromium.org/16337012/diff/22001/experimental/SkiaExamples...
File experimental/SkiaExamples/HelloSkiaExample.h (right):

https://codereview.chromium.org/16337012/diff/22001/experimental/SkiaExamples...
experimental/SkiaExamples/HelloSkiaExample.h:1: #ifndef HelloSkia_DEFINED
Forgot to add copyright header comment

https://codereview.chromium.org/16337012/diff/22001/gyp/experimental.gyp
File gyp/experimental.gyp (right):

https://codereview.chromium.org/16337012/diff/22001/gyp/experimental.gyp#newc...
gyp/experimental.gyp:16: 'SkiaExamples.gyp:HelloSkia'
Is this OK?
This is the way I managed to have a ninja target.

[caryclark, 2013-06-10 14:53:07]

errors when building on Windows

1>------ Build started: Project: HelloSkia, Configuration: Debug Win32 ------
1>  BaseExample.cpp
1>  Presentation.cpp
1>d:\skia\trunk\experimental\skiaexamples\presentation.h(20): error C3668:
'SkPresentation::onEvent' : method with override specifier 'override' did not
override any base class methods
(..\..\gyp\..\experimental\SkiaExamples\Presentation.cpp)
1>d:\skia\trunk\experimental\skiaexamples\presentation.cpp(67): error C2723:
'SkPresentation::onEvent' : 'override' specifier illegal on function definition
1>d:\skia\trunk\experimental\skiaexamples\baseexample.cpp(65): error C2065:
'GL_FRAMEBUFFER_BINDING' : undeclared identifier
2>------ Build started: Project: everything, Configuration: Debug Win32 ------
========== Build: 1 succeeded, 1 failed, 50 up-to-date, 0 skipped ==========

[caryclark, 2013-06-10 15:00:09]

errors when building on Mac


Build target HelloSkia

CompileC
../xcodebuild/SkiaExamples.build/Debug/HelloSkia.build/Objects-normal/i386/Presentation.o
../experimental/SkiaExamples/Presentation.cpp normal i386 c++
com.apple.compilers.llvm.clang.1_0.compiler
    cd /skia/trunk/gyp
    setenv LANG en_US.US-ASCII
   
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang
-x c++ -arch i386 -fmessage-length=0 -Wno-trigraphs -fpascal-strings -O0
-Wno-missing-field-initializers -Wmissing-prototypes -Wreturn-type
-Wno-non-virtual-dtor -Wno-overloaded-virtual -Wno-exit-time-destructors
-Wformat -Wmissing-braces -Wparentheses -Wswitch -Wunused-function
-Wunused-label -Wno-unused-parameter -Wunused-variable -Wunused-value
-Wno-empty-body -Wno-uninitialized -Wunknown-pragmas -Wno-shadow
-Wno-four-char-constants -Wno-conversion -Wno-constant-conversion
-Wno-int-conversion -Wno-enum-conversion -Wsign-compare -Wshorten-64-to-32
-Wnewline-eof -Wno-c++11-extensions -DSK_GAMMA_SRGB -DSK_GAMMA_APPLY_TO_A8
-DSK_ALLOW_STATIC_GLOBAL_INITIALIZERS=1 -DSK_SCALAR_IS_FLOAT -DSK_CAN_USE_FLOAT
-DSK_SUPPORT_GPU=1 -DSK_BUILD_FOR_MAC -DSK_USE_POSIX_THREADS -DGR_MAC_BUILD=1
-DSK_DEBUG -DGR_DEBUG=1 -DSK_DEVELOPER=1 -isysroot
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.7.sdk
-fasm-blocks -fstrict-aliasing -Wdeprecated-declarations -Winvalid-offsetof
-mmacosx-version-min=10.7 -g -fvisibility-inlines-hidden -Wno-sign-conversion
-I/skia/trunk/gyp/../xcodebuild/SkiaExamples.build/Debug/HelloSkia.build/HelloSkia.hmap
-I/skia/trunk/gyp/../xcodebuild/Debug/include -I../experimental/SkiaExamples
-I../src/gpu -Iconfig -I../include/config -I../include/core -I../include/lazy
-I../include/pathops -I../include/pipe -Iext -I../include/utils/mac
-I../third_party/freetype/include -I../include/effects -I../include/images
-I../third_party/externals/libjpeg -I../include/ports -I../src/sfnt
-I../include/utils -I../include/gpu -I../include/views -I../include/xml
-I/skia/trunk/gyp/../xcodebuild/SkiaExamples.build/Debug/HelloSkia.build/DerivedSources/i386
-I/skia/trunk/gyp/../xcodebuild/SkiaExamples.build/Debug/HelloSkia.build/DerivedSources
-F/skia/trunk/gyp/../xcodebuild/Debug -mssse3 -fvisibility=hidden
-fvisibility-inlines-hidden -Werror -MMD -MT dependencies -MF
/skia/trunk/gyp/../xcodebuild/SkiaExamples.build/Debug/HelloSkia.build/Objects-normal/i386/Presentation.d
--serialize-diagnostics
/skia/trunk/gyp/../xcodebuild/SkiaExamples.build/Debug/HelloSkia.build/Objects-normal/i386/Presentation.dia
-c /skia/trunk/gyp/../experimental/SkiaExamples/Presentation.cpp -o
/skia/trunk/gyp/../xcodebuild/SkiaExamples.build/Debug/HelloSkia.build/Objects-normal/i386/Presentation.o

In file included from
/skia/trunk/gyp/../experimental/SkiaExamples/Presentation.cpp:10:
../experimental/SkiaExamples/Presentation.h:20:22: error: 'onEvent' marked
'override' but does not override any member functions
        virtual void onEvent(SkEvent* evt) SK_OVERRIDE;
                     ^
/skia/trunk/gyp/../experimental/SkiaExamples/Presentation.cpp:38:13: error: no
previous prototype for function 'create_sk_window'
[-Werror,-Wmissing-prototypes]
SkOSWindow* create_sk_window(void* hwnd, int argc, char** argv) {
            ^
/skia/trunk/gyp/../experimental/SkiaExamples/Presentation.cpp:42:6: error: no
previous prototype for function 'application_init'
[-Werror,-Wmissing-prototypes]
void application_init() {
     ^
/skia/trunk/gyp/../experimental/SkiaExamples/Presentation.cpp:47:6: error: no
previous prototype for function 'application_term'
[-Werror,-Wmissing-prototypes]
void application_term() {
     ^
/skia/trunk/gyp/../experimental/SkiaExamples/Presentation.cpp:67:44: error:
expected function body after function declarator
void SkPresentation::onEvent(SkEvent* evt) SK_OVERRIDE {
                                           ^
../include/core/SkPostConfig.h:336:37: note: expanded from macro 'SK_OVERRIDE'
                #define SK_OVERRIDE override
                                    ^
5 errors generated.

CompileC
../xcodebuild/SkiaExamples.build/Debug/HelloSkia.build/Objects-normal/i386/BaseExample.o
../experimental/SkiaExamples/BaseExample.cpp normal i386 c++
com.apple.compilers.llvm.clang.1_0.compiler
    cd /skia/trunk/gyp
    setenv LANG en_US.US-ASCII
   
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang
-x c++ -arch i386 -fmessage-length=0 -Wno-trigraphs -fpascal-strings -O0
-Wno-missing-field-initializers -Wmissing-prototypes -Wreturn-type
-Wno-non-virtual-dtor -Wno-overloaded-virtual -Wno-exit-time-destructors
-Wformat -Wmissing-braces -Wparentheses -Wswitch -Wunused-function
-Wunused-label -Wno-unused-parameter -Wunused-variable -Wunused-value
-Wno-empty-body -Wno-uninitialized -Wunknown-pragmas -Wno-shadow
-Wno-four-char-constants -Wno-conversion -Wno-constant-conversion
-Wno-int-conversion -Wno-enum-conversion -Wsign-compare -Wshorten-64-to-32
-Wnewline-eof -Wno-c++11-extensions -DSK_GAMMA_SRGB -DSK_GAMMA_APPLY_TO_A8
-DSK_ALLOW_STATIC_GLOBAL_INITIALIZERS=1 -DSK_SCALAR_IS_FLOAT -DSK_CAN_USE_FLOAT
-DSK_SUPPORT_GPU=1 -DSK_BUILD_FOR_MAC -DSK_USE_POSIX_THREADS -DGR_MAC_BUILD=1
-DSK_DEBUG -DGR_DEBUG=1 -DSK_DEVELOPER=1 -isysroot
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.7.sdk
-fasm-blocks -fstrict-aliasing -Wdeprecated-declarations -Winvalid-offsetof
-mmacosx-version-min=10.7 -g -fvisibility-inlines-hidden -Wno-sign-conversion
-I/skia/trunk/gyp/../xcodebuild/SkiaExamples.build/Debug/HelloSkia.build/HelloSkia.hmap
-I/skia/trunk/gyp/../xcodebuild/Debug/include -I../experimental/SkiaExamples
-I../src/gpu -Iconfig -I../include/config -I../include/core -I../include/lazy
-I../include/pathops -I../include/pipe -Iext -I../include/utils/mac
-I../third_party/freetype/include -I../include/effects -I../include/images
-I../third_party/externals/libjpeg -I../include/ports -I../src/sfnt
-I../include/utils -I../include/gpu -I../include/views -I../include/xml
-I/skia/trunk/gyp/../xcodebuild/SkiaExamples.build/Debug/HelloSkia.build/DerivedSources/i386
-I/skia/trunk/gyp/../xcodebuild/SkiaExamples.build/Debug/HelloSkia.build/DerivedSources
-F/skia/trunk/gyp/../xcodebuild/Debug -mssse3 -fvisibility=hidden
-fvisibility-inlines-hidden -Werror -MMD -MT dependencies -MF
/skia/trunk/gyp/../xcodebuild/SkiaExamples.build/Debug/HelloSkia.build/Objects-normal/i386/BaseExample.d
--serialize-diagnostics
/skia/trunk/gyp/../xcodebuild/SkiaExamples.build/Debug/HelloSkia.build/Objects-normal/i386/BaseExample.dia
-c /skia/trunk/gyp/../experimental/SkiaExamples/BaseExample.cpp -o
/skia/trunk/gyp/../xcodebuild/SkiaExamples.build/Debug/HelloSkia.build/Objects-normal/i386/BaseExample.o

/skia/trunk/gyp/../experimental/SkiaExamples/BaseExample.cpp:65:33: error: use
of undeclared identifier 'GL_FRAMEBUFFER_BINDING'
        interface->fGetIntegerv(GL_FRAMEBUFFER_BINDING, &buffer);
                                ^
1 error generated.

Ld ../xcodebuild/Debug/HelloSkia.app/Contents/MacOS/HelloSkia normal i386
    cd /skia/trunk/gyp
    setenv MACOSX_DEPLOYMENT_TARGET 10.7
   
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++
-arch i386 -isysroot
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.7.sdk
-L/skia/trunk/gyp/../xcodebuild/Debug
-L/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.7.sdk/System/Library/Frameworks
-F/skia/trunk/gyp/../xcodebuild/Debug -filelist
/skia/trunk/gyp/../xcodebuild/SkiaExamples.build/Debug/HelloSkia.build/Objects-normal/i386/HelloSkia.LinkFileList
-mmacosx-version-min=10.7 /skia/trunk/xcodebuild/Debug/libskia_views.a
/skia/trunk/xcodebuild/Debug/libskia_xml.a
/skia/trunk/xcodebuild/Debug/libskia_core.a
/skia/trunk/xcodebuild/Debug/libskia_effects.a
/skia/trunk/xcodebuild/Debug/libskia_images.a
/skia/trunk/xcodebuild/Debug/libjpeg.a
/skia/trunk/xcodebuild/Debug/libwebp_dec.a
/skia/trunk/xcodebuild/Debug/libwebp_dsp.a
/skia/trunk/xcodebuild/Debug/libwebp_enc.a
/skia/trunk/xcodebuild/Debug/libwebp_utils.a
/skia/trunk/xcodebuild/Debug/libskia_utils.a
/skia/trunk/xcodebuild/Debug/libskia_opts.a
/skia/trunk/xcodebuild/Debug/libskia_opts_ssse3.a
/skia/trunk/xcodebuild/Debug/libskia_ports.a
/skia/trunk/xcodebuild/Debug/libskia_sfnt.a
/skia/trunk/xcodebuild/Debug/libskia_gr.a
/skia/trunk/xcodebuild/Debug/libskia_skgr.a -framework ApplicationServices
-framework AGL -framework OpenGL -framework Cocoa -framework Foundation -o
/skia/trunk/gyp/../xcodebuild/Debug/HelloSkia.app/Contents/MacOS/HelloSkia

ld: file not found:
/skia/trunk/gyp/../xcodebuild/SkiaExamples.build/Debug/HelloSkia.build/Objects-normal/i386/Presentation.o
clang: error: linker command failed with exit code 1 (use -v to see invocation)

[caryclark, 2013-06-10 15:48:40]

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
File experimental/SkiaExamples/BaseExample.cpp (right):

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.cpp:23: fType = kRaster_DeviceType;
setupBackend also sets fType -- pick one or the other

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.cpp:29: if (fType == kGPU_DeviceType) {
This style is OK and we (Skia) are not very consistent about it, but

if (kGPU_DeviceType == fType) {

allows detecting this error consistently:

if (kGPU_DeviceType = fType) { // flags an assignment error on all compilers
if (fType = kGPU_DeviceType) { // may escape notice on some compilers

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.cpp:38: if(type == kGPU_DeviceType) {
for long blocks like this, I'd rather see

if (kGPU_DeviceType != type) {
    return;
}
AttachmentInfo attachmentInfo;
...

to minimize the size of large indentations

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.cpp:46: return;
does the caller need to know that this failed? (e.g., should setupBackend return
bool to indicate success?)

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.cpp:50: fInterface =
const_cast<GrGLInterface*>(interface);
Throwing away a const is a red flag. Either the interface is broken or this
usage is. At minimum, this requires a comment to show why you think casting is
OK.

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.cpp:76: NULL != fContext && NULL !=
fRenderTarget) {
indent by 4 more so it doesn't look like the body of the if

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.cpp:76: NULL != fContext && NULL !=
fRenderTarget) {
When can fContext be NULL?

If fRenderTarget returned NULL above, should the fType have been set to
kRaster_DeviceType?

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.cpp:86: present();
When draw() is overridden by a child, should present() be called before or after
the other drawing?

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
File experimental/SkiaExamples/BaseExample.h (right):

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.h:27: ~BaseExample();
This hides the virtual destructor for SkOSWIndows, which you don't want to do.

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.h:43: 
why two blank lines?

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
File experimental/SkiaExamples/HelloSkiaExample.cpp (right):

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/HelloSkiaExample.cpp:50: SkRect rect = {10, 10, 128,
128};
these need SkIntToScalar functions

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/HelloSkiaExample.cpp:68: canvas->drawCircle(200, 200,
64, paint);
SkIntToScalar

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/HelloSkiaExample.cpp:104: if (getDeviceType() ==
kGPU_DeviceType) {
since the base class does this, call it here

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
File experimental/SkiaExamples/Presentation.cpp (right):

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/Presentation.cpp:45: }
can this go into base so it isn't repeated in children? (ditto with other
repeats)

https://codereview.chromium.org/16337012/diff/29001/gyp/SkiaExamples.gyp
File gyp/SkiaExamples.gyp (right):

https://codereview.chromium.org/16337012/diff/29001/gyp/SkiaExamples.gyp#newc...
gyp/SkiaExamples.gyp:4: 'target_name': 'HelloSkia',
seems inconsistent to name this here and comment it out below

[sglez, 2013-06-11 04:24:38]

Cary, here are some corrections for the code review and some new code to create
a XCode project. It is not working on OSX yet.

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
File experimental/SkiaExamples/BaseExample.cpp (right):

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.cpp:23: fType = kRaster_DeviceType;
On 2013/06/10 15:48:40, caryclark wrote:
> setupBackend also sets fType -- pick one or the other

Removed this and also the call to setupBackend below, to avoid calling it twice
by subclasses calling this constructor.

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.cpp:29: if (fType == kGPU_DeviceType) {
Actually I have been trying to keep to that style, force of habit

On 2013/06/10 15:48:40, caryclark wrote:
> This style is OK and we (Skia) are not very consistent about it, but
> 
> if (kGPU_DeviceType == fType) {
> 
> allows detecting this error consistently:
> 
> if (kGPU_DeviceType = fType) { // flags an assignment error on all compilers
> if (fType = kGPU_DeviceType) { // may escape notice on some compilers

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.cpp:46: return;
On 2013/06/10 15:48:40, caryclark wrote:
> does the caller need to know that this failed? (e.g., should setupBackend
return
> bool to indicate success?)

The caller would probably like to know this. 
Changed to return a bool indicating success or failure

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.cpp:50: fInterface =
const_cast<GrGLInterface*>(interface);
On 2013/06/10 15:48:40, caryclark wrote:
> Throwing away a const is a red flag. Either the interface is broken or this
> usage is. At minimum, this requires a comment to show why you think casting is
> OK.

Sorry about the const_cast, fixed by setting the class member to const.

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.cpp:76: NULL != fContext && NULL !=
fRenderTarget) {
On 2013/06/10 15:48:40, caryclark wrote:
> indent by 4 more so it doesn't look like the body of the if

Done.

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.cpp:76: NULL != fContext && NULL !=
fRenderTarget) {
On 2013/06/10 15:48:40, caryclark wrote:
> When can fContext be NULL?
> 
> If fRenderTarget returned NULL above, should the fType have been set to
> kRaster_DeviceType?

The fContext is just a precaution. 
You are right. fType is now changed here.

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.cpp:86: present();
On 2013/06/10 15:48:40, caryclark wrote:
> When draw() is overridden by a child, should present() be called before or
after
> the other drawing?
 present() should be called at the end. It is a buffer swap when using GL.

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
File experimental/SkiaExamples/BaseExample.h (right):

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.h:27: ~BaseExample();
On 2013/06/10 15:48:40, caryclark wrote:
> This hides the virtual destructor for SkOSWIndows, which you don't want to do.

Thanks,
Refactored into a tearDownBackend to use when we add the feature to change
backends during runtime.

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.h:43: 
On 2013/06/10 15:48:40, caryclark wrote:
> why two blank lines?

Corrected.

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
File experimental/SkiaExamples/HelloSkiaExample.cpp (right):

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/HelloSkiaExample.cpp:50: SkRect rect = {10, 10, 128,
128};
On 2013/06/10 15:48:40, caryclark wrote:
> these need SkIntToScalar functions

Thanks; did a regex substitution on the file.

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/HelloSkiaExample.cpp:104: if (getDeviceType() ==
kGPU_DeviceType) {
On 2013/06/10 15:48:40, caryclark wrote:
> since the base class does this, call it here

Done.

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
File experimental/SkiaExamples/Presentation.cpp (right):

https://codereview.chromium.org/16337012/diff/29001/experimental/SkiaExamples...
experimental/SkiaExamples/Presentation.cpp:45: }
On 2013/06/10 15:48:40, caryclark wrote:
> can this go into base so it isn't repeated in children? (ditto with other
> repeats)

Done. (But just for the record, defining these functions for every example could
come in handy one day if someone wants to do something besides starting and
stopping subsystems)

https://codereview.chromium.org/16337012/diff/29001/gyp/SkiaExamples.gyp
File gyp/SkiaExamples.gyp (right):

https://codereview.chromium.org/16337012/diff/29001/gyp/SkiaExamples.gyp#newc...
gyp/SkiaExamples.gyp:4: 'target_name': 'HelloSkia',
On 2013/06/10 15:48:40, caryclark wrote:
> seems inconsistent to name this here and comment it out below

Changed the target name to SkiaExamples. Also removed the comment below.

https://codereview.chromium.org/16337012/diff/36002/src/views/mac/SkSampleNSV...
File src/views/mac/SkSampleNSView.mm (right):

https://codereview.chromium.org/16337012/diff/36002/src/views/mac/SkSampleNSV...
src/views/mac/SkSampleNSView.mm:19: fWind = create_sk_window(self,
*_NSGetArgc(), *_NSGetArgv());
This seemed like the right thing to do, since Linux and Windows call this
function instead of directly creating a SampleWindow

[sglez, 2013-06-11 22:51:43]

So, OSX is still on the way, but here is a commit that makes SkiaExamples
support resizing.

[caryclark, 2013-06-12 16:03:46]

On 2013/06/11 22:51:43, sglez wrote:
> So, OSX is still on the way, but here is a commit that makes SkiaExamples
> support resizing.

Unfortunately resizing still doesn't work (for reasons that aren't your fault)
If the draw calls inval as often as possible, the resize event is never
received.
If the app is built to do raster drawing instead of gpu drawing, the resize
event isn't posted.

These restrictions are (probably) Linux-specific. Ben Wagoner (bungeman) has
some ideas about how to fix them.

[sglez, 2013-06-12 18:01:42]

Updated the gyp file for windows. It compiles, runs and seems to work fine on
Nate's thinkpad.

[sglez, 2013-06-13 00:47:15]

Hi all,
Here are some thoughts on the resizing issue, along with a workaround patch.

Right now what my app is doing to do animation is to call inval() at draw time.

This causes problems on Linux:

[unix_main.cpp]
    [... inside main]
    // drain any events that occurred before gWindow was assigned.
    while (SkEvent::ProcessEvent());
 
    gWindow->loop();

    delete gWindow;
    application_term();
    return 0;

Here is what I have figured out so far:
There is an inval event on the queue by default. 
The program goes into the while (SkEvent::ProcessEvent()); loop.
Inside the loop, it sees the inval and calls draw(), which calls inval(), which
calls draw(), and so on...

We might get out of this loop if by some chance the inval event takes long
enough to propagate so that the SkEvent queue is empty. 
If/when that happens, the app will resize.

The resizing is very, very slow, I don't want to make assumptions here, but it
might not have to do with the way events are handled. I think it is more likely
that resizing is slow because every resize event is triggering the creation of a
new render context.

I was looking at how SampleApp deals with animation and I think that it is a
good workaround.
I implemented something similar in this patch:

There is a new function: postRedraw(),that essentially posts an inval message
with a delay.
There is another function, setDelay(ms). The default is 16ms.

The 16ms delay is enough to take us out of the while loop and to make resizing
not take forever.

This is not really a fix, but it is easier and less error prone than to
implement a priority queue or to make things asynchronous.

Cary, can you try replacing calls to inval() with calls to postRedraw() and see
if that helps?

I hope the email wasn't too long, I'll see you tomorrow so that you can tell me
how wrong I am =)

[sglez, 2013-06-13 19:57:04]

Here are the latest changes. (CC'ing Mike)

Removed the Presentation class.
Turned HelloSkia.[cpp|h] into HelloSkia.cpp.
Dropped experimental.gyp changes.

[sglez, 2013-06-14 18:11:49]

Cary,
Here's a patch that seems to fix the slowdowns on Windows.

[caryclark, 2013-06-17 15:08:39]

https://codereview.chromium.org/16337012/diff/95001/experimental/SkiaExamples...
File experimental/SkiaExamples/BaseExample.cpp (right):

https://codereview.chromium.org/16337012/diff/95001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.cpp:92: if (fType == kGPU_DeviceType &&
note if (fType ... ) here ... but below ...

https://codereview.chromium.org/16337012/diff/95001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.cpp:102: }
I would rewrite this as:

if (fType == kGPU_DeviceType) {
  if (NULL != fContext ... {
    ...
    return newSkCanvas(device);
  }
  tearDownBackend();
  ...
}
return INHERITED::createCanvas();

My rationale is that there's less to parse if duplicate conditionals are removed
(both the if () and the else)

https://codereview.chromium.org/16337012/diff/95001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.cpp:106: if (getDeviceType() ==
kGPU_DeviceType) {
if (getDeviceType() ... here.
Choose one or the other. (I prefer if (fType ... since it's private and then I
don't have to go look at getDeviceType() to see if it does anything tricky)

https://codereview.chromium.org/16337012/diff/95001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.cpp:110: if (kRaster_DeviceType ==
getDeviceType()) {
reverse compare order

https://codereview.chromium.org/16337012/diff/95001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.cpp:127: fDelayMs = ms;
One line implementations might as well live in the .h file

https://codereview.chromium.org/16337012/diff/95001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.cpp:138: Sleep(fDelayMs);
This suggests that one thread should be doing computation for the next frame
since this thread will always wait 1/60th of a second. Is this your intent?

https://codereview.chromium.org/16337012/diff/95001/experimental/SkiaExamples...
File experimental/SkiaExamples/HelloSkiaExample.cpp (right):

https://codereview.chromium.org/16337012/diff/95001/experimental/SkiaExamples...
experimental/SkiaExamples/HelloSkiaExample.cpp:26:
setupBackend(kGPU_DeviceType);
add comment that caller can also use kRaster_DeviceType instead

https://codereview.chromium.org/16337012/diff/95001/experimental/SkiaExamples...
experimental/SkiaExamples/HelloSkiaExample.cpp:27: }
add a blank line

https://codereview.chromium.org/16337012/diff/95001/experimental/SkiaExamples...
experimental/SkiaExamples/HelloSkiaExample.cpp:34: paint.setColor(SK_ColorRED); 
// SkColor is ARGB
not sure the comment clarifies

https://codereview.chromium.org/16337012/diff/95001/experimental/SkiaExamples...
experimental/SkiaExamples/HelloSkiaExample.cpp:37: SkRect rect =
{SkIntToScalar(10), SkIntToScalar(10), SkIntToScalar(128), SkIntToScalar(128)};
too wide (rewrap to fit in 100 cols)

https://codereview.chromium.org/16337012/diff/95001/experimental/SkiaExamples...
experimental/SkiaExamples/HelloSkiaExample.cpp:50: linearPoints, linearColors,
NULL, 2, SkShader::kMirror_TileMode, linearMapper);
too wide (rewrap to fit in 100 cols)

https://codereview.chromium.org/16337012/diff/95001/experimental/SkiaExamples...
experimental/SkiaExamples/HelloSkiaExample.cpp:56:
canvas->drawCircle(SkIntToScalar(200), SkIntToScalar(200), SkIntToScalar(64),
paint);
too wide (rewrap to fit in 100 cols)

https://codereview.chromium.org/16337012/diff/95001/experimental/SkiaExamples...
experimental/SkiaExamples/HelloSkiaExample.cpp:84: canvas->drawText(message,
strlen(message), SkIntToScalar(0), SkIntToScalar(0), paint);
sizeof(message) - 1 would also work

https://codereview.chromium.org/16337012/diff/95001/experimental/SkiaExamples...
experimental/SkiaExamples/HelloSkiaExample.cpp:90: canvas->flush();
Do you need this? It shouldn't normally be required

https://codereview.chromium.org/16337012/diff/95001/experimental/SkiaExamples...
experimental/SkiaExamples/HelloSkiaExample.cpp:101: SkOSWindow*
create_sk_window(void* hwnd, int argc, char** argv) {
It would be great if I could switch between multiple examples trivially. Right
now, I comment out all but the one I want to run. 

Maybe this could be a macro in BaseExample.h to make this easier?

Skia's unit tests and others have a test class that is registered with a static
initializer so that all the tests can live simultaneously, and then the desired
test is selected by an argument to command line. I'm not suggesting we do this
now, but something like that would allow this to be scalable.

https://codereview.chromium.org/16337012/diff/95001/src/views/unix/skia_unix.cpp
File src/views/unix/skia_unix.cpp (right):

https://codereview.chromium.org/16337012/diff/95001/src/views/unix/skia_unix....
src/views/unix/skia_unix.cpp:29: // Start normal Skia sequence
A comment as to why the init() needs to be called before creating the window
would be great

[sglez, 2013-06-17 18:04:35]

https://codereview.chromium.org/16337012/diff/95001/experimental/SkiaExamples...
File experimental/SkiaExamples/BaseExample.cpp (right):

https://codereview.chromium.org/16337012/diff/95001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.cpp:102: }
On 2013/06/17 15:08:39, caryclark wrote:
> I would rewrite this as:
> 
> if (fType == kGPU_DeviceType) {
>   if (NULL != fContext ... {
>     ...
>     return newSkCanvas(device);
>   }
>   tearDownBackend();
>   ...
> }
> return INHERITED::createCanvas();
> 
> My rationale is that there's less to parse if duplicate conditionals are
removed
> (both the if () and the else)

The else is still needed to handle a nil context or render target, but yes, this
is cleaner.

https://codereview.chromium.org/16337012/diff/95001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.cpp:127: fDelayMs = ms;
On 2013/06/17 15:08:39, caryclark wrote:
> One line implementations might as well live in the .h file

Mike's changes make my delay fix obsolete so I'm taking this out.

https://codereview.chromium.org/16337012/diff/95001/experimental/SkiaExamples...
experimental/SkiaExamples/BaseExample.cpp:138: Sleep(fDelayMs);
On 2013/06/17 15:08:39, caryclark wrote:
> This suggests that one thread should be doing computation for the next frame
> since this thread will always wait 1/60th of a second. Is this your intent?

The intent was to mimic the Linux behavior by making the app sleep when
invalidated. I took the call out. 
The rest of this function is still needed to fix the choppiness on Windows.

https://codereview.chromium.org/16337012/diff/95001/experimental/SkiaExamples...
File experimental/SkiaExamples/HelloSkiaExample.cpp (right):

https://codereview.chromium.org/16337012/diff/95001/experimental/SkiaExamples...
experimental/SkiaExamples/HelloSkiaExample.cpp:84: canvas->drawText(message,
strlen(message), SkIntToScalar(0), SkIntToScalar(0), paint);
On 2013/06/17 15:08:39, caryclark wrote:
> sizeof(message) - 1 would also work

Is that a suggestion or just a note? strlen seems to be the convention.

https://codereview.chromium.org/16337012/diff/95001/experimental/SkiaExamples...
experimental/SkiaExamples/HelloSkiaExample.cpp:101: SkOSWindow*
create_sk_window(void* hwnd, int argc, char** argv) {
On 2013/06/17 15:08:39, caryclark wrote:
> It would be great if I could switch between multiple examples trivially. Right
> now, I comment out all but the one I want to run. 
> 
> Maybe this could be a macro in BaseExample.h to make this easier?
> 
> Skia's unit tests and others have a test class that is registered with a
static
> initializer so that all the tests can live simultaneously, and then the
desired
> test is selected by an argument to command line. I'm not suggesting we do this
> now, but something like that would allow this to be scalable.

Perhaps do this on a new CL after this one is submitted?

https://codereview.chromium.org/16337012/diff/117002/src/views/unix/SkOSWindo...
File src/views/unix/SkOSWindow_Unix.cpp (right):

https://codereview.chromium.org/16337012/diff/117002/src/views/unix/SkOSWindo...
src/views/unix/SkOSWindow_Unix.cpp:380: }
This is a fix to Mike's recent changes; it fixes a screen flickering problem.
The XPutImage call causes the problem when drawing with GL so we return early.

[caryclark, 2013-06-17 20:11:16]

Looks really good. I found a couple of nits, but once you've run the trybots, I
think we're ready to check it in

https://codereview.chromium.org/16337012/diff/117002/experimental/SkiaExample...
File experimental/SkiaExamples/BaseExample.cpp (right):

https://codereview.chromium.org/16337012/diff/117002/experimental/SkiaExample...
experimental/SkiaExamples/BaseExample.cpp:56: SkDebugf("Not possible to create
backend.");
add a \n to the end

https://codereview.chromium.org/16337012/diff/117002/experimental/SkiaExample...
experimental/SkiaExamples/BaseExample.cpp:96: } else{
the else isn't needed, but if you feel strongly about it, at least put a space
after it

https://codereview.chromium.org/16337012/diff/117002/experimental/SkiaExample...
experimental/SkiaExamples/BaseExample.cpp:119: present();
INHERITED::present()

[commit-bot: I haz the power, 2013-06-18 15:20:49]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[caryclark, 2013-06-18 15:29:34]

lgtm

[caryclark, 2013-06-18 15:29:35]

lgtm

[commit-bot: I haz the power, 2013-06-18 15:30:51]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/sglez@google.com/16337012/141001

[commit-bot: I haz the power, 2013-06-18 15:38:53]

Change committed as 9657