[webkit-reviews] review granted: [Bug 21358] DragData should not depend on Clipboard, DocumentFragment, and Document : [Attachment 237217] Rebased patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Sep 14 12:16:06 PDT 2014
Darin Adler <darin at apple.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 21358: DragData should not depend on Clipboard, DocumentFragment, and
Document
https://bugs.webkit.org/show_bug.cgi?id=21358
Attachment 237217: Rebased patch
https://bugs.webkit.org/attachment.cgi?id=237217&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
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.
More information about the webkit-reviews
mailing list