Remove Task inheritance from base::Timer.

Remove Task inheritance from base::Timer.

BUG=none
TEST=trybots
TBR=brettw

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114679

[dcheng, 2011-12-13 20:17:07]

I wonder if it would be nicer and more consistent for Timer to take a
base::Closure instead of a method pointer. It'd let us devirtualize TimerTask,
and maybe the size of the template instantiations would be smaller?

[dcheng, 2011-12-13 20:17:47]

I wonder if it would be nicer and more consistent for Timer to take a
base::Closure instead of a method pointer. It'd let us devirtualize TimerTask,
and maybe the size of the template instantiations would be smaller?

(This time adding a reviewer)

[willchan no longer on Chromium, 2011-12-14 01:38:05]

I think your suggestion is good, although it'll be a pain to do that transition.
Perhaps we should finish the current base::Bind() transition and then do this
timer transition afterward? If you're especially gungho about it, then I guess
we can do it now too. I'd probably advocate adding an overload and then do the
transition and delete the old path. Would temporarily bloat Timer.

http://codereview.chromium.org/8898019/diff/13/base/timer.h
File base/timer.h (right):

http://codereview.chromium.org/8898019/diff/13/base/timer.h#newcode166
base/timer.h:166: virtual void Run() {
How about we get rid of this TimerTask class and just move everything into the
base class. Change TimerTask to take a Closure instead of
Receiver+ReceiverMethod.

[dcheng, 2011-12-14 02:07:06]

http://codereview.chromium.org/8898019/diff/13/base/timer.h
File base/timer.h (right):

http://codereview.chromium.org/8898019/diff/13/base/timer.h#newcode166
base/timer.h:166: virtual void Run() {
On 2011/12/14 01:38:05, willchan wrote:
> How about we get rid of this TimerTask class and just move everything into the
> base class. Change TimerTask to take a Closure instead of
> Receiver+ReceiverMethod.

Won't that require me to include bind.h in timer.h because of the templating? I
thought that was a pattern to be avoided, due to the complexity of bind.h.

[willchan no longer on Chromium, 2011-12-14 19:58:53]

http://codereview.chromium.org/8898019/diff/13/base/timer.h
File base/timer.h (right):

http://codereview.chromium.org/8898019/diff/13/base/timer.h#newcode166
base/timer.h:166: virtual void Run() {
On 2011/12/14 02:07:07, dcheng wrote:
> On 2011/12/14 01:38:05, willchan wrote:
> > How about we get rid of this TimerTask class and just move everything into
the
> > base class. Change TimerTask to take a Closure instead of
> > Receiver+ReceiverMethod.
> 
> Won't that require me to include bind.h in timer.h because of the templating?
I
> thought that was a pattern to be avoided, due to the complexity of bind.h.

Closure does not require bind.h. The base::Bind() call would be done at the
callsite to pass a Closure into Start(). And Start() itself should be
implemented in the .cc file. There's no need to template BaseTimer if we use a
Closure, unless we want to keep is_repeating as a template parameter.

[dcheng, 2011-12-14 21:15:00]

http://codereview.chromium.org/8898019/diff/13/base/timer.h
File base/timer.h (right):

http://codereview.chromium.org/8898019/diff/13/base/timer.h#newcode166
base/timer.h:166: virtual void Run() {
On 2011/12/14 19:58:53, willchan wrote:
> On 2011/12/14 02:07:07, dcheng wrote:
> > On 2011/12/14 01:38:05, willchan wrote:
> > > How about we get rid of this TimerTask class and just move everything into
> the
> > > base class. Change TimerTask to take a Closure instead of
> > > Receiver+ReceiverMethod.
> > 
> > Won't that require me to include bind.h in timer.h because of the
templating?
> I
> > thought that was a pattern to be avoided, due to the complexity of bind.h.
> 
> Closure does not require bind.h. The base::Bind() call would be done at the
> callsite to pass a Closure into Start(). And Start() itself should be
> implemented in the .cc file. There's no need to template BaseTimer if we use a
> Closure, unless we want to keep is_repeating as a template parameter.

Ah. I was going to keep kIsRepeating as a template parameter so the branches
could be optimized out. For the moment, maybe it'd be more appropriate to do the
straightforward conversion; any improvements could be made in followup patches
with appropriate benchmarks, etc.

[willchan no longer on Chromium, 2011-12-14 22:16:56]

lgtm

[commit-bot: I haz the power, 2011-12-14 23:46:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/8898019/13

[commit-bot: I haz the power, 2011-12-14 23:46:36]

Presubmit check for 8898019-13 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/browser/renderer_host/render_widget_host.h,content/plugin/webplugin_proxy.h

Presubmit checks took 1.2s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[dcheng, 2011-12-15 01:12:50]

Adding brettw as TBR for content stuff.

[commit-bot: I haz the power, 2011-12-15 01:13:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/8898019/13

[commit-bot: I haz the power, 2011-12-15 02:22:12]

Try job failure for 8898019-13 (retry) on linux_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-12-15 08:02:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/8898019/13

[dcheng, 2011-12-15 08:03:19]

Going to give this one more try... I'm hoping it was flakiness since I can't
reproduce it locally but I'm not too optimistic...

[commit-bot: I haz the power, 2011-12-15 08:53:16]

Try job failure for 8898019-13 (retry) on linux_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-12-15 18:19:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/8898019/19004

[commit-bot: I haz the power, 2011-12-15 19:35:51]

Change committed as 114679