[webkit-reviews] review denied: [Bug 78882] [CSSRegions][CSSOM] Implement regionLayoutEvent : [Attachment 128202] patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Raul Hudea <rhudea at adobe.com>'s request
for review:
Bug 78882: [CSSRegions][CSSOM] Implement regionLayoutEvent
https://bugs.webkit.org/show_bug.cgi?id=78882

Attachment 128202: patch
https://bugs.webkit.org/attachment.cgi?id=128202&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
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.