Bug 44116 – Move BlobRegistry interface and implementation to platform/network

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Jian Li]

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.

[Jian Li]

Created attachment 64603 [details]
Proposed Patch

[Darin Fisher (:fishd, Google)]

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.

[Darin Fisher (:fishd, Google)]

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.

[Jian Li]

(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.

[Kinuko Yasuda]

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.)

[Jian Li]

(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.

[Kinuko Yasuda]

(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.

[Jian Li]

Committed as http://trac.webkit.org/changeset/65526.