Bug 43874 – Add ThreadableBlobRegistry to support calling BlobRegistry methods in main thread

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

[Jian Li]

Add ThreadableBlobRegistry to support calling BlobRegistry methods in main thread. This is needed to support blobs in workers.

[Jian Li]

Created attachment 64168 [details]
Proposed Patch

[David Levin]

Comment on attachment 64168 [details]
Proposed Patch

Just a few things.

I think you're missing the Android build file.


> diff --git a/WebCore/dom/ScriptExecutionContext.cpp b/WebCore/dom/ScriptExecutionContext.cpp

I think there are a lot of items in here which should be guarded with if ENABLE(BLOB) (even some which don't have it from the previous checkin).



FUTURE (maybe next checkin): trackBlobURL and revokeBlobURL feel very asymetric. revoke (and ~ScriptExecutionContext) calls unregisterBlobURL but track doesn't call registerBlobURL. (It feels like one class doing the ref and another doing the deref.)

I'd recommend that at least registerBlobURL be moved into trackBlobURL and perhaps the name changed to createPublicURL. (Maybe most of the guts of Blob::createPublicURL should move into the ScriptExecutionContext function.)
 


> diff --git a/WebCore/html/Blob.cpp b/WebCore/html/Blob.cpp
>  Blob::Blob(ScriptExecutionContext*, const PassRefPtr<BlobItem>& item)
> -    : m_size(0)
> +    : m_scriptExecutionContext(0)
> +    , m_size(0)

Why isn't m_type initialized for this constructor?



> diff --git a/WebCore/html/BlobRegistryImpl.cpp b/WebCore/html/BlobRegistryImpl.cpp
 >  void BlobRegistryImpl::registerBlobURL(const KURL& url, PassOwnPtr<BlobData> blobData)
>  {
> +    ASSERT(isMainThread());

I like the assert. Can this assert be added to instance() (even if it needs to move into the cpp file)?


> diff --git a/WebCore/html/ThreadableBlobRegistry.cpp b/WebCore/html/ThreadableBlobRegistry.cpp

> +void postTaskToMainThread(ScriptExecutionContext*, PassOwnPtr<ScriptExecutionContext::Task>);
> +void registerBlobURLTask(ScriptExecutionContext*, const KURL&, PassOwnPtr<BlobData>);
> +void registerBlobURLFromTask(ScriptExecutionContext*, const KURL& url, const KURL& srcURL);
> +void unregisterBlobURLTask(ScriptExecutionContext*, const KURL&);

I think these prototypes should be removed, and the functions declared as "static" instead.



> diff --git a/WebCore/html/ThreadableBlobRegistry.h b/WebCore/html/ThreadableBlobRegistry.h
> +public:
> +    static void registerBlobURL(ScriptExecutionContext*, const KURL&, PassOwnPtr<BlobData>);
> +    static void registerBlobURL(ScriptExecutionContext*, const KURL& url, const KURL& srcURL);

|url| param name.

[Jian Li]

Created attachment 64246 [details]
Proposed Patch

[Jian Li]

All fixed except commented ones.

(In reply to comment #2)
> (From update of attachment 64168 [details])
> Just a few things.
> 
> I think you're missing the Android build file.
> 
> 
> > diff --git a/WebCore/dom/ScriptExecutionContext.cpp b/WebCore/dom/ScriptExecutionContext.cpp
> 
> I think there are a lot of items in here which should be guarded with if ENABLE(BLOB) (even some which don't have it from the previous checkin).
> 
I am trying to figure out the boundary of using guards or not in order to be compatible with the existing file feature. I will fix this in my next checkin altogether.
> 
> 
> FUTURE (maybe next checkin): trackBlobURL and revokeBlobURL feel very asymetric. revoke (and ~ScriptExecutionContext) calls unregisterBlobURL but track doesn't call registerBlobURL. (It feels like one class doing the ref and another doing the deref.)
> 
> I'd recommend that at least registerBlobURL be moved into trackBlobURL and perhaps the name changed to createPublicURL. (Maybe most of the guts of Blob::createPublicURL should move into the ScriptExecutionContext function.)
> 
I fixed this in this patch since your suggestion really makes thing much clear.
> 
> 
> > diff --git a/WebCore/html/Blob.cpp b/WebCore/html/Blob.cpp
> >  Blob::Blob(ScriptExecutionContext*, const PassRefPtr<BlobItem>& item)
> > -    : m_size(0)
> > +    : m_scriptExecutionContext(0)
> > +    , m_size(0)
> 
> Why isn't m_type initialized for this constructor?
> 
This is legacy constructor that is going to be removed when we switch to new BlobData structure. The type string is simply empty for this constructor.

[WebKit Review Bot]

http://trac.webkit.org/changeset/65271 might have broken Qt Linux Release minimal

[Jian Li]

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