Refactor Timer classes in preparation for landing FrameTimers.

Refactor Timer classes in preparation for landing FrameTimers.

- The original Timer template is now TaskRunnerTimer and requires a
  TaskRunner argument.
- A new Timer template implements the behavior of the original Timer
  template to avoid breaking a lot of code.
- The timer callback parameter has changed to be a TimerBase*, to make
  it easier to share the underlying template implementations. This is a
  safe change, because almost all TaskRunnerTimer callbacks don't use
  the parameter. The few callbacks that care use it to multiplex
  multiple timers in one callback, and only use it for equality
  comparisons with subclasses of TimerBase, so there's no problem.

Followup CLs will introduce frame-specific timers (e.g. LoadingTimer)
which will use TaskRunnerTimer as the base template, as well as rename
the new Timer template to DefaultThreadTimer.

BUG=624694
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/c6dbdbb5994554bcb90de049a61a1ae1c73d13cd
Cr-Commit-Position: refs/heads/master@{#408626}

[dcheng, 2016-07-28 13:02:16]

Description was changed from

==========
Refactor Timer classes in preparation for landing FrameTimers.

- The original Timer template is now TaskRunnerTimer and requires a
  TaskRunner argument.
- A new Timer template implements the behavior of the original Timer
  template to avoid breaking a lot of code.
- The timer callback parameter has changed to be a TimerBase*, to make
  it easier to share the underlying template implementations. This is a
  safe change, because almost all TaskRunnerTimer callbacks don't use
  the parameter. The few callbacks that care use it to multiplex
  multiple timers in one callback, and only use it for equality
  comparisons with subclasses of TimerBase, so there's no problem.

Followup CLs will introduce frame-specific timers (e.g. LoadingTimer),
as well as rename the new Timer template to DefaultThreadTimer.

BUG=624694
==========

to

==========
Refactor Timer classes in preparation for landing FrameTimers.

- The original Timer template is now TaskRunnerTimer and requires a
  TaskRunner argument.
- A new Timer template implements the behavior of the original Timer
  template to avoid breaking a lot of code.
- The timer callback parameter has changed to be a TimerBase*, to make
  it easier to share the underlying template implementations. This is a
  safe change, because almost all TaskRunnerTimer callbacks don't use
  the parameter. The few callbacks that care use it to multiplex
  multiple timers in one callback, and only use it for equality
  comparisons with subclasses of TimerBase, so there's no problem.

Followup CLs will introduce frame-specific timers (e.g. LoadingTimer),
as well as rename the new Timer template to DefaultThreadTimer.

BUG=624694
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[dcheng, 2016-07-28 13:03:02]

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

[commit-bot: I haz the power, 2016-07-28 13:03:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2016-07-28 13:04:07]

Description was changed from

==========
Refactor Timer classes in preparation for landing FrameTimers.

- The original Timer template is now TaskRunnerTimer and requires a
  TaskRunner argument.
- A new Timer template implements the behavior of the original Timer
  template to avoid breaking a lot of code.
- The timer callback parameter has changed to be a TimerBase*, to make
  it easier to share the underlying template implementations. This is a
  safe change, because almost all TaskRunnerTimer callbacks don't use
  the parameter. The few callbacks that care use it to multiplex
  multiple timers in one callback, and only use it for equality
  comparisons with subclasses of TimerBase, so there's no problem.

Followup CLs will introduce frame-specific timers (e.g. LoadingTimer),
as well as rename the new Timer template to DefaultThreadTimer.

BUG=624694
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Refactor Timer classes in preparation for landing FrameTimers.

- The original Timer template is now TaskRunnerTimer and requires a
  TaskRunner argument.
- A new Timer template implements the behavior of the original Timer
  template to avoid breaking a lot of code.
- The timer callback parameter has changed to be a TimerBase*, to make
  it easier to share the underlying template implementations. This is a
  safe change, because almost all TaskRunnerTimer callbacks don't use
  the parameter. The few callbacks that care use it to multiplex
  multiple timers in one callback, and only use it for equality
  comparisons with subclasses of TimerBase, so there's no problem.

Followup CLs will introduce frame-specific timers (e.g. LoadingTimer)
which will use TaskRunnerTimer as the base template, as well as rename
the new Timer template to DefaultThreadTimer.

BUG=624694
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[dcheng, 2016-07-28 13:04:07]

dcheng@chromium.org changed reviewers:
+ haraken@chromium.org

[dcheng, 2016-07-28 13:09:51]

https://codereview.chromium.org/2191533003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ImageQualityController.h (right):

https://codereview.chromium.org/2191533003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ImageQualityController.h:84: void
setTimer(TaskRunnerTimer<ImageQualityController>*);
Do we have a better way to mock out timers for tests with the new scheduling
stuff? This is pretty ugly.

https://codereview.chromium.org/2191533003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/Timer.h (left):

https://codereview.chromium.org/2191533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/Timer.h:47: TimerBase();
This was only used by three subclasses: SuspendableTImer, SecureTextTimer and
XMLHttpRequestProgessEventThrottle. For now, they just keep the current
behavior, but we'll likely want to revisit.

https://codereview.chromium.org/2191533003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/Timer.h (right):

https://codereview.chromium.org/2191533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/Timer.h:198: using TimerFiredFunction =
typename TaskRunnerTimer<TimerFiredClass>::TimerFiredFunction;
I switched these to using type aliases, because it's more robust to any changes
in the signature in the base template.

To avoid having to define a slightly different typedef for each one, I just use
TimerBase* as the parameter: it's good enough for all the uses we care about
(most callbacks don't even care), and it's less typing.

[alex clarke (OOO till 29th), 2016-07-28 13:21:44]

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

[alex clarke (OOO till 29th), 2016-07-28 13:21:45]

Non owner LGTM

[commit-bot: I haz the power, 2016-07-28 13:52:26]

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

[commit-bot: I haz the power, 2016-07-28 13:52:27]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[dcheng, 2016-07-28 14:05:57]

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

[commit-bot: I haz the power, 2016-07-28 14:06:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[haraken, 2016-07-28 14:44:47]

This is a great step forward!

https://codereview.chromium.org/2191533003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ImageQualityController.h (right):

https://codereview.chromium.org/2191533003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ImageQualityController.h:84: void
setTimer(TaskRunnerTimer<ImageQualityController>*);
On 2016/07/28 13:09:51, dcheng (OOO Aug 1 - Aug 11) wrote:
> Do we have a better way to mock out timers for tests with the new scheduling
> stuff? This is pretty ugly.

Yeah, this looks ugly. Can we make MockTimer inherit from TimerBase and make
setTimer take a |TimerBase*| parameter?

https://codereview.chromium.org/2191533003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/Timer.h (left):

https://codereview.chromium.org/2191533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/Timer.h:47: TimerBase();
On 2016/07/28 13:09:51, dcheng (OOO Aug 1 - Aug 11) wrote:
> This was only used by three subclasses: SuspendableTImer, SecureTextTimer and
> XMLHttpRequestProgessEventThrottle. For now, they just keep the current
> behavior, but we'll likely want to revisit.

I believe that SuspendableTImer was intentionally using TimerBase() because
SuspendableTImer wants to use the timer task runner. But I'm not sure if
SecureTextTimer and XMLHttpRequestProgessEventThrottle had that intention :)

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.h (right):

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.h:277:
TaskRunnerTimer<FrameLoader> m_checkTimer;

Is this change needed?

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.h (right):

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.h:65:
std::unique_ptr<TaskRunnerTimer<SVGImageChromeClient>> m_animationTimer;

Yeah, this looks ugly...

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/Timer.cpp (right):

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/Timer.cpp:96: WebTaskRunner*
TimerBase::getDefaultTaskRunner()

getDefaultTaskRunner => getUnthrottledTaskRunner ?

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/Timer.h (right):

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/Timer.h:203: :
TaskRunnerTimer<TimerFiredClass>(TimerBase::getTimerTaskRunner(),
timerFiredClass, timerFiredFunction)

Doesn't this mean that all Timers start using the timer task runner? The timer
task runner should be limited to SuspendableTimers and a couple of other timers
(at least until we figure out what timers should  use what task runners).

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/Timer.h:212: class UnthrottledThreadTimer :
public TaskRunnerTimer<TimerFiredClass> {

Maybe can we keep the current name as is? UnthrottledTimer sounds nicer to me.

[commit-bot: I haz the power, 2016-07-28 15:18:05]

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

[commit-bot: I haz the power, 2016-07-28 15:18:07]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Sami, 2016-07-28 16:38:53]

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

[Sami, 2016-07-28 16:38:55]

Basic idea of marrying a timer with a WebTaskRunner seems fine to me.

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/Timer.cpp (right):

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/Timer.cpp:96: WebTaskRunner*
TimerBase::getDefaultTaskRunner()
On 2016/07/28 14:44:46, haraken wrote:
> 
> getDefaultTaskRunner => getUnthrottledTaskRunner ?

+1

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/Timer.h (right):

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/Timer.h:203: :
TaskRunnerTimer<TimerFiredClass>(TimerBase::getTimerTaskRunner(),
timerFiredClass, timerFiredFunction)
On 2016/07/28 14:44:46, haraken wrote:
> 
> Doesn't this mean that all Timers start using the timer task runner? The timer
> task runner should be limited to SuspendableTimers and a couple of other
timers
> (at least until we figure out what timers should  use what task runners).

All timers already use the timer task runner, unless I missed something :)

[haraken, 2016-07-28 17:38:15]

https://codereview.chromium.org/2191533003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/Timer.h (left):

https://codereview.chromium.org/2191533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/Timer.h:47: TimerBase();
On 2016/07/28 14:44:46, haraken wrote:
> On 2016/07/28 13:09:51, dcheng (OOO Aug 1 - Aug 11) wrote:
> > This was only used by three subclasses: SuspendableTImer, SecureTextTimer
and
> > XMLHttpRequestProgessEventThrottle. For now, they just keep the current
> > behavior, but we'll likely want to revisit.
> 
> I believe that SuspendableTImer was intentionally using TimerBase() because
> SuspendableTImer wants to use the timer task runner. But I'm not sure if
> SecureTextTimer and XMLHttpRequestProgessEventThrottle had that intention :)

