WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
53188
requestAnimationFrame callbacks should not fire within a modal dialog
https://bugs.webkit.org/show_bug.cgi?id=53188
Summary
requestAnimationFrame callbacks should not fire within a modal dialog
James Robinson
Reported
2011-01-26 12:57:15 PST
requestAnimationFrame callbacks should not fire within a modal dialog
Attachments
Patch
(15.83 KB, patch)
2011-01-26 14:08 PST
,
James Robinson
no flags
Details
Formatted Diff
Diff
Patch
(16.42 KB, patch)
2011-01-28 15:32 PST
,
James Robinson
no flags
Details
Formatted Diff
Diff
adds a test
(19.60 KB, patch)
2011-01-30 21:47 PST
,
James Robinson
no flags
Details
Formatted Diff
Diff
without ActiveDOMObject
(21.58 KB, patch)
2011-01-31 00:12 PST
,
James Robinson
no flags
Details
Formatted Diff
Diff
Patch
(22.02 KB, patch)
2011-02-08 14:11 PST
,
James Robinson
no flags
Details
Formatted Diff
Diff
with explicit extra suspend/resume calls
(24.85 KB, patch)
2011-02-15 14:57 PST
,
James Robinson
no flags
Details
Formatted Diff
Diff
with rename
(25.27 KB, patch)
2011-02-15 15:18 PST
,
James Robinson
no flags
Details
Formatted Diff
Diff
Patch
(25.27 KB, patch)
2011-02-15 15:37 PST
,
James Robinson
ap
: review+
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
James Robinson
Comment 1
2011-01-26 14:08:15 PST
Created
attachment 80234
[details]
Patch
James Robinson
Comment 2
2011-01-26 16:22:27 PST
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 3
2011-01-27 17:15:59 PST
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
Comment 4
2011-01-28 11:06:33 PST
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 5
2011-01-28 11:33:29 PST
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 6
2011-01-28 14:55:52 PST
Comment on
attachment 80234
[details]
Patch Thanks for the feedback, I'll fix the issues raised and post a new patch.
James Robinson
Comment 7
2011-01-28 15:32:12 PST
Created
attachment 80510
[details]
Patch
James Robinson
Comment 8
2011-01-28 15:34:00 PST
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 9
2011-01-28 16:53:20 PST
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
Comment 10
2011-01-28 16:54:11 PST
On a technical side, I strongly disagree with further abusing ActiveDOMObject.
James Robinson
Comment 11
2011-01-28 16:59:33 PST
(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)
Comment 12
2011-01-28 21:00:18 PST
(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)
Comment 13
2011-01-28 21:03:30 PST
(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
Comment 14
2011-01-28 22:52:40 PST
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
Comment 15
2011-01-30 18:41:21 PST
(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)
Comment 16
2011-01-30 18:58:38 PST
It seems like the real answer is a non-blocking dialog API.
James Robinson
Comment 17
2011-01-30 19:05:36 PST
(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
Comment 18
2011-01-30 21:47:32 PST
Created
attachment 80618
[details]
adds a test
James Robinson
Comment 19
2011-01-30 21:49:08 PST
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 20
2011-01-30 22:17:38 PST
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
Comment 21
2011-01-31 00:12:02 PST
Created
attachment 80622
[details]
without ActiveDOMObject
James Robinson
Comment 22
2011-01-31 00:14:16 PST
(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 23
2011-01-31 01:38:29 PST
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 24
2011-01-31 02:01:17 PST
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 25
2011-01-31 08:37:05 PST
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)
Comment 26
2011-01-31 10:14:47 PST
(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
Comment 27
2011-01-31 11:16:34 PST
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
Comment 28
2011-01-31 11:24:39 PST
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)
Comment 29
2011-01-31 11:29:04 PST
I don't think most users of this API will give a hoot about modal dialogs.
James Robinson
Comment 30
2011-01-31 11:56:45 PST
(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)
Comment 31
2011-01-31 12:33:27 PST
(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
Comment 32
2011-01-31 12:40:50 PST
(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 33
2011-02-08 13:05:19 PST
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
Comment 34
2011-02-08 13:16:03 PST
I still object to making changes like this without a webkit-dev discussion.
James Robinson
Comment 35
2011-02-08 13:18:03 PST
(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
Comment 36
2011-02-08 13:54:47 PST
(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
Comment 37
2011-02-08 14:11:42 PST
Created
attachment 81690
[details]
Patch
Alexey Proskuryakov
Comment 38
2011-02-08 14:23:22 PST
Comment on
attachment 81690
[details]
Patch Automatic r-.
Alexey Proskuryakov
Comment 39
2011-02-08 15:55:37 PST
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
Comment 40
2011-02-08 16:00:59 PST
(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
Comment 41
2011-02-08 16:18:35 PST
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
Comment 42
2011-02-08 16:20:56 PST
(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
Comment 43
2011-02-08 17:07:21 PST
> 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)
Comment 44
2011-02-08 17:10:48 PST
(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
Comment 45
2011-02-08 17:19:36 PST
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
Comment 46
2011-02-08 17:34:41 PST
(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
Comment 47
2011-02-15 14:31:34 PST
(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
Comment 48
2011-02-15 14:37:51 PST
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
Comment 49
2011-02-15 14:57:39 PST
Created
attachment 82528
[details]
with explicit extra suspend/resume calls
James Robinson
Comment 50
2011-02-15 14:59:31 PST
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 51
2011-02-15 15:02:23 PST
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
Comment 52
2011-02-15 15:05:03 PST
(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
Comment 53
2011-02-15 15:18:36 PST
Created
attachment 82534
[details]
with rename
Alexey Proskuryakov
Comment 54
2011-02-15 15:27:29 PST
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
Comment 55
2011-02-15 15:34:16 PST
(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
Comment 56
2011-02-15 15:37:59 PST
Created
attachment 82535
[details]
Patch
Alexey Proskuryakov
Comment 57
2011-02-15 15:54:42 PST
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
Comment 58
2011-02-15 15:56:33 PST
(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
Comment 59
2011-02-15 16:06:41 PST
(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
Comment 60
2011-02-15 16:53:47 PST
Committed
r78648
: <
http://trac.webkit.org/changeset/78648
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug