Refactored OpenGL embedder that works on Android, Mac or Linux.

Refactored OpenGL embedder that works on Android, Mac or Linux.

To build on/for linux: tools/build.py -a x64 samples
To build on/for Mac: tools/build.py -a ia32 samples
To build on Linux for Android: tools/bui8ld.py --os=android samples

Note that to run on a Mac, you need to delete the first line of the
fragment shader in raytrace.dart, that says:

  precision mediump float;
Committed: https://code.google.com/p/dart/source/detail?r=17161

[gram, 2013-01-14 18:23:54]

Happy Monday ;-)

[vsm, 2013-01-14 21:31:40]

Nice!

https://chromiumcodereview.appspot.com/11883013/diff/12006/dart.gyp
File dart.gyp (right):

https://chromiumcodereview.appspot.com/11883013/diff/12006/dart.gyp#newcode124
dart.gyp:124: },
Note, if you add an extra {} clause, it's the else clause.  You might hoist
sample_extension up here (basically if OS!="android").

https://chromiumcodereview.appspot.com/11883013/diff/12006/dart.gyp#newcode132
dart.gyp:132: ['OS=="mac"', {
I see "and", but not "or" in the chrome gyp files.  You should be able to
combine this with the below as:
['OS!="win" and OS!="android"', { ...

https://chromiumcodereview.appspot.com/11883013/diff/12006/runtime/dart-runti...
File runtime/dart-runtime.gyp (right):

https://chromiumcodereview.appspot.com/11883013/diff/12006/runtime/dart-runti...
runtime/dart-runtime.gyp:11:
'embedders/openglui/emulator/emulator_embedder.gypi',
Can we have just one openglui gypi to keep things terser here?

https://chromiumcodereview.appspot.com/11883013/diff/12006/runtime/embedders/...
File runtime/embedders/openglui/common/context.h (right):

https://chromiumcodereview.appspot.com/11883013/diff/12006/runtime/embedders/...
runtime/embedders/openglui/common/context.h:1: // Copyright (c) 2012, the Dart
project authors.  Please see the AUTHORS file
New files should be 2013.  :-)

https://chromiumcodereview.appspot.com/11883013/diff/12006/runtime/embedders/...
File runtime/embedders/openglui/common/events.h (right):

https://chromiumcodereview.appspot.com/11883013/diff/12006/runtime/embedders/...
runtime/embedders/openglui/common/events.h:20: } lifecycle_event_t;
We should change this to LifeCycleEvent to match style rules:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Type_N...

https://chromiumcodereview.appspot.com/11883013/diff/12006/runtime/embedders/...
runtime/embedders/openglui/common/events.h:26: } key_event_t;
ditto

https://chromiumcodereview.appspot.com/11883013/diff/12006/runtime/embedders/...
runtime/embedders/openglui/common/events.h:36: } motion_event_t;
ditto

https://chromiumcodereview.appspot.com/11883013/diff/12006/runtime/embedders/...
File runtime/embedders/openglui/common/isized.h (right):

