Canvas API. There are still a number of things to do:

Canvas API. There are still a number of things to do:

- off-screen canvases/contexts
- image APIs
- clips need to be tested
- text baseline and direction
- image smoothing
- global composite op testing
Committed: https://code.google.com/p/dart/source/detail?r=18084

[vsm, 2013-02-04 18:18:41]

Mostly LGTM, but I really think the samples be separated into a raytracer and
the canvas tests first.

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
File runtime/embedders/openglui/common/canvas_context.cc (right):

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
runtime/embedders/openglui/common/canvas_context.cc:39: // then we introduce
more complexity; this can't just be an URL.
We might want to abstract this out with some concept of Asset or Resource.

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
File runtime/embedders/openglui/common/canvas_context.h (right):

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
runtime/embedders/openglui/common/canvas_context.h:22: ImageData() {}
Initialize 0/NULL on empty constructor?

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
runtime/embedders/openglui/common/canvas_context.h:35: inline float
Radians2Degrees(float angle) {
Can make static to avoid implicit this argument.

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
runtime/embedders/openglui/common/canvas_context.h:105: if (state_ !=
popped_state) {
Not sure I understand the model here.  Is it possible for the last two states to
be same?  Is it possible for states to repeat interleaving?  Is the delete save,
if so?

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
File runtime/embedders/openglui/common/canvas_state.h (right):

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
runtime/embedders/openglui/common/canvas_state.h:77: delete[] lineDash_;
Note, lineDash_ is init NULL in the constructor.  Might want to check if it's
set before deleting.

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
File runtime/embedders/openglui/common/gl.dart (right):

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
runtime/embedders/openglui/common/gl.dart:7: class CanvasElement {
I think I'd prefer not exposing CanvasElement - it's sort of DOM-y, but not
quite.  But we can revisit later.

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
runtime/embedders/openglui/common/gl.dart:232: double C2DSetGlobalAlpha(int
handle, double globalAlpha)
I think we should use "num" instead of "double" here and elsewhere.  Otherwise,
something like:
cxt.globalAlpha = 1;
will throw a type error in checked mode.  (Note, we do this in dart:html.)

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
runtime/embedders/openglui/common/gl.dart:580: if (w1 == null) { //
drawImage(image, dx, dy)
"if (!?w1)" will test if w1 was actually not passed in (vs passed in with an
explicit null which should really be an error).

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
runtime/embedders/openglui/common/gl.dart:729: // TODO(vsm): Kill.
Maybe we can delete this now.

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/op...
File runtime/embedders/openglui/openglui_embedder.gypi (right):

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/op...
runtime/embedders/openglui/openglui_embedder.gypi:93:
'-Lthird_party/skia/trunk/out/config/android-x86/Debug/obj.target/gyp',
What about Release mode?  Perhaps a TODO here to clean this up.

https://codereview.chromium.org/12186002/diff/1/samples/openglui/src/raytrace...
File samples/openglui/src/raytrace.dart (right):

https://codereview.chromium.org/12186002/diff/1/samples/openglui/src/raytrace...
samples/openglui/src/raytrace.dart:10: */
I think we should make a separate sample.  E.g., something like:

openglui_raytrace for the raytracer
and
openglui_showcase for these tests

[gram, 2013-02-04 22:02:28]

PTAL. I split the samples up. There are two separate HTML files; for the
desktop, you have to specify the dart script as the last argument. It's a bit of
a kludge but will do for now.

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
File runtime/embedders/openglui/common/canvas_context.cc (right):

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
runtime/embedders/openglui/common/canvas_context.cc:39: // then we introduce
more complexity; this can't just be an URL.
On 2013/02/04 18:18:41, vsm wrote:
> We might want to abstract this out with some concept of Asset or Resource.

I agree, but I think that can wait for a later CL; this doesn't work yet as it
is.

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
File runtime/embedders/openglui/common/canvas_context.h (right):

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
runtime/embedders/openglui/common/canvas_context.h:22: ImageData() {}
On 2013/02/04 18:18:41, vsm wrote:
> Initialize 0/NULL on empty constructor?

Done.

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
runtime/embedders/openglui/common/canvas_context.h:35: inline float
Radians2Degrees(float angle) {
On 2013/02/04 18:18:41, vsm wrote:
> Can make static to avoid implicit this argument.

Done.

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
runtime/embedders/openglui/common/canvas_context.h:105: if (state_ !=
popped_state) {
On 2013/02/04 18:18:41, vsm wrote:
> Not sure I understand the model here.  Is it possible for the last two states
to
> be same?  Is it possible for states to repeat interleaving?  Is the delete
save,
> if so?

I'm not sure on the exact semantics of Canvas if you pop more than you restore.
In my case, I wanted a pop when there is no saved state to be a no-op. In this
case we simply return the current state and hence no delete. I'll add a comment.

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
File runtime/embedders/openglui/common/gl.dart (right):

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
runtime/embedders/openglui/common/gl.dart:7: class CanvasElement {
On 2013/02/04 18:18:41, vsm wrote:
> I think I'd prefer not exposing CanvasElement - it's sort of DOM-y, but not
> quite.  But we can revisit later.

The motivation is because if we want to create additional contexts, such as
off-screen contexts, this is done by calling canvas.getContext("2d"), so we need
to have CanvasElements. Basically we can have Dart code that works both in the
browser and on the desktop, using:

context = (new CanvasElement(width, height)).getContext("2d");

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
runtime/embedders/openglui/common/gl.dart:232: double C2DSetGlobalAlpha(int
handle, double globalAlpha)
On 2013/02/04 18:18:41, vsm wrote:
> I think we should use "num" instead of "double" here and elsewhere. 
Otherwise,
> something like:
> cxt.globalAlpha = 1;
> will throw a type error in checked mode.  (Note, we do this in dart:html.)

No, it won't throw, because the wrapper functions in the context all do casts to
double.

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
runtime/embedders/openglui/common/gl.dart:580: if (w1 == null) { //
drawImage(image, dx, dy)
On 2013/02/04 18:18:41, vsm wrote:
> "if (!?w1)" will test if w1 was actually not passed in (vs passed in with an
> explicit null which should really be an error).  

Done.

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
runtime/embedders/openglui/common/gl.dart:729: // TODO(vsm): Kill.
On 2013/02/04 18:18:41, vsm wrote:
> Maybe we can delete this now.

I find it is still useful to catch bugs occasionally.

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/op...
File runtime/embedders/openglui/openglui_embedder.gypi (right):

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/op...
runtime/embedders/openglui/openglui_embedder.gypi:93:
'-Lthird_party/skia/trunk/out/config/android-x86/Debug/obj.target/gyp',
On 2013/02/04 18:18:41, vsm wrote:
> What about Release mode?  Perhaps a TODO here to clean this up.

Done.

https://codereview.chromium.org/12186002/diff/1/samples/openglui/src/raytrace...
File samples/openglui/src/raytrace.dart (right):

https://codereview.chromium.org/12186002/diff/1/samples/openglui/src/raytrace...
samples/openglui/src/raytrace.dart:10: */
On 2013/02/04 18:18:41, vsm wrote:
> I think we should make a separate sample.  E.g., something like:
> 
> openglui_raytrace for the raytracer
> and
> openglui_showcase for these tests

Done.

[vsm, 2013-02-04 22:16:01]

lgtm

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
File runtime/embedders/openglui/common/gl.dart (right):

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
runtime/embedders/openglui/common/gl.dart:232: double C2DSetGlobalAlpha(int
handle, double globalAlpha)
Got it.

On 2013/02/04 22:02:28, gram wrote:
> On 2013/02/04 18:18:41, vsm wrote:
> > I think we should use "num" instead of "double" here and elsewhere. 
> Otherwise,
> > something like:
> > cxt.globalAlpha = 1;
> > will throw a type error in checked mode.  (Note, we do this in dart:html.)
> 
> No, it won't throw, because the wrapper functions in the context all do casts
to
> double.

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
runtime/embedders/openglui/common/gl.dart:412: set globalAlpha(a) {
num a

https://codereview.chromium.org/12186002/diff/1/runtime/embedders/openglui/co...
runtime/embedders/openglui/common/gl.dart:447: set lineJoin(lj) => 
C2DSetLineJoin(_handle, _lineJoin = lj);
String lj