[blink] Support moving timers between WebTaskRunners

[blink] Support moving timers between WebTaskRunners

Support moving timers between WebTaskRunners for supporting per-frame
queues in use cases when objects (e.g. WebMediaPlayer) move between frames.

R=haraken@chromium.org
CC=alexclarke@chromium.org,skyostil@chromium.org

BUG=624696
Committed: https://crrev.com/3941bd7f8f4e92357151d25d4bf52584d783707e
Cr-Commit-Position: refs/heads/master@{#437258}

[altimin, 2016-12-07 15:57:48]

PTAL

[Sami, 2016-12-07 18:58:13]

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

[Sami, 2016-12-07 18:58:13]

lgtm with a suggestion.

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

https://codereview.chromium.org/2556053004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/Timer.cpp:93: TRACE_EVENT0("blink",
"TimerBase::moveToNewTaskRunner");
Might want to check if we're already on |taskRunner| (tip: you can compare the
underlying SingleThreadedTaskRunner).

[dcheng, 2016-12-07 18:59:57]

drive-by

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

https://codereview.chromium.org/2556053004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/Timer.cpp:97: m_webTaskRunner =
std::unique_ptr<WebTaskRunner>(taskRunner->clone());
Nit: clone() already returns a unique_ptr, doesn't it?

[haraken, 2016-12-08 01:37:20]

LGTM

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

https://codereview.chromium.org/2556053004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/Timer.cpp:93: TRACE_EVENT0("blink",
"TimerBase::moveToNewTaskRunner");

Do you really want to have the trace event? moveToNewTaskRunner is not doing
anything heavy.

https://codereview.chromium.org/2556053004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/Timer.cpp:101: m_nextFireTime = 0;

Line 100 and 101 wouldn't be needed. I think you can just call:

  setNextFireTime(now, m_nextFireTime - now);

[altimin, 2016-12-08 14:14:38]

Thanks for the review.

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

https://codereview.chromium.org/2556053004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/Timer.cpp:93: TRACE_EVENT0("blink",
"TimerBase::moveToNewTaskRunner");
On 2016/12/08 01:37:20, haraken wrote:
> 
> Do you really want to have the trace event? moveToNewTaskRunner is not doing
> anything heavy.

Agreed, removed.

https://codereview.chromium.org/2556053004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/Timer.cpp:93: TRACE_EVENT0("blink",
"TimerBase::moveToNewTaskRunner");
On 2016/12/07 18:58:13, Sami wrote:
> Might want to check if we're already on |taskRunner| (tip: you can compare the
> underlying SingleThreadedTaskRunner).

Done.

https://codereview.chromium.org/2556053004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/Timer.cpp:97: m_webTaskRunner =
std::unique_ptr<WebTaskRunner>(taskRunner->clone());
On 2016/12/07 18:59:57, dcheng wrote:
> Nit: clone() already returns a unique_ptr, doesn't it?

Thanks.

https://codereview.chromium.org/2556053004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/Timer.cpp:101: m_nextFireTime = 0;
On 2016/12/08 01:37:20, haraken wrote:
> 
> Line 100 and 101 wouldn't be needed. I think you can just call:
> 
>   setNextFireTime(now, m_nextFireTime - now);

They are needed because of line 127. setNextFireTime compares newTime with
m_nextFireTime and does nothing if they are equal.

[altimin, 2016-12-08 14:14:49]

The CQ bit was checked by altimin@chromium.org

[altimin, 2016-12-08 14:14:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2556053004/#ps20001
(title: "Address nits")

[commit-bot: I haz the power, 2016-12-08 14:15:05]

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-12-08 16:18:51]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481206489700080,
"parent_rev": "1006fb95874d36984f708d2dc02d1b762c1bba54", "commit_rev":
"fea3dce909c8fa865185d51719a00120bddfc333"}

[commit-bot: I haz the power, 2016-12-08 16:19:25]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-08 16:22:29]

Description was changed from

==========
[blink] Support moving timers between WebTaskRunners

Support moving timers between WebTaskRunners for supporting per-frame
queues in use cases when objects (e.g. WebMediaPlayer) move between frames.

R=haraken@chromium.org
CC=alexclarke@chromium.org,skyostil@chromium.org

BUG=624696
==========

to

==========
[blink] Support moving timers between WebTaskRunners

Support moving timers between WebTaskRunners for supporting per-frame
queues in use cases when objects (e.g. WebMediaPlayer) move between frames.

R=haraken@chromium.org
CC=alexclarke@chromium.org,skyostil@chromium.org

BUG=624696

Committed: https://crrev.com/3941bd7f8f4e92357151d25d4bf52584d783707e
Cr-Commit-Position: refs/heads/master@{#437258}
==========

[commit-bot: I haz the power, 2016-12-08 16:22:32]

Patchset 2 (id:??) landed as
https://crrev.com/3941bd7f8f4e92357151d25d4bf52584d783707e
Cr-Commit-Position: refs/heads/master@{#437258}