[webkit-reviews] review denied: [Bug 52343] WebKit2: add support for drag and drop : [Attachment 78772] Patch3

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 78772: Patch3
https://bugs.webkit.org/attachment.cgi?id=78772&action=review

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

Looks good.

Breaks the Qt and Windows builds, so please upload at least one more version of
the patch that fixes those issues and some of the comments as well.

>> Source/WebCore/page/DragController.h:55

> 
> Should this enum be in WebKit?  It doesn't appear to be used in WebCore.

Good point. I think this should be in WebKit2.

> Source/WebCore/page/mac/DragControllerMac.mm:58
> -    if (!m_documentUnderMouse ||
(![[m_page->mainFrame()->view()->getOuterView() window] attachedSheet] 
> -	   && [dragData->platformData() draggingSource] !=
m_page->mainFrame()->view()->getOuterView()))
> +    if (!m_documentUnderMouse || (!(dragData->flags() &
DragApplicationHasAttachedSheet) 
> +				     && !(dragData->flags() &
DragApplicationIsSource)))

Indentation here before followed WebKit style. We don't align things with
parentheses since this requires reindenting when we rename things. Putting it
all one one line might be OK. If the logic is to confusing like that, then an
inline helper function is one way to make it clearer.

> Source/WebCore/platform/DragData.h:77
> +enum DragApplicationFlags {

We need to get our style straight for flags. If you look in RenderLayer.h at
the PaintLayerFlag/PaintLayerFlags type you’ll see a style that I thought was
our standard, with some benefits, but I see different styles in different parts
of the code.

> Source/WebCore/platform/DragData.h:91
> +    DragData(DragDataRef, const IntPoint& clientPosition, const IntPoint&
globalPosition, DragOperation operation, DragApplicationFlags flags);

No need for the argument names “operation” and “flags”.

> Source/WebCore/platform/DragData.h:120


I am almost certain this needs to be RetainPtr<NSPasteboard>.

Another way to do it would be to have this be a WebCore::Pasteboard and keep
the #ifdefs down a bit, but I suppose on other platforms pasteboard and drag
don’t have much to do with each other. Platform #if code mixed into a
platform-independent header or class is distasteful and harder to maintain and
we should work hard to avoid it.

> Source/WebCore/platform/DragData.h:121
> +    DragApplicationFlags m_applicationFlags;

Seems we could just initialize this to none on platforms that are not passing
any flags in. I don’t think this data member needs to be Mac-only.

> WebKit2/UIProcess/WebPageProxy.h:79
> +    class DragData;

Should be sorted alphabetically with other classes. We normally put the classes
before the structs.

> WebKit2/UIProcess/WebPageProxy.h:603
> -
> +    

Shouldn’t add this whitespace.

> WebKit2/UIProcess/API/mac/WKView.mm:163
> +NSString *WebArchivePboardType = @"Apple Web Archive pasteboard type";
> +NSString *WebURLsWithTitlesPboardType = @"WebURLsWithTitlesPboardType";
> +NSString *WebURLPboardType = @"public.url";
> +NSString *WebURLNamePboardType = @"public.url-name";

Since these are used only within this file, they should be marked static so we
get internal linkage. Since they are constant they should be const. Like this:

    static NSString * const WebArchivePboardType = @"...";

> WebKit2/UIProcess/API/mac/WKView.mm:175
> +    static NSArray *types = nil;
> +    if (!types) {
> +	   types = [[NSArray alloc] initWithObjects:WebArchivePboardType,
NSHTMLPboardType, NSFilenamesPboardType, NSTIFFPboardType, NSPDFPboardType,
> +#if defined(BUILDING_ON_TIGER) || defined(BUILDING_ON_LEOPARD)
> +		    NSPICTPboardType,
> +#endif
> +		    NSURLPboardType, NSRTFDPboardType, NSRTFPboardType,
NSStringPboardType, NSColorPboardType, kUTTypePNG, nil];
> +	   CFRetain(types);
> +    }

Since this is C++ code this can be done without the if statement. A separate
method can be used to allocate the array, and called inside the initializer
rather than initializing to nil.

We indent by 4, not by lining things up, so we don’t have to reindent if we
rename.

> WebKit2/UIProcess/API/mac/WKView.mm:185
> +					  NSStringPboardType,
> +					  NSFilenamesPboardType,
> +					  nil];

