Template methods on Timer classes instead of the classes themselves.

base: Template methods on Timer classes instead of the classes themselves.

The base class for OneShotTimer and DelayedTimer is templated but
only the Start method needs to be (which has a TODO to make it go away
entirely too).

The DelayedTimer class is also templated but only its constructor
needs to be, and the type can be inferred at the callsite, so less
typing all around.

R=thakis@chromium.org
TBR=sky,brettw
BUG=148832
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/8c3eb804c56b7173f00e58e040f47fe867dabda7
Cr-Commit-Position: refs/heads/master@{#350496}

[danakj, 2015-09-22 00:47:41]

Today I asked myself "Why is OneShotTimer templated?" It appears to be for no
reason. Only the Start method needs to be templated (and the DelayTimer
constructor).

This also makes it easier for
https://code.google.com/p/chromium/issues/detail?id=148832, as timer users who
don't call this version of Start, but instead pass in a Closure, do not need to
declare a template type that goes unused anymore.

[danakj, 2015-09-22 00:49:28]

The patch is just a regex to drop the <T> from call sites, with these 2
exceptions.

You might need to open this in an incog window (or turn off extensions) to load
the CL cuz it's so big (sorry D:)

https://codereview.chromium.org/1355063004/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/system/automatic_reboot_manager_unittest.cc (left):

https://codereview.chromium.org/1355063004/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/system/automatic_reboot_manager_unittest.cc:137: typedef
base::OneShotTimer<AutomaticRebootManager> Timer;
I removed this typedef and made "Timer" into actual OneShotTimers below.

https://codereview.chromium.org/1355063004/diff/60001/chrome/browser/net/pred...
File chrome/browser/net/predictor_unittest.cc (left):

https://codereview.chromium.org/1355063004/diff/60001/chrome/browser/net/pred...
chrome/browser/net/predictor_unittest.cc:42: typedef
base::RepeatingTimer<WaitForResolutionHelper> HelperTimer;
I dropped this typedef also, and just use a RepeatingTimer* everywhere below.

[danakj, 2015-09-22 00:55:38]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-22 00:56:43]

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

[commit-bot: I haz the power, 2015-09-22 01:04:03]

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

[commit-bot: I haz the power, 2015-09-22 01:04:04]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Nico, 2015-09-22 01:16:22]

rad! lgtm

https://codereview.chromium.org/1355063004/diff/60001/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/1355063004/diff/60001/base/timer/timer.h#newc...
base/timer/timer.h:212: BaseTimerMethodPointer(bool repeats) : Timer(repeats,
repeats) {}
nit: 

  enum RepeatMode { kOneShot, kRepeating };
  BaseTImerMethodPointer(RepeatMode mode) : Timer(mode == kRepeating, mode ==
kRepeating) {}

Then the calling sites below become

  OneShotTimer : BaseTImerMethodPointer(kOneShot) {}

and

  RepeatingTimer : BaseTImerMethodPointer(kRepeating) {}

which is more readable

[Nico, 2015-09-22 01:16:52]

(once it builds :-) )

[danakj, 2015-09-22 18:28:03]

On 2015/09/22 01:16:52, Nico (traveling Tue Sep 22) wrote:
> (once it builds :-) )

Aw of course people have added new timers in the meantime. :)

[danakj, 2015-09-22 18:31:14]

https://codereview.chromium.org/1355063004/diff/60001/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/1355063004/diff/60001/base/timer/timer.h#newc...
base/timer/timer.h:212: BaseTimerMethodPointer(bool repeats) : Timer(repeats,
repeats) {}
On 2015/09/22 01:16:22, Nico (traveling Tue Sep 22) wrote:
> nit: 
> 
>   enum RepeatMode { kOneShot, kRepeating };
>   BaseTImerMethodPointer(RepeatMode mode) : Timer(mode == kRepeating, mode ==
> kRepeating) {}
> 
> Then the calling sites below become
> 
>   OneShotTimer : BaseTImerMethodPointer(kOneShot) {}
> 
> and
> 
>   RepeatingTimer : BaseTImerMethodPointer(kRepeating) {}
> 
> which is more readable

Done.

[danakj, 2015-09-22 18:52:23]

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

[danakj, 2015-09-22 18:52:50]

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

[danakj, 2015-09-22 18:53:56]

TBR'ing sky/brettw since this touches... pretty much every file in chrome, but
it's mechanical outside of base.

[danakj, 2015-09-22 18:54:04]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-09-22 18:54:05]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1355063004/#ps100001
(title: "timer: rebase")

[commit-bot: I haz the power, 2015-09-22 18:57:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1355063004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1355063004/100001

[commit-bot: I haz the power, 2015-09-22 19:14:51]

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

[commit-bot: I haz the power, 2015-09-22 19:14:55]

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

[danakj, 2015-09-22 19:53:58]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-09-22 19:53:59]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1355063004/#ps120001
(title: "timer: castexplicit")

[commit-bot: I haz the power, 2015-09-22 19:55:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1355063004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1355063004/120001

[commit-bot: I haz the power, 2015-09-22 20:18:34]

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

[commit-bot: I haz the power, 2015-09-22 20:18:37]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[danakj, 2015-09-22 21:10:31]

Patchset #7 (id:120001) has been deleted

[danakj, 2015-09-22 21:10:51]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-09-22 21:10:52]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1355063004/#ps140001
(title: "timer: .")

[brettw, 2015-09-22 21:11:45]

