Bug 45410 - Decouple Blobs from ScriptExecutionContext.
: Decouple Blobs from ScriptExecutionContext.
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-09-08 12:42 PST by
Modified: 2010-09-10 09:20 PST (History)


Attachments
decouple (37.54 KB, patch)
2010-09-08 12:47 PST, Michael Nordman
jianli: review-
Review Patch | Details | Formatted Diff | Diff
decouple (38.97 KB, patch)
2010-09-08 16:58 PST, Michael Nordman
jianli: review+
Review Patch | Details | Formatted Diff | Diff
decouple (39.07 KB, patch)
2010-09-08 18:27 PST, Michael Nordman
no flags Review Patch | Details | Formatted Diff | Diff
decouple (39.11 KB, patch)
2010-09-08 18:32 PST, Michael Nordman
jianli: review+
Review Patch | Details | Formatted Diff | Diff
decouple (39.12 KB, patch)
2010-09-09 12:26 PST, Michael Nordman
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-09-08 12:42:02 PST
There's no need to have this coupling. It makes it awkward to create instances, invites "layering violations", and present problems with passing Blob references from context to context.
------- Comment #1 From 2010-09-08 12:47:21 PST -------
Created an attachment (id=66930) [details]
decouple
------- Comment #2 From 2010-09-08 13:35:10 PST -------
(From update of attachment 66930 [details])
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.
------- Comment #3 From 2010-09-08 14:32:06 PST -------
(In reply to comment #2)
> (From update of attachment 66930 [details] [details])
> 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.

I buy that... more robust
------- Comment #4 From 2010-09-08 16:58:07 PST -------
Created an attachment (id=66966) [details]
decouple
------- Comment #5 From 2010-09-08 17:31:14 PST -------
(From update of attachment 66966 [details])
Looks good except some minor issues.

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

> WebCore/bindings/js/SerializedScriptValue.cpp:977
> +        file = File::create(String(path.ustring().impl()), KURL(KURL(), String(url.ustring().impl())), String(type.ustring().impl()));
Do we need to keep m_isDOMGlobalObject check here? The blob reading part has this logic kept.
    if (!m_isDOMGlobalObject)
        return jsNull();

> WebCore/fileapi/FileReader.cpp:164
> +void FileReader::clearLoaderAndReadableURL()
Probably it is better to call it cleanup since we might put other cleanup logic in this function in the future.

> WebCore/fileapi/FileReader.h:145
> +    KURL m_readableURL;
m_readableURL sounds like to mean the URL is legible. Might be better to name it something like m_urlForReading. If you make this change, do not forget to update ChangeLog.

> WebCore/fileapi/FileReaderSync.cpp:154
> +    // The blob is read by routing through the request handling layer given the tempory public url.
tempory => temporary
------- Comment #6 From 2010-09-08 17:44:22 PST -------
(From update of attachment 66966 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=66966&action=prettypatch

> WebCore/bindings/js/SerializedScriptValue.cpp:977
> +        file = File::create(String(path.ustring().impl()), KURL(KURL(), String(url.ustring().impl())), String(type.ustring().impl()));
I couldn't tell you if this check is needed or not anymore in this code path. There was no way to construct an File w/o it previously, now there is. The 'blob' reading calls toJS, which takes the m_globlObject as a param which explains why the test is needed in that code path.

I'll put the test base in to keep the deltas smaller.

> WebCore/fileapi/FileReader.cpp:164
> +void FileReader::clearLoaderAndReadableURL()
rename sounds good

> WebCore/fileapi/FileReader.h:145
> +    KURL m_readableURL;
rename sounds good here too

> WebCore/fileapi/FileReaderSync.cpp:154
> +    // The blob is read by routing through the request handling layer given the tempory public url.
spelling error, thnx
------- Comment #7 From 2010-09-08 17:56:19 PST -------
(From update of attachment 66966 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=66966&action=prettypatch

> WebCore/fileapi/FileReader.cpp:177
> +    ThreadableBlobRegistry::registerBlobURL(readableURL, m_blob->url());
ooops 'readableURL'... that's a compile error

(of course, this isn't being compiled in regular webkit builds so i don't see that when poking at this in a std webkit client, i really despise moving files/patches around)
------- Comment #8 From 2010-09-08 18:06:53 PST -------
(In reply to comment #7)
> (From update of attachment 66966 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=66966&action=prettypatch
> 
> > WebCore/fileapi/FileReader.cpp:177
> > +    ThreadableBlobRegistry::registerBlobURL(readableURL, m_blob->url());
> ooops 'readableURL'... that's a compile error
> 
> (of course, this isn't being compiled in regular webkit builds so i don't see that when poking at this in a std webkit client, i really despise moving files/patches around)

ENABLE(BLOB) is also turned on for WebKit Mac. If you have a mac machine, it is easy to build and test it.
------- Comment #9 From 2010-09-08 18:22:05 PST -------
> ENABLE(BLOB) is also turned on for WebKit Mac. If you have a mac machine, it is easy to build and test it.

Of course, i'll just move the patch there in order to see compile errors... did i mention how much i despise moving patches around :)
------- Comment #10 From 2010-09-08 18:27:21 PST -------
Created an attachment (id=66983) [details]
decouple

minor mods mentioned above
------- Comment #11 From 2010-09-08 18:28:39 PST -------
> minor mods mentioned above

doh... forgot one of the minor mods
------- Comment #12 From 2010-09-08 18:32:26 PST -------
Created an attachment (id=66985) [details]
decouple

including the minor mod to only deserialize Files if (m_isDOMGlobalObject) for jsc
------- Comment #13 From 2010-09-08 19:04:01 PST -------
(From update of attachment 66985 [details])
r=me

Please fix the minor typo issue before you land.

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

> WebCore/fileapi/FileReaderSync.cpp:154
> +    // The blob is read by routing through the request handling layer given the tempory public url.
tempory => temporary
------- Comment #14 From 2010-09-09 12:26:14 PST -------
Created an attachment (id=67079) [details]
decouple

> tempory => temporary

Fixed. Also renamed the local var in that method from readableURL to urlForReading.
------- Comment #15 From 2010-09-09 12:31:16 PST -------
(From update of attachment 67079 [details])
rs=me. previously r+'d by jianli.
------- Comment #16 From 2010-09-10 09:20:23 PST -------
(From update of attachment 67079 [details])
Clearing flags on attachment: 67079

Committed r67208: <http://trac.webkit.org/changeset/67208>
------- Comment #17 From 2010-09-10 09:20:28 PST -------
All reviewed patches have been landed.  Closing bug.