Bug 78882 – [CSSRegions][CSSOM] Implement regionLayoutEvent

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Raul Hudea]

http://dev.w3.org/csswg/css3-regions/#region-flow-layout-events

[Raul Hudea]

Created attachment 128202 [details]
patch

[Dave Hyatt]

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.

[Raul Hudea]

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

[Raul Hudea]

Created attachment 130439 [details]
New patch

[Dave Hyatt]

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.

[Raul Hudea]

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

[Build Bot]

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

[Dave Hyatt]

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.

[Alexandru Chiculita]

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.

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

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.

[Adam Barth]

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