WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
45410
Decouple Blobs from ScriptExecutionContext.
https://bugs.webkit.org/show_bug.cgi?id=45410
Summary
Decouple Blobs from ScriptExecutionContext.
Michael Nordman
Reported
2010-09-08 12:42:02 PDT
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.
Attachments
decouple
(37.54 KB, patch)
2010-09-08 12:47 PDT
,
Michael Nordman
jianli
: review-
Details
Formatted Diff
Diff
decouple
(38.97 KB, patch)
2010-09-08 16:58 PDT
,
Michael Nordman
jianli
: review+
Details
Formatted Diff
Diff
decouple
(39.07 KB, patch)
2010-09-08 18:27 PDT
,
Michael Nordman
no flags
Details
Formatted Diff
Diff
decouple
(39.11 KB, patch)
2010-09-08 18:32 PDT
,
Michael Nordman
jianli
: review+
Details
Formatted Diff
Diff
decouple
(39.12 KB, patch)
2010-09-09 12:26 PDT
,
Michael Nordman
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Michael Nordman
Comment 1
2010-09-08 12:47:21 PDT
Created
attachment 66930
[details]
decouple
Jian Li
Comment 2
2010-09-08 13:35:10 PDT
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.
Michael Nordman
Comment 3
2010-09-08 14:32:06 PDT
(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
Michael Nordman
Comment 4
2010-09-08 16:58:07 PDT
Created
attachment 66966
[details]
decouple
Jian Li
Comment 5
2010-09-08 17:31:14 PDT
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
Michael Nordman
Comment 6
2010-09-08 17:44:22 PDT
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
Michael Nordman
Comment 7
2010-09-08 17:56:19 PDT
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)
Jian Li
Comment 8
2010-09-08 18:06:53 PDT
(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.
Michael Nordman
Comment 9
2010-09-08 18:22:05 PDT
> 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 :)
Michael Nordman
Comment 10
2010-09-08 18:27:21 PDT
Created
attachment 66983
[details]
decouple minor mods mentioned above
Michael Nordman
Comment 11
2010-09-08 18:28:39 PDT
> minor mods mentioned above
doh... forgot one of the minor mods
Michael Nordman
Comment 12
2010-09-08 18:32:26 PDT
Created
attachment 66985
[details]
decouple including the minor mod to only deserialize Files if (m_isDOMGlobalObject) for jsc
Jian Li
Comment 13
2010-09-08 19:04:01 PDT
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
Michael Nordman
Comment 14
2010-09-09 12:26:14 PDT
Created
attachment 67079
[details]
decouple
> tempory => temporary
Fixed. Also renamed the local var in that method from readableURL to urlForReading.
Dumitru Daniliuc
Comment 15
2010-09-09 12:31:16 PDT
Comment on
attachment 67079
[details]
decouple rs=me. previously r+'d by jianli.
WebKit Commit Bot
Comment 16
2010-09-10 09:20:23 PDT
Comment on
attachment 67079
[details]
decouple Clearing flags on attachment: 67079 Committed
r67208
: <
http://trac.webkit.org/changeset/67208
>
WebKit Commit Bot
Comment 17
2010-09-10 09:20:28 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug