Plumb the DragOperation through all the layers between the platform DnD code ...

Plumb the DragOperation through all the layers between the platform Drag-n-drop code and WebCore.
This allows the HTML5 DataTransfer effectAllowed and dropEffect properties to be set correctly in JS handlers, as per the HTML5 spec.
(The drag-dropeffect test isn't in WebKit yet -- it's part of a separate WebKit patch that's been in review for weeks.)
R=darin,pink
BUG=http://code.google.com/p/chromium/issues/detail?id=14654, http://code.google.com/p/chromium/issues/detail?id=20985
TEST=LayoutTests/fast/events/drag-dropeffect.html, LayoutTests/editing/pasteboard/files-during-page-drags.html

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

[darin (slow to review), 2009-08-28 18:35:34]

more feedback in a bit...

http://codereview.chromium.org/174364/diff/511/1332
File webkit/glue/drag_operations.h (right):

http://codereview.chromium.org/174364/diff/511/1332#newcode8
Line 8: namespace WebKit {
please move this file into webkit/api/public.  make sure all of the style
matches webkit conventions.

http://codereview.chromium.org/174364/diff/511/1332#newcode22
Line 22: DragOperationEvery   = UINT_MAX
UINT_MAX is not defined anywhere above.  headers should be complete.

http://codereview.chromium.org/174364/diff/511/1332#newcode23
Line 23: } DragOperation;
please avoid typedef here.

[Jens Alfke, 2009-08-28 20:38:13]

OK, moved drag_operations.h to WebDragOperation.h, changed style and renamed
types and constants to match other WebKit API.

[darin (slow to review), 2009-09-01 18:11:41]

http://codereview.chromium.org/174364/diff/6086/6107
File chrome/browser/cocoa/web_drag_source.h (right):

http://codereview.chromium.org/174364/diff/6086/6107#newcode27
Line 27: NSDragOperation dragOperationMask_;
please find someone who is more well versed in objc programming to review the
objc code.

http://codereview.chromium.org/174364/diff/6086/6096
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/174364/diff/6086/6096#newcode279
Line 279: WebKit::WebDragOperationsMask operation_mask) {
add using WebKit::WebDragOperation{Mask} to the top of the file

http://codereview.chromium.org/174364/diff/6086/6101
File chrome/browser/gtk/tab_contents_drag_source.cc (right):

http://codereview.chromium.org/174364/diff/6086/6101#newcode197
Line 197: void TabContentsDragSource::OnDragEnd(WebKit::WebDragOperation
operation) {
add a using WebKit::WebDragOperation to the top of the file

http://codereview.chromium.org/174364/diff/6086/6102
File chrome/browser/gtk/tab_contents_drag_source.h (right):

http://codereview.chromium.org/174364/diff/6086/6102#newcode49
Line 49: // FIXME(snej): Pass actual operation instead of hardcoding copy
TODO

http://codereview.chromium.org/174364/diff/6086/6104
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/174364/diff/6086/6104#newcode464
Line 464: WebKit::WebDragOperationsMask operations_allowed) {
add a using directive to the top of the file

http://codereview.chromium.org/174364/diff/6086/6104#newcode616
Line 616: WebKit::WebDragOperation operation) {
add a using directive to the top of the file

http://codereview.chromium.org/174364/diff/6086/6104#newcode621
Line 621: true, static_cast<uint32>(operation)));
please just send the enum directly over ipc

http://codereview.chromium.org/174364/diff/6086/6105
File chrome/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/174364/diff/6086/6105#newcode526
Line 526: void OnUpdateDragCursor(uint32 drag_operation);
do these need to be uint32?  can we instead just use the enums?  you can add
ParamTraits to webkit_param_traits.h for the enums.

http://codereview.chromium.org/174364/diff/6086/6098
File chrome/browser/tab_contents/interstitial_page.cc (right):

http://codereview.chromium.org/174364/diff/6086/6098#newcode90
Line 90: WebKit::WebDragOperationsMask operations_allowed);
please add using directives for these types

http://codereview.chromium.org/174364/diff/6086/6118
File chrome/browser/tab_contents/tab_contents_view_gtk.cc (right):