So, ignore this comment.

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/Timer.h (right):

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/Timer.h:203: :
TaskRunnerTimer<TimerFiredClass>(TimerBase::getTimerTaskRunner(),
timerFiredClass, timerFiredFunction)
On 2016/07/28 16:38:55, Sami wrote:
> On 2016/07/28 14:44:46, haraken wrote:
> > 
> > Doesn't this mean that all Timers start using the timer task runner? The
timer
> > task runner should be limited to SuspendableTimers and a couple of other
> timers
> > (at least until we figure out what timers should  use what task runners).
> 
> All timers already use the timer task runner, unless I missed something :)

Oh, I was misunderstanding. You're right.

- Only DOMTimer is in frameScheduler()->timerTaskRunner().
- Other timers are in currentThread()->scheduler()->timerTaskRunner(). If I'm
not misunderstanding our plan, currentThread()->scheduler() is going to be
removed
(https://docs.google.com/document/d/10It1DFRP7H3gev9hQA-7dtTcvcMFhrxuCt3-k21yj...).

[dcheng, 2016-07-29 02:22:56]

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

[commit-bot: I haz the power, 2016-07-29 02:23:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2016-07-29 02:23:19]

https://codereview.chromium.org/2191533003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ImageQualityController.h (right):

https://codereview.chromium.org/2191533003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ImageQualityController.h:84: void
setTimer(TaskRunnerTimer<ImageQualityController>*);
On 2016/07/28 14:44:46, haraken wrote:
> On 2016/07/28 13:09:51, dcheng (OOO Aug 1 - Aug 11) wrote:
> > Do we have a better way to mock out timers for tests with the new scheduling
> > stuff? This is pretty ugly.
> 
> Yeah, this looks ugly. Can we make MockTimer inherit from TimerBase and make
> setTimer take a |TimerBase*| parameter?

Hmm, the more I think about, I think that Timer should just not be templated
(and use WTF::bind() internally to capture the timer task).

I suppose making it a TimerBase won't hurt for now though, so done.

https://codereview.chromium.org/2191533003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/Timer.h (left):

https://codereview.chromium.org/2191533003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/Timer.h:47: TimerBase();
On 2016/07/28 17:38:15, haraken wrote:
> On 2016/07/28 14:44:46, haraken wrote:
> > On 2016/07/28 13:09:51, dcheng (OOO Aug 1 - Aug 11) wrote:
> > > This was only used by three subclasses: SuspendableTImer, SecureTextTimer
> and
> > > XMLHttpRequestProgessEventThrottle. For now, they just keep the current
> > > behavior, but we'll likely want to revisit.
> > 
> > I believe that SuspendableTImer was intentionally using TimerBase() because
> > SuspendableTImer wants to use the timer task runner. But I'm not sure if
> > SecureTextTimer and XMLHttpRequestProgessEventThrottle had that intention :)
> 
> So, ignore this comment.

