Mojo bindings: merge the control messages of sending disconnect reason and notifying endpoint close…

Mojo bindings: merge the control messages of sending disconnect reason and notifying endpoint closed.

This is part of the work to remove AssociatedGroup and simplify the associated interface API.

BUG=682334
Review-Url: https://codereview.chromium.org/2649573004
Cr-Commit-Position: refs/heads/master@{#445473}
Committed: https://chromium.googlesource.com/chromium/src/+/8be41d3abfb2a8f5d7b58b3e70ebbc86fc7aa5d6

[yzshen1, 2017-01-21 02:01:08]

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

[commit-bot: I haz the power, 2017-01-21 02:01:40]

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

[yzshen1, 2017-01-21 02:04:08]

yzshen@chromium.org changed reviewers:
+ dcheng@chromium.org, rockot@chromium.org

[yzshen1, 2017-01-21 02:04:09]

Hi, Ken and Daniel.

Would you please take a look? Thanks!

Ken: everything.
Daniel: the changes to mojom files.

[commit-bot: I haz the power, 2017-01-21 03:29:50]

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

[commit-bot: I haz the power, 2017-01-21 03:29:51]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2017-01-23 18:58:03]

lgtm

[yzshen1, 2017-01-23 18:58:39]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2017-01-23 18:59: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-01-23 20:40:42]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1485197919862930, "parent_rev":
"dc25423dd85608261051a18d0f45224d70228d12", "commit_rev":
"8be41d3abfb2a8f5d7b58b3e70ebbc86fc7aa5d6"}

[commit-bot: I haz the power, 2017-01-23 20:41:18]

Description was changed from

==========
Mojo bindings: merge the control messages of sending disconnect reason and
notifying endpoint closed.

This is part of the work to remove AssociatedGroup and simplify the associated
interface API.

BUG=682334
==========

to

==========
Mojo bindings: merge the control messages of sending disconnect reason and
notifying endpoint closed.

This is part of the work to remove AssociatedGroup and simplify the associated
interface API.

BUG=682334

Review-Url: https://codereview.chromium.org/2649573004
Cr-Commit-Position: refs/heads/master@{#445473}
Committed:
https://chromium.googlesource.com/chromium/src/+/8be41d3abfb2a8f5d7b58b3e70eb...
==========

[commit-bot: I haz the power, 2017-01-23 20:41:19]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/8be41d3abfb2a8f5d7b58b3e70eb...

[dcheng, 2017-01-23 23:20:22]

LGTM

However, I'm wondering how the OWNERS check passed, since I think the mojom
files have a per-file security rule...

https://codereview.chromium.org/2649573004/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/scoped_interface_endpoint_handle.cc (right):

https://codereview.chromium.org/2649573004/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/scoped_interface_endpoint_handle.cc:43:
ResetInternal(reason);
FWIW, here and a couple other places

ResetInternal(DisconnectReason(custom_reason, description))

would also work and should save a copy.

[yzshen1, 2017-01-23 23:30:09]

Sorry Daniel. I did add you as security OWNER but somehow forgot to wait for
your review before I checked the CQ.

And I am also surprised that our PRESUMIT check didn't catch 
that.

https://codereview.chromium.org/2649573004/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/scoped_interface_endpoint_handle.cc (right):

https://codereview.chromium.org/2649573004/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/scoped_interface_endpoint_handle.cc:43:
ResetInternal(reason);
On 2017/01/23 23:20:22, dcheng wrote:
> FWIW, here and a couple other places
> 
> ResetInternal(DisconnectReason(custom_reason, description))
> 
> would also work and should save a copy.

I think the original code doesn't copy either. Have I missed anything?

But the change you proposed is more concise. I could make such a change in my
next CL.

[dcheng, 2017-01-24 03:39:21]

On 2017/01/23 23:30:09, yzshen1 wrote:
> Sorry Daniel. I did add you as security OWNER but somehow forgot to wait for
> your review before I checked the CQ.
> 
> And I am also surprised that our PRESUMIT check didn't catch 
> that.
> 

No problem, it's the tool's job to enforce it, not the developer's anyway. I'll
figure out what's changed.

https://codereview.chromium.org/2649573004/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/scoped_interface_endpoint_handle.cc (right):

https://codereview.chromium.org/2649573004/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/scoped_interface_endpoint_handle.cc:43:
ResetInternal(reason);
On 2017/01/23 23:30:09, yzshen1 wrote:
> On 2017/01/23 23:20:22, dcheng wrote:
> > FWIW, here and a couple other places
> > 
> > ResetInternal(DisconnectReason(custom_reason, description))
> > 
> > would also work and should save a copy.
> 
> I think the original code doesn't copy either. Have I missed anything?
> 
> But the change you proposed is more concise. I could make such a change in my
> next CL.

Ah, you're right: my comment is only true if |reason| is passed by value instead
of const ref. Sorry!