http://codereview.chromium.org/174364/diff/6086/6118#newcode142
Line 142: void UpdateDragStatus(WebKit::WebDragOperation operation) {
using directives

http://codereview.chromium.org/174364/diff/6086/6118#newcode148
Line 148: // FIXME(snej): Pass appropriate GDK action instead of hardcoding COPY
TODO

http://codereview.chromium.org/174364/diff/6086/6118#newcode209
Line 209: // FIXME(snej): Pass appropriate DragOperation instead of hardcoding
TODO

http://codereview.chromium.org/174364/diff/6086/6118#newcode274
Line 274: // FIXME(snej): Pass appropriate DragOperation instead of hardcoding
TODO

http://codereview.chromium.org/174364/diff/6086/6116
File chrome/browser/tab_contents/web_drag_source.cc (right):

http://codereview.chromium.org/174364/diff/6086/6116#newcode71
Line 71: // FIXME(jpa): This needs to be fixed to send the actual drag
operation.
TODO

http://codereview.chromium.org/174364/diff/6086/6117
File chrome/browser/tab_contents/web_drop_target.cc (right):

http://codereview.chromium.org/174364/diff/6086/6117#newcode117
Line 117: WebKit::WebDragOperationCopy);  // FIXME(snej): Send actual operation
TODO

http://codereview.chromium.org/174364/diff/6086/6117#newcode140
Line 140: WebKit::WebDragOperationCopy);  // FIXME(snej): Send actual operation
TODO

http://codereview.chromium.org/174364/diff/6086/6121
File chrome/browser/views/tab_contents/tab_contents_view_gtk.cc (right):

http://codereview.chromium.org/174364/diff/6086/6121#newcode164
Line 164: WebKit::WebDragOperationsMask ops) {
add using directives

http://codereview.chromium.org/174364/diff/6086/6121#newcode166
Line 166: // FIXME(snej): Make use of WebDragOperationsMask
TODO

http://codereview.chromium.org/174364/diff/6086/6114
File chrome/browser/views/tab_contents/tab_contents_view_win.cc (right):

http://codereview.chromium.org/174364/diff/6086/6114#newcode193
Line 193: // FIXME(snej): Use 'ops' param instead of hardcoding dropeffects
TODO

http://codereview.chromium.org/174364/diff/6086/6110
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/174364/diff/6086/6110#newcode338
Line 338: uint32 /* operations_allowed */)
please just use the enum types directly

http://codereview.chromium.org/174364/diff/6086/6110#newcode1111
Line 1111: uint32 /* drag_operations_mask */)
use enum types here

http://codereview.chromium.org/174364/diff/6086/6112
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/174364/diff/6086/6112#newcode2138
Line 2138: WebKit::WebDragOperationsMask allowed_ops) {
add using directive for WebDragOperationsMask

http://codereview.chromium.org/174364/diff/6086/6111
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/174364/diff/6086/6111#newcode531
Line 531: uint32 operations_allowed);
enum names

http://codereview.chromium.org/174364/diff/6086/6122
File webkit/api/public/WebDragOperation.h (right):

http://codereview.chromium.org/174364/diff/6086/6122#newcode39
Line 39: // destination.
nit: kill the new line here?  so you get a paragraph.

http://codereview.chromium.org/174364/diff/6086/6122#newcode50
Line 50: WebDragOperationEvery   = UINT_MAX
nit: how about we just use -1 here?  alternatively, i think it is more common in
webkit code to include <limits> and then use
std::numeric_limits<unsigned>::max() for this.  although i worry about that max
colliding with the max in windows.h :-/

http://codereview.chromium.org/174364/diff/6086/6122#newcode57
Line 57: }  // namespace WebKit
nit: webkit style is to only have one space before the '//'

http://codereview.chromium.org/174364/diff/6086/6089
File webkit/glue/dragclient_impl.cc (right):

http://codereview.chromium.org/174364/diff/6086/6089#newcode81
Line 81: static_cast<WebKit::WebDragOperationsMask>(drag_operation_mask));
it would be good to have some COMPILE_ASSERTS somewhere the ensure that these
enum values stay in sync.

http://codereview.chromium.org/174364/diff/6086/6091
File webkit/glue/webview_delegate.h (right):

http://codereview.chromium.org/174364/diff/6086/6091#newcode671
Line 671: int x, int y,
can the x,y be expressed using WebPoint instead?  in general, i have been trying
to do that to minimize parameters to our methods.

http://codereview.chromium.org/174364/diff/6086/6090
File webkit/glue/webview_impl.cc (right):

