Add support for dispatch contexts on BindingSet

Add support for dispatch contexts on BindingSet

Allows BindingSet<T>::AddBinding to associate an arbitrary
|context| value with a new binding when added. The set's
dispatch_context() accessor will return this value during
the extent of any message or error dispatch targeting
that specific binding.

Refactors BindingSet to simplify lifetime management
and allow it to be reused for associated bindings as well.

Also implements AddFilter for master interface bindings on
interface types which support associated interfaces, as this
was accidentally omitted when AddFilter was introduced.

BUG=612500
R=yzshen@chromium.org

Committed: https://crrev.com/21bb88540cbc926f52b7e6547984ea772c441a69
Cr-Commit-Position: refs/heads/master@{#414809}

[Ken Rockot(use gerrit already), 2016-08-25 23:31:46]

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

[commit-bot: I haz the power, 2016-08-25 23:33:00]

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

[Ken Rockot(use gerrit already), 2016-08-25 23:33:15]

Hi Yuzhu, could you please take a look at this? Thanks!

[commit-bot: I haz the power, 2016-08-25 23:47:07]

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

[commit-bot: I haz the power, 2016-08-25 23:47:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ken Rockot(use gerrit already), 2016-08-26 01:31:35]

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

[commit-bot: I haz the power, 2016-08-26 01:32:14]

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

[Ken Rockot(use gerrit already), 2016-08-26 01:32:40]

Description was changed from

==========
Add support for dispatch contexts on BindingSet

Allows BindingSet<T>::AddBinding to associate an arbitrary
|context| value with a new binding when added. The set's
dispatch_context() accessor will return this value during
the extent of any message or error dispatch targeting
that specific binding.

Also refactors BindingSet to simplify lifetime management
and allow it to be reused for associated bindings as well.

BUG=612500
R=yzshen@chromium.org
==========

to

==========
Add support for dispatch contexts on BindingSet

Allows BindingSet<T>::AddBinding to associate an arbitrary
|context| value with a new binding when added. The set's
dispatch_context() accessor will return this value during
the extent of any message or error dispatch targeting
that specific binding.

Refactors BindingSet to simplify lifetime management
and allow it to be reused for associated bindings as well.

Also implements AddFilter for master interface bindings on
interface types which support associated interfaces, as this
was accidentally omitted when AddFilter was introduced.

BUG=612500
R=yzshen@chromium.org
==========

[commit-bot: I haz the power, 2016-08-26 03:11:05]

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

[commit-bot: I haz the power, 2016-08-26 03:11:06]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[yzshen1, 2016-08-26 16:30:47]

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

https://codereview.chromium.org/2283543002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/binding_set.h:60: AddBinding(impl,
Traits::GetProxy(&proxy));
Is it intentional that we don't provide a similar method for
AssociatedBindingSet?

https://codereview.chromium.org/2283543002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/binding_set.h:64: void CloseAllBindings() {}
Why this is a non-op?

https://codereview.chromium.org/2283543002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/binding_set.h:87:
binding_.AddFilter(base::MakeUnique<DispatchFilter>(this));
Do we expect that setting context is a common operation for BindingSet? If not,
maybe we could consider a mode of BindingSet which skips adding such filter.
This filter is one more layer of method call (which won't be inlined) for each
message.

What do you think?

[yzshen1, 2016-08-26 16:30:48]

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

https://codereview.chromium.org/2283543002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/binding_set.h:60: AddBinding(impl,
Traits::GetProxy(&proxy));
Is it intentional that we don't provide a similar method for
AssociatedBindingSet?

https://codereview.chromium.org/2283543002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/binding_set.h:64: void CloseAllBindings() {}
Why this is a non-op?

https://codereview.chromium.org/2283543002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/binding_set.h:87:
binding_.AddFilter(base::MakeUnique<DispatchFilter>(this));
Do we expect that setting context is a common operation for BindingSet? If not,
maybe we could consider a mode of BindingSet which skips adding such filter.
This filter is one more layer of method call (which won't be inlined) for each
message.

What do you think?

[Ken Rockot(use gerrit already), 2016-08-26 16:58:54]

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

[commit-bot: I haz the power, 2016-08-26 16:59:38]

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

[Ken Rockot(use gerrit already), 2016-08-26 17:01:20]

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

https://codereview.chromium.org/2283543002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/binding_set.h:60: AddBinding(impl,
Traits::GetProxy(&proxy));
On 2016/08/26 at 16:30:48, yzshen1 wrote:
> Is it intentional that we don't provide a similar method for
AssociatedBindingSet?

It's not possible to implement since we wouldn't know what group to associate
with. In practice, all bindings in an AssociatedBindingSet will likely belong to
the same group, but that isn't something we know for sure. There's no link to a
master interface.

We could add a version of CreateInterfacePtrAndBind which takes an
AssociatedGroup and calls a different GetProxy signature, but I'm not sure I
like CreateInterfacePtrAndBind anyway. WDYT?

