Make HandleWatcher watch on the same thread if the thread is running a MessagePumpMojo.

Make HandleWatcher watch on the same thread if the thread is running a MessagePumpMojo.

This CL also makes ApplicationRunnerChromium use MessagePumpMojo by default

BUG=None
TEST=None

Committed: https://crrev.com/d928bdd26e4800d3ffc76a56877dda8dafae26b4
Cr-Commit-Position: refs/heads/master@{#292702}

[yzshen1, 2014-08-27 20:39:53]

yzshen@chromium.org changed reviewers:
+ darin@chormium.org, sky@chromium.org

[darin (slow to review), 2014-08-27 21:20:43]

darin@chromium.org changed reviewers:
+ darin@chromium.org

[darin (slow to review), 2014-08-27 21:20:43]

LGTM

https://codereview.chromium.org/506353002/diff/60001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/lib/application_runner_chromium.cc (right):

https://codereview.chromium.org/506353002/diff/60001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/application_runner_chromium.cc:32:
DCHECK_EQ(base::MessageLoop::TYPE_CUSTOM, message_loop_type_);
I don't understand this assertion. Why is it invalid to change the message loop
type multiple times? Perhaps what we should do is assert that
set_message_loop_type is not called with TYPE_CUSTOM? And, we should probably
assert that it is not called after Run is called since it would have no effect
at that point.

It seems like set_message_loop_type() is also an unfortunate API if we want to
get rid of the TYPE_* enum, but changing that can wait for another day.

[sky, 2014-08-27 21:37:27]

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

https://codereview.chromium.org/506353002/diff/60001/mojo/common/handle_watch...
mojo/common/handle_watcher.cc:317: class HandleWatcher::StateBase : public
base::MessageLoop::DestructionObserver {
Is the DestructionObserver code necessary for SameThread variant? It seems like
it isn't as if the current thread is a MessagePumpMojo, then during it's
destruction it'll notify all handlers.

https://codereview.chromium.org/506353002/diff/60001/mojo/common/handle_watch...
mojo/common/handle_watcher.cc:356: };
DISALLOW_...

https://codereview.chromium.org/506353002/diff/60001/mojo/common/handle_watch...
mojo/common/handle_watcher.cc:358: class HandleWatcher::SameThreadWatchingState
: public StateBase,
Add description, especially since same thread is not clear.

https://codereview.chromium.org/506353002/diff/60001/mojo/common/handle_watch...
mojo/common/handle_watcher.cc:394: };
DISALLOW..

https://codereview.chromium.org/506353002/diff/60001/mojo/common/handle_watch...
mojo/common/handle_watcher.cc:427: };
DISALLOW...

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

https://codereview.chromium.org/506353002/diff/60001/mojo/common/message_pump...
mojo/common/message_pump_mojo.cc:66: g_tls_current_pump.Pointer()->Set(NULL);
DCHECK that this is the current pointer.

[darin (slow to review), 2014-08-27 21:58:26]

On 2014/08/27 21:37:27, sky wrote:
>
https://codereview.chromium.org/506353002/diff/60001/mojo/common/handle_watch...
> File mojo/common/handle_watcher.cc (right):
> 
>
https://codereview.chromium.org/506353002/diff/60001/mojo/common/handle_watch...
> mojo/common/handle_watcher.cc:317: class HandleWatcher::StateBase : public
> base::MessageLoop::DestructionObserver {
> Is the DestructionObserver code necessary for SameThread variant? It seems
like
> it isn't as if the current thread is a MessagePumpMojo, then during it's
> destruction it'll notify all handlers.
>

Good catch. It should not be necessary.

-Darin

[yzshen1, 2014-08-27 22:31:48]

Thanks, Darin and Scott!

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

https://codereview.chromium.org/506353002/diff/60001/mojo/common/handle_watch...
mojo/common/handle_watcher.cc:317: class HandleWatcher::StateBase : public
base::MessageLoop::DestructionObserver {
On 2014/08/27 21:37:27, sky wrote:
> Is the DestructionObserver code necessary for SameThread variant? It seems
like
> it isn't as if the current thread is a MessagePumpMojo, then during it's
> destruction it'll notify all handlers.

Thanks for the suggestion!
According to discussion with Scott, I added a TODO to remove the observer,
because the aforementioned behavior hasn't been landed.

(Or please let me know if you think I should hold off this CL until that one
lands.)

https://codereview.chromium.org/506353002/diff/60001/mojo/common/handle_watch...
mojo/common/handle_watcher.cc:356: };
On 2014/08/27 21:37:27, sky wrote:
> DISALLOW_...

Done.

https://codereview.chromium.org/506353002/diff/60001/mojo/common/handle_watch...
mojo/common/handle_watcher.cc:394: };
On 2014/08/27 21:37:27, sky wrote:
> DISALLOW..

Done.

https://codereview.chromium.org/506353002/diff/60001/mojo/common/handle_watch...
mojo/common/handle_watcher.cc:427: };
On 2014/08/27 21:37:27, sky wrote:
> DISALLOW...

Done.

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

