[Webkit-unassigned] [Bug 78882] [CSSRegions][CSSOM] Implement regionLayoutEvent

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=78882


Dave Hyatt <hyatt at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #128202|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #2 from Dave Hyatt <hyatt at apple.com>  2012-02-22 13:36:25 PST ---
(From update of attachment 128202)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.