mojo: MessageReceiver*::AcceptWithResponder() now take a unique_ptr to the responder

mojo: MessageReceiver*::AcceptWithResponder() now take a unique_ptr to the responder

Previously these functions took raw pointers to MessageReceivers and
took ownership on success or left ownership with the caller if they
failed. Now they take unique_ptrs (always assuming ownership) to make
the interface clearer and safer.

BUG=703489
Review-Url: https://codereview.chromium.org/2766523002
Cr-Commit-Position: refs/heads/master@{#458684}
Committed: https://chromium.googlesource.com/chromium/src/+/25f0f5f955e5d61d055dbe69a791db80541ef8ee

[watk, 2017-03-21 03:45:07]

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

[commit-bot: I haz the power, 2017-03-21 03:45:25]

Dry run: 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-03-21 03:53:35]

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

[commit-bot: I haz the power, 2017-03-21 03:53:36]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[watk, 2017-03-21 04:01:47]

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

[commit-bot: I haz the power, 2017-03-21 04:02:24]

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

[watk, 2017-03-21 04:05:45]

Description was changed from

==========
mojo: MessageReceiver*::AcceptWithResponder() now take a unique_ptr to the
responder

Previously these functions took raw pointers to MessageReceivers but
only took ownership if they succeeded, which forced the caller to delete
the responder. To make the interface clearer and safer, now they take
unique_ptrs.

BUG=703489
==========

to

==========
mojo: MessageReceiver*::AcceptWithResponder() now take a unique_ptr to the
responder

Previously these functions took raw pointers to MessageReceivers and
took ownership on success or left ownership with the caller if they
failed. Now they take unique_ptrs (always assuming ownership) to make
the interface clearer and safer.

BUG=703489
==========

[watk, 2017-03-21 04:09:38]

watk@chromium.org changed reviewers:
+ yzshen@chromium.org

[watk, 2017-03-21 04:09:38]

https://codereview.chromium.org/2766523002/diff/1/mojo/public/cpp/bindings/me...
File mojo/public/cpp/bindings/message.h (right):

https://codereview.chromium.org/2766523002/diff/1/mojo/public/cpp/bindings/me...
mojo/public/cpp/bindings/message.h:189: // or upon its own destruction.
Do you think this comment is useful to keep or shall I delete it?

[commit-bot: I haz the power, 2017-03-21 05:14:46]

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

[commit-bot: I haz the power, 2017-03-21 05:14:47]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2017-03-21 16:10:00]

LGTM with a few nits

https://codereview.chromium.org/2766523002/diff/1/mojo/public/cpp/bindings/me...
File mojo/public/cpp/bindings/message.h (right):

https://codereview.chromium.org/2766523002/diff/1/mojo/public/cpp/bindings/me...
mojo/public/cpp/bindings/message.h:189: // or upon its own destruction.
On 2017/03/21 04:09:38, watk wrote:
> Do you think this comment is useful to keep or shall I delete it?

IMO it is not very useful now so I lean towards deleting it.

https://codereview.chromium.org/2766523002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/pipe_control_message_proxy.cc (right):

https://codereview.chromium.org/2766523002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/pipe_control_message_proxy.cc:10: #include
"base/compiler_specific.h"
Is this still needed?

https://codereview.chromium.org/2766523002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/message.h (right):

https://codereview.chromium.org/2766523002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/message.h:225: // immediately. Otherwise it will delete
it after calling |responder->Accept|
ditto.

https://codereview.chromium.org/2766523002/diff/20001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/2766523002/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:210:
// This return value may be ignored as !ok implies the Connector has
Please move this comment to be above line 209, and change "!ok" to "false".

https://codereview.chromium.org/2766523002/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:351:
message->has_flag(mojo::Message::kFlagIsSync), std::move(responder));
Please make the lines <80 chars. Although we don't have to style guide of .tmpl
files, it is easier to read that way.

https://codereview.chromium.org/2766523002/diff/20001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_stub_declaration.tmpl
(right):

https://codereview.chromium.org/2766523002/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/interface_stub_declaration.tmpl:6:
std::unique_ptr<mojo::MessageReceiverWithStatus> responder);
< 80 chars please.

