WebKit Bugzilla
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.
Created attachment 66930 [details] decouple
Comment on attachment 66930 [details] decouple 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.
(In reply to comment #2) > (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. I buy that... more robust
Created attachment 66966 [details] decouple
Comment on attachment 66966 [details] decouple 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 on attachment 66966 [details] decouple 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 on attachment 66966 [details] decouple 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)
(In reply to comment #7) > (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) ENABLE(BLOB) is also turned on for WebKit Mac. If you have a mac machine, it is easy to build and test it.
> 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 :)
Created attachment 66983 [details] decouple minor mods mentioned above
> minor mods mentioned above doh... forgot one of the minor mods
Created attachment 66985 [details] decouple including the minor mod to only deserialize Files if (m_isDOMGlobalObject) for jsc
Comment on attachment 66985 [details] decouple 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
Created attachment 67079 [details] decouple > tempory => temporary Fixed. Also renamed the local var in that method from readableURL to urlForReading.
Comment on attachment 67079 [details] decouple rs=me. previously r+'d by jianli.
Comment on attachment 67079 [details] decouple Clearing flags on attachment: 67079 Committed r67208: <http://trac.webkit.org/changeset/67208>
All reviewed patches have been landed. Closing bug.