Make HistoryDataStoreTest and SearchHistoryTest's SequencedWorkerPool multi-threaded.

Make HistoryDataStoreTest and SearchHistoryTest's SequencedWorkerPool multi-threaded.

BUG=646443
Committed: https://crrev.com/eb5fb080ec2c32864e7da2a2c1a3783120eafa1e
Cr-Commit-Position: refs/heads/master@{#419085}

[gab, 2016-09-14 19:56:14]

Description was changed from

==========
Make SearchHistoryTest's SequencedWorkerPool multi-threaded.

BUG=646443
==========

to

==========
Make HistoryDataStoreTest and SearchHistoryTest's SequencedWorkerPool
multi-threaded.

BUG=646443
==========

[gab, 2016-09-14 19:57:07]

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

[gab, 2016-09-14 19:57:08]

Matt PTaL, thanks!

[gab, 2016-09-14 19:57:46]

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

[commit-bot: I haz the power, 2016-09-14 19:59:10]

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-09-14 20:04:15]

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

[commit-bot: I haz the power, 2016-09-14 20:04:16]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  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...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_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, 2016-09-14 20:27:43]

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

[commit-bot: I haz the power, 2016-09-14 20:28: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, 2016-09-14 21:11:43]

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

[commit-bot: I haz the power, 2016-09-14 21:11:44]

Dry run: This issue passed the CQ dry run.

[gab, 2016-09-15 18:01:06]

On 2016/09/14 19:57:08, gab wrote:
> Matt PTaL, thanks!

ping mgiuca@, thanks!

[Matt Giuca, 2016-09-15 23:54:20]

https://codereview.chromium.org/2345583002/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/search/history_unittest.cc (right):

https://codereview.chromium.org/2345583002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/history_unittest.cc:99: new
base::SequencedWorkerPoolOwner(2, "AppLauncherTest"));
I'm not really familiar with this code. Is there a good reason it has to be
multi-threaded in a unit test?

It is probably not going to change anything, but it means there's a new
possibility of flakiness (things happening out of order) that weren't flaky
before.

https://codereview.chromium.org/2345583002/diff/40001/ui/app_list/search/hist...
File ui/app_list/search/history_data_store_unittest.cc (right):

https://codereview.chromium.org/2345583002/diff/40001/ui/app_list/search/hist...
ui/app_list/search/history_data_store_unittest.cc:44: HistoryDataStoreTest() :
worker_pool_owner_(2, "AppLanucherTest") {}
Same.

[gab, 2016-09-16 00:54:41]

https://codereview.chromium.org/2345583002/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/search/history_unittest.cc (right):

https://codereview.chromium.org/2345583002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/history_unittest.cc:99: new
base::SequencedWorkerPoolOwner(2, "AppLauncherTest"));
On 2016/09/15 23:54:20, Matt Giuca wrote:
> I'm not really familiar with this code. Is there a good reason it has to be
> multi-threaded in a unit test?
> 
> It is probably not going to change anything, but it means there's a new
> possibility of flakiness (things happening out of order) that weren't flaky
> before.

In this test it's used to pass to a DictionaryDataStore
(https://cs.chromium.org/chromium/src/ui/app_list/search/dictionary_data_store...)
which in production receives a real (multi-threaded) SequencedWorkerPool. So if
this results in flakes it would mean DictionaryDataStore doesn't probably use a
SequencedWorkerPool.

i.e. if anything this brings the test closer to what runs in production.

Making the SWP multi-threaded (i.e. max_threads > 1) just means it's allowed to
have multiple threads, but if the caller only uses one sequence things are still
going to be sequenced on a single thread -- if that's the case this change is a
no-op in practice (but we want to do it regardless as we're looking to disallow
single-threaded SequencedWorkerPools so that assuming thread-affinity from a SWP
is never okay, users that want that should use base::Thread).

https://codereview.chromium.org/2345583002/diff/40001/ui/app_list/search/hist...
File ui/app_list/search/history_data_store_unittest.cc (right):

https://codereview.chromium.org/2345583002/diff/40001/ui/app_list/search/hist...
ui/app_list/search/history_data_store_unittest.cc:44: HistoryDataStoreTest() :
worker_pool_owner_(2, "AppLanucherTest") {}
On 2016/09/15 23:54:20, Matt Giuca wrote:
> Same.

Same.

[Matt Giuca, 2016-09-16 01:27:44]

OK thanks for the explanation, lgtm.

[gab, 2016-09-16 02:32:27]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2016-09-16 02:32:39]

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-09-16 03:02:40]

Description was changed from

==========
Make HistoryDataStoreTest and SearchHistoryTest's SequencedWorkerPool
multi-threaded.

BUG=646443
==========

to

==========
Make HistoryDataStoreTest and SearchHistoryTest's SequencedWorkerPool
multi-threaded.

BUG=646443
==========

[commit-bot: I haz the power, 2016-09-16 03:02:45]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-16 03:04:52]

Description was changed from

==========
Make HistoryDataStoreTest and SearchHistoryTest's SequencedWorkerPool
multi-threaded.

BUG=646443
==========

to

==========
Make HistoryDataStoreTest and SearchHistoryTest's SequencedWorkerPool
multi-threaded.

BUG=646443

Committed: https://crrev.com/eb5fb080ec2c32864e7da2a2c1a3783120eafa1e
Cr-Commit-Position: refs/heads/master@{#419085}
==========

[commit-bot: I haz the power, 2016-09-16 03:04:55]

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