Convert more test helpers to base::RunLoop, fix page title checks.

Convert more test helpers to base::RunLoop, fix page title checks.

This CL removes the "deferred quit" from most of the touched helpers.

The exclusions are UrlCommitObserver and TestFrameNavigationObserver.
They were already using immediate quit mode, so there is no change in
their behavior, just cleanup.

Regarding title checks in browser_browsertest.cc: in these tests there
is no guarantee on when the new tab will start and stop loading. It
can start loading after WindowedNotificationObserver::Wait returns; also
it can stop loading while we're still inside it. We can explicitly wait
for the title to handle all these cases, and this makes the call to
WaitForLoadStop unnecessary.


BUG=668707
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2601843002
Cr-Commit-Position: refs/heads/master@{#443395}
Committed: https://chromium.googlesource.com/chromium/src/+/e7469132628fa7bd530d68c134e4159d2cfc1804

[Alexander Semashko, 2016-12-27 21:47:29]

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

[commit-bot: I haz the power, 2016-12-27 21:47:45]

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

[Alexander Semashko, 2016-12-27 22:01:47]

Description was changed from

==========
Convert more test helpers to base::RunLoop.

Fix wrong checks of a page's title. The title can arrive an arbitrary
time after load stops, so it needs to be specifically waited for.

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

to

==========
Convert more test helpers to base::RunLoop, fix page title checks.

This CL removes the "deferred quit" from most of the touched helpers.

The exclusions are UrlCommitObserver and TestFrameNavigationObserver.
They were already using immediate quit mode, so there is no change in
their behavior, just cleanup.

Regarding title checks: the title can arrive an arbitrary time after
load stops, so it needs to be specifically waited for. I did not (yet)
examine all uses of WebContents::GetTitle in tests, maybe it would be
good to do.


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

[Alexander Semashko, 2016-12-27 22:01:48]

ahest@yandex-team.ru changed reviewers:
+ nasko@chromium.org

[commit-bot: I haz the power, 2016-12-27 22:08:45]

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

[commit-bot: I haz the power, 2016-12-27 22:08:46]

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, 2016-12-29 10:10:06]

Description was changed from

==========
Convert more test helpers to base::RunLoop, fix page title checks.

This CL removes the "deferred quit" from most of the touched helpers.

The exclusions are UrlCommitObserver and TestFrameNavigationObserver.
They were already using immediate quit mode, so there is no change in
their behavior, just cleanup.

Regarding title checks: the title can arrive an arbitrary time after
load stops, so it needs to be specifically waited for. I did not (yet)
examine all uses of WebContents::GetTitle in tests, maybe it would be
good to do.


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

to

==========
Convert more test helpers to base::RunLoop, fix page title checks.

This CL removes the "deferred quit" from most of the touched helpers.

The exclusions are UrlCommitObserver and TestFrameNavigationObserver.
They were already using immediate quit mode, so there is no change in
their behavior, just cleanup.

Regarding title checks: the title can arrive an arbitrary time after
load stops, so it needs to be specifically waited for. I did not (yet)
examine all uses of WebContents::GetTitle in tests, maybe it would be
good to do.


BUG=668707
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Alexander Semashko, 2016-12-29 10:14:13]

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

[commit-bot: I haz the power, 2016-12-29 10:14:19]

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, 2016-12-29 10:59:44]

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

[commit-bot: I haz the power, 2016-12-29 10:59:45]

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...)

[Alexander Semashko, 2017-01-02 18:34:21]

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

[commit-bot: I haz the power, 2017-01-02 18:34:28]

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-01-02 19:15:50]

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

[commit-bot: I haz the power, 2017-01-02 19:15:51]

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...)

[nasko, 2017-01-04 00:54:46]

LGTM

[Alexander Semashko, 2017-01-11 14:33:04]

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

[commit-bot: I haz the power, 2017-01-11 14:33:26]

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-01-11 15:47:01]

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

[commit-bot: I haz the power, 2017-01-11 15:47:01]

Dry run: This issue passed the CQ dry run.

[Alexander Semashko, 2017-01-12 15:31:16]

Description was changed from

==========
Convert more test helpers to base::RunLoop, fix page title checks.

This CL removes the "deferred quit" from most of the touched helpers.

The exclusions are UrlCommitObserver and TestFrameNavigationObserver.
They were already using immediate quit mode, so there is no change in
their behavior, just cleanup.

Regarding title checks: the title can arrive an arbitrary time after
load stops, so it needs to be specifically waited for. I did not (yet)
examine all uses of WebContents::GetTitle in tests, maybe it would be
good to do.


BUG=668707
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Convert more test helpers to base::RunLoop, fix page title checks.

This CL removes the "deferred quit" from most of the touched helpers.

