Bug 28891

Created attachment 38890 [details] Proposed Patch

Comment on attachment 38890 [details] Proposed Patch Bah! Copy paste code is bad, even in LayoutTests. :(

(In reply to comment #2) > (From update of attachment 38890 [details]) > Bah! Copy paste code is bad, even in LayoutTests. :( Not sure I understand your comment. I did copy & paste some code in layout test script, but I looked at it carefully. Which part of the code do you refer to? Thanks.

Sorry, my brief comment wasn't very helpful. :) if (filesToDrag.length > 0) 16 eventShouldContainTransferType(event, "Files"); 17 else 18 eventShouldNotContainTransferType(event, "Files"); looks repeated a bunch. Passing NSPasteboard to 107 static void addHTMLClipboardTypesForCocoaType(HashSet<String>& resultTypes, NSString *cocoaType, NSPasteboard* pasteboard) is a little ugly, but OK I guess. There is some danger that the changeCount could have changed since we grabbed the types array, so that the pasteboard could now have files when it didn't before, or not have them when it did before. I don't think that's gonna be an issue in this case though.

Created attachment 38932 [details] Proposed Patch Changed the script to avoid duplicate code. Also added the comment to ClipboardMac.mm.

Comment on attachment 38932 [details] Proposed Patch Just a few things to consider and reply to. Meta questions: * Where is a spec reference for this change? (What does Firefox/IE do?) * Does the windows code/other platforms need an equivalent change? > diff --git a/LayoutTests/http/tests/security/clipboard/resources/clipboard-file-access.js b/LayoutTests/http/tests/security/clipboard/resources/clipboard-file-access.js > +function checkEventTransferType(event, typeString, shouldContain) I'm left asking shouldContain what? How about "shouldContainType"? Also, regarding the function namehHow about checkForEventTransferType? > { > + if (event.dataTransfer.types && event.dataTransfer.types.indexOf(typeString) != -1) { Why did the check for "event.dataTransfer.types" get added? > + if (shouldContain) > + testPassed("event.dataTransfer.types contains " + typeString + "."); > + else > + testFailed("event.dataTransfer.types should not contain " + typeString + "."); > + } else { > + if (shouldContain) > + testFailed("event.dataTransfer.types " + typeString + " expected."); > + else > + testPassed("event.dataTransfer.types does not contain " + typeString + "."); > + } How about this instead? if (event.dataTransfer.types.indexOf(typeString) != -1) { passedCheck = shouldContain; message = "event.dataTransfer.types contains " + typeString + "."; } else { passedCheck = !shouldContain; message = "event.dataTransfer.types does not contain " + typeString + "."; } if (passedCheck) testPassed(message); else testFailed(message); > diff --git a/WebCore/platform/mac/ClipboardMac.mm b/WebCore/platform/mac/ClipboardMac.mm > +static void addHTMLClipboardTypesForCocoaType(HashSet<String>& resultTypes, NSString *cocoaType, NSPasteboard* pasteboard) "NSPasteboard*" Since this is Objective-C the rule for * placement changes (just to keep you sharp :) ). > + // If file list is empty, add nothing. If *the* file list is empty ... > + // Note that there is a chance that the file list count could have changed since we grabbed the types array. > + // However, this is not really an issue for us doing a sanity check here. > + NSArray *fileList = [pasteboard propertyListForType:NSFilenamesPboardType]; > + if ([fileList count] != 0) { Avoid comparisons to 0.

Created attachment 39079 [details] Proposed Patch Fixed all the problems. As to the meta-questions, I've update the ChangeLog to indicate why we make the behavior change. Also, we do not need to update the implementation for other platforms since clipboard files are not supported there yet.

Comment on attachment 39079 [details] Proposed Patch Exception: Unknown committer: jianli@chromium.org If you're a committer, please make sure you've added yourself to: WebKitTools/Scripts/modules/committers.py If not, please don't set commit-queue+ :) Marking cq- because noel seems to have raised an objection via email, waiting for him to respond.

> Marking cq- because noel seems to have raised an objection via email, waiting > for him to respond. myu mistake, no objections from me.

Comment on attachment 39079 [details] Proposed Patch Rejecting patch 39079 from commit-queue. This patch will require manual commit. ['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1

Comment on attachment 39079 [details] Proposed Patch compositing/self-painting-layers.html -> crashed Caused by the commit-queue's arch nemesis, bug 28845.

Actually, the trace looks different. I filed bug 29035.

Committed as http://trac.webkit.org/changeset/48169.