Bug 36484 – [chromium] correctly handle move drag operations

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Chang]

[chromium] correctly handle move drag operations

[Tony Chang]

Created attachment 51400 [details]
Patch

[Tony Chang]

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/.

[Jian Li]

> --- 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.

[Tony Chang]

Created attachment 51473 [details]
Patch

[Tony Chang]

(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.

[Tony Chang]

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.

[Tony Chang]

Created attachment 51604 [details]
Patch

[Tony Chang]

Created attachment 51605 [details]
Patch

[David Levin]

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).

[Tony Chang]

Created attachment 51698 [details]
Patch

[Tony Chang]

(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.

[David Levin]

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.

[Tony Chang]

Created attachment 51704 [details]
Patch

[Tony Chang]

(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.

[Tony Chang]

Created attachment 51707 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 51707 [details]
Patch for landing

Clearing flags on attachment: 51707

Committed r56603: <http://trac.webkit.org/changeset/56603>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.