[webkit-reviews] review denied: [Bug 107800] [Shadow] Implements event re-targeting for Touch Events. : [Attachment 191447] Fix a build hopefully

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Hayato Ito
<hayato at chromium.org>'s request for review:
Bug 107800: [Shadow] Implements event re-targeting for Touch Events.
https://bugs.webkit.org/show_bug.cgi?id=107800

Attachment 191447: Fix a build hopefully
https://bugs.webkit.org/attachment.cgi?id=191447&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=191447&action=review


This looks great, I only have two quibbles below: the loop avoidance and the
cloning of touches. If the latter is not important (I don't know), we can land
this and improve on it.

> Source/WebCore/dom/EventRetargeter.cpp:135
> +	   TouchEventContext* touchEventContext =
static_cast<TouchEventContext*>(eventPath[i].get());

Sounds like we need a toTouchEventContext helper?

> Source/WebCore/dom/EventRetargeter.cpp:150
> +    for (size_t i = 0; i< touchList.length(); ++i) {

"i<" to "i <"

Also, I wonder if we could check to see if adjustment is going to be needed at
a given node in event path and eliminate this loop in those cases?

> Source/WebCore/dom/EventRetargeter.cpp:156
> +	      
touchListPath[j]->append(touch.cloneWithNewTarget(adjustedNodes[j].get()));

Wait.. this is interesting. Are we supposed to return the same object from
.touches each time? If so, this code is not doing the right thing -- we should
be only adjusting the target on touch.

> Source/WebCore/dom/EventRetargeter.h:54
> +    typedef Vector<RefPtr<TouchList> > TouchListPath;

EventPathTouchLists?

> Source/WebCore/dom/EventRetargeter.h:69
> +    static void calculateAdjustedNodes(const Node*, const Node* relatedNode,
bool stopEventAtBoundaryIfNeeded, EventPath&, AdjustedNodes&);

Turn stopEventAtBoundaryIfNeeded into an enum?