Should just be indented by 4, not way in like this. Or could all go on one
line.

> WebKit2/UIProcess/API/mac/WKView.mm:1028
> +    return (DragApplicationFlags)flags;

Use a C++-style cast please.

> WebKit2/UIProcess/API/mac/WKView.mm:1035
> +    DragData dragData(draggingInfo, client, global,
(DragOperation)[draggingInfo draggingSourceOperationMask], [self
applicationFlags:draggingInfo]);

This should also be a C++-style cast, possibly with a local variable if it
makes the line too long.

> WebKit2/UIProcess/API/mac/WKView.mm:1045
> +    DragData dragData(draggingInfo, client, global,
(DragOperation)[draggingInfo draggingSourceOperationMask], [self
applicationFlags:draggingInfo]);

Ditto.

> WebKit2/UIProcess/API/mac/WKView.mm:1054
> +    DragData dragData(draggingInfo, client, global,
(DragOperation)[draggingInfo draggingSourceOperationMask], [self
applicationFlags:draggingInfo]);

Ditto.

> WebKit2/UIProcess/API/mac/WKView.mm:1068
> +    DragData dragData(draggingInfo, client, global,
(DragOperation)[draggingInfo draggingSourceOperationMask], [self
applicationFlags:draggingInfo]);

And again.

Also, the code to create a DragData should be refactored into a helper function
or helper method rather than being repeated three times.

> WebKit2/WebProcess/WebPage/WebPage.cpp:1258
> +    switch (action) {
> +	   case WebCore::DragControllerActionEntered:

WebKit coding style puts the case under the switch rather than indented.

> WebKit2/WebProcess/WebPage/WebPage.cpp:1280
> +	   default:
> +	       break;

If possible we should omit the default case. All it does is quiet the warning
that tells us to handle all the cases. Best style is to return from the cases,
include cases for all values and then ASSERT_NOT_REACHED after the switch.

> WebKit2/WebProcess/WebPage/WebPage.h:301
> +    void performDragControllerAction(uint64_t, WebCore::IntPoint,
WebCore::IntPoint, uint64_t, const WTF::String&, uint32_t);

The arguments here need names. The types alone do not make their purpose clear.


> WebKit2/WebProcess/WebPage/WebPage.messages.in:101
>      CountStringMatches(WTF::String string, uint32_t findOptions, unsigned
maxMatchCount)
>  
> +    # Drag and drop.
> +    PerformDragControllerAction(uint64_t action, WebCore::IntPoint
clientPosition, WebCore::IntPoint globalPosition, uint64_t
draggingSourceOperationMask, WTF::String dragStorageName, uint32_t flags)
>      # Popup menu.

Should have a blank line after the new function to match the format of the rest
of the file.

> WebKit/mac/WebView/WebView.mm:3780
> +- (DragApplicationFlags)applicationFlags:(id <NSDraggingInfo>)draggingInfo
> +{
> +    uint32_t flags = 0;
> +	   flags = DragApplicationIsModal;
> +    if ([[self window] attachedSheet])
> +	   flags |= DragApplicationHasAttachedSheet;
> +    if ([draggingInfo draggingSource] == self)
> +	   flags |= DragApplicationIsSource;
> +    if ([[NSApp currentEvent] modifierFlags] & NSAlternateKeyMask)
> +	   flags |= DragApplicationIsCopyKeyDown;
> +    return (DragApplicationFlags)flags;
> +}

Maybe this function could be in WebCore instead.

> WebKit/mac/WebView/WebView.mm:3787
>      IntPoint client([draggingInfo draggingLocation]);
>      IntPoint global(globalPoint([draggingInfo draggingLocation], [self
window]));
> -    DragData dragData(draggingInfo, client, global,
(DragOperation)[draggingInfo draggingSourceOperationMask]);
> +    DragData dragData(draggingInfo, client, global,
(DragOperation)[draggingInfo draggingSourceOperationMask], [self
applicationFlags:draggingInfo]);
>      return core(self)->dragController()->dragEntered(&dragData);

I think the same refactoring would be good here that I suggested in WebKit2
code.