The exclusions are UrlCommitObserver and TestFrameNavigationObserver.
They were already using immediate quit mode, so there is no change in
their behavior, just cleanup.

Regarding title checks: in these tests there is no guarantee on when the
new tab will start and stop loading. It it can start loading after
WindowedNotificationObserver::Wait returns; also it can stop loading
while we're still inside it. We can explicitly wait for the title to
handle all these cases, and this makes the call to WaitForLoadStop
unnecessary.

I did not (yet) examine other uses of WebContents::GetTitle in tests,
maybe it would be good to do.

BUG=668707
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Alexander Semashko, 2017-01-12 15:34:48]

Description was changed from

==========
Convert more test helpers to base::RunLoop, fix page title checks.

This CL removes the "deferred quit" from most of the touched helpers.

The exclusions are UrlCommitObserver and TestFrameNavigationObserver.
They were already using immediate quit mode, so there is no change in
their behavior, just cleanup.

Regarding title checks: in these tests there is no guarantee on when the
new tab will start and stop loading. It it can start loading after
WindowedNotificationObserver::Wait returns; also it can stop loading
while we're still inside it. We can explicitly wait for the title to
handle all these cases, and this makes the call to WaitForLoadStop
unnecessary.

I did not (yet) examine other uses of WebContents::GetTitle in tests,
maybe it would be good to do.

BUG=668707
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Convert more test helpers to base::RunLoop, fix page title checks.

This CL removes the "deferred quit" from most of the touched helpers.

The exclusions are UrlCommitObserver and TestFrameNavigationObserver.
They were already using immediate quit mode, so there is no change in
their behavior, just cleanup.

Regarding title checks in browser_browsertest.cc: in these tests there
is no guarantee on when the new tab will start and stop loading. It it
can start loading after WindowedNotificationObserver::Wait returns; also
it can stop loading while we're still inside it. We can explicitly wait
for the title to handle all these cases, and this makes the call to
WaitForLoadStop unnecessary.


BUG=668707
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Alexander Semashko, 2017-01-12 15:35:31]

ahest@yandex-team.ru changed reviewers:
+ pkasting@chromium.org

[Alexander Semashko, 2017-01-12 15:36:30]

Peter, can you take a look at chrome/browser/ui/browser_browsertest.cc please?

[Peter Kasting, 2017-01-12 18:16:17]

LGTM

https://codereview.chromium.org/2601843002/diff/80001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/2601843002/diff/80001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:2446: }
Nit: The preceding lines could optionally be written more briefly as:

    bool fg_tab = disposition == WindowOpenDisposition::NEW_FOREGROUND_TAB;
    ASSERT_TRUE(
        fg_tab || disposition == WindowOpenDisposition::NEW_BACKGROUND_TAB);
    auto expected_title = fg_tab ? GetFirstPageTitle() : GetSecondPageTitle();

[Alexander Semashko, 2017-01-12 20:48:19]

https://codereview.chromium.org/2601843002/diff/80001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/2601843002/diff/80001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:2446: }
On 2017/01/12 18:16:17, Peter Kasting wrote:
> Nit: The preceding lines could optionally be written more briefly as:
> 
>     bool fg_tab = disposition == WindowOpenDisposition::NEW_FOREGROUND_TAB;
>     ASSERT_TRUE(
>         fg_tab || disposition == WindowOpenDisposition::NEW_BACKGROUND_TAB);
>     auto expected_title = fg_tab ? GetFirstPageTitle() : GetSecondPageTitle();

To me the current code is much more clear, I'd leave it as it is.

[Alexander Semashko, 2017-01-12 20:48:25]

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

[Alexander Semashko, 2017-01-12 20:48:25]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2601843002/#ps80001
(title: "Remove unneeded call.")

[commit-bot: I haz the power, 2017-01-12 20:49:04]

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

[Alexander Semashko, 2017-01-12 20:58:28]

Description was changed from

==========
Convert more test helpers to base::RunLoop, fix page title checks.

This CL removes the "deferred quit" from most of the touched helpers.

The exclusions are UrlCommitObserver and TestFrameNavigationObserver.
They were already using immediate quit mode, so there is no change in
their behavior, just cleanup.

Regarding title checks in browser_browsertest.cc: in these tests there
is no guarantee on when the new tab will start and stop loading. It it
can start loading after WindowedNotificationObserver::Wait returns; also
it can stop loading while we're still inside it. We can explicitly wait
for the title to handle all these cases, and this makes the call to
WaitForLoadStop unnecessary.


BUG=668707
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Convert more test helpers to base::RunLoop, fix page title checks.

This CL removes the "deferred quit" from most of the touched helpers.

The exclusions are UrlCommitObserver and TestFrameNavigationObserver.
They were already using immediate quit mode, so there is no change in
their behavior, just cleanup.

