Mojo: Add BindingsSupportImpl on top of HandleWatcher

Mojo: Add BindingsSupportImpl on top of HandleWatcher

Plumb MojoResult to the callback set on HandleWatcher. Revise BindingsSupport interface to make the implementation on top of HandleWatcher trivial.

R=sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234247

[sky, 2013-11-08 16:44:01]

https://codereview.chromium.org/62773003/diff/130001/mojo/common/bindings_sup...
File mojo/common/bindings_support_impl.cc (right):

https://codereview.chromium.org/62773003/diff/130001/mojo/common/bindings_sup...
mojo/common/bindings_support_impl.cc:27: }
Is there a expectation that deleting BindingsSupportImpl implicitly cancels all
outstanding async waits?

https://codereview.chromium.org/62773003/diff/130001/mojo/common/bindings_sup...
mojo/common/bindings_support_impl.cc:44: HandleWatcher* watcher =
static_cast<HandleWatcher*>(async_wait_id);
I'm surprised you don't need reinterpret_cast here.

https://codereview.chromium.org/62773003/diff/130001/mojo/common/bindings_sup...
File mojo/common/bindings_support_impl.h (right):

https://codereview.chromium.org/62773003/diff/130001/mojo/common/bindings_sup...
mojo/common/bindings_support_impl.h:26: };
private: DISALLOW...

https://codereview.chromium.org/62773003/diff/130001/mojo/common/handle_watch...
File mojo/common/handle_watcher.cc (right):

https://codereview.chromium.org/62773003/diff/130001/mojo/common/handle_watch...
mojo/common/handle_watcher.cc:388: }
Can you add a comment that we may have been deleted here. We should add a test
for this at some point too.

https://codereview.chromium.org/62773003/diff/130001/mojo/public/bindings/lib...
File mojo/public/bindings/lib/connector.h (right):

https://codereview.chromium.org/62773003/diff/130001/mojo/public/bindings/lib...
mojo/public/bindings/lib/connector.h:55: };
DISALLOW_...?

[darin (slow to review), 2013-11-08 21:29:17]

On 2013/11/08 16:44:01, sky wrote:
>
https://codereview.chromium.org/62773003/diff/130001/mojo/common/bindings_sup...
> File mojo/common/bindings_support_impl.cc (right):
> 
>
https://codereview.chromium.org/62773003/diff/130001/mojo/common/bindings_sup...
> mojo/common/bindings_support_impl.cc:27: }
> Is there a expectation that deleting BindingsSupportImpl implicitly cancels
all
> outstanding async waits?

No. Rather, I has assumed that we would have stopped using the bindings system
before tearing down the BindingsSupportImpl. On that note, I think I will add a
counter of the active HandleWatcher objects. In ~BindingsSupportImpl, I'll
assert that the active count is zero.


> 
>
https://codereview.chromium.org/62773003/diff/130001/mojo/common/bindings_sup...
> mojo/common/bindings_support_impl.cc:44: HandleWatcher* watcher =
> static_cast<HandleWatcher*>(async_wait_id);
> I'm surprised you don't need reinterpret_cast here.
> 

Any pointer type "is a" void pointer, so static_cast works. Maybe this is
revealing the implementation detail of AsyncWaitID :-/


>
https://codereview.chromium.org/62773003/diff/130001/mojo/common/bindings_sup...
> File mojo/common/bindings_support_impl.h (right):
> 
>
https://codereview.chromium.org/62773003/diff/130001/mojo/common/bindings_sup...
> mojo/common/bindings_support_impl.h:26: };
> private: DISALLOW...

Fixed.

> 
>
https://codereview.chromium.org/62773003/diff/130001/mojo/common/handle_watch...
> File mojo/common/handle_watcher.cc (right):
> 
>
https://codereview.chromium.org/62773003/diff/130001/mojo/common/handle_watch...
> mojo/common/handle_watcher.cc:388: }
> Can you add a comment that we may have been deleted here.

Good idea.


> We should add a test for this at some point too.

Good idea. I'll take a look at adding that.


> 
>
https://codereview.chromium.org/62773003/diff/130001/mojo/public/bindings/lib...
> File mojo/public/bindings/lib/connector.h (right):
> 
>
https://codereview.chromium.org/62773003/diff/130001/mojo/public/bindings/lib...
> mojo/public/bindings/lib/connector.h:55: };
> DISALLOW_...?

Thanks, fixed.

[sky, 2013-11-08 21:47:35]

Ok, LGTM

[darin (slow to review), 2013-11-09 00:30:14]

PTAL

I added an assertion that all HandleWatchers have been deleted that were created
by BindingsSupportImpl.

I also added a unit test that deleting a HandleWatcher inside the callback is
OK.

[darin (slow to review), 2013-11-09 05:38:54]

And now BindingsSupportImpl uses a member variable to count the number of active
HandleWatcher instances.

[sky, 2013-11-11 17:23:50]

LGTM

https://codereview.chromium.org/62773003/diff/500001/mojo/common/bindings_sup...
File mojo/common/bindings_support_impl.cc (right):

https://codereview.chromium.org/62773003/diff/500001/mojo/common/bindings_sup...
mojo/common/bindings_support_impl.cc:14: // The Context struct is used to track
the number of HandleWatcher objects in
nit: struct -> class

https://codereview.chromium.org/62773003/diff/500001/mojo/common/bindings_sup...
mojo/common/bindings_support_impl.cc:45: HandleWatcher* watcher = new
HandleWatcher();
nit: document destroyed when done or canceled.

[darin (slow to review), 2013-11-11 18:47:28]

Committed patchset #8 manually as r234247 (presubmit successful).

[darin (slow to review), 2013-11-11 22:11:15]

On 2013/11/11 18:47:28, darin wrote:
> Committed patchset #8 manually as r234247 (presubmit successful).

This change had to be reverted due to Android bustage. Revised CL is here:
https://codereview.chromium.org/69633002