Canvas.concat takes a 16-element Float32List instead of an array.

Canvas.concat takes a 16-element Float32List instead of an array.

The array should be in column-major format, in the format used by vector_math.dart.

R=abarth@chromium.org

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

[Matt Perry, 2015-06-04 16:37:18]

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

[abarth-chromium, 2015-06-04 16:58:16]

https://codereview.chromium.org/1155193004/diff/80001/sky/engine/core/paintin...
File sky/engine/core/painting/Float32List.cpp (right):

https://codereview.chromium.org/1155193004/diff/80001/sky/engine/core/paintin...
sky/engine/core/painting/Float32List.cpp:14: void Float32List::Init(Dart_Handle
list) {
Is there some reason this isn't a constructor?

https://codereview.chromium.org/1155193004/diff/80001/sky/engine/core/paintin...
sky/engine/core/painting/Float32List.cpp:43: return result;
When ~Float32List runs, it will call Dart_TypedDataReleaseData, which will make
|data| invalid.  Here's you're returning |result| by value, which (in principle)
runs ~Float32List.

You're actually saved here by NRVO, but this is very fragile...

I'd recommend making Float32List a move-only type by removing the copy
constructor and instead relying on the move constructor.

https://codereview.chromium.org/1155193004/diff/80001/sky/engine/core/paintin...
File sky/engine/core/painting/Float32List.h (right):

https://codereview.chromium.org/1155193004/diff/80001/sky/engine/core/paintin...
sky/engine/core/painting/Float32List.h:18: class Float32List {
This class should be in //sky/engine/tonic.  It's very general.

https://codereview.chromium.org/1155193004/diff/80001/sky/engine/core/paintin...
sky/engine/core/painting/Float32List.h:23: ~Float32List();
We should have a Float32List constructor that initializes |data| and
|num_elements| to zero.

[Matt Perry, 2015-06-04 17:22:44]

I also made the member variables private, since it's more a class than a struct
now.

https://codereview.chromium.org/1155193004/diff/80001/sky/engine/core/paintin...
File sky/engine/core/painting/Float32List.cpp (right):

https://codereview.chromium.org/1155193004/diff/80001/sky/engine/core/paintin...
sky/engine/core/painting/Float32List.cpp:14: void Float32List::Init(Dart_Handle
list) {
On 2015/06/04 16:58:16, abarth wrote:
> Is there some reason this isn't a constructor?

Not a good one.

https://codereview.chromium.org/1155193004/diff/80001/sky/engine/core/paintin...
sky/engine/core/painting/Float32List.cpp:43: return result;
On 2015/06/04 16:58:16, abarth wrote:
> When ~Float32List runs, it will call Dart_TypedDataReleaseData, which will
make
> |data| invalid.  Here's you're returning |result| by value, which (in
principle)
> runs ~Float32List.
> 
> You're actually saved here by NRVO, but this is very fragile...
> 
> I'd recommend making Float32List a move-only type by removing the copy
> constructor and instead relying on the move constructor.

Good point. Done.

https://codereview.chromium.org/1155193004/diff/80001/sky/engine/core/paintin...
File sky/engine/core/painting/Float32List.h (right):

https://codereview.chromium.org/1155193004/diff/80001/sky/engine/core/paintin...
sky/engine/core/painting/Float32List.h:18: class Float32List {
On 2015/06/04 16:58:16, abarth wrote:
> This class should be in //sky/engine/tonic.  It's very general.

Done.

https://codereview.chromium.org/1155193004/diff/80001/sky/engine/core/paintin...
sky/engine/core/painting/Float32List.h:23: ~Float32List();
On 2015/06/04 16:58:16, abarth wrote:
> We should have a Float32List constructor that initializes |data| and
> |num_elements| to zero.

Done.

[abarth-chromium, 2015-06-04 17:29:27]

LGTM

https://codereview.chromium.org/1155193004/diff/100001/sky/engine/tonic/BUILD.gn
File sky/engine/tonic/BUILD.gn (right):

https://codereview.chromium.org/1155193004/diff/100001/sky/engine/tonic/BUILD...
sky/engine/tonic/BUILD.gn:21: "dart_float32list.h",
We can just call these float32_list.cc and float32_list.h if the class is called
Float32List.

https://codereview.chromium.org/1155193004/diff/100001/sky/engine/tonic/dart_...
File sky/engine/tonic/dart_float32list.cc (right):

https://codereview.chromium.org/1155193004/diff/100001/sky/engine/tonic/dart_...
sky/engine/tonic/dart_float32list.cc:27: other.data_ = nullptr;
I'd null out the other dart_handle_ too for paranoia.

https://codereview.chromium.org/1155193004/diff/100001/sky/engine/tonic/dart_...
sky/engine/tonic/dart_float32list.cc:43: return std::move(result);
return Float32List(list);

No need for std::move.

https://codereview.chromium.org/1155193004/diff/100001/sky/engine/tonic/dart_...
File sky/engine/tonic/dart_float32list.h (right):

https://codereview.chromium.org/1155193004/diff/100001/sky/engine/tonic/dart_...
sky/engine/tonic/dart_float32list.h:20: Float32List(Dart_Handle list);
explicit

https://codereview.chromium.org/1155193004/diff/100001/sky/engine/tonic/dart_...
sky/engine/tonic/dart_float32list.h:25: size_t num_elements() const { return
num_elements_; }
Why the change from intptr_t to size_t?  We should use a consistent type for
this value.

[abarth-chromium, 2015-06-04 17:29:28]

LGTM

https://codereview.chromium.org/1155193004/diff/100001/sky/engine/tonic/BUILD.gn
File sky/engine/tonic/BUILD.gn (right):

https://codereview.chromium.org/1155193004/diff/100001/sky/engine/tonic/BUILD...
sky/engine/tonic/BUILD.gn:21: "dart_float32list.h",
We can just call these float32_list.cc and float32_list.h if the class is called
Float32List.

https://codereview.chromium.org/1155193004/diff/100001/sky/engine/tonic/dart_...
File sky/engine/tonic/dart_float32list.cc (right):

https://codereview.chromium.org/1155193004/diff/100001/sky/engine/tonic/dart_...
sky/engine/tonic/dart_float32list.cc:27: other.data_ = nullptr;
I'd null out the other dart_handle_ too for paranoia.

https://codereview.chromium.org/1155193004/diff/100001/sky/engine/tonic/dart_...
sky/engine/tonic/dart_float32list.cc:43: return std::move(result);
return Float32List(list);

No need for std::move.

https://codereview.chromium.org/1155193004/diff/100001/sky/engine/tonic/dart_...
File sky/engine/tonic/dart_float32list.h (right):

https://codereview.chromium.org/1155193004/diff/100001/sky/engine/tonic/dart_...
sky/engine/tonic/dart_float32list.h:20: Float32List(Dart_Handle list);
explicit

https://codereview.chromium.org/1155193004/diff/100001/sky/engine/tonic/dart_...
sky/engine/tonic/dart_float32list.h:25: size_t num_elements() const { return
num_elements_; }
Why the change from intptr_t to size_t?  We should use a consistent type for
this value.

[Matt Perry, 2015-06-04 17:34:36]

https://codereview.chromium.org/1155193004/diff/100001/sky/engine/tonic/BUILD.gn
File sky/engine/tonic/BUILD.gn (right):

https://codereview.chromium.org/1155193004/diff/100001/sky/engine/tonic/BUILD...
sky/engine/tonic/BUILD.gn:21: "dart_float32list.h",
On 2015/06/04 17:29:27, abarth wrote:
> We can just call these float32_list.cc and float32_list.h if the class is
called
> Float32List.

Done.

https://codereview.chromium.org/1155193004/diff/100001/sky/engine/tonic/dart_...
File sky/engine/tonic/dart_float32list.cc (right):

https://codereview.chromium.org/1155193004/diff/100001/sky/engine/tonic/dart_...
sky/engine/tonic/dart_float32list.cc:27: other.data_ = nullptr;
On 2015/06/04 17:29:28, abarth wrote:
> I'd null out the other dart_handle_ too for paranoia.

Done.

https://codereview.chromium.org/1155193004/diff/100001/sky/engine/tonic/dart_...
sky/engine/tonic/dart_float32list.cc:43: return std::move(result);
On 2015/06/04 17:29:28, abarth wrote:
> return Float32List(list);
> 
> No need for std::move.

Done.

https://codereview.chromium.org/1155193004/diff/100001/sky/engine/tonic/dart_...
File sky/engine/tonic/dart_float32list.h (right):

https://codereview.chromium.org/1155193004/diff/100001/sky/engine/tonic/dart_...
sky/engine/tonic/dart_float32list.h:20: Float32List(Dart_Handle list);
On 2015/06/04 17:29:28, abarth wrote:
> explicit

Done.

https://codereview.chromium.org/1155193004/diff/100001/sky/engine/tonic/dart_...
sky/engine/tonic/dart_float32list.h:25: size_t num_elements() const { return
num_elements_; }
On 2015/06/04 17:29:28, abarth wrote:
> Why the change from intptr_t to size_t?  We should use a consistent type for
> this value.

Done.

[Matt Perry, 2015-06-04 17:51:50]

Committed patchset #7 (id:120001) manually as
cc118d27d8650b84942149eb844dcff8e9444f3b (presubmit successful).