Use MessageLoop for CompositorTest.LocksTimeOut

Use MessageLoop for CompositorTest.LocksTimeOut

Instead of TestSimpleTaskRunner, which DCHECKs when posting message
from worker thread, use MessageLoop and set a timeout that is
equivalent to CompositorLock's timemout.

BUG=473507
TEST=run w/ --ui-enable-impl-side-painting

Committed: https://crrev.com/4b4d89e3c7dd813d433f851166396bff25938b83
Cr-Commit-Position: refs/heads/master@{#325385}

[weiliangc, 2015-04-14 21:13:34]

Patchset #3 (id:40001) has been deleted

[weiliangc, 2015-04-14 21:15:24]

weiliangc@chromium.org changed reviewers:
+ reveman@chromium.org, vollick@chromium.org

[weiliangc, 2015-04-14 21:16:35]

Create a simple task runner that is thread safe, and use that for
CompositorTest.LocksTimeOut.

[reveman, 2015-04-14 21:47:19]

https://codereview.chromium.org/1081223002/diff/60001/ui/compositor/composito...
File ui/compositor/compositor_unittest.cc (right):

https://codereview.chromium.org/1081223002/diff/60001/ui/compositor/composito...
ui/compositor/compositor_unittest.cc:26: // Thread safe and slim version of
TestSimpleTaskRunner. This is done by using
nit: s/TestSimpleTaskRunner/base::TestSimpleTaskRunner/

https://codereview.chromium.org/1081223002/diff/60001/ui/compositor/composito...
ui/compositor/compositor_unittest.cc:27: // mutex to proect pending_tasks_.
protect

https://codereview.chromium.org/1081223002/diff/60001/ui/compositor/composito...
ui/compositor/compositor_unittest.cc:28: class TestThreadSimpleTaskRunner :
public base::SingleThreadTaskRunner {
nit: s/TestThreadSimpleTaskRunner/TestThreadSafeSimpleTaskRunner/ or maybe it's
fine to just reuse the TestSimpleTaskRunner name here.

https://codereview.chromium.org/1081223002/diff/60001/ui/compositor/composito...
ui/compositor/compositor_unittest.cc:59: void ClearPendingTasks() {
Do you need this function?

https://codereview.chromium.org/1081223002/diff/60001/ui/compositor/composito...
ui/compositor/compositor_unittest.cc:82: while (!pending_tasks_.empty()) {
This is not thread safe. You need to acquire the lock before accessing
pending_tasks_.

Maybe RunPendingTasks() can return true if |tasks_to_run| was non-empty and we
can just do something like this here:

while (RunPendingTasks())
  continue;

[weiliangc, 2015-04-14 22:07:53]

Address review comments.

https://codereview.chromium.org/1081223002/diff/60001/ui/compositor/composito...
File ui/compositor/compositor_unittest.cc (right):

https://codereview.chromium.org/1081223002/diff/60001/ui/compositor/composito...
ui/compositor/compositor_unittest.cc:26: // Thread safe and slim version of
TestSimpleTaskRunner. This is done by using
On 2015/04/14 21:47:19, reveman wrote:
> nit: s/TestSimpleTaskRunner/base::TestSimpleTaskRunner/

Done.

https://codereview.chromium.org/1081223002/diff/60001/ui/compositor/composito...
ui/compositor/compositor_unittest.cc:27: // mutex to proect pending_tasks_.
On 2015/04/14 21:47:19, reveman wrote:
> protect

Done.

https://codereview.chromium.org/1081223002/diff/60001/ui/compositor/composito...
ui/compositor/compositor_unittest.cc:28: class TestThreadSimpleTaskRunner :
public base::SingleThreadTaskRunner {
On 2015/04/14 21:47:19, reveman wrote:
> nit: s/TestThreadSimpleTaskRunner/TestThreadSafeSimpleTaskRunner/ or maybe
it's
> fine to just reuse the TestSimpleTaskRunner name here.

Done.

https://codereview.chromium.org/1081223002/diff/60001/ui/compositor/composito...
ui/compositor/compositor_unittest.cc:59: void ClearPendingTasks() {
On 2015/04/14 21:47:19, reveman wrote:
> Do you need this function?

Not really. Gone.

https://codereview.chromium.org/1081223002/diff/60001/ui/compositor/composito...
ui/compositor/compositor_unittest.cc:82: while (!pending_tasks_.empty()) {
On 2015/04/14 21:47:19, reveman wrote:
> This is not thread safe. You need to acquire the lock before accessing
> pending_tasks_.
> 
> Maybe RunPendingTasks() can return true if |tasks_to_run| was non-empty and we
> can just do something like this here:
> 
> while (RunPendingTasks())
>   continue;

Done.

[reveman, 2015-04-14 22:42:10]

lgtm with nit

https://codereview.chromium.org/1081223002/diff/80001/ui/compositor/composito...
File ui/compositor/compositor_unittest.cc (right):

https://codereview.chromium.org/1081223002/diff/80001/ui/compositor/composito...
ui/compositor/compositor_unittest.cc:28: // mutex to protect pending_tasks_.
nit: fix line wrapping

https://codereview.chromium.org/1081223002/diff/80001/ui/compositor/composito...
ui/compositor/compositor_unittest.cc:67: if (tasks_to_run.empty())
nit: you can remove this and just "return !tasks_to_run.empty()" instead of
"return true" below instead.

[weiliangc, 2015-04-15 15:47:26]

Updated and rebased. :)

[weiliangc, 2015-04-15 15:56:51]

Patchset #6 (id:120001) has been deleted

[weiliangc, 2015-04-16 00:06:12]

Edit according to piman's suggestion:
- send another timeout task to MessageLoop to check and use MessageLoop.

This is also because the thread safe version of TestSimpleTaskRunner doesn't
respect the delay that task is posted with.

[weiliangc, 2015-04-16 00:06:25]

weiliangc@chromium.org changed reviewers:
+ piman@chromium.org

[piman, 2015-04-16 00:10:03]

lgtm

https://codereview.chromium.org/1081223002/diff/160001/ui/compositor/composit...
File ui/compositor/compositor_unittest.cc (right):

https://codereview.chromium.org/1081223002/diff/160001/ui/compositor/composit...
ui/compositor/compositor_unittest.cc:21: const int kCompositorLockTimeoutMs =
67;
nit: maybe you can move it to the header?

[weiliangc, 2015-04-16 01:35:20]

On 2015/04/16 00:10:03, piman (Very slow to review) wrote:
> lgtm
> 
>
https://codereview.chromium.org/1081223002/diff/160001/ui/compositor/composit...
> File ui/compositor/compositor_unittest.cc (right):
> 
>
https://codereview.chromium.org/1081223002/diff/160001/ui/compositor/composit...
> ui/compositor/compositor_unittest.cc:21: const int kCompositorLockTimeoutMs =
> 67;
> nit: maybe you can move it to the header?

Thanks!

Done.

[weiliangc, 2015-04-16 01:35:29]

The CQ bit was checked by weiliangc@chromium.org

[weiliangc, 2015-04-16 01:35:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1081223002/#ps180001
(title: "move constant to header")

[commit-bot: I haz the power, 2015-04-16 01:36:08]

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

[commit-bot: I haz the power, 2015-04-16 03:19:08]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2015-04-16 03:20:26]

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

[danakj, 2015-04-21 21:18:28]

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

[danakj, 2015-04-21 21:18:29]

LGTM but MLP is deprecated.

https://codereview.chromium.org/1081223002/diff/180001/ui/compositor/composit...
File ui/compositor/compositor_unittest.cc (right):

https://codereview.chromium.org/1081223002/diff/180001/ui/compositor/composit...
ui/compositor/compositor_unittest.cc:33: task_runner_ =
base::MessageLoopProxy::current();
This should be ThreadTaskRunnerHandle::Get()

https://codereview.chromium.org/1081223002/diff/180001/ui/compositor/composit...
ui/compositor/compositor_unittest.cc:47: base::MessageLoopProxy* task_runner() {
return task_runner_.get(); }
This should be a SingleThreadTaskRunner, MLP is deprecated.

https://codereview.chromium.org/1081223002/diff/180001/ui/compositor/composit...
ui/compositor/compositor_unittest.cc:51: scoped_refptr<base::MessageLoopProxy>
task_runner_;
This should be SingleThreadTaskRunner too

[weiliangc, 2015-04-22 19:33:04]

On 2015/04/21 at 21:18:29, danakj wrote:
> LGTM but MLP is deprecated.
> 
>
https://codereview.chromium.org/1081223002/diff/180001/ui/compositor/composit...
> File ui/compositor/compositor_unittest.cc (right):
> 
>
https://codereview.chromium.org/1081223002/diff/180001/ui/compositor/composit...
> ui/compositor/compositor_unittest.cc:33: task_runner_ =
base::MessageLoopProxy::current();
> This should be ThreadTaskRunnerHandle::Get()
> 
>
https://codereview.chromium.org/1081223002/diff/180001/ui/compositor/composit...
> ui/compositor/compositor_unittest.cc:47: base::MessageLoopProxy* task_runner()
{ return task_runner_.get(); }
> This should be a SingleThreadTaskRunner, MLP is deprecated.
> 
>
https://codereview.chromium.org/1081223002/diff/180001/ui/compositor/composit...
> ui/compositor/compositor_unittest.cc:51: scoped_refptr<base::MessageLoopProxy>
task_runner_;
> This should be SingleThreadTaskRunner too

here: https://codereview.chromium.org/1063823007