Switch WindowedNotificationObserver to use base::RunLoop.

Switch WindowedNotificationObserver to use base::RunLoop.

Now it will quit the message loop immediately after receiving the
notification. Also it does not allow nested tasks anymore.

BUG=668707
Review-Url: https://codereview.chromium.org/2701473007
Cr-Commit-Position: refs/heads/master@{#452739}
Committed: https://chromium.googlesource.com/chromium/src/+/474e1f148ab83dde92b93a93e53cf83d77b9be13

[Alexander Semashko, 2017-02-16 17:02:50]

The CQ bit was checked by ahest@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 17:03:37]

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-02-16 17:55:59]

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

[commit-bot: I haz the power, 2017-02-16 17:56:00]

Dry run: 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_...)

[Alexander Semashko, 2017-02-20 14:33:33]

The CQ bit was checked by ahest@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-02-20 14:33:44]

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-02-20 15:19:36]

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

[commit-bot: I haz the power, 2017-02-20 15:19:37]

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

[Alexander Semashko, 2017-02-20 16:37:43]

The CQ bit was checked by ahest@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-02-20 16:38:01]

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-02-20 18:26:24]

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

[commit-bot: I haz the power, 2017-02-20 18:26:25]

Dry run: This issue passed the CQ dry run.

[Alexander Semashko, 2017-02-21 21:33:07]

Description was changed from

==========
Switch WindowedNotificationObserver to use base::RunLoop.

BUG=668707
==========

to

==========
Switch WindowedNotificationObserver to use base::RunLoop.

Now it will quit the message loop immediately after receiving the
notification. Also it does not allow nested tasks anymore.

BUG=668707
==========

[Alexander Semashko, 2017-02-21 21:33:08]

ahest@yandex-team.ru changed reviewers:
+ jochen@chromium.org, rdevlin.cronin@chromium.org

[Alexander Semashko, 2017-02-21 21:33:47]

PTAL:

jochen - chrome/ and content/

rdevlin.cronin - extensions/

https://codereview.chromium.org/2701473007/diff/40001/chrome/browser/ui/login...
File chrome/browser/ui/login/login_handler_browsertest.cc (left):

