Make nesting/running states a RunLoop rather than a MessageLoop concept.

Make nesting/running states a RunLoop rather than a MessageLoop concept.

First CL of a series to split inter-dependency between RunLoop and MessageLoop.

Also modernized bluetooth_socket_bluez_unittest.cc away from using deprecated MessageLoop Quit
methods on its fixture's MessageLoop member to ease transition.

Lastly, tried to add thread safety checks for documentation purposes but turns
out there are already improper usage of the API... those will have to be
addressed first through issue 715235.

https://codereview.chromium.org/2828913003 follows to 
s/nested message loop/nested run loop/ in comments.

BUG=703346, 715235
Review-Url: https://codereview.chromium.org/2818533003
Cr-Commit-Position: refs/heads/master@{#469636}
Committed: https://chromium.googlesource.com/chromium/src/+/7af9dc076d2349ade19a95e24f6ee6811e939765

[gab, 2017-04-12 20:53:59]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 20:54:40]

Dry run: 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-04-12 21:05:43]

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

[commit-bot: I haz the power, 2017-04-12 21:05:44]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[gab, 2017-04-19 16:33:22]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 16:33:55]

Dry run: 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-04-19 16:38:01]

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

[commit-bot: I haz the power, 2017-04-19 16:38:02]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[gab, 2017-04-19 16:59:10]

Description was changed from

==========
Make nesting/running states a RunLoop rather than a MessageLoop concept.

BUG=
==========

to

==========
Make nesting/running states a RunLoop rather than a MessageLoop concept.

First CL of a series to split inter-dependency between RunLoop and MessageLoop.

Also modernized bluetooth_socket_bluez_unittest.cc away from using deprecated
MessageLoop Quit
methods on its fixture's MessageLoop member to ease transition.

BUG=703346
==========

[gab, 2017-04-19 16:59:10]

gab@chromium.org changed reviewers:
+ danakj@chromium.org

[gab, 2017-04-19 17:47:51]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 17:48:31]

Dry run: 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-04-19 18:01:29]

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

[commit-bot: I haz the power, 2017-04-19 18:01:30]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[gab, 2017-04-19 18:53:39]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 18:54:05]

Dry run: 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-04-19 19:08:24]

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

[commit-bot: I haz the power, 2017-04-19 19:08:25]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[gab, 2017-04-19 19:19:05]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 19:19:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-04-19 19:19:35]

Patchset #4 (id:60001) has been deleted

[gab, 2017-04-19 19:21:46]

gab@chromium.org changed reviewers:
+ sky@chromium.org, stevenjb@chromium.org

[gab, 2017-04-19 19:21:47]

Dana PTaL at overall change in //base

@other owners for side-effects in:
 - sky@ : chrome\browser\*

https://codereview.chromium.org/2818533003/diff/20001/base/message_loop/messa...
File base/message_loop/message_loop_test.h (left):

https://codereview.chromium.org/2818533003/diff/20001/base/message_loop/messa...
base/message_loop/message_loop_test.h:31: void
RunTest_NestingObserver(MessagePumpFactory factory);
This test listing paradigm is weird... and this one was missing from the list
below so wasn't actually running..! (double-checked that others are properly
listed)

[gab, 2017-04-19 19:29:52]

On 2017/04/19 19:21:47, gab wrote:
> Dana PTaL at overall change in //base
> 
> @other owners for side-effects in:

Oops, hit send early:
 - sky@: chrome\browser\* 
         + mash\* 
         + mojo\* 
         + services\service_manager 
         + services\ui 
         + ui\aura
 - stevenjb@: chromeos\*
 - pkasting@: components\omnibox\*
 - pavely@: components\sync\*
 - ncarter@: content\*
 - scheib@: device\bluetooth\*
 - mek@: extensions\*
 - tzik@: storage\browser\fileapi\*
 - skyostil@: third_party\WebKit\Source\platform\scheduler\*
 - 

