[webkit-reviews] review granted: [Bug 131770] Dropzone effects don't work in non-file documents : [Attachment 230110] proposed fix

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 131770: Dropzone effects don't work in non-file documents
https://bugs.webkit.org/show_bug.cgi?id=131770

Attachment 230110: proposed fix
https://bugs.webkit.org/attachment.cgi?id=230110&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.