[webkit-reviews] review granted: [Bug 52343] WebKit2: add support for drag and drop : [Attachment 78848] Patch5

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Enrica Casucci <enrica at apple.com>'s
request for review:
Bug 52343: WebKit2: add support for drag and drop
https://bugs.webkit.org/show_bug.cgi?id=52343

Attachment 78848: Patch5
https://bugs.webkit.org/attachment.cgi?id=78848&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=78848&action=review

The most serious mistake here is the missing return statement in
performDragControllerAction.

> Source/WebCore/page/mac/DragControllerMac.mm:57
> +    if (!m_documentUnderMouse || (!(dragData->flags() &
DragApplicationHasAttachedSheet) && !(dragData->flags() &
DragApplicationIsSource)))

It occurs to me this could also be written like this:

    if (!m_documentUnderMouse || !(dragData->flags() &
(DragApplicationHasAttachedSheet | DragApplicationIsSource)))

I like that a little better than what you have here.

> Source/WebCore/platform/mac/DragDataMac.mm:48
> +    m_pasteboard = [m_platformDragData draggingPasteboard];

Why not use construction syntax instead of assignment syntax for this?

> Source/WebCore/platform/mac/DragDataMac.mm:51
> +DragData::DragData(const String& dragStorageName,  const IntPoint&
clientPosition, const IntPoint& globalPosition,

Extra space here after the first comma.

> Source/WebCore/platform/mac/DragDataMac.mm:59
> +    m_pasteboard = [NSPasteboard pasteboardWithName:dragStorageName];

Why not use construction syntax instead of assignment syntax for this?

> WebKit2/Shared/DragControllerActions.h:2
> + * Copyright (C) 2010 Apple Inc. All rights reserved.

2011

> WebKit2/Shared/DragControllerActions.h:27
> +#ifndef DragControllerActions_h
> +#define DragControllerActions_h

I think normally we name the file after the enum, rather than including an “s”.


> WebKit2/Shared/DragControllerActions.h:35
> +    DragControllerActionEntered = 0,
> +    DragControllerActionUpdated = 1,
> +    DragControllerActionExited = 2,
> +    DragControllerActionPerformDrag = 3

I suggest we let the compiler generate the values rather than specifying them.

> WebKit2/UIProcess/WebPageProxy.cpp:602
> +    m_currentDragOperation = (DragOperation)resultOperation;

Please use a C++-style cast.

> WebKit2/UIProcess/WebPageProxy.h:277
> +    void didPerformDragControllerAction(uint64_t resultOperation);

64-bit seems a bit overkill for 2 bits. Might check with Anders what the best
practice is.

> WebKit2/UIProcess/API/mac/WKView.mm:172
> +    NSArray *URLTypes = [NSArray arrayWithObjects:
WebURLsWithTitlesPboardType, NSURLPboardType, WebURLPboardType, 
WebURLNamePboardType, NSStringPboardType, NSFilenamesPboardType, nil];

Extraneous space after colon here.

> WebKit2/WebProcess/WebPage/WebPage.cpp:1260
> +	  
send(Messages::WebPageProxy::DidPerformDragControllerAction(m_page->dragControl
ler()->dragEntered(&dragData)));

Why no null check for m_page here?

> WebKit2/WebProcess/WebPage/WebPage.cpp:1265
> +	   if (!m_page)
> +	      
send(Messages::WebPageProxy::DidPerformDragControllerAction(DragOperationNone))
;

Need a return here inside this if statement; otherwise we null-deref on the
next line.

> WebKit2/WebProcess/WebPage/WebPage.cpp:1277
> +	   m_page->dragController()->performDrag(&dragData);

Why no null check for m_page here?

> WebKit2/WebProcess/WebPage/WebPage.cpp:1281
> +    default:
> +	   ASSERT_NOT_REACHED();

For the record, this is not what I suggested. My suggestion is to leave out the
default, end individual cases with return, and put ASSERT_NOT_REACHED() after
the switch statement. This gets both a compile time check that all enums are
covered because the compiler will warn if an enum is not covered, and a runtime
check, outside the switch statement.

But my style would only work if the type of action was the enum type, not an
integer type.

> WebKit/mac/WebView/WebView.mm:3779
> +    return (DragApplicationFlags)flags;

C++-style cast please