Bug 53188 – requestAnimationFrame callbacks should not fire within a modal dialog

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[James Robinson]

requestAnimationFrame callbacks should not fire within a modal dialog

[James Robinson]

Created attachment 80234 [details]
Patch

[James Robinson]

Actually, it turns out that firefox does fire the callbacks under the modal dialog: https://bugzilla.mozilla.org/show_bug.cgi?id=629192, but that's a bug on their end.

[Mihai Parparita]

Comment on attachment 80234 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80234&action=review

> Source/WebCore/dom/Document.h:1409
> +    OwnPtr<RequestAnimationFrameController> m_requestAnimationFrameCallbacks;

Shouldn't this be called m_requestAnimationFrameController?

> Source/WebCore/dom/RequestAnimationFrameController.cpp:106
> +    // them until nothing new becomes visible.

Not sure I understand the "nothing new becomes visible" stopping condition. From reading the code, it looks like you keep going until you've fired all callbacks that weren't cancelled or attached to an invisible element.

[Alexey Proskuryakov]

This is quite related to what happens for media elements, so CC'ing Eric Carlson. It's not really clear if the principle of "nothing observable from JS should happen asynchronously, and in particular below modal dialogs" is still sustainable.

Media elements have to update things like current time asynchronously, and at this point, you've already violated the general principle, and can as well invoke event handlers.

[Simon Fraser (smfr)]

Comment on attachment 80234 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80234&action=review

> Source/WebCore/dom/Document.cpp:4963
> +        m_requestAnimationFrameCallbacks = new RequestAnimationFrameController(this);

We avoid uses of bare 'new' now in favor of adoptPtr().

[James Robinson]

Comment on attachment 80234 [details]
Patch

Thanks for the feedback, I'll fix the issues raised and post a new patch.

[James Robinson]

Created attachment 80510 [details]
Patch

[James Robinson]

Updated to address review feedback.  There appears to still be debate about whether ActiveDOMObject is the ideal mechanism for this (see bugs 53202 and 24030), but resolving that seems mostly orthogonal to this bugfix.  When/if ActiveDOMObject is adjusted this code can be migrated similarly.

[Alexey Proskuryakov]

Comment on attachment 80510 [details]
Patch

As discussed on IRC, it's not at all obvious that this should be done at all. I suggest a webkit-dev discussion.

[Alexey Proskuryakov]

On a technical side, I strongly disagree with further abusing ActiveDOMObject.

[James Robinson]

