Bug 78882

Created attachment 128202 [details] patch

Comment on attachment 128202 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=128202&action=review Minusing for the lack of a check for any listeners at all to prevent having to fire the timer/events when nobody is listening, but see my spec-related questions also. > Source/WebCore/rendering/RenderFlowThread.cpp:404 > + if (!m_regionLayoutUpdateEventTimer.isActive()) > + m_regionLayoutUpdateEventTimer.startOneShot(0); I would add a check if any listeners for the event are registered at all. I think the common case will be that nobody will have any listeners registered, and if so, you can avoid even kicking off this timer and firing the events. > Source/WebCore/rendering/RenderFlowThread.cpp:931 > + for (Vector<RefPtr<Node> >::const_iterator it = regionNodes.begin(); it != regionNodes.end(); ++it) { > + RefPtr<Node> node = *it; > + RefPtr<Document> document = node->document(); > + if (!document) > + continue; > + // Layout needs to be uptodate after each event listener > + document->updateLayoutIgnorePendingStylesheets(); > + RenderObject* renderer = node->renderer(); > + if (renderer && renderer->isRenderRegion()) > + node->dispatchRegionLayoutUpdateEvent(); > + } It seems very weird to me that as you fire the event on each region, the layout has been updated from any event handler changes made by the previous region. This is more of a spec issue, but I don't really agree with the idea of firing N events for N regions. Why not simply fire one event? If the code is always just going to fire on every region, you might as well just have one event that fires on the document itself and then you just grab the region node list and do whatever it is you need to do. I see no benefit to always firing an event on every single region every time you get a layout (it's also not particularly performant). Let the author decide what regions to look at and only fire one event instead.

Created attachment 128477 [details] not yet for review (no new tests)

Created attachment 130439 [details] New patch

Comment on attachment 130439 [details] New patch View in context: https://bugs.webkit.org/attachment.cgi?id=130439&action=review > Source/WebCore/rendering/RenderFlowThread.cpp:349 > +static bool hasRegionLayoutUpdateListeners(Node* node) > +{ > + if (!node) > + return false; > + > + if (node->hasEventListeners(eventNames().webkitRegionLayoutUpdateEvent)) > + return true; > + > + for (node = node->parentNode(); node; node = node->parentNode()) > + if (node->hasEventListeners(eventNames().webkitRegionLayoutUpdateEvent)) > + return true; > + > + return false; > +} I wasn't really asking for a node crawl all the way up... just for something a bit simpler, e.g., a single global that indicated whether there are any registered listeners anywhere in the document.

Created attachment 131355 [details] Added simple check for any existing listeners

Comment on attachment 131355 [details] Added simple check for any existing listeners Attachment 131355 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11943016

Comment on attachment 131355 [details] Added simple check for any existing listeners View in context: https://bugs.webkit.org/attachment.cgi?id=131355&action=review I still disagree with the idea of firing on every region, but I guess we can argue about that on the list. Especially once we have regions that have no elements, I don't think it will make sense to try to make them the target of events. Anyway, r=me. > Source/WebCore/dom/Node.cpp:2782 > + if (!document()->hasListenerType(Document::REGIONLAYOUTUPDATE_LISTENER)) > + return; Yup, great, that's all I was looking for here.

Comment on attachment 131355 [details] Added simple check for any existing listeners The mac build worked locally, so adding CQ+ to make it try again.

Comment on attachment 131355 [details] Added simple check for any existing listeners Clearing flags on attachment: 131355 Committed r110731: <http://trac.webkit.org/changeset/110731>

All reviewed patches have been landed. Closing bug.

webkitRegionLayoutUpdate is named incorrectly. It should be in all lower case. Please let me know if you'd like me to file a bug on this topic.

(In reply to comment #12) > webkitRegionLayoutUpdate is named incorrectly. It should be in all lower case. Please let me know if you'd like me to file a bug on this topic. I filed bug 100335 on this topic.