Change mojo geometry structs from using type converters to StructTraits.

Change mojo geometry structs from using type converters to StructTraits.

Committed: https://crrev.com/813a8eb3f62bb12d902cd9e31cb60a9b4e745e92
Cr-Commit-Position: refs/heads/master@{#396631}

[Sam McNally, 2016-05-25 06:38:31]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2016-05-25 06:38:47]

Patchset #1 (id:20001) has been deleted

[Sam McNally, 2016-05-26 05:48:33]

Patchset #1 (id:40001) has been deleted

[Sam McNally, 2016-05-26 05:48:44]

Patchset #1 (id:60001) has been deleted

[Sam McNally, 2016-05-26 05:48:46]

Patchset #1 (id:80001) has been deleted

[Sam McNally, 2016-05-26 06:07:23]

sammc@chromium.org changed reviewers:
+ ben@chromium.org, dcheng@chromium.org, sky@chromium.org, xhwang@chromium.org

[Sam McNally, 2016-05-26 06:07:24]

+dcheng for media/mojo/common/media_type_converters.cc

+sky for ash, components, mash, mojo and ui

+xhwang for media

+ben for content and services

[Ben Goodger (Google), 2016-05-26 13:42:30]

awesome! thanks for doing this. you'll have to merge my change from last night
that moved mojo/converters/geometry to ui/gfx/geometry/mojo

content, services -> lgtm

[sky, 2016-05-26 16:12:56]

LGTM

[xhwang, 2016-05-26 16:14:16]

media/ lgtm

[dcheng, 2016-05-26 22:20:41]

https://codereview.chromium.org/2008193002/diff/100001/components/mus/public/...
File components/mus/public/cpp/lib/command_buffer_client_impl.cc (right):

https://codereview.chromium.org/2008193002/diff/100001/components/mus/public/...
components/mus/public/cpp/lib/command_buffer_client_impl.cc:216: gfx::Size
size(static_cast<int32_t>(width), static_cast<int32_t>(height));
Not related to your change but I wonder why this function takes size_t's when
there's many assumptions elsewhere that the parameters need to fit in an int...

https://codereview.chromium.org/2008193002/diff/100001/components/mus/public/...
File components/mus/public/cpp/lib/window_tree_client_impl.cc (right):

https://codereview.chromium.org/2008193002/diff/100001/components/mus/public/...
components/mus/public/cpp/lib/window_tree_client_impl.cc:309:
std::vector<gfx::Rect>(additional_client_areas));
Why is there an explicit copy here?

https://codereview.chromium.org/2008193002/diff/100001/content/renderer/image...
File content/renderer/image_downloader/image_downloader_impl.cc (right):

https://codereview.chromium.org/2008193002/diff/100001/content/renderer/image...
content/renderer/image_downloader/image_downloader_impl.cc:224:
std::vector<gfx::Size>(result_original_image_sizes));
Ditto: why the explicit copy?

[Sam McNally, 2016-05-27 03:32:09]

Patchset #2 (id:120001) has been deleted

[Sam McNally, 2016-05-27 03:44:47]

https://codereview.chromium.org/2008193002/diff/100001/components/mus/public/...
File components/mus/public/cpp/lib/window_tree_client_impl.cc (right):

https://codereview.chromium.org/2008193002/diff/100001/components/mus/public/...
components/mus/public/cpp/lib/window_tree_client_impl.cc:309:
std::vector<gfx::Rect>(additional_client_areas));
On 2016/05/26 22:20:40, dcheng wrote:
> Why is there an explicit copy here?

mojo::Array has a std::vector constructor, but it only takes rvalue references.
Thus, converting from a const std::vector& to a mojo::Array requires a copy to
get a non-const std::vector.

[dcheng, 2016-05-27 03:47:54]

https://codereview.chromium.org/2008193002/diff/100001/components/mus/public/...
File components/mus/public/cpp/lib/window_tree_client_impl.cc (right):