> 
>
https://codereview.chromium.org/2818533003/diff/20001/base/message_loop/messa...
> File base/message_loop/message_loop_test.h (left):
> 
>
https://codereview.chromium.org/2818533003/diff/20001/base/message_loop/messa...
> base/message_loop/message_loop_test.h:31: void
> RunTest_NestingObserver(MessagePumpFactory factory);
> This test listing paradigm is weird... and this one was missing from the list
> below so wasn't actually running..! (double-checked that others are properly
> listed)

[gab, 2017-04-19 19:30:01]

gab@chromium.org changed reviewers:
+ mek@chromium.org, ncarter@chromium.org, pavely@chromium.org,
scheib@chromium.org, skyostil@chromium.org, tzik@chromium.org

[gab, 2017-04-19 19:32:00]

And one last time with everyone actually on reviewer list... sigh.. why can't I
automate this..! See below for your review assignment, thanks!

On 2017/04/19 19:29:52, gab wrote:
> On 2017/04/19 19:21:47, gab wrote:
> > Dana PTaL at overall change in //base
> > 
> > @other owners for side-effects in:
> 
> Oops, hit send early:
>  - sky@: chrome\browser\* 
>          + mash\* 
>          + mojo\* 
>          + services\service_manager 
>          + services\ui 
>          + ui\aura
>  - stevenjb@: chromeos\*
>  - pkasting@: components\omnibox\*
>  - pavely@: components\sync\*
>  - ncarter@: content\*
>  - scheib@: device\bluetooth\*
>  - mek@: extensions\*
>  - tzik@: storage\browser\fileapi\*
>  - skyostil@: third_party\WebKit\Source\platform\scheduler\*
>  - 
> 
> > 
> >
>
https://codereview.chromium.org/2818533003/diff/20001/base/message_loop/messa...
> > File base/message_loop/message_loop_test.h (left):
> > 
> >
>
https://codereview.chromium.org/2818533003/diff/20001/base/message_loop/messa...
> > base/message_loop/message_loop_test.h:31: void
> > RunTest_NestingObserver(MessagePumpFactory factory);
> > This test listing paradigm is weird... and this one was missing from the
list
> > below so wasn't actually running..! (double-checked that others are properly
> > listed)

[commit-bot: I haz the power, 2017-04-19 19:33:37]

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

[commit-bot: I haz the power, 2017-04-19 19:33:37]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[sky, 2017-04-19 20:10:04]

LGTM

[pavely, 2017-04-19 20:12:39]

components\sync\* lgtm

[stevenjb, 2017-04-19 20:51:31]

chromeos/ lgtm

[tzik, 2017-04-20 06:14:57]

lgtm

[Sami, 2017-04-20 10:13:23]

lgtm

https://codereview.chromium.org/2818533003/diff/80001/base/run_loop.h
File base/run_loop.h (right):

https://codereview.chromium.org/2818533003/diff/80001/base/run_loop.h#newcode90
base/run_loop.h:90: virtual void OnBeginNestedMessageLoop() = 0;
nit: TODO to rename this to OnBeginNestedRunLoop (or just do it now?)

[gab, 2017-04-20 14:17:28]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 14:17:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-04-20 14:19:09]

gab@chromium.org changed reviewers:
+ mpearson@chromium.org, nick@chromium.org
- ncarter@chromium.org

[gab, 2017-04-20 14:21:18]

Thanks, @skyostil: done.

Remaining owners:
>  - danakj@: core changes in //base
>  - nick@: content\*
>  - scheib@: device\bluetooth\*
>  - mek@: extensions\*
>  - mpearson@: components\omnibox\*

https://codereview.chromium.org/2818533003/diff/80001/base/run_loop.h
File base/run_loop.h (right):

https://codereview.chromium.org/2818533003/diff/80001/base/run_loop.h#newcode90
base/run_loop.h:90: virtual void OnBeginNestedMessageLoop() = 0;
On 2017/04/20 10:13:22, Sami wrote:
> nit: TODO to rename this to OnBeginNestedRunLoop (or just do it now?)

Ah yes, missed that one, definitely doing it now, thanks!