https://codereview.chromium.org/2701473007/diff/40001/chrome/browser/ui/login...
chrome/browser/ui/login/login_handler_browsertest.cc:465:
IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest, MultipleRealmCancellation) {
For easer review: the functional changes for these tests are in patch-set 1. The
main thing is that I added waiting for all LoginHandlers to be created. Also I
removed the implicit dependency on their creation order. In PS 2 and 3 I only
extracted common part of these two almost identical tests.

https://codereview.chromium.org/2701473007/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/net_internals/net_internals_ui_browsertest.cc
(right):

https://codereview.chromium.org/2701473007/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/net_internals/net_internals_ui_browsertest.cc:214:
base::MessageLoop::ScopedNestableTaskAllower nestable_task_allower(
Some of these tests relied on the presence of ScopedNestableTaskAllower when
using the WindowedNotificationObserver. I changed that to be explicit, because
IMO allowing task nesting everywhere is not a good thing.

https://codereview.chromium.org/2701473007/diff/40001/extensions/test/extensi...
File extensions/test/extension_test_notification_observer.cc (right):

https://codereview.chromium.org/2701473007/diff/40001/extensions/test/extensi...
extensions/test/extension_test_notification_observer.cc:143: // TODO(...): Find
out why tests fail without it.
Please advise what to do here, e.g. assign this TODO to somebody or create a
bug.
As to what is going on here: some tests started failing after the removal of
implicit deferred quit in the WindowedNotificationObserver. This means that
waiting for the notification itself is not sufficient. What is needed here is to
find out what is missing, and (probably) add code to wait for that event(s). Or
just leave RunAllPending if it turns out that these events do not involve other
threads/processes and happen without any delay.

[Devlin, 2017-02-22 00:26:48]

https://codereview.chromium.org/2701473007/diff/40001/extensions/test/extensi...
File extensions/test/extension_test_notification_observer.cc (right):

https://codereview.chromium.org/2701473007/diff/40001/extensions/test/extensi...
extensions/test/extension_test_notification_observer.cc:143: // TODO(...): Find
out why tests fail without it.
On 2017/02/21 21:33:47, Alexander Semashko wrote:
> Please advise what to do here, e.g. assign this TODO to somebody or create a
> bug.
> As to what is going on here: some tests started failing after the removal of
> implicit deferred quit in the WindowedNotificationObserver. This means that
> waiting for the notification itself is not sufficient. What is needed here is
to
> find out what is missing, and (probably) add code to wait for that event(s).
Or
> just leave RunAllPending if it turns out that these events do not involve
other
> threads/processes and happen without any delay.

Filing a bug would probably be best here, ideally with a list of the tests that
are affected.

[jochen (gone - plz use gerrit), 2017-02-22 09:01:13]

lgtm

[Alexander Semashko, 2017-02-22 17:09:52]

The CQ bit was checked by ahest@yandex-team.ru to run a CQ dry run

[Alexander Semashko, 2017-02-22 17:10:08]

https://codereview.chromium.org/2701473007/diff/40001/extensions/test/extensi...
File extensions/test/extension_test_notification_observer.cc (right):

https://codereview.chromium.org/2701473007/diff/40001/extensions/test/extensi...
extensions/test/extension_test_notification_observer.cc:143: // TODO(...): Find
out why tests fail without it.
On 2017/02/22 00:26:48, Devlin wrote:
> On 2017/02/21 21:33:47, Alexander Semashko wrote:
> > Please advise what to do here, e.g. assign this TODO to somebody or create a
> > bug.
> > As to what is going on here: some tests started failing after the removal of
> > implicit deferred quit in the WindowedNotificationObserver. This means that
> > waiting for the notification itself is not sufficient. What is needed here
is
> to
> > find out what is missing, and (probably) add code to wait for that event(s).
> Or
> > just leave RunAllPending if it turns out that these events do not involve
> other
> > threads/processes and happen without any delay.
> 
> Filing a bug would probably be best here, ideally with a list of the tests
that
> are affected.

Done, https://bugs.chromium.org/p/chromium/issues/detail?id=695073

[commit-bot: I haz the power, 2017-02-22 17:10:27]

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-02-22 19:18:29]

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

[commit-bot: I haz the power, 2017-02-22 19:18:31]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-02-23 23:02:57]

extensions lgtm

[Alexander Semashko, 2017-02-24 00:34:55]

The CQ bit was checked by ahest@yandex-team.ru

[Alexander Semashko, 2017-02-24 00:34:55]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2701473007/#ps60001
(title: "Mention the bug; remove deferred quit in WaitForCondition.")

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

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-02-24 03:24:55]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487896495224270,
"parent_rev": "20e8838145837ef381ebe3116c4af6bb1734b116", "commit_rev":
"474e1f148ab83dde92b93a93e53cf83d77b9be13"}

[commit-bot: I haz the power, 2017-02-24 03:25:26]

Description was changed from

==========
Switch WindowedNotificationObserver to use base::RunLoop.

Now it will quit the message loop immediately after receiving the
notification. Also it does not allow nested tasks anymore.

BUG=668707
==========

to

==========
Switch WindowedNotificationObserver to use base::RunLoop.

Now it will quit the message loop immediately after receiving the
notification. Also it does not allow nested tasks anymore.

BUG=668707

Review-Url: https://codereview.chromium.org/2701473007
Cr-Commit-Position: refs/heads/master@{#452739}
Committed:
https://chromium.googlesource.com/chromium/src/+/474e1f148ab83dde92b93a93e53c...
==========

[commit-bot: I haz the power, 2017-02-24 03:25:28]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/474e1f148ab83dde92b93a93e53c...

[Marc Treib, 2017-02-24 12:48:07]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2715733003/ by treib@chromium.org.

The reason for reverting is: Suspected of causing flakiness in
PrintPreviewDialogControllerBrowserTest.ReloadInitiatorTab

More details in crbug.com/695835.