[watk, 2017-03-22 07:09:15]

Thanks! CQing

https://codereview.chromium.org/2766523002/diff/1/mojo/public/cpp/bindings/me...
File mojo/public/cpp/bindings/message.h (right):

https://codereview.chromium.org/2766523002/diff/1/mojo/public/cpp/bindings/me...
mojo/public/cpp/bindings/message.h:189: // or upon its own destruction.
On 2017/03/21 16:10:00, yzshen1 wrote:
> On 2017/03/21 04:09:38, watk wrote:
> > Do you think this comment is useful to keep or shall I delete it?
> 
> IMO it is not very useful now so I lean towards deleting it. 

Done.

https://codereview.chromium.org/2766523002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/pipe_control_message_proxy.cc (right):

https://codereview.chromium.org/2766523002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/pipe_control_message_proxy.cc:10: #include
"base/compiler_specific.h"
On 2017/03/21 16:10:00, yzshen1 wrote:
> Is this still needed?

Removed, thanks

https://codereview.chromium.org/2766523002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/message.h (right):

https://codereview.chromium.org/2766523002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/message.h:225: // immediately. Otherwise it will delete
it after calling |responder->Accept|
On 2017/03/21 16:10:00, yzshen1 wrote:
> ditto.

Done.

https://codereview.chromium.org/2766523002/diff/20001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/2766523002/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:210:
// This return value may be ignored as !ok implies the Connector has
On 2017/03/21 16:10:00, yzshen1 wrote:
> Please move this comment to be above line 209, and change "!ok" to "false".

Done.

https://codereview.chromium.org/2766523002/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl:351:
message->has_flag(mojo::Message::kFlagIsSync), std::move(responder));
On 2017/03/21 16:10:00, yzshen1 wrote:
> Please make the lines <80 chars. Although we don't have to style guide of
.tmpl
> files, it is easier to read that way.

Done.

https://codereview.chromium.org/2766523002/diff/20001/mojo/public/tools/bindi...
File
mojo/public/tools/bindings/generators/cpp_templates/interface_stub_declaration.tmpl
(right):

https://codereview.chromium.org/2766523002/diff/20001/mojo/public/tools/bindi...
mojo/public/tools/bindings/generators/cpp_templates/interface_stub_declaration.tmpl:6:
std::unique_ptr<mojo::MessageReceiverWithStatus> responder);
On 2017/03/21 16:10:00, yzshen1 wrote:
> < 80 chars please.

Done.

[watk, 2017-03-22 07:09:24]

The CQ bit was checked by watk@chromium.org

[watk, 2017-03-22 07:09:25]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/2766523002/#ps40001
(title: "comments")

[commit-bot: I haz the power, 2017-03-22 07:10:04]

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-03-22 08:26:56]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490166564845850,
"parent_rev": "d6f8a937c0d8fbfcbdfe7edc93255e454c5de3cf", "commit_rev":
"25f0f5f955e5d61d055dbe69a791db80541ef8ee"}

[commit-bot: I haz the power, 2017-03-22 08:27:42]

Description was changed from

==========
mojo: MessageReceiver*::AcceptWithResponder() now take a unique_ptr to the
responder

Previously these functions took raw pointers to MessageReceivers and
took ownership on success or left ownership with the caller if they
failed. Now they take unique_ptrs (always assuming ownership) to make
the interface clearer and safer.

BUG=703489
==========

to

==========
mojo: MessageReceiver*::AcceptWithResponder() now take a unique_ptr to the
responder

Previously these functions took raw pointers to MessageReceivers and
took ownership on success or left ownership with the caller if they
failed. Now they take unique_ptrs (always assuming ownership) to make
the interface clearer and safer.

BUG=703489

Review-Url: https://codereview.chromium.org/2766523002
Cr-Commit-Position: refs/heads/master@{#458684}
Committed:
https://chromium.googlesource.com/chromium/src/+/25f0f5f955e5d61d055dbe69a791...
==========

[commit-bot: I haz the power, 2017-03-22 08:27:44]

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