Bug 32580 - JS Event listeners should not call Document::updateStyleForAllDocuments()
Summary: JS Event listeners should not call Document::updateStyleForAllDocuments()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 32641
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-15 14:26 PST by James Robinson
Modified: 2010-03-04 21:18 PST (History)
8 users (show)

See Also:


Attachments
Removes the calls to Document::updateStyleForAllDocuments() in event dispatch code (1.70 KB, patch)
2009-12-15 14:32 PST, James Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2009-12-15 14:26:30 PST
Currently whenever a JS event handler for a DOM event is fired at a document context a call to Document::updateStyleForAllDocuments() is made.  This used to be necessary since many parts of the codebase would assume styles were up to date and there was no useful way to defer the style updates, but I believe this is no longer the case as all layout codepaths should be ensuring that styles are up to date.

This is really bad for pages that register DOM mutation event listeners since styles are updated on every DOM mutation.
Comment 1 James Robinson 2009-12-15 14:32:56 PST
Created attachment 44911 [details]
Removes the calls to Document::updateStyleForAllDocuments() in event dispatch code

This patch just deletes the calls for both the JSC and V8 bindings.  This does not regress any layout tests (including pixel tests) so I believe it is correct.
Comment 2 WebKit Review Bot 2009-12-15 14:34:00 PST
style-queue ran check-webkit-style on attachment 44911 [details] without any errors.
Comment 3 Eric Seidel (no email) 2009-12-15 14:35:24 PST
Comment on attachment 44911 [details]
Removes the calls to Document::updateStyleForAllDocuments() in event dispatch code

I would agree, this seems like a bug.  We should get the original author of these lines to confirm as such though. :)
Comment 4 Darin Adler 2009-12-15 14:35:54 PST
Comment on attachment 44911 [details]
Removes the calls to Document::updateStyleForAllDocuments() in event dispatch code

I see how this is bad for performance. But I suspect that removing this call is wrong for correctness. It's entirely possible that a script changes something that triggers a style change and there is no other mechanism to guarantee the style change happens.

We need to replace this mechanism with a better one, but just ripping this code out is probably wrong. We'll need to construct a test case to demonstrate this.

I hope I'm wrong. I'm not saying review- because I'm not sure. Hyatt might know better.
Comment 5 James Robinson 2009-12-15 14:53:27 PST
Thanks for the feedback Darin.  This is basically a continuation of http://trac.webkit.org/changeset/42384 (which sadly doesn't seem to have an associated bugzilla bug).  I think any code that queries properties derived from styles without making sure styles are up to date is wrong and should be fixed.  I'm not sure if there is any code that does that currently - if there is then the layout tests do not seem to be hitting it.
Comment 6 Eric Seidel (no email) 2009-12-16 13:26:43 PST
The important quote there being "For now in order to reduce the risk of regressions, I have left in all the synchronous machinery for updating style following DOM events and JavaScript timeouts. Eventually these calls will be removed." - Dave Hyatt.

I'm ready to r+ this based on the comments above and hyatt's comments in http://trac.webkit.org/changeset/42384, but I agree, it would be nice to see Hyatt's stamp on this.
Comment 7 James Robinson 2009-12-16 16:49:53 PST
https://bugs.webkit.org/show_bug.cgi?id=32641 is one case where this patch could cause a regression.  This patch only removes the call from the individual dispatches - i.e. between multiple event handlers on the same event, so it doesn't cause a layout test regression currently.  However, the logical next thing to do is to remove the call from Node::dispatchGenericEvent which would break fast/forms/HTMLOptionElement-selected.html.
Comment 8 Eric Seidel (no email) 2009-12-21 00:20:43 PST
Comment on attachment 44911 [details]
Removes the calls to Document::updateStyleForAllDocuments() in event dispatch code

I don't see how this patch can be up for review with the other bug depending on this one.  I'm ready to r+ this (I think part of the only way to figure this out is to land it and wathc for regressions), but an r+ is not possible until bug 32641 is fixed.

Clearing review flag.
Comment 9 Adam Barth 2010-02-07 18:48:14 PST
Comment on attachment 44911 [details]
Removes the calls to Document::updateStyleForAllDocuments() in event dispatch code

Re-nominating fore review.  It looks like the blocking bug has been fixed.
Comment 10 Darin Adler 2010-02-07 20:07:29 PST
Hyatt, can we do this now or not?
Comment 11 James Robinson 2010-02-07 20:12:13 PST
The forms code still depends on style recalcs to update internal state, so we can't remove _all_ of the Document::updateStyleForAllDocuments() calls.  I think this one is safe to remove, since the call at the end of Node::dispatchGenericEvents() will still ensure that everything is up to date before anything outside of the event dispatching code runs.
Comment 12 Darin Adler 2010-02-07 20:58:48 PST
(In reply to comment #11)
> The forms code still depends on style recalcs to update internal state, so we
> can't remove _all_ of the Document::updateStyleForAllDocuments() calls.

Really? Doesn't the style recalc timer take care of that case?
Comment 13 James Robinson 2010-02-14 20:00:13 PST
No, because recalcStyle() has side effects other than updating styles.  For instance. HTMLFormControl::recalcStyle() enqueues a call to the virtual RenderObject::updateFromElement() function.  Several overrides of this (RenderMenuList::updateFromElement, RenderListBox::updateFromElement) cause updates within the DOM like updating selected state, etc.

What happens in most tests currently is:

1.) The parser creates some DOM structure
2.) DOMContentLoaded fires, which calls Document::updateStyleForAllDocuments(), which forces pending updates to occur.
3.) The test code runs in the body's onload handler.
4.) The style time fires.
Comment 14 Eric Seidel (no email) 2010-02-17 15:07:36 PST
Comment on attachment 44911 [details]
Removes the calls to Document::updateStyleForAllDocuments() in event dispatch code

Looks OK to me.  Would be nice to see a comment from Hyatt, but we can always back this out if it breaks things.
Comment 15 Adam Barth 2010-03-04 19:10:35 PST
Comment on attachment 44911 [details]
Removes the calls to Document::updateStyleForAllDocuments() in event dispatch code

Ready to land?  This has been sitting here for a while.
Comment 16 WebKit Commit Bot 2010-03-04 21:18:13 PST
Comment on attachment 44911 [details]
Removes the calls to Document::updateStyleForAllDocuments() in event dispatch code

Clearing flags on attachment: 44911

Committed r55568: <http://trac.webkit.org/changeset/55568>
Comment 17 WebKit Commit Bot 2010-03-04 21:18:19 PST
All reviewed patches have been landed.  Closing bug.