Bug 85296 – Allow a pre-targeted node to be specified when dispatching a GestureTap event

• Component: UI Events
• Status: RESOLVED FIXED

[Terry Anderson]

Add a PassRefPtr<Node> parameter to EventHandler::handleGestureTap representing a DOM Node which has been pre-computed as the target of the GestureTap event. If the parameter is unused, the existing path of code execution does not change. If the parameter is used, adjustedPoint is changed to be the center of the node's bounding rectangle.

[Terry Anderson]

Created attachment 139650 [details]
Patch

[WebKit Review Bot]

Attachment 139650 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Robert Kroeger]

Comment on attachment 139650 [details]
Patch

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

FWIW: this looks good to me and aligns with what we discussed.

> Source/WebCore/page/EventHandler.cpp:2444
> +    // FIXME: Refactor to avoid hit testing multiple times (this is only an interim step)

put a period at the end of the sentence.

[Adam Barth]

Comment on attachment 139650 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        No new tests. (OOPS!)
> 
> You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]

The style bot is right.  It doesn't look like anyone calls this function with this new parameter.  Is this going to be used in the future?

> Source/WebCore/page/EventHandler.h:166
> -    bool handleGestureTap(const PlatformGestureEvent&);
> +    bool handleGestureTap(const PlatformGestureEvent&, PassRefPtr<Node> preTargetedNode = 0);

Is there a reason why we're using a PassRefPtr here?  It seems like just a Node* would work fine.  Generally, we use PassRefPtr when transferring ownership (see http://www.webkit.org/coding/RefPtr.html for more details).

[Adam Barth]

Comment on attachment 139650 [details]
Patch

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

>>> Source/WebCore/ChangeLog:8
>>> +        No new tests. (OOPS!)
>> 
>> You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
> 
> The style bot is right.  It doesn't look like anyone calls this function with this new parameter.  Is this going to be used in the future?

Would you be willing to replace this line with a link to https://bugs.webkit.org/show_bug.cgi?id=85101 and an explanation that the new parameter will be used (and hopefully tested!) by that patch?

[Terry Anderson]

Created attachment 139654 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 139654 [details]
Patch for landing

Clearing flags on attachment: 139654

Committed r115747: <http://trac.webkit.org/changeset/115747>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.