[webkit-reviews] review denied: [Bug 28891] dataTransfer.types() should not return Files if file list is empty in the clipboard. : [Attachment 38932] Proposed Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Jian Li <jianli at chromium.org>'s
request for review:
Bug 28891: dataTransfer.types() should not return Files if file list is empty
in the clipboard.
https://bugs.webkit.org/show_bug.cgi?id=28891

Attachment 38932: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=38932&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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.