Provision to start base::SimpleTestTickClock at initial ticks

Provision to start base::SimpleTestTickClock at initial ticks

This will let tests avoid a forced Advance(initial) before running.
Cleaned up cc and components tests on this ideology.
Updated all other call sites.

BUG=494894
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[Ankur Verma, 2015-06-17 09:08:55]

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

[Ankur Verma, 2015-06-17 09:21:33]

Hello mithro & Sami,
PTAL.
This is a first cut, where we're advancing the SimpleTestTickClock by 1
microsecond to avoid forceful Advance() by tests. I had to correct the
expectations of tests which considered default value to be TimeTicks(0). Have
covered as many tests as possible but this patch may potentially break tests
which I'm not aware of, so running through trybots will give better idea.

I did'nt want to dupe users by faking NowTicks() to return TimeTicks(1) only
initially, because it does'nt seem right and it breaks many more tests than
those covered in this patch-set.

I'd first request you to check if the approach is correct. If so, i'll add more
OWNERS for review.

Thanks!

[mithro-old, 2015-06-18 04:02:36]

I bumped this CL onto the try bots. Change is looking okay but we should see how
that goes before we review it too much further.

Tim 'mithro' Ansell

[Ankur Verma, 2015-06-18 05:13:17]

On 2015/06/18 04:02:36, mithro wrote:
> I bumped this CL onto the try bots. Change is looking okay but we should see
how
> that goes before we review it too much further.
> 
> Tim 'mithro' Ansell

Try bots results seem fine.
The breakage of unit_tests on linux_android_rel_ng is not related to this
change.
PTAL.

[Sami, 2015-06-18 20:57:43]

https://codereview.chromium.org/1178423008/diff/1/cc/scheduler/begin_frame_so...
File cc/scheduler/begin_frame_source_unittest.cc (right):

https://codereview.chromium.org/1178423008/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source_unittest.cc:347:
now_src_->Advance(base::TimeDelta::FromMicroseconds(999));
It feels like having two constructor for SimpleTestTickClock would help to avoid
cases like these. One would initialize it to zero and the other would take a
TimeTicks value to initialize to. WDYT?

[Ankur Verma, 2015-06-20 03:57:53]

On 2015/06/18 20:57:43, Sami wrote:
>
https://codereview.chromium.org/1178423008/diff/1/cc/scheduler/begin_frame_so...
> File cc/scheduler/begin_frame_source_unittest.cc (right):
> 
>
https://codereview.chromium.org/1178423008/diff/1/cc/scheduler/begin_frame_so...
> cc/scheduler/begin_frame_source_unittest.cc:347:
> now_src_->Advance(base::TimeDelta::FromMicroseconds(999));
> It feels like having two constructor for SimpleTestTickClock would help to
avoid
> cases like these. One would initialize it to zero and the other would take a
> TimeTicks value to initialize to. WDYT?

Sounds great. Least impact. I'll upload a patch asap.
Lets hope base/test owners buy it, see here =>
https://codereview.chromium.org/1132753008/diff/20001/base/test/simple_test_t...

[Ankur Verma, 2015-06-22 13:31:21]

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

[Ankur Verma, 2015-06-22 13:40:36]

PTAL.

@phajdan.jr
Please check base/test. Added ctor to start SimpleTestTickClock at some initial
ticks. This helps tests avoid doing a forced Advance() to some initial ticks
before running.
 
@mithro & Sami,
Have updated cc and components tests to start at 1 microsecond, removing the
unnecessary Advance()'s to high ticks.

[Ankur Verma, 2015-06-23 14:22:09]

Pls note, none of the try bot failures are attributed to this change.
PTAL.
Thanks!

[mithro-old, 2015-06-30 08:17:49]

Sorry about the delay, can you please upload a rebased version of this patch?

@phajdan.jr - ping? Can you please review?

If phajdan.jr doesn't get back to you, we should try the OWNERS of base.

