[webkit-reviews] review granted: [Bug 43600] Add the support to register the blob data. : [Attachment 63944] Proposed Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted 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 63944: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=63944&action=review

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