Sky: Add a Point class.

Sky: Add a Point class.

Also, fix the style in Rect.cpp/.h.

R=eseidel@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/df8dcacada98e9a93142ae28d8c4a9be30be0750

[Matt Perry, 2015-05-26 22:09:13]

mpcomplete@chromium.org changed reviewers:
+ eseidel@chromium.org

[eseidel, 2015-05-26 22:21:11]

lgtm

https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Po...
File sky/engine/core/painting/Point.h (right):

https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Po...
sky/engine/core/painting/Point.h:17: bool isNull;
Why this style change?  Is this Chromium style? (I don't even know..., but I
know we're trying to follow it in new code.)

[Matt Perry, 2015-05-26 22:24:09]

https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Po...
File sky/engine/core/painting/Point.h (right):

https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Po...
sky/engine/core/painting/Point.h:17: bool isNull;
On 2015/05/26 22:21:11, eseidel wrote:
> Why this style change?  Is this Chromium style? (I don't even know..., but I
> know we're trying to follow it in new code.)

Oh - the old code is Chromium style. I saw Blink style everywhere and assumed we
were trying to use that. I'll revert this part of the change.

[abarth-chromium, 2015-05-27 02:17:35]

abarth@chromium.org changed reviewers:
+ abarth@chromium.org

[abarth-chromium, 2015-05-27 02:17:36]

Minor comments.  Feel free to address in a followup patch.

https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Po...
File sky/engine/core/painting/Point.cpp (right):

https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Po...
sky/engine/core/painting/Point.cpp:26: Dart_Handle yValue =
Dart_GetField(dartPoint, Dart_NewStringFromCString("y"));
We should cache the "x" and "y" strings so that we don't have to malloc up new
strings each time we want to convert a point.  We probably want to use Symbols
rather than strings as well for added speed.

https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Po...
sky/engine/core/painting/Point.cpp:30: Dart_Handle err =
Dart_DoubleValue(xValue, &y);
Why do we capture an error for y and not for x?

https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Po...
File sky/engine/core/painting/Point.dart (right):

https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Po...
sky/engine/core/painting/Point.dart:9: double y;
final?

https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Po...
sky/engine/core/painting/Point.dart:11: Point(this.x, this.y);
const ?

https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Re...
File sky/engine/core/painting/Rect.cpp (right):

https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Re...
sky/engine/core/painting/Rect.cpp:26: Dart_NewStringFromCString("_value"));
Same caching point here.  We should cache in
https://github.com/domokit/mojo/blob/master/sky/engine/core/script/dom_dart_s...

[Matt Perry, 2015-05-27 17:55:43]

https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Po...
File sky/engine/core/painting/Point.cpp (right):

https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Po...
sky/engine/core/painting/Point.cpp:26: Dart_Handle yValue =
Dart_GetField(dartPoint, Dart_NewStringFromCString("y"));
On 2015/05/27 02:17:36, abarth wrote:
> We should cache the "x" and "y" strings so that we don't have to malloc up new
> strings each time we want to convert a point.  We probably want to use Symbols
> rather than strings as well for added speed.

OK. Not sure what you mean by Symbols. Do we have code that does that?

https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Po...
sky/engine/core/painting/Point.cpp:30: Dart_Handle err =
Dart_DoubleValue(xValue, &y);
On 2015/05/27 02:17:36, abarth wrote:
> Why do we capture an error for y and not for x?

I was thinking the error would propagate to the last API call, but I guess that
doesn't make sense. (Does that happen if I call Dart_Foo(handle) if handle has
an error?)

https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Po...
File sky/engine/core/painting/Point.dart (right):

https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Po...
sky/engine/core/painting/Point.dart:9: double y;
On 2015/05/27 02:17:36, abarth wrote:
> final?

I was envisioning adding methods like offset(x,y) which would modify the Point
in place.

https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Po...
File sky/engine/core/painting/Point.h (right):

https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Po...
sky/engine/core/painting/Point.h:17: bool isNull;
On 2015/05/26 22:24:08, Matt Perry wrote:
> On 2015/05/26 22:21:11, eseidel wrote:
> > Why this style change?  Is this Chromium style? (I don't even know..., but I
> > know we're trying to follow it in new code.)
> 
> Oh - the old code is Chromium style. I saw Blink style everywhere and assumed
we
> were trying to use that. I'll revert this part of the change.

OK, so I'll keep the .cpp/.h filenames because that's the convention in this
directory. But inside the files I'll use Chromium style, with the exception of
any IDL-generated method names (setFoo, etc.)

[abarth-chromium, 2015-05-27 18:17:00]

On 2015/05/27 at 17:55:43, mpcomplete wrote:
>
https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Po...
> File sky/engine/core/painting/Point.cpp (right):
> 
>
https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Po...
> sky/engine/core/painting/Point.cpp:26: Dart_Handle yValue =
Dart_GetField(dartPoint, Dart_NewStringFromCString("y"));
> On 2015/05/27 02:17:36, abarth wrote:
> > We should cache the "x" and "y" strings so that we don't have to malloc up
new
> > strings each time we want to convert a point.  We probably want to use
Symbols
> > rather than strings as well for added speed.
> 
> OK. Not sure what you mean by Symbols. Do we have code that does that?

Looks like symbols aren't exposed by the DartVM API, so let's not worry about
it.  (I was referring to
https://api.dartlang.org/apidocs/channels/stable/dartdoc-viewer/dart:core.Symbol
)

>
https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Po...
> sky/engine/core/painting/Point.cpp:30: Dart_Handle err =
Dart_DoubleValue(xValue, &y);
> On 2015/05/27 02:17:36, abarth wrote:
> > Why do we capture an error for y and not for x?
> 
> I was thinking the error would propagate to the last API call, but I guess
that doesn't make sense. (Does that happen if I call Dart_Foo(handle) if handle
has an error?)

Dunno...

>
https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Po...
> File sky/engine/core/painting/Point.dart (right):
> 
>
https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Po...
> sky/engine/core/painting/Point.dart:9: double y;
> On 2015/05/27 02:17:36, abarth wrote:
> > final?
> 
> I was envisioning adding methods like offset(x,y) which would modify the Point
in place.

Ok, we can go the mutable path.

>
https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Po...
> File sky/engine/core/painting/Point.h (right):
> 
>
https://codereview.chromium.org/1158843002/diff/1/sky/engine/core/painting/Po...
> sky/engine/core/painting/Point.h:17: bool isNull;
> On 2015/05/26 22:24:08, Matt Perry wrote:
> > On 2015/05/26 22:21:11, eseidel wrote:
> > > Why this style change?  Is this Chromium style? (I don't even know..., but
I
> > > know we're trying to follow it in new code.)
> > 
> > Oh - the old code is Chromium style. I saw Blink style everywhere and
assumed we
> > were trying to use that. I'll revert this part of the change.
> 
> OK, so I'll keep the .cpp/.h filenames because that's the convention in this
directory. But inside the files I'll use Chromium style, with the exception of
any IDL-generated method names (setFoo, etc.)

Ok.

[Matt Perry, 2015-05-27 18:40:07]

Committed patchset #2 (id:20001) manually as
df8dcacada98e9a93142ae28d8c4a9be30be0750 (presubmit successful).