Done.

[commit-bot: I haz the power, 2017-04-20 14:43:43]

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

[commit-bot: I haz the power, 2017-04-20 14:43:44]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[danakj, 2017-04-20 14:57:32]

LGTM w/ a few comments

https://codereview.chromium.org/2818533003/diff/80001/base/message_loop/messa...
File base/message_loop/message_loop_test.cc (right):

https://codereview.chromium.org/2818533003/diff/80001/base/message_loop/messa...
base/message_loop/message_loop_test.cc:375: EXPECT_EQ(depth, 0);
Is there a test that posts a non-nestable task and verifies it doesn't run?

https://codereview.chromium.org/2818533003/diff/80001/base/run_loop.cc
File base/run_loop.cc (right):

https://codereview.chromium.org/2818533003/diff/80001/base/run_loop.cc#newcode44
base/run_loop.cc:44: std::stack<RunLoop*> active_run_loops;
stacks are deques by default which are terrible for wasting memory. the number
of things here is small, maybe make the stack backed by a vector?

https://codereview.chromium.org/2818533003/diff/80001/base/run_loop.cc#newcod...
base/run_loop.cc:169: std::stack<RunLoop*>& active_run_loops =
nit: auto?

https://codereview.chromium.org/2818533003/diff/80001/base/run_loop.cc#newcod...
base/run_loop.cc:200: RunLoop* underlying_run_loop =
nit: i liked previous better than underlying as a name

https://codereview.chromium.org/2818533003/diff/80001/base/run_loop.h
File base/run_loop.h (right):

https://codereview.chromium.org/2818533003/diff/80001/base/run_loop.h#newcode90
base/run_loop.h:90: virtual void OnBeginNestedMessageLoop() = 0;
On 2017/04/20 10:13:22, Sami wrote:
> nit: TODO to rename this to OnBeginNestedRunLoop (or just do it now?)

+1

https://codereview.chromium.org/2818533003/diff/80001/base/run_loop.h#newcode92
base/run_loop.h:92: protected:
why protected

https://codereview.chromium.org/2818533003/diff/80001/base/run_loop_unittest.cc
File base/run_loop_unittest.cc (right):

https://codereview.chromium.org/2818533003/diff/80001/base/run_loop_unittest....
base/run_loop_unittest.cc:121: Bind([]() {
EXPECT_TRUE(RunLoop::IsRunningOnCurrentThread()); }));
Is there a test for this being false?

https://codereview.chromium.org/2818533003/diff/80001/base/run_loop_unittest....
base/run_loop_unittest.cc:188: testing::Mock::VerifyAndClear(&nesting_observer);
I don't think you need this, it would change when the errors are printed but
mock doesn't allow calls before the EXPECT happens, it would report unexpected
calls. You can break things and verify if I'm right tho.

[gab, 2017-04-20 16:00:26]

Description was changed from

==========
Make nesting/running states a RunLoop rather than a MessageLoop concept.

First CL of a series to split inter-dependency between RunLoop and MessageLoop.

Also modernized bluetooth_socket_bluez_unittest.cc away from using deprecated
MessageLoop Quit
methods on its fixture's MessageLoop member to ease transition.

BUG=703346
==========

to

==========
Make nesting/running states a RunLoop rather than a MessageLoop concept.

First CL of a series to split inter-dependency between RunLoop and MessageLoop.

Also modernized bluetooth_socket_bluez_unittest.cc away from using deprecated
MessageLoop Quit
methods on its fixture's MessageLoop member to ease transition.

https://codereview.chromium.org/2828913003 follows to 
s/nested message loop/nested run loop/ in comments.

BUG=703346
==========

[gab, 2017-04-20 16:10:50]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 16:11:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-04-20 16:22:02]

Thanks Dana, done.

Remaining owners:
>  - nick@: content\*
>  - scheib@: device\bluetooth\*
>  - mek@: extensions\*
>  - mpearson@: components\omnibox\*

