Return an approximately correct drop effect from WebDragTarget::OnDrop.

Return an approximately correct drop effect from WebDragTarget::OnDrop.

This isn't actually guaranteed to be correct, since it's the cached value from previous dragenter / dragover results, but hopefully it's somewhat correct part of the time. To prevent potential data loss, all moves are mapped to copies. A future improvement might to fire a ViewHostMsg_UpdateDragCursor after the renderer processes the drop event.

BUG=39399
TEST=Manual testing using the attached test case on the bug.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=43925

[tony, 2010-04-08 00:00:20]

http://codereview.chromium.org/1572027/diff/1/4
File chrome/browser/tab_contents/web_drop_target_win.cc (right):

http://codereview.chromium.org/1572027/diff/1/4#newcode138
chrome/browser/tab_contents/web_drop_target_win.cc:138: return
web_drag_utils_win::WebDragOpToWinDragOp(drag_cursor_);
I think this is only supposed to return one operation, not a bitmask of all
possible operations.  This is why the previous method returned early (and
followed a specific order).

http://codereview.chromium.org/1572027/diff/1/4#newcode166
chrome/browser/tab_contents/web_drop_target_win.cc:166: return
web_drag_utils_win::WebDragOpToWinDragOp(drag_cursor_);
Same comment as above.

http://codereview.chromium.org/1572027/diff/1/4#newcode218
chrome/browser/tab_contents/web_drop_target_win.cc:218: return
web_drag_utils_win::WebDragOpToWinDragOp(drag_cursor_);
What are the consequences of getting it wrong?  I was worried about the case of
saying a move happened when it didn't.  If it thinks a move happened, can the
drag source delete contents?

[dcheng, 2010-04-08 00:10:10]

http://codereview.chromium.org/1572027/diff/1/4
File chrome/browser/tab_contents/web_drop_target_win.cc (right):

http://codereview.chromium.org/1572027/diff/1/4#newcode138
chrome/browser/tab_contents/web_drop_target_win.cc:138: return
web_drag_utils_win::WebDragOpToWinDragOp(drag_cursor_);
On 2010/04/08 00:00:20, tony wrote:
> I think this is only supposed to return one operation, not a bitmask of all
> possible operations.  This is why the previous method returned early (and
> followed a specific order).

drag_cursor_ is actually the corresponding WebDragOperation that was set in
dragenter / dragover's event.dataTransfer.dropEffect. It is (supposed to be)
impossible for it to contain more than one op. I updated this code to follow the
convention used by the GTK platform (the Mac platform doesn't have to do
anything special since it just so happens that WebDragOperation mirrors
NSDragOperation).

http://codereview.chromium.org/1572027/diff/1/4#newcode218
chrome/browser/tab_contents/web_drop_target_win.cc:218: return
web_drag_utils_win::WebDragOpToWinDragOp(drag_cursor_);
On 2010/04/08 00:00:20, tony wrote:
> What are the consequences of getting it wrong?  I was worried about the case
of
> saying a move happened when it didn't.  If it thinks a move happened, can the
> drag source delete contents?

Possibly, depending on how the drag source is written. In practice, this
shouldn't be wrong *too* often, and it mirrors the behavior of the OS X / GTK
ports (which suffer from the same potential problems, I believe).

[tony, 2010-04-08 00:21:01]

http://codereview.chromium.org/1572027/diff/1/4
File chrome/browser/tab_contents/web_drop_target_win.cc (right):

http://codereview.chromium.org/1572027/diff/1/4#newcode138
chrome/browser/tab_contents/web_drop_target_win.cc:138: return
web_drag_utils_win::WebDragOpToWinDragOp(drag_cursor_);
On 2010/04/08 00:10:10, dcheng wrote:
> drag_cursor_ is actually the corresponding WebDragOperation that was set in
> dragenter / dragover's event.dataTransfer.dropEffect. It is (supposed to be)
> impossible for it to contain more than one op.

Can you add a DCHECK to the code (also below)?

http://codereview.chromium.org/1572027/diff/1/4#newcode218
chrome/browser/tab_contents/web_drop_target_win.cc:218: return
web_drag_utils_win::WebDragOpToWinDragOp(drag_cursor_);
On 2010/04/08 00:10:10, dcheng wrote:
> Possibly, depending on how the drag source is written. In practice, this
> shouldn't be wrong *too* often, and it mirrors the behavior of the OS X / GTK
> ports (which suffer from the same potential problems, I believe).

