r210683 introduced logic to prevent file URLs from being copied to the clipboard in unexpected cases. In order to achieve this functionality, checkURLReceivedFromWebProcess was called on all items in the pathnames array passed into WebPasteboardProxy::setPasteboardPathnamesForType. However, this method is a misnomer, as the pathnames array always contains exactly one URL and one title for the URL. We should not call checkURLReceivedFromWebProcess on the title and we should refactor the associated methods to avoid referring to multiple pathnames.
<rdar://problem/41663196>
Created attachment 343988 [details] Patch
Comment on attachment 343988 [details] Patch Attachment 343988 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8391722 New failing tests: http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html http/tests/preload/onload_event.html
Created attachment 343994 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 343996 [details] Patch
Comment on attachment 343996 [details] Patch Attachment 343996 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/8392716 New failing tests: editing/mac/pasteboard/can-copy-url-without-title.html
Created attachment 344001 [details] Archive of layout-test-results from ews101 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
The added LayoutTest should be mac-wk2 only.
Comment on attachment 343996 [details] Patch Attachment 343996 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8392848 New failing tests: editing/mac/pasteboard/can-copy-url-without-title.html
Created attachment 344003 [details] Archive of layout-test-results from ews113 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 343996 [details] Patch Attachment 343996 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8394955 New failing tests: http/tests/preload/onload_event.html
Created attachment 344012 [details] Archive of layout-test-results from ews204 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 344043 [details] Patch
Comment on attachment 344043 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344043&action=review > Source/WebCore/platform/mac/PlatformPasteboardMac.mm:311 > + RetainPtr<NSMutableArray> paths = adoptNS([[NSMutableArray alloc] init]); > + NSURL *cocoaURL = pasteboardURL.url; > + [paths.get() addObject:[NSArray arrayWithObject:[cocoaURL absoluteString]]]; > + [paths.get() addObject:[NSArray arrayWithObject:pasteboardURL.title]]; Here’s a way to write this with array literals: NSURL *cocoaURL = pasteboardURL.url; NSArray *paths = @[ @[ cocoaURL.absoluteString ], @[ pasteboardURL.title ] ]; This has advantages like not creating a mutable array, sizing the array better, and being two lines of code instead of four, but a tiny disadvantage of doing one more autorelease. > Source/WebCore/platform/mac/PlatformPasteboardMac.mm:313 > + NSString *pasteboardType = [NSString stringWithFormat:@"%s", WebURLsWithTitlesPboardType]; This should be one of these instead: NSString *pasteboardType = [NSString stringWithCString:WebURLsWithTitlesPboardType encoding:NSASCIIStringEncoding]; NSString *pasteboardType = [NSString stringWithUTF8String:WebURLsWithTitlesPboardType]; Using the formatting version instead is less efficient. I also think it’s a bit peculiar that we have constants named WebURLsWithTitlesPboardType in both WebCore and WebKit, but in WebCore it’s a C string whereas in WebKit it’s an NSString. Might be nice to get rid of that pattern some day. If this was already an NSString it would be better for this call site, but I presume it would be worse for other call sites.
Created attachment 344273 [details] Patch
Comment on attachment 344273 [details] Patch Attachment 344273 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8436553 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html
Created attachment 344286 [details] Archive of layout-test-results from ews205 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 344273 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344273&action=review > LayoutTests/TestExpectations:-201 > -webkit.org/b/186901 imported/w3c/web-platform-tests/css/css-display/display-contents-first-letter-002.html [ Skip ] Did you intend to skip this test?
Comment on attachment 344273 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344273&action=review >> LayoutTests/TestExpectations:-201 >> -webkit.org/b/186901 imported/w3c/web-platform-tests/css/css-display/display-contents-first-letter-002.html [ Skip ] > > Did you intend to skip this test? (Unskip, rather)
Comment on attachment 344273 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344273&action=review > Source/WebCore/platform/mac/PlatformPasteboardMac.mm:309 > + NSArray *paths = @[ @[ cocoaURL.absoluteString ], @[ pasteboardURL.title ] ]; Nit - I think calling this variable `paths` is a bit misleading. Perhaps something like `urlAndTitle` instead? > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1671 > + return false; Now that PasteboardURL can be sent over IPC, we probably want to ensure that all of its additional platform-specific members (`userVisibleForm` on macOS and `markup` in GTK) are also serialized over IPC.
Comment on attachment 344273 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344273&action=review > Source/WebCore/platform/PlatformPasteboard.h:88 > + WEBCORE_EXPORT long setPasteboardURL(const PasteboardURL&); I don’t think having the word "Pasteboard" in this function makes sense. The entire file is about pasteboard, so why have this one function mention Pasteboard >> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:309 >> + NSArray *paths = @[ @[ cocoaURL.absoluteString ], @[ pasteboardURL.title ] ]; > > Nit - I think calling this variable `paths` is a bit misleading. Perhaps something like `urlAndTitle` instead? Another way to dodge that strangeness is to call this "array".
(In reply to Darin Adler from comment #21) > Comment on attachment 344273 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=344273&action=review > > > Source/WebCore/platform/PlatformPasteboard.h:88 > > + WEBCORE_EXPORT long setPasteboardURL(const PasteboardURL&); > > I don’t think having the word "Pasteboard" in this function makes sense. The > entire file is about pasteboard, so why have this one function mention > Pasteboard I named it setPasteboardURL due to the parameter type. I feel that 'setURL' would not be correct as a URL is not the only information that is contained within a PasteboardURL - we also have the title.
Created attachment 344315 [details] Patch
Comment on attachment 344315 [details] Patch Attachment 344315 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/8443411 New failing tests: animations/needs-layout.html
Created attachment 344317 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Comment on attachment 344273 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344273&action=review >>> Source/WebCore/platform/PlatformPasteboard.h:88 >>> + WEBCORE_EXPORT long setPasteboardURL(const PasteboardURL&); >> >> I don’t think having the word "Pasteboard" in this function makes sense. The entire file is about pasteboard, so why have this one function mention Pasteboard > > I named it setPasteboardURL due to the parameter type. I feel that 'setURL' would not be correct as a URL is not the only information that is contained within a PasteboardURL - we also have the title. Sure, but due to overloading C++ functions don’t always have to mention their parameter types. I am not suggesting setURL as the name. In fact there are no other "set" functions in this class, only "set for type" functions. I am not even sure I understand the difference between this function and the write function that takes a PasteboardURL.
Comment on attachment 344315 [details] 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.
Comment on attachment 344315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344315&action=review >> 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. This encode/decode logic is currently only PLATFORM(IOS). The patch moves it out to cross-platform code.
Created attachment 344693 [details] Patch
Comment on attachment 344693 [details] Patch Attachment 344693 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8494964 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html
Created attachment 344703 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 344693 [details] Patch Rejecting attachment 344693 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 344693, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Logging in as commit-queue@webkit.org... Fetching: https://bugs.webkit.org/attachment.cgi?id=344693&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=187212&ctype=xml&excludefield=attachmentdata Processing 1 patch from 1 bug. Processing patch 344693 from bug 187212. Fetching: https://bugs.webkit.org/attachment.cgi?id=344693 Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Ryosuke Niwa']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Parsed 24 diffs from patch file(s). patching file Source/WebCore/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Source/WebKit/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Source/WebKitLegacy/mac/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Source/WebCore/platform/PasteboardStrategy.h patching file Source/WebCore/platform/PlatformPasteboard.h patching file Source/WebCore/platform/ios/PlatformPasteboardIOS.mm patching file Source/WebCore/platform/mac/PasteboardMac.mm patching file Source/WebCore/platform/mac/PlatformPasteboardMac.mm patching file Source/WebKit/Shared/WebCoreArgumentCoders.cpp patching file Source/WebKit/Shared/WebCoreArgumentCoders.h patching file Source/WebKit/UIProcess/Cocoa/WebPasteboardProxyCocoa.mm patching file Source/WebKit/UIProcess/WebPasteboardProxy.h patching file Source/WebKit/UIProcess/WebPasteboardProxy.messages.in patching file Source/WebKit/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp patching file Source/WebKit/WebProcess/WebCoreSupport/WebPlatformStrategies.h patching file Source/WebKitLegacy/mac/WebCoreSupport/WebPlatformStrategies.h patching file Source/WebKitLegacy/mac/WebCoreSupport/WebPlatformStrategies.mm patching file Tools/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Tools/TestWebKitAPI/Tests/mac/ContextMenuCanCopyURL.html patching file Tools/TestWebKitAPI/Tests/mac/ContextMenuCanCopyURL.mm patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/editing/mac/pasteboard/can-copy-url-without-title-expected.txt patching file LayoutTests/editing/mac/pasteboard/can-copy-url-without-title.html patching file LayoutTests/platform/mac-wk1/TestExpectations Hunk #1 FAILED at 564. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/mac-wk1/TestExpectations.rej Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Ryosuke Niwa']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: https://webkit-queues.webkit.org/results/8507090
Committed r233753: <https://trac.webkit.org/changeset/233753>