Implements mojo::StrongBindingSet.

Implements mojo::StrongBindingSet.

Review-Url: https://codereview.chromium.org/2692413002
Cr-Commit-Position: refs/heads/master@{#450631}
Committed: https://chromium.googlesource.com/chromium/src/+/363e6ab2daf14159f94823b004b23e7cc1619728

[alokp, 2017-02-15 05:42:44]

alokp@chromium.org changed reviewers:
+ rockot@chromium.org, xhwang@chromium.org

[alokp, 2017-02-15 05:42:45]

xhwang@: If you do not have any objections, I can replace
media::StrongBindingSet with this one in a follow-up patch. Please let me know.

[Ken Rockot(use gerrit already), 2017-02-15 06:23:35]

Lgtm - thanks for doing this!

[alokp, 2017-02-15 06:27:45]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2017-02-15 06:28:19]

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-15 07:53:08]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1487140065377930, "parent_rev":
"1ed8301be8b59170a3c11e793d86696f17b44940", "commit_rev":
"363e6ab2daf14159f94823b004b23e7cc1619728"}

[commit-bot: I haz the power, 2017-02-15 07:53:41]

Description was changed from

==========
Implements mojo::StrongBindingSet.
==========

to

==========
Implements mojo::StrongBindingSet.

Review-Url: https://codereview.chromium.org/2692413002
Cr-Commit-Position: refs/heads/master@{#450631}
Committed:
https://chromium.googlesource.com/chromium/src/+/363e6ab2daf14159f94823b004b2...
==========

[commit-bot: I haz the power, 2017-02-15 07:53:44]

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

[xhwang, 2017-02-16 03:54:50]

Thanks for the patch!

Sorry I have some late comments, but mostly nits.

Yes, it'll be great if you could help replace media::StrongBindingSet with this
new one.

https://codereview.chromium.org/2692413002/diff/1/mojo/public/cpp/bindings/st...
File mojo/public/cpp/bindings/strong_binding_set.h (right):

https://codereview.chromium.org/2692413002/diff/1/mojo/public/cpp/bindings/st...
mojo/public/cpp/bindings/strong_binding_set.h:18: template <typename Interface,
typename ContextType = void>
The comment above is copied from media::StrongBindingSet. But in that interface
we don't have ContextType. So how would a user of this class know how to use
ContextType without looking at implementation details?

https://codereview.chromium.org/2692413002/diff/1/mojo/public/cpp/bindings/te...
File mojo/public/cpp/bindings/tests/binding_set_unittest.cc (right):

https://codereview.chromium.org/2692413002/diff/1/mojo/public/cpp/bindings/te...
mojo/public/cpp/bindings/tests/binding_set_unittest.cc:411: EXPECT_EQ(0,
PingInstanceCounter::instance_count);
What happens if you call RemoveBinding with a random/invalid binding ID?

[xhwang, 2017-02-16 04:03:07]

https://codereview.chromium.org/2692413002/diff/1/mojo/public/cpp/bindings/st...
File mojo/public/cpp/bindings/strong_binding_set.h (right):

https://codereview.chromium.org/2692413002/diff/1/mojo/public/cpp/bindings/st...
mojo/public/cpp/bindings/strong_binding_set.h:18: template <typename Interface,
typename ContextType = void>
On 2017/02/16 03:54:49, xhwang_slow wrote:
> The comment above is copied from media::StrongBindingSet. But in that
interface
> we don't have ContextType. So how would a user of this class know how to use
> ContextType without looking at implementation details?

Nevermind, I guess user of this class has to look at BindingSetBase anyways
since this is only a "using" alias.

[alokp, 2017-02-16 04:42:47]

https://codereview.chromium.org/2692413002/diff/1/mojo/public/cpp/bindings/te...
File mojo/public/cpp/bindings/tests/binding_set_unittest.cc (right):

https://codereview.chromium.org/2692413002/diff/1/mojo/public/cpp/bindings/te...
mojo/public/cpp/bindings/tests/binding_set_unittest.cc:411: EXPECT_EQ(0,
PingInstanceCounter::instance_count);
On 2017/02/16 03:54:49, xhwang_slow wrote:
> What happens if you call RemoveBinding with a random/invalid binding ID?

That would be a test for any BindingSet derived from BindingSetBase. Removing an
invalid binding id is not related specifically to StrongBindingSet. I guess we
need some type-parametrized tests.

https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...

[Ken Rockot(use gerrit already), 2017-02-16 04:44:20]

On Wed, Feb 15, 2017 at 8:42 PM, <alokp@chromium.org> wrote:

>
> https://codereview.chromium.org/2692413002/diff/1/mojo/
> public/cpp/bindings/tests/binding_set_unittest.cc
> File mojo/public/cpp/bindings/tests/binding_set_unittest.cc (right):
>
> https://codereview.chromium.org/2692413002/diff/1/mojo/
> public/cpp/bindings/tests/binding_set_unittest.cc#newcode411
> mojo/public/cpp/bindings/tests/binding_set_unittest.cc:411: EXPECT_EQ(0,
> PingInstanceCounter::instance_count);
> On 2017/02/16 03:54:49, xhwang_slow wrote:
> > What happens if you call RemoveBinding with a random/invalid binding
> ID?
>
> That would be a test for any BindingSet derived from BindingSetBase.
> Removing an invalid binding id is not related specifically to
> StrongBindingSet. I guess we need some type-parametrized tests.
>

Is this really worth unit testing? The implementation is trivial.


>
> https://github.com/google/googletest/blob/master/
> googletest/docs/AdvancedGuide.md#type-parameterized-tests
>
> https://codereview.chromium.org/2692413002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.