Bug 85296 - Allow a pre-targeted node to be specified when dispatching a GestureTap event
Summary: Allow a pre-targeted node to be specified when dispatching a GestureTap event
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Terry Anderson
URL:
Keywords:
Depends on:
Blocks: 83947 85101
  Show dependency treegraph
 
Reported: 2012-05-01 10:46 PDT by Terry Anderson
Modified: 2012-05-01 13:49 PDT (History)
2 users (show)

See Also:


Attachments
Patch (3.62 KB, patch)
2012-05-01 10:57 PDT, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch for landing (3.69 KB, patch)
2012-05-01 11:58 PDT, Terry Anderson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Terry Anderson 2012-05-01 10:46:24 PDT
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.
Comment 1 Terry Anderson 2012-05-01 10:57:51 PDT
Created attachment 139650 [details]
Patch
Comment 2 WebKit Review Bot 2012-05-01 11:02:22 PDT
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.
Comment 3 Robert Kroeger 2012-05-01 11:16:03 PDT
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.
Comment 4 Adam Barth 2012-05-01 11:42:34 PDT
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).
Comment 5 Adam Barth 2012-05-01 11:44:38 PDT
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?
Comment 6 Terry Anderson 2012-05-01 11:58:59 PDT
Created attachment 139654 [details]
Patch for landing
Comment 7 WebKit Review Bot 2012-05-01 13:49:13 PDT
Comment on attachment 139654 [details]
Patch for landing

Clearing flags on attachment: 139654

Committed r115747: <http://trac.webkit.org/changeset/115747>
Comment 8 WebKit Review Bot 2012-05-01 13:49:18 PDT
All reviewed patches have been landed.  Closing bug.