[Paweł Hajdan Jr., 2015-06-30 09:12:51]

It's still not clear to me why this patch is needed.

If Advance is always required, does default ctor still make sense? Can we remove
it?

[Ankur Verma, 2015-06-30 09:19:26]

Rebased. Please trigger try bots.

@phajdan.jr - PTAL at base/test?

Thanks!

https://codereview.chromium.org/1178423008/diff/1/cc/scheduler/begin_frame_so...
File cc/scheduler/begin_frame_source_unittest.cc (right):

https://codereview.chromium.org/1178423008/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source_unittest.cc:347:
now_src_->Advance(base::TimeDelta::FromMicroseconds(999));
On 2015/06/18 20:57:43, Sami (OoO) wrote:
> It feels like having two constructor for SimpleTestTickClock would help to
avoid
> cases like these. One would initialize it to zero and the other would take a
> TimeTicks value to initialize to. WDYT?

Done.

[Ankur Verma, 2015-06-30 09:40:13]

On 2015/06/30 09:12:51, Paweł Hajdan Jr. wrote:
> It's still not clear to me why this patch is needed.
> 
> If Advance is always required, does default ctor still make sense? Can we
remove
> it?

I had made an attempt in Patch Set 1 to Advance to 1 microsecond from the
default ctor.
This ended up breaking expectations of some tests which considered default value
to be TimeTicks(0).
- cast_unittests (rtcp_utility_unittest)
- content_unittests (render_widget_host_view_aura_unittest)
- extensions_unittests (logger_unittest)
to name a few.

It also introduced some ugliness like:
=====
// SimpleTestTickClock starts at 1 microsecond, advance to 1000
now_src_->Advance(base::TimeDelta::FromMicroseconds(999));
=====

Thus keeping the default ctor as is, this patch avoid tests which consider
TimeTicks(0)
to be the "null" value, to avoid having to Advance(initial).

Hope this make sense.

[Ankur Verma, 2015-06-30 10:10:25]

Sorry for missing out on the point of default ctor.

We can probably remove the default ctor with something like this:
=====
SimpleTestTickClock(TimeTicks initial = TimeTicks());
=====

Will upload a revised patch in a few mins.

[Ankur Verma, 2015-06-30 10:31:45]

Updated change with removal of default ctor for SimpleTestTickClock.

PTAL. Thanks!

[Paweł Hajdan Jr., 2015-06-30 11:40:19]

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

https://codereview.chromium.org/1178423008/diff/60001/base/test/simple_test_t...
base/test/simple_test_tick_clock.h:22: SimpleTestTickClock(TimeTicks initial =
TimeTicks());
Default arguments are prohibited, please see
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argu...
.

[Ankur Verma, 2015-06-30 11:58:39]

@phajdan.jr
The guideline mentions an exception for ctor's.
Even if its still not acceptable, would you prefer the overloaded version in
Patch Set 3 or you still dont agree with the changes?

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

https://codereview.chromium.org/1178423008/diff/60001/base/test/simple_test_t...
base/test/simple_test_tick_clock.h:22: SimpleTestTickClock(TimeTicks initial =
TimeTicks());
On 2015/06/30 11:40:19, Paweł Hajdan Jr. wrote:
> Default arguments are prohibited, please see
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argu...
> .

Quoting from the above documentation:
"In addition, default function parameters are allowed in constructors. Most of
the cons listed above don't apply to constructors because it's impossible to
take their address."

[Ankur Verma, 2015-07-15 05:18:17]

Jumping back here. Ping!

@phajdan.jr,
Requesting your feedback on SimpleTestTickClock changes. Based on previous
reviews, i have four options:
1. Keep patch set 4 as-is on the pretext that Google cpp style guide allows
default arguments for constructors.
2. Revert back to patch set 3 which contains overloaded version of
SimpleTestTickclock ctor.
3. Remove the default ctor of SimpleTestTickClock and update all call-sites(~54)
accordingly.
4. Forget about this patch.

