Use RunLoop to wait for readback on compositor unittest

Use RunLoop to wait for readback on compositor unittest

In layer unittest ReadPixel loops multiple times to wait to a readback.
This causes flake on cros valgrind bot. Instead use RunLoop to wait for
readback.

The flake is exposed if ui::Compositor uses Scheduler (CL 535733002)

BUG=403528
Committed: https://crrev.com/cb9e0df0797ec7d2c894b95570b66553d3bfa661
Cr-Commit-Position: refs/heads/master@{#295972}

[weiliangc, 2014-09-19 20:44:08]

weiliangc@chromium.org changed reviewers:
+ danakj@chromium.org, enne@chromium.org

[weiliangc, 2014-09-19 20:48:56]

Fix for ui::Compositor using Scheduler cros valgrind bot flake.
Out for comment, not for commit. Will merge with enne's patch before commit.

[danakj, 2014-09-19 20:52:27]

https://codereview.chromium.org/587863002/diff/1/ui/compositor/layer_unittest.cc
File ui/compositor/layer_unittest.cc (right):

https://codereview.chromium.org/587863002/diff/1/ui/compositor/layer_unittest...
ui/compositor/layer_unittest.cc:136: return ReadPixels(bitmap,
gfx::Rect(GetCompositor()->size()));
remove return

https://codereview.chromium.org/587863002/diff/1/ui/compositor/layer_unittest...
ui/compositor/layer_unittest.cc:190: DCHECK(run_loop_->running());
what if the result comes before the wait starts, this looks like a race and it's
not important.

https://codereview.chromium.org/587863002/diff/1/ui/compositor/layer_unittest...
ui/compositor/layer_unittest.cc:196: void WaitForReadback() { run_loop_->Run();
}
can you verify that Run() will exit if you called the QuitClosure() before
calling Run() otherwise the race is bad, and you need to check if the callback
already happened and skip Run()

[weiliangc, 2014-09-19 21:02:47]

https://codereview.chromium.org/587863002/diff/1/ui/compositor/layer_unittest.cc
File ui/compositor/layer_unittest.cc (right):

https://codereview.chromium.org/587863002/diff/1/ui/compositor/layer_unittest...
ui/compositor/layer_unittest.cc:136: return ReadPixels(bitmap,
gfx::Rect(GetCompositor()->size()));
On 2014/09/19 20:52:26, danakj wrote:
> remove return

Done.

https://codereview.chromium.org/587863002/diff/1/ui/compositor/layer_unittest...
ui/compositor/layer_unittest.cc:190: DCHECK(run_loop_->running());
On 2014/09/19 20:52:26, danakj wrote:
> what if the result comes before the wait starts, this looks like a race and
it's
> not important.

Make sense. Removed.

https://codereview.chromium.org/587863002/diff/1/ui/compositor/layer_unittest...
ui/compositor/layer_unittest.cc:196: void WaitForReadback() { run_loop_->Run();
}
On 2014/09/19 20:52:26, danakj wrote:
> can you verify that Run() will exit if you called the QuitClosure() before
> calling Run() otherwise the race is bad, and you need to check if the callback
> already happened and skip Run()

RunLoop's comment on its Quit and QuitClose function says:
- calling Quit before Run will just cause Run to exit immediately
- QuitClosure calls Quit safely, has no effect if RunLoop is gone

How much trust can I put into these comments?

[danakj, 2014-09-19 22:51:15]

Why is this (NotForCommit)?

https://codereview.chromium.org/587863002/diff/20001/ui/compositor/layer_unit...
File ui/compositor/layer_unittest.cc (right):

https://codereview.chromium.org/587863002/diff/20001/ui/compositor/layer_unit...
ui/compositor/layer_unittest.cc:191:
base::MessageLoop::current()->PostTask(FROM_HERE,
you could just QuitClosure().Run() instead of posttasking it.

but even better just run_loop_.Quit(); :)

[weiliangc, 2014-09-19 23:04:45]

On 2014/09/19 22:51:15, danakj wrote:
> Why is this (NotForCommit)?
> 

\BecauseWeSmart{}

(Just typed a long response then realized this CL is pretty independent...)

[danakj, 2014-09-19 23:36:17]

https://codereview.chromium.org/587863002/diff/20001/ui/compositor/layer_unit...
File ui/compositor/layer_unittest.cc (right):

https://codereview.chromium.org/587863002/diff/20001/ui/compositor/layer_unit...
ui/compositor/layer_unittest.cc:191:
base::MessageLoop::current()->PostTask(FROM_HERE,
On 2014/09/19 22:51:15, danakj wrote:
> you could just QuitClosure().Run() instead of posttasking it.
> 
> but even better just run_loop_.Quit(); :)

LGTM once you Quit() more directly here.

[weiliangc, 2014-09-20 00:05:57]

On 2014/09/19 23:36:17, danakj wrote:
>
https://codereview.chromium.org/587863002/diff/20001/ui/compositor/layer_unit...
> File ui/compositor/layer_unittest.cc (right):
> 
>
https://codereview.chromium.org/587863002/diff/20001/ui/compositor/layer_unit...
> ui/compositor/layer_unittest.cc:191:
> base::MessageLoop::current()->PostTask(FROM_HERE,
> On 2014/09/19 22:51:15, danakj wrote:
> > you could just QuitClosure().Run() instead of posttasking it.
> > 
> > but even better just run_loop_.Quit(); :)
> 
> LGTM once you Quit() more directly here.

Track trunk, will submit after some green trybots.

[weiliangc, 2014-09-22 14:12:19]

The CQ bit was checked by weiliangc@chromium.org

[commit-bot: I haz the power, 2014-09-22 14:13:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/587863002/60001

[commit-bot: I haz the power, 2014-09-22 14:22:37]

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

[commit-bot: I haz the power, 2014-09-22 14:22:38]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[danakj, 2014-09-22 14:37:43]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-09-22 14:38:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/587863002/60001

[commit-bot: I haz the power, 2014-09-22 14:55:30]

Committed patchset #4 (id:60001) as 907a55f1b26028558f3a6e4542f5b7fe2bae545a

[commit-bot: I haz the power, 2014-09-22 14:56:08]

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