I looked at exactly one file and it seemed like an improvement. LGTM :)

[commit-bot: I haz the power, 2015-09-22 21:12:43]

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

[commit-bot: I haz the power, 2015-09-22 21:25:21]

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

[commit-bot: I haz the power, 2015-09-22 21:25:24]

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...)

[danakj, 2015-09-22 22:03:07]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-09-22 22:03:08]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1355063004/#ps160001
(title: "timer: rebase")

[commit-bot: I haz the power, 2015-09-22 22:06:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1355063004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1355063004/160001

[danakj, 2015-09-22 22:09:23]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-09-22 22:09:24]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1355063004/#ps180001
(title: "timer: anothernewtimer")

[commit-bot: I haz the power, 2015-09-22 22:11:58]

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

[commit-bot: I haz the power, 2015-09-22 23:50:26]

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

[commit-bot: I haz the power, 2015-09-22 23:50:29]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[danakj, 2015-09-23 00:19:59]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-09-23 00:20:00]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1355063004/#ps200001
(title: "timer: fixwindowslink+rebase")

[commit-bot: I haz the power, 2015-09-23 00:21:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1355063004/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1355063004/200001

[Nico, 2015-09-23 00:21:25]

https://codereview.chromium.org/1355063004/diff/200001/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/1355063004/diff/200001/base/timer/timer.h#new...
base/timer/timer.h:257: BASE_EXPORT DelayTimer(const tracked_objects::Location&
posted_from,
Huh, that's really needed? Surprising, given that the class already has this…

[commit-bot: I haz the power, 2015-09-23 01:41:11]

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

[commit-bot: I haz the power, 2015-09-23 01:41:15]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[danakj, 2015-09-23 19:00:32]

https://codereview.chromium.org/1355063004/diff/200001/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/1355063004/diff/200001/base/timer/timer.h#new...
base/timer/timer.h:257: BASE_EXPORT DelayTimer(const tracked_objects::Location&
posted_from,
On 2015/09/23 00:21:25, Nico (traveling Tue Sep 22) wrote:
> Huh, that's really needed? Surprising, given that the class already has this…

Ya.. compiler complains both with and without this. For others following along:

Without this it says it can't find the method at link time, cuz the class is
exported so it expects to find everything at linking. (MSVC only).

With this it fails too, for even better reasons it doesn't make sense to export
templates.

Nico gave me the idea to just export Reset since it's the whole reason for this
problem as it's virtual and clang-plugin makes me move it to the .cc so it
requires export. So trying that on the bots now.

[danakj, 2015-09-23 19:59:08]

Darn, no luck. This is related to
https://code.google.com/p/chromium/issues/detail?id=441916

clang-plugin ignores this class right now because it's a template, but with this
patch it's not so we have to move the Reset() function implementation to the
.cc.

Just exporting Reset() does not build anywhere, as the vtable for the class is
missing/not exported.

[danakj, 2015-09-23 21:01:55]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-09-23 21:01:56]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1355063004/#ps240001
(title: "timer: okthistimeforrealhahahahahaha")

[danakj, 2015-09-23 21:01:57]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2015-09-23 21:02:12]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[danakj, 2015-09-23 21:02:47]

On 2015/09/23 19:59:08, danakj wrote:
> Darn, no luck. This is related to
> https://code.google.com/p/chromium/issues/detail?id=441916
> 
> clang-plugin ignores this class right now because it's a template, but with
this
> patch it's not so we have to move the Reset() function implementation to the
> .cc.
> 
> Just exporting Reset() does not build anywhere, as the vtable for the class is
> missing/not exported.

Filed https://code.google.com/p/chromium/issues/detail?id=535296 about this
craziness. And trying a non-inline definition marked as "inline" and in the
header to please both clang-plugin and MSVC. Maybe.

[commit-bot: I haz the power, 2015-09-23 21:04:22]

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

[danakj, 2015-09-23 22:14:18]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2015-09-23 22:14:30]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-09-23 22:14:45]

It builds!
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...

[commit-bot: I haz the power, 2015-09-23 22:16:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1355063004/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1355063004/240001

[commit-bot: I haz the power, 2015-09-23 22:30:54]

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

[commit-bot: I haz the power, 2015-09-23 22:30:58]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[danakj, 2015-09-23 22:35:01]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-09-23 22:38:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1355063004/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1355063004/240001

[commit-bot: I haz the power, 2015-09-23 23:07:21]

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

[commit-bot: I haz the power, 2015-09-23 23:07:25]

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

[danakj, 2015-09-23 23:09:26]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-09-23 23:11:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1355063004/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1355063004/240001

[danakj, 2015-09-23 23:38:30]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-09-23 23:38:31]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1355063004/#ps260001
(title: "timer: rebase-yetanothernewtimer-D:")

[commit-bot: I haz the power, 2015-09-23 23:41:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1355063004/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1355063004/260001

[commit-bot: I haz the power, 2015-09-24 00:16:52]

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

[commit-bot: I haz the power, 2015-09-24 00:16:55]

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

[danakj, 2015-09-24 00:34:11]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-09-24 00:34:12]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1355063004/#ps280001
(title: "timer: fixcaller")

[commit-bot: I haz the power, 2015-09-24 00:38:39]

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

[commit-bot: I haz the power, 2015-09-24 07:53:13]

Committed patchset #14 (id:280001)

[commit-bot: I haz the power, 2015-09-24 07:53:54]

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