[webkit-reviews] review denied: [Bug 43874] Add ThreadableBlobRegistry to support calling BlobRegistry methods in main thread : [Attachment 64168] Proposed Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Jian Li <jianli at chromium.org>'s
request for review:
Bug 43874: Add ThreadableBlobRegistry to support calling BlobRegistry methods
in main thread
https://bugs.webkit.org/show_bug.cgi?id=43874

Attachment 64168: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=64168&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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.