Regarding title checks in browser_browsertest.cc: in these tests there
is no guarantee on when the new tab will start and stop loading. It
can start loading after WindowedNotificationObserver::Wait returns; also
it can stop loading while we're still inside it. We can explicitly wait
for the title to handle all these cases, and this makes the call to
WaitForLoadStop unnecessary.


BUG=668707
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-01-12 23:56:36]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484254105625570,
"parent_rev": "da097d30d0e99243c59c75d336a761b0ec5e8def", "commit_rev":
"e7469132628fa7bd530d68c134e4159d2cfc1804"}

[commit-bot: I haz the power, 2017-01-12 23:57:13]

Description was changed from

==========
Convert more test helpers to base::RunLoop, fix page title checks.

This CL removes the "deferred quit" from most of the touched helpers.

The exclusions are UrlCommitObserver and TestFrameNavigationObserver.
They were already using immediate quit mode, so there is no change in
their behavior, just cleanup.

Regarding title checks in browser_browsertest.cc: in these tests there
is no guarantee on when the new tab will start and stop loading. It
can start loading after WindowedNotificationObserver::Wait returns; also
it can stop loading while we're still inside it. We can explicitly wait
for the title to handle all these cases, and this makes the call to
WaitForLoadStop unnecessary.


BUG=668707
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Convert more test helpers to base::RunLoop, fix page title checks.

This CL removes the "deferred quit" from most of the touched helpers.

The exclusions are UrlCommitObserver and TestFrameNavigationObserver.
They were already using immediate quit mode, so there is no change in
their behavior, just cleanup.

Regarding title checks in browser_browsertest.cc: in these tests there
is no guarantee on when the new tab will start and stop loading. It
can start loading after WindowedNotificationObserver::Wait returns; also
it can stop loading while we're still inside it. We can explicitly wait
for the title to handle all these cases, and this makes the call to
WaitForLoadStop unnecessary.


BUG=668707
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2601843002
Cr-Commit-Position: refs/heads/master@{#443395}
Committed:
https://chromium.googlesource.com/chromium/src/+/e7469132628fa7bd530d68c134e4...
==========

[commit-bot: I haz the power, 2017-01-12 23:57:14]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/e7469132628fa7bd530d68c134e4...

[xlai (Olivia), 2017-01-13 21:16:05]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2630683003/ by xlai@chromium.org.

The reason for reverting is: ClickModifierTest.WindowOpenBasicClickTest and
ClickModifierTest.WindowOpenControlShiftClickTest become very flaky recently.
Example builds are all
recorded at crbug.com/681035.

By looking at the error log file
(https://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20%2...),
I notice that
the error comes from src\chrome\browser\ui\browser_browsertest.cc(2451) which is
recently modified by this CL. Therefore the revert.
.

[gab, 2017-01-13 21:20:28]

On 2017/01/13 21:16:05, xlai (Olivia) wrote:
> A revert of this CL (patchset #5 id:80001) has been created in
> https://codereview.chromium.org/2630683003/ by
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=xlai@chromium.org.
> 
> The reason for reverting is: ClickModifierTest.WindowOpenBasicClickTest and
> ClickModifierTest.WindowOpenControlShiftClickTest become very flaky recently.
> Example builds are all
> recorded at crbug.com/681035.
> 
> By looking at the error log file
>
(https://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20%2...),
> I notice that
> the error comes from src\chrome\browser\ui\browser_browsertest.cc(2451) which
is
> recently modified by this CL. Therefore the revert.
> .

Agreed, was going to revert too, example failure:

ClickModifierTest.WindowOpenControlShiftClickTest (run #1):
[ RUN      ] ClickModifierTest.WindowOpenControlShiftClickTest
Xlib:  extension "RANDR" missing on display ":99".
Xlib:  extension "RANDR" missing on display ":99".
[8570:8570:0113/112014.368268:ERROR:variations_util.cc(102)] Missing Worker Pool
Configuration: Background
[8570:8570:0113/112014.417654:WARNING:audio_manager.cc(317)] Multiple instances
of AudioManager detected
[8570:8570:0113/112014.417772:WARNING:audio_manager.cc(278)] Multiple instances
of AudioManager detected
[8570:8570:0113/112014.840752:WARNING:password_store_factory.cc(247)] Using
basic (unencrypted) store for password storage. See
https://chromium.googlesource.com/chromium/src/+/master/docs/linux_password_s...
for more information about password storage options.
[8696:8696:0113/112015.175671:ERROR:variations_util.cc(102)] Missing Worker Pool
Configuration: RendererBackground
../../chrome/browser/ui/browser_browsertest.cc:2451: Failure
Value of: content::TitleWatcher(web_contents).WaitAndGetTitle()
  Actual: data:text/html,<title>New window!</title>this is the new window
Expected: expected_title
Which is: New window!