Mojom structs for copy requests / results should map to unique_ptr

Mojom structs for copy requests / results should map to unique_ptr

Currently, cc.mojom.CopyOutputRequest and cc.mojom.CopyOutputResult map
to cc::CopyOutputRequest and cc::CopyOutputResult respectively, but
they should map to std::unique_ptr<cc::CopyOutputRequest> and
std::unique_ptr<cc::CopyOutputResult>. This is because 
cc::CopyOutputRequest and cc::CopyOutputResult can be only created
exclusively as unique_ptr and we are going to have problem passing
their objects over mojo.

BUG=672071
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2670213006
Cr-Commit-Position: refs/heads/master@{#448177}
Committed: https://chromium.googlesource.com/chromium/src/+/e0c6f958254ff01ada02bfc4c9ba99a602c67930

[Saman Sami, 2017-02-03 22:08:07]

Description was changed from

==========
Mojom structs for copy requests / results should map to unique_ptr

BUG=672071
==========

to

==========
Mojom structs for copy requests / results should map to unique_ptr

BUG=672071
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2017-02-03 22:30:32]

Description was changed from

==========
Mojom structs for copy requests / results should map to unique_ptr

BUG=672071
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Mojom structs for copy requests / results should map to unique_ptr

Currently, cc.mojom.CopyOutputRequest and cc.mojom.CopyOutputResult map
to cc::CopyOutputRequest and cc::CopyOutputResult respectively, but
they should map to std::unique_ptr<cc::CopyOutputRequest> and
std::unique_ptr<cc::CopyOutputResult>. This is because 
cc::CopyOutputRequest and cc::CopyOutputResult can be only created
exclusively as unique_ptr and we are going to have problem passing
their objects over mojo.

BUG=672071
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2017-02-03 22:32:26]

samans@chromium.org changed reviewers:
+ dcheng@chromium.org, enne@chromium.org, fsamuel@chromium.org

[Saman Sami, 2017-02-03 22:32:37]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 22:33:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-02-03 22:33:54]

dcheng@: Please review mojo
enne@: Please review cc/
fsamuel@: Plesae review all files

[enne (OOO), 2017-02-03 22:57:48]

lgtm

[Fady Samuel, 2017-02-03 23:03:20]

lgtm

[Saman Sami, 2017-02-03 23:06:45]

samans@chromium.org changed reviewers:
+ tsepez@chromium.org

[Saman Sami, 2017-02-03 23:07:11]

tsepez@: Please review mojo

[Saman Sami, 2017-02-03 23:07:34]

samans@chromium.org changed reviewers:
- dcheng@chromium.org

[commit-bot: I haz the power, 2017-02-03 23:35:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-03 23:35:58]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2017-02-04 00:32:14]

LGTM, but a couple of general questions, not specific to this particular change.

https://codereview.chromium.org/2670213006/diff/20001/cc/ipc/copy_output_requ...
File cc/ipc/copy_output_request.typemap (right):

https://codereview.chromium.org/2670213006/diff/20001/cc/ipc/copy_output_requ...
cc/ipc/copy_output_request.typemap:11: type_mappings = [
"cc.mojom.CopyOutputRequest=std::unique_ptr<cc::CopyOutputRequest>[move_only]" ]
Ah, I didn't know this syntax existed.  Is it a new addition?

https://codereview.chromium.org/2670213006/diff/20001/cc/ipc/copy_output_requ...
File cc/ipc/copy_output_request_struct_traits.cc (right):

https://codereview.chromium.org/2670213006/diff/20001/cc/ipc/copy_output_requ...
cc/ipc/copy_output_request_struct_traits.cc:17: return
data.ReadSource(&out->source_) && data.ReadArea(&out->area_) &&
I suppose it's ok to short-circuit here, the caller will free the object
eventually even though its only partially-filled in.

This is going to come up over and again when reviewing these, is there a
precedent for keeping the unique_ptr local to immediately clean up on error and
then std::move() it at the end?  I'd argue for such a thing, but I can see that
is isn't strictly necesary.

[Saman Sami, 2017-02-04 01:09:25]

https://codereview.chromium.org/2670213006/diff/20001/cc/ipc/copy_output_requ...
File cc/ipc/copy_output_request.typemap (right):

https://codereview.chromium.org/2670213006/diff/20001/cc/ipc/copy_output_requ...
cc/ipc/copy_output_request.typemap:11: type_mappings = [
"cc.mojom.CopyOutputRequest=std::unique_ptr<cc::CopyOutputRequest>[move_only]" ]
On 2017/02/04 00:32:14, Tom Sepez wrote:
> Ah, I didn't know this syntax existed.  Is it a new addition?

