Implements HandleWatcher in terms of MessagePumpMojo

Implements HandleWatcher in terms of MessagePumpMojo

BUG=none
TEST=none
R=darin@chromium.org

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

[darin (slow to review), 2013-11-21 18:14:51]

https://codereview.chromium.org/69883008/diff/20001/mojo/common/handle_watche...
File mojo/common/handle_watcher.cc (right):

https://codereview.chromium.org/69883008/diff/20001/mojo/common/handle_watche...
mojo/common/handle_watcher.cc:30: // TODO(sky): shouldn't be necessary to cache
this.
This is going to be fixed once we sort out the RTTI business for MessageLoop?
Would be good to note that in this comment. You might even write a
GetCurrentMessagePumpMojo() function so the rest of this file doesn't need to
directly access message_pump_mojo_.

https://codereview.chromium.org/69883008/diff/20001/mojo/common/handle_watche...
mojo/common/handle_watcher.cc:31: MessagePumpMojo* message_pump_mojo_ = NULL;
nit: shouldn't end with "_"

https://codereview.chromium.org/69883008/diff/20001/mojo/common/handle_watche...
mojo/common/handle_watcher.cc:123: data.message_loop->PostTask(FROM_HERE,
base::Bind(data.callback, result));
Hmm, when called from OnHandleReady, this PostTask is not necessary. If we
changed OnHandleError to only get called from MessagePumpMojo::Run, then we
wouldn't need to use PostTask in that case either. (I'm concerned about
notification latency.)

https://codereview.chromium.org/69883008/diff/20001/mojo/common/message_pump_...
File mojo/common/message_pump_mojo.cc (right):

https://codereview.chromium.org/69883008/diff/20001/mojo/common/message_pump_...
mojo/common/message_pump_mojo.cc:55: handlers_[handle].handler = handler;
nit: it'd probably be worth optimizing this code to not perform so many lookups
in the map. one approach would be to just construct a new Handler and then do
handlers_[handle] = new_handler;

https://codereview.chromium.org/69883008/diff/20001/mojo/common/message_pump_...
mojo/common/message_pump_mojo.cc:59: old_handler.handler->OnHandleError(handle,
MOJO_RESULT_CANCELLED);
I'm a little bit worried about calling OnHandleError on some potentially deeply
nested call stack. Normally, calls to the Handler only happen from the run loop.
How important is it to make this callback?

https://codereview.chromium.org/69883008/diff/20001/mojo/common/message_pump_...
mojo/common/message_pump_mojo.cc:159: handler.handler->OnHandleError(handle,
MOJO_RESULT_DEADLINE_EXCEEDED);
OnHandleError can call AddHandler or RemoveHandler which could mess up your
iteration here. You probably want to build a separate list of handlers to
notify, and then notify them all as a separate loop.

https://codereview.chromium.org/69883008/diff/20001/mojo/common/message_pump_...
mojo/common/message_pump_mojo.cc:174: // the handle too our iterator isn't
invalidated.
nit: too -> to

[sky, 2013-11-22 19:39:44]

All addressed. Take another look?

https://codereview.chromium.org/69883008/diff/20001/mojo/common/handle_watche...
File mojo/common/handle_watcher.cc (right):

https://codereview.chromium.org/69883008/diff/20001/mojo/common/handle_watche...
mojo/common/handle_watcher.cc:30: // TODO(sky): shouldn't be necessary to cache
this.
On 2013/11/21 18:14:52, darin wrote:
> This is going to be fixed once we sort out the RTTI business for MessageLoop?
> Would be good to note that in this comment. You might even write a
> GetCurrentMessagePumpMojo() function so the rest of this file doesn't need to
> directly access message_pump_mojo_.

I updated the TODO for now.

https://codereview.chromium.org/69883008/diff/20001/mojo/common/handle_watche...
mojo/common/handle_watcher.cc:31: MessagePumpMojo* message_pump_mojo_ = NULL;
On 2013/11/21 18:14:52, darin wrote:
> nit: shouldn't end with "_"

Done.

