Bug 32580

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.

style-queue ran check-webkit-style on attachment 44911 [details] without any errors.

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 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.

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.

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.

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 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 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.

Hyatt, can we do this now or not?

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.

(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?

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 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 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 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>

All reviewed patches have been landed. Closing bug.