https://codereview.chromium.org/2818533003/diff/80001/base/message_loop/messa...
File base/message_loop/message_loop_test.cc (right):

https://codereview.chromium.org/2818533003/diff/80001/base/message_loop/messa...
base/message_loop/message_loop_test.cc:375: EXPECT_EQ(depth, 0);
On 2017/04/20 14:57:32, danakj wrote:
> Is there a test that posts a non-nestable task and verifies it doesn't run?

Yes: RunTest_NonNestableInNestedLoop and RunTest_NonNestableWithNoNesting

https://codereview.chromium.org/2818533003/diff/80001/base/run_loop.cc
File base/run_loop.cc (right):

https://codereview.chromium.org/2818533003/diff/80001/base/run_loop.cc#newcode44
base/run_loop.cc:44: std::stack<RunLoop*> active_run_loops;
On 2017/04/20 14:57:32, danakj wrote:
> stacks are deques by default which are terrible for wasting memory. the number
> of things here is small, maybe make the stack backed by a vector?

Good point, done.

https://codereview.chromium.org/2818533003/diff/80001/base/run_loop.cc#newcod...
base/run_loop.cc:169: std::stack<RunLoop*>& active_run_loops =
On 2017/04/20 14:57:32, danakj wrote:
> nit: auto?

Done.

https://codereview.chromium.org/2818533003/diff/80001/base/run_loop.cc#newcod...
base/run_loop.cc:200: RunLoop* underlying_run_loop =
On 2017/04/20 14:57:32, danakj wrote:
> nit: i liked previous better than underlying as a name

Done.

https://codereview.chromium.org/2818533003/diff/80001/base/run_loop.h
File base/run_loop.h (right):

https://codereview.chromium.org/2818533003/diff/80001/base/run_loop.h#newcode90
base/run_loop.h:90: virtual void OnBeginNestedMessageLoop() = 0;
On 2017/04/20 14:57:32, danakj wrote:
> On 2017/04/20 10:13:22, Sami wrote:
> > nit: TODO to rename this to OnBeginNestedRunLoop (or just do it now?)
> 
> +1

Done.

https://codereview.chromium.org/2818533003/diff/80001/base/run_loop.h#newcode92
base/run_loop.h:92: protected:
On 2017/04/20 14:57:32, danakj wrote:
> why protected

Copied as-is from MessageLoop::NestingObserver.

I assume this was done to make it clear that the owner of the full-type keeps
ownership (it's impossible for someone getting a plain NestingObserver* to
delete).

https://codereview.chromium.org/2818533003/diff/80001/base/run_loop_unittest.cc
File base/run_loop_unittest.cc (right):

https://codereview.chromium.org/2818533003/diff/80001/base/run_loop_unittest....
base/run_loop_unittest.cc:121: Bind([]() {
EXPECT_TRUE(RunLoop::IsRunningOnCurrentThread()); }));
On 2017/04/20 14:57:32, danakj wrote:
> Is there a test for this being false?

3 lines above on 118 in this test :)

https://codereview.chromium.org/2818533003/diff/80001/base/run_loop_unittest....
base/run_loop_unittest.cc:188: testing::Mock::VerifyAndClear(&nesting_observer);
On 2017/04/20 14:57:32, danakj wrote:
> I don't think you need this, it would change when the errors are printed but
> mock doesn't allow calls before the EXPECT happens, it would report unexpected
> calls. You can break things and verify if I'm right tho.

Ah right, removed, thanks

[gab, 2017-04-20 19:46:06]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 19:47:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-04-20 19:51:05]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 19:51:56]

Dry run: 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-04-20 20:33:59]

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

[commit-bot: I haz the power, 2017-04-20 20:34:00]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[gab, 2017-04-20 21:17:08]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 21:17:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-04-20 21:17:45]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 21:18:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-04-20 21:20:05]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 21:20:46]

Dry run: 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-04-20 21:26:03]

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

[commit-bot: I haz the power, 2017-04-20 21:26:06]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[scheib, 2017-04-21 06:17:10]

