Mojo C++ bindings: refactor SyncHandleWatcher.

Mojo C++ bindings: refactor SyncHandleWatcher.

This is a preparation to share more code with sync call support for
associated interfaces.

It also makes the sync handle watching API easier to understand.

BUG=577699
Committed: https://crrev.com/cd6fc12f440d5c30bdb55d74bb121d7c7d9ea834
Cr-Commit-Position: refs/heads/master@{#383458}

[yzshen1, 2016-03-25 20:42:28]

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

[yzshen1, 2016-03-25 20:42:29]

Hi, Ken.

Would you please take a look? Thanks!

[Ken Rockot(use gerrit already), 2016-03-26 03:09:27]

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

https://codereview.chromium.org/1832193002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/router.cc:284: DCHECK(!encountered_error_);
If this DCHECK is appropriate then the function should always return immediately
by the condition below. One of these should be removed, I think probably the
DCHECK?

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

https://codereview.chromium.org/1832193002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/sync_handle_watcher.cc:61: if (!registered_ &&
handle_.is_valid()) {
Is it OK to simply DCHECK that handle_.is_valid() given the comment on the
constructor which says handle must outlive this object?

[yzshen1, 2016-03-26 06:33:18]

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

https://codereview.chromium.org/1832193002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/router.cc:284: DCHECK(!encountered_error_);
On 2016/03/26 03:09:26, Ken Rockot wrote:
> If this DCHECK is appropriate then the function should always return
immediately
> by the condition below. One of these should be removed, I think probably the
> DCHECK?

Oops! The DCHECK should be removed. Thanks!

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

https://codereview.chromium.org/1832193002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/sync_handle_watcher.cc:61: if (!registered_ &&
handle_.is_valid()) {
On 2016/03/26 03:09:26, Ken Rockot wrote:
> Is it OK to simply DCHECK that handle_.is_valid() given the comment on the
> constructor which says handle must outlive this object?

I did it this way because I was thinking that it made SyncHandleWatcher worked
correctly with a null handle as input. (The check is not used to deal with
handle doesn't outlive the object, because in that case, the handle value won't
be updated to null when it is closed.)

Now that I looked closely, however, I realized that MojoAddHandle() system API
returns failure for null handle, that means SyncHandleRegistry::RegisterHandle()
already deals with null handle correctly. So I removed the check.

Thanks for pointing this out!

[Ken Rockot(use gerrit already), 2016-03-26 15:11:39]

lgtm

[yzshen1, 2016-03-26 16:44:34]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2016-03-26 16:44:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1832193002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1832193002/40001

[commit-bot: I haz the power, 2016-03-26 17:28:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-26 17:29:31]

Description was changed from

==========
Mojo C++ bindings: refactor SyncHandleWatcher.

This is a preparation to share more code with sync call support for
associated interfaces.

It also makes the sync handle watching API easier to understand.

BUG=577699
==========

to

==========
Mojo C++ bindings: refactor SyncHandleWatcher.

This is a preparation to share more code with sync call support for
associated interfaces.

It also makes the sync handle watching API easier to understand.

BUG=577699

Committed: https://crrev.com/cd6fc12f440d5c30bdb55d74bb121d7c7d9ea834
Cr-Commit-Position: refs/heads/master@{#383458}
==========

[commit-bot: I haz the power, 2016-03-26 17:29:33]

Patchset 3 (id:??) landed as
https://crrev.com/cd6fc12f440d5c30bdb55d74bb121d7c7d9ea834
Cr-Commit-Position: refs/heads/master@{#383458}