[Webkit-unassigned] [Bug 21358] DragData should not depend on Clipboard, DocumentFragment, and Document

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=21358


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #237217|review?                     |review+
               Flag|                            |




--- Comment #8 from Darin Adler <darin at apple.com>  2014-09-14 12:16:09 PST ---
(From update of attachment 237217)
View in context: https://bugs.webkit.org/attachment.cgi?id=237217&action=review

We should make sure we are adding the minimum includes here.

> Source/WebCore/page/DragController.h:119
> +        static PassRefPtr<DocumentFragment> documentFragmentFromDragData(DragData&, Frame&, Range&, bool allowPlainText, bool& chosePlainText);
> +        static PassRefPtr<DocumentFragment> createFragmentFromDragData(DragData&, Frame&, Range&, bool allowPlainText, bool& chosePlainText);

The names of these two functions don’t make it at all clear why there are two of them and how they differ.

I think the second one exists only to have a platform-dependent hook to do things differently on every platform. Long term, I want to refactor this code so that it’s shared and not platform-specific. I think it can be done. In the meantime, I suggest a name that is clearer.

I’m also not sure that the first function needs to be a class member. Maybe it can just be a function with internal linkages inside the .cpp file and not in the header at all.

> Source/WebCore/page/win/DragControllerWin.cpp:93
> +     * Order is richest format first. On OSX this is:

You just moved this code, but this is a Windows-specific file. Not sure why a list of OS X formats is relevant. Also, it’s “OS X”, not “OSX”.

I suggest we just drop the comment entirely.

> Source/WebCore/page/win/DragControllerWin.cpp:122
> +    if (DragDataRef platformDragData = dragData.platformData()) {
> +        if (containsFilenames(platformDragData)) {
> +            if (PassRefPtr<DocumentFragment> fragment = fragmentFromFilenames(frame.document(), platformDragData))
> +                return fragment;
> +        }
> +
> +        if (containsHTML(platformDragData)) {
> +            if (PassRefPtr<DocumentFragment> fragment = fragmentFromHTML(frame.document(), platformDragData))
> +                return fragment;
> +        }
> +    } else {
> +        if (containsFilenames(&dragData.dragDataMap())) {
> +            if (PassRefPtr<DocumentFragment> fragment = fragmentFromFilenames(frame.document(), &dragData.dragDataMap()))
> +                return fragment;
> +        }
> +
> +        if (containsHTML(&dragData.dragDataMap())) {
> +            if (PassRefPtr<DocumentFragment> fragment = fragmentFromHTML(frame.document(), &dragData.dragDataMap()))
> +                return fragment;
> +        }
> +    }

You just moved this code, but sure would be nice to factor this so we don’t have to repeat everything twice.

> Source/WebCore/platform/DragData.h:95
> +    bool containsURL(FilenameConversionPolicy filenamePolicy = ConvertFilenames) const;

The argument name filenamePolicy doesn’t add anything and should be removed.

> Source/WebCore/platform/DragData.h:98
> +    String asURL(FilenameConversionPolicy filenamePolicy = ConvertFilenames, String* title = 0) const;

The argument name filenamePolicy doesn’t add anything and should be removed. Should use nullptr rather than 0.

> Source/WebCore/platform/mac/DragDataMac.mm:111
> +    // FIXME: It's not clear this is 100% correct since we know -[NSURL URLWithString:] does not handle
> +    // all the same cases we handle well in the URL code for creating an NSURL.

Where was this code moved from? It’s easy for us to rewrite this use WebCore::URL to make the NSURL.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.