Bug 28891 – dataTransfer.types() should not return Files if file list is empty in the clipboard.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Jian Li]

dataTransfer.types() should not return Files if file list is empty in the clipboard.

[Jian Li]

Created attachment 38890 [details]
Proposed Patch

[Eric Seidel (no email)]

Comment on attachment 38890 [details]
Proposed Patch

Bah!  Copy paste code is bad, even in LayoutTests. :(

[Jian Li]

(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.

[Eric Seidel (no email)]

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.

[Jian Li]

Created attachment 38932 [details]
Proposed Patch

Changed the script to avoid duplicate code. Also added the comment to ClipboardMac.mm.

[David Levin]

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.

[Jian Li]

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.

[Eric Seidel (no email)]

Comment on attachment 39079 [details]
Proposed Patch

Exception: Unknown committer: [e-mail]
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.

[noel gordon]

> Marking cq- because noel seems to have raised an objection via email, waiting
> for him to respond.

myu mistake, no objections from me.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Comment on attachment 39079 [details]
Proposed Patch

compositing/self-painting-layers.html -> crashed

Caused by the commit-queue's arch nemesis, bug 28845.

[Eric Seidel (no email)]

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

[Jian Li]

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