[webkit-reviews] review granted: [Bug 52093] Paste and drag and drop use different code paths to interact with the pasteboard : [Attachment 78540] Patch6

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Enrica Casucci
<enrica at apple.com>'s request for review:
Bug 52093: Paste and drag and drop use different code paths to interact with
the pasteboard
https://bugs.webkit.org/show_bug.cgi?id=52093

Attachment 78540: Patch6
https://bugs.webkit.org/attachment.cgi?id=78540&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=78540&action=review

This seems like moving in the right direction - hopefully, passing the frame is
temporary.

> ChangeLog:17
> +	   * loader/EmptyClients.h: Added two Mac only methods to call into
WebKit to use functionality
> +	   that is in NSURLExtras.
> +	   (WebCore::EmptyEditorClient::canonicalizeURL):
> +	   (WebCore::EmptyEditorClient::canonicalizeURLString):

This doesn't sound like the kind of functionality we should call out to WebKit
for. Would there be any known breakage if KURL canonicalization was used?

Even if we need NSURL canonicalization, it would probably be better and easier
to move the implementation from WebNSURLExtras.mm to KURLMac.mm instead of
adding temporary code to EditorClient.

> ChangeLog:88
> +2011-01-10  Enrica Casucci  <enrica at apple.com>

ChangeLogs are quite messed up (looks like there are several copies in root
ChangeLog, and then in Sources/WebCore/ChangeLog?)

> Source/WebCore/page/qt/DragControllerQt.cpp:54
> -    if (dragData->containsURL())
> +    if (dragData->containsURL(0))

Is this going away soon? These zeroes are infuriating.

> Source/WebCore/platform/DragData.h:77
> +class DragData {
>      public:
>	   enum FilenameConversionPolicy { DoNotConvertFilenames,
ConvertFilenames };

Style is wrong here - I think that you should just keep the whole class
indented, rather than indent its content by 8 spaces.

> Source/WebCore/platform/DragData.h:104
> -} //namespace WebCore
> +} // namespace WebCore

I generally suggest omitting these comments. It's more reliable to double-click
on a closing brace to really check what it corresponds to - and generally, one
just ignores this boilerplate code, so comments are visual noise.

> Source/WebCore/platform/Pasteboard.h:127
> +    NSURL *getBestURL(Frame *);

Style: should be "Frame*".

> Source/WebCore/platform/mac/DragDataMac.mm:122
> +bool DragData::containsURL(Frame *frame, FilenameConversionPolicy
filenamePolicy) const

Frame*

> Source/WebCore/platform/mac/PasteboardMac.mm:440
> +NSURL *Pasteboard::getBestURL(Frame *frame)

Frame*

> Source/WebCore/platform/win/DragDataWin.cpp:41
> +bool DragData::containsURL(Frame *, FilenameConversionPolicy filenamePolicy)
const

Frame*

> Source/WebCore/platform/win/DragDataWin.cpp:50
> +String DragData::asURL(Frame *, FilenameConversionPolicy filenamePolicy,
String* title) const

Frame*

> Source/WebCore/platform/win/DragDataWin.cpp:93
> +String DragData::asPlainText(Frame *) const

Frame*