Bug 43600 – Add the support to register the blob data.

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

[Jian Li]

This is the first step towards adding Blob URL support. The blob will have an internal URL (used by FileReader) and public URLs (for blob URL, to be added later). The URL and its associated data are managed by the BlobRegistry interface.

This patch does not change the existing infrastructure of using BlobItem.

[Jian Li]

Created attachment 63675 [details]
Proposed Patch

[WebKit Review Bot]

Attachment 63675 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/html/BlobStorageData.h:1:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
WebCore/platform/BlobData.cpp:55:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebCore/bindings/js/SerializedScriptValue.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/html/BlobURL.cpp:62:  More than one command on the same line  [whitespace/newline] [4]
WebCore/html/Blob.cpp:70:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/bindings/js/SerializedScriptValue.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 113 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 63675 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3637378

[Jian Li]

Created attachment 63756 [details]
Proposed Patch

Fixed style errors.

[David Levin]

Comment on attachment 63756 [details]
Proposed Patch

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

[Jian Li]

Created attachment 63944 [details]
Proposed Patch

[Jian Li]

All fixed except the one commented inline.

(In reply to comment #5)
> (From update of attachment 63756 [details])
> The biggest concern is the threadsafety issues mentioned below.
> 
> 
> > 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?

The thread-safe support for worker thread is not hooked yet. I am going to introduce a separate proxy class to handle all BlobRegistry calls in the main thread. For now, since we have not switched to using blob data code path yet, I think we can keep the current patent. I added comments in BlobRegistry.h and all the calls of BlobRegistry methods to explain this.
> 
> 
> > +
> > +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.)
> 
> 
Unfortunately we have been using "-1" to mean to end of file for quite some time. There're other codes that assume this.

For better readability, I introduced constants in BlobData.h.

[WebKit Review Bot]

Attachment 63944 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/html/BlobStorageData.h:1:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
WebCore/html/BlobURL.cpp:62:  More than one command on the same line  [whitespace/newline] [4]
WebCore/bindings/js/SerializedScriptValue.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 110 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Levin]

Comment on attachment 63944 [details]
Proposed Patch

Just a few things to consider...

> diff --git a/WebCore/html/BlobRegistryImpl.cpp b/WebCore/html/BlobRegistryImpl.cpp
> +void BlobRegistryImpl::appendStorageItems(BlobStorageData* blobStorageData, const BlobStorageDataItemList& items, long long offset, long long length)
> +{

Convert length == BlobDataItem::toEndOfFile to MAX LONG LONG here using an if statement?

> +    BlobStorageDataItemList::const_iterator iter = items.begin();
> +    if (offset) {
> +        for (; iter != items.end(); ++iter) {
> +            ASSERT(iter->length != BlobDataItem::toEndOfFile);
> +            if (offset >= iter->length)
> +                offset -= iter->length;
> +            else
> +                break;
> +        }
> +    }
> +
> +    for (; iter != items.end() && (length == BlobDataItem::toEndOfFile || length > 0); ++iter) {
> +        long long currentLength = iter->length - offset;
> +        long long newLength = (currentLength > length && length != BlobDataItem::toEndOfFile) ? length : currentLength;
> +        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 != BlobDataItem::toEndOfFile)
> +            length -= newLength;
> +        offset = 0;
> +    }
> +}


> diff --git a/WebCore/html/BlobRegistryImpl.h b/WebCore/html/BlobRegistryImpl.h
> +#if ENABLE(BLOB)
> +
> +#include "BlobData.h"
> +#include "BlobRegistry.h"
> +#include "BlobStorageData.h"
> +#include "PlatformString.h"
> +#include "StringHash.h"
> +#include <wtf/HashMap.h>
> +#include <wtf/RefCounted.h>

RefCounted doesn't seem to be used in this header (but PassOwnPtr and RefPtr are.)


> diff --git a/WebCore/html/BlobStorageData.h b/WebCore/html/BlobStorageData.h

> +    typedef enum { Data, File } BlobStoreDataItemType;

Why not just

 enum BlobStoreDataItemType { 
    Data, 
    File 
 };

without the typedef?

[WebKit Review Bot]

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

[Jian Li]

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