Change base::TickClock to a ref counted class.

Change base::TickClock to a ref counted class.

Typically when testing a class which uses the current clock, we replace calls
to base::Time::Now() with our own TickClock. This allows us to test various
situations by controlling the time that is visible to our code. The usual way
to do this is by keeping a scoped_ptr to a TickClock and set it to the default
tick clock on construction of our object. When we replace the clock for testing
our previous clock gets freed and the new clock takes its place. This clock is
freed when our object finally dies.

This model already has the issue with losing the pointer to our clock since we
lose ownership to the class we give it to; but this is mitigated in the code by
holding on to a pointer of the scoper that we pass to the set_clock_for_testing
method and call Advance on that pointer.

This problem becomes much worse when you have several objects that use the
current time, that need to function synchronously with each other. There are a
few bad options to go with here,
a.) Create one Clock, pass it to all the objects, and somehow put in a hack
    telling the objects that they aren't supposed to delete this clock.
    Otherwise, all the objects will try to delete the clock and cause a crash
    or none of them will delete it, causing a leak. Managing who should free
    the clock can get very tricky and error prone when several different
    objects get involved.

b.) Use several clocks, and move them all in tadem. This can get really tricky
    if you have objects that own other objects that need to share a clock (as
    is required in my case). Now you need to set the clock for the parent
    object, then reach into the object and pull out the instance of the child
    and give that child another clock.

Even though refpointers are avoided like the plague in Chrome, this seems to be
a very valid use case for them. This is an object that needs to be shared among
one more more objects who may or may not have any lifetime relationships with
each other.

The natural thought is the performance implication of using refptrs instead of
raw/scoped ptrs. To start with, most implementations of this were already using
scopers, so the performance difference there isn't super huge.

Second and more importantly, the performance sensitive aspect of a clock is
when we get the current time/ticks. This with a refptr is exactly the same as
a scoper and in release builds, possibly identical to raw pointers too. The
overhead added is during the construction, deletion and passing around - most
of the last will be done in tests, hence any performance impact of this change
will be negligible to none.

Sorry for the long essay, but I presume that people would have strong opionions
on this, so just putting down why I think this needs to be done preemptively :)

Since this is not a mechanical change, I do need owners to see if I have
changed their code over correctly. Most of this was changing scoped_ptrs to
scoped_refptrs but not all. There were some changes that were fairly involved
and should be reviewed in depth.

OWNERs reviews requested,
mark:
base/test/simple_test_tick_clock.h
base/time/default_tick_clock.h
base/time/tick_clock.h

dalecurtis:
chrome/browser/media/cast_transport_host_filter.cc
chrome/browser/media/cast_transport_host_filter.h
chrome/renderer/media/cast_session_delegate.cc
content/browser/media/audio_stream_monitor.cc
content/browser/media/audio_stream_monitor.h
content/browser/media/audio_stream_monitor_unittest.cc
content/renderer/media/render_media_log.cc
content/renderer/media/render_media_log.h
media/base/wall_clock_time_source.cc
media/base/wall_clock_time_source.h
media/cast/cast_environment.cc
media/cast/cast_environment.h
media/cast/test/skewed_tick_clock.h
media/filters/pipeline_integration_test_base.h
media/filters/video_frame_scheduler_impl.cc
media/filters/video_frame_scheduler_impl.h

wez:
extensions/browser/api/cast_channel/cast_channel_api.cc
extensions/browser/api/cast_channel/logger.cc
extensions/browser/api/cast_channel/logger.h

zea:
components/network_time/network_time_tracker.cc
components/network_time/network_time_tracker.h
components/network_time/network_time_tracker_unittest.cc

xiyuan:
chrome/browser/chromeos/system/automatic_reboot_manager.cc
chrome/browser/chromeos/system/automatic_reboot_manager.h
chrome/browser/chromeos/system/automatic_reboot_manager_unittest.cc

jhawkins:
chrome/browser/browser_process_impl.cc
chrome/browser/browser_process_platform_part_chromeos.cc

jennyz:
ash/system/chromeos/session/logout_confirmation_controller.cc
ash/system/chromeos/session/logout_confirmation_controller.h
ash/system/chromeos/session/logout_confirmation_controller_unittest.cc

flackr:
ash/wm/maximize_mode/maximize_mode_controller.cc
ash/wm/maximize_mode/maximize_mode_controller.h

derat:
ui/display/chromeos/x11/native_display_event_dispatcher_x11.cc
ui/display/chromeos/x11/native_display_event_dispatcher_x11.h

sadrul:
ui/events/test/event_generator.cc
ui/events/test/event_generator.h

