[webkit-reviews] review granted: [Bug 183314] [Mac] Teach WebCore::Pasteboard about file promise drags : [Attachment 334967] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andy Estes <aestes at apple.com>'s
request for review:
Bug 183314: [Mac] Teach WebCore::Pasteboard about file promise drags
https://bugs.webkit.org/show_bug.cgi?id=183314

Attachment 334967: Patch

https://bugs.webkit.org/attachment.cgi?id=334967&action=review




--- Comment #8 from Darin Adler <darin at apple.com> ---
Comment on attachment 334967
  --> https://bugs.webkit.org/attachment.cgi?id=334967
Patch

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

Great work!

Those FIXMEs are vexing; it would be great to add more test cases demonstrating
the FIXME code is doing the wrong thing.

> Source/WebCore/platform/FileSystem.h:159
> -bool hardLinkOrCopyFile(const String& source, const String& destination);
> +WEBCORE_EXPORT bool hardLinkOrCopyFile(const String& source, const String&
destination);

Stray thought on seeing this: I wonder if the clients of this function would
ever want to clone files on APFS rather than hard linking or copying.

> Source/WebCore/platform/mac/PasteboardMac.mm:279
> +static String toString(const Vector<String>& pathnames)

I don’t think this is a specific enough function name. Maybe instead of
thinking of it as a Pasteboard function we should think of it as a generally
useful string vector function and call it joinSeparatedByNewlines. Otherwise I
think we should call it joinPathnames or something like that.

We can make this function run faster by computing the sum of all the string
lengths and then:

    builder.reserveCapacity(totalLength + size - 1);

We need to write the code with overflow-checking arithmetic, though. No need to
do that version now, but if we want a more optimal join function we can either
do that, or maybe a version that uses StringImpl::createUninitialized.

> Source/WebCore/platform/mac/PasteboardMac.mm:287
> +    StringBuilder builder;
> +    for (size_t i = 0; i < pathnames.size(); ++i) {
> +	   if (i)
> +	       builder.append('\n');
> +	   builder.append(pathnames[i]);
> +    }
> +    return builder.toString();

Could use modern for loop and:

    if (!builder.isEmpty())
	builder.append('\n');

> Tools/DumpRenderTree/mac/DumpRenderTreeDraggingInfo.mm:49
> + at interface DumpRenderTreeFilePromiseReceiver : NSFilePromiseReceiver {

A little sad seeing all this new stuff for DumpRenderTree. Seems a little bit
backward-looking. What’s the equivalent going to be for WebKitTestRunner?

> Tools/DumpRenderTree/mac/DumpRenderTreeDraggingInfo.mm:111
> +    while (!FileSystem::hardLinkOrCopyFile(sourceURL.path,
destinationURL.path)) {

Not sure we really need to use FileSystem here. This is Cocoa-specific so we
could use NSFileManager methods.