[webkit-reviews] review granted: [Bug 187212] [macOS] Don't treat URL titles as URLs when adding to the pasteboard : [Attachment 344315] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Aditya Keerthi
<akeerthi at apple.com>'s request for review:
Bug 187212: [macOS] Don't treat URL titles as URLs when adding to the
pasteboard
https://bugs.webkit.org/show_bug.cgi?id=187212

Attachment 344315: Patch

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




--- Comment #27 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 344315
  --> https://bugs.webkit.org/attachment.cgi?id=344315
Patch

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

r=me provided all my comments below are addressed.

> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:306
> +long PlatformPasteboard::setPasteboardURL(const PasteboardURL&
pasteboardURL)

Given there is no other function which writes URL, I agree with Darin that
don't need Pasteboard in this name.
I'd suggest just calling it setURL instead.

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1659
> +void ArgumentCoder<PasteboardURL>::encode(Encoder& encoder, const
PasteboardURL& content)

Why are we moving these functions? Please keep them at their original locations
so that the change we're making is clear.

> Source/WebKit/Shared/WebCoreArgumentCoders.h:-416
> -template<> struct ArgumentCoder<WebCore::PasteboardURL> {

What's the point of this re-ordering? We should avoid unnecessary code change
like this.

> Source/WebKit/UIProcess/Cocoa/WebPasteboardProxyCocoa.mm:-123
> -void WebPasteboardProxy::setPasteboardPathnamesForType(IPC::Connection&
connection, const String& pasteboardName, const String& pasteboardType, const
Vector<String>& pathnames, uint64_t& newChangeCount)

Again, please don't move all these functions.
Make the code change at the location they already exist so that we can see what
changed in the diff, not the entire function.

> LayoutTests/platform/mac/TestExpectations:1780
> +webkit.org/b/187230 editing/mac/pasteboard/can-copy-url-without-title.html [
Skip ]

Just add this to LayoutTests/platform/mac-wk1/TestExpectations instead.