Bug 57923 - Change EventHandler::updateSelectionForMouseDrag to take a HitTestResult only.
Summary: Change EventHandler::updateSelectionForMouseDrag to take a HitTestResult only.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P3 Normal
Assignee: Alice Boxhall
URL:
Keywords:
Depends on: 57921
Blocks: 55552
  Show dependency tree / graph
 
Reported: 2011-04-05 23:06 PDT by Alice Boxhall
Modified: 2011-04-12 18:30 PDT (History)
5 users (show)

See Also:

Attachments
Patch (3.53 KB, patch)
2011-04-07 16:42 PDT, Alice Boxhall 		no flags Details | Formatted Diff | Diff
Patch (4.16 KB, patch)
2011-04-10 17:57 PDT, Alice Boxhall 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Alice Boxhall 2011-04-05 23:06:17 PDT 
Change EventHandler::updateSelectionForMouseDrag to take a HitTestResult rather than a Node* and an IntPoint&, as the selection may actually not extend into the Node found by the HitTest.
Comment 1 Alice Boxhall 2011-04-07 16:42:29 PDT 
Created attachment 88731 [details]
Patch
Comment 2 Ryosuke Niwa 2011-04-08 00:28:04 PDT 
Comment on attachment 88731 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:631
> +    Node* targetNode = EventHandler::targetNode(hitTestResult);
>      if (!targetNode)

You should probably rename the local variable so that you don't have to resolve the name of member function like this.

> Source/WebCore/page/EventHandler.cpp:642
> +    IntPoint localPoint = hitTestResult.localPoint();
> +    VisiblePosition targetPosition = targetRenderer->positionForPoint(localPoint);

Why do you need to declare a local variable fot IntPoint?
Comment 3 Ojan Vafai 2011-04-10 17:03:04 PDT 
Comment on attachment 88731 [details]
Patch

r- per the above comments. otherwise, this is a nice cleanup.
Comment 4 Alice Boxhall 2011-04-10 17:57:14 PDT 
Created attachment 88961 [details]
Patch
Comment 5 Alice Boxhall 2011-04-10 17:59:14 PDT 
(In reply to comment #2)
> (From update of attachment 88731 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88731&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:631
> > +    Node* targetNode = EventHandler::targetNode(hitTestResult);
> >      if (!targetNode)
> 
> You should probably rename the local variable so that you don't have to resolve the name of member function like this.

Done.

> > Source/WebCore/page/EventHandler.cpp:642
> > +    IntPoint localPoint = hitTestResult.localPoint();
> > +    VisiblePosition targetPosition = targetRenderer->positionForPoint(localPoint);
> 
> Why do you need to declare a local variable fot IntPoint?

No reason.
Comment 6 WebKit Commit Bot 2011-04-10 19:22:01 PDT 
Comment on attachment 88961 [details]
Patch

Clearing flags on attachment: 88961

Committed r83414: <http://trac.webkit.org/changeset/83414>
Comment 7 WebKit Commit Bot 2011-04-10 19:22:05 PDT 
All reviewed patches have been landed.  Closing bug.