Yes, but in that case, it can lead to data loss.  One possibility is to only
return NONE or COPY (i.e., map MOVE to COPY).

[dcheng, 2010-04-08 01:02:46]

http://codereview.chromium.org/1572027/diff/1/4
File chrome/browser/tab_contents/web_drop_target_win.cc (right):

http://codereview.chromium.org/1572027/diff/1/4#newcode138
chrome/browser/tab_contents/web_drop_target_win.cc:138: return
web_drag_utils_win::WebDragOpToWinDragOp(drag_cursor_);
On 2010/04/08 00:21:01, tony wrote:
> On 2010/04/08 00:10:10, dcheng wrote:
> > drag_cursor_ is actually the corresponding WebDragOperation that was set in
> > dragenter / dragover's event.dataTransfer.dropEffect. It is (supposed to be)
> > impossible for it to contain more than one op.
> 
> Can you add a DCHECK to the code (also below)?

Done.

http://codereview.chromium.org/1572027/diff/1/4#newcode218
chrome/browser/tab_contents/web_drop_target_win.cc:218: return
web_drag_utils_win::WebDragOpToWinDragOp(drag_cursor_);
On 2010/04/08 00:21:01, tony wrote:
> On 2010/04/08 00:10:10, dcheng wrote:
> > Possibly, depending on how the drag source is written. In practice, this
> > shouldn't be wrong *too* often, and it mirrors the behavior of the OS X /
GTK
> > ports (which suffer from the same potential problems, I believe).
> 
> Yes, but in that case, it can lead to data loss.  One possibility is to only
> return NONE or COPY (i.e., map MOVE to COPY).

If we do map MOVE to COPY, people can simply use LINK as an internal way to say
'this is really a MOVE'. I think we should bring Windows into alignment with the
behavior of the other platforms and create a separate issue to try to figure out
how to handle the cases where Chromium will misbehave.

[tony, 2010-04-08 01:35:38]

http://codereview.chromium.org/1572027/diff/1/4
File chrome/browser/tab_contents/web_drop_target_win.cc (right):

http://codereview.chromium.org/1572027/diff/1/4#newcode218
chrome/browser/tab_contents/web_drop_target_win.cc:218: return
web_drag_utils_win::WebDragOpToWinDragOp(drag_cursor_);
On 2010/04/08 01:02:46, dcheng wrote:
> If we do map MOVE to COPY, people can simply use LINK as an internal way to
say
> 'this is really a MOVE'. I think we should bring Windows into alignment with
the
> behavior of the other platforms and create a separate issue to try to figure
out
> how to handle the cases where Chromium will misbehave.

On GTK, we never do a move.  The FALSE in the third param here means to not do a
move:
http://src.chromium.org/cgi-bin/gitweb.cgi?p=chromium.git;a=blob;f=chrome/bro...
http://library.gnome.org/devel/gtk/stable/gtk-Drag-and-Drop.html#gtk-drag-finish

I would much rather have the wrong cursor or drop animation than cause data
loss.

[dcheng, 2010-04-08 03:02:20]

http://codereview.chromium.org/1572027/diff/1/4
File chrome/browser/tab_contents/web_drop_target_win.cc (right):

http://codereview.chromium.org/1572027/diff/1/4#newcode218
chrome/browser/tab_contents/web_drop_target_win.cc:218: return
web_drag_utils_win::WebDragOpToWinDragOp(drag_cursor_);
On 2010/04/08 01:35:38, tony wrote:
> On 2010/04/08 01:02:46, dcheng wrote:
> > If we do map MOVE to COPY, people can simply use LINK as an internal way to
> say
> > 'this is really a MOVE'. I think we should bring Windows into alignment with
> the
> > behavior of the other platforms and create a separate issue to try to figure
> out
> > how to handle the cases where Chromium will misbehave.
> 
> On GTK, we never do a move.  The FALSE in the third param here means to not do
a
> move:
>
http://src.chromium.org/cgi-bin/gitweb.cgi?p=chromium.git;a=blob;f=chrome/bro...
>
http://library.gnome.org/devel/gtk/stable/gtk-Drag-and-Drop.html#gtk-drag-finish
> 
> I would much rather have the wrong cursor or drop animation than cause data
> loss.
> 