https://chromiumcodereview.appspot.com/11883013/diff/12006/runtime/embedders/...
runtime/embedders/openglui/common/isized.h:8: class ISized {
Can you add a comment on this as to what it is and why it's factored out?

https://chromiumcodereview.appspot.com/11883013/diff/12006/runtime/embedders/...
File runtime/embedders/openglui/common/opengl.h (right):

https://chromiumcodereview.appspot.com/11883013/diff/12006/runtime/embedders/...
runtime/embedders/openglui/common/opengl.h:14: #  endif
Why the funny spacing with '#'?  It is inconsistent with the below.

Looks like either style is ok:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Type_N...
but we should be consistent.

https://chromiumcodereview.appspot.com/11883013/diff/12006/samples/android_sa...
File samples/android_sample/assets/dart/main.dart (right):

https://chromiumcodereview.appspot.com/11883013/diff/12006/samples/android_sa...
samples/android_sample/assets/dart/main.dart:8: //precision mediump float;
Delete the above.

https://chromiumcodereview.appspot.com/11883013/diff/12006/samples/android_sa...
samples/android_sample/assets/dart/main.dart:330:
//playBackground("music/la_ere_gymnopedie.mp3");
Either delete or add TODO to restore.  Nit: space after the "//".

https://chromiumcodereview.appspot.com/11883013/diff/12006/samples/android_sa...
File samples/android_sample/src/com/google/dartndk/DummyActivity.java (left):

https://chromiumcodereview.appspot.com/11883013/diff/12006/samples/android_sa...
samples/android_sample/src/com/google/dartndk/DummyActivity.java:20: 
Nice!

https://chromiumcodereview.appspot.com/11883013/diff/12006/samples/mobile_emu...
File samples/mobile_emulator_sample/main.dart (right):

https://chromiumcodereview.appspot.com/11883013/diff/12006/samples/mobile_emu...
samples/mobile_emulator_sample/main.dart:5: import "dart:math" as Math;
Is this the same as the file in the android sample?  How hard would it be to
merge the two?  It'd be nice to just have the same sample that can be deployed
different ways.

https://chromiumcodereview.appspot.com/11883013/diff/12006/samples/mobile_emu...
samples/mobile_emulator_sample/main.dart:8: //precision mediump float;
delete

[gram, 2013-01-16 01:49:16]

PTAL. Sorry for the multiple patch sets but I keep getting SSL timeouts on
uploads.

https://chromiumcodereview.appspot.com/11883013/diff/12006/dart.gyp
File dart.gyp (right):

https://chromiumcodereview.appspot.com/11883013/diff/12006/dart.gyp#newcode124
dart.gyp:124: },
On 2013/01/14 21:31:40, vsm wrote:
> Note, if you add an extra {} clause, it's the else clause.  You might hoist
> sample_extension up here (basically if OS!="android").

I saw that but I find it more readable expanded out.

https://chromiumcodereview.appspot.com/11883013/diff/12006/dart.gyp#newcode132
dart.gyp:132: ['OS=="mac"', {
On 2013/01/14 21:31:40, vsm wrote:
> I see "and", but not "or" in the chrome gyp files.  You should be able to
> combine this with the below as:
> ['OS!="win" and OS!="android"', { ...

I used or elsewhere and it seemed to work so I'll try the same here.

https://chromiumcodereview.appspot.com/11883013/diff/12006/runtime/dart-runti...
File runtime/dart-runtime.gyp (right):

https://chromiumcodereview.appspot.com/11883013/diff/12006/runtime/dart-runti...
runtime/dart-runtime.gyp:11:
'embedders/openglui/emulator/emulator_embedder.gypi',
On 2013/01/14 21:31:40, vsm wrote:
> Can we have just one openglui gypi to keep things terser here?

Done.

https://chromiumcodereview.appspot.com/11883013/diff/12006/runtime/embedders/...
File runtime/embedders/openglui/common/context.h (right):

https://chromiumcodereview.appspot.com/11883013/diff/12006/runtime/embedders/...
runtime/embedders/openglui/common/context.h:1: // Copyright (c) 2012, the Dart
project authors.  Please see the AUTHORS file
On 2013/01/14 21:31:40, vsm wrote:
> New files should be 2013.  :-)

Done for some files. I tried to approximate based on whether the bulk of the
code in a file existed last year or not.

https://chromiumcodereview.appspot.com/11883013/diff/12006/runtime/embedders/...
File runtime/embedders/openglui/common/events.h (right):

https://chromiumcodereview.appspot.com/11883013/diff/12006/runtime/embedders/...
runtime/embedders/openglui/common/events.h:20: } lifecycle_event_t;
On 2013/01/14 21:31:40, vsm wrote:
> We should change this to LifeCycleEvent to match style rules:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Type_N...

Done.

https://chromiumcodereview.appspot.com/11883013/diff/12006/runtime/embedders/...
runtime/embedders/openglui/common/events.h:26: } key_event_t;
On 2013/01/14 21:31:40, vsm wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/11883013/diff/12006/runtime/embedders/...
runtime/embedders/openglui/common/events.h:36: } motion_event_t;
On 2013/01/14 21:31:40, vsm wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/11883013/diff/12006/runtime/embedders/...
File runtime/embedders/openglui/common/isized.h (right):

