Bug 131770

Summary: Dropzone effects don't work in non-file documents
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: WebCore Misc.Assignee: Alexey Proskuryakov <ap>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, darin, esprehn+autocc, kangil.han, rniwa, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 131767    
Attachments:
Description Flags
proposed fix darin: review+

Description Alexey Proskuryakov 2014-04-16 15:44:46 PDT
We have dropzone implemented (with prefix), and fast/events/dropzone-004.html verifies that setting effects works. But the test passes almost by accident - the code only works in local files, because it uses DataTransfer.files, and only local documents can read that while dragging.
Comment 1 Alexey Proskuryakov 2014-04-24 15:09:37 PDT
Created attachment 230110 [details]
proposed fix
Comment 2 Darin Adler 2014-04-24 16:51:13 PDT
Comment on attachment 230110 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=230110&action=review

r=me as is, although I have a few suggestions for future consideration

> Source/WebCore/ChangeLog:10
> +        2. On Mac, sandbox doesn't prevent File object creation, as we alraedy have the access.

typo: alraedy

> Source/WebCore/dom/DataTransfer.cpp:187
> +bool DataTransfer::hasFileOfType(const String& type)

Sure would be nice if this took a StringView instead of a const String&.

> Source/WebCore/dom/DataTransfer.cpp:189
> +    ASSERT_WITH_SECURITY_IMPLICATION(canReadTypes());

Why not just return false in this case instead of asserting? Are callers really in a good position to alway preflight correctly?

> Source/WebCore/dom/DataTransfer.cpp:199
> +bool DataTransfer::hasStringOfType(const String& type)

Sure would be nice if this took a StringView instead of a const String&.

> Source/WebCore/dom/DataTransfer.cpp:201
> +    ASSERT_WITH_SECURITY_IMPLICATION(canReadTypes());

Same question.

> Source/WebCore/page/EventHandler.cpp:2068
> +        return dataTransfer.hasFileOfType(keyword.substring(5));

If hasFileOfType took a StringView, then this would not need to allocate memory. Could just do StringView(keyword).substring(5) or have this function itself take a StringView.

> Source/WebCore/page/EventHandler.cpp:2071
> +        return dataTransfer.hasStringOfType(keyword.substring(7));

Ditto.
Comment 3 Alexey Proskuryakov 2014-04-24 17:37:18 PDT
>> Source/WebCore/dom/DataTransfer.cpp:189
>> +    ASSERT_WITH_SECURITY_IMPLICATION(canReadTypes());
>
>Why not just return false in this case instead of asserting? Are callers really in a good position to alway preflight correctly?

Yes, I think that they should be in good position. canReadTypes() is not quite the right check anyway, because it's literally "can get dataTransfer.types from JavaScript". It doesn't matter for dropzone, because it only calls these functions when dataTransfer.types can be read. What I wanted to prevent here was potential future callers exposing data to the web without consideration - but if the assertion proves too restrictive, it can be improved when the need arises.

There is quite a bit of fragility in how this class is used to expose data to JS code, and is also used in native code (the confusion that caused this bug in the first place).
Comment 4 Alexey Proskuryakov 2014-04-24 17:49:15 PDT
It would be nice to teach equalIgnoringCase about StringViews I guess - without that, switching to StringView was somewhat too annoying here. 

Committed <http://trac.webkit.org/r167784>.
Comment 5 Darin Adler 2014-04-25 00:52:24 PDT
(In reply to comment #4)
> It would be nice to teach equalIgnoringCase about StringViews

OK. I’ll do that when I get a chance.