Support base::test::ScopedTaskEnvironment and content::TestBrowserThreadBundle in the same scope.

Support base::test::ScopedTaskEnvironment and content::TestBrowserThreadBundle in the same scope.

A test suite that depends on a test suite base that provides a
base::test::ScopedTaskEnvironment may need browser threads. Currently,
the test suite cannot use content::TestBrowserThreadBundle to get
these browser threads because base::test::ScopedTaskEnvironment and
content::TestBrowserThreadBundle both set the main MessageLoop and
the TaskScheduler. With this CL, content::TestBrowserThreadBundle
doesn't set the main MessageLoop and the TaskScheduler if they
already exist.

E.g.
Test suite: ToolbarActionViewUnitTest
            Needs BrowserThreads
Base test suite: views::ViewsTestBase
                 Provides a ScopedTaskEnvironment

BUG=

Review-Url: https://codereview.chromium.org/2860533002
Cr-Original-Commit-Position: refs/heads/master@{#469304}
Committed: https://chromium.googlesource.com/chromium/src/+/ba0dacfc73d208fc779907890bc94dbd09ef1aad
Review-Url: https://codereview.chromium.org/2860533002
Cr-Commit-Position: refs/heads/master@{#470021}
Committed: https://chromium.googlesource.com/chromium/src/+/8c5805290290f2e7749c27ee7ea005a980398947

[fdoray, 2017-05-02 19:53:50]

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

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

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-05-02 20:27:29]

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

[commit-bot: I haz the power, 2017-05-02 20:27:30]

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

[fdoray, 2017-05-02 20:51:56]

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

[fdoray, 2017-05-02 20:52:18]

fdoray@chromium.org changed reviewers:
+ gab@chromium.org, robliao@chromium.org

[fdoray, 2017-05-02 20:52:18]

fdoray@chromium.org changed reviewers:
+ gab@chromium.org, robliao@chromium.org

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

Dry run: CQ is trying da patch.

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

[fdoray, 2017-05-02 20:52:26]

PTAL

[commit-bot: I haz the power, 2017-05-02 21:29:10]

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

[commit-bot: I haz the power, 2017-05-02 21:29:11]

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

[fdoray, 2017-05-03 12:49:17]

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

[commit-bot: I haz the power, 2017-05-03 12:49:29]

Dry run: CQ is trying da patch.

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

[fdoray, 2017-05-03 12:58:58]

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

[commit-bot: I haz the power, 2017-05-03 12:59:03]

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

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

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

Dry run: This issue passed the CQ dry run.

[gab, 2017-05-03 15:33:11]

Also document this possibility in test_browser_thread_bundle.h's meta-comment

https://codereview.chromium.org/2860533002/diff/70001/content/public/test/tes...
File content/public/test/test_browser_thread_bundle.cc (left):

https://codereview.chromium.org/2860533002/diff/70001/content/public/test/tes...
content/public/test/test_browser_thread_bundle.cc:66:
CHECK(message_loop_->IsIdleForTesting());
Why move (this was checking above comment) and change from CHECK (since this is
test-only code CHECK makes sure this asserts even in non-dcheck builds)?

https://codereview.chromium.org/2860533002/diff/70001/content/public/test/tes...
content/public/test/test_browser_thread_bundle.cc:72: CHECK(!(options_ &
IO_MAINLOOP) || !(options_ & REAL_IO_THREAD));
ditto

https://codereview.chromium.org/2860533002/diff/70001/content/public/test/tes...
File content/public/test/test_browser_thread_bundle.cc (right):

https://codereview.chromium.org/2860533002/diff/70001/content/public/test/tes...
content/public/test/test_browser_thread_bundle.cc:79: //
BrowserMainLoop::MainMessageLoopStart().
Update comment to mention why it could exist already in tests.

https://codereview.chromium.org/2860533002/diff/70001/content/public/test/tes...
content/public/test/test_browser_thread_bundle.cc:91:
DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
You can use BrowserThread::IsThreadInitialized() to avoid requiring a
MessageLoop for the check (and then have the check at the top).

https://codereview.chromium.org/2860533002/diff/70001/content/public/test/tes...
content/public/test/test_browser_thread_bundle.cc:104: if
(!base::TaskScheduler::GetInstance()) {
Above this:

// TaskScheduler can sometimes be externally provided by a
base::test::ScopedTaskEnvironment in a sub-fixture. In that case it's expected
to have provided the MessageLoop as well (in which case our |message_loop_|
remains null in Init()).
DCHECK(!base::TaskScheduler::GetInstance() || !message_loop_);

https://codereview.chromium.org/2860533002/diff/70001/content/public/test/tes...
File content/public/test/test_browser_thread_bundle_unittest.cc (right):

https://codereview.chromium.org/2860533002/diff/70001/content/public/test/tes...
content/public/test/test_browser_thread_bundle_unittest.cc:16:
TestBrowserThreadBundle test_browser_thread_bundle;
I don't think we need to support this use case.

[fdoray, 2017-05-03 17:01:25]

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

[commit-bot: I haz the power, 2017-05-03 17:02:21]

Dry run: CQ is trying da patch.

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

[fdoray, 2017-05-03 17:03:50]

Please take another look

https://codereview.chromium.org/2860533002/diff/70001/content/public/test/tes...
File content/public/test/test_browser_thread_bundle.cc (left):

https://codereview.chromium.org/2860533002/diff/70001/content/public/test/tes...
content/public/test/test_browser_thread_bundle.cc:66:
CHECK(message_loop_->IsIdleForTesting());
On 2017/05/03 15:33:10, gab wrote:
> Why move (this was checking above comment) and change from CHECK (since this
is
> test-only code CHECK makes sure this asserts even in non-dcheck builds)?

Done.

https://codereview.chromium.org/2860533002/diff/70001/content/public/test/tes...
content/public/test/test_browser_thread_bundle.cc:72: CHECK(!(options_ &
IO_MAINLOOP) || !(options_ & REAL_IO_THREAD));
On 2017/05/03 15:33:10, gab wrote:
> ditto

Done.

https://codereview.chromium.org/2860533002/diff/70001/content/public/test/tes...
File content/public/test/test_browser_thread_bundle.cc (right):

https://codereview.chromium.org/2860533002/diff/70001/content/public/test/tes...
content/public/test/test_browser_thread_bundle.cc:79: //
BrowserMainLoop::MainMessageLoopStart().
On 2017/05/03 15:33:10, gab wrote:
> Update comment to mention why it could exist already in tests.

Done.

https://codereview.chromium.org/2860533002/diff/70001/content/public/test/tes...
content/public/test/test_browser_thread_bundle.cc:91:
DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2017/05/03 15:33:10, gab wrote:
> You can use BrowserThread::IsThreadInitialized() to avoid requiring a
> MessageLoop for the check (and then have the check at the top).

Done.

https://codereview.chromium.org/2860533002/diff/70001/content/public/test/tes...
content/public/test/test_browser_thread_bundle.cc:104: if
(!base::TaskScheduler::GetInstance()) {
On 2017/05/03 15:33:10, gab wrote:
> Above this:
> 
> // TaskScheduler can sometimes be externally provided by a
> base::test::ScopedTaskEnvironment in a sub-fixture. In that case it's expected
> to have provided the MessageLoop as well (in which case our |message_loop_|
> remains null in Init()).
> DCHECK(!base::TaskScheduler::GetInstance() || !message_loop_);

Done.

https://codereview.chromium.org/2860533002/diff/70001/content/public/test/tes...
File content/public/test/test_browser_thread_bundle_unittest.cc (right):

https://codereview.chromium.org/2860533002/diff/70001/content/public/test/tes...
content/public/test/test_browser_thread_bundle_unittest.cc:16:
TestBrowserThreadBundle test_browser_thread_bundle;
On 2017/05/03 15:33:10, gab wrote:
> I don't think we need to support this use case.

Done.

[gab, 2017-05-03 17:44:41]

lgtm w/ nit

https://codereview.chromium.org/2860533002/diff/90001/content/public/test/tes...
File content/public/test/test_browser_thread_bundle.h (right):

https://codereview.chromium.org/2860533002/diff/90001/content/public/test/tes...
content/public/test/test_browser_thread_bundle.h:58: // a ScopedTaskEnvironment.
In that case, it will use the MessageLoop and the
base::test::ScopedTaskEnvironment

[fdoray, 2017-05-03 17:59:57]

https://codereview.chromium.org/2860533002/diff/90001/content/public/test/tes...
File content/public/test/test_browser_thread_bundle.h (right):

https://codereview.chromium.org/2860533002/diff/90001/content/public/test/tes...
content/public/test/test_browser_thread_bundle.h:58: // a ScopedTaskEnvironment.
In that case, it will use the MessageLoop and the
On 2017/05/03 17:44:41, gab wrote:
> base::test::ScopedTaskEnvironment

Done.

[fdoray, 2017-05-03 18:00:27]

fdoray@chromium.org changed reviewers:
+ sky@chromium.org

[fdoray, 2017-05-03 18:00:28]

sky@: PTAL

[fdoray, 2017-05-03 18:00:37]

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

[commit-bot: I haz the power, 2017-05-03 18:01: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-05-03 19:09:07]

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

[commit-bot: I haz the power, 2017-05-03 19:09:08]

Dry run: This issue passed the CQ dry run.

[sky, 2017-05-04 03:54:31]

LGTM

[fdoray, 2017-05-04 10:16:26]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-05-04 10:16:26]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2860533002/#ps110001
(title: "CR-gab-24")

[commit-bot: I haz the power, 2017-05-04 10:16:45]

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 10:21:27]

CQ is committing da patch.

Bot data: {"patchset_id": 110001, "attempt_start_ts": 1493892986022690,
"parent_rev": "9c4bf486b60e7913eff8100ca33dc0505ef421d9", "commit_rev":
"ba0dacfc73d208fc779907890bc94dbd09ef1aad"}

[commit-bot: I haz the power, 2017-05-04 10:21:39]

Description was changed from

==========
Support base::test::ScopedTaskEnvironment and content::TestBrowserThreadBundle
in the same scope.

A test suite that depends on a test suite base that provides a
base::test::ScopedTaskEnvironment may need browser threads. Currently,
the test suite cannot use content::TestBrowserThreadBundle to get
these browser threads because base::test::ScopedTaskEnvironment and
content::TestBrowserThreadBundle both set the main MessageLoop and
the TaskScheduler. With this CL, content::TestBrowserThreadBundle
doesn't set the main MessageLoop and the TaskScheduler if they
already exist.

E.g.
Test suite: ToolbarActionViewUnitTest
            Needs BrowserThreads
Base test suite: views::ViewsTestBase
                 Provides a ScopedTaskEnvironment

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

to

==========
Support base::test::ScopedTaskEnvironment and content::TestBrowserThreadBundle
in the same scope.

A test suite that depends on a test suite base that provides a
base::test::ScopedTaskEnvironment may need browser threads. Currently,
the test suite cannot use content::TestBrowserThreadBundle to get
these browser threads because base::test::ScopedTaskEnvironment and
content::TestBrowserThreadBundle both set the main MessageLoop and
the TaskScheduler. With this CL, content::TestBrowserThreadBundle
doesn't set the main MessageLoop and the TaskScheduler if they
already exist.

E.g.
Test suite: ToolbarActionViewUnitTest
            Needs BrowserThreads
Base test suite: views::ViewsTestBase
                 Provides a ScopedTaskEnvironment

BUG=

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

[commit-bot: I haz the power, 2017-05-04 10:21:40]

Committed patchset #7 (id:110001) as
https://chromium.googlesource.com/chromium/src/+/ba0dacfc73d208fc779907890bc9...

[Avi (use Gerrit), 2017-05-04 19:10:56]

A revert of this CL (patchset #7 id:110001) has been created in
https://codereview.chromium.org/2859283002/ by avi@chromium.org.

The reason for reverting is:
https://uberchromegw.corp.google.com/i/official.desktop.continuous/builders/w...
fails

failures:
TestBrowserThreadBundleTest.MultipleTestBrowserThreadBundle
TestBrowserThreadBundleTest.MessageLoopTypeMismatch
 .

[fdoray, 2017-05-08 15:46:12]

Description was changed from

==========
Support base::test::ScopedTaskEnvironment and content::TestBrowserThreadBundle
in the same scope.

A test suite that depends on a test suite base that provides a
base::test::ScopedTaskEnvironment may need browser threads. Currently,
the test suite cannot use content::TestBrowserThreadBundle to get
these browser threads because base::test::ScopedTaskEnvironment and
content::TestBrowserThreadBundle both set the main MessageLoop and
the TaskScheduler. With this CL, content::TestBrowserThreadBundle
doesn't set the main MessageLoop and the TaskScheduler if they
already exist.

E.g.
Test suite: ToolbarActionViewUnitTest
            Needs BrowserThreads
Base test suite: views::ViewsTestBase
                 Provides a ScopedTaskEnvironment

BUG=

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

to

==========
Support base::test::ScopedTaskEnvironment and content::TestBrowserThreadBundle
in the same scope.

A test suite that depends on a test suite base that provides a
base::test::ScopedTaskEnvironment may need browser threads. Currently,
the test suite cannot use content::TestBrowserThreadBundle to get
these browser threads because base::test::ScopedTaskEnvironment and
content::TestBrowserThreadBundle both set the main MessageLoop and
the TaskScheduler. With this CL, content::TestBrowserThreadBundle
doesn't set the main MessageLoop and the TaskScheduler if they
already exist.

E.g.
Test suite: ToolbarActionViewUnitTest
            Needs BrowserThreads
Base test suite: views::ViewsTestBase
                 Provides a ScopedTaskEnvironment

BUG=

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

[fdoray, 2017-05-08 15:46:30]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-05-08 15:46:31]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2860533002/#ps130001
(title: "remove check failed")

[commit-bot: I haz the power, 2017-05-08 15:47:08]

CQ is trying da patch.

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

[fdoray, 2017-05-08 15:47:10]

Relanding without expecting "Check failed" string in unit tests.

[fdoray, 2017-05-08 15:48:04]

The CQ bit was unchecked by fdoray@chromium.org

[fdoray, 2017-05-08 15:48:20]

The CQ bit was checked by fdoray@chromium.org

[fdoray, 2017-05-08 15:48:20]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2860533002/#ps150001
(title: "remove check failed")

[commit-bot: I haz the power, 2017-05-08 15:48:58]

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-08 16:59:46]

