Summary: | Add the support to register the blob data. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jian Li <jianli> | ||||||||
Component: | WebCore JavaScript | Assignee: | Jian Li <jianli> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, dglazkov, dimich, eric, kinuko, levin, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Jian Li
2010-08-05 18:09:54 PDT
Created attachment 63675 [details]
Proposed Patch
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.
Attachment 63675 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3637378 Created attachment 63756 [details]
Proposed Patch
Fixed style errors.
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). Created attachment 63944 [details]
Proposed Patch
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. 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.
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? http://trac.webkit.org/changeset/65102 might have broken Qt Linux Release minimal Committed as http://trac.webkit.org/changeset/65102. |