Implement requestIdleCallback API

Implementation of the requestIdleCallback API

Initial implementation of the requestIdleCallback API based on
the spec at https://w3c.github.io/requestidlecallback/.

This API is behind a flag and is not enabled by default.

BUG=514651
TBR=jochen@chromium.org
NOTRY=true

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200992

[esprehn, 2015-06-10 07:11:40]

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

[esprehn, 2015-06-10 07:11:42]

https://codereview.chromium.org/1119683003/diff/20001/Source/core/dom/Scripte...
File Source/core/dom/ScriptedIdleTaskController.cpp (right):

https://codereview.chromium.org/1119683003/diff/20001/Source/core/dom/Scripte...
Source/core/dom/ScriptedIdleTaskController.cpp:21:
ScriptedIdleTaskController::ScriptedIdleTaskController(Document* document)
reference

https://codereview.chromium.org/1119683003/diff/20001/Source/core/dom/Scripte...
Source/core/dom/ScriptedIdleTaskController.cpp:23: , m_callbacks()
no need to list things with default constructors

https://codereview.chromium.org/1119683003/diff/20001/Source/core/dom/Scripte...
Source/core/dom/ScriptedIdleTaskController.cpp:46:
Platform::current()->currentThread()->scheduler()->postIdleTask(
Can any of these ever be null? We should either make them start returning
references or add some null checks I guess.

https://codereview.chromium.org/1119683003/diff/20001/Source/core/dom/Scripte...
Source/core/dom/ScriptedIdleTaskController.cpp:47: FROM_HERE,
WTF::bind<double>(&ScriptedIdleTaskController::IdleRequestCallbackWrapper::runCallback,
callbackWrapper));
This has scary lifetime management, wouldn't you want runCallback to take an id
instead?

https://codereview.chromium.org/1119683003/diff/20001/Source/core/dom/Scripte...
Source/core/dom/ScriptedIdleTaskController.cpp:61: double highResDeadlineMs =
1000.0 *
m_document->loader()->timing().monotonicTimeToZeroBasedDocumentTime(deadlineSeconds);
How does the callback use the highResNow? Seems like they want deadline but not
now?

https://codereview.chromium.org/1119683003/diff/20001/Source/core/dom/Scripte...
Source/core/dom/ScriptedIdleTaskController.cpp:79:
callback->m_controller.clear();
If we just post ids you don't need this.

https://codereview.chromium.org/1119683003/diff/20001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1119683003/diff/20001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:1374: if (Document* d = document())
document, don't abbreviate.

https://codereview.chromium.org/1119683003/diff/20001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:1381: if (Document* d = document())
ditto, also can this really be null? I don't think it can.

[esprehn, 2015-06-10 07:12:24]

btw, I know this wasn't really up for review, but I thought I'd just share a
bunch of random comments for when it was ready. This looks pretty good, very
excited to ship this feature.

[rmcilroy, 2015-06-10 18:32:41]

> btw, I know this wasn't really up for review, but I thought  I'd just share a
> bunch of random comments for when it was ready. This looks  pretty good, very
> excited to ship this feature.

No problem, thanks for the review Elliott. Glad to hear you are excited to ship
the feature :).

https://codereview.chromium.org/1119683003/diff/20001/Source/core/dom/Scripte...
File Source/core/dom/ScriptedIdleTaskController.cpp (right):

https://codereview.chromium.org/1119683003/diff/20001/Source/core/dom/Scripte...
Source/core/dom/ScriptedIdleTaskController.cpp:21:
ScriptedIdleTaskController::ScriptedIdleTaskController(Document* document)
On 2015/06/10 07:11:41, esprehn wrote:
> reference

Moved to ActiveDOMObject so this isn't needed any more, but still takes
ExecutionContext as pointer since that's what ActiveDOMObject's constructor
needs.

https://codereview.chromium.org/1119683003/diff/20001/Source/core/dom/Scripte...
Source/core/dom/ScriptedIdleTaskController.cpp:23: , m_callbacks()
On 2015/06/10 07:11:41, esprehn wrote:
> no need to list things with default constructors

Done.

https://codereview.chromium.org/1119683003/diff/20001/Source/core/dom/Scripte...
Source/core/dom/ScriptedIdleTaskController.cpp:46:
Platform::current()->currentThread()->scheduler()->postIdleTask(
On 2015/06/10 07:11:41, esprehn wrote:
> Can any of these ever be null? We should either make them start returning
> references or add some null checks I guess.

They shouldn't return null whenever Blink is initialized. There are quite a few
other places in the code where we do the same sequence of
"Platform::current()->currentThread()->scheduler()", so it should be safe.

It is probably difficult to make them return references though since the
Platform, WebThread and WebScheduler are all abstract classes in Blink, which
are implemented by the embedder, and thus have default noop implementations
which return nullptr until they are overriden by the embedder.

https://codereview.chromium.org/1119683003/diff/20001/Source/core/dom/Scripte...
Source/core/dom/ScriptedIdleTaskController.cpp:47: FROM_HERE,
WTF::bind<double>(&ScriptedIdleTaskController::IdleRequestCallbackWrapper::runCallback,
callbackWrapper));
On 2015/06/10 07:11:42, esprehn wrote:
> This has scary lifetime management, wouldn't you want runCallback to take an
id
> instead?

It needs some reference to the ScriptedIdleTaskController though (otherwise it
couldn't do anything with the id), and since ScriptedIdleTaskController will be
GCed in the Oilpan world, it can't be passed as a raw pointer out of Blink (as
is required for post[Idle]Task) so needs to be wrapped up in an off-oilpan-heap
object which contains a Persistent with a reference to the
ScriptedIdleTaskController.

I've changed the IdleRequestCallbackWrapper to hold an id and a reference to the
ScriptedIdleTaskController directly (instead of holding an IdleRequestCallback
with a reference to IdleRequestCallbackWrapper which I originally did because I
wanted to avoid a pending idle task stuck on the message loop holding onto the
ScriptedIdleTaskController and preventing it from being GCed, however I don't
think this is really much of a concern now that ScriptedIdleTaskController's
m_callbacks hashmap get cleared when the document goes inactive). Is this
better?

If there is a better way to do this I'd be very open to it.

https://codereview.chromium.org/1119683003/diff/20001/Source/core/dom/Scripte...
Source/core/dom/ScriptedIdleTaskController.cpp:61: double highResDeadlineMs =
1000.0 *
m_document->loader()->timing().monotonicTimeToZeroBasedDocumentTime(deadlineSeconds);
On 2015/06/10 07:11:41, esprehn wrote:
> How does the callback use the highResNow? Seems like they want deadline but
not
> now?

I've already removed |now| in the latest patch-set (it was originally in the
draft spec to avoid the web-dev having to call the perf.now() during the
callback but we decided against this).

https://codereview.chromium.org/1119683003/diff/20001/Source/core/dom/Scripte...
Source/core/dom/ScriptedIdleTaskController.cpp:79:
callback->m_controller.clear();
On 2015/06/10 07:11:41, esprehn wrote:
> If we just post ids you don't need this.