https://codereview.chromium.org/506353002/diff/60001/mojo/common/message_pump...
mojo/common/message_pump_mojo.cc:66: g_tls_current_pump.Pointer()->Set(NULL);
On 2014/08/27 21:37:27, sky wrote:
> DCHECK that this is the current pointer.

Done.

https://codereview.chromium.org/506353002/diff/60001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/lib/application_runner_chromium.cc (right):

https://codereview.chromium.org/506353002/diff/60001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/application_runner_chromium.cc:32:
DCHECK_EQ(base::MessageLoop::TYPE_CUSTOM, message_loop_type_);
On 2014/08/27 21:20:43, darin wrote:
> I don't understand this assertion. Why is it invalid to change the message
loop
> type multiple times? 
I honestly didn't think about it. :)

> Perhaps what we should do is assert that
> set_message_loop_type is not called with TYPE_CUSTOM?> And, we should probably
> assert that it is not called after Run is called since it would have no effect
> at that point.

Done.

> It seems like set_message_loop_type() is also an unfortunate API if we want to
> get rid of the TYPE_* enum, but changing that can wait for another day.
Okay. Thanks!

[sky, 2014-08-27 22:49:46]

You should add test coverage too.

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

https://codereview.chromium.org/506353002/diff/80001/mojo/common/handle_watch...
mojo/common/handle_watcher.cc:336: void NotifyHandleReady(MojoResult result) {
nit: make this (and got_ready()) protected.

https://codereview.chromium.org/506353002/diff/80001/mojo/common/handle_watch...
mojo/common/handle_watcher.cc:392: DCHECK_EQ(handle.value(), handle_.value());
nit: refactor this and OnHandleError into a common function as implementations
are the same.

[yzshen1, 2014-08-28 19:26:06]

Thanks Scott.

Please take another look.

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

https://codereview.chromium.org/506353002/diff/80001/mojo/common/handle_watch...
mojo/common/handle_watcher.cc:319: // TODO(yzshen): Very soon MessagePumpMojo
will be changed to notify all handles
(Actually we don't have such a plan, so removed the TODO.)

https://codereview.chromium.org/506353002/diff/80001/mojo/common/handle_watch...
mojo/common/handle_watcher.cc:336: void NotifyHandleReady(MojoResult result) {
On 2014/08/27 22:49:45, sky wrote:
> nit: make this (and got_ready()) protected.

Done.

https://codereview.chromium.org/506353002/diff/80001/mojo/common/handle_watch...
mojo/common/handle_watcher.cc:392: DCHECK_EQ(handle.value(), handle_.value());
On 2014/08/27 22:49:45, sky wrote:
> nit: refactor this and OnHandleError into a common function as implementations
> are the same.

Done.

[sky, 2014-08-28 23:12:19]

Everything else looks good

https://codereview.chromium.org/506353002/diff/100001/mojo/common/handle_watc...
File mojo/common/handle_watcher_unittest.cc (right):

https://codereview.chromium.org/506353002/diff/100001/mojo/common/handle_watc...
mojo/common/handle_watcher_unittest.cc:62: break;
NOTREACHED

https://codereview.chromium.org/506353002/diff/100001/mojo/common/handle_watc...
mojo/common/handle_watcher_unittest.cc:184: while (NextTestConfig()) {
Did you look into making these parameterized tests:
https://code.google.com/p/googletest/wiki/AdvancedGuide#Value_Parameterized_T...
?

[yzshen1, 2014-08-29 00:23:45]

Thanks Scott!
Please take another look.

https://codereview.chromium.org/506353002/diff/100001/mojo/common/handle_watc...
File mojo/common/handle_watcher_unittest.cc (right):

https://codereview.chromium.org/506353002/diff/100001/mojo/common/handle_watc...
mojo/common/handle_watcher_unittest.cc:62: break;
On 2014/08/28 23:12:19, sky wrote:
> NOTREACHED

This 'case' is removed entirely.

https://codereview.chromium.org/506353002/diff/100001/mojo/common/handle_watc...
mojo/common/handle_watcher_unittest.cc:184: while (NextTestConfig()) {
On 2014/08/28 23:12:19, sky wrote:
> Did you look into making these parameterized tests:
>
https://code.google.com/p/googletest/wiki/AdvancedGuide#Value_Parameterized_T...
> ?

Great suggestion!

[sky, 2014-08-29 02:18:50]

LGTM

[yzshen1, 2014-08-29 02:20:18]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2014-08-29 02:21:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yzshen@chromium.org/506353002/140001

[commit-bot: I haz the power, 2014-08-29 03:30:07]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[commit-bot: I haz the power, 2014-08-29 03:36:51]

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

[commit-bot: I haz the power, 2014-08-29 03:36:52]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[yzshen1, 2014-08-29 18:54:29]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2014-08-29 18:55:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yzshen@chromium.org/506353002/160001

[commit-bot: I haz the power, 2014-08-29 22:52:29]

Committed patchset #9 (id:160001) as 542fc641c6c9871f7843f63824f954ba9c82931a

[commit-bot: I haz the power, 2014-09-10 03:11:14]

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