http://codereview.chromium.org/174364/diff/6086/6090#newcode1499
Line 1499: WebKit::WebDragOperation operation) {
nit: please add a using WebKit::WebDragOperation to the top of this file.

http://codereview.chromium.org/174364/diff/6086/6090#newcode1500
Line 1500: if(0)fprintf(stderr,"WebViewImpl::DragSourceEndedAt\n");//TEMP
please remove this debugging line

http://codereview.chromium.org/174364/diff/6086/6090#newcode1506
Line 1506: static_cast<WebCore::DragOperation>(operation));
make sure we have a COMPILE_ASSERT to verify this conversion.  you can see
examples of enum COMPILE_ASSERTS in webkit/api/src/*.cpp

http://codereview.chromium.org/174364/diff/6086/6090#newcode1512
Line 1512: if(0)fprintf(stderr,"WebViewImpl::DragSourceMovedTo\n");//TEMP
delete this

http://codereview.chromium.org/174364/diff/6086/6090#newcode1562
Line 1562: if(0)fprintf(stderr,"WebViewImpl::DragTargetDragEnter:
drag_operation_ = %i\n", drag_operation_); //TEMP
delete this line

http://codereview.chromium.org/174364/diff/6086/6090#newcode1572
Line 1572: if(0)fprintf(stderr,"WebViewImpl::DragTargetDragOver:
operations_allowed = %i\n", operations_allowed); //TEMP
delete this line

http://codereview.chromium.org/174364/diff/6086/6090#newcode1614
Line 1614: if(0)fprintf(stderr,"WebViewImpl::DragTargetDragLeave:
drag_operation_ = %i\n", drag_operation_); //TEMP
delete

http://codereview.chromium.org/174364/diff/6086/6090#newcode1630
Line 1630: if(0)fprintf(stderr,"WebViewImpl::DragTargetDragDrop: At start,
drag_operation_ = %i\n", drag_operation_); //TEMP
delete

http://codereview.chromium.org/174364/diff/6086/6090#newcode1806
Line 1806: void WebViewImpl::StartDragging(int x, int y,
WebPoint instead of x,y

http://codereview.chromium.org/174364/diff/6086/6088
File webkit/glue/webview_impl.h (right):

http://codereview.chromium.org/174364/diff/6086/6088#newcode221
Line 221: void StartDragging(int x, int y,
nit: please use WebPoint instead of x,y

http://codereview.chromium.org/174364/diff/6086/6119
File webkit/tools/test_shell/drag_delegate.cc (right):

http://codereview.chromium.org/174364/diff/6086/6119#newcode39
Line 39: // FIXME(snej): Pass the real drag operation instead
nit: since this is chromium code, use TODO(snej) instead.

http://codereview.chromium.org/174364/diff/6086/6120
File webkit/tools/test_shell/drop_delegate.cc (right):

http://codereview.chromium.org/174364/diff/6086/6120#newcode12
Line 12: using WebKit::WebPoint;
add a using WebKit::WebDragOperation here

http://codereview.chromium.org/174364/diff/6086/6120#newcode30
Line 30: // FIXME(snej): Pass the real drag operation instead
FIXME -> TODO

http://codereview.chromium.org/174364/diff/6086/6093
File webkit/tools/test_shell/event_sending_controller.cc (right):

http://codereview.chromium.org/174364/diff/6086/6093#newcode222
Line 222: if(0)fprintf(stderr,"DRAG: enter DoDragDrop\n");//TEMP
delete this line

http://codereview.chromium.org/174364/diff/6086/6093#newcode232
Line 232: if(0)fprintf(stderr,"DRAG: Started drag at (%i,%i) with
effectsAllowed=%i, dropEffect=%i\n",
delete this line

http://codereview.chromium.org/174364/diff/6086/6094
File webkit/tools/test_shell/event_sending_controller.h (right):

http://codereview.chromium.org/174364/diff/6086/6094#newcode44
Line 44: static void DoDragDrop(int x, int y,
gfx::Point (or WebPoint if that's what the caller has) instead of x,y. 
gfx::Point is what we use for chromium code, but since you are in the
neighborhood of webkit, you might be better off with a WebPoint.

[Jens Alfke, 2009-09-01 23:07:25]

Whew! I think I've addressed everything.

http://codereview.chromium.org/174364/diff/6086/6096
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/174364/diff/6086/6096#newcode279
Line 279: WebKit::WebDragOperationsMask operation_mask) {
On 2009/09/01 18:11:41, darin wrote:
> add using WebKit::WebDragOperation{Mask} to the top of the file

Done.

http://codereview.chromium.org/174364/diff/6086/6101
File chrome/browser/gtk/tab_contents_drag_source.cc (right):

http://codereview.chromium.org/174364/diff/6086/6101#newcode197
Line 197: void TabContentsDragSource::OnDragEnd(WebKit::WebDragOperation
operation) {
On 2009/09/01 18:11:41, darin wrote:
> add a using WebKit::WebDragOperation to the top of the file

Done.

http://codereview.chromium.org/174364/diff/6086/6102
File chrome/browser/gtk/tab_contents_drag_source.h (right):

http://codereview.chromium.org/174364/diff/6086/6102#newcode49
Line 49: // FIXME(snej): Pass actual operation instead of hardcoding copy
On 2009/09/01 18:11:41, darin wrote:
> TODO

Done.

http://codereview.chromium.org/174364/diff/6086/6104
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/174364/diff/6086/6104#newcode464
Line 464: WebKit::WebDragOperationsMask operations_allowed) {
On 2009/09/01 18:11:41, darin wrote:
> add a using directive to the top of the file

Done.

http://codereview.chromium.org/174364/diff/6086/6104#newcode616
Line 616: WebKit::WebDragOperation operation) {
On 2009/09/01 18:11:41, darin wrote:
> add a using directive to the top of the file

Done.

http://codereview.chromium.org/174364/diff/6086/6104#newcode621
Line 621: true, static_cast<uint32>(operation)));
On 2009/09/01 18:11:41, darin wrote:
> please just send the enum directly over ipc

Done.

http://codereview.chromium.org/174364/diff/6086/6105
File chrome/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/174364/diff/6086/6105#newcode526
Line 526: void OnUpdateDragCursor(uint32 drag_operation);
On 2009/09/01 18:11:41, darin wrote:
> do these need to be uint32?  can we instead just use the enums?  you can add
> ParamTraits to webkit_param_traits.h for the enums.

Done.

http://codereview.chromium.org/174364/diff/6086/6098
File chrome/browser/tab_contents/interstitial_page.cc (right):

http://codereview.chromium.org/174364/diff/6086/6098#newcode90
Line 90: WebKit::WebDragOperationsMask operations_allowed);
On 2009/09/01 18:11:41, darin wrote:
> please add using directives for these types

Done.

http://codereview.chromium.org/174364/diff/6086/6118
File chrome/browser/tab_contents/tab_contents_view_gtk.cc (right):

http://codereview.chromium.org/174364/diff/6086/6118#newcode142
Line 142: void UpdateDragStatus(WebKit::WebDragOperation operation) {
On 2009/09/01 18:11:41, darin wrote:
> using directives

Done.

http://codereview.chromium.org/174364/diff/6086/6118#newcode148
Line 148: // FIXME(snej): Pass appropriate GDK action instead of hardcoding COPY
On 2009/09/01 18:11:41, darin wrote:
> TODO

Done.

http://codereview.chromium.org/174364/diff/6086/6118#newcode209
Line 209: // FIXME(snej): Pass appropriate DragOperation instead of hardcoding
On 2009/09/01 18:11:41, darin wrote:
> TODO

Done.

http://codereview.chromium.org/174364/diff/6086/6118#newcode274
Line 274: // FIXME(snej): Pass appropriate DragOperation instead of hardcoding
On 2009/09/01 18:11:41, darin wrote:
> TODO

Done.

http://codereview.chromium.org/174364/diff/6086/6116
File chrome/browser/tab_contents/web_drag_source.cc (right):

http://codereview.chromium.org/174364/diff/6086/6116#newcode71
Line 71: // FIXME(jpa): This needs to be fixed to send the actual drag
operation.
On 2009/09/01 18:11:41, darin wrote:
> TODO

Done.

http://codereview.chromium.org/174364/diff/6086/6114
File chrome/browser/views/tab_contents/tab_contents_view_win.cc (right):

http://codereview.chromium.org/174364/diff/6086/6114#newcode193
Line 193: // FIXME(snej): Use 'ops' param instead of hardcoding dropeffects
On 2009/09/01 18:11:41, darin wrote:
> TODO

Done.

http://codereview.chromium.org/174364/diff/6086/6110
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/174364/diff/6086/6110#newcode338
Line 338: uint32 /* operations_allowed */)
On 2009/09/01 18:11:41, darin wrote:
> please just use the enum types directly

