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

[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 79454: Patch9
https://bugs.webkit.org/attachment.cgi?id=79454&action=review

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

review- because of the memory leak in PageClientImpl::setDragImage.

Otherwise seems all right.

> Source/WebKit2/Shared/mac/PasteboardTypes.h:34
> +extern NSString * const WebArchivePboardType;
> +extern NSString * const WebURLsWithTitlesPboardType;
> +extern NSString * const WebURLPboardType;
> +extern NSString * const WebURLNamePboardType;

I think it would be better to export functions for these, as you have done with
the arrays below.

> Source/WebKit2/Shared/mac/PasteboardTypes.h:38
> +public:
> +    

There’s an extra blank line here.

> Source/WebKit2/Shared/mac/PasteboardTypes.h:42
> +    static NSArray* forEditing();
> +    static NSArray* forURL();
> +    static NSArray* forImages();
> +    static NSArray* forImagesWithArchive();

The space goes before the * for Objective-C types like NSArray.

> Source/WebKit2/Shared/mac/PasteboardTypes.mm:2
> + * Copyright (C) 2010 Apple Inc. All rights reserved.

2011

> Source/WebKit2/Shared/mac/PasteboardTypes.mm:42
> +    static RetainPtr<NSArray> types = [NSArray
arrayWithObjects:WebArchivePboardType, NSHTMLPboardType, NSFilenamesPboardType,
NSTIFFPboardType, NSPDFPboardType,
> +#if defined(BUILDING_ON_TIGER) || defined(BUILDING_ON_LEOPARD)
> +	   NSPICTPboardType,
> +#endif
> +	   NSURLPboardType, NSRTFDPboardType, NSRTFPboardType,
NSStringPboardType, NSColorPboardType, kUTTypePNG, nil];

Using a RetainPtr in a global variable like this will create a static
destructor that will run at process exit time. There are other idioms that will
not do this, and so are preferred. For example, putting something into a
RetainPtr but then using leakPtr and a raw pointer for the global. Or using
CFRetain on the object, but that will probably require a typecast as well.

> Source/WebKit2/UIProcess/PageClient.h:104
>      virtual void interceptKeyEvent(const NativeWebKeyboardEvent&,
Vector<WebCore::KeypressCommand>&, uint32_t, uint32_t,
Vector<WebCore::CompositionUnderline>&) = 0;
> +    virtual void setDragImage(const WebCore::IntPoint&, const
WebCore::IntSize&, WTF::PassRefPtr<ShareableBitmap>, bool) = 0;
> +    virtual void didEndDrag(const WebCore::IntPoint&) = 0;

No need for WTF:: in WTF::PassRefPtr.

The meanings of these arguments are not clear from the types alone, so the
argument names should be included. Same for the interceptKeyEvent function
above, although not for the event itself.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:626
> +    m_pageClient->setDragImage(clientPosition, imageSize, dragImage,
isLinkDrag);

Should be dragImage.release().

> Source/WebKit2/UIProcess/API/mac/PageClientImpl.mm:284
> +    NSImage* dragNSImage = [[[NSImage alloc]
initWithCGImage:CGBitmapContextCreateImage(dragImage->createGraphicsContext()->
platformContext()) size:imageSize] autorelease];

This leaks both the CGBitmapContext, and the NSImage. Both need to be adopted
into smart pointers so they won’t leak.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:760
> +- (void)_mouseHandler:(NSEvent *)theEvent

You should use the argument name “event” rather than “theEvent” in new code.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1065
> +    // Once the dragging machinery kicks in, we no longer get mouse drags or
the up event.
> +    // WebCore expects to get balanced down/up's, so we must fake up a
mouseup.

I’m not sure we need a fake NSEvent in WebKit2. In WebKit1 we did, because the
NSEvent was visible to WebCore, but in WebKit2 that is not the case. OK to do
this for now, but it’s almost certainly not needed.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1082
> +   _data->_ignoringMouseDraggedEvents = YES;

This line is not indented correctly.

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebDragClientMac.mm:2
> + * Copyright (C) 2010 Apple Inc. All rights reserved.

2011

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebDragClientMac.mm:42
> +#import <WebKit/WebKitNSStringExtras.h>
> +#import <WebKit/WebNSURLExtras.h>

Do we really need to use these WebKit methods in WebKit2? I think we’d like to
avoid that when possible, but I suppose it’s not practical.

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebDragClientMac.mm:74
> +    FontPlatformData f(font, [font pointSize]);
> +    currentRenderer = Font(f, ![[NSGraphicsContext currentContext]
isDrawingToScreen]);

I assume this code was copied from somewhere else. "f" is a very poor name for
this local variable, though. I’m not even sure the local variable is needed.