[webkit-reviews] review denied: [Bug 78648] FileReader is dysfunctional in documents with "null" origin string : [Attachment 146668] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Jian Li <jianli at chromium.org>'s
request for review:
Bug 78648: FileReader is dysfunctional in documents with "null" origin string
https://bugs.webkit.org/show_bug.cgi?id=78648

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=146668&action=review


This patch looks like the wrong approach.  Rather than trying to hack together
a blob URL that works for file URLs, it seems like a better approach is to
teach FileReader to work with Blobs directly without needing to use URLs at
all.

> Source/WebCore/fileapi/BlobURL.cpp:52
> -    return createBlobURL(securityOrigin->toString());
> +    String originString;
> +    if (securityOrigin->protocol() == "file")
> +	   originString = "file://" + securityOrigin->filePath();
> +    else
> +	   originString = securityOrigin->toString();
> +    return createBlobURL(originString);

This isn't right.  We shouldn't be special-casing protocol() == "file".  It
also won't work right depending on various bools in Settings.

> Source/WebCore/fileapi/BlobURL.cpp:67
> +KURL BlobURL::extractInnerURL(const KURL& url)
> +{
> +    String unescapedUrl = decodeURLEscapeSequences(url.path());
> +    size_t lastSlashPos = unescapedUrl.reverseFind('/');
> +    if (lastSlashPos != notFound)
> +	   unescapedUrl = unescapedUrl.substring(0, lastSlashPos);
> +    return KURL(ParsedURLString, unescapedUrl);
> +}

This is wrong.	KURL has a function for doing this correctly these days.

> Source/WebCore/page/SecurityOrigin.cpp:92
> +#if ENABLE(BLOB)
> +    if (url.protocolIs("blob"))
> +	   return BlobURL::extractInnerURL(url);
> +#endif

Why isn't KURL able to extract this inner URL properly?

> Source/WebCore/page/SecurityOrigin.h:68
> +    String filePath() const { return m_filePath; }

Please don't add an accessor for this information.  It's private data for
SecurityOrigin that no code outside this class should need.