Bug 122687 – Extract an iterator/resolver class from calculateAdjustedNodes

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Ryosuke Niwa]

Right now, we traverse through the DOM tree three times to figure a mouse event.

1. To create EventContexts for each node
2. To set relatedTarget on each context we've created
3. Actually firing events through

We reduce this to at most two traversals.  In this bug, I'm going to extract EventRelatedNodeResolver class, a stageful iterator class, that lets us update the relatedTarget as we walk up the tree.
Using this class, the current calculateAdjustedNodes can be reimplemented as:

static size_t calculateAdjustedNodes(Node* relatedNode, const EventPath& eventPath, Vector<RefPtr<Node>>& adjustedNodes)
{
    EventRelatedNodeResolver it(*relatedNode);

    size_t eventPathSize = eventPath.size();
    for (size_t i = 0; i < eventPathSize; i++)
        adjustedNodes.append(it.moveToParentOrShadowHost(*eventPath.contextAt(i).node()));

    return eventPathSize;
}

This allows us to compute the related targets of event contexts as we create them.

[Ryosuke Niwa]

Created attachment 214046 [details]
Cleanup

[Darin Adler]

Comment on attachment 214046 [details]
Cleanup

View in context: https://bugs.webkit.org/attachment.cgi?id=214046&action=review

Surprised that so much if this in terms of nodes rather than elements.

> Source/WebCore/dom/EventContext.h:98
> +    enum TouchListType {
> +        Touches,
> +        TargetTouches,
> +        ChangedTouches,
> +    };

I think this would read nicely on one line.

> Source/WebCore/dom/EventContext.h:100
> +    TouchList* touchList(TouchListType type)

Why a pointer instead of a reference for the return value?

> Source/WebCore/dom/EventContext.h:111
> +        return 0;

nullptr

> Source/WebCore/dom/EventDispatcher.cpp:114
> +        , m_relatedNodeInCurrentTreeScope(0)
> +        , m_currentTreeScope(0)

nullptr

> Source/WebCore/dom/EventDispatcher.cpp:115
> +    { }

When the rest of the definition is vertical, I prefer that this be vertical too:

    {
    }

> Source/WebCore/dom/EventDispatcher.cpp:117
> +    Node* moveToParentOrShadowHost(Node& newTarget)

Does the return value really need to be a pointer?

> Source/WebCore/dom/EventDispatcher.cpp:413
> +        EventRelatedNodeResolver it(*touch.target()->toNode());

I suggest the name resolver rather than it.

> Source/WebCore/dom/EventDispatcher.cpp:436
> +    EventRelatedNodeResolver it(*relatedNode);

I suggest calling this resolver rather than it.

[EFL EWS Bot]

Comment on attachment 214046 [details]
Cleanup

Attachment 214046 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3953292

[Ryosuke Niwa]

Created attachment 214047 [details]
Patch for landing

[EFL EWS Bot]

Comment on attachment 214047 [details]
Patch for landing

Attachment 214047 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3951268

[EFL EWS Bot]

Comment on attachment 214047 [details]
Patch for landing

Attachment 214047 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/3766348

[Ryosuke Niwa]

Created attachment 214048 [details]
Fixed EFL build

[WebKit Commit Bot]

Comment on attachment 214048 [details]
Fixed EFL build

Clearing flags on attachment: 214048

Committed r157331: <http://trac.webkit.org/changeset/157331>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.