Bug 36484 - [chromium] correctly handle move drag operations
Summary: [chromium] correctly handle move drag operations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-23 01:17 PDT by Tony Chang
Modified: 2010-03-25 22:48 PDT (History)
2 users (show)

See Also:


Attachments
Patch (9.50 KB, patch)
2010-03-23 01:19 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (9.82 KB, patch)
2010-03-23 19:21 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (5.60 KB, patch)
2010-03-25 01:27 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (6.87 KB, patch)
2010-03-25 01:32 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (7.18 KB, patch)
2010-03-25 17:27 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (11.71 KB, patch)
2010-03-25 17:58 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch for landing (11.70 KB, patch)
2010-03-25 18:36 PDT, Tony Chang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2010-03-23 01:17:18 PDT
[chromium] correctly handle move drag operations
Comment 1 Tony Chang 2010-03-23 01:19:21 PDT
Created attachment 51400 [details]
Patch
Comment 2 Tony Chang 2010-03-23 01:20:57 PDT
This is to work around the fact that the "move" maps to DragOperationGeneric (see https://bugs.webkit.org/show_bug.cgi?id=33697).

This is consistent with what happens in WebKit/WebKit/win/.
Comment 3 Jian Li 2010-03-23 18:54:07 PDT
> --- a/WebKit/chromium/src/WebViewImpl.cpp
> +++ b/WebKit/chromium/src/WebViewImpl.cpp

> -unsigned long WebViewImpl::createUniqueIdentifierForRequest() {
> +WebDragOperation WebViewImpl::dragTargetDragging(WebCore::DragData* dragData, bool isEntering)
> +{
I think the creation of DragData could also be shared in this function.
    DragData dragData(
        m_currentDragData.get(),
        clientPoint,
        screenPoint,
        static_cast<DragOperation>(operationsAllowed));

> --- a/WebKit/chromium/src/WebViewImpl.h
> +++ b/WebKit/chromium/src/WebViewImpl.h

> +    // Consolidate some common code between starting a drag over a target and
> +    // updating a drag over a target.  If we're starting a drag, |isEntering|
> +    // should be true.
> +    WebDragOperation dragTargetDragging(WebCore::DragData*, bool isEntering);
Probably it might be better to call it as dragTargetDragEnterOrOver.
In addition, since WebKit does not have line length, we do not need to split the long comment into 3 tidy lines.
Comment 4 Tony Chang 2010-03-23 19:21:09 PDT
Created attachment 51473 [details]
Patch
Comment 5 Tony Chang 2010-03-23 19:22:19 PDT
(In reply to comment #3)
> I think the creation of DragData could also be shared in this function.

Done.

> Probably it might be better to call it as dragTargetDragEnterOrOver.
> In addition, since WebKit does not have line length, we do not need to split
> the long comment into 3 tidy lines.

I renamed the function to dragTargetDragEnterOrOver, but kept the comment as 3 lines because that matches the style in the rest of the file.
Comment 6 Tony Chang 2010-03-25 01:09:35 PDT
Comment on attachment 51473 [details]
Patch

Since https://bugs.webkit.org/show_bug.cgi?id=33697 got r+, I'm going to rev this patch to no longer need the GENERIC -> MOVE conversions.
Comment 7 Tony Chang 2010-03-25 01:27:09 PDT
Created attachment 51604 [details]
Patch
Comment 8 Tony Chang 2010-03-25 01:32:40 PDT
Created attachment 51605 [details]
Patch
Comment 9 David Levin 2010-03-25 11:43:11 PDT
Comment on attachment 51605 [details]
Patch


> diff --git a/WebKit/chromium/src/WebViewImpl.h b/WebKit/chromium/src/WebViewImpl.h

> +    // Consolidate some common code between starting a drag over a target and
> +    // updating a drag over a target.  If we're starting a drag, |isEntering|
> +    // should be true.
> +    WebDragOperation dragTargetDragEnterOrOver(const WebPoint& clientPoint,
> +                                               const WebPoint& screenPoint,
> +                                               bool isEntering);

"bool isEntering" should be an enum, so it is clearer at the calling site what the function is being passed (instead of "true/false" be passed into the function).
Comment 10 Tony Chang 2010-03-25 17:27:44 PDT
Created attachment 51698 [details]
Patch
Comment 11 Tony Chang 2010-03-25 17:29:19 PDT
(In reply to comment #9)
> "bool isEntering" should be an enum, so it is clearer at the calling site what
> the function is being passed (instead of "true/false" be passed into the
> function).

Done.  New patch up.
Comment 12 David Levin 2010-03-25 17:50:33 PDT
Comment on attachment 51698 [details]
Patch


> diff --git a/WebKit/chromium/src/WebViewImpl.h b/WebKit/chromium/src/WebViewImpl.h
> +    // Consolidate some common code between starting a drag over a target and
> +    // updating a drag over a target.  If we're starting a drag, |isEntering|

Single space after . in comments (in WK land). Sorry, I should have caught this last time.
Comment 13 Tony Chang 2010-03-25 17:58:19 PDT
Created attachment 51704 [details]
Patch
Comment 14 Tony Chang 2010-03-25 17:59:14 PDT
(In reply to comment #12)
> (From update of attachment 51698 [details])
> 
> > diff --git a/WebKit/chromium/src/WebViewImpl.h b/WebKit/chromium/src/WebViewImpl.h
> > +    // Consolidate some common code between starting a drag over a target and
> > +    // updating a drag over a target.  If we're starting a drag, |isEntering|
> 
> Single space after . in comments (in WK land). Sorry, I should have caught this
> last time.

Fixed throughout the file.  If this is really something that's important, it should be in the style checker.
Comment 15 Tony Chang 2010-03-25 18:36:42 PDT
Created attachment 51707 [details]
Patch for landing
Comment 16 WebKit Commit Bot 2010-03-25 22:48:39 PDT
Comment on attachment 51707 [details]
Patch for landing

Clearing flags on attachment: 51707

Committed r56603: <http://trac.webkit.org/changeset/56603>
Comment 17 WebKit Commit Bot 2010-03-25 22:48:44 PDT
All reviewed patches have been landed.  Closing bug.