Don't write file URLs to iOS Pasteboard
Created attachment 315862 [details] Patch
Comment on attachment 315862 [details] Patch Attachment 315862 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4144937 New failing tests: fast/images/image-copy-memory-usage.html
Created attachment 315872 [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.12.5
Comment on attachment 315862 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315862&action=review > Source/WebCore/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=174647 Please include a radar number here. > Source/WebCore/editing/ios/EditorIOS.mm:210 > + if (!url.isLocalFile()) { Our logic here sets pasteboardImage.url.url to the passed in url, but falls back to imageSourceURL if url is empty, so if url.isEmpty() and imageSourceURL is a file URL, we'll still end up writing a file URL here. We should account for this here. > Tools/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=174647 Ditto. > Tools/TestWebKitAPI/Tests/ios/DataInteractionTests.mm:-169 > - checkTypeIdentifierPrecedesOtherTypeIdentifier(dataInteractionSimulator.get(), (NSString *)kUTTypePNG, (NSString *)kUTTypeFileURL); We should still check that kUTTypePNG is in the item provider, and then additionally check that kUTTypeFileURL is not in the item provider. > Tools/TestWebKitAPI/Tests/ios/DataInteractionTests.mm:-189 > - checkTypeIdentifierPrecedesOtherTypeIdentifier(dataInteractionSimulator.get(), (NSString *)kUTTypePNG, (NSString *)kUTTypeFileURL); Ditto. > Tools/TestWebKitAPI/Tests/ios/DataInteractionTests.mm:-994 > - checkTypeIdentifierPrecedesOtherTypeIdentifier(dataInteractionSimulator.get(), (NSString *)kUTTypePNG, (NSString *)kUTTypeFileURL); Ditto.
Comment on attachment 315862 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315862&action=review > Source/WebCore/ChangeLog:8 > + Tests updated to reflect the changes. I would also give a very brief recap of the change in Editor here (maybe mention that this impacts both DnD and copy/paste).
Created attachment 315938 [details] Patch
Comment on attachment 315938 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315938&action=review LGTM, but I'm a bit concerned about the failing test (fast/images/image-copy-memory-usage.html). It does seem to exercise this Editor::writeImageToPasteboard codepath. Let's see if this test is actually catching anything useful, and if we just need to rebaseline it. > Tools/TestWebKitAPI/Tests/ios/DataInteractionTests.mm:122 > + EXPECT_TRUE([registeredTypes indexOfObject:firstType] < [registeredTypes indexOfObject:secondType]); We don't need this last check here, since [registeredTypes indexOfObject:secondType] should be NSNotFound anyways.
> LGTM, but I'm a bit concerned about the failing test > (fast/images/image-copy-memory-usage.html). It does seem to exercise this > Editor::writeImageToPasteboard codepath. Let's see if this test is actually > catching anything useful, and if we just need to rebaseline it. Oh, after actually looking at the LayoutTest failure logs, you'll probably need to update PlatformPasteboard::write() as well to not crash when pasteboardImage.url.url is empty.
Comment on attachment 315938 [details] Patch Attachment 315938 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4149149 New failing tests: fast/images/image-copy-memory-usage.html
Created attachment 315947 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Created attachment 315951 [details] Patch
Comment on attachment 315951 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315951&action=review > Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:328 > + if (!pasteboardImage.url.url.isNull()) This should probably be an isEmpty() check, which encompasses isNull() > Tools/TestWebKitAPI/Tests/ios/DataInteractionTests.mm:122 > + EXPECT_TRUE([registeredTypes indexOfObject:firstType] < [registeredTypes indexOfObject:secondType]); We don't need this last check here, since [registeredTypes indexOfObject:secondType] should be NSNotFound anyways.
Created attachment 315952 [details] Patch
> > Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:328 > > + if (!pasteboardImage.url.url.isNull()) > > This should probably be an isEmpty() check, which encompasses isNull() On second thought, it looks like isNull() is OK here (a URL created with an empty string is converted to a non-null NSURL, which will be OK to add as a value to an NSDictionary).
https://trac.webkit.org/changeset/219666/webkit