Please let me know what you think.

[Paweł Hajdan Jr., 2015-07-15 17:36:59]

3. Remove the default ctor of SimpleTestTickClock and update all call-sites(~54)
accordingly. sounds best to me.

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

https://codereview.chromium.org/1178423008/diff/60001/base/test/simple_test_t...
base/test/simple_test_tick_clock.h:22: SimpleTestTickClock(TimeTicks initial =
TimeTicks());
On 2015/06/30 at 11:58:39, Ankur Verma wrote:
> On 2015/06/30 11:40:19, Paweł Hajdan Jr. wrote:
> > Default arguments are prohibited, please see
> >
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argu...
> > .
> 
> Quoting from the above documentation:
> "In addition, default function parameters are allowed in constructors. Most of
the cons listed above don't apply to constructors because it's impossible to
take their address."

Ah, you're right, I didn't notice this. Sorry.

[Ankur Verma, 2015-07-16 04:20:21]

Thanks Pawel. I'll put up a patch soon enough.

On 2015/07/15 17:36:59, Paweł Hajdan Jr. wrote:
> 3. Remove the default ctor of SimpleTestTickClock and update all
call-sites(~54)
> accordingly. sounds best to me.
>

[Ankur Verma, 2015-12-18 14:24:16]

Apologies for a long lay-off on this. Have re-worked based on review comments.

@phajdan.jr
PTAL. As per your suggestion, the default ctor of SimpleTestTickClock is now
removed. All call sites have been updated. If you deem this fit for base/test, i
would get other OWNERS on board too.

@mithro
Please take an overview until phajdan.jr approves.

[Ankur Verma, 2015-12-18 14:29:07]

On 2015/12/18 14:24:16, Ankur Verma wrote:
> Apologies for a long lay-off on this. Have re-worked based on review comments.
> 
> @phajdan.jr
> PTAL. As per your suggestion, the default ctor of SimpleTestTickClock is now
> removed. All call sites have been updated. If you deem this fit for base/test,
i
> would get other OWNERS on board too.
> 
> @mithro
> Please take an overview until phajdan.jr approves.

@mithro,
Please trigger trybots if you will. Thanks!

[Ankur Verma, 2015-12-20 03:24:45]

The CQ bit was checked by ankur1.verma@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-20 03:24:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1178423008/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1178423008/140001

[commit-bot: I haz the power, 2015-12-20 03:24:56]

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

[commit-bot: I haz the power, 2015-12-20 03:24:57]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Ankur Verma, 2015-12-20 10:27:44]

Description was changed from

==========
Provision to start base::SimpleTestTickClock at initial ticks
    
This will let tests avoid a forced Advance(initial)
before running. Cleaned up cc and components tests
on this ideology.

BUG=494894
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Provision to start base::SimpleTestTickClock at initial ticks

This will let tests avoid a forced Advance(initial) before running. Cleaned up
cc and components tests on this ideology. Updated all other call sites.

BUG=494894
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Ankur Verma, 2015-12-20 10:28:11]

Description was changed from

==========
Provision to start base::SimpleTestTickClock at initial ticks

This will let tests avoid a forced Advance(initial) before running. Cleaned up
cc and components tests on this ideology. Updated all other call sites.

BUG=494894
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Provision to start base::SimpleTestTickClock at initial ticks

This will let tests avoid a forced Advance(initial) before running.
Cleaned up cc and components tests on this ideology.
Updated all other call sites.

BUG=494894
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Ankur Verma, 2015-12-24 09:54:59]

Ping!

@phajdan.jr
PTAL. As per your suggestion, the default ctor of SimpleTestTickClock is now
removed. All call sites have been updated. If you deem this fit for base/test, i
would get other OWNERS on board too.

The try-bot failures have been corrected in the Rebase patch.

@mithro
Request you to please triggers the bots again.