Done.

http://codereview.chromium.org/174364/diff/6086/6110#newcode1111
Line 1111: uint32 /* drag_operations_mask */)
On 2009/09/01 18:11:41, darin wrote:
> use enum types here

Done.

http://codereview.chromium.org/174364/diff/6086/6112
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/174364/diff/6086/6112#newcode2138
Line 2138: WebKit::WebDragOperationsMask allowed_ops) {
On 2009/09/01 18:11:41, darin wrote:
> add using directive for WebDragOperationsMask

Done.

http://codereview.chromium.org/174364/diff/6086/6111
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/174364/diff/6086/6111#newcode531
Line 531: uint32 operations_allowed);
On 2009/09/01 18:11:41, darin wrote:
> enum names

Done.

http://codereview.chromium.org/174364/diff/6086/6122
File webkit/api/public/WebDragOperation.h (right):

http://codereview.chromium.org/174364/diff/6086/6122#newcode39
Line 39: // destination.
On 2009/09/01 18:11:41, darin wrote:
> nit: kill the new line here?  so you get a paragraph.

I want to keep the second part on a separate line because it's a different
topic, and to keep it more visible since it's important.

http://codereview.chromium.org/174364/diff/6086/6122#newcode50
Line 50: WebDragOperationEvery   = UINT_MAX
On 2009/09/01 18:11:41, darin wrote:
> nit: how about we just use -1 here?  alternatively, i think it is more common
in
> webkit code to include <limits> and then use
> std::numeric_limits<unsigned>::max() for this.  although i worry about that
max
> colliding with the max in windows.h :-/

