[webkit-reviews] review denied: [Bug 43600] Add the support to register the blob data. : [Attachment 63756] 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 43600: Add the support to register the blob data.
https://bugs.webkit.org/show_bug.cgi?id=43600

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

------- Additional Comments from David Levin <levin at chromium.org>
The biggest concern is the threadsafety issues mentioned below.


> diff --git a/WebCore/WebCore.xcodeproj/project.pbxproj
b/WebCore/WebCore.xcodeproj/project.pbxproj

There are a lot of changes in here unrelated to blobs. it would be nice to get
rid of those.

> diff --git a/WebCore/bindings/js/SerializedScriptValue.cpp
b/WebCore/bindings/js/SerializedScriptValue.cpp

> +    SerializedBlob(const Blob* blob)
> +    {
> +	   m_url = blob->url().copy();
> +	   m_type = blob->type().crossThreadString();
> +	   m_size = blob->size();

Ideally these would be initialized using the m_url() format.


> +    SerializedFile(const File* file)
> +    {
> +	   m_path = file->path().crossThreadString();
> +	   m_url = file->url().copy();
> +	   m_type = file->type().crossThreadString();

Ditto.


> diff --git a/WebCore/bindings/v8/SerializedScriptValue.cpp
b/WebCore/bindings/v8/SerializedScriptValue.cpp
> +	   PassRefPtr<Blob> blob = Blob::create(getScriptExecutionContext(),
KURL(ParsedURLString, url), type, size);

Use RefPtr for local variables.

> +	   PassRefPtr<File> file = File::create(getScriptExecutionContext(),
path, KURL(ParsedURLString, url), type);

Ditto.



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


> +BlobRegistry& BlobRegistry::instance()
> +{
> +    DEFINE_STATIC_LOCAL(BlobRegistryImpl, instance, ());

Doesn't this initialization need to be thread-safe?

If so, use AtomicallyInitializedStatic.


Actually none of these methods appears thread safe, so how does it function
with workers?


> +
> +void BlobRegistryImpl::appendStorageItems(BlobStorageData* blobStorageData,
const BlobStorageDataItemList& items, long long offset, long long length)
> +{

Why not change length = -1 to MAX LONG LONG and then get rid of the special
casing of it throughout. (Is all of the code OK with a blob that has a size of
MAX LONG LONG.)


> +    for (; iter != items.end() && (length == -1 || length > 0); ++iter) {
> +	   long long remainingLength = iter->length - offset;

I think "currentLength" reads better than remainingLength. (remainingLength
indicates the length remaining to append but that is "length" in this code.)

> +	   long long newLength = (remainingLength > length && length != -1) ?
length : remainingLength;
> +	   if (iter->type == BlobStorageDataItem::Data)
> +	       blobStorageData->appendData(iter->data, iter->offset + offset,
newLength);
> +	   else {
> +	       ASSERT(iter->type == BlobStorageDataItem::File);
> +	       blobStorageData->appendFile(iter->path, iter->offset + offset,
newLength, iter->expectedModificationTime);
> +	   }
> +	   if (length != -1)
> +	       length -= newLength;
> +	   offset = 0;
> +    }
> +}
> +
> +void BlobRegistryImpl::registerBlobURL(const KURL& url, PassOwnPtr<BlobData>
blobData)

> +	       break;
> +	   } default:

It is preferred not to have "default" when switching on an enum to allow the
compiler to detect the unhandled enum.

> +	       ASSERT_NOT_REACHED();
> +	   }
> +    }
> +
> +
> +    m_blobs.set(url.string(), blobStorageData);
> +}



> diff --git a/WebCore/html/BlobRegistryImpl.h
b/WebCore/html/BlobRegistryImpl.h
> +    virtual void registerBlobURL(const KURL& url, const KURL& srcURL);

"url" param name not needed here.

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

> +KURL BlobURL::getOrigin(const KURL& url)
> +{
> +    ASSERT(url.protocolIs("blob"));
> +
> +    unsigned s = url.pathStart();;
> +    unsigned e = url.pathAfterLastSlash();

Prefer whole words. Perhaps startIndex, afterEndIndex?

> +    String origin = url.string().substring(s, e - s);
> +    return KURL(ParsedURLString, decodeURLEscapeSequences(origin));


> diff --git a/WebCore/platform/BlobData.h b/WebCore/platform/BlobData.h

> +    void appendBlob(const KURL& url, long long offset, long long length);

Param name "url" not needed.

> diff --git a/WebCore/platform/BlobRegistry.h
b/WebCore/platform/BlobRegistry.h
> +    virtual void registerBlobURL(const KURL&, PassOwnPtr<BlobData>) = 0;
> +    virtual void registerBlobURL(const KURL& url, const KURL& srcURL) = 0;

No need for "url" parameter name here (1st parameter name).