R=dalecurtis@chromium.org, derat@chromium.org, flackr@chromium.org, jennyz@chromium.org, jhawkins@chromium.org, mark@chromium.org, sadrul@chromium.org, wez@chromium.org, xiyuan@chromium.org, zea@chromium.org
BUG=None.

[Daniel Erat, 2014-10-21 16:30:10]

(i don't have an opinion on the change to base)

https://codereview.chromium.org/670623002/diff/1/ui/display/chromeos/x11/nati...
File ui/display/chromeos/x11/native_display_event_dispatcher_x11.cc (right):

https://codereview.chromium.org/670623002/diff/1/ui/display/chromeos/x11/nati...
ui/display/chromeos/x11/native_display_event_dispatcher_x11.cc:103:
tick_clock_.swap(tick_clock);
why swap() instead of just assigning it?

[rkc, 2014-10-21 16:54:42]

https://codereview.chromium.org/670623002/diff/1/ui/display/chromeos/x11/nati...
File ui/display/chromeos/x11/native_display_event_dispatcher_x11.cc (right):

https://codereview.chromium.org/670623002/diff/1/ui/display/chromeos/x11/nati...
ui/display/chromeos/x11/native_display_event_dispatcher_x11.cc:103:
tick_clock_.swap(tick_clock);
On 2014/10/21 16:30:10, Daniel Erat wrote:
> why swap() instead of just assigning it?

I was trying to follow this pattern that is being used a lot of places where
tick clocks were being passed in. Looking at the code, there doesn't seem to be
any performance benefit, and ideally refptr should be passed in as const-ref -
which would not allow .swap anyway.

Changed it here, and, well 30 other places :)

[Daniel Erat, 2014-10-21 17:01:28]

https://codereview.chromium.org/670623002/diff/20001/ui/display/chromeos/x11/...
File ui/display/chromeos/x11/native_display_event_dispatcher_x11.cc (right):

https://codereview.chromium.org/670623002/diff/20001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/native_display_event_dispatcher_x11.cc:103:
tick_clock_.swap(tick_clock);
thanks, but i think the swap() got missed here, right?

[miu, 2014-10-21 17:28:25]

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

[miu, 2014-10-21 17:28:30]

Adding myself as media/cast reviewer.

Rahul: Have you considered using a proxy instead?  Meaning, a scheme that is
analogous to using the thread-safe MessageLoopProxy to utilize a MessageLoop
with an unknown lifetime?  I would also suggest using WeakPtrs except for the
fact that TickClock is often used across multiple threads...

[rkc, 2014-10-21 18:28:45]

On 2014/10/21 17:28:30, miu wrote:
> Adding myself as media/cast reviewer.
> 
> Rahul: Have you considered using a proxy instead?  Meaning, a scheme that is
> analogous to using the thread-safe MessageLoopProxy to utilize a MessageLoop
> with an unknown lifetime?  I would also suggest using WeakPtrs except for the
> fact that TickClock is often used across multiple threads...

Hey Yuri,

I had actually not considered a proxy pattern, thanks for the suggestion.
It does give a 'cleaner' solution, but it is still kinda ugly - I have upload a
CL that adds the proxy for a ref counted TickClock and uses it in my tests here
https://codereview.chromium.org/665353002/

Look over it and let me know if you think that's a cleaner change than this one?
(despite the fact, this does touch a lot more files)

I would rather that we go with a solution we'd want to use in the future. Having
the tick clock itself ref-counted seems like something we'd want to do but that
is purely my 'opinion' :)

[rkc, 2014-10-21 18:29:02]

https://codereview.chromium.org/670623002/diff/20001/ui/display/chromeos/x11/...
File ui/display/chromeos/x11/native_display_event_dispatcher_x11.cc (right):

https://codereview.chromium.org/670623002/diff/20001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/native_display_event_dispatcher_x11.cc:103:
tick_clock_.swap(tick_clock);
On 2014/10/21 17:01:27, Daniel Erat wrote:
> thanks, but i think the swap() got missed here, right?

Done.

[Mark Mentovai, 2014-10-22 16:30:39]

I think that if you’ve found a way to do this in a more isolated fashion
(https://codereview.chromium.org/665353002/), that’s probably a better way to
go. We shouldn’t have to impose this on everyone or add to base’s complexity if
there’s only one downstream user of the feature.

[rkc, 2014-10-22 20:04:15]

On 2014/10/22 16:30:39, Mark Mentovai wrote:
> I think that if you’ve found a way to do this in a more isolated fashion
> (<crxref style="background-image:
url(chrome-extension://ppnlfijacbhfpjgkafbeiaklmgoljooo/res/type/codereview.png);">https://codereview.chromium.org/665353002/</crxref>),
that’s probably a better way to
> go. We shouldn’t have to impose this on everyone or add to base’s complexity
if
> there’s only one downstream user of the feature.

Makes sense. Closing for now.