Bug 28891 - dataTransfer.types() should not return Files if file list is empty in the clipboard.
Summary: dataTransfer.types() should not return Files if file list is empty in the cli...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Jian Li
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-01 15:15 PDT by Jian Li
Modified: 2009-09-08 11:03 PDT (History)
2 users (show)

See Also:


Attachments
Proposed Patch (8.17 KB, patch)
2009-09-01 15:23 PDT, Jian Li
eric: review-
jianli: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (8.22 KB, patch)
2009-09-02 11:05 PDT, Jian Li
levin: review-
jianli: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (8.30 KB, patch)
2009-09-04 11:59 PDT, Jian Li
levin: review+
jianli: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jian Li 2009-09-01 15:15:41 PDT
dataTransfer.types() should not return Files if file list is empty in the clipboard.
Comment 1 Jian Li 2009-09-01 15:23:28 PDT
Created attachment 38890 [details]
Proposed Patch
Comment 2 Eric Seidel (no email) 2009-09-01 17:18:00 PDT
Comment on attachment 38890 [details]
Proposed Patch

Bah!  Copy paste code is bad, even in LayoutTests. :(
Comment 3 Jian Li 2009-09-01 17:30:37 PDT
(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.
Comment 4 Eric Seidel (no email) 2009-09-02 00:10:20 PDT
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.
Comment 5 Jian Li 2009-09-02 11:05:21 PDT
Created attachment 38932 [details]
Proposed Patch

Changed the script to avoid duplicate code. Also added the comment to ClipboardMac.mm.
Comment 6 David Levin 2009-09-04 11:04:06 PDT
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.
Comment 7 Jian Li 2009-09-04 11:59:29 PDT
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 8 Eric Seidel (no email) 2009-09-05 01:27:29 PDT
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.
Comment 9 noel gordon 2009-09-06 22:50:28 PDT
> Marking cq- because noel seems to have raised an objection via email, waiting
> for him to respond.

myu mistake, no objections from me.
Comment 10 Eric Seidel (no email) 2009-09-08 08:56:23 PDT
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 11 Eric Seidel (no email) 2009-09-08 09:00:42 PDT
Comment on attachment 39079 [details]
Proposed Patch

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

Caused by the commit-queue's arch nemesis, bug 28845.
Comment 12 Eric Seidel (no email) 2009-09-08 09:03:41 PDT
Actually, the trace looks different.  I filed bug 29035.
Comment 13 Jian Li 2009-09-08 11:03:30 PDT
Committed as http://trac.webkit.org/changeset/48169.