https://chromiumcodereview.appspot.com/11883013/diff/12006/runtime/embedders/...
runtime/embedders/openglui/common/isized.h:8: class ISized {
On 2013/01/14 21:31:40, vsm wrote:
> Can you add a comment on this as to what it is and why it's factored out?

Done.

https://chromiumcodereview.appspot.com/11883013/diff/12006/runtime/embedders/...
File runtime/embedders/openglui/common/opengl.h (right):

https://chromiumcodereview.appspot.com/11883013/diff/12006/runtime/embedders/...
runtime/embedders/openglui/common/opengl.h:14: #  endif
On 2013/01/14 21:31:40, vsm wrote:
> Why the funny spacing with '#'?  It is inconsistent with the below.
> 
> Looks like either style is ok:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Type_N...
> but we should be consistent.

Done.

https://chromiumcodereview.appspot.com/11883013/diff/12006/samples/android_sa...
File samples/android_sample/assets/dart/main.dart (right):

https://chromiumcodereview.appspot.com/11883013/diff/12006/samples/android_sa...
samples/android_sample/assets/dart/main.dart:8: //precision mediump float;
On 2013/01/14 21:31:40, vsm wrote:
> Delete the above.

Done.

https://chromiumcodereview.appspot.com/11883013/diff/12006/samples/android_sa...
samples/android_sample/assets/dart/main.dart:330:
//playBackground("music/la_ere_gymnopedie.mp3");
On 2013/01/14 21:31:40, vsm wrote:
> Either delete or add TODO to restore.  Nit: space after the "//".

Done.

https://chromiumcodereview.appspot.com/11883013/diff/12006/samples/android_sa...
File samples/android_sample/src/com/google/dartndk/DummyActivity.java (left):

https://chromiumcodereview.appspot.com/11883013/diff/12006/samples/android_sa...
samples/android_sample/src/com/google/dartndk/DummyActivity.java:20: 
On 2013/01/14 21:31:40, vsm wrote:
> Nice!

Done.

https://chromiumcodereview.appspot.com/11883013/diff/12006/samples/mobile_emu...
File samples/mobile_emulator_sample/main.dart (right):

https://chromiumcodereview.appspot.com/11883013/diff/12006/samples/mobile_emu...
samples/mobile_emulator_sample/main.dart:5: import "dart:math" as Math;
On 2013/01/14 21:31:40, vsm wrote:
> Is this the same as the file in the android sample?  How hard would it be to
> merge the two?  It'd be nice to just have the same sample that can be deployed
> different ways.

Done.

https://chromiumcodereview.appspot.com/11883013/diff/12006/samples/mobile_emu...
samples/mobile_emulator_sample/main.dart:8: //precision mediump float;
On 2013/01/14 21:31:40, vsm wrote:
> delete

This is needed in WebGL. I seem to remember that it caused issues on MacOS and
so removed it from the fragment program, but I've put it back for now as WebGL
must have it.

[vsm, 2013-01-16 05:16:02]

lgtm with some nits.

I'd prefer having 1 sample directory for all three targets instead of 3, but
that could be done in a separate CL.

https://codereview.chromium.org/11883013/diff/25011/runtime/embedders/openglu...
File runtime/embedders/openglui/emulator/emulator_embedder.h (right):

https://codereview.chromium.org/11883013/diff/25011/runtime/embedders/openglu...
runtime/embedders/openglui/emulator/emulator_embedder.h:11: const char* script =
"main.dart");
Is it worth having a default here?

https://codereview.chromium.org/11883013/diff/25011/samples/simplegl/web/rayt...
File samples/simplegl/web/raytrace.dart (right):

https://codereview.chromium.org/11883013/diff/25011/samples/simplegl/web/rayt...
samples/simplegl/web/raytrace.dart:207: // TODO(gram): This should go away at
some point. For now it is a kludge to allow
line len

https://codereview.chromium.org/11883013/diff/25011/samples/simplegl/web/rayt...
samples/simplegl/web/raytrace.dart:354: */
delete?

[vsm, 2013-01-16 19:36:02]

Adding Daniel to this.