[webkit-reviews] review granted: [Bug 122687] Extract an iterator/resolver class from calculateAdjustedNodes : [Attachment 214046] Cleanup

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 122687: Extract an iterator/resolver class from calculateAdjustedNodes
https://bugs.webkit.org/show_bug.cgi?id=122687

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

------- Additional Comments from Darin Adler <darin at apple.com>
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.