device/bluetooth LGTM, with an optional naming note:

https://codereview.chromium.org/2818533003/diff/220001/device/bluetooth/bluez...
File device/bluetooth/bluez/bluetooth_socket_bluez_unittest.cc (right):

https://codereview.chromium.org/2818533003/diff/220001/device/bluetooth/bluez...
device/bluetooth/bluez/bluetooth_socket_bluez_unittest.cc:108: void
AdapterCallback(base::OnceClosure on_callback,
I'd suggest 'continuation' instead of 'on_callback', but I won't hold up the
review for it and consistency (that I assume I've already missed the boat on) is
better.

[gab, 2017-04-21 13:59:33]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 13:59:52]

Dry run: 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-04-21 14:38:25]

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

[commit-bot: I haz the power, 2017-04-21 14:38:26]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Marijn Kruisselbrink, 2017-04-21 18:48:56]

On 2017/04/20 at 14:21:18, gab wrote:
> Remaining owners:
> >  - mek@: extensions\*

extensions/ lgtm, although I'm no longer an OWNER there

[gab, 2017-04-21 19:51:46]

gab@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org
- mek@chromium.org

[gab, 2017-04-21 19:52:14]

On 2017/04/21 18:48:56, Marijn Kruisselbrink wrote:
> On 2017/04/20 at 14:21:18, gab wrote:
> > Remaining owners:
> > >  - mek@: extensions\*
> 
> extensions/ lgtm, although I'm no longer an OWNER there

Oops looks like my checkout was behind this change... +Devlin in replacement :)

[Devlin, 2017-04-21 20:58:54]

extensions/browser/api/alarms/alarms_api_unittest.cc lgtm

[Mark P, 2017-04-21 22:02:58]

components/omnibox/browser/autocomplete_provider_unittest.cc lgtm

--mark

[ncarter (slow), 2017-04-21 22:04:32]

lgtm

[gab, 2017-04-25 17:58:32]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 17:58:56]

Dry run: 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-04-25 18:47:27]

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

[commit-bot: I haz the power, 2017-04-25 18:47:29]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[gab, 2017-04-25 20:14:55]

The CQ bit was checked by gab@chromium.org

[gab, 2017-04-25 20:14:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, sky@chromium.org, pavely@chromium.org, tzik@chromium.org,
skyostil@chromium.org, danakj@chromium.org, scheib@chromium.org,
mpearson@chromium.org, nick@chromium.org, rdevlin.cronin@chromium.org,
mek@chromium.org
Link to the patchset: https://codereview.chromium.org/2818533003/#ps260001
(title: "fix tests")

[gab, 2017-04-25 20:15:00]

Description was changed from

==========
Make nesting/running states a RunLoop rather than a MessageLoop concept.

First CL of a series to split inter-dependency between RunLoop and MessageLoop.

Also modernized bluetooth_socket_bluez_unittest.cc away from using deprecated
MessageLoop Quit
methods on its fixture's MessageLoop member to ease transition.

https://codereview.chromium.org/2828913003 follows to 
s/nested message loop/nested run loop/ in comments.

BUG=703346
==========

to

==========
Make nesting/running states a RunLoop rather than a MessageLoop concept.

First CL of a series to split inter-dependency between RunLoop and MessageLoop.

Also modernized bluetooth_socket_bluez_unittest.cc away from using deprecated
MessageLoop Quit
methods on its fixture's MessageLoop member to ease transition.

Lastly, tried to add thread safety checks for documentation purposes but turns
out there are already improper usage of the API... those will have to be
addressed first through issue 715235.

https://codereview.chromium.org/2828913003 follows to 
s/nested message loop/nested run loop/ in comments.

BUG=703346, 715235
==========

[gab, 2017-04-25 20:15:07]

The CQ bit was unchecked by gab@chromium.org

[gab, 2017-04-25 20:15:17]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2017-04-25 20:16:06]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-04-25 20:16:36]

@scheib: no objection, done, thanks!