!!! I made a mistake. I changed SuspendableTimer to use the timer task runner
now instead of the unthrottled task runner (I accidentally confused myself with
the naming yesterday)

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.h (right):

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.h:277:
TaskRunnerTimer<FrameLoader> m_checkTimer;
On 2016/07/28 14:44:46, haraken wrote:
> 
> Is this change needed?

Yes: in this CL, Timer no longer takes a TaskRunner argument. I want to rename
the Timer class to something else (DefaultThreadTimer?) but I haven't thought of
a good name yet.

This one will change to something like LoadingTimer or LoadingFrameTimer. WDYT?

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/Timer.cpp (right):

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/Timer.cpp:96: WebTaskRunner*
TimerBase::getDefaultTaskRunner()
On 2016/07/28 16:38:55, Sami wrote:
> On 2016/07/28 14:44:46, haraken wrote:
> > 
> > getDefaultTaskRunner => getUnthrottledTaskRunner ?
> 
> +1

Done.

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/Timer.h (right):

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/Timer.h:203: :
TaskRunnerTimer<TimerFiredClass>(TimerBase::getTimerTaskRunner(),
timerFiredClass, timerFiredFunction)
On 2016/07/28 17:38:15, haraken wrote:
> On 2016/07/28 16:38:55, Sami wrote:
> > On 2016/07/28 14:44:46, haraken wrote:
> > > 
> > > Doesn't this mean that all Timers start using the timer task runner? The
> timer
> > > task runner should be limited to SuspendableTimers and a couple of other
> > timers
> > > (at least until we figure out what timers should  use what task runners).
> > 
> > All timers already use the timer task runner, unless I missed something :)
> 
> Oh, I was misunderstanding. You're right.
> 
> - Only DOMTimer is in frameScheduler()->timerTaskRunner().
> - Other timers are in currentThread()->scheduler()->timerTaskRunner(). If I'm
> not misunderstanding our plan, currentThread()->scheduler() is going to be
> removed
>
(https://docs.google.com/document/d/10It1DFRP7H3gev9hQA-7dtTcvcMFhrxuCt3-k21yj...).

Right, per Sami, the default for Timer right now is to already use the timer
task queue, so this doesn't change anything for them.

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/Timer.h:212: class UnthrottledThreadTimer :
public TaskRunnerTimer<TimerFiredClass> {
On 2016/07/28 14:44:46, haraken wrote:
> 
> Maybe can we keep the current name as is? UnthrottledTimer sounds nicer to me.

I think this might be confusing in combination with UnthrottledTimer for frame
though, right? Since we want to discourage this, I think it's OK to give the
discouraged versions longer names. I think that I will actually be removing this
in a followup, and moving all uses of this to frame's unthrottled task runner.
Does that make sense?

[dcheng, 2016-07-29 02:26:41]

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

[commit-bot: I haz the power, 2016-07-29 02:27:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 04:08:46]

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

[commit-bot: I haz the power, 2016-07-29 04:08:47]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-07-29 08:35:27]

LGTM

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/Timer.h (right):

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/Timer.h:212: class UnthrottledThreadTimer :
public TaskRunnerTimer<TimerFiredClass> {
On 2016/07/29 02:23:18, dcheng (OOO Aug 1 - Aug 11) wrote:
> On 2016/07/28 14:44:46, haraken wrote:
> > 
> > Maybe can we keep the current name as is? UnthrottledTimer sounds nicer to
me.
> 
> I think this might be confusing in combination with UnthrottledTimer for frame
> though, right? Since we want to discourage this, I think it's OK to give the
> discouraged versions longer names. I think that I will actually be removing
this
> in a followup, and moving all uses of this to frame's unthrottled task runner.
> Does that make sense?

Is your plan to replace UnthrottledThreadTimer with
TaskRunnerHelper::getUnthrottledTaskRunner()->postTask()? Then it makes sense.

Maybe it would be helpful to prefix deprecated classes with "Deprecated" or
"Legacy". At least add a comment that this timer is deprecated.

[dcheng, 2016-07-29 08:44:40]

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/Timer.h (right):

https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/Timer.h:212: class UnthrottledThreadTimer :
public TaskRunnerTimer<TimerFiredClass> {
On 2016/07/29 08:35:27, haraken wrote:
> On 2016/07/29 02:23:18, dcheng (OOO Aug 1 - Aug 11) wrote:
> > On 2016/07/28 14:44:46, haraken wrote:
> > > 
> > > Maybe can we keep the current name as is? UnthrottledTimer sounds nicer to
> me.
> > 
> > I think this might be confusing in combination with UnthrottledTimer for
frame
> > though, right? Since we want to discourage this, I think it's OK to give the
> > discouraged versions longer names. I think that I will actually be removing
> this
> > in a followup, and moving all uses of this to frame's unthrottled task
runner.
> > Does that make sense?
> 
> Is your plan to replace UnthrottledThreadTimer with
> TaskRunnerHelper::getUnthrottledTaskRunner()->postTask()? Then it makes sense.
> 
> Maybe it would be helpful to prefix deprecated classes with "Deprecated" or
> "Legacy". At least add a comment that this timer is deprecated.
> 

Yes, though I would probably just use a subclass of the not-yet-landed
FrameTimer since there are cancellation semantics involved.

Since we don't have any alternatives for code to use yet, I'll add the comment
about deprecation once we land some FrameTimers that associate with a frame
scheduler. Does that sound good?

[haraken, 2016-07-29 08:51:34]

On 2016/07/29 08:44:40, dcheng (OOO Aug 1 - Aug 11) wrote:
>
https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/Timer.h (right):
> 
>
https://codereview.chromium.org/2191533003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/Timer.h:212: class UnthrottledThreadTimer :
> public TaskRunnerTimer<TimerFiredClass> {
> On 2016/07/29 08:35:27, haraken wrote:
> > On 2016/07/29 02:23:18, dcheng (OOO Aug 1 - Aug 11) wrote:
> > > On 2016/07/28 14:44:46, haraken wrote:
> > > > 
> > > > Maybe can we keep the current name as is? UnthrottledTimer sounds nicer
to
> > me.
> > > 
> > > I think this might be confusing in combination with UnthrottledTimer for
> frame
> > > though, right? Since we want to discourage this, I think it's OK to give
the
> > > discouraged versions longer names. I think that I will actually be
removing
> > this
> > > in a followup, and moving all uses of this to frame's unthrottled task
> runner.
> > > Does that make sense?
> > 
> > Is your plan to replace UnthrottledThreadTimer with
> > TaskRunnerHelper::getUnthrottledTaskRunner()->postTask()? Then it makes
sense.
> > 
> > Maybe it would be helpful to prefix deprecated classes with "Deprecated" or
> > "Legacy". At least add a comment that this timer is deprecated.
> > 
> 
> Yes, though I would probably just use a subclass of the not-yet-landed
> FrameTimer since there are cancellation semantics involved.
> 
> Since we don't have any alternatives for code to use yet, I'll add the comment
> about deprecation once we land some FrameTimers that associate with a frame
> scheduler. Does that sound good?

Sounds good.

[haraken, 2016-07-29 08:53:19]

Also I want to get everyone on the same page about how the Timers are going to
be refactored, so would you post your proposal to the bug
(https://bugs.chromium.org/p/chromium/issues/detail?id=624694)?

[Sami, 2016-07-29 13:13:02]

lgtm. Thanks for outlining the plan too.

[dcheng, 2016-07-29 13:16:38]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-07-29 13:16:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
alexclarke@chromium.org
Link to the patchset: https://codereview.chromium.org/2191533003/#ps60001
(title: "More build fixes")

[commit-bot: I haz the power, 2016-07-29 13:16:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 13:24:13]

Description was changed from

==========
Refactor Timer classes in preparation for landing FrameTimers.

- The original Timer template is now TaskRunnerTimer and requires a
  TaskRunner argument.
- A new Timer template implements the behavior of the original Timer
  template to avoid breaking a lot of code.
- The timer callback parameter has changed to be a TimerBase*, to make
  it easier to share the underlying template implementations. This is a
  safe change, because almost all TaskRunnerTimer callbacks don't use
  the parameter. The few callbacks that care use it to multiplex
  multiple timers in one callback, and only use it for equality
  comparisons with subclasses of TimerBase, so there's no problem.

Followup CLs will introduce frame-specific timers (e.g. LoadingTimer)
which will use TaskRunnerTimer as the base template, as well as rename
the new Timer template to DefaultThreadTimer.

BUG=624694
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Refactor Timer classes in preparation for landing FrameTimers.

- The original Timer template is now TaskRunnerTimer and requires a
  TaskRunner argument.
- A new Timer template implements the behavior of the original Timer
  template to avoid breaking a lot of code.
- The timer callback parameter has changed to be a TimerBase*, to make
  it easier to share the underlying template implementations. This is a
  safe change, because almost all TaskRunnerTimer callbacks don't use
  the parameter. The few callbacks that care use it to multiplex
  multiple timers in one callback, and only use it for equality
  comparisons with subclasses of TimerBase, so there's no problem.

Followup CLs will introduce frame-specific timers (e.g. LoadingTimer)
which will use TaskRunnerTimer as the base template, as well as rename
the new Timer template to DefaultThreadTimer.

BUG=624694
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-07-29 13:24:17]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-29 13:25:47]

Description was changed from

==========
Refactor Timer classes in preparation for landing FrameTimers.

- The original Timer template is now TaskRunnerTimer and requires a
  TaskRunner argument.
- A new Timer template implements the behavior of the original Timer
  template to avoid breaking a lot of code.
- The timer callback parameter has changed to be a TimerBase*, to make
  it easier to share the underlying template implementations. This is a
  safe change, because almost all TaskRunnerTimer callbacks don't use
  the parameter. The few callbacks that care use it to multiplex
  multiple timers in one callback, and only use it for equality
  comparisons with subclasses of TimerBase, so there's no problem.

Followup CLs will introduce frame-specific timers (e.g. LoadingTimer)
which will use TaskRunnerTimer as the base template, as well as rename
the new Timer template to DefaultThreadTimer.

BUG=624694
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Refactor Timer classes in preparation for landing FrameTimers.

- The original Timer template is now TaskRunnerTimer and requires a
  TaskRunner argument.
- A new Timer template implements the behavior of the original Timer
  template to avoid breaking a lot of code.
- The timer callback parameter has changed to be a TimerBase*, to make
  it easier to share the underlying template implementations. This is a
  safe change, because almost all TaskRunnerTimer callbacks don't use
  the parameter. The few callbacks that care use it to multiplex
  multiple timers in one callback, and only use it for equality
  comparisons with subclasses of TimerBase, so there's no problem.

Followup CLs will introduce frame-specific timers (e.g. LoadingTimer)
which will use TaskRunnerTimer as the base template, as well as rename
the new Timer template to DefaultThreadTimer.

BUG=624694
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/c6dbdbb5994554bcb90de049a61a1ae1c73d13cd
Cr-Commit-Position: refs/heads/master@{#408626}
==========

[commit-bot: I haz the power, 2016-07-29 13:25:51]

Patchset 4 (id:??) landed as
https://crrev.com/c6dbdbb5994554bcb90de049a61a1ae1c73d13cd
Cr-Commit-Position: refs/heads/master@{#408626}