This is to address fishd's feedback at bug 43871. We want to move BlobRegistry interface and implementation to a better place: platform/network. We also need to move BlobData and BlobStorageData.
Created attachment 64603 [details] Proposed Patch
Comment on attachment 64603 [details] Proposed Patch WebCore/platform/network/BlobData.h:65 + BlobDataItem(const String& path) explicit? WebCore/platform/network/BlobData.h:55 + BlobDataItem(const CString& data) explicit? WebCore/platform/network/BlobRegistry.h:59 + virtual ~BlobRegistry() { } can this destructor be protected? WebCore/platform/network/BlobRegistry.h:53 + virtual void registerBlobURL(const KURL&, PassOwnPtr<BlobData>) = 0; i think it would be helpful to document these methods. it is at least not entirely obvious what the second form of registerBlobURL means. WebCore/platform/network/BlobRegistry.h:51 + static BlobRegistry& instance(); i think a global WebCore::blobRegistry() method would be more conventional for WebCore. WebCore/platform/network/BlobStorageData.h:41 + struct BlobStorageDataItem { there seems to be unfortunate repetition between BlobDataItem and BlobStorageDataItem. have you considered reusing BlobDataItem here? WebCore/platform/network/BlobRegistryImpl.h:62 + PassRefPtr<BlobStorageData> getBlobDataFromURL(const KURL&) const; why not just reuse BlobData here? you can say that getBlobDataFromURL returns a "canonical" BlobData that has all Blob references resolved to either Data or File references. it just seems like it would be simpler to have fewer classes.
Comment on attachment 64603 [details] Proposed Patch Since this bug is really just about moving the files, I'm going to R+. The other suggested changes can certainly happen separately if you prefer.
(In reply to comment #3) > (From update of attachment 64603 [details]) > Since this bug is really just about moving the files, I'm going to R+. The > other suggested changes can certainly happen separately if you prefer. I originally plan to change to use blobRegistry() in another patch. Since you have more feedbacks, I will address all of them in another patch.
I assume this move implies that we'll have BlobResourceHandle in platform/network too and consequently we'll want to move FileStream and related stuff under platform. Is that the case? (We have a reference to blob in unimplemented FileStream::write but it can be replaced with a file path or blob ID) (By the way I thought it'd be good to have separate classes for BlobStorageData and BlobData - but ok they look VERY similar so it'd be better to unify them. Sorry Jian if I contributed to the decision.)
(In reply to comment #5) > I assume this move implies that we'll have BlobResourceHandle in platform/network too and consequently we'll want to move FileStream and related stuff under platform. Is that the case? > (We have a reference to blob in unimplemented FileStream::write but it can be replaced with a file path or blob ID) > > (By the way I thought it'd be good to have separate classes for BlobStorageData and BlobData - but ok they look VERY similar so it'd be better to unify them. Sorry Jian if I contributed to the decision.) Ideally BlobResourceHandle should be put in platform/network and FileStream and FileStreamProxy should be moved to platform/network. However, there're some kind of dependency violations we need to resolve. The first one is reference to blob in FileStream::write. This can be resolved as you suggest. In addition, we cannot pass Blob object between thread. The other one is reference to ScriptExecutionContext (and CrossThreadTask) in FileStreamProxy and BlobResourceHandle. This is needed to post task to different thread. I am not sure if there is a good way to fix this. We could add something like postTask to ResourceHandleClient. However, Task is defined in dom/ScriptExecutionContext.
(In reply to comment #6) > Ideally BlobResourceHandle should be put in platform/network and FileStream and FileStreamProxy should be moved to platform/network. However, there're some kind of dependency violations we need to resolve. > > The first one is reference to blob in FileStream::write. This can be resolved as you suggest. In addition, we cannot pass Blob object between thread. > > The other one is reference to ScriptExecutionContext (and CrossThreadTask) in FileStreamProxy and BlobResourceHandle. This is needed to post task to different thread. I am not sure if there is a good way to fix this. We could add something like postTask to ResourceHandleClient. However, Task is defined in dom/ScriptExecutionContext. Ah, you're right there're more dependencies. ...I see so it needs more consideration to move BlobResourceHandle.
Committed as http://trac.webkit.org/changeset/65526.