Done.

https://codereview.chromium.org/1119683003/diff/20001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1119683003/diff/20001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:1374: if (Document* d = document())
On 2015/06/10 07:11:42, esprehn wrote:
> document, don't abbreviate.

I couldn't call this document because of the function of the same name - instead
I just removed the local variable and used document() in both places (also in
rAF).

https://codereview.chromium.org/1119683003/diff/20001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:1381: if (Document* d = document())
On 2015/06/10 07:11:42, esprehn wrote:
> ditto, also can this really be null? I don't think it can.

clearDocument resets m_document. I'm not sure on whether this can have happened
before a requestIdleFrame/cancelIdleFrame though (I was following the same
pattern as rAF above).

[rmcilroy, 2015-07-23 17:00:31]

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

[commit-bot: I haz the power, 2015-07-23 17:00:45]

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

[commit-bot: I haz the power, 2015-07-23 18:10:57]

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

[commit-bot: I haz the power, 2015-07-23 18:10:58]

Dry run: Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/71381)

[rmcilroy, 2015-07-29 15:21:42]

rmcilroy@chromium.org changed reviewers:
+ jochen@chromium.org

[rmcilroy, 2015-07-29 15:21:42]

+jochen. Still to add LayoutTests, but feel free to take a look before then if
you like. Thanks.

[rmcilroy, 2015-07-31 09:04:28]

Added LayoutTests, please take a look, thanks.

[jochen (gone - plz use gerrit), 2015-07-31 09:19:19]

https://codereview.chromium.org/1119683003/diff/120001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/1119683003/diff/120001/Source/core/core.gypi#...
Source/core/core.gypi:2286: 'dom/IdleCallbackDeadline.cpp',
alphabetical ordering

https://codereview.chromium.org/1119683003/diff/120001/Source/core/core.gypi#...
Source/core/core.gypi:2372: 'dom/ScriptedIdleController.h',
ScripedIdle*Task*Controller.h

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/Docume...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/Docume...
Source/core/dom/Document.cpp:5251: if (!m_scriptedIdleTaskController) {
no { }

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/Docume...
Source/core/dom/Document.cpp:5265: return;
that should be unreachable, no?

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/Docume...
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/Docume...
Source/core/dom/Document.h:937: 
nit. remove

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/IdleCa...
File Source/core/dom/IdleCallbackDeadline.h (right):

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/IdleCa...
Source/core/dom/IdleCallbackDeadline.h:20: static IdleCallbackDeadline*
create(double deadline, bool didTimeout, const DocumentLoadTiming& timing)
please use an enum instead of a bool

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/IdleRe...
File Source/core/dom/IdleRequestCallback.h (right):

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/IdleRe...
Source/core/dom/IdleRequestCallback.h:23: int m_id;
interfaces shouldn't have data members. and they shouldn't be public anyways

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/Script...
File Source/core/dom/ScriptedIdleTaskController.h (right):

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.h:22: 
nit. remove empty line

https://codereview.chromium.org/1119683003/diff/120001/public/web/WebRuntimeF...
File public/web/WebRuntimeFeatures.h (right):

https://codereview.chromium.org/1119683003/diff/120001/public/web/WebRuntimeF...
public/web/WebRuntimeFeatures.h:109: BLINK_EXPORT static void
RequestIdleCallback(bool);
why do you need this?

should also be called enableRequestIdleCallback(bool) if you need it

[rmcilroy, 2015-07-31 12:01:54]

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

[commit-bot: I haz the power, 2015-07-31 12:02:18]

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

[commit-bot: I haz the power, 2015-07-31 12:05:37]

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

[commit-bot: I haz the power, 2015-07-31 12:05:37]

Dry run: Try jobs failed on following builders:
  android_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)
  android_blink_compile_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)
  linux_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_compile_dbg/...)
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...)
  linux_chromium_gn_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)

[Sami, 2015-08-10 09:47:46]

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

[Sami, 2015-08-10 09:47:47]

Looks pretty good I think. A few comments below.

https://codereview.chromium.org/1119683003/diff/120001/LayoutTests/virtual/th...
File LayoutTests/virtual/threaded/fast/idle-callback/README.txt (right):

https://codereview.chromium.org/1119683003/diff/120001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/fast/idle-callback/README.txt:2: #
--enable-threaded-compositing since the tests require threaded compositing
Do we still expose requestIdleCallback in non-threaded configurations? If it
doesn't work (i.e., you never get idle time), we might want to hide it instead.
Then again I'm not sure where we even ship single threaded compositing these
days.

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/Docume...
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/Docume...
Source/core/dom/Document.h:931: int requestIdleCallback(IdleRequestCallback*,
double timeout);
nit: timeoutMs/Millis/Milliseconds? (here and elsewhere)

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/IdleCa...
File Source/core/dom/IdleCallbackDeadline.h (right):

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/IdleCa...
Source/core/dom/IdleCallbackDeadline.h:25: double deadline()
nit: const

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/IdleCa...
Source/core/dom/IdleCallbackDeadline.h:30: bool didTimeout()
nit const

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/IdleCa...
Source/core/dom/IdleCallbackDeadline.h:35: bool isExceeded();
nit const

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/Script...
File Source/core/dom/ScriptedIdleTaskController.h (right):

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.h:36: void stop() override;
Do we need to support suspend/resume like with rAF? IIRC this could fire at
unfortunate times otherwise -- while we're suspended on a breakpoint for
instance.

[rmcilroy, 2015-08-11 16:30:52]

All comments addressed, ptal, thanks.

https://codereview.chromium.org/1119683003/diff/120001/LayoutTests/virtual/th...
File LayoutTests/virtual/threaded/fast/idle-callback/README.txt (right):

https://codereview.chromium.org/1119683003/diff/120001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/fast/idle-callback/README.txt:2: #
--enable-threaded-compositing since the tests require threaded compositing
On 2015/08/10 09:47:46, Sami wrote:
> Do we still expose requestIdleCallback in non-threaded configurations? If it
> doesn't work (i.e., you never get idle time), we might want to hide it
instead.
> Then again I'm not sure where we even ship single threaded compositing these
> days.

It doesn't seem to work with non threaded compositor mode (the compositor
doesn't send BeginMainFrame's to the scheduler). I'm not sure how we could hide
it if we are in single thread compositor mode though. It would still work with
timeouts though, and given we don't ship single thread compositor mode anywhere
anyway, maybe only running rIC during timeouts ofor single threaded compositor
would be OK, wdyt?

https://codereview.chromium.org/1119683003/diff/120001/Source/core/core.gypi
File Source/core/core.gypi (right):

https://codereview.chromium.org/1119683003/diff/120001/Source/core/core.gypi#...
Source/core/core.gypi:2286: 'dom/IdleCallbackDeadline.cpp',
On 2015/07/31 09:19:19, jochen (traveling - slow) wrote:
> alphabetical ordering

Done.

