Add FlushForTesting to base::Thread.

Add FlushForTesting to base::Thread.

Grew tired of the post+wait pattern in many tests to flush a base::Thread.
Was going to need it again in https://codereview.chromium.org/2333023003/
and decided to add an API for everyone's sake instead :-).

Off the top of my head, will also be useful in
https://codereview.chromium.org/2299523003/

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

[gab, 2016-09-13 20:30:34]

Description was changed from

==========
Add FlushForTesting to base::Thread.

Grew tired of the post+wait pattern in many tests to flush a base::Thread.
Was going to need it again in https://codereview.chromium.org/2333023003/
and decided to add an API for everyone's sake instead :-).

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

to

==========
Add FlushForTesting to base::Thread.

Grew tired of the post+wait pattern in many tests to flush a base::Thread.
Was going to need it again in https://codereview.chromium.org/2333023003/
and decided to add an API for everyone's sake instead :-).

Off the top of my head, will also be useful in
https://codereview.chromium.org/2299523003/

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

[gab, 2016-09-13 20:30:38]

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

[gab, 2016-09-13 20:30:53]

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

[gab, 2016-09-13 20:30:54]

Dana PTAL, thanks!

[commit-bot: I haz the power, 2016-09-13 20:30:57]

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-13 22:51:05]

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

[commit-bot: I haz the power, 2016-09-13 22:51:07]

Dry run: This issue passed the CQ dry run.

[gab, 2016-09-15 17:58:46]

On 2016/09/13 20:30:54, gab wrote:
> Dana PTAL, thanks!

post-perf ping, thanks!

[danakj, 2016-09-15 18:09:33]

https://codereview.chromium.org/2337253003/diff/1/base/threading/thread.cc
File base/threading/thread.cc (right):

https://codereview.chromium.org/2337253003/diff/1/base/threading/thread.cc#ne...
base/threading/thread.cc:147: void Thread::FlushForTesting() {
I'd prefer a free function in base/test/ actually. There's no reason this has to
be on Thread right?

[gab, 2016-09-15 20:18:34]

https://codereview.chromium.org/2337253003/diff/1/base/threading/thread.cc
File base/threading/thread.cc (right):

https://codereview.chromium.org/2337253003/diff/1/base/threading/thread.cc#ne...
base/threading/thread.cc:147: void Thread::FlushForTesting() {
On 2016/09/15 18:09:33, danakj wrote:
> I'd prefer a free function in base/test/ actually. There's no reason this has
to
> be on Thread right?

Maybe discoverability is a reason? i.e. I just realized that such a thing
already more or less exists
(https://cs.chromium.org/chromium/src/base/test/thread_test_helper.h) ...
whereas I've always seen "FlushForTesting" methods on APIs themselves and never
thought of looking in base/test for this.

WDYT?

[danakj, 2016-09-15 20:54:49]

On Thu, Sep 15, 2016 at 1:18 PM, <gab@chromium.org> wrote:

>
> https://codereview.chromium.org/2337253003/diff/1/base/threading/thread.cc
> File base/threading/thread.cc (right):
>
> https://codereview.chromium.org/2337253003/diff/1/base/
> threading/thread.cc#newcode147
> base/threading/thread.cc:147: void Thread::FlushForTesting() {
> On 2016/09/15 18:09:33, danakj wrote:
> > I'd prefer a free function in base/test/ actually. There's no reason
> this has to
> > be on Thread right?
>
> Maybe discoverability is a reason? i.e. I just realized that such a
> thing already more or less exists
> (https://cs.chromium.org/chromium/src/base/test/thread_test_helper.h)
> ... whereas I've always seen "FlushForTesting" methods on APIs
> themselves and never thought of looking in base/test for this.
>
> WDYT?
>

I was kinda wondering if such a thing might already exist, that is a bit
more complicated though. For code search I see FlushForTesting() has become
a bit of a meme in the codebase already, so maybe we do this... I just
don't like ForTesting methods, unless they are really needed to avoid
terrible things. If it's for testing it should be in a test-only target.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2016-09-15 20:55:00]

So LGTM cuz this appears to be a pattern, but if you're semi convinced about
doing this in base/test/ or reusing/changing base/test/ things I'd personally
prefer.

[gab, 2016-09-15 21:11:47]

On 2016/09/15 20:55:00, danakj wrote:
> So LGTM cuz this appears to be a pattern, but if you're semi convinced about
> doing this in base/test/ or reusing/changing base/test/ things I'd personally
> prefer.

I was semi-convinced about base/test/ initially until I realized it already
kinda had a thing doing this that I wasn't aware of, which made me realize that
base/test/ isn't very discoverable compared to a class' API. So this makes me
lean towards FlushForTesting() for now.

[gab, 2016-09-15 21:11:55]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2016-09-15 21:12:41]

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-15 23:04:28]

Description was changed from

==========
Add FlushForTesting to base::Thread.

Grew tired of the post+wait pattern in many tests to flush a base::Thread.
Was going to need it again in https://codereview.chromium.org/2333023003/
and decided to add an API for everyone's sake instead :-).

Off the top of my head, will also be useful in
https://codereview.chromium.org/2299523003/

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

to

==========
Add FlushForTesting to base::Thread.

Grew tired of the post+wait pattern in many tests to flush a base::Thread.
Was going to need it again in https://codereview.chromium.org/2333023003/
and decided to add an API for everyone's sake instead :-).

Off the top of my head, will also be useful in
https://codereview.chromium.org/2299523003/

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

[commit-bot: I haz the power, 2016-09-15 23:04:29]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-15 23:06:34]

Description was changed from

==========
Add FlushForTesting to base::Thread.

Grew tired of the post+wait pattern in many tests to flush a base::Thread.
Was going to need it again in https://codereview.chromium.org/2333023003/
and decided to add an API for everyone's sake instead :-).

Off the top of my head, will also be useful in
https://codereview.chromium.org/2299523003/

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

to

==========
Add FlushForTesting to base::Thread.

Grew tired of the post+wait pattern in many tests to flush a base::Thread.
Was going to need it again in https://codereview.chromium.org/2333023003/
and decided to add an API for everyone's sake instead :-).

Off the top of my head, will also be useful in
https://codereview.chromium.org/2299523003/

BUG=646443

Committed: https://crrev.com/54ef83562c4846bba5b4a7f923b15410eefd1a0f
Cr-Commit-Position: refs/heads/master@{#419026}
==========

[commit-bot: I haz the power, 2016-09-15 23:06:35]

Patchset 1 (id:??) landed as
https://crrev.com/54ef83562c4846bba5b4a7f923b15410eefd1a0f
Cr-Commit-Position: refs/heads/master@{#419026}