CQ is committing da patch.

Bot data: {"patchset_id": 150001, "attempt_start_ts": 1494258500511010,
"parent_rev": "b38dc9a7d0f5457f892a031b407f0346b666430d", "commit_rev":
"8c5805290290f2e7749c27ee7ea005a980398947"}

[commit-bot: I haz the power, 2017-05-08 16:59:56]

Description was changed from

==========
Support base::test::ScopedTaskEnvironment and content::TestBrowserThreadBundle
in the same scope.

A test suite that depends on a test suite base that provides a
base::test::ScopedTaskEnvironment may need browser threads. Currently,
the test suite cannot use content::TestBrowserThreadBundle to get
these browser threads because base::test::ScopedTaskEnvironment and
content::TestBrowserThreadBundle both set the main MessageLoop and
the TaskScheduler. With this CL, content::TestBrowserThreadBundle
doesn't set the main MessageLoop and the TaskScheduler if they
already exist.

E.g.
Test suite: ToolbarActionViewUnitTest
            Needs BrowserThreads
Base test suite: views::ViewsTestBase
                 Provides a ScopedTaskEnvironment

BUG=

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

to

==========
Support base::test::ScopedTaskEnvironment and content::TestBrowserThreadBundle
in the same scope.

A test suite that depends on a test suite base that provides a
base::test::ScopedTaskEnvironment may need browser threads. Currently,
the test suite cannot use content::TestBrowserThreadBundle to get
these browser threads because base::test::ScopedTaskEnvironment and
content::TestBrowserThreadBundle both set the main MessageLoop and
the TaskScheduler. With this CL, content::TestBrowserThreadBundle
doesn't set the main MessageLoop and the TaskScheduler if they
already exist.

E.g.
Test suite: ToolbarActionViewUnitTest
            Needs BrowserThreads
Base test suite: views::ViewsTestBase
                 Provides a ScopedTaskEnvironment

BUG=

Review-Url: https://codereview.chromium.org/2860533002
Cr-Original-Commit-Position: refs/heads/master@{#469304}
Committed:
https://chromium.googlesource.com/chromium/src/+/ba0dacfc73d208fc779907890bc9...
Review-Url: https://codereview.chromium.org/2860533002
Cr-Commit-Position: refs/heads/master@{#470021}
Committed:
https://chromium.googlesource.com/chromium/src/+/8c5805290290f2e7749c27ee7ea0...
==========

[commit-bot: I haz the power, 2017-05-08 16:59:57]

Committed patchset #9 (id:150001) as
https://chromium.googlesource.com/chromium/src/+/8c5805290290f2e7749c27ee7ea0...