https://codereview.chromium.org/1119683003/diff/120001/Source/core/core.gypi#...
Source/core/core.gypi:2372: 'dom/ScriptedIdleController.h',
On 2015/07/31 09:19:18, jochen (traveling - slow) wrote:
> ScripedIdle*Task*Controller.h

Opps, thanks. Done.

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/Docume...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/Docume...
Source/core/dom/Document.cpp:5251: if (!m_scriptedIdleTaskController) {
On 2015/07/31 09:19:19, jochen (traveling - slow) wrote:
> no { }

Done.

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/Docume...
Source/core/dom/Document.cpp:5265: return;
On 2015/07/31 09:19:19, jochen (traveling - slow) wrote:
> that should be unreachable, no?

If the page calls cancelIdleCallback (with a random int) but hasn't yet called
requestIdleCallback then we could reach this without having a
m_scriptedIdleTaskController. I copied the pattern from cancelAnimationFrame
above.

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/Docume...
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/Docume...
Source/core/dom/Document.h:931: int requestIdleCallback(IdleRequestCallback*,
double timeout);
On 2015/08/10 09:47:46, Sami wrote:
> nit: timeoutMs/Millis/Milliseconds? (here and elsewhere)

Done.

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/Docume...
Source/core/dom/Document.h:937: 
On 2015/07/31 09:19:19, jochen (traveling - slow) wrote:
> nit. remove

Done.

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/IdleCa...
File Source/core/dom/IdleCallbackDeadline.h (right):

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/IdleCa...
Source/core/dom/IdleCallbackDeadline.h:20: static IdleCallbackDeadline*
create(double deadline, bool didTimeout, const DocumentLoadTiming& timing)
On 2015/07/31 09:19:19, jochen (traveling - slow) wrote:
> please use an enum instead of a bool

Done.

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/IdleCa...
Source/core/dom/IdleCallbackDeadline.h:25: double deadline()
On 2015/08/10 09:47:47, Sami wrote:
> nit: const

Done.

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/IdleCa...
Source/core/dom/IdleCallbackDeadline.h:30: bool didTimeout()
On 2015/08/10 09:47:46, Sami wrote:
> nit const

Done.

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/IdleCa...
Source/core/dom/IdleCallbackDeadline.h:35: bool isExceeded();
On 2015/08/10 09:47:46, Sami wrote:
> nit const

Done.

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/IdleRe...
File Source/core/dom/IdleRequestCallback.h (right):

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/IdleRe...
Source/core/dom/IdleRequestCallback.h:23: int m_id;
On 2015/07/31 09:19:19, jochen (traveling - slow) wrote:
> interfaces shouldn't have data members. and they shouldn't be public anyways

Done (although FrameRequestCallback which I was following as an example does
this).

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/Script...
File Source/core/dom/ScriptedIdleTaskController.h (right):

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.h:22: 
On 2015/07/31 09:19:19, jochen (traveling - slow) wrote:
> nit. remove empty line

Done - git cl format does this though :(.

https://codereview.chromium.org/1119683003/diff/120001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.h:36: void stop() override;
On 2015/08/10 09:47:47, Sami wrote:
> Do we need to support suspend/resume like with rAF? IIRC this could fire at
> unfortunate times otherwise -- while we're suspended on a breakpoint for
> instance.

Good catch! This should now be addressed, thanks.

https://codereview.chromium.org/1119683003/diff/120001/public/web/WebRuntimeF...
File public/web/WebRuntimeFeatures.h (right):

https://codereview.chromium.org/1119683003/diff/120001/public/web/WebRuntimeF...
public/web/WebRuntimeFeatures.h:109: BLINK_EXPORT static void
RequestIdleCallback(bool);
On 2015/07/31 09:19:19, jochen (traveling - slow) wrote:
> why do you need this?
> 
> should also be called enableRequestIdleCallback(bool) if you need it

Removed.

[rmcilroy, 2015-08-11 16:45:35]

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

[commit-bot: I haz the power, 2015-08-11 16:45:41]

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

[Sami, 2015-08-11 18:03:00]

Thanks, just tiny nits.

https://codereview.chromium.org/1119683003/diff/140001/LayoutTests/virtual/th...
File LayoutTests/virtual/threaded/fast/idle-callback/timeout.html (right):

https://codereview.chromium.org/1119683003/diff/140001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/fast/idle-callback/timeout.html:39: },
'requestIdleCallback not scheduled when event loop is busy.');
FYI we're considering a model where the main thread will request a lower
framerate if there's not enough idle time, which could break this test. I'm not
sure if there's a way to make this robust against that so we can always revisit
this later.

https://codereview.chromium.org/1119683003/diff/140001/Source/core/dom/IdleCa...
File Source/core/dom/IdleCallbackDeadline.h (right):

https://codereview.chromium.org/1119683003/diff/140001/Source/core/dom/IdleCa...
Source/core/dom/IdleCallbackDeadline.h:19: enum CallbackType {
nit: enum class?

https://codereview.chromium.org/1119683003/diff/140001/Source/core/dom/IdleCa...
Source/core/dom/IdleCallbackDeadline.h:25: static IdleCallbackDeadline*
create(double deadline, CallbackType callbackType, const DocumentLoadTiming&
timing)
nit: deadlineMillis (here and below)

https://codereview.chromium.org/1119683003/diff/140001/Source/core/dom/Script...
File Source/core/dom/ScriptedIdleTaskController.cpp (right):

https://codereview.chromium.org/1119683003/diff/140001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.cpp:47:
Platform::current()->currentThread()->scheduler()->postIdleTask(
nit: Maybe it's worth saving a pointer to the scheduler to cut down on the
pointer chasing a bit? Current way works too.

https://codereview.chromium.org/1119683003/diff/140001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.cpp:71: // Just drop callbacks called
while idle, these will be reposted on the idle task queue when we are resumed.
s/idle/suspended/?

https://codereview.chromium.org/1119683003/diff/140001/Source/core/dom/Script...
File Source/core/dom/ScriptedIdleTaskController.h (right):

https://codereview.chromium.org/1119683003/diff/140001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.h:61: class IdleRequestTimoutTimer :
public Timer<IdleRequestTimoutTimer> {
typo: Timeout

https://codereview.chromium.org/1119683003/diff/140001/Source/core/frame/Loca...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1119683003/diff/140001/Source/core/frame/Loca...
Source/core/frame/LocalDOMWindow.cpp:1268: return
document()->requestAnimationFrame(callback);
Was this an intended change?

[commit-bot: I haz the power, 2015-08-11 18:46:42]

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

[commit-bot: I haz the power, 2015-08-11 18:46:42]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2015-08-12 14:17:31]

https://codereview.chromium.org/1119683003/diff/140001/LayoutTests/virtual/th...
File LayoutTests/virtual/threaded/fast/idle-callback/timeout.html (right):

https://codereview.chromium.org/1119683003/diff/140001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/fast/idle-callback/timeout.html:39: },
'requestIdleCallback not scheduled when event loop is busy.');
On 2015/08/11 18:03:00, Sami wrote:
> FYI we're considering a model where the main thread will request a lower
> framerate if there's not enough idle time, which could break this test. I'm
not
> sure if there's a way to make this robust against that so we can always
revisit
> this later.

How much would you lower the frame rate? I made this 40ms to ensure it still
filled the frame if we dropped down to 30fps. In anycase, let's revisit this if
necessary.

https://codereview.chromium.org/1119683003/diff/140001/Source/core/dom/IdleCa...
File Source/core/dom/IdleCallbackDeadline.h (right):

https://codereview.chromium.org/1119683003/diff/140001/Source/core/dom/IdleCa...
Source/core/dom/IdleCallbackDeadline.h:19: enum CallbackType {
On 2015/08/11 18:03:00, Sami wrote:
> nit: enum class?

Done.

https://codereview.chromium.org/1119683003/diff/140001/Source/core/dom/IdleCa...
Source/core/dom/IdleCallbackDeadline.h:25: static IdleCallbackDeadline*
create(double deadline, CallbackType callbackType, const DocumentLoadTiming&
timing)
On 2015/08/11 18:03:00, Sami wrote:
> nit: deadlineMillis (here and below)

Done.

https://codereview.chromium.org/1119683003/diff/140001/Source/core/dom/Script...
File Source/core/dom/ScriptedIdleTaskController.cpp (right):

https://codereview.chromium.org/1119683003/diff/140001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.cpp:47:
Platform::current()->currentThread()->scheduler()->postIdleTask(
On 2015/08/11 18:03:00, Sami wrote:
> nit: Maybe it's worth saving a pointer to the scheduler to cut down on the
> pointer chasing a bit? Current way works too.

Done.

https://codereview.chromium.org/1119683003/diff/140001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.cpp:71: // Just drop callbacks called
while idle, these will be reposted on the idle task queue when we are resumed.
On 2015/08/11 18:03:00, Sami wrote:
> s/idle/suspended/?

Done.

https://codereview.chromium.org/1119683003/diff/140001/Source/core/dom/Script...
File Source/core/dom/ScriptedIdleTaskController.h (right):

https://codereview.chromium.org/1119683003/diff/140001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.h:61: class IdleRequestTimoutTimer :
public Timer<IdleRequestTimoutTimer> {
On 2015/08/11 18:03:00, Sami wrote:
> typo: Timeout

Done.

https://codereview.chromium.org/1119683003/diff/140001/Source/core/frame/Loca...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1119683003/diff/140001/Source/core/frame/Loca...
Source/core/frame/LocalDOMWindow.cpp:1268: return
document()->requestAnimationFrame(callback);
On 2015/08/11 18:03:00, Sami wrote:
> Was this an intended change?

Yes (previous review with Eliott suggested this for requestIdleCallback so I did
it here as well to keep things consistent)

[Sami, 2015-08-12 15:47:39]

Thanks! Non-owner lgtm!

https://codereview.chromium.org/1119683003/diff/140001/LayoutTests/virtual/th...
File LayoutTests/virtual/threaded/fast/idle-callback/timeout.html (right):

https://codereview.chromium.org/1119683003/diff/140001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/fast/idle-callback/timeout.html:39: },
'requestIdleCallback not scheduled when event loop is busy.');
On 2015/08/12 14:17:30, rmcilroy wrote:
> On 2015/08/11 18:03:00, Sami wrote:
> > FYI we're considering a model where the main thread will request a lower
> > framerate if there's not enough idle time, which could break this test. I'm
> not
> > sure if there's a way to make this robust against that so we can always
> revisit
> > this later.
> 
> How much would you lower the frame rate? I made this 40ms to ensure it still
> filled the frame if we dropped down to 30fps. In anycase, let's revisit this
if
> necessary.

Not sure yet -- I think we could go almost arbitrarily low if we needed to.
Still, this is fine as is for now.

[jochen (gone - plz use gerrit), 2015-08-12 16:33:49]

i won't have the time to review this thoroughly before my vacation, sorry

[rmcilroy, 2015-08-12 21:53:37]

rmcilroy@chromium.org changed reviewers:
- jochen@chromium.org

[rmcilroy, 2015-08-12 21:56:19]

rmcilroy@chromium.org changed reviewers:
+ jochen@chromium.org

[rmcilroy, 2015-08-12 21:57:26]

On 2015/08/12 16:33:49, jochen (ooo) wrote:
> i won't have the time to review this thoroughly before my vacation, sorry

No worries, thanks for the reviews

Elliott: Could you give this an OWNERS review since Jochen is out? Thanks.

[rmcilroy, 2015-08-13 21:43:36]

On 2015/08/12 21:57:26, rmcilroy wrote:
> On 2015/08/12 16:33:49, jochen (ooo) wrote:
> > i won't have the time to review this thoroughly before my vacation, sorry
> 
> No worries, thanks for the reviews
> 
> Elliott: Could you give this an OWNERS review since Jochen is out? Thanks.

Elliott, would you be able to take a look or is there someone else you would
suggest?

[rmcilroy, 2015-08-13 21:44:34]

Elliott, would you be able to take a look or is there someone else you would
suggest?

[esprehn, 2015-08-13 21:59:41]

On 2015/08/13 at 21:44:34, rmcilroy wrote:
> Elliott, would you be able to take a look or is there someone else you would
> suggest?

Sure I'll look this over

[rmcilroy, 2015-08-18 11:17:25]

> Sure I'll look this over

Thanks Elliott. Any chance you could have a look over this soon? I'd really like
to get this in behind a flag before the m46 branch if possible.

[rmcilroy, 2015-08-20 13:33:08]

Patchset #10 (id:180001) has been deleted

[rmcilroy, 2015-08-20 13:34:08]

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

[commit-bot: I haz the power, 2015-08-20 13:34:34]

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

[rmcilroy, 2015-08-20 13:39:09]

Updated to incorporate the spec change from deadline/isExceeded() to
timeRemaining() as discussed on
https://github.com/w3c/requestidlecallback/issues/11.

Elliott if you could take a look today so that I can do any changes UK time on
Friday I would much appreciated it. I'm happy to chat over VC and/or chat this
evening if you have any concerns / questions with the implementation.

[commit-bot: I haz the power, 2015-08-20 18:37:28]

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

[commit-bot: I haz the power, 2015-08-20 18:37:29]

Dry run: This issue passed the CQ dry run.

[esprehn, 2015-08-20 20:45:56]

https://codereview.chromium.org/1119683003/diff/220001/LayoutTests/virtual/th...
File LayoutTests/virtual/threaded/fast/idle-callback/README.txt (right):

https://codereview.chromium.org/1119683003/diff/220001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/fast/idle-callback/README.txt:2: #
--enable-threaded-compositing since the tests require threaded compositing
It sucks that regular tests don't run with a real scheduler, we really need to
switch to that and just not paint.

https://codereview.chromium.org/1119683003/diff/220001/LayoutTests/virtual/th...
File LayoutTests/virtual/threaded/fast/idle-callback/basic.html (right):

https://codereview.chromium.org/1119683003/diff/220001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/fast/idle-callback/basic.html:3: <head>
leave out hmtl, head, meta charset

https://codereview.chromium.org/1119683003/diff/220001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/fast/idle-callback/basic.html:62: </body>
leave out body and html

https://codereview.chromium.org/1119683003/diff/220001/LayoutTests/virtual/th...
File LayoutTests/virtual/threaded/fast/idle-callback/timeout.html (right):

https://codereview.chromium.org/1119683003/diff/220001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/fast/idle-callback/timeout.html:3: <head>
ditto for test simplification

https://codereview.chromium.org/1119683003/diff/220001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/fast/idle-callback/timeout.html:67:
busy_raf_loop_iterations_remaining--;
These variables are all named wrong, but I guess you plan to upstream the test?

https://codereview.chromium.org/1119683003/diff/220001/Source/core/dom/Script...
File Source/core/dom/ScriptedIdleTaskController.cpp (right):

https://codereview.chromium.org/1119683003/diff/220001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.cpp:107: while
(!m_pendingTimeouts.empty()) {
Vector<...> pendingTimeouts;
m_pendingTimeouts.swap(pendingTimeouts);
for (auto& id : pendingTimeouts)
  runCallback(id, -1, IdleCallbackDeadline::CallbackType::CalledByTimeout);

https://codereview.chromium.org/1119683003/diff/220001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.cpp:108:
runCallback(m_pendingTimeouts.front(), -1,
IdleCallbackDeadline::CallbackType::CalledByTimeout);
Why is the deadline -1?

https://codereview.chromium.org/1119683003/diff/220001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.cpp:113: for (auto callback =
m_callbacks.begin(); callback != m_callbacks.end();
for(auto& callback : m_callbacks)

https://codereview.chromium.org/1119683003/diff/220001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.cpp:132:
ScriptedIdleTaskController::IdleRequestCallbackWrapper::~IdleRequestCallbackWrapper()
Can we un nested the classes and put them in separate files?

https://codereview.chromium.org/1119683003/diff/220001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.cpp:150: if (m_timeoutTimer.get()) {
Why is this heap allocated?

The destructor should run stop(), and this should just do .clear()

https://codereview.chromium.org/1119683003/diff/220001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.cpp:162:
m_callbackWrapper->controller()->callbackFired(m_callbackWrapper.get(),
monotonicallyIncreasingTime(),
IdleCallbackDeadline::CallbackType::CalledByTimeout);
Can we just do this with methods on the controller instead? WHy do we need the
IdleRequestCallbackWrapper?

Normally we just use a Timer<> and .startOneShot()

https://codereview.chromium.org/1119683003/diff/220001/Source/core/dom/Script...
File Source/core/dom/ScriptedIdleTaskController.h (right):

https://codereview.chromium.org/1119683003/diff/220001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.h:61: class IdleRequestTimeoutTimer :
public Timer<IdleRequestTimeoutTimer> {
Woah doubly nested class is a little crazy, does this really need to be nested
inside IdleRequestCallbackWrapper?

https://codereview.chromium.org/1119683003/diff/220001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.h:63:
IdleRequestTimeoutTimer(PassRefPtr<IdleRequestCallbackWrapper>);
explicit

https://codereview.chromium.org/1119683003/diff/220001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.h:87: std::queue<CallbackId>
m_pendingTimeouts;
I think this can just be a Vector, why do you need to use a queue?

https://codereview.chromium.org/1119683003/diff/220001/Source/core/frame/Loca...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1119683003/diff/220001/Source/core/frame/Loca...
Source/core/frame/LocalDOMWindow.cpp:1268: return
document()->requestAnimationFrame(callback);
Why did you change this? document() makes a bunch of pointer hops, you want

if (Document* document = this->document())
  return document->requestAnimationFrame(callback);

https://codereview.chromium.org/1119683003/diff/220001/Source/core/frame/Loca...
Source/core/frame/LocalDOMWindow.cpp:1283: document()->cancelAnimationFrame(id);
ditto, don't change all these

https://codereview.chromium.org/1119683003/diff/220001/Source/core/frame/Loca...
Source/core/frame/LocalDOMWindow.cpp:1295: if (document())
ditto

[esprehn, 2015-08-20 21:52:40]

https://codereview.chromium.org/1119683003/diff/220001/Source/core/dom/Script...
File Source/core/dom/ScriptedIdleTaskController.h (right):

https://codereview.chromium.org/1119683003/diff/220001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.h:86:
PersistentHeapHashMapWillBeHeapHashMap<CallbackId, Member<IdleRequestCallback>>
m_callbacks;
I think you just want WillBeHeapHashMap, your object is on the heap, it doesn't
need to use Persistents.

[rmcilroy, 2015-08-21 00:03:45]

Thanks for the review Elliott. All comments addressed as discussed offline. If
you get another chance to give this a once over before tomorrow that would be
great.

https://codereview.chromium.org/1119683003/diff/220001/LayoutTests/virtual/th...
File LayoutTests/virtual/threaded/fast/idle-callback/README.txt (right):

https://codereview.chromium.org/1119683003/diff/220001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/fast/idle-callback/README.txt:2: #
--enable-threaded-compositing since the tests require threaded compositing
On 2015/08/20 20:45:55, esprehn wrote:
> It sucks that regular tests don't run with a real scheduler, we really need to
> switch to that and just not paint.

It does actually run with a real scheduler, it's just not got a real threaded
compositor, and it is the compositor which drives the idle period's in the
scheduler so without it we don't get any. Agree that we should switch over to a
real threaded compositor for layouttests though.

https://codereview.chromium.org/1119683003/diff/220001/LayoutTests/virtual/th...
File LayoutTests/virtual/threaded/fast/idle-callback/basic.html (right):

https://codereview.chromium.org/1119683003/diff/220001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/fast/idle-callback/basic.html:3: <head>
On 2015/08/20 20:45:55, esprehn wrote:
> leave out hmtl, head, meta charset 

Done.

https://codereview.chromium.org/1119683003/diff/220001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/fast/idle-callback/basic.html:62: </body>
On 2015/08/20 20:45:55, esprehn wrote:
> leave out body and html

Done.

https://codereview.chromium.org/1119683003/diff/220001/LayoutTests/virtual/th...
File LayoutTests/virtual/threaded/fast/idle-callback/timeout.html (right):

https://codereview.chromium.org/1119683003/diff/220001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/fast/idle-callback/timeout.html:3: <head>
On 2015/08/20 20:45:55, esprehn wrote:
> ditto for test simplification

Done.

https://codereview.chromium.org/1119683003/diff/220001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/fast/idle-callback/timeout.html:67:
busy_raf_loop_iterations_remaining--;
On 2015/08/20 20:45:55, esprehn wrote:
> These variables are all named wrong, but I guess you plan to upstream the
test?

Yes I am planning to upstream these tests so was following the style of the
other w3c tests that I found.

https://codereview.chromium.org/1119683003/diff/220001/Source/core/dom/Script...
File Source/core/dom/ScriptedIdleTaskController.cpp (right):

https://codereview.chromium.org/1119683003/diff/220001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.cpp:107: while
(!m_pendingTimeouts.empty()) {
On 2015/08/20 20:45:55, esprehn wrote:
> Vector<...> pendingTimeouts;
> m_pendingTimeouts.swap(pendingTimeouts);
> for (auto& id : pendingTimeouts)
>   runCallback(id, -1, IdleCallbackDeadline::CallbackType::CalledByTimeout);

Done.

https://codereview.chromium.org/1119683003/diff/220001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.cpp:108:
runCallback(m_pendingTimeouts.front(), -1,
IdleCallbackDeadline::CallbackType::CalledByTimeout);
On 2015/08/20 20:45:55, esprehn wrote:
> Why is the deadline -1?

Good catch, this was a leftover from the spec before the timeRemaining() change
- fixed.

https://codereview.chromium.org/1119683003/diff/220001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.cpp:113: for (auto callback =
m_callbacks.begin(); callback != m_callbacks.end();
On 2015/08/20 20:45:55, esprehn wrote:
> for(auto& callback : m_callbacks)

Done.

https://codereview.chromium.org/1119683003/diff/220001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.cpp:132:
ScriptedIdleTaskController::IdleRequestCallbackWrapper::~IdleRequestCallbackWrapper()
On 2015/08/20 20:45:55, esprehn wrote:
> Can we un nested the classes and put them in separate files?

I've removed the IdleRequestTimeoutTimer class entirely, and un-nested
IdleRequestCallbackWrapper (and removed it from the header), but I've kept it as
a private internal class in this cc file because it's an implementation detail
which shouldn't be used by anything else. Is this better?

https://codereview.chromium.org/1119683003/diff/220001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.cpp:150: if (m_timeoutTimer.get()) {
On 2015/08/20 20:45:55, esprehn wrote:
> Why is this heap allocated?
> 
> The destructor should run stop(), and this should just do .clear()

Removed entirely.

https://codereview.chromium.org/1119683003/diff/220001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.cpp:162:
m_callbackWrapper->controller()->callbackFired(m_callbackWrapper.get(),
monotonicallyIncreasingTime(),
IdleCallbackDeadline::CallbackType::CalledByTimeout);
On 2015/08/20 20:45:55, esprehn wrote:
> Can we just do this with methods on the controller instead? WHy do we need the
> IdleRequestCallbackWrapper?
> 
> Normally we just use a Timer<> and .startOneShot()

As discussed offline, changed this to be a bind() then post directly to the
scheduler's postTimerTask().

https://codereview.chromium.org/1119683003/diff/220001/Source/core/dom/Script...
File Source/core/dom/ScriptedIdleTaskController.h (right):

https://codereview.chromium.org/1119683003/diff/220001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.h:61: class IdleRequestTimeoutTimer :
public Timer<IdleRequestTimeoutTimer> {
On 2015/08/20 20:45:55, esprehn wrote:
> Woah doubly nested class is a little crazy, does this really need to be nested
> inside IdleRequestCallbackWrapper?

Removed.

https://codereview.chromium.org/1119683003/diff/220001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.h:63:
IdleRequestTimeoutTimer(PassRefPtr<IdleRequestCallbackWrapper>);
On 2015/08/20 20:45:55, esprehn wrote:
> explicit

Removed.

https://codereview.chromium.org/1119683003/diff/220001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.h:86:
PersistentHeapHashMapWillBeHeapHashMap<CallbackId, Member<IdleRequestCallback>>
m_callbacks;
On 2015/08/20 21:52:40, esprehn wrote:
> I think you just want WillBeHeapHashMap, your object is on the heap, it
doesn't
> need to use Persistents.

I get an Oilpan error if I do that:
 [blink-gc] Member field 'm_callbacks' in unmanaged class declared here:

I think it's because the ScriptedIdleTaskController isn't yet garbage collected
(it's RefCountedWillBeGarbageCollected).

https://codereview.chromium.org/1119683003/diff/220001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.h:87: std::queue<CallbackId>
m_pendingTimeouts;
On 2015/08/20 20:45:55, esprehn wrote:
> I think this can just be a Vector, why do you need to use a queue?

Done.

https://codereview.chromium.org/1119683003/diff/220001/Source/core/frame/Loca...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1119683003/diff/220001/Source/core/frame/Loca...
Source/core/frame/LocalDOMWindow.cpp:1268: return
document()->requestAnimationFrame(callback);
On 2015/08/20 20:45:55, esprehn wrote:
> Why did you change this? document() makes a bunch of pointer hops, you want
> 
> if (Document* document = this->document())
>   return document->requestAnimationFrame(callback);

Done, but I can't call the variable "document" since this conflicts with the
function name. Called doc instead.

https://codereview.chromium.org/1119683003/diff/220001/Source/core/frame/Loca...
Source/core/frame/LocalDOMWindow.cpp:1283: document()->cancelAnimationFrame(id);
On 2015/08/20 20:45:55, esprehn wrote:
> ditto, don't change all these

Ditto.

https://codereview.chromium.org/1119683003/diff/220001/Source/core/frame/Loca...
Source/core/frame/LocalDOMWindow.cpp:1295: if (document())
On 2015/08/20 20:45:55, esprehn wrote:
> ditto

Ditto.

[rmcilroy, 2015-08-21 00:04:19]

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

[commit-bot: I haz the power, 2015-08-21 00:04:45]

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

[commit-bot: I haz the power, 2015-08-21 02:42:54]

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

[commit-bot: I haz the power, 2015-08-21 02:42:54]

Dry run: This issue passed the CQ dry run.

[esprehn, 2015-08-21 08:22:21]

Please fix the going negative bug, timeRemaining needs to be >= 0. We also need
to get the timer clamping behavior added before we ship this, I guess it's okay
behind the flag...

lgtm

https://codereview.chromium.org/1119683003/diff/240001/LayoutTests/virtual/th...
File LayoutTests/virtual/threaded/fast/idle-callback/basic.html (right):

https://codereview.chromium.org/1119683003/diff/240001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/fast/idle-callback/basic.html:1:
<title>window.requestIdleCallback exists</title>
You need <!DOCTYPE html>

https://codereview.chromium.org/1119683003/diff/240001/LayoutTests/virtual/th...
File LayoutTests/virtual/threaded/fast/idle-callback/timeout.html (right):

https://codereview.chromium.org/1119683003/diff/240001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/fast/idle-callback/timeout.html:1:
<title>window.requestIdleCallback deals with timeouts correctly</title>
ditto

https://codereview.chromium.org/1119683003/diff/240001/Source/core/dom/IdleCa...
File Source/core/dom/IdleCallbackDeadline.cpp (right):

https://codereview.chromium.org/1119683003/diff/240001/Source/core/dom/IdleCa...
Source/core/dom/IdleCallbackDeadline.cpp:23: return m_deadlineMillis - now;
Won't this go negative after the deadline is hit? That'll make loops like:

while (thing.timeRemaining) { }

into infinite loops if you miss the 0 mark. I guess you're trying to show how
much over the deadline you went? Hmm, like it hitting 0 better.

https://codereview.chromium.org/1119683003/diff/240001/Source/core/dom/IdleCa...
File Source/core/dom/IdleCallbackDeadline.idl (right):

https://codereview.chromium.org/1119683003/diff/240001/Source/core/dom/IdleCa...
Source/core/dom/IdleCallbackDeadline.idl:9: double timeRemaining();
This should be a property I think.

https://codereview.chromium.org/1119683003/diff/240001/Source/core/dom/Script...
File Source/core/dom/ScriptedIdleTaskController.cpp (right):

https://codereview.chromium.org/1119683003/diff/240001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.cpp:35:
callbackWrapper->controller()->callbackFired(callbackWrapper->id(),
deadlineSeconds, IdleCallbackDeadline::CallbackType::CalledWhenIdle);
This doesn't have the clamping behavior where short tasks are always 2ms and
long tasks are always > 20-30ms to avoid some timing attacks. Is that
implemented in Chromium?

https://codereview.chromium.org/1119683003/diff/240001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.cpp:43:
ScriptedIdleTaskController::CallbackId id() { return m_id; }
const

https://codereview.chromium.org/1119683003/diff/240001/Source/core/frame/Loca...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1119683003/diff/240001/Source/core/frame/Loca...
Source/core/frame/LocalDOMWindow.cpp:1288: if (Document* doc = document())
you can actually use the variable name document, you need to write
this->document() (the this-> is important) to call the function. that's why I
wrote:

if (Document* document = this->document())
  ...

https://codereview.chromium.org/1119683003/diff/240001/Source/platform/Runtim...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/1119683003/diff/240001/Source/platform/Runtim...
Source/platform/RuntimeEnabledFeatures.in:134: RequestIdleCallback status=test
This won't be available under the experimental-web-platform-features flag. Were
you trying to have this actually in M46?

[rmcilroy, 2015-08-21 11:21:52]

All comments addressed. Sami, could you give this a final once-over and check
you're OK with the extra helper function in WebScheduler.

https://codereview.chromium.org/1119683003/diff/240001/LayoutTests/virtual/th...
File LayoutTests/virtual/threaded/fast/idle-callback/basic.html (right):

https://codereview.chromium.org/1119683003/diff/240001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/fast/idle-callback/basic.html:1:
<title>window.requestIdleCallback exists</title>
On 2015/08/21 08:22:20, esprehn wrote:
> You need <!DOCTYPE html>

Done.

https://codereview.chromium.org/1119683003/diff/240001/LayoutTests/virtual/th...
File LayoutTests/virtual/threaded/fast/idle-callback/timeout.html (right):

https://codereview.chromium.org/1119683003/diff/240001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/fast/idle-callback/timeout.html:1:
<title>window.requestIdleCallback deals with timeouts correctly</title>
On 2015/08/21 08:22:20, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/1119683003/diff/240001/Source/core/dom/IdleCa...
File Source/core/dom/IdleCallbackDeadline.cpp (right):

https://codereview.chromium.org/1119683003/diff/240001/Source/core/dom/IdleCa...
Source/core/dom/IdleCallbackDeadline.cpp:23: return m_deadlineMillis - now;
On 2015/08/21 08:22:20, esprehn wrote:
> Won't this go negative after the deadline is hit? That'll make loops like:
> 
> while (thing.timeRemaining) { }
> 
> into infinite loops if you miss the 0 mark. I guess you're trying to show how
> much over the deadline you went? Hmm, like it hitting 0 better.

Yeah, makes sense. Done (I'll update the spec to specify this too).

https://codereview.chromium.org/1119683003/diff/240001/Source/core/dom/IdleCa...
File Source/core/dom/IdleCallbackDeadline.idl (right):

https://codereview.chromium.org/1119683003/diff/240001/Source/core/dom/IdleCa...
Source/core/dom/IdleCallbackDeadline.idl:9: double timeRemaining();
On 2015/08/21 08:22:20, esprehn wrote:
> This should be a property I think.

Done (I'll also update the Spec as such).

https://codereview.chromium.org/1119683003/diff/240001/Source/core/dom/Script...
File Source/core/dom/ScriptedIdleTaskController.cpp (right):

https://codereview.chromium.org/1119683003/diff/240001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.cpp:35:
callbackWrapper->controller()->callbackFired(callbackWrapper->id(),
deadlineSeconds, IdleCallbackDeadline::CallbackType::CalledWhenIdle);
On 2015/08/21 08:22:21, esprehn wrote:
> This doesn't have the clamping behavior where short tasks are always 2ms and
> long tasks are always > 20-30ms to avoid some timing attacks. Is that
> implemented in Chromium?

I know that there is discussion on clamping to avoid timing attacks, but I don't
know if Chromium implements any of this yet. 

Ilya: Do you know about this?

Added a TODO for now since this will be behind a flag anyway.

https://codereview.chromium.org/1119683003/diff/240001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.cpp:43:
ScriptedIdleTaskController::CallbackId id() { return m_id; }
On 2015/08/21 08:22:21, esprehn wrote:
> const

Done.

https://codereview.chromium.org/1119683003/diff/240001/Source/core/frame/Loca...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1119683003/diff/240001/Source/core/frame/Loca...
Source/core/frame/LocalDOMWindow.cpp:1288: if (Document* doc = document())
On 2015/08/21 08:22:21, esprehn wrote:
> you can actually use the variable name document, you need to write
> this->document() (the this-> is important) to call the function. that's why I
> wrote:
> 
> if (Document* document = this->document())
>   ...

Ahh of course, thanks. Done.

https://codereview.chromium.org/1119683003/diff/240001/Source/platform/Runtim...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/1119683003/diff/240001/Source/platform/Runtim...
Source/platform/RuntimeEnabledFeatures.in:134: RequestIdleCallback status=test
On 2015/08/21 08:22:21, esprehn wrote:
> This won't be available under the experimental-web-platform-features flag.
Were
> you trying to have this actually in M46?

Done (I mistakenly thought status=experimental wouldn't be enabled in tests due
to the issues I was having with the non-threaded compositor used in tests.

[rmcilroy, 2015-08-21 11:21:52]

All comments addressed. Sami, could you give this a final once-over and check
you're OK with the extra helper function in WebScheduler.

https://codereview.chromium.org/1119683003/diff/240001/LayoutTests/virtual/th...
File LayoutTests/virtual/threaded/fast/idle-callback/basic.html (right):

https://codereview.chromium.org/1119683003/diff/240001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/fast/idle-callback/basic.html:1:
<title>window.requestIdleCallback exists</title>
On 2015/08/21 08:22:20, esprehn wrote:
> You need <!DOCTYPE html>

Done.

https://codereview.chromium.org/1119683003/diff/240001/LayoutTests/virtual/th...
File LayoutTests/virtual/threaded/fast/idle-callback/timeout.html (right):

https://codereview.chromium.org/1119683003/diff/240001/LayoutTests/virtual/th...
LayoutTests/virtual/threaded/fast/idle-callback/timeout.html:1:
<title>window.requestIdleCallback deals with timeouts correctly</title>
On 2015/08/21 08:22:20, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/1119683003/diff/240001/Source/core/dom/IdleCa...
File Source/core/dom/IdleCallbackDeadline.cpp (right):

https://codereview.chromium.org/1119683003/diff/240001/Source/core/dom/IdleCa...
Source/core/dom/IdleCallbackDeadline.cpp:23: return m_deadlineMillis - now;
On 2015/08/21 08:22:20, esprehn wrote:
> Won't this go negative after the deadline is hit? That'll make loops like:
> 
> while (thing.timeRemaining) { }
> 
> into infinite loops if you miss the 0 mark. I guess you're trying to show how
> much over the deadline you went? Hmm, like it hitting 0 better.

Yeah, makes sense. Done (I'll update the spec to specify this too).

https://codereview.chromium.org/1119683003/diff/240001/Source/core/dom/IdleCa...
File Source/core/dom/IdleCallbackDeadline.idl (right):

https://codereview.chromium.org/1119683003/diff/240001/Source/core/dom/IdleCa...
Source/core/dom/IdleCallbackDeadline.idl:9: double timeRemaining();
On 2015/08/21 08:22:20, esprehn wrote:
> This should be a property I think.

Done (I'll also update the Spec as such).

https://codereview.chromium.org/1119683003/diff/240001/Source/core/dom/Script...
File Source/core/dom/ScriptedIdleTaskController.cpp (right):

https://codereview.chromium.org/1119683003/diff/240001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.cpp:35:
callbackWrapper->controller()->callbackFired(callbackWrapper->id(),
deadlineSeconds, IdleCallbackDeadline::CallbackType::CalledWhenIdle);
On 2015/08/21 08:22:21, esprehn wrote:
> This doesn't have the clamping behavior where short tasks are always 2ms and
> long tasks are always > 20-30ms to avoid some timing attacks. Is that
> implemented in Chromium?

I know that there is discussion on clamping to avoid timing attacks, but I don't
know if Chromium implements any of this yet. 

Ilya: Do you know about this?

Added a TODO for now since this will be behind a flag anyway.

https://codereview.chromium.org/1119683003/diff/240001/Source/core/dom/Script...
Source/core/dom/ScriptedIdleTaskController.cpp:43:
ScriptedIdleTaskController::CallbackId id() { return m_id; }
On 2015/08/21 08:22:21, esprehn wrote:
> const

Done.

https://codereview.chromium.org/1119683003/diff/240001/Source/core/frame/Loca...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1119683003/diff/240001/Source/core/frame/Loca...
Source/core/frame/LocalDOMWindow.cpp:1288: if (Document* doc = document())
On 2015/08/21 08:22:21, esprehn wrote:
> you can actually use the variable name document, you need to write
> this->document() (the this-> is important) to call the function. that's why I
> wrote:
> 
> if (Document* document = this->document())
>   ...

Ahh of course, thanks. Done.

https://codereview.chromium.org/1119683003/diff/240001/Source/platform/Runtim...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/1119683003/diff/240001/Source/platform/Runtim...
Source/platform/RuntimeEnabledFeatures.in:134: RequestIdleCallback status=test
On 2015/08/21 08:22:21, esprehn wrote:
> This won't be available under the experimental-web-platform-features flag.
Were
> you trying to have this actually in M46?

Done (I mistakenly thought status=experimental wouldn't be enabled in tests due
to the issues I was having with the non-threaded compositor used in tests.

[rmcilroy, 2015-08-21 11:25:01]

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

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

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

[commit-bot: I haz the power, 2015-08-21 11:26:55]

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

[commit-bot: I haz the power, 2015-08-21 11:26:56]

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

[Sami, 2015-08-21 11:32:48]

Looks great. Posting the timeout as normal timer task is way cleaner. lgtm.

[rmcilroy, 2015-08-21 12:06:40]

On 2015/08/21 11:32:48, Sami wrote:
> Looks great. Posting the timeout as normal timer task is way cleaner. lgtm.

Thanks for the reviews. TBRing Jochen on minor public/platform/WebScheduler.h
change.

[rmcilroy, 2015-08-21 12:06:47]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-08-21 12:06:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
esprehn@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/1119683003/#ps280001
(title: "Rebased")

[commit-bot: I haz the power, 2015-08-21 12:07:04]

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

[commit-bot: I haz the power, 2015-08-21 13:21:52]

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

[commit-bot: I haz the power, 2015-08-21 13:21:53]

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

[rmcilroy, 2015-08-21 14:49:59]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2015-08-21 14:50:09]

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

[igrigorik, 2015-08-21 14:55:02]

>
https://codereview.chromium.org/1119683003/diff/240001/Source/core/dom/Script...
> Source/core/dom/ScriptedIdleTaskController.cpp:35:
> callbackWrapper->controller()->callbackFired(callbackWrapper->id(),
> deadlineSeconds, IdleCallbackDeadline::CallbackType::CalledWhenIdle);
> On 2015/08/21 08:22:21, esprehn wrote:
> > This doesn't have the clamping behavior where short tasks are always 2ms and
> > long tasks are always > 20-30ms to avoid some timing attacks. Is that
> > implemented in Chromium?
> 
> I know that there is discussion on clamping to avoid timing attacks, but I
don't
> know if Chromium implements any of this yet. 

We do, we clamp resolution of now() to 5us. 

> Ilya: Do you know about this? 
> Added a TODO for now since this will be behind a flag anyway.

What Elliott is highlighting here is a bit different: 
- For idle periods that are <20ms have a minimum reporting granularity of ~2ms -
e.g. 10ms, 8ms, 6ms.
- For long idle periods that are >20ms we can be precise.

The intent is to avoid giving extra information to raF-based timing attacks.

[commit-bot: I haz the power, 2015-08-21 17:04:31]

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

[commit-bot: I haz the power, 2015-08-21 17:04:32]

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

[rmcilroy, 2015-08-21 17:12:37]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2015-08-21 17:12:49]

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

[commit-bot: I haz the power, 2015-08-21 17:17:53]

Committed patchset #14 (id:280001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200992

[rmcilroy, 2015-08-21 17:24:08]

> What Elliott is highlighting here is a bit different: 
> - For idle periods that are <20ms have a minimum reporting granularity of ~2ms
-
> e.g. 10ms, 8ms, 6ms.
> - For long idle periods that are >20ms we can be precise.
> 
> The intent is to avoid giving extra information to raF-based timing attacks.

Is this something which has been discussed somewhere already or is it just an
idea at the moment? Personally 2ms seems a bit too granular, particularly since
the clamping of now() is only to 5us which is much smaller. In any case, This
should be something we add to the spec once we decide.