https://codereview.chromium.org/2818533003/diff/220001/device/bluetooth/bluez...
File device/bluetooth/bluez/bluetooth_socket_bluez_unittest.cc (right):

https://codereview.chromium.org/2818533003/diff/220001/device/bluetooth/bluez...
device/bluetooth/bluez/bluetooth_socket_bluez_unittest.cc:108: void
AdapterCallback(base::OnceClosure on_callback,
On 2017/04/21 06:17:10, scheib wrote:
> I'd suggest 'continuation' instead of 'on_callback', but I won't hold up the
> review for it and consistency (that I assume I've already missed the boat on)
is
> better.

Done.

[commit-bot: I haz the power, 2017-04-25 21:28:09]

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

[commit-bot: I haz the power, 2017-04-25 21:28:12]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[gab, 2017-04-26 18:16:02]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 18:16:40]

Dry run: 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-04-26 19:24:54]

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

[commit-bot: I haz the power, 2017-04-26 19:24:55]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[gab, 2017-05-01 20:59:11]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 21:00:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-05-01 21:01:42]

@skyostil PTanL (PS 15): had to change approach for
lazy_scheduler_message_loop_delegate_for_tests.cc because of mass test failures,
see comment in there for details! (also posted to http://crbug.com/495659)

Thanks!

[gab, 2017-05-01 21:07:00]

The CQ bit was unchecked by gab@chromium.org

[gab, 2017-05-01 21:07:20]

The CQ bit was checked by gab@chromium.org

[gab, 2017-05-01 21:07:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, sky@chromium.org, pavely@chromium.org, tzik@chromium.org,
mpearson@chromium.org, nick@chromium.org, danakj@chromium.org,
scheib@chromium.org, rdevlin.cronin@chromium.org, mek@chromium.org,
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2818533003/#ps300001
(title: "Fix nesting observer issues with
LazySchedulerMessageLoopDelegateForTests")

[commit-bot: I haz the power, 2017-05-01 21:08:27]

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-05-01 23:47:58]

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

[commit-bot: I haz the power, 2017-05-01 23:48:01]

Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sami, 2017-05-04 15:15:25]

Thanks, still lgtm. I thought about making DoWork support nesting that happens
outside of it, but it seems like a shame to do it if we basically only need it
for tests. Hopefully we can instead make tests which bring up the Blink
scheduler do so on a more realistic way.

https://codereview.chromium.org/2818533003/diff/300001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/test/lazy_scheduler_message_loop_delegate_for_tests.cc
(right):

https://codereview.chromium.org/2818533003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/test/lazy_scheduler_message_loop_delegate_for_tests.cc:123:
// TODO(skyostil): fix this convulotion as part of http://crbug.com/495659.
typo: convolution :)

[gab, 2017-05-04 22:05:14]

The CQ bit was checked by gab@chromium.org

[gab, 2017-05-04 22:05:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, sky@chromium.org, pavely@chromium.org, tzik@chromium.org,
mpearson@chromium.org, nick@chromium.org, danakj@chromium.org,
scheib@chromium.org, rdevlin.cronin@chromium.org, mek@chromium.org,
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2818533003/#ps340001
(title: "need to RunUntilIdle for ConnectProfile with DoNothing callbacks -- how
did this possibly not hang …")

[commit-bot: I haz the power, 2017-05-04 22:06:00]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-05-04 22:12:12]

@scheib: FYI, had to make a tweak to the reviewed device\bluetooth test code

https://codereview.chromium.org/2818533003/diff/340001/device/bluetooth/bluez...
File device/bluetooth/bluez/bluetooth_socket_bluez_unittest.cc (left):

https://codereview.chromium.org/2818533003/diff/340001/device/bluetooth/bluez...
device/bluetooth/bluez/bluetooth_socket_bluez_unittest.cc:388:
base::RunLoop().Run();
@scheib: I don't understand how this possibly worked before since the above
call's callback are "DoNothings" and can't possibly quit this loop... and the
RunLoop starts with only that task per the preceding
base::RunLoop().RunUntilIdle(); ...

Anyways... with the new code doing a matching run_loop.Run() on 420 in the RHS
of this diff hangs as I'd expect.

Perhaps some other quit was somehow filtering to this before but now
RunUntilIdle() is clearly the right thing IMO.

[gab, 2017-05-04 22:17:15]

The CQ bit was checked by gab@chromium.org

[gab, 2017-05-04 22:17:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, sky@chromium.org, pavely@chromium.org, tzik@chromium.org,
mpearson@chromium.org, nick@chromium.org, danakj@chromium.org,
scheib@chromium.org, rdevlin.cronin@chromium.org, mek@chromium.org,
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2818533003/#ps360001
(title: "fix compile")

[commit-bot: I haz the power, 2017-05-04 22:17:57]

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-05-04 23:34:56]

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

[commit-bot: I haz the power, 2017-05-04 23:34:59]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gab, 2017-05-05 02:18:08]