Are you talking about move_only? I'm new to mojo, so I'm not qualified to give a
proper answer, but I think being able to send move-only objects with mojo is
crucial so probably it's been around for a while but there are not many
move-only objects so I guess we don't see it very often.

https://codereview.chromium.org/2670213006/diff/20001/cc/ipc/copy_output_requ...
File cc/ipc/copy_output_request_struct_traits.cc (right):

https://codereview.chromium.org/2670213006/diff/20001/cc/ipc/copy_output_requ...
cc/ipc/copy_output_request_struct_traits.cc:17: return
data.ReadSource(&out->source_) && data.ReadArea(&out->area_) &&
On 2017/02/04 00:32:14, Tom Sepez wrote:
> I suppose it's ok to short-circuit here, the caller will free the object
> eventually even though its only partially-filled in.
> 
> This is going to come up over and again when reviewing these, is there a
> precedent for keeping the unique_ptr local to immediately clean up on error
and
> then std::move() it at the end?  I'd argue for such a thing, but I can see
that
> is isn't strictly necesary.

Good idea. It'll make the code cleaner too. Here is something similar in the
code base:
https://cs.chromium.org/chromium/src/ui/display/mojo/display_layout_struct_tr...

[Saman Sami, 2017-02-04 01:09:30]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-04 01:10:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Sepez, 2017-02-04 01:15:17]

LGTM.

[Saman Sami, 2017-02-04 01:15:32]

The CQ bit was unchecked by samans@chromium.org

[Saman Sami, 2017-02-04 01:15:45]

The CQ bit was checked by samans@chromium.org

[Saman Sami, 2017-02-04 01:15:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2670213006/#ps40001
(title: "c")

[commit-bot: I haz the power, 2017-02-04 01:16:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-02-04 04:19:57]

The CQ bit was unchecked by samans@chromium.org

[Saman Sami, 2017-02-04 04:20:04]

The CQ bit was checked by samans@chromium.org

[commit-bot: I haz the power, 2017-02-04 04:20:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-04 05:35:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-04 05:35:29]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no
build URL)

[Saman Sami, 2017-02-04 13:46:22]

The CQ bit was checked by samans@chromium.org

[commit-bot: I haz the power, 2017-02-04 13:46:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-04 15:47:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-04 15:47:02]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no
build URL)

[Saman Sami, 2017-02-05 04:13:40]

The CQ bit was checked by samans@chromium.org

[commit-bot: I haz the power, 2017-02-05 04:13:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-05 05:32:49]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486268020426420,
"parent_rev": "4f43f2eacab108429daea2bf64678922fc85491e", "commit_rev":
"e0c6f958254ff01ada02bfc4c9ba99a602c67930"}

[commit-bot: I haz the power, 2017-02-05 05:33:43]

Description was changed from

==========
Mojom structs for copy requests / results should map to unique_ptr

Currently, cc.mojom.CopyOutputRequest and cc.mojom.CopyOutputResult map
to cc::CopyOutputRequest and cc::CopyOutputResult respectively, but
they should map to std::unique_ptr<cc::CopyOutputRequest> and
std::unique_ptr<cc::CopyOutputResult>. This is because 
cc::CopyOutputRequest and cc::CopyOutputResult can be only created
exclusively as unique_ptr and we are going to have problem passing
their objects over mojo.

BUG=672071
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Mojom structs for copy requests / results should map to unique_ptr

Currently, cc.mojom.CopyOutputRequest and cc.mojom.CopyOutputResult map
to cc::CopyOutputRequest and cc::CopyOutputResult respectively, but
they should map to std::unique_ptr<cc::CopyOutputRequest> and
std::unique_ptr<cc::CopyOutputResult>. This is because 
cc::CopyOutputRequest and cc::CopyOutputResult can be only created
exclusively as unique_ptr and we are going to have problem passing
their objects over mojo.

BUG=672071
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2670213006
Cr-Commit-Position: refs/heads/master@{#448177}
Committed:
https://chromium.googlesource.com/chromium/src/+/e0c6f958254ff01ada02bfc4c9ba...
==========

[commit-bot: I haz the power, 2017-02-05 05:33:45]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/e0c6f958254ff01ada02bfc4c9ba...

[Saman Sami, 2017-02-10 03:00:36]

On 2017/02/05 05:33:45, commit-bot: I haz the power wrote:
> Committed patchset #3 (id:40001) as
>
https://chromium.googlesource.com/chromium/src/+/e0c6f958254ff01ada02bfc4c9ba...