Bug 78882 - [CSSRegions][CSSOM] Implement regionLayoutEvent
Summary: [CSSRegions][CSSOM] Implement regionLayoutEvent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 57312
  Show dependency treegraph
 
Reported: 2012-02-17 00:53 PST by Raul Hudea
Modified: 2017-10-27 20:10 PDT (History)
2 users (show)

See Also:


Attachments
patch (19.43 KB, patch)
2012-02-22 06:35 PST, Raul Hudea
hyatt: review-
hyatt: commit-queue-
Details | Formatted Diff | Diff
not yet for review (no new tests) (23.48 KB, patch)
2012-02-23 07:52 PST, Raul Hudea
no flags Details | Formatted Diff | Diff
New patch (24.97 KB, patch)
2012-03-06 14:03 PST, Raul Hudea
hyatt: review-
hyatt: commit-queue-
Details | Formatted Diff | Diff
Added simple check for any existing listeners (24.86 KB, patch)
2012-03-12 11:13 PDT, Raul Hudea
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Raul Hudea 2012-02-22 06:35:22 PST
Created attachment 128202 [details]
patch
Comment 2 Dave Hyatt 2012-02-22 13:36:25 PST
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.
Comment 3 Raul Hudea 2012-02-23 07:52:19 PST
Created attachment 128477 [details]
not yet for review (no new tests)
Comment 4 Raul Hudea 2012-03-06 14:03:30 PST
Created attachment 130439 [details]
New patch
Comment 5 Dave Hyatt 2012-03-07 09:10:15 PST
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.
Comment 6 Raul Hudea 2012-03-12 11:13:33 PDT
Created attachment 131355 [details]
Added simple check for any existing listeners
Comment 7 Build Bot 2012-03-12 11:36:07 PDT
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 8 Dave Hyatt 2012-03-14 10:43:28 PDT
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 9 Alexandru Chiculita 2012-03-14 11:02:00 PDT
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 10 WebKit Review Bot 2012-03-14 12:48:21 PDT
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>
Comment 11 WebKit Review Bot 2012-03-14 12:48:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Adam Barth 2012-10-24 23:20:20 PDT
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.
Comment 13 Adam Barth 2012-10-24 23:28:21 PDT
(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.