The CQ bit was checked by gab@chromium.org

[gab, 2017-05-05 02:18:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, sky@chromium.org, pavely@chromium.org, tzik@chromium.org,
mpearson@chromium.org, nick@chromium.org, danakj@chromium.org,
scheib@chromium.org, rdevlin.cronin@chromium.org, mek@chromium.org,
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2818533003/#ps380001
(title: "ah ah, need to link ConnectProfile call and their matching Accept calls
in same RunLoop!")

[commit-bot: I haz the power, 2017-05-05 02:18:58]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-05-05 02:21:54]

https://codereview.chromium.org/2818533003/diff/340001/device/bluetooth/bluez...
File device/bluetooth/bluez/bluetooth_socket_bluez_unittest.cc (left):

https://codereview.chromium.org/2818533003/diff/340001/device/bluetooth/bluez...
device/bluetooth/bluez/bluetooth_socket_bluez_unittest.cc:388:
base::RunLoop().Run();
On 2017/05/04 22:12:12, gab wrote:
> @scheib: I don't understand how this possibly worked before since the above
> call's callback are "DoNothings" and can't possibly quit this loop... and the
> RunLoop starts with only that task per the preceding
> base::RunLoop().RunUntilIdle(); ...
> 
> Anyways... with the new code doing a matching run_loop.Run() on 420 in the RHS
> of this diff hangs as I'd expect.
> 
> Perhaps some other quit was somehow filtering to this before but now
> RunUntilIdle() is clearly the right thing IMO.

Ah ah, figured it out, with this PS the expectations on 423-425 were failing.
The issue isn't that the loop isn't quit, the issue is that it's quit through
the closure passed in the server_socket->Accept() call above -- as such that
call and the ConnectProfile() call here need to share the same RunLoop!

[gab, 2017-05-05 02:22:23]

Patchset #19 (id:380001) has been deleted

[gab, 2017-05-05 02:23:26]

The CQ bit was checked by gab@chromium.org

[gab, 2017-05-05 02:23:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, sky@chromium.org, pavely@chromium.org, tzik@chromium.org,
mpearson@chromium.org, nick@chromium.org, danakj@chromium.org,
scheib@chromium.org, rdevlin.cronin@chromium.org, mek@chromium.org,
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2818533003/#ps420001
(title: "ah ah, need to link ConnectProfile call and their matching Accept calls
in same RunLoop!")

[commit-bot: I haz the power, 2017-05-05 02:24:00]

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-05-05 03:31:30]

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

[commit-bot: I haz the power, 2017-05-05 03:31:32]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gab, 2017-05-05 03:50:10]

The CQ bit was checked by gab@chromium.org

[gab, 2017-05-05 03:50:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, sky@chromium.org, pavely@chromium.org, tzik@chromium.org,
mpearson@chromium.org, nick@chromium.org, danakj@chromium.org,
scheib@chromium.org, rdevlin.cronin@chromium.org, mek@chromium.org,
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2818533003/#ps440001
(title: "can't run same RunLoop twice...")

[commit-bot: I haz the power, 2017-05-05 03:50:26]

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-05-05 06:22:26]

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

[commit-bot: I haz the power, 2017-05-05 06:22:29]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[gab, 2017-05-05 11:49:50]

The CQ bit was checked by gab@chromium.org

[gab, 2017-05-05 11:49:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, sky@chromium.org, pavely@chromium.org, tzik@chromium.org,
mpearson@chromium.org, nick@chromium.org, danakj@chromium.org,
scheib@chromium.org, rdevlin.cronin@chromium.org, mek@chromium.org,
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2818533003/#ps460001
(title: "still need to check MessageLoop::current() in Mojo's
RunLoopNestingObserver::GetForThread()")

[commit-bot: I haz the power, 2017-05-05 11:50:09]

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-05-05 13:39:00]

CQ is committing da patch.

Bot data: {"patchset_id": 460001, "attempt_start_ts": 1493984990308740,
"parent_rev": "403845af284556f00645e2790618722fb99f9d34", "commit_rev":
"7af9dc076d2349ade19a95e24f6ee6811e939765"}

[commit-bot: I haz the power, 2017-05-05 13:39:13]

Description was changed from

==========
Make nesting/running states a RunLoop rather than a MessageLoop concept.

First CL of a series to split inter-dependency between RunLoop and MessageLoop.

Also modernized bluetooth_socket_bluez_unittest.cc away from using deprecated
MessageLoop Quit
methods on its fixture's MessageLoop member to ease transition.

Lastly, tried to add thread safety checks for documentation purposes but turns
out there are already improper usage of the API... those will have to be
addressed first through issue 715235.

https://codereview.chromium.org/2828913003 follows to 
s/nested message loop/nested run loop/ in comments.

BUG=703346, 715235
==========

to

==========
Make nesting/running states a RunLoop rather than a MessageLoop concept.

First CL of a series to split inter-dependency between RunLoop and MessageLoop.

Also modernized bluetooth_socket_bluez_unittest.cc away from using deprecated
MessageLoop Quit
methods on its fixture's MessageLoop member to ease transition.

Lastly, tried to add thread safety checks for documentation purposes but turns
out there are already improper usage of the API... those will have to be
addressed first through issue 715235.

https://codereview.chromium.org/2828913003 follows to 
s/nested message loop/nested run loop/ in comments.

BUG=703346, 715235

Review-Url: https://codereview.chromium.org/2818533003
Cr-Commit-Position: refs/heads/master@{#469636}
Committed:
https://chromium.googlesource.com/chromium/src/+/7af9dc076d2349ade19a95e24f6e...
==========

[commit-bot: I haz the power, 2017-05-05 13:39:15]

Committed patchset #22 (id:460001) as
https://chromium.googlesource.com/chromium/src/+/7af9dc076d2349ade19a95e24f6e...

[Will Harris, 2017-05-05 18:39:26]

this appears to have caused a regression on official builder win base_unittests 

https://uberchromegw.corp.google.com/i/official.desktop.continuous/builders/w...

RunLoopTest.DisallowWaitingDeathTest

[mfomitchev, 2017-05-05 18:41:42]

A revert of this CL (patchset #22 id:460001) has been created in
https://codereview.chromium.org/2867533002/ by mfomitchev@chromium.org.

The reason for reverting is: Broke RunLoopTest.DisallowWaitingDeathTest on
Windows. Link to first failure:
https://uberchromegw.corp.google.com/i/official.desktop.continuous/builders/w....

[gab, 2017-05-05 19:18:18]

On 2017/05/05 18:41:42, mfomitchev wrote:
> A revert of this CL (patchset #22 id:460001) has been created in
> https://codereview.chromium.org/2867533002/ by
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=mfomitchev@chromium.org.
> 
> The reason for reverting is: Broke RunLoopTest.DisallowWaitingDeathTest on
> Windows. Link to first failure:
>
https://uberchromegw.corp.google.com/i/official.desktop.continuous/builders/w....

Revert won't apply, fix incoming @ https://codereview.chromium.org/2857393006/