I wouldn't be particularly surprised if a number of applications deleted the
drag data in their drag-end signal instead of doing in drag-data-delete on
GTK...

The purpose of this patch was to converge DnD behavior on Mac/Linux/Windows. If
you feel this strongly, then I'll make the change, but it'll be on OS X (which
also suffers from this problem) and GTK as well. I'll open another issue too to
discuss potential mitigations to try to make MOVE work at least part of the
time.

[tony, 2010-04-08 03:17:12]

On 2010/04/08 03:02:20, dcheng wrote:
> I wouldn't be particularly surprised if a number of applications deleted the
> drag data in their drag-end signal instead of doing in drag-data-delete on
> GTK...

That would be a bug in the other application.  That doesn't justify introducing
a bug caused by Chrome.

> The purpose of this patch was to converge DnD behavior on Mac/Linux/Windows.
If
> you feel this strongly, then I'll make the change, but it'll be on OS X (which
> also suffers from this problem) and GTK as well. I'll open another issue too
to
> discuss potential mitigations to try to make MOVE work at least part of the
> time.

If your goal is to make sure all platforms behave the same, it sounds like
should change OSX and GTK.  However, I would step back and ask what bug you're
trying to fix.  Do you have an example of a web app or a use case that is broken
by the current behavior?

[dcheng1, 2010-04-08 03:41:01]

On Wed, Apr 7, 2010 at 8:17 PM, <tony@chromium.org> wrote:

> On 2010/04/08 03:02:20, dcheng wrote:
>
>> I wouldn't be particularly surprised if a number of applications deleted
>> the
>> drag data in their drag-end signal instead of doing in drag-data-delete on
>> GTK...
>>
>
> That would be a bug in the other application.  That doesn't justify
> introducing
> a bug caused by Chrome.
>
>
>  The purpose of this patch was to converge DnD behavior on
>> Mac/Linux/Windows.
>>
> If
>
>> you feel this strongly, then I'll make the change, but it'll be on OS X
>> (which
>> also suffers from this problem) and GTK as well. I'll open another issue
>> too
>>
> to
>
>> discuss potential mitigations to try to make MOVE work at least part of
>> the
>> time.
>>
>
> If your goal is to make sure all platforms behave the same, it sounds like
> should change OSX and GTK.  However, I would step back and ask what bug
> you're
> trying to fix.  Do you have an example of a web app or a use case that is
> broken
> by the current behavior?
>
>
I believe the current behavior is broken for two reasons:
- According to HTML5, dragend is always wrong if a drop actually happened.
- Authors will need to account for different Chrome behaviors on different
operating systems.

Even if I didn't submit this change, OS X would still need to be updated to
prevent the potential data loss issue you raised,


> http://codereview.chromium.org/1572027/show
>

[dcheng, 2010-04-08 04:59:10]

http://codereview.chromium.org/1572027/diff/1/4
File chrome/browser/tab_contents/web_drop_target_win.cc (right):

http://codereview.chromium.org/1572027/diff/1/4#newcode218
chrome/browser/tab_contents/web_drop_target_win.cc:218: return
web_drag_utils_win::WebDragOpToWinDragOp(drag_cursor_);
On 2010/04/08 00:21:01, tony wrote:
> On 2010/04/08 00:10:10, dcheng wrote:
> > Possibly, depending on how the drag source is written. In practice, this
> > shouldn't be wrong *too* often, and it mirrors the behavior of the OS X /
GTK
> > ports (which suffer from the same potential problems, I believe).
> 
> Yes, but in that case, it can lead to data loss.  One possibility is to only
> return NONE or COPY (i.e., map MOVE to COPY).

On closer inspection, it's non-trivial to implement drop exactly as HTML5
specifies. It doesn't seem like GTK or OS X have an officially sanctioned way of
letting the drop target specify the drop operation during the actual drop. I've
updated the Windows implementation and mapped all moves to a copy for the time
being.

[tony, 2010-04-08 05:02:00]

LGTM