https://codereview.chromium.org/2008193002/diff/100001/components/mus/public/...
components/mus/public/cpp/lib/window_tree_client_impl.cc:309:
std::vector<gfx::Rect>(additional_client_areas));
On 2016/05/27 at 03:44:47, Sam McNally wrote:
> On 2016/05/26 22:20:40, dcheng wrote:
> > Why is there an explicit copy here?
> 
> mojo::Array has a std::vector constructor, but it only takes rvalue
references. Thus, converting from a const std::vector& to a mojo::Array requires
a copy to get a non-const std::vector.

Why can't it have a const std::vector<>& version too? At that point, you're
going to have to make it a copy either way, and it seems more idiomatic for that
to happen inside mojo::Array.

[Sam McNally, 2016-05-27 06:34:06]

https://codereview.chromium.org/2008193002/diff/100001/components/mus/public/...
File components/mus/public/cpp/lib/window_tree_client_impl.cc (right):

https://codereview.chromium.org/2008193002/diff/100001/components/mus/public/...
components/mus/public/cpp/lib/window_tree_client_impl.cc:309:
std::vector<gfx::Rect>(additional_client_areas));
On 2016/05/27 03:47:54, dcheng wrote:
> On 2016/05/27 at 03:44:47, Sam McNally wrote:
> > On 2016/05/26 22:20:40, dcheng wrote:
> > > Why is there an explicit copy here?
> > 
> > mojo::Array has a std::vector constructor, but it only takes rvalue
> references. Thus, converting from a const std::vector& to a mojo::Array
requires
> a copy to get a non-const std::vector.
> 
> Why can't it have a const std::vector<>& version too? At that point, you're
> going to have to make it a copy either way, and it seems more idiomatic for
that
> to happen inside mojo::Array.

Done. mojo::Array is getting quite strange now: move-only, but sometimes
cloneable with an explicit Clone(), convertible to and from a sometimes-copyable
vector.

[dcheng, 2016-05-27 06:53:09]

mojom lgtm

Given that Chrome generally passes by const ref, I think this makes sense as an
extension to mojo::Array: it's just not common to have to explicitly create a
temporary copy just to pass to a function that only accepts rvalues. The net
result is the same either way, right?

[Sam McNally, 2016-05-28 01:06:35]

Patchset #5 (id:200001) has been deleted

[Sam McNally, 2016-05-28 03:26:14]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2016-05-28 03:26:15]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
ben@chromium.org, sky@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2008193002/#ps220001
(title: "rebase")

[commit-bot: I haz the power, 2016-05-28 03:26:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2008193002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2008193002/220001

[commit-bot: I haz the power, 2016-05-28 03:34:20]

Committed patchset #5 (id:220001)

[commit-bot: I haz the power, 2016-05-28 03:35:39]

Description was changed from

==========
Change mojo geometry structs from using type converters to StructTraits.
==========

to

==========
Change mojo geometry structs from using type converters to StructTraits.

Committed: https://crrev.com/813a8eb3f62bb12d902cd9e31cb60a9b4e745e92
Cr-Commit-Position: refs/heads/master@{#396631}
==========

[commit-bot: I haz the power, 2016-05-28 03:35:41]

Patchset 5 (id:??) landed as
https://crrev.com/813a8eb3f62bb12d902cd9e31cb60a9b4e745e92
Cr-Commit-Position: refs/heads/master@{#396631}

[dcheng, 2016-06-07 23:38:37]

https://codereview.chromium.org/2008193002/diff/220001/ui/gfx/geometry/mojo/g...
File ui/gfx/geometry/mojo/geometry_struct_traits.h (right):

https://codereview.chromium.org/2008193002/diff/220001/ui/gfx/geometry/mojo/g...
ui/gfx/geometry/mojo/geometry_struct_traits.h:83: out->SetRect(data.x(),
data.y(), data.width(), data.height());
I missed this in the initial review, but Rect/RectF should be checked for
negative sizes (that's not permissible). Ditto for gfx::Size/gfx::SizeF.