https://codereview.chromium.org/2283543002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/binding_set.h:64: void CloseAllBindings() {}
On 2016/08/26 at 16:30:48, yzshen1 wrote:
> Why this is a non-op?

Oops :>  Fixed

https://codereview.chromium.org/2283543002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/binding_set.h:87:
binding_.AddFilter(base::MakeUnique<DispatchFilter>(this));
On 2016/08/26 at 16:30:48, yzshen1 wrote:
> Do we expect that setting context is a common operation for BindingSet? If
not, maybe we could consider a mode of BindingSet which skips adding such
filter. This filter is one more layer of method call (which won't be inlined)
for each message.
> 
> What do you think?

Seems reasonable. I've added a mojo::BindingSetDispatchMode enum and a
BindingSet constructor which takes takes it. Default constructor does
WITHOUT_CONTEXT.

[Ken Rockot(use gerrit already), 2016-08-26 17:08:59]

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

[commit-bot: I haz the power, 2016-08-26 17:09:34]

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

[Ken Rockot(use gerrit already), 2016-08-26 17:12:44]

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

[commit-bot: I haz the power, 2016-08-26 17:12:58]

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

[yzshen1, 2016-08-26 18:11:29]

lgtm

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

https://codereview.chromium.org/2283543002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/binding_set.h:60: AddBinding(impl,
Traits::GetProxy(&proxy));
On 2016/08/26 17:01:20, Ken Rockot wrote:
> On 2016/08/26 at 16:30:48, yzshen1 wrote:
> > Is it intentional that we don't provide a similar method for
> AssociatedBindingSet?
> 
> It's not possible to implement since we wouldn't know what group to associate
> with. In practice, all bindings in an AssociatedBindingSet will likely belong
to
> the same group, but that isn't something we know for sure. There's no link to
a
> master interface.
> 
> We could add a version of CreateInterfacePtrAndBind which takes an
> AssociatedGroup and calls a different GetProxy signature, but I'm not sure I
> like CreateInterfacePtrAndBind anyway. WDYT?

Right. I don't think it is that important to support this method for associated
bindings.

https://codereview.chromium.org/2283543002/diff/70001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/binding_set.h (right):

https://codereview.chromium.org/2283543002/diff/70001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/binding_set.h:135: if (binding_set_->SupportsContext())
nit: we could move this check out of WillDispatch(), only OnConnectionError
needs this check.

[commit-bot: I haz the power, 2016-08-26 20:03:28]

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

[commit-bot: I haz the power, 2016-08-26 20:03:29]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-08-26 20:09:02]

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

[commit-bot: I haz the power, 2016-08-26 20:09:39]

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

[Ken Rockot(use gerrit already), 2016-08-26 20:11:02]

The CQ bit was unchecked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-08-26 20:11:05]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-08-26 20:11:06]

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/2283543002/#ps90001
(title: ".")

[commit-bot: I haz the power, 2016-08-26 20:11:25]

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

[Ken Rockot(use gerrit already), 2016-08-26 20:15:02]

https://codereview.chromium.org/2283543002/diff/70001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/binding_set.h (right):

https://codereview.chromium.org/2283543002/diff/70001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/binding_set.h:135: if (binding_set_->SupportsContext())
On 2016/08/26 at 18:11:28, yzshen1 wrote:
> nit: we could move this check out of WillDispatch(), only OnConnectionError
needs this check.

Done

[commit-bot: I haz the power, 2016-08-26 21:21:53]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2016-08-26 21:25:03]

Description was changed from

==========
Add support for dispatch contexts on BindingSet

Allows BindingSet<T>::AddBinding to associate an arbitrary
|context| value with a new binding when added. The set's
dispatch_context() accessor will return this value during
the extent of any message or error dispatch targeting
that specific binding.

Refactors BindingSet to simplify lifetime management
and allow it to be reused for associated bindings as well.

Also implements AddFilter for master interface bindings on
interface types which support associated interfaces, as this
was accidentally omitted when AddFilter was introduced.

BUG=612500
R=yzshen@chromium.org
==========

to

==========
Add support for dispatch contexts on BindingSet

Allows BindingSet<T>::AddBinding to associate an arbitrary
|context| value with a new binding when added. The set's
dispatch_context() accessor will return this value during
the extent of any message or error dispatch targeting
that specific binding.

Refactors BindingSet to simplify lifetime management
and allow it to be reused for associated bindings as well.

Also implements AddFilter for master interface bindings on
interface types which support associated interfaces, as this
was accidentally omitted when AddFilter was introduced.

BUG=612500
R=yzshen@chromium.org

Committed: https://crrev.com/21bb88540cbc926f52b7e6547984ea772c441a69
Cr-Commit-Position: refs/heads/master@{#414809}
==========

[commit-bot: I haz the power, 2016-08-26 21:25:04]

Patchset 6 (id:??) landed as
https://crrev.com/21bb88540cbc926f52b7e6547984ea772c441a69
Cr-Commit-Position: refs/heads/master@{#414809}