rAF: Introduce FrameRequestCallbackCollection for managing rAF callbacks.

rAF: Introduce FrameRequestCallbackCollection for managing rAF callbacks.

Rename the rAF callback from RequestAnimationFrameCallback to
FrameRequestCallback, since that's what it's called in the spec
(https://html.spec.whatwg.org/multipage/browsers.html#the-window-object).
Split out the functionality related to managing the rAF callbacks into a
separate FrameRequestCallbackCollection so that it can be used by
compositor-workers.

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

[sadrul, 2015-03-30 18:23:30]

sadrul@chromium.org changed reviewers:
+ dgozman@chromium.org, esprehn@chromium.org

[sadrul, 2015-03-30 18:23:31]

dgozman@chromium.org: Please review changes in inspector/

esprehn@chromium.org: Please review changes in dom/

[sadrul, 2015-03-30 18:25:00]

This is used for CompositorWorker in https://codereview.chromium.org/1047783002
(it's pretty small. Let me know if you think I should merge the two CLs into
one)

[esprehn, 2015-03-30 18:36:38]

Compositor worker doesn't call this thing requestAnimationFrame though?

https://codereview.chromium.org/1043903003/diff/1/Source/core/dom/RequestAnim...
File Source/core/dom/RequestAnimationFrameCallbackCollection.h (right):

https://codereview.chromium.org/1043903003/diff/1/Source/core/dom/RequestAnim...
Source/core/dom/RequestAnimationFrameCallbackCollection.h:15: class
RequestAnimationFrameCallbackCollection {
final

[sadrul, 2015-03-30 18:40:47]

On 2015/03/30 18:36:38, esprehn wrote:
> Compositor worker doesn't call this thing requestAnimationFrame though?

Indeed. My understanding is the correct name for RequestAnimationFrameCallback
is FrameRequestCallback (https://www.w3.org/Bugs/Public/show_bug.cgi?id=28152).
Once that bug is resolved, it will make sense for here. I can do that rename if
you think that would be appropriate?

https://codereview.chromium.org/1043903003/diff/1/Source/core/dom/RequestAnim...
File Source/core/dom/RequestAnimationFrameCallbackCollection.h (right):

https://codereview.chromium.org/1043903003/diff/1/Source/core/dom/RequestAnim...
Source/core/dom/RequestAnimationFrameCallbackCollection.h:15: class
RequestAnimationFrameCallbackCollection {
On 2015/03/30 18:36:38, esprehn wrote:
> final

Done.

[esprehn, 2015-03-30 18:44:59]

On 2015/03/30 at 18:40:47, sadrul wrote:
> On 2015/03/30 18:36:38, esprehn wrote:
> > Compositor worker doesn't call this thing requestAnimationFrame though?
> 
> Indeed. My understanding is the correct name for RequestAnimationFrameCallback
is FrameRequestCallback (https://www.w3.org/Bugs/Public/show_bug.cgi?id=28152).
Once that bug is resolved, it will make sense for here. I can do that rename if
you think that would be appropriate?
> 
>
https://codereview.chromium.org/1043903003/diff/1/Source/core/dom/RequestAnim...
> File Source/core/dom/RequestAnimationFrameCallbackCollection.h (right):
> 
>
https://codereview.chromium.org/1043903003/diff/1/Source/core/dom/RequestAnim...
> Source/core/dom/RequestAnimationFrameCallbackCollection.h:15: class
RequestAnimationFrameCallbackCollection {
> On 2015/03/30 18:36:38, esprehn wrote:
> > final
> 
> Done.

Yeah I'd prefer you renamed this to all be FrameRequestCallback (and the
bindings object for the callback too) either in this patch, or in the one where
compositor worker uses this feature.

[sadrul, 2015-03-30 19:03:48]

Patchset #3 (id:40001) has been deleted

[sadrul, 2015-03-30 19:34:09]

On 2015/03/30 18:44:59, esprehn wrote:
> On 2015/03/30 at 18:40:47, sadrul wrote:
> > On 2015/03/30 18:36:38, esprehn wrote:
> > > Compositor worker doesn't call this thing requestAnimationFrame though?
> > 
> > Indeed. My understanding is the correct name for
RequestAnimationFrameCallback
> is FrameRequestCallback
(https://www.w3.org/Bugs/Public/show_bug.cgi?id=28152).
> Once that bug is resolved, it will make sense for here. I can do that rename
if
> you think that would be appropriate?
> > 
> >
>
https://codereview.chromium.org/1043903003/diff/1/Source/core/dom/RequestAnim...
> > File Source/core/dom/RequestAnimationFrameCallbackCollection.h (right):
> > 
> >
>
https://codereview.chromium.org/1043903003/diff/1/Source/core/dom/RequestAnim...
> > Source/core/dom/RequestAnimationFrameCallbackCollection.h:15: class
> RequestAnimationFrameCallbackCollection {
> > On 2015/03/30 18:36:38, esprehn wrote:
> > > final
> > 
> > Done.
> 
> Yeah I'd prefer you renamed this to all be FrameRequestCallback (and the
> bindings object for the callback too) either in this patch, or in the one
where
> compositor worker uses this feature.

I have done the renaming in this CL. PTAL.

[dgozman, 2015-03-31 10:10:04]

dgozman@chromium.org changed reviewers:
+ caseq@chromium.org

[dgozman, 2015-03-31 10:10:05]

InspectorDOMDebugger looks good, over to caseq@ for InspectorTraceEvents.

https://codereview.chromium.org/1043903003/diff/100001/Source/core/inspector/...
File Source/core/inspector/InspectorTraceEvents.cpp (right):

https://codereview.chromium.org/1043903003/diff/100001/Source/core/inspector/...
Source/core/inspector/InspectorTraceEvents.cpp:429:
PassRefPtr<TraceEvent::ConvertableToTraceFormat>
InspectorAnimationFrameEvent::data(ExecutionContext* context, int callbackId)
Maybe this whole event makes sense only for document? +caseq@ to answer this.

[caseq, 2015-03-31 10:27:42]

https://codereview.chromium.org/1043903003/diff/100001/Source/core/dom/FrameR...
File Source/core/dom/FrameRequestCallbackCollection.cpp (right):

https://codereview.chromium.org/1043903003/diff/100001/Source/core/dom/FrameR...
Source/core/dom/FrameRequestCallbackCollection.cpp:27:
InspectorInstrumentation::didRequestAnimationFrame(m_context, id);
Here and 2 other InspectorInstrumentation calls below: is this guaranteed to be
called from the main thread?

https://codereview.chromium.org/1043903003/diff/100001/Source/core/inspector/...
File Source/core/inspector/InspectorTraceEvents.cpp (right):

https://codereview.chromium.org/1043903003/diff/100001/Source/core/inspector/...
Source/core/inspector/InspectorTraceEvents.cpp:429:
PassRefPtr<TraceEvent::ConvertableToTraceFormat>
InspectorAnimationFrameEvent::data(ExecutionContext* context, int callbackId)
On 2015/03/31 10:10:05, dgozman wrote:
> Maybe this whole event makes sense only for document? +caseq@ to answer this.

From what I understand, we can no longer assume the execution context is a
Document becuase the code is being generalized to handle compositor workers. If
so, the trace events should be fine(*), but I'm worried for
InspectorInstrumentation calls -- these may only be invoked by the main thread.

(*) we may still want some way of identifying the context, so we can tell apart
workers for different pages if they share the process, but I think this can be
deferred to a later stage, considering it would require some front-end support.

[sadrul, 2015-03-31 12:39:41]

https://codereview.chromium.org/1043903003/diff/100001/Source/core/dom/FrameR...
File Source/core/dom/FrameRequestCallbackCollection.cpp (right):

https://codereview.chromium.org/1043903003/diff/100001/Source/core/dom/FrameR...
Source/core/dom/FrameRequestCallbackCollection.cpp:27:
InspectorInstrumentation::didRequestAnimationFrame(m_context, id);
On 2015/03/31 10:27:42, caseq wrote:
> Here and 2 other InspectorInstrumentation calls below: is this guaranteed to
be
> called from the main thread?

For compositor-worker, these will be called from the worker-thread, not the
main-thread.

Are these supposed to be called only from the main-thread? Would you advise
checking isMainThread() before making these InspectorInstrumentation calls (at
least for the short term, I suspect we would want to eventually fix to work on
compositor-worker-thread too)? I notice that there are some
InspectorInstrumentation calls in core/workers/WorkerThread.cpp that also happen
from the non-main worker-thread. So I am not sure if only some of the calls to
InspectorInstrumentation are safe to be called from non-main thread or not.

https://codereview.chromium.org/1043903003/diff/100001/Source/core/inspector/...
File Source/core/inspector/InspectorTraceEvents.cpp (right):

https://codereview.chromium.org/1043903003/diff/100001/Source/core/inspector/...
Source/core/inspector/InspectorTraceEvents.cpp:429:
PassRefPtr<TraceEvent::ConvertableToTraceFormat>
InspectorAnimationFrameEvent::data(ExecutionContext* context, int callbackId)
On 2015/03/31 10:27:42, caseq wrote:
> On 2015/03/31 10:10:05, dgozman wrote:
> > Maybe this whole event makes sense only for document? +caseq@ to answer
this.
> 
> From what I understand, we can no longer assume the execution context is a
> Document becuase the code is being generalized to handle compositor workers.
If
> so, the trace events should be fine(*), but I'm worried for
> InspectorInstrumentation calls -- these may only be invoked by the main
thread.
> 
> (*) we may still want some way of identifying the context, so we can tell
apart
> workers for different pages if they share the process, but I think this can be
> deferred to a later stage, considering it would require some front-end
support.

Considering for the Document, we simply list the pointer to the frame, would it
make sense to do something similar for the worker like this:

  if (context->isDocument()) {
    value->SetString("frame", toHexString(toDocument(context)->frame()));
  } else if (context->isGlobalWorkerScope()) {
    value->SetString("worker", toHexString(toWorkerGlobalScope(context)));
  }

?

[caseq, 2015-03-31 13:25:32]

On 2015/03/31 12:39:41, sadrul wrote:
>
https://codereview.chromium.org/1043903003/diff/100001/Source/core/dom/FrameR...
> File Source/core/dom/FrameRequestCallbackCollection.cpp (right):
> 
>
https://codereview.chromium.org/1043903003/diff/100001/Source/core/dom/FrameR...
> Source/core/dom/FrameRequestCallbackCollection.cpp:27:
> InspectorInstrumentation::didRequestAnimationFrame(m_context, id);
> On 2015/03/31 10:27:42, caseq wrote:
> > Here and 2 other InspectorInstrumentation calls below: is this guaranteed to
> be
> > called from the main thread?
> 
> For compositor-worker, these will be called from the worker-thread, not the
> main-thread.
> 
> Are these supposed to be called only from the main-thread?

Never mind what I said, I had a closer look and it turns out we handle this
properly -- in case of worker, we'll fetch a different set of
InstrumentingAgents and will dispatch this to a proper instance of an agent. In
case of these methods, there won't be DOMDebugger and AsyncCallTracker in worker
instrumenting agents, so we will just bail out, which is I guess a reasonable
thing to do for now.

> Considering for the Document, we simply list the pointer to the frame, would
it
> make sense to do something similar for the worker like this:
> 
>   if (context->isDocument()) {
>     value->SetString("frame", toHexString(toDocument(context)->frame()));
>   } else if (context->isGlobalWorkerScope()) {
>     value->SetString("worker", toHexString(toWorkerGlobalScope(context)));
>   }

Yes, I had something similar in mind.

devtools lgtm

[sadrul, 2015-03-31 16:40:27]

https://codereview.chromium.org/1043903003/diff/100001/Source/core/inspector/...
File Source/core/inspector/InspectorTraceEvents.cpp (right):

https://codereview.chromium.org/1043903003/diff/100001/Source/core/inspector/...
Source/core/inspector/InspectorTraceEvents.cpp:429:
PassRefPtr<TraceEvent::ConvertableToTraceFormat>
InspectorAnimationFrameEvent::data(ExecutionContext* context, int callbackId)
On 2015/03/31 12:39:41, sadrul wrote:
> On 2015/03/31 10:27:42, caseq wrote:
> > On 2015/03/31 10:10:05, dgozman wrote:
> > > Maybe this whole event makes sense only for document? +caseq@ to answer
> this.
> > 
> > From what I understand, we can no longer assume the execution context is a
> > Document becuase the code is being generalized to handle compositor workers.
> If
> > so, the trace events should be fine(*), but I'm worried for
> > InspectorInstrumentation calls -- these may only be invoked by the main
> thread.
> > 
> > (*) we may still want some way of identifying the context, so we can tell
> apart
> > workers for different pages if they share the process, but I think this can
be
> > deferred to a later stage, considering it would require some front-end
> support.
> 
> Considering for the Document, we simply list the pointer to the frame, would
it
> make sense to do something similar for the worker like this:
> 
>   if (context->isDocument()) {
>     value->SetString("frame", toHexString(toDocument(context)->frame()));
>   } else if (context->isGlobalWorkerScope()) {
>     value->SetString("worker", toHexString(toWorkerGlobalScope(context)));
>   }
> 
> ?

Done.

[esprehn, 2015-04-01 05:03:25]

lgtm w/ empty -> isEmpty fixed. btw in the future I'd do renames and code
movement in different patches from data type changes like Document ->
ExecutionContext so they're easier to review.

https://codereview.chromium.org/1043903003/diff/120001/Source/core/dom/FrameR...
File Source/core/dom/FrameRequestCallbackCollection.h (right):

https://codereview.chromium.org/1043903003/diff/120001/Source/core/dom/FrameR...
Source/core/dom/FrameRequestCallbackCollection.h:24: bool empty() const { return
!m_callbacks.size(); }
isEmpty().

https://codereview.chromium.org/1043903003/diff/120001/Source/core/inspector/...
File Source/core/inspector/InspectorTraceEvents.cpp (right):

https://codereview.chromium.org/1043903003/diff/120001/Source/core/inspector/...
Source/core/inspector/InspectorTraceEvents.cpp:436: value->setString("worker",
toHexString(toWorkerGlobalScope(context)));
This seems unrelated to the rename, it's good to do renames in separate patches
so they can be reviewed faster as having no behavior changes. So for example I
would have made this new objet store a Document* and not an ExecutionContext*.
Landed this change, then did the datatype swap in a new patch.

[sadrul, 2015-04-01 06:38:53]

https://codereview.chromium.org/1043903003/diff/120001/Source/core/dom/FrameR...
File Source/core/dom/FrameRequestCallbackCollection.h (right):

https://codereview.chromium.org/1043903003/diff/120001/Source/core/dom/FrameR...
Source/core/dom/FrameRequestCallbackCollection.h:24: bool empty() const { return
!m_callbacks.size(); }
On 2015/04/01 05:03:25, esprehn wrote:
> isEmpty().

Done.

https://codereview.chromium.org/1043903003/diff/120001/Source/core/inspector/...
File Source/core/inspector/InspectorTraceEvents.cpp (right):

https://codereview.chromium.org/1043903003/diff/120001/Source/core/inspector/...
Source/core/inspector/InspectorTraceEvents.cpp:436: value->setString("worker",
toHexString(toWorkerGlobalScope(context)));
On 2015/04/01 05:03:25, esprehn wrote:
> This seems unrelated to the rename, it's good to do renames in separate
patches
> so they can be reviewed faster as having no behavior changes. So for example I
> would have made this new objet store a Document* and not an ExecutionContext*.
> Landed this change, then did the datatype swap in a new patch.

Acknowledged. Sorry. I will pay more attention to keeping CLs more focused on a
single change at a time.

[sadrul, 2015-04-01 06:41:59]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2015-04-01 06:41:59]

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

[commit-bot: I haz the power, 2015-04-01 06:42:32]

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

[commit-bot: I haz the power, 2015-04-01 11:18:13]

Committed patchset #7 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192918