It's declared exactly like this in WebCore/page/DragActions.h. I'm leery of
changing it in any way, because this enum is just a shadow of the one declared
in WebKit and needs to behave identically.

http://codereview.chromium.org/174364/diff/6086/6122#newcode57
Line 57: }  // namespace WebKit
On 2009/09/01 18:11:41, darin wrote:
> nit: webkit style is to only have one space before the '//'

Done.

http://codereview.chromium.org/174364/diff/6086/6089
File webkit/glue/dragclient_impl.cc (right):

http://codereview.chromium.org/174364/diff/6086/6089#newcode81
Line 81: static_cast<WebKit::WebDragOperationsMask>(drag_operation_mask));
On 2009/09/01 18:11:41, darin wrote:
> it would be good to have some COMPILE_ASSERTS somewhere the ensure that these
> enum values stay in sync.

OK, I've added them to webview_impl.cc.

http://codereview.chromium.org/174364/diff/6086/6091
File webkit/glue/webview_delegate.h (right):

http://codereview.chromium.org/174364/diff/6086/6091#newcode671
Line 671: int x, int y,
On 2009/09/01 18:11:41, darin wrote:
> can the x,y be expressed using WebPoint instead?  in general, i have been
trying
> to do that to minimize parameters to our methods.

Done.

http://codereview.chromium.org/174364/diff/6086/6090
File webkit/glue/webview_impl.cc (right):

