Replaced TestNowSource with SimpleTestTickClock.

Replaced TestNowSource with SimpleTestTickClock.

Removed TestNowSource completely. Its usage updated under cc/test, cc/scheduler, cc/surfaces and components/scheduler with coressponding api of SimpleTestTickClock. Refactored num_now_calls_ functionality in NowNotCalledWhenThereAreNoDelayedTasks test to use TestAlwaysFailTimeSource

BUG=440281, 494887
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2e4c4e14344779d903ea578ae1be4663801d9f05
Cr-Commit-Position: refs/heads/master@{#334341}

[Ankur Verma, 2015-05-14 07:02:21]

ankur1.verma@samsung.com changed reviewers:
+ brianderson@chromium.org, mithro@mithis.com, skyostil@chromium.org

[Ankur Verma, 2015-05-14 07:02:21]

Please have a look. Thanks!

[mithro-old, 2015-05-19 02:20:40]

Hi Ankur,

Before we proceed you'll need to check with OWNERS of base/test to see if they
are happy with the changes there, then we can look at it more closely from a cc/
perspective. This patch should actually delete TestNowSource too.

Tim 'mithro' Ansell

[Ankur Verma, 2015-05-19 11:22:08]

Thanks for taking a look, Tim.

I've re-based the patch onto latest code since the previous patch ended up
conflicting for try jobs.
I've also removed some unnecessary api's from base/test/simple_test_tick_clock.h

However, i cannot delete TestNowSource just yet since there still exist
dependencies under:
components/scheduler/child/test_time_source.cc
components/scheduler/child/worker_scheduler_impl_unittest.cc
components/scheduler/child/scheduler_helper_unittest.cc
components/scheduler/child/test_time_source.h
components/scheduler/child/task_queue_manager_unittest.cc
components/scheduler/renderer/deadline_task_runner_unittest.cc
components/scheduler/renderer/renderer_scheduler_impl_unittest.cc

If this patch looks good, then i'll move onto removing other dependencies in a
separate patch.

For base/test, i'll add phajdan.jr@chromium.org for review.

[Ankur Verma, 2015-05-19 11:23:35]

ankur1.verma@samsung.com changed reviewers:
+ phajdan.jr@chromium.org

[Ankur Verma, 2015-05-19 11:28:57]

Hi Pawel,
Could you please review changes under base/test/?
I've added few api's to SimpleTestTickClock to support usage in cc/test/.

[Ankur Verma, 2015-05-26 07:11:25]

Hi Pawel,
Gentle reminder. Could you please take a look at minor changes to
base/test/simple_test_tick_clock.*
Thanks!

[Paweł Hajdan Jr., 2015-05-26 11:30:25]

https://codereview.chromium.org/1132753008/diff/20001/base/test/simple_test_t...
File base/test/simple_test_tick_clock.h (right):

https://codereview.chromium.org/1132753008/diff/20001/base/test/simple_test_t...
base/test/simple_test_tick_clock.h:22: SimpleTestTickClock(int64_t initial);
Why are these additional ctors needed?

https://codereview.chromium.org/1132753008/diff/20001/base/test/simple_test_t...
base/test/simple_test_tick_clock.h:27: void Set(TimeTicks time);
Why is Set needed?

https://codereview.chromium.org/1132753008/diff/20001/base/test/simple_test_t...
base/test/simple_test_tick_clock.h:32: // Convenience functions to make the now
source easier to use in unit
I don't see a reason enough for these convenience methods. Can't callers call
Advance?

[Ankur Verma, 2015-05-26 12:58:29]

Need to remove additions to SimpleTestTickClock and update usage accordingly
under cc/test. Will upload updated patch.
Thanks Pawel, for having a look.

https://codereview.chromium.org/1132753008/diff/20001/base/test/simple_test_t...
File base/test/simple_test_tick_clock.h (right):

https://codereview.chromium.org/1132753008/diff/20001/base/test/simple_test_t...
base/test/simple_test_tick_clock.h:22: SimpleTestTickClock(int64_t initial);
These are convenience ctors really, for usage under cctests. Can be replaced by
callers with Advance().

https://codereview.chromium.org/1132753008/diff/20001/base/test/simple_test_t...
base/test/simple_test_tick_clock.h:27: void Set(TimeTicks time);
Again meant for convenient usage under cctests. Can be replaced with Advance().

https://codereview.chromium.org/1132753008/diff/20001/base/test/simple_test_t...
base/test/simple_test_tick_clock.h:32: // Convenience functions to make the now
source easier to use in unit
I understand SimpleTestTickClock must be as simple as possible. I will remove
these additions and change cctests accordingly.

Thanks for the review.

[Ankur Verma, 2015-05-28 14:14:59]

ankur1.verma@samsung.com changed reviewers:
- phajdan.jr@chromium.org

[Ankur Verma, 2015-05-28 14:15:00]

Hello Tim & Sami. PTAL.

[Ankur Verma, 2015-05-29 04:45:43]

@Tim
I've removed TestNowSource completely. I realized that my previous changes broke
components_unittests, thus updated components/scheduler parts as well.

@Sami
Request you to kindly review the components/scheduler changes which fall part of
this cleanup.

Thanks!

[mithro-old, 2015-05-29 08:26:48]

Generally looks good (there is a bit of weirdness from stuff unrelated to this
patch). 

I also start the patch running on a couple of the try bots to see if it breaks
anything.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
File components/scheduler/child/task_queue_manager_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/task_queue_manager_unittest.cc:250:
TEST_F(TaskQueueManagerTest, NowNotCalledWhenThereAreNoDelayedTasks) {
Sami, this seems like a very weird test - any idea on what the history here is?

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
File components/scheduler/child/test_time_source.h (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/test_time_source.h:25: mutable int num_now_calls_;
The mutable keyword doesn't do what you probably think it does. However, that
looks like it was a bug in the original code too.

[Sami, 2015-05-29 14:00:58]

Thanks, I've added a few comments.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
File components/scheduler/child/idle_helper_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/idle_helper_unittest.cc:142:
clock_->Advance(base::TimeDelta::FromInternalValue(5000));
Do any of the tests fail if we start the clock at zero? I'm not sure we actually
need to start at a non-zero time here.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/idle_helper_unittest.cc:214:
base::SimpleTestTickClock* clock_;
Why isn't this a scoped_ptr? Looks like we're leaking the allocation now.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
File components/scheduler/child/scheduler_helper_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/scheduler_helper_unittest.cc:47:
clock_->Advance(base::TimeDelta::FromInternalValue(5000));
Same question about the starting value as before.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/scheduler_helper_unittest.cc:90:
base::SimpleTestTickClock* clock_;
scoped_ptr here too.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
File components/scheduler/child/task_queue_manager_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/task_queue_manager_unittest.cc:250:
TEST_F(TaskQueueManagerTest, NowNotCalledWhenThereAreNoDelayedTasks) {
On 2015/05/29 08:26:48, mithro wrote:
> Sami, this seems like a very weird test - any idea on what the history here
is?

The idea is to avoid reading the current time anywhere unless we have tasks that
require knowing it (i.e., delayed tasks).

I think it might be cleaner to rewrite it to use a SimpleTestTickClock that
fails an assert if anyone calls into it.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/task_queue_manager_unittest.cc:1050:
clock->Advance(base::TimeDelta::FromInternalValue(10000));
Where did the value 10000 come from?

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
File components/scheduler/child/worker_scheduler_impl_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/worker_scheduler_impl_unittest.cc:103:
clock_->Advance(base::TimeDelta::FromInternalValue(5000));
Ditto about the starting time value.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/worker_scheduler_impl_unittest.cc:192:
base::SimpleTestTickClock* clock_;
Ditto about scoped_ptr.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/re...
File components/scheduler/renderer/deadline_task_runner_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/re...
components/scheduler/renderer/deadline_task_runner_unittest.cc:21:
clock_->Advance(base::TimeDelta::FromInternalValue(5000));
Ditto about base time.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/re...
File components/scheduler/renderer/renderer_scheduler_impl_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl_unittest.cc:127:
clock_->Advance(base::TimeDelta::FromInternalValue(5000));
Ditto about base time.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl_unittest.cc:156:
clock_->Advance(base::TimeDelta::FromInternalValue(5000));
Ditto about base time.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl_unittest.cc:296:
base::SimpleTestTickClock* clock_;
Ditto about scoped_ptr.

[mithro-old, 2015-06-01 03:34:13]

On 2015/05/29 14:00:58, Sami wrote:
> Thanks, I've added a few comments.
> 
>
https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
> File components/scheduler/child/idle_helper_unittest.cc (right):
> 
>
https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
> components/scheduler/child/idle_helper_unittest.cc:142:
> clock_->Advance(base::TimeDelta::FromInternalValue(5000));
> Do any of the tests fail if we start the clock at zero? I'm not sure we
actually
> need to start at a non-zero time here.
> 
>
https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
> components/scheduler/child/idle_helper_unittest.cc:214:
> base::SimpleTestTickClock* clock_;
> Why isn't this a scoped_ptr? Looks like we're leaking the allocation now.
> 
>
https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
> File components/scheduler/child/scheduler_helper_unittest.cc (right):
> 
>
https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
> components/scheduler/child/scheduler_helper_unittest.cc:47:
> clock_->Advance(base::TimeDelta::FromInternalValue(5000));
> Same question about the starting value as before.
> 
>
https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
> components/scheduler/child/scheduler_helper_unittest.cc:90:
> base::SimpleTestTickClock* clock_;
> scoped_ptr here too.
> 
>
https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
> File components/scheduler/child/task_queue_manager_unittest.cc (right):
> 
>
https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
> components/scheduler/child/task_queue_manager_unittest.cc:250:
> TEST_F(TaskQueueManagerTest, NowNotCalledWhenThereAreNoDelayedTasks) {
> On 2015/05/29 08:26:48, mithro wrote:
> > Sami, this seems like a very weird test - any idea on what the history here
> is?
> 
> The idea is to avoid reading the current time anywhere unless we have tasks
that
> require knowing it (i.e., delayed tasks).
> 
> I think it might be cleaner to rewrite it to use a SimpleTestTickClock that
> fails an assert if anyone calls into it.
> 
>
https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
> components/scheduler/child/task_queue_manager_unittest.cc:1050:
> clock->Advance(base::TimeDelta::FromInternalValue(10000));
> Where did the value 10000 come from?
> 
>
https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
> File components/scheduler/child/worker_scheduler_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
> components/scheduler/child/worker_scheduler_impl_unittest.cc:103:
> clock_->Advance(base::TimeDelta::FromInternalValue(5000));
> Ditto about the starting time value.
> 
>
https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
> components/scheduler/child/worker_scheduler_impl_unittest.cc:192:
> base::SimpleTestTickClock* clock_;
> Ditto about scoped_ptr.
> 
>
https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/re...
> File components/scheduler/renderer/deadline_task_runner_unittest.cc (right):
> 
>
https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/re...
> components/scheduler/renderer/deadline_task_runner_unittest.cc:21:
> clock_->Advance(base::TimeDelta::FromInternalValue(5000));
> Ditto about base time.
> 
>
https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/re...
> File components/scheduler/renderer/renderer_scheduler_impl_unittest.cc
(right):
> 
>
https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/re...
> components/scheduler/renderer/renderer_scheduler_impl_unittest.cc:127:
> clock_->Advance(base::TimeDelta::FromInternalValue(5000));
> Ditto about base time.
> 
>
https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/re...
> components/scheduler/renderer/renderer_scheduler_impl_unittest.cc:156:
> clock_->Advance(base::TimeDelta::FromInternalValue(5000));
> Ditto about base time.
> 
>
https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/re...
> components/scheduler/renderer/renderer_scheduler_impl_unittest.cc:296:
> base::SimpleTestTickClock* clock_;
> Ditto about scoped_ptr.

It doesn't look like this patch compiles, please see the try bots;
---------------------------------------------------------

e:\b\build\slave\win\build\src\cc\surfaces\display_scheduler_unittest.cc(176) :
error C2039: 'Now' : is not a member of 'base::SimpleTestTickClock'
        e:\b\build\slave\win\build\src\base\test\simple_test_tick_clock.h(18) :
see declaration of 'base::SimpleTestTickClock'
e:\b\build\slave\win\build\src\cc\surfaces\display_scheduler_unittest.cc(176) :
error C2660: 'testing::internal::CmpHelperLT' : function does not take 3
arguments
e:\b\build\slave\win\build\src\cc\surfaces\display_scheduler_unittest.cc(176) :
error C2512: 'testing::AssertionResult' : no appropriate default constructor
available
e:\b\build\slave\win\build\src\cc\surfaces\display_scheduler_unittest.cc(179) :
error C2039: 'Now' : is not a member of 'base::SimpleTestTickClock'
        e:\b\build\slave\win\build\src\base\test\simple_test_tick_clock.h(18) :
see declaration of 'base::SimpleTestTickClock'
e:\b\build\slave\win\build\src\cc\surfaces\display_scheduler_unittest.cc(179) :
error C2660: 'testing::internal::CmpHelperGE' : function does not take 3
arguments
e:\b\build\slave\win\build\src\cc\surfaces\display_scheduler_unittest.cc(179) :
error C2512: 'testing::AssertionResult' : no appropriate default constructor
available
ninja: build stopped: subcommand failed.

---------------------------------------------------------

[mithro-old, 2015-06-01 03:37:37]

Hi,

Answers for Sami about a couple of his questions.

Tim 'mithro' Ansell

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
File components/scheduler/child/idle_helper_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/idle_helper_unittest.cc:142:
clock_->Advance(base::TimeDelta::FromInternalValue(5000));
On 2015/05/29 14:00:58, Sami wrote:
> Do any of the tests fail if we start the clock at zero? I'm not sure we
actually
> need to start at a non-zero time here.

Actually yes, a zero time is considered "null" and there are a bunch of checks
that the time given is not null.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
File components/scheduler/child/task_queue_manager_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/task_queue_manager_unittest.cc:250:
TEST_F(TaskQueueManagerTest, NowNotCalledWhenThereAreNoDelayedTasks) {
On 2015/05/29 14:00:58, Sami wrote:
> On 2015/05/29 08:26:48, mithro wrote:
> > Sami, this seems like a very weird test - any idea on what the history here
> is?
> 
> The idea is to avoid reading the current time anywhere unless we have tasks
that
> require knowing it (i.e., delayed tasks).
> 
> I think it might be cleaner to rewrite it to use a SimpleTestTickClock that
> fails an assert if anyone calls into it.

Yeah, maybe we should log a cleanup bug about changing this test to do that
instead?

[Ankur Verma, 2015-06-01 05:30:44]

Thanks Tim & Sami for the review.
I've responded to your queries with comments.

@Tim,
Yes i've checked the try bot logs. It broke on a patch which had landed after my
patch was submitted for review. I had made sure that nothing broke before i had
submitted, but unfortunate. Hence i shall rebase and re-submit my patch after
performing changes for review comments.

Best regards.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
File components/scheduler/child/idle_helper_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/idle_helper_unittest.cc:142:
clock_->Advance(base::TimeDelta::FromInternalValue(5000));
On 2015/05/29 14:00:58, Sami wrote:
> Do any of the tests fail if we start the clock at zero? I'm not sure we
actually
> need to start at a non-zero time here.

A quick check here with zero, crashes IdleHelperTest.TestRepostingIdleTask in
TaskQueue::ScheduleDelayedWorkLocked() since LazyNow would contain
TaskQueueManager as null for 0 TimeTicks.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/idle_helper_unittest.cc:214:
base::SimpleTestTickClock* clock_;
On 2015/05/29 14:00:57, Sami wrote:
> Why isn't this a scoped_ptr? Looks like we're leaking the allocation now.
I will get on to this and update patch accordingly.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
File components/scheduler/child/task_queue_manager_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/task_queue_manager_unittest.cc:250:
TEST_F(TaskQueueManagerTest, NowNotCalledWhenThereAreNoDelayedTasks) {
On 2015/06/01 03:37:37, mithro wrote:
> Yeah, maybe we should log a cleanup bug about changing this test to do that
> instead?
Should i exclude this cleanup request for this particular patch-set?

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/task_queue_manager_unittest.cc:1050:
clock->Advance(base::TimeDelta::FromInternalValue(10000));
On 2015/05/29 14:00:58, Sami wrote:
> Where did the value 10000 come from?
cc::TestNowSource::Create() had internally started the clock 10000 and hence
replayed it here.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
File components/scheduler/child/test_time_source.h (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/test_time_source.h:25: mutable int num_now_calls_;
On 2015/05/29 08:26:48, mithro wrote:
> The mutable keyword doesn't do what you probably think it does. However, that
> looks like it was a bug in the original code too.

Shall remove this if its not really mutable.

[mithro-old, 2015-06-01 07:13:16]

On 2015/06/01 05:30:44, Ankur Verma wrote:
> Thanks Tim & Sami for the review.
> I've responded to your queries with comments.
> 
> @Tim,
> Yes i've checked the try bot logs. It broke on a patch which had landed after
my
> patch was submitted for review. I had made sure that nothing broke before i
had
> submitted, but unfortunate. Hence i shall rebase and re-submit my patch after
> performing changes for review comments.

Just some terminology, we generally say "uploaded for review". The word
submitting is generally reserved for sending into the commit queue for being
committed.

Please also upload a rebase separately to any changes so we can clearly see how
this patch has evolved over time (otherwise we'll see the rebase changes mixed
in with the review changes).

[mithro-old, 2015-06-01 07:32:16]

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
File components/scheduler/child/idle_helper_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/idle_helper_unittest.cc:142:
clock_->Advance(base::TimeDelta::FromInternalValue(5000));
On 2015/06/01 05:30:44, Ankur Verma wrote:
> On 2015/05/29 14:00:58, Sami wrote:
> > Do any of the tests fail if we start the clock at zero? I'm not sure we
> actually
> > need to start at a non-zero time here.
> 
> A quick check here with zero, crashes IdleHelperTest.TestRepostingIdleTask in
> TaskQueue::ScheduleDelayedWorkLocked() since LazyNow would contain
> TaskQueueManager as null for 0 TimeTicks.
> 

I wonder is the default SimpleTestTickClock should actually start a non-zero for
this very reason?

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
File components/scheduler/child/task_queue_manager_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/task_queue_manager_unittest.cc:250:
TEST_F(TaskQueueManagerTest, NowNotCalledWhenThereAreNoDelayedTasks) {
On 2015/06/01 05:30:44, Ankur Verma wrote:
> On 2015/06/01 03:37:37, mithro wrote:
> > Yeah, maybe we should log a cleanup bug about changing this test to do that
> > instead?
> Should i exclude this cleanup request for this particular patch-set?

I logged a bug about this issue at
https://code.google.com/p/chromium/issues/detail?id=494887

You may need to do the cleanup mentioned in that bug as moving mutable into
components/scheduler/child/test_time_source.cc is probably not going to be
allowed.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
File components/scheduler/child/test_time_source.h (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/test_time_source.h:25: mutable int num_now_calls_;
On 2015/06/01 05:30:44, Ankur Verma wrote:
> On 2015/05/29 08:26:48, mithro wrote:
> > The mutable keyword doesn't do what you probably think it does. However,
that
> > looks like it was a bug in the original code too.
> 
> Shall remove this if its not really mutable.

I don't think you should add the NumNowCalls() functionality to this clock here.
I think that means you will need to do the refactoring mentioned in
https://code.google.com/p/chromium/issues/detail?id=494887

It also looks like this TimeSource interface is a duplicate of TickClock? I've
logged a bug about that too
https://code.google.com/p/chromium/issues/detail?id=494892

[Ankur Verma, 2015-06-01 09:21:41]

@Tim,
Pls refer to my comments.
I suppose i'll first upload a patch for review with review comments incorporated
and then do a rebase if it looks good to you.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
File components/scheduler/child/idle_helper_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/idle_helper_unittest.cc:142:
clock_->Advance(base::TimeDelta::FromInternalValue(5000));
On 2015/06/01 07:32:15, mithro wrote:
> On 2015/06/01 05:30:44, Ankur Verma wrote:
> > A quick check here with zero, crashes IdleHelperTest.TestRepostingIdleTask
in
> > TaskQueue::ScheduleDelayedWorkLocked() since LazyNow would contain
> > TaskQueueManager as null for 0 TimeTicks.
> > 
> 
> I wonder is the default SimpleTestTickClock should actually start a non-zero
for
> this very reason?

I'd wait for a response on
https://code.google.com/p/chromium/issues/detail?id=494894 for this.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
File components/scheduler/child/task_queue_manager_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/task_queue_manager_unittest.cc:250:
TEST_F(TaskQueueManagerTest, NowNotCalledWhenThereAreNoDelayedTasks) {
On 2015/06/01 07:32:15, mithro wrote:
> On 2015/06/01 05:30:44, Ankur Verma wrote:
> > On 2015/06/01 03:37:37, mithro wrote:
> > > Yeah, maybe we should log a cleanup bug about changing this test to do
that
> > > instead?
> > Should i exclude this cleanup request for this particular patch-set?
> 
> I logged a bug about this issue at
> https://code.google.com/p/chromium/issues/detail?id=494887
> 
> You may need to do the cleanup mentioned in that bug as moving mutable into
> components/scheduler/child/test_time_source.cc is probably not going to be
> allowed.

Yes i understand. I am thinking of renaming the test as per your suggestion,
hooking up a bool always_fail_time_source in TestTimeSource (default False) and
assert in TestTimeSource::Now() as per Sami's suggestion. Remove num_now_calls
stuff. Does that make sense? I wonder what to EXPECT_EQ at the end of test
though.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
File components/scheduler/child/test_time_source.h (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/test_time_source.h:25: mutable int num_now_calls_;
On 2015/06/01 07:32:15, mithro wrote:
> On 2015/06/01 05:30:44, Ankur Verma wrote:
> > On 2015/05/29 08:26:48, mithro wrote:
> > > The mutable keyword doesn't do what you probably think it does. However,
> that
> > > looks like it was a bug in the original code too.
> > 
> > Shall remove this if its not really mutable.
> 
> I don't think you should add the NumNowCalls() functionality to this clock
here.
> I think that means you will need to do the refactoring mentioned in
> https://code.google.com/p/chromium/issues/detail?id=494887

Right. I've put in my thoughts in relation to this in an earlier comment. Pls
let me know if you think otherwise.

> 
> It also looks like this TimeSource interface is a duplicate of TickClock? I've
> logged a bug about that too
> https://code.google.com/p/chromium/issues/detail?id=494892

Yes it seems so. History here: https://codereview.chromium.org/1098033002

[mithro-old, 2015-06-01 11:31:20]

> I suppose i'll first upload a patch for review with review comments
incorporated
> and then do a rebase if it looks good to you.

It is fine that once you do fixes you then upload a rebase straight afterwards.
The important part is the rebase upload should *only* have changes related to
the rebase of the patch in it.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
File components/scheduler/child/idle_helper_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/idle_helper_unittest.cc:142:
clock_->Advance(base::TimeDelta::FromInternalValue(5000));
I don't think you need to wait for that bug to be fixed. 

If we decide that issue should be fixed after this patch lands then we can
remove the clock_->Advance() from these tests.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
File components/scheduler/child/task_queue_manager_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/task_queue_manager_unittest.cc:250:
TEST_F(TaskQueueManagerTest, NowNotCalledWhenThereAreNoDelayedTasks) {
On 2015/06/01 09:21:40, Ankur Verma wrote:
> On 2015/06/01 07:32:15, mithro wrote:
> > On 2015/06/01 05:30:44, Ankur Verma wrote:
> > > On 2015/06/01 03:37:37, mithro wrote:
> > > > Yeah, maybe we should log a cleanup bug about changing this test to do
> that
> > > > instead?
> > > Should i exclude this cleanup request for this particular patch-set?
> > 
> > I logged a bug about this issue at
> > https://code.google.com/p/chromium/issues/detail?id=494887
> > 
> > You may need to do the cleanup mentioned in that bug as moving mutable into
> > components/scheduler/child/test_time_source.cc is probably not going to be
> > allowed.
> 
> Yes i understand. I am thinking of renaming the test as per your suggestion,
> hooking up a bool always_fail_time_source in TestTimeSource (default False)
and
> assert in TestTimeSource::Now() as per Sami's suggestion. Remove num_now_calls
> stuff. Does that make sense? I wonder what to EXPECT_EQ at the end of test
> though.

I don't think you want booleans or anything inside the TestTimeSource, I think
you want a totally separate TestTimeSource for this test which has something
like something like the following inside the Now() method;
-------------------------
EXPECT_FALSE(true) << "Now() was called!";
return TimeSource::Now();
-------------------------

[Ankur Verma, 2015-06-03 14:56:11]

Hello Tim & Sami,
I've incorporated the review comment changes. Scoped leaking clocks and
refactored the num_now_calls_ functionality.
Please have a look.

PS: I still need to rebase this patch.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
File components/scheduler/child/idle_helper_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/idle_helper_unittest.cc:142:
clock_->Advance(base::TimeDelta::FromInternalValue(5000));
On 2015/06/01 11:31:20, mithro wrote:
> I don't think you need to wait for that bug to be fixed. 
> 
> If we decide that issue should be fixed after this patch lands then we can
> remove the clock_->Advance() from these tests.

Acknowledged.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/idle_helper_unittest.cc:214:
base::SimpleTestTickClock* clock_;
On 2015/05/29 14:00:57, Sami wrote:
> Why isn't this a scoped_ptr? Looks like we're leaking the allocation now.

Done.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
File components/scheduler/child/scheduler_helper_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/scheduler_helper_unittest.cc:47:
clock_->Advance(base::TimeDelta::FromInternalValue(5000));
On 2015/05/29 14:00:58, Sami wrote:
> Same question about the starting value as before.
Have'nt changed starting values yet as per mithro's explaination.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/scheduler_helper_unittest.cc:90:
base::SimpleTestTickClock* clock_;
On 2015/05/29 14:00:58, Sami wrote:
> scoped_ptr here too.

Done.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
File components/scheduler/child/task_queue_manager_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/task_queue_manager_unittest.cc:250:
TEST_F(TaskQueueManagerTest, NowNotCalledWhenThereAreNoDelayedTasks) {
On 2015/06/01 11:31:20, mithro wrote:
> On 2015/06/01 09:21:40, Ankur Verma wrote:
> > On 2015/06/01 07:32:15, mithro wrote:
> > > On 2015/06/01 05:30:44, Ankur Verma wrote:
> > > > On 2015/06/01 03:37:37, mithro wrote:
> > > > > Yeah, maybe we should log a cleanup bug about changing this test to do
> > that
> > > > > instead?
> > > > Should i exclude this cleanup request for this particular patch-set?
> > > 
> > > I logged a bug about this issue at
> > > https://code.google.com/p/chromium/issues/detail?id=494887
> > > 
> > > You may need to do the cleanup mentioned in that bug as moving mutable
into
> > > components/scheduler/child/test_time_source.cc is probably not going to be
> > > allowed.
> > 
> > Yes i understand. I am thinking of renaming the test as per your suggestion,
> > hooking up a bool always_fail_time_source in TestTimeSource (default False)
> and
> > assert in TestTimeSource::Now() as per Sami's suggestion. Remove
num_now_calls
> > stuff. Does that make sense? I wonder what to EXPECT_EQ at the end of test
> > though.
> 
> I don't think you want booleans or anything inside the TestTimeSource, I think
> you want a totally separate TestTimeSource for this test which has something
> like something like the following inside the Now() method;
> -------------------------
> EXPECT_FALSE(true) << "Now() was called!";
> return TimeSource::Now();
> -------------------------

Done.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
File components/scheduler/child/test_time_source.h (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/test_time_source.h:25: mutable int num_now_calls_;
On 2015/06/01 07:32:15, mithro wrote:
> On 2015/06/01 05:30:44, Ankur Verma wrote:
> > On 2015/05/29 08:26:48, mithro wrote:
> > > The mutable keyword doesn't do what you probably think it does. However,
> that
> > > looks like it was a bug in the original code too.
> > 
> > Shall remove this if its not really mutable.
> 
> I don't think you should add the NumNowCalls() functionality to this clock
here.
> I think that means you will need to do the refactoring mentioned in
> https://code.google.com/p/chromium/issues/detail?id=494887
> 
> It also looks like this TimeSource interface is a duplicate of TickClock? I've
> logged a bug about that too
> https://code.google.com/p/chromium/issues/detail?id=494892

Done.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
File components/scheduler/child/worker_scheduler_impl_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/worker_scheduler_impl_unittest.cc:103:
clock_->Advance(base::TimeDelta::FromInternalValue(5000));
On 2015/05/29 14:00:58, Sami wrote:
> Ditto about the starting time value.
Have'nt changed starting values yet as per mithro's explaination.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/ch...
components/scheduler/child/worker_scheduler_impl_unittest.cc:192:
base::SimpleTestTickClock* clock_;
On 2015/05/29 14:00:58, Sami wrote:
> Ditto about scoped_ptr.

Done.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/re...
File components/scheduler/renderer/deadline_task_runner_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/re...
components/scheduler/renderer/deadline_task_runner_unittest.cc:21:
clock_->Advance(base::TimeDelta::FromInternalValue(5000));
On 2015/05/29 14:00:58, Sami wrote:
> Ditto about base time.
Have'nt changed starting values yet as per mithro's explaination.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/re...
File components/scheduler/renderer/renderer_scheduler_impl_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl_unittest.cc:127:
clock_->Advance(base::TimeDelta::FromInternalValue(5000));
On 2015/05/29 14:00:58, Sami wrote:
> Ditto about base time.
Have'nt changed starting values yet as per mithro's explaination.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl_unittest.cc:156:
clock_->Advance(base::TimeDelta::FromInternalValue(5000));
On 2015/05/29 14:00:58, Sami wrote:
> Ditto about base time.
Have'nt changed starting values yet as per mithro's explaination.

https://codereview.chromium.org/1132753008/diff/60001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl_unittest.cc:296:
base::SimpleTestTickClock* clock_;
On 2015/05/29 14:00:58, Sami wrote:
> Ditto about scoped_ptr.

Done.

[mithro-old, 2015-06-04 07:40:43]

Please do a rebase ASAP so we can make sure this runs on the trybots.

Generally looks good with some minor issues, please get Sami's review too
(you'll need his approval for components/scheduler anyway).

[mithro-old, 2015-06-04 07:41:10]

Please do a rebase ASAP so we can make sure this runs on the trybots.

Generally looks good with some minor issues, please get Sami's review too
(you'll need his approval for components/scheduler anyway).

https://codereview.chromium.org/1132753008/diff/80001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/80001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source_unittest.cc:511: now_src_ = make_scoped_ptr(new
base::SimpleTestTickClock());
I think this should be?

 now_src_.reset(new base::SimpleTestTickClock());

https://codereview.chromium.org/1132753008/diff/80001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/80001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_unittest.cc:259: task_runner_ = new
OrderedSimpleTaskRunner(now_src_.get(), true);
task_runner_ should be created in the initializer list.

https://codereview.chromium.org/1132753008/diff/80001/cc/surfaces/display_sch...
File cc/surfaces/display_scheduler_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/80001/cc/surfaces/display_sch...
cc/surfaces/display_scheduler_unittest.cc:73: now_src_ = make_scoped_ptr(new
base::SimpleTestTickClock());
This should be done in the initializer list.

https://codereview.chromium.org/1132753008/diff/80001/cc/test/ordered_simple_...
File cc/test/ordered_simple_task_runner_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/80001/cc/test/ordered_simple_...
cc/test/ordered_simple_task_runner_unittest.cc:59: task_runner_ = new
OrderedSimpleTaskRunner(now_src_.get(), true);
This should probably be done in the initializer list.

https://codereview.chromium.org/1132753008/diff/80001/components/scheduler/ch...
File components/scheduler/child/test_always_fail_time_source.cc (right):

https://codereview.chromium.org/1132753008/diff/80001/components/scheduler/ch...
components/scheduler/child/test_always_fail_time_source.cc:20: return
time_source_->NowTicks();
I'm not sure returning a real value here is a good idea? Maybe we should just
always return a base::TimeTicks() value?

[Sami, 2015-06-04 14:47:07]

+1 for putting this through the trybots.

https://codereview.chromium.org/1132753008/diff/80001/components/components_t...
File components/components_tests.gyp (right):

https://codereview.chromium.org/1132753008/diff/80001/components/components_t...
components/components_tests.gyp:475:
'scheduler/child/test_always_fail_time_source.cc',
Could you please put this in scheduler/test instead?

https://codereview.chromium.org/1132753008/diff/80001/components/scheduler/ch...
File components/scheduler/child/test_always_fail_time_source.cc (right):

https://codereview.chromium.org/1132753008/diff/80001/components/scheduler/ch...
components/scheduler/child/test_always_fail_time_source.cc:19:
EXPECT_FALSE(true) << "Now() was called!";
FAIL() << "..."

https://codereview.chromium.org/1132753008/diff/80001/components/scheduler/ch...
components/scheduler/child/test_always_fail_time_source.cc:20: return
time_source_->NowTicks();
On 2015/06/04 07:41:10, mithro wrote:
> I'm not sure returning a real value here is a good idea? Maybe we should just
> always return a base::TimeTicks() value?

+1

https://codereview.chromium.org/1132753008/diff/80001/components/scheduler/ch...
File components/scheduler/child/test_time_source.h (right):

https://codereview.chromium.org/1132753008/diff/80001/components/scheduler/ch...
components/scheduler/child/test_time_source.h:22: base::SimpleTestTickClock*
time_source_;
Can you split this change out since you had another patch for it?

[Ankur Verma, 2015-06-04 15:25:08]

Thanks Tim & Sami for reviewing again.
I have just uploaded a rebase of this patch. Try bots can be run now.
As for the latest review comments, i have gone through all and will be updating
the patch ASAP.

@Sami
I just need clarification for your comment mentioned below.

https://codereview.chromium.org/1132753008/diff/80001/components/scheduler/ch...
File components/scheduler/child/test_time_source.h (right):

https://codereview.chromium.org/1132753008/diff/80001/components/scheduler/ch...
components/scheduler/child/test_time_source.h:22: base::SimpleTestTickClock*
time_source_;
On 2015/06/04 14:47:07, Sami wrote:
> Can you split this change out since you had another patch for it?

I could not understand completely (this being my first chromium bug).

This change bring us back to the original patch.
1. First, i had deleted test_now_source.* files and converted this to use
SimpleTestTickClock.
2. Then, i had added the num_now_calls_ functionality to it.
3. Then, i removed the num_now_calls_functionality from it. Hence back to 1.

Would you like me to restore test_now_source.* files and remove this change. And
then remove test_now_source.* in a separate patch?

[Ankur Verma, 2015-06-05 14:39:17]

Tim & Sami, PTAL.

I've updated all review comment changes except one mentioned below.

@Tim,
Could you please put this through trybots?

https://codereview.chromium.org/1132753008/diff/80001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/80001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source_unittest.cc:511: now_src_ = make_scoped_ptr(new
base::SimpleTestTickClock());
On 2015/06/04 07:41:10, mithro wrote:
> I think this should be?
> 
>  now_src_.reset(new base::SimpleTestTickClock());

Done.

https://codereview.chromium.org/1132753008/diff/80001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/80001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_unittest.cc:259: task_runner_ = new
OrderedSimpleTaskRunner(now_src_.get(), true);
On 2015/06/04 07:41:10, mithro wrote:
> task_runner_ should be created in the initializer list.

Done.

https://codereview.chromium.org/1132753008/diff/80001/cc/surfaces/display_sch...
File cc/surfaces/display_scheduler_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/80001/cc/surfaces/display_sch...
cc/surfaces/display_scheduler_unittest.cc:73: now_src_ = make_scoped_ptr(new
base::SimpleTestTickClock());
On 2015/06/04 07:41:10, mithro wrote:
> This should be done in the initializer list.

Done.

https://codereview.chromium.org/1132753008/diff/80001/cc/test/ordered_simple_...
File cc/test/ordered_simple_task_runner_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/80001/cc/test/ordered_simple_...
cc/test/ordered_simple_task_runner_unittest.cc:59: task_runner_ = new
OrderedSimpleTaskRunner(now_src_.get(), true);
On 2015/06/04 07:41:10, mithro wrote:
> This should probably be done in the initializer list.

Done.

https://codereview.chromium.org/1132753008/diff/80001/components/components_t...
File components/components_tests.gyp (right):

https://codereview.chromium.org/1132753008/diff/80001/components/components_t...
components/components_tests.gyp:475:
'scheduler/child/test_always_fail_time_source.cc',
On 2015/06/04 14:47:06, Sami wrote:
> Could you please put this in scheduler/test instead?

Done.

https://codereview.chromium.org/1132753008/diff/80001/components/scheduler/ch...
File components/scheduler/child/test_always_fail_time_source.cc (right):

https://codereview.chromium.org/1132753008/diff/80001/components/scheduler/ch...
components/scheduler/child/test_always_fail_time_source.cc:19:
EXPECT_FALSE(true) << "Now() was called!";
On 2015/06/04 14:47:07, Sami wrote:
> FAIL() << "..."

FAIL() implicates a void return. Since we're returning base::Timeticks(), i
have'nt changed this.

https://codereview.chromium.org/1132753008/diff/80001/components/scheduler/ch...
components/scheduler/child/test_always_fail_time_source.cc:20: return
time_source_->NowTicks();
On 2015/06/04 07:41:10, mithro wrote:
> I'm not sure returning a real value here is a good idea? Maybe we should just
> always return a base::TimeTicks() value?

Done.

https://codereview.chromium.org/1132753008/diff/80001/components/scheduler/ch...
File components/scheduler/child/test_time_source.h (right):

https://codereview.chromium.org/1132753008/diff/80001/components/scheduler/ch...
components/scheduler/child/test_time_source.h:22: base::SimpleTestTickClock*
time_source_;
On 2015/06/04 14:47:07, Sami wrote:
> Can you split this change out since you had another patch for it?

On rebasing for patch set 8, i realised that
https://codereview.chromium.org/1163143002
performed a separate cleanup on this.

I've now replaced cc::TestNowSource with base::SimpleTestTickClock on top of
that.

[Sami, 2015-06-05 17:11:07]

Thanks, I've added a few comments.

https://codereview.chromium.org/1132753008/diff/140001/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/1132753008/diff/140001/AUTHORS#newcode608
AUTHORS:608: Ankur Verma <ankur1.verma@samsung.com>
Alphabetic order please.

https://codereview.chromium.org/1132753008/diff/140001/cc/test/ordered_simple...
File cc/test/ordered_simple_task_runner.cc (right):

https://codereview.chromium.org/1132753008/diff/140001/cc/test/ordered_simple...
cc/test/ordered_simple_task_runner.cc:96: const base::TimeTicks
OrderedSimpleTaskRunner::kAbsoluteMaxNow =
No non-POD global static initializers please. You probably want to make this a
64-bit integer or add a helper function for computing this value when needed.

https://codereview.chromium.org/1132753008/diff/140001/cc/test/ordered_simple...
File cc/test/ordered_simple_task_runner.h (right):

https://codereview.chromium.org/1132753008/diff/140001/cc/test/ordered_simple...
cc/test/ordered_simple_task_runner.h:142: base::SimpleTestTickClock* now_src_;
Who owns this object? One of the constructors takes it from the caller and the
other allocates a new one but never frees it. I think we only want one of these
options.

https://codereview.chromium.org/1132753008/diff/140001/cc/test/scheduler_test...
File cc/test/scheduler_test_common.h (right):

https://codereview.chromium.org/1132753008/diff/140001/cc/test/scheduler_test...
cc/test/scheduler_test_common.h:73: base::SimpleTestTickClock* now_src_;
In general it would be good to document ownership of raw pointers (e.g., "// Not
owned.")

https://codereview.chromium.org/1132753008/diff/140001/components/scheduler/t...
File components/scheduler/test/test_always_fail_time_source.cc (right):

https://codereview.chromium.org/1132753008/diff/140001/components/scheduler/t...
components/scheduler/test/test_always_fail_time_source.cc:19: EXPECT_FALSE(true)
<< "NowTicks() was called!";
Ah, good point about FAIL(). Could we use ADD_FAILURE() instead?

https://codereview.chromium.org/1132753008/diff/140001/components/scheduler/t...
File components/scheduler/test/test_always_fail_time_source.h (right):

https://codereview.chromium.org/1132753008/diff/140001/components/scheduler/t...
components/scheduler/test/test_always_fail_time_source.h:5: #ifndef
COMPONENTS_SCHEDULER_TEST_ALWAYS_FAIL_TIME_SOURCE_H_
Missing one TEST here.

https://codereview.chromium.org/1132753008/diff/140001/components/scheduler/t...
components/scheduler/test/test_always_fail_time_source.h:21:
base::SimpleTestTickClock* time_source_;
We don't need this member for anything, right?

https://codereview.chromium.org/1132753008/diff/140001/components/scheduler/t...
components/scheduler/test/test_always_fail_time_source.h:28: #endif  //
COMPONENTS_SCHEDULER_TEST_ALWAYS_FAIL_TIME_SOURCE_H_
Ditto about TEST.

[Ankur Verma, 2015-06-08 07:11:35]

Hello Sami & Tim,
Have updated all review changes. PTAL.
Kindly trigger trybots as well.

https://codereview.chromium.org/1132753008/diff/140001/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/1132753008/diff/140001/AUTHORS#newcode608
AUTHORS:608: Ankur Verma <ankur1.verma@samsung.com>
On 2015/06/05 17:11:07, Sami wrote:
> Alphabetic order please.

Done.

https://codereview.chromium.org/1132753008/diff/140001/cc/test/ordered_simple...
File cc/test/ordered_simple_task_runner.cc (right):

https://codereview.chromium.org/1132753008/diff/140001/cc/test/ordered_simple...
cc/test/ordered_simple_task_runner.cc:96: const base::TimeTicks
OrderedSimpleTaskRunner::kAbsoluteMaxNow =
On 2015/06/05 17:11:07, Sami wrote:
> No non-POD global static initializers please. You probably want to make this a
> 64-bit integer or add a helper function for computing this value when needed.

Done. Changed to helper function.

https://codereview.chromium.org/1132753008/diff/140001/cc/test/ordered_simple...
File cc/test/ordered_simple_task_runner.h (right):

https://codereview.chromium.org/1132753008/diff/140001/cc/test/ordered_simple...
cc/test/ordered_simple_task_runner.h:142: base::SimpleTestTickClock* now_src_;
On 2015/06/05 17:11:07, Sami wrote:
> Who owns this object? One of the constructors takes it from the caller and the
> other allocates a new one but never frees it. I think we only want one of
these
> options.

Done. Removed the ctor which allocated but never freed. Updated caller with a
scoped clock (SurfaceDisplayOutputSurfaceTest())

https://codereview.chromium.org/1132753008/diff/140001/cc/test/scheduler_test...
File cc/test/scheduler_test_common.h (right):

https://codereview.chromium.org/1132753008/diff/140001/cc/test/scheduler_test...
cc/test/scheduler_test_common.h:73: base::SimpleTestTickClock* now_src_;
On 2015/06/05 17:11:07, Sami wrote:
> In general it would be good to document ownership of raw pointers (e.g., "//
Not
> owned.")

Done.

https://codereview.chromium.org/1132753008/diff/140001/components/scheduler/t...
File components/scheduler/test/test_always_fail_time_source.cc (right):

https://codereview.chromium.org/1132753008/diff/140001/components/scheduler/t...
components/scheduler/test/test_always_fail_time_source.cc:19: EXPECT_FALSE(true)
<< "NowTicks() was called!";
On 2015/06/05 17:11:07, Sami wrote:
> Ah, good point about FAIL(). Could we use ADD_FAILURE() instead?

Done.

https://codereview.chromium.org/1132753008/diff/140001/components/scheduler/t...
File components/scheduler/test/test_always_fail_time_source.h (right):

https://codereview.chromium.org/1132753008/diff/140001/components/scheduler/t...
components/scheduler/test/test_always_fail_time_source.h:5: #ifndef
COMPONENTS_SCHEDULER_TEST_ALWAYS_FAIL_TIME_SOURCE_H_
On 2015/06/05 17:11:07, Sami wrote:
> Missing one TEST here.

Done.

https://codereview.chromium.org/1132753008/diff/140001/components/scheduler/t...
components/scheduler/test/test_always_fail_time_source.h:21:
base::SimpleTestTickClock* time_source_;
On 2015/06/05 17:11:07, Sami wrote:
> We don't need this member for anything, right?

Done.

https://codereview.chromium.org/1132753008/diff/140001/components/scheduler/t...
components/scheduler/test/test_always_fail_time_source.h:28: #endif  //
COMPONENTS_SCHEDULER_TEST_ALWAYS_FAIL_TIME_SOURCE_H_
On 2015/06/05 17:11:07, Sami wrote:
> Ditto about TEST.

Done.

[Sami, 2015-06-08 13:59:02]

Thanks for the fixes. Just a couple of comments about time units.

https://codereview.chromium.org/1132753008/diff/200001/cc/test/ordered_simple...
File cc/test/ordered_simple_task_runner.cc (right):

https://codereview.chromium.org/1132753008/diff/200001/cc/test/ordered_simple...
cc/test/ordered_simple_task_runner.cc:283:
state->SetInteger("now_in_microseconds",
The internal value is not really defined to be in any known units, so it would
be better to do something like this:

state->SetDouble("now_in_ms", now_src->NowTicks().InMillisecondsF());

(Looks like we were already doing that before your patch too.)

https://codereview.chromium.org/1132753008/diff/200001/cc/test/ordered_simple...
File cc/test/ordered_simple_task_runner_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/200001/cc/test/ordered_simple...
cc/test/ordered_simple_task_runner_unittest.cc:117:
now_src_->NowTicks().ToInternalValue() /
This could just be .InMilliseconds()

https://codereview.chromium.org/1132753008/diff/200001/components/scheduler/c...
File components/scheduler/child/worker_scheduler_impl_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/200001/components/scheduler/c...
components/scheduler/child/worker_scheduler_impl_unittest.cc:101:
clock_->Advance(base::TimeDelta::FromInternalValue(5000));
Could you change all the places you're adding this to use
base::TimeDelta::FromMicroseconds() instead?

[Ankur Verma, 2015-06-09 07:53:48]

@Sami,
I have a query on previous review comments. Please clarify.

https://codereview.chromium.org/1132753008/diff/200001/cc/test/ordered_simple...
File cc/test/ordered_simple_task_runner.cc (right):

https://codereview.chromium.org/1132753008/diff/200001/cc/test/ordered_simple...
cc/test/ordered_simple_task_runner.cc:283:
state->SetInteger("now_in_microseconds",
On 2015/06/08 13:59:02, Sami wrote:
> The internal value is not really defined to be in any known units, so it would
> be better to do something like this:
> 
> state->SetDouble("now_in_ms", now_src->NowTicks().InMillisecondsF());
> 
> (Looks like we were already doing that before your patch too.)

This was replayed from TestNowSource.
base::TimeTicks does not define InMillisecondsF(). Unless we'd do
base::TimeDelta::FromMicroseconds(now_src_->NowTicks().ToInternalValue()).InMilliseconds()
which sounds effectively the same thing except explicitly converting to
milliseconds. Is that what is intended?

https://codereview.chromium.org/1132753008/diff/200001/cc/test/ordered_simple...
File cc/test/ordered_simple_task_runner_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/200001/cc/test/ordered_simple...
cc/test/ordered_simple_task_runner_unittest.cc:117:
now_src_->NowTicks().ToInternalValue() /
On 2015/06/08 13:59:02, Sami wrote:
> This could just be .InMilliseconds()

Same argument as before about explicitly constituting a TimeDelta and converting
to milliseconds.

https://codereview.chromium.org/1132753008/diff/200001/components/scheduler/c...
File components/scheduler/child/worker_scheduler_impl_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/200001/components/scheduler/c...
components/scheduler/child/worker_scheduler_impl_unittest.cc:101:
clock_->Advance(base::TimeDelta::FromInternalValue(5000));
On 2015/06/08 13:59:02, Sami wrote:
> Could you change all the places you're adding this to use
> base::TimeDelta::FromMicroseconds() instead?

Acknowledged.

[Sami, 2015-06-09 10:16:02]

https://codereview.chromium.org/1132753008/diff/200001/cc/test/ordered_simple...
File cc/test/ordered_simple_task_runner.cc (right):

https://codereview.chromium.org/1132753008/diff/200001/cc/test/ordered_simple...
cc/test/ordered_simple_task_runner.cc:283:
state->SetInteger("now_in_microseconds",
On 2015/06/09 07:53:47, Ankur Verma wrote:
> On 2015/06/08 13:59:02, Sami wrote:
> > The internal value is not really defined to be in any known units, so it
would
> > be better to do something like this:
> > 
> > state->SetDouble("now_in_ms", now_src->NowTicks().InMillisecondsF());
> > 
> > (Looks like we were already doing that before your patch too.)
> 
> This was replayed from TestNowSource.
> base::TimeTicks does not define InMillisecondsF(). Unless we'd do
>
base::TimeDelta::FromMicroseconds(now_src_->NowTicks().ToInternalValue()).InMilliseconds()
> which sounds effectively the same thing except explicitly converting to
> milliseconds. Is that what is intended?

Oh right, with TimeTicks we usually do this:

    (ticks - base::TimeTicks()).InMillisecondsF()

I think that is preferable over using internal values.

https://codereview.chromium.org/1132753008/diff/200001/cc/test/ordered_simple...
File cc/test/ordered_simple_task_runner_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/200001/cc/test/ordered_simple...
cc/test/ordered_simple_task_runner_unittest.cc:117:
now_src_->NowTicks().ToInternalValue() /
On 2015/06/09 07:53:47, Ankur Verma wrote:
> On 2015/06/08 13:59:02, Sami wrote:
> > This could just be .InMilliseconds()
> 
> Same argument as before about explicitly constituting a TimeDelta and
converting
> to milliseconds.

Yep, let's do the same conversion that I mentioned above here.

[Ankur Verma, 2015-06-09 13:53:43]

Hello Sami & Tim,
Have incorporated review changes. PTAL.

https://codereview.chromium.org/1132753008/diff/200001/cc/test/ordered_simple...
File cc/test/ordered_simple_task_runner.cc (right):

https://codereview.chromium.org/1132753008/diff/200001/cc/test/ordered_simple...
cc/test/ordered_simple_task_runner.cc:283:
state->SetInteger("now_in_microseconds",
On 2015/06/09 10:16:02, Sami wrote:
> On 2015/06/09 07:53:47, Ankur Verma wrote:
> > On 2015/06/08 13:59:02, Sami wrote:
> > > The internal value is not really defined to be in any known units, so it
> would
> > > be better to do something like this:
> > > 
> > > state->SetDouble("now_in_ms", now_src->NowTicks().InMillisecondsF());
> > > 
> > > (Looks like we were already doing that before your patch too.)
> > 
> > This was replayed from TestNowSource.
> > base::TimeTicks does not define InMillisecondsF(). Unless we'd do
> >
>
base::TimeDelta::FromMicroseconds(now_src_->NowTicks().ToInternalValue()).InMilliseconds()
> > which sounds effectively the same thing except explicitly converting to
> > milliseconds. Is that what is intended?
> 
> Oh right, with TimeTicks we usually do this:
> 
>     (ticks - base::TimeTicks()).InMillisecondsF()
> 
> I think that is preferable over using internal values.

Done.

https://codereview.chromium.org/1132753008/diff/200001/cc/test/ordered_simple...
File cc/test/ordered_simple_task_runner_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/200001/cc/test/ordered_simple...
cc/test/ordered_simple_task_runner_unittest.cc:117:
now_src_->NowTicks().ToInternalValue() /
On 2015/06/09 10:16:02, Sami wrote:
> On 2015/06/09 07:53:47, Ankur Verma wrote:
> > On 2015/06/08 13:59:02, Sami wrote:
> > > This could just be .InMilliseconds()
> > 
> > Same argument as before about explicitly constituting a TimeDelta and
> converting
> > to milliseconds.
> 
> Yep, let's do the same conversion that I mentioned above here.

Done.

https://codereview.chromium.org/1132753008/diff/200001/components/scheduler/c...
File components/scheduler/child/worker_scheduler_impl_unittest.cc (right):

https://codereview.chromium.org/1132753008/diff/200001/components/scheduler/c...
components/scheduler/child/worker_scheduler_impl_unittest.cc:101:
clock_->Advance(base::TimeDelta::FromInternalValue(5000));
On 2015/06/08 13:59:02, Sami wrote:
> Could you change all the places you're adding this to use
> base::TimeDelta::FromMicroseconds() instead?

Done.

[Ankur Verma, 2015-06-11 08:34:28]

Patchset #14 (id:260001) has been deleted

[Ankur Verma, 2015-06-11 08:37:24]

Hello Tim & Sami,
Rebased again. PTAL if you have time.
Please trigger trybots too.

[Sami, 2015-06-11 13:58:42]

Thanks for your patience. lgtm.

[mithro-old, 2015-06-12 13:13:55]

The CQ bit was checked by mithro@mithis.com

[mithro-old, 2015-06-12 13:13:57]

lgtm

[commit-bot: I haz the power, 2015-06-12 13:14:07]

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

[commit-bot: I haz the power, 2015-06-12 13:20:27]

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

[commit-bot: I haz the power, 2015-06-12 13:20:29]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mithro-old, 2015-06-12 13:26:47]

Hi,

I tried to send this patch to the commit queue but it looks like there was a
conflict in the AUTHORS file. See the following for more information
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...

If you upload a patch which fixes that I'll try sending it to the commit queue
ASAP.

Tim 'mithro' Ansell

[Ankur Verma, 2015-06-13 11:34:26]

Thanks Tim & Sami for the review.
Apologies for the delayed response, had to take a day off.
Have had to rebase again. PTAL.

[mithro-old, 2015-06-13 12:00:51]

The CQ bit was checked by mithro@mithis.com

[mithro-old, 2015-06-13 12:00:52]

lgtm

[mithro-old, 2015-06-13 12:00:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/1132753008/#ps300001
(title: "Rebased.")

[commit-bot: I haz the power, 2015-06-13 12:01:03]

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

[commit-bot: I haz the power, 2015-06-13 13:38:41]

Committed patchset #15 (id:300001)

[commit-bot: I haz the power, 2015-06-13 13:39:21]

Patchset 15 (id:??) landed as
https://crrev.com/2e4c4e14344779d903ea578ae1be4663801d9f05
Cr-Commit-Position: refs/heads/master@{#334341}