https://codereview.chromium.org/69883008/diff/20001/mojo/common/handle_watche...
mojo/common/handle_watcher.cc:123: data.message_loop->PostTask(FROM_HERE,
base::Bind(data.callback, result));
On 2013/11/21 18:14:52, darin wrote:
> Hmm, when called from OnHandleReady, this PostTask is not necessary. If we
> changed OnHandleError to only get called from MessagePumpMojo::Run, then we
> wouldn't need to use PostTask in that case either. (I'm concerned about
> notification latency.)

Remember this is on the backend thread, so the PostTask is always necessary.

https://codereview.chromium.org/69883008/diff/20001/mojo/common/message_pump_...
File mojo/common/message_pump_mojo.cc (right):

https://codereview.chromium.org/69883008/diff/20001/mojo/common/message_pump_...
mojo/common/message_pump_mojo.cc:55: handlers_[handle].handler = handler;
On 2013/11/21 18:14:52, darin wrote:
> nit: it'd probably be worth optimizing this code to not perform so many
lookups
> in the map. one approach would be to just construct a new Handler and then do
> handlers_[handle] = new_handler;

Done.

https://codereview.chromium.org/69883008/diff/20001/mojo/common/message_pump_...
mojo/common/message_pump_mojo.cc:59: old_handler.handler->OnHandleError(handle,
MOJO_RESULT_CANCELLED);
On 2013/11/21 18:14:52, darin wrote:
> I'm a little bit worried about calling OnHandleError on some potentially
deeply
> nested call stack. Normally, calls to the Handler only happen from the run
loop.
> How important is it to make this callback?

As discussed I made this DCHECK that there isn't an existing handler.

https://codereview.chromium.org/69883008/diff/20001/mojo/common/message_pump_...
mojo/common/message_pump_mojo.cc:159: handler.handler->OnHandleError(handle,
MOJO_RESULT_DEADLINE_EXCEEDED);
On 2013/11/21 18:14:52, darin wrote:
> OnHandleError can call AddHandler or RemoveHandler which could mess up your
> iteration here. You probably want to build a separate list of handlers to
> notify, and then notify them all as a separate loop.

Done.

https://codereview.chromium.org/69883008/diff/20001/mojo/common/message_pump_...
mojo/common/message_pump_mojo.cc:174: // the handle too our iterator isn't
invalidated.
On 2013/11/21 18:14:52, darin wrote:
> nit: too -> to

Done.

[darin (slow to review), 2013-11-22 23:03:31]

https://codereview.chromium.org/69883008/diff/110001/mojo/common/message_pump...
File mojo/common/message_pump_mojo.cc (right):

https://codereview.chromium.org/69883008/diff/110001/mojo/common/message_pump...
mojo/common/message_pump_mojo.cc:160: handlers_.find(i->first) !=
handlers_.end() &&
I think there could be subtle bugs here with the handler being removed and
re-added with a different deadline not equal to i->second.deadline.

Too bad we don't have a map equivalent to base/observer_list.h.
base/id_map.{h,cc} implements mutation safe iteration.

Maybe you could have a map to entries in the ObserverList? That way when you
need fast lookup, you go through the map, and when you need to iterate over all
entries, you just use the ObserverList.

[sky, 2013-11-22 23:25:25]

Take another look?

https://codereview.chromium.org/69883008/diff/110001/mojo/common/message_pump...
File mojo/common/message_pump_mojo.cc (right):

https://codereview.chromium.org/69883008/diff/110001/mojo/common/message_pump...
mojo/common/message_pump_mojo.cc:160: handlers_.find(i->first) !=
handlers_.end() &&
On 2013/11/22 23:03:32, darin wrote:
> I think there could be subtle bugs here with the handler being removed and
> re-added with a different deadline not equal to i->second.deadline.
> 
> Too bad we don't have a map equivalent to base/observer_list.h.
> base/id_map.{h,cc} implements mutation safe iteration.
> 
> Maybe you could have a map to entries in the ObserverList? That way when you
> need fast lookup, you go through the map, and when you need to iterate over
all
> entries, you just use the ObserverList.

I went with an id for equality.

[darin (slow to review), 2013-11-22 23:31:58]

LGTM, clever!

[commit-bot: I haz the power, 2013-11-23 06:59:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/69883008/310001

[commit-bot: I haz the power, 2013-11-23 19:04:22]

Change committed as 236954