(In reply to comment #9)
> (From update of attachment 80510 [details])
> As discussed on IRC, it's not at all obvious that this should be done at all. I suggest a webkit-dev discussion.

I disagree (as does mozilla) - the default for new APIs should definitely be not to violate run-to-completion.  I don't think this is controversial, regardless of the desired behavior for legacy APIs that may have compatibility constraints.

[Darin Fisher (:fishd, Google)]

(In reply to comment #11)
> (In reply to comment #9)
> > (From update of attachment 80510 [details] [details])
> > As discussed on IRC, it's not at all obvious that this should be done at all. I suggest a webkit-dev discussion.
> 
> I disagree (as does mozilla) - the default for new APIs should definitely be not to violate run-to-completion.  I don't think this is controversial, regardless of the desired behavior for legacy APIs that may have compatibility constraints.

Ditto.  It is very problematic to violate run-to-completion.  Alexey, if your goal is to allow animations to work nicely while an alert() is showing, then I disagree with that goal.  It is not worth the risk.  Page authors shouldn't have to write re-entrant safe code just because some library decides to pop an alert!

[Darin Fisher (:fishd, Google)]

(In reply to comment #4)
> This is quite related to what happens for media elements, so CC'ing Eric Carlson. It's not really clear if the principle of "nothing observable from JS should happen asynchronously, and in particular below modal dialogs" is still sustainable.
> 
> Media elements have to update things like current time asynchronously, and at this point, you've already violated the general principle, and can as well invoke event handlers.

I think there is a difference between updating DOM object state while an alert is showing and allowing javascript to execute under an alert.  The former can certainly be confusing but the latter requires unsuspecting code to be re-entrant!

[Alexey Proskuryakov]

The theory that I heard from Mozilla folks was that state (including cookies!) also shouldn't change.

What I see here is that a lot of effort is being poured into a theory that doesn't stand very well, including both implementation changes and spec ones (like the dreaded storage mutex). Some of the theory's requirements contradict common sense (like media elements' current time attribute behavior), or make user experience worse (what's wrong with animations playing nicely)?

I think that changes in this area shouldn't be rushed, and may warrant a wider webkit-dev discussion.

> Page authors shouldn't have to write re-entrant safe code just because some library decides to pop an alert!

We can't fully avoid that - a library can call showModalDialog, whose code can dispatch events in the opener etc.

It is also not clear whether we should disable some other events below modal alerts and dialogs. One example that came to my mind is orientation events - I expect a page displaying an alert to rotate when I rotate my iPhone, and orientation events by design are needed to properly rotate. Others may come up with more examples.

There are contradicting arguments to make here. On one hand, making reentrancy less frequent makes problems less frequent. On the other hand, it makes such problem less known to us and web developers, leading to more instability and potentially exploitable crashes.

[James Robinson]

(In reply to comment #14)
> The theory that I heard from Mozilla folks was that state (including cookies!) also shouldn't change.
> 
> What I see here is that a lot of effort is being poured into a theory that doesn't stand very well, including both implementation changes and spec ones (like the dreaded storage mutex). Some of the theory's requirements contradict common sense (like media elements' current time attribute behavior), or make user experience worse (what's wrong with animations playing nicely)?
> 
> I think that changes in this area shouldn't be rushed, and may warrant a wider webkit-dev discussion.

Are you saying that you disagree with run-to-completion?  This behavior is clearly specced (see http://www.whatwg.org/specs/web-apps/current-work/multipage/timers.html#dom-alert and http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#pause) so if you think this is unwise I'd suggest raising it in the WHATWG.

> 
> > Page authors shouldn't have to write re-entrant safe code just because some library decides to pop an alert!
> 
> We can't fully avoid that - a library can call showModalDialog, whose code can dispatch events in the opener etc.
> 
> It is also not clear whether we should disable some other events below modal alerts and dialogs. One example that came to my mind is orientation events - I expect a page displaying an alert to rotate when I rotate my iPhone, and orientation events by design are needed to properly rotate. Others may come up with more examples.

Why would orientation events be treated differently from any other sort of event that may trigger a change to the UI?  Authors who want a responsive UI should, obviously, minimize their use of modal dialogs.  I don't see why this sort of event requires special treatment.

> 
> There are contradicting arguments to make here. On one hand, making reentrancy less frequent makes problems less frequent. On the other hand, it makes such problem less known to us and web developers, leading to more instability and potentially exploitable crashes.

In cases where it is easy and straightforward to avoid reentrancy (such as this one) I think it's best to avoid it.

[Simon Fraser (smfr)]

It seems like the real answer is a non-blocking dialog API.

[James Robinson]

(In reply to comment #16)
> It seems like the real answer is a non-blocking dialog API.

Definitely, but we would still need to deal with legacy code even if we had a non-blocking dialog API.

[James Robinson]

Created attachment 80618 [details]
adds a test

[James Robinson]

Adds a layout test.  If you still object to the use of ActiveDOMObject, Alexey, please let me know what specifically you have a problem with so that I can address it and land this bugfix.  I don't think the overhead should be a problem given that there will be at most one RequestAnimationFrameController per Document.

[Alexey Proskuryakov]

Comment on attachment 80618 [details]
adds a test

ActiveDOMObject shouldn't be used with objects that don't have a JS wrapper.

Darin mentioned that this fix breaks "nice animations". If so, just do this. We can revisit this after Firefox 5 comes out, and if it really has this fix.

[James Robinson]

Created attachment 80622 [details]
without ActiveDOMObject

[James Robinson]

(In reply to comment #20)
> (From update of attachment 80618 [details])
> ActiveDOMObject shouldn't be used with objects that don't have a JS wrapper.
> 

I've posted a new patch that does not use ActiveDOMObject.  If you prefer we can use this pattern for EventQueue as well.

> Darin mentioned that this fix breaks "nice animations". If so, just do this. We can revisit this after Firefox 5 comes out, and if it really has this fix.

I'm not sure exactly what you are referring to here.  This patch makes animations driven by webkitRequestAnimationFrame() behave identically to those driven by setTimeout().  As webkitRequestAnimationFrame is intended to be a drop-in replacement for setTimeout() for imperative animations, this is exactly the behavior we want.

[Alexey Proskuryakov]

Comment on attachment 80622 [details]
without ActiveDOMObject

View in context: https://bugs.webkit.org/attachment.cgi?id=80622&action=review

It may be unfair to pick on a single patch to question run to completion, and maybe my comments sound out of context because of that. But I do think that these issues need to be discussed in the WebKit community, and this time is as good as any. There doesn't seem to be any rush - I can see no indication that Mozilla will be changing this behavior before Firefox 5.

> Source/WebCore/dom/Document.cpp:4741
> +void Document::suspendActiveDOMObjects(ActiveDOMObject::ReasonForSuspension reason)
> +{
> +    ScriptExecutionContext::suspendActiveDOMObjects(reason);
> +#if ENABLE(REQUEST_ANIMATION_FRAME)
> +    if (m_requestAnimationFrameController)
> +        m_requestAnimationFrameController->suspend();
> +#endif
> +}

I think it should be obvious that this is a no-go. A function called suspendActiveDOMObjects() cannot be affecting things that aren't ActiveDOMObjects.

[James Robinson]

Comment on attachment 80622 [details]
without ActiveDOMObject

I think it's unreasonable to block a bugfix because you want to debate the execution model of the web.  Feel free to start such a discussion, but in the meantime I strongly believe that requestAnimationFrame callbacks should not fire while modal dialogs are up.

[Alexey Proskuryakov]

Comment on attachment 80622 [details]
without ActiveDOMObject

This patch already has r-, please don't set it back to r? just because you disagree.

[Darin Fisher (:fishd, Google)]

(In reply to comment #23)
> (From update of attachment 80622 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80622&action=review
> 
> It may be unfair to pick on a single patch to question run to completion, and maybe my comments sound out of context because of that. But I do think that these issues need to be discussed in the WebKit community, and this time is as good as any. There doesn't seem to be any rush - I can see no indication that Mozilla will be changing this behavior before Firefox 5.

Alexey, I think you are putting a bit too much emphasis on matching Firefox behavior.  The Mozilla code is fairly inconsistent about enforcing run-to-completion.  That's not because of any intentional desire by the coders.

Instead, I think we should focus on making WebKit good instead of just defacto matching Firefox.  There are real problems impacting WebKit and web applications if we allow JS code to run underneath an alert.  (I can go on at length about some of the weird problems if you like.)  We should prevent it as a means of making WebKit more sane and reducing complexity for ourselves.

Moreover, we should not try so hard to make the web platform "awesome" when an alert dialog is showing because alert is a horrifically bad API that'll never work great.  We should instead put our energy into making the web platform awesome when an alert dialog is NOT showing, and we should complement that by adding an API for an asynchronous alert dialogs.

Enforcing run-to-completion consistently has been the status-quo for WebKit.  I was always impressed to see how well this was implemented (again referring to how messy it is in Mozilla), and I think it makes sense for run-to-completion to remain the default behavior for WebKit.  If we need to violate that in a particular scenario, then we should have a thoughtful discussion about why.

[James Robinson]

I doubt many web authors (especially library authors) will be excited about adopting this API instead of setTimeout if it means they have to defend against arbitrary reentrancy in their animation systems.

[Alexey Proskuryakov]

You may be right, or they could be excited that this API actually provides a benefit over setInterval in that the animation doesn't stop below modal dialogs.

[Simon Fraser (smfr)]

I don't think most users of this API will give a hoot about modal dialogs.

[James Robinson]

(In reply to comment #29)
> I don't think most users of this API will give a hoot about modal dialogs.

Users who write animation libraries (like jquery, scriptaculus, etc) will have to worry about making their code robust.

[Simon Fraser (smfr)]

(In reply to comment #30)
> (In reply to comment #29)
> > I don't think most users of this API will give a hoot about modal dialogs.
> 
> Users who write animation libraries (like jquery, scriptaculus, etc) will have to worry about making their code robust.

Right, but I don't think they'll complain if the animation stops while an alert is up. They should be smart enough to not use modal alerts.

[James Robinson]

(In reply to comment #31)
> (In reply to comment #30)
> > (In reply to comment #29)
> > > I don't think most users of this API will give a hoot about modal dialogs.
> > 
> > Users who write animation libraries (like jquery, scriptaculus, etc) will have to worry about making their code robust.
> 
> Right, but I don't think they'll complain if the animation stops while an alert is up. They should be smart enough to not use modal alerts.

Ah yes - I agree completely :)

[Simon Fraser (smfr)]

Comment on attachment 80622 [details]
without ActiveDOMObject

View in context: https://bugs.webkit.org/attachment.cgi?id=80622&action=review

I think the patch is fine other than some naming issues. Leaving the r- so we can see another patch.

If we need to reconsider allowing animations to run under modal dialogs, that should be another bug.

>> Source/WebCore/dom/Document.cpp:4741
>> +}
> 
> I think it should be obvious that this is a no-go. A function called suspendActiveDOMObjects() cannot be affecting things that aren't ActiveDOMObjects.

Agreed. This method could be renamed, or you could create a wrapper method.

> Source/WebCore/dom/Document.h:1413
> +    OwnPtr<RequestAnimationFrameController> m_requestAnimationFrameController;

I think this could be called m_animationFrameController, or m_scriptedAnimationController.

> Source/WebCore/dom/RequestAnimationFrameController.cpp:53
> +    m_suspended = true;

Should you allow nesting?

> Source/WebCore/dom/RequestAnimationFrameController.cpp:58
> +    m_suspended = false;

Is it OK if we're not suspended?

> Source/WebCore/dom/RequestAnimationFrameController.h:42
> +class RequestAnimationFrameController {

Maybe rename to AnimationFrameController, or ScriptedAnimationController?

> Source/WebCore/dom/RequestAnimationFrameController.h:51
> +    int registerCallback(PassRefPtr<RequestAnimationFrameCallback>, Element*);

Consider a typedef for the return value?

> Source/WebCore/dom/RequestAnimationFrameController.h:52
> +    void cancelCallback(int id);

No need for 'id'

> Source/WebCore/dom/RequestAnimationFrameController.h:53
> +    void serviceScriptedAnimations(DOMTimeStamp);

Will there be kinds other than scripted?

> Source/WebCore/dom/RequestAnimationFrameController.h:65
> +    int m_nextCallbackId;
> +    bool m_suspended;
> +    Document* m_document;

Put m_document first for better alignment.

[Alexey Proskuryakov]

I still object to making changes like this without a webkit-dev discussion.

[James Robinson]

(In reply to comment #34)
> I still object to making changes like this without a webkit-dev discussion.

Then please start the discussion on webkit-dev.

[James Robinson]

(In reply to comment #33)
> (From update of attachment 80622 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80622&action=review
> 
> I think the patch is fine other than some naming issues. Leaving the r- so we can see another patch.
> 
> If we need to reconsider allowing animations to run under modal dialogs, that should be another bug.
> 
> >> Source/WebCore/dom/Document.cpp:4741
> >> +}
> > 
> > I think it should be obvious that this is a no-go. A function called suspendActiveDOMObjects() cannot be affecting things that aren't ActiveDOMObjects.
> 
> Agreed. This method could be renamed, or you could create a wrapper method.

I'll add a wrapper method to separate the ActiveDOMObject concerns from suspending callbacks that aren't ActiveDOMObjects.

> 
> > Source/WebCore/dom/Document.h:1413
> > +    OwnPtr<RequestAnimationFrameController> m_requestAnimationFrameController;
> 
> I think this could be called m_animationFrameController, or m_scriptedAnimationController.
> 
> > Source/WebCore/dom/RequestAnimationFrameController.h:42
> > +class RequestAnimationFrameController {
> 
> Maybe rename to AnimationFrameController, or ScriptedAnimationController?
>
> > Source/WebCore/dom/RequestAnimationFrameController.h:53
> > +    void serviceScriptedAnimations(DOMTimeStamp);
> 
> Will there be kinds other than scripted?

I like ScriptedAnimationController, I'll rename it to that.  I do not anticipate that we'll use this mechanism for anything other than scripted animations.

> > Source/WebCore/dom/RequestAnimationFrameController.cpp:53
> > +    m_suspended = true;
> 
> Should you allow nesting?
> 
> > Source/WebCore/dom/RequestAnimationFrameController.cpp:58
> > +    m_suspended = false;
> 
> Is it OK if we're not suspended?

I'll change this to a suspension count so it properly handles nesting (i.e. if there are multiple suspend() calls, wait until a matching number of resume() calls are made before firing callbacks again).

[James Robinson]

Created attachment 81690 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 81690 [details]
Patch

Automatic r-.

[Alexey Proskuryakov]

I thought that is was obvious from previous comments, but apparently it wasn't. The reasons for r- are all the same, as the patch still plugs into ActiveDOMObject code for something different. Per IRC discussion, I see two ways to proceed:

(1) enumerate the use cases for "suspending", and then create a design for suspending things that agrees with reality.
(2) hook this code into a different place, getting rid of a pretense that you're using some established generic mechanism that exists in WebCore (it doesn't exist).

[James Robinson]

(In reply to comment #39)
> I thought that is was obvious from previous comments, but apparently it wasn't. The reasons for r- are all the same, as the patch still plugs into ActiveDOMObject code for something different. Per IRC discussion, I see two ways to proceed:
> 
> (1) enumerate the use cases for "suspending", and then create a design for suspending things that agrees with reality.
> (2) hook this code into a different place, getting rid of a pretense that you're using some established generic mechanism that exists in WebCore (it doesn't exist).

I don't see why either of these (are they supposed to be independent?) should block this patch.  This patch integrates with the suspend/resume system we have today.  If we come up with an alternate suspend/resume mechanism then so long as the interaction with the Document is the same, this patch will apply.

I do think it's completely unreasonable to block this patch on creating a replacement suspend/resume mechanism.

[Maciej Stachowiak]

Does this bug have an actual end-user symptom? James mentioned on IRC that this "breaks the js debugger", but that info is nowhere in the bug:

<jamesr_> ap: currently i'm in a situation where this API breaks the js debugger, because it does not suspend as it should.

Can someone please confirm whether there is debugger breakage, and more specifically what is broken (e.g. steps to reproduce)?

[James Robinson]

(In reply to comment #41)
> Does this bug have an actual end-user symptom? James mentioned on IRC that this "breaks the js debugger", but that info is nowhere in the bug:
> 
> <jamesr_> ap: currently i'm in a situation where this API breaks the js debugger, because it does not suspend as it should.
> 
> Can someone please confirm whether there is debugger breakage, and more specifically what is broken (e.g. steps to reproduce)?

Without this patch anything that relies on the suspend/resume mechanism breaks.  This means, in particular, if a page is using webkitRequestAnimationFrame and the page opens a modal dialog or someone attempts to pause execution using the Inspector's JS debugger, or an external JS debugger, the page breaks because the callbacks continue firing.  Try setting a breakpoint within a requestAnimationFrame callback itself to see what happens (hint: it's not pretty).

This makes it pretty much impossible to develop pages using the new API, which ap apparently does not think is valuable anyway.

[Alexey Proskuryakov]

> This patch integrates with the suspend/resume system we have today.

There is no real system to speak of. Adding things to it is unhelpful.

You're saying that these ScriptExecutionContext suspend/resume calls are made at exactly the right times now, why not just make animation suspend/resume calls on the next line? That way, they won't be affected when ScriptExecutionContext calls change.

[Darin Fisher (:fishd, Google)]

(In reply to comment #43)
> > This patch integrates with the suspend/resume system we have today.
> 
> There is no real system to speak of. Adding things to it is unhelpful.
> 
> You're saying that these ScriptExecutionContext suspend/resume calls are made at exactly the right times now, why not just make animation suspend/resume calls on the next line? That way, they won't be affected when ScriptExecutionContext calls change.

^^^ This seems like a reasonable approach to me.  One could generalize it so that at the callsite we only need a pair of suspend/resumeThingers methods, so as to create a common pinchpoint, but maybe that is not necessary.

[Alexey Proskuryakov]

I'd feel uncomfortable about generalized suspend/resumeThingers without a design enumerating use cases and how they fit in the model. We almost never do this for WebKit, but the multiple year confusion in this area makes me feel that one is needed.

[Maciej Stachowiak]

(In reply to comment #45)
> I'd feel uncomfortable about generalized suspend/resumeThingers without a design enumerating use cases and how they fit in the model. We almost never do this for WebKit, but the multiple year confusion in this area makes me feel that one is needed.

Since James clarified that there is a real and urgent end-user symptom to this issue, I think it is reasonable to do a stopgap fix and then have that wider discussion.

If there wasn't a concrete symptom (something I didn't know until today), then I'd agree that we should get the design straight first.

One thought: in the long run, perhaps the debugger should stall/suspend more things than a modal dialog would, since in that case, there truly is an expectation of freezing the world. Even CSS transitions, CSS animations and media should stop when you are at a debugger breakpoint, I would presume.

[James Robinson]

(In reply to comment #46)
> (In reply to comment #45)
> > I'd feel uncomfortable about generalized suspend/resumeThingers without a design enumerating use cases and how they fit in the model. We almost never do this for WebKit, but the multiple year confusion in this area makes me feel that one is needed.
> 
> Since James clarified that there is a real and urgent end-user symptom to this issue, I think it is reasonable to do a stopgap fix and then have that wider discussion.

Would you mind reviewing the patch as it is now, or suggesting other measures that should be taken at this point rather than later?  I'd still like to land something ASAP.


> 
> If there wasn't a concrete symptom (something I didn't know until today), then I'd agree that we should get the design straight first.
> 
> One thought: in the long run, perhaps the debugger should stall/suspend more things than a modal dialog would, since in that case, there truly is an expectation of freezing the world. Even CSS transitions, CSS animations and media should stop when you are at a debugger breakpoint, I would presume.

[Alexey Proskuryakov]

I talked to Darin Fisher, and he agreed that (2) is the way to go as a stopgap fix. Please don't override the r-.

[James Robinson]

Created attachment 82528 [details]
with explicit extra suspend/resume calls

[James Robinson]

This implements what I believe you suggested.  In effect, every* call to (suspend|resume)ActiveDOMObjects() now makes an additional call to (suspend|resume)Callbacks().

*qt appears to expose qt_suspendActiveDOMObjects() and qt_resumeActiveDOMObjects(). As far as I can tell these functions are uncalled - there are no in-tree callers and google code search can't find any references to these outside the WebKit project - but I'll leave the implementation along until someone with familiarity with the Qt port can comment on what these functions are supposed to do.

[Alexey Proskuryakov]

Comment on attachment 82528 [details]
with explicit extra suspend/resume calls

+void Document::suspendCallbacks()
+{
+#if ENABLE(REQUEST_ANIMATION_FRAME)
+    if (m_scriptedAnimationController)
+        m_scriptedAnimationController->suspend();
+#endif
+}

What is the meaning of this function? Why isn't it called suspendScriptedAnimationControllerCallbacks? I think that it should be.

[James Robinson]

(In reply to comment #51)
> (From update of attachment 82528 [details])
> +void Document::suspendCallbacks()
> +{
> +#if ENABLE(REQUEST_ANIMATION_FRAME)
> +    if (m_scriptedAnimationController)
> +        m_scriptedAnimationController->suspend();
> +#endif
> +}
> 
> What is the meaning of this function? Why isn't it called suspendScriptedAnimationControllerCallbacks? I think that it should be.

That's a good description of what this function does today.  I think as far as the ScriptExecutionContext interface is concerned, the important part for callers to know is that invoking the suspendCallbacks() function prevents callbacks of any sort from running on the context and the resumeCallbacks() does the inverse, and it shouldn't be a concern of the caller what the nature of the callbacks is.

I'd be happy to rename the function if it will unblock landing this.

[James Robinson]

Created attachment 82534 [details]
with rename

[Alexey Proskuryakov]

Thank you for renaming the functions. "Callback" can mean a lot of things in different contexts, so looking at a "suspendCallbacks" function doesn't really tell one anything about what it does or when it should be called.

For example, currently there are suspendPostAttachCallbacks(), unregisterForDocumentActivationCallbacks(), unregisterForMediaVolumeCallbacks(), and argument callbacks in processArguments() - and that's in Document alone.

+        virtual void suspendCallbacks() {}
+        virtual void resumeCallbacks() {}

There should be spaces between braces.

+ * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY

Pelase use a correct license block.

+#include "config.h"
+
+#include "ScriptedAnimationController.h"

There shouldn't be a space between these lines.

+    m_suspendCount++;

There is no reason to use post increment here, so stylistically it's slightly nicer to use pre-increment.

+    m_suspendCount--;

Ditto.

+        if (FrameView* fv = m_document->view())

Please don't abbreviate frameView.

I don't understand how this gets away with only implementing suspend/resume and not stop, but OK.

+        if (FrameView* v = m_document->view())

Please don't abbreviate.

+} // namespace WebCore

As discussed on webkit-dev, it's best to avoid these comments for whole file namespaces. The only time they can useful, you are not going to trust them anyway, and normally it's just noise.

+ * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY

Please use a correct license block.

+    static PassOwnPtr<ScriptedAnimationController> create(Document* d)

Please don't a.

I couldn't find what protects RequestAnimationFrameCallback from garbage collection. I expected to see that code in JSDocumentCustom.cpp.

[James Robinson]

(In reply to comment #54)
> Thank you for renaming the functions. "Callback" can mean a lot of things in different contexts, so looking at a "suspendCallbacks" function doesn't really tell one anything about what it does or when it should be called.
> 
> For example, currently there are suspendPostAttachCallbacks(), unregisterForDocumentActivationCallbacks(), unregisterForMediaVolumeCallbacks(), and argument callbacks in processArguments() - and that's in Document alone.
> 
> +        virtual void suspendCallbacks() {}
> +        virtual void resumeCallbacks() {}
> 
> There should be spaces between braces.
> 
> + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY
> 
> Pelase use a correct license block.

I believe I am using the correct license block.  Which part is incorrect?

> I couldn't find what protects RequestAnimationFrameCallback from garbage collection. I expected to see that code in JSDocumentCustom.cpp.

The RequestAnimationFrameCallback item is held by a reference count held by the ScriptedAnimationController, which is owned (via an OwnPtr<>) by the Document object.  The javascript wrapper for the RequestAnimationFrameCallback is not currently protected from GC.  I plan to address that issue ASAP (as a separate patch).

I'll address the rest of the comments and post a new patch.

[James Robinson]

Created attachment 82535 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 82535 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82535&action=review

> I believe I am using the correct license block.  Which part is incorrect?

Apple Computer, Inc. has been renamed to Apple, Inc. some time ago. We have the canonical text of the license at <http://www.webkit.org/coding/bsd-license.html>, although I think that most Google contributors use a slightly different one. I'd like to know where it comes from, so that I could point that out in review more specifically than "please ask your manager".

> I plan to address that issue ASAP (as a separate patch).

Yes, the GC issue is certainly for a separate patch.

> Source/WebCore/dom/Document.cpp:4917
> +    return m_scriptedAnimationController->registerCallback(callback, e);

Missed this one - please don't abbreviate "element". This could actually use a name that explains what element it is (targetElement? animationElement? rootElement?)

> Source/WebCore/dom/ScriptedAnimationController.cpp:29
> +#include "config.h"
> +
> +#include "ScriptedAnimationController.h"

This still has an extra newline.

> Source/WebCore/dom/ScriptedAnimationController.cpp:60
> +ScriptedAnimationController::CallbackId ScriptedAnimationController::registerCallback(PassRefPtr<RequestAnimationFrameCallback> callback, Element* e)

Please don't abbreviate "element"

> Source/WebCore/dom/ScriptedAnimationController.h:69
> +} // namespace WebCore

This still has the comment. Not a big deal - I think that many people missed the webkit-dev discussion, and still land new code with these comments.

[Darin Adler]

(In reply to comment #57)
> Apple Computer, Inc. has been renamed to Apple, Inc. some time ago.

Not important but: Apple Inc. without a comma.

[James Robinson]

(In reply to comment #57)
> (From update of attachment 82535 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82535&action=review
> 
> > I believe I am using the correct license block.  Which part is incorrect?
> 
> Apple Computer, Inc. has been renamed to Apple, Inc. some time ago. We have the canonical text of the license at <http://www.webkit.org/coding/bsd-license.html>, although I think that most Google contributors use a slightly different one. I'd like to know where it comes from, so that I could point that out in review more specifically than "please ask your manager".

Gotcha.  I was using an internal reference which predates http://trac.webkit.org/changeset/50154.  I'll make sure to update the reference for future Googlers.

> > Source/WebCore/dom/Document.cpp:4917
> > +    return m_scriptedAnimationController->registerCallback(callback, e);
> 
> Missed this one - please don't abbreviate "element". This could actually use a name that explains what element it is (targetElement? animationElement? rootElement?)
> 
> > Source/WebCore/dom/ScriptedAnimationController.cpp:29
> > +#include "config.h"
> > +
> > +#include "ScriptedAnimationController.h"
> 
> This still has an extra newline.
> 
> > Source/WebCore/dom/ScriptedAnimationController.cpp:60
> > +ScriptedAnimationController::CallbackId ScriptedAnimationController::registerCallback(PassRefPtr<RequestAnimationFrameCallback> callback, Element* e)
> 
> Please don't abbreviate "element"
> 
> > Source/WebCore/dom/ScriptedAnimationController.h:69
> > +} // namespace WebCore
> 
> This still has the comment. Not a big deal - I think that many people missed the webkit-dev discussion, and still land new code with these comments.

I'll fix all of these prior to landing.  Thanks.

[James Robinson]

Committed r78648: <http://trac.webkit.org/changeset/78648>