[webkit-reviews] review denied: [Bug 45410] Decouple Blobs from ScriptExecutionContext. : [Attachment 66930] decouple

[bugzilla-daemon at webkit.org]

Jian Li <jianli at chromium.org> has denied Michael Nordman
<michaeln at google.com>'s request for review:
Bug 45410: Decouple Blobs from ScriptExecutionContext.
https://bugs.webkit.org/show_bug.cgi?id=45410

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

------- Additional Comments from Jian Li <jianli at chromium.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=66930&action=prettypatch

> WebCore/ChangeLog:6
> +	   Decouple Blob from ScriptExecutionContext.
Normally we put the description line before the bug link line.

> WebCore/ChangeLog:8
> +	   * Removes ScriptExecutionContext's collection of Blobs instance in
that context.
Removes => Removed

> WebCore/ChangeLog:14
> +	   No new tests. (OOPS!)
Please remove this line.

> WebCore/bindings/js/SerializedScriptValue.cpp:957
> +	   file = File::create(String(path.ustring().impl()), KURL(KURL(),
String(url.ustring().impl())), String(type.ustring().impl()));
This file is updated. Please merge and resolve.

> WebCore/fileapi/FileReader.cpp:184
> +    ThreadableBlobRegistry::unregisterBlobURL(readableURL);
I think we should unregister this temporary URL after we finish the reading or
encounter an error. This is because otherwise we have to assume that the
un-registration always happens after the loading. This might be risky depending
on how ThreadableLoader is implemented. For example, suppose we invoke
FileReader in workers. If ThreadableLoader implementation needs to call into
main thread once and then call other method asynchronously, we will have a
problem.