Bug 52343 – WebKit2: add support for drag and drop

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Enrica Casucci]

Need to be able to drag to and from WebKit.

[Enrica Casucci]

Created attachment 78767 [details]
Patch

Initial work to support WebKit as drop target.

[Enrica Casucci]

Created attachment 78768 [details]
Patch2

Sorry wrong patch.

[WebKit Review Bot]

Attachment 78767 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/DragData.h:90:  The parameter name "operation" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/DragData.h:90:  The parameter name "flags" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/DragData.h:91:  The parameter name "operation" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/DragData.h:91:  The parameter name "flags" adds no information, so it should be removed.  [readability/parameter_name] [5]
WebKit2/WebProcess/WebPage/WebPage.cpp:1258:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 5 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 78768 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/DragData.h:90:  The parameter name "operation" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/DragData.h:90:  The parameter name "flags" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/DragData.h:91:  The parameter name "operation" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/DragData.h:91:  The parameter name "flags" adds no information, so it should be removed.  [readability/parameter_name] [5]
WebKit2/WebProcess/WebPage/WebPage.cpp:1258:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 5 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 78768 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7533003

[WebKit Review Bot]

Attachment 78768 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7601004

[Early Warning System Bot]

Attachment 78768 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7617001

[Enrica Casucci]

Created attachment 78772 [details]
Patch3

Fixing other platforms.

[WebKit Review Bot]

Attachment 78772 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/DragData.h:91:  The parameter name "operation" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/DragData.h:91:  The parameter name "flags" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/DragData.h:92:  The parameter name "operation" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/DragData.h:92:  The parameter name "flags" adds no information, so it should be removed.  [readability/parameter_name] [5]
WebKit2/WebProcess/WebPage/WebPage.cpp:1258:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 5 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Levin]

Comment on attachment 78772 [details]
Patch3

View in context: https://bugs.webkit.org/attachment.cgi?id=78772&action=review

It looks like the stylebot noted a number of valid issues plus I noticed a few other things below.

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

Use c++ style cast.

> WebKit2/WebProcess/WebPage/WebPage.cpp:1256
> +    DragData dragData(dragStorageName, clientPosition, globalPosition, (DragOperation)draggingSourceOperationMask, (DragApplicationFlags)flags);

Use c++ style casts.

[Build Bot]

Attachment 78768 [details] did not build on win:
Build output: http://queues.webkit.org/results/7610003

[Early Warning System Bot]

Attachment 78772 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7566007

[Build Bot]

Attachment 78772 [details] did not build on win:
Build output: http://queues.webkit.org/results/7546005

[Tony Chang]

Comment on attachment 78772 [details]
Patch3

View in context: https://bugs.webkit.org/attachment.cgi?id=78772&action=review

> Source/WebCore/page/DragController.h:55
> +    enum DragControllerAction {
> +        DragControllerActionEntered = 0,
> +        DragControllerActionUpdated = 1,
> +        DragControllerActionExited = 2,
> +        DragControllerActionPerformDrag = 3
> +    };

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

[Darin Adler]

Comment on attachment 78772 [details]
Patch3

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.

[Enrica Casucci]

I've addressed all the comments except for the following.

> > 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.

I don't understand the difference between my flags and PaintLayerFlag (other than the plural).
Could you explain, please?
 
 
> Also, the code to create a DragData should be refactored into a helper function or helper method rather than being repeated three times.

I'll do that when I finish the drag and drop support, this code is still subject to change.

> > 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.
>
I don't want to have new code in WebCore that depends on having access to the application or the view.

[Enrica Casucci]

Created attachment 78844 [details]
Patch4

Addresses comments and build breakage.

[Build Bot]

Attachment 78844 [details] did not build on win:
Build output: http://queues.webkit.org/results/7519018

[Enrica Casucci]

Created attachment 78848 [details]
Patch5

One more attempt to fix build break.

[Darin Adler]

Comment on attachment 78848 [details]
Patch5

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->dragController()->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

[WebKit Review Bot]

http://trac.webkit.org/changeset/75743 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
editing/input/page-up-down-scrolls.html

[Enrica Casucci]

Created attachment 79388 [details]
Patch6

Final drag and drop implementation on the Mac.

[Enrica Casucci]

Created attachment 79432 [details]
Patch7

Re-posted patch from the root.

[Early Warning System Bot]

Attachment 79432 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7551210

[Enrica Casucci]

Created attachment 79437 [details]
Patch8

Fixes qt build.

[Build Bot]

Attachment 79432 [details] did not build on win:
Build output: http://queues.webkit.org/results/7501199

[Early Warning System Bot]

Attachment 79437 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7505186

[Build Bot]

Attachment 79437 [details] did not build on win:
Build output: http://queues.webkit.org/results/7534224

[Enrica Casucci]

Created attachment 79454 [details]
Patch9

I found a problem with the previous patch and fixed it.
Also fixed other platforms (hopefully)

[Darin Adler]

Comment on attachment 79454 [details]
Patch9

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.

[Enrica Casucci]

Thanks for all the feedback, I will address all the comments and post a new patch.
I was unsure about the need of generating an additional mouseUp.
I will try without it, and if it works correctly, I'll remove all the unnecessary code.

[Enrica Casucci]

> 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.

I plan on removing these dependencies in a separate patch.

[Enrica Casucci]

Created attachment 79521 [details]
Patch10

New patch that addresses memory leaks and all the other comments.

[Darin Adler]

Comment on attachment 79521 [details]
Patch10

View in context: https://bugs.webkit.org/attachment.cgi?id=79521&action=review

> Source/WebKit2/Shared/mac/PasteboardTypes.mm:43
> +    CFRetain(types);

This will CFRetain every time the function is called, not just the first time. So it will work, but its performance will be a little slow and there is some small risk of overflow.

You could do something simple like this:

    static inline NSArray *retain(NSArray *array)
    {
        CFRetain(array);
        return array;
    }

    ...

    static NSArray *types = retain([NSArray ...]);

That would work better.

> Source/WebKit2/UIProcess/API/mac/PageClientImpl.mm:285
> +    RetainPtr<NSImage> dragNSImage = [[[NSImage alloc] initWithCGImage:CGBitmapContextCreateImage(graphicsContext->platformContext()) size:imageSize] autorelease];

Instead of doing autorelease you should do an adopt here.

    RetainPtr<NSImage> dragNSImage(AdoptNS, [[NSImage alloc] initWithCGImage:CGBitmapContextCreateImage(graphicsContext->platformContext()) size:imageSize]);

The code is already correct, with no leak, but the adopt version is more efficient.

[Enrica Casucci]

Made both changes.

[Enrica Casucci]

http://trac.webkit.org/changeset/76186. The Mac implementation is now complete.
https://bugs.webkit.org/show_bug.cgi?id=52775 now tracks the Windows work.

[WebKit Review Bot]

http://trac.webkit.org/changeset/76186 might have broken SnowLeopard Intel Release (Tests)
The following tests are not passing:
fast/forms/input-text-scroll-left-on-blur.html

[Enrica Casucci]

(In reply to comment #37)
> http://trac.webkit.org/changeset/76186 might have broken SnowLeopard Intel Release (Tests)
> The following tests are not passing:
> fast/forms/input-text-scroll-left-on-blur.html

I don't see this failing on my machine.