http://codereview.chromium.org/174364/diff/6086/6090#newcode1499
Line 1499: WebKit::WebDragOperation operation) {
On 2009/09/01 18:11:41, darin wrote:
> nit: please add a using WebKit::WebDragOperation to the top of this file.

Done.

http://codereview.chromium.org/174364/diff/6086/6090#newcode1500
Line 1500: if(0)fprintf(stderr,"WebViewImpl::DragSourceEndedAt\n");//TEMP
On 2009/09/01 18:11:41, darin wrote:
> please remove this debugging line

Done.

http://codereview.chromium.org/174364/diff/6086/6090#newcode1506
Line 1506: static_cast<WebCore::DragOperation>(operation));
On 2009/09/01 18:11:41, darin wrote:
> make sure we have a COMPILE_ASSERT to verify this conversion.  you can see
> examples of enum COMPILE_ASSERTS in webkit/api/src/*.cpp

Done.

http://codereview.chromium.org/174364/diff/6086/6090#newcode1512
Line 1512: if(0)fprintf(stderr,"WebViewImpl::DragSourceMovedTo\n");//TEMP
On 2009/09/01 18:11:41, darin wrote:
> delete this

Done.

http://codereview.chromium.org/174364/diff/6086/6090#newcode1562
Line 1562: if(0)fprintf(stderr,"WebViewImpl::DragTargetDragEnter:
drag_operation_ = %i\n", drag_operation_); //TEMP
On 2009/09/01 18:11:41, darin wrote:
> delete this line

Done.

http://codereview.chromium.org/174364/diff/6086/6090#newcode1572
Line 1572: if(0)fprintf(stderr,"WebViewImpl::DragTargetDragOver:
operations_allowed = %i\n", operations_allowed); //TEMP
On 2009/09/01 18:11:41, darin wrote:
> delete this line

Done.

http://codereview.chromium.org/174364/diff/6086/6090#newcode1614
Line 1614: if(0)fprintf(stderr,"WebViewImpl::DragTargetDragLeave:
drag_operation_ = %i\n", drag_operation_); //TEMP
On 2009/09/01 18:11:41, darin wrote:
> delete

Done.

http://codereview.chromium.org/174364/diff/6086/6090#newcode1630
Line 1630: if(0)fprintf(stderr,"WebViewImpl::DragTargetDragDrop: At start,
drag_operation_ = %i\n", drag_operation_); //TEMP
On 2009/09/01 18:11:41, darin wrote:
> delete

Done.

http://codereview.chromium.org/174364/diff/6086/6090#newcode1806
Line 1806: void WebViewImpl::StartDragging(int x, int y,
On 2009/09/01 18:11:41, darin wrote:
> WebPoint instead of x,y

Done.

http://codereview.chromium.org/174364/diff/6086/6088
File webkit/glue/webview_impl.h (right):

http://codereview.chromium.org/174364/diff/6086/6088#newcode221
Line 221: void StartDragging(int x, int y,
On 2009/09/01 18:11:41, darin wrote:
> nit: please use WebPoint instead of x,y

Done.

http://codereview.chromium.org/174364/diff/6086/6119
File webkit/tools/test_shell/drag_delegate.cc (right):

http://codereview.chromium.org/174364/diff/6086/6119#newcode39
Line 39: // FIXME(snej): Pass the real drag operation instead
On 2009/09/01 18:11:41, darin wrote:
> nit: since this is chromium code, use TODO(snej) instead.

Done.

http://codereview.chromium.org/174364/diff/6086/6120
File webkit/tools/test_shell/drop_delegate.cc (right):

http://codereview.chromium.org/174364/diff/6086/6120#newcode12
Line 12: using WebKit::WebPoint;
On 2009/09/01 18:11:41, darin wrote:
> add a using WebKit::WebDragOperation here

Done.

http://codereview.chromium.org/174364/diff/6086/6120#newcode30
Line 30: // FIXME(snej): Pass the real drag operation instead
On 2009/09/01 18:11:41, darin wrote:
> FIXME -> TODO

Done.

http://codereview.chromium.org/174364/diff/6086/6093
File webkit/tools/test_shell/event_sending_controller.cc (right):

http://codereview.chromium.org/174364/diff/6086/6093#newcode222
Line 222: if(0)fprintf(stderr,"DRAG: enter DoDragDrop\n");//TEMP
On 2009/09/01 18:11:41, darin wrote:
> delete this line

Done.

http://codereview.chromium.org/174364/diff/6086/6093#newcode232
Line 232: if(0)fprintf(stderr,"DRAG: Started drag at (%i,%i) with
effectsAllowed=%i, dropEffect=%i\n",
On 2009/09/01 18:11:41, darin wrote:
> delete this line

Done.

http://codereview.chromium.org/174364/diff/6086/6094
File webkit/tools/test_shell/event_sending_controller.h (right):

http://codereview.chromium.org/174364/diff/6086/6094#newcode44
Line 44: static void DoDragDrop(int x, int y,
On 2009/09/01 18:11:41, darin wrote:
> gfx::Point (or WebPoint if that's what the caller has) instead of x,y. 
> gfx::Point is what we use for chromium code, but since you are in the
> neighborhood of webkit, you might be better off with a WebPoint.

Done.

[darin (slow to review), 2009-09-01 23:12:45]

http://codereview.chromium.org/174364/diff/6086/6122
File webkit/api/public/WebDragOperation.h (right):

http://codereview.chromium.org/174364/diff/6086/6122#newcode39
Line 39: // destination.
On 2009/09/01 23:07:28, Jens Alfke wrote:
> On 2009/09/01 18:11:41, darin wrote:
> > nit: kill the new line here?  so you get a paragraph.
> 
> I want to keep the second part on a separate line because it's a different
> topic, and to keep it more visible since it's important.

ok

http://codereview.chromium.org/174364/diff/6086/6122#newcode50
Line 50: WebDragOperationEvery   = UINT_MAX
On 2009/09/01 23:07:28, Jens Alfke wrote:
> On 2009/09/01 18:11:41, darin wrote:
> > nit: how about we just use -1 here?  alternatively, i think it is more
common
> in
> > webkit code to include <limits> and then use
> > std::numeric_limits<unsigned>::max() for this.  although i worry about that
> max
> > colliding with the max in windows.h :-/
> 
> It's declared exactly like this in WebCore/page/DragActions.h. I'm leery of
> changing it in any way, because this enum is just a shadow of the one declared
> in WebKit and needs to behave identically.

OK... so long as the COMPILE_ASSERT is in place.  i wouldn't put it past them to
change WebCore to use something other than UINT_MAX in the future.

http://codereview.chromium.org/174364/diff/6086/6089
File webkit/glue/dragclient_impl.cc (right):

http://codereview.chromium.org/174364/diff/6086/6089#newcode81
Line 81: static_cast<WebKit::WebDragOperationsMask>(drag_operation_mask));
On 2009/09/01 23:07:28, Jens Alfke wrote:
> On 2009/09/01 18:11:41, darin wrote:
> > it would be good to have some COMPILE_ASSERTS somewhere the ensure that
these
> > enum values stay in sync.
> 
> OK, I've added them to webview_impl.cc.

ok

[darin (slow to review), 2009-09-01 23:22:54]

Just a few more issues...

http://codereview.chromium.org/174364/diff/7018/3039
File chrome/browser/cocoa/web_drop_target.mm (right):

http://codereview.chromium.org/174364/diff/7018/3039#newcode92
Line 92: static_cast<WebKit::WebDragOperationsMask>(mask));
oh, wow... so these enums are supposed to match cocoa enum values?  can we get
some compiler assertions to help with those too?

same nit about using WebKit::Foo directives in this file.

http://codereview.chromium.org/174364/diff/7018/3056
File chrome/common/common_param_traits.h (right):

http://codereview.chromium.org/174364/diff/7018/3056#newcode334
Line 334: struct ParamTraits<WebDragOperation> {
please put this in webkit_param_traits.h.  that file is for all WebKit types,
which get serialized over IPC.

http://codereview.chromium.org/174364/diff/7018/3023
File webkit/glue/webview_impl.cc (right):

http://codereview.chromium.org/174364/diff/7018/3023#newcode140
Line 140: #define COMPILE_ASSERT_MATCHING_ENUM(webcoreName) \
nit: webcore_name

http://codereview.chromium.org/174364/diff/7018/3026
File webkit/tools/test_shell/event_sending_controller.cc (right):

http://codereview.chromium.org/174364/diff/7018/3026#newcode224
Line 224: gfx::Point(event_pos.x, event_pos.y), &event);
WebPoint auto-casts to gfx::Point, so you can just pass the event_pos parameter
directly.

http://codereview.chromium.org/174364/diff/7018/3025
File webkit/tools/test_shell/test_webview_delegate.cc (right):

http://codereview.chromium.org/174364/diff/7018/3025#newcode621
Line 621: WebKit::WebDragOperationsMask mask) {
nit: using WebKit::Foo

[Jens Alfke, 2009-09-01 23:52:41]

Darin: I've addressed your remaining issues, I think.

Mike: Could you look at the three .mm files here? The diffs are very minor, I'm
just passing the current NSDragOperation into Chromium in a few places. Thanks!

http://codereview.chromium.org/174364/diff/7018/3039
File chrome/browser/cocoa/web_drop_target.mm (right):

http://codereview.chromium.org/174364/diff/7018/3039#newcode92
Line 92: static_cast<WebKit::WebDragOperationsMask>(mask));
On 2009/09/01 23:22:54, darin wrote:
> oh, wow... so these enums are supposed to match cocoa enum values?  can we get
> some compiler assertions to help with those too?
> 
> same nit about using WebKit::Foo directives in this file.

Yeah, WebCore::DragOperation is basically a copy-and-paste of NSDragOperation,
probably created during the initial Windows port of WebKit. I trust the WebKit
folks to keep these in sync because Mac Safari would break if they diverged.

"using": Fixed.

http://codereview.chromium.org/174364/diff/7018/3056
File chrome/common/common_param_traits.h (right):

http://codereview.chromium.org/174364/diff/7018/3056#newcode334
Line 334: struct ParamTraits<WebDragOperation> {
On 2009/09/01 23:22:54, darin wrote:
> please put this in webkit_param_traits.h.  that file is for all WebKit types,
> which get serialized over IPC.

Done.

http://codereview.chromium.org/174364/diff/7018/3023
File webkit/glue/webview_impl.cc (right):

http://codereview.chromium.org/174364/diff/7018/3023#newcode140
Line 140: #define COMPILE_ASSERT_MATCHING_ENUM(webcoreName) \
On 2009/09/01 23:22:54, darin wrote:
> nit: webcore_name

Done.

http://codereview.chromium.org/174364/diff/7018/3026
File webkit/tools/test_shell/event_sending_controller.cc (right):

http://codereview.chromium.org/174364/diff/7018/3026#newcode224
Line 224: gfx::Point(event_pos.x, event_pos.y), &event);
On 2009/09/01 23:22:54, darin wrote:
> WebPoint auto-casts to gfx::Point, so you can just pass the event_pos
parameter
> directly.

Done.

http://codereview.chromium.org/174364/diff/7018/3025
File webkit/tools/test_shell/test_webview_delegate.cc (right):

http://codereview.chromium.org/174364/diff/7018/3025#newcode621
Line 621: WebKit::WebDragOperationsMask mask) {
On 2009/09/01 23:22:54, darin wrote:
> nit: using WebKit::Foo

Done.

[pink (ping after 24hrs), 2009-09-02 14:34:43]

Mac stuff LGTM (w/ one nit) as long as we're sure the WebKit constants will
always match the cocoa ones, like Darin says.

http://codereview.chromium.org/174364/diff/7019/3072
File chrome/browser/tab_contents/tab_contents_view_mac.mm (right):

http://codereview.chromium.org/174364/diff/7019/3072#newcode350
Line 350: return [dragSource_ draggingSourceOperationMaskForLocal: isLocal];
no space after colon

[Jens Alfke, 2009-09-02 16:04:38]

http://codereview.chromium.org/174364/diff/7019/3072
File chrome/browser/tab_contents/tab_contents_view_mac.mm (right):

http://codereview.chromium.org/174364/diff/7019/3072#newcode350
Line 350: return [dragSource_ draggingSourceOperationMaskForLocal: isLocal];
On 2009/09/02 14:34:43, pink wrote:
> no space after colon

Done.

[darin (slow to review), 2009-09-02 16:15:36]

http://codereview.chromium.org/174364/diff/7018/3039
File chrome/browser/cocoa/web_drop_target.mm (right):

http://codereview.chromium.org/174364/diff/7018/3039#newcode92
Line 92: static_cast<WebKit::WebDragOperationsMask>(mask));
On 2009/09/01 23:52:41, Jens Alfke wrote:
> On 2009/09/01 23:22:54, darin wrote:
> > oh, wow... so these enums are supposed to match cocoa enum values?  can we
get
> > some compiler assertions to help with those too?
> > 
> > same nit about using WebKit::Foo directives in this file.
> 
> Yeah, WebCore::DragOperation is basically a copy-and-paste of NSDragOperation,
> probably created during the initial Windows port of WebKit. I trust the WebKit
> folks to keep these in sync because Mac Safari would break if they diverged.

I think we should still have COMPILE_ASSERT (for this code, use the one defined
in base/basictypes.h).  Imagine if in the future they redefine the WebCore types
and implement a case-switch to map between NSDragOperation and the WebCore
types.  Our compile asserts at the WebKit level won't help here since eventually
those will live upstream, so the WebKit enums will be fixed with the WebCore
types in the same CL.  Chrome will then start failing only on Mac in a silent
way.

[Jens Alfke, 2009-09-02 16:30:53]

http://codereview.chromium.org/174364/diff/7018/3039
File chrome/browser/cocoa/web_drop_target.mm (right):

http://codereview.chromium.org/174364/diff/7018/3039#newcode92
Line 92: static_cast<WebKit::WebDragOperationsMask>(mask));
On 2009/09/02 16:15:36, darin wrote:
> On 2009/09/01 23:52:41, Jens Alfke wrote:
> > On 2009/09/01 23:22:54, darin wrote:
> > > oh, wow... so these enums are supposed to match cocoa enum values?  can we
> get
> > > some compiler assertions to help with those too?
> > > 
> > > same nit about using WebKit::Foo directives in this file.
> > 
> > Yeah, WebCore::DragOperation is basically a copy-and-paste of
NSDragOperation,
> > probably created during the initial Windows port of WebKit. I trust the
WebKit
> > folks to keep these in sync because Mac Safari would break if they diverged.
> 
> I think we should still have COMPILE_ASSERT (for this code, use the one
defined
> in base/basictypes.h).  Imagine if in the future they redefine the WebCore
types
> and implement a case-switch to map between NSDragOperation and the WebCore
> types.  Our compile asserts at the WebKit level won't help here since
eventually
> those will live upstream, so the WebKit enums will be fixed with the WebCore
> types in the same CL.  Chrome will then start failing only on Mac in a silent
> way.

Oh, good point. Done -- I put it in tab_contents_view_mac.mm, which is where
most of those static_casts happen.

[darin (slow to review), 2009-09-02 16:32:57]

LGTM