[chromium] Chromium side implementation of blob data and blob registry

[Jian Li]

Need to hook up the logic in chromium webkit to register the blob data.

[Jian Li]

Created attachment 64154 [details]
Proposed Patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/BlobRegistryProxy.cpp:42:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/chromium/src/BlobRegistryProxy.h:31:  #ifndef header guard has wrong style, please use: BlobRegistryProxy_h  [build/header_guard] [5]
WebKit/chromium/public/WebBlobRegistry.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 14 files


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

[Jian Li]

Created attachment 64156 [details]
Proposed Patch

Fixed style errors. The 1st error is false alarm.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/BlobRegistryProxy.cpp:42:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 14 files


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

[David Levin]

Comment on attachment 64156 [details]
Proposed Patch

Here's my pass. Of course, we should get Darin to review the chromium/public files before committing.



There appear to be a lot of places in here that use blob but don't have #if ENABLE(BLOB) around them (like WebKit/chromium/public/WebBlobRegistry.h or WebKit/chromium/src/WebBlobData.cpp). Does it need to be added?


> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 630eeb0..0f0aaa6 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,15 @@
> [phone]  Jian Li  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Chromium side implementation of blob data and blob registry.
> +        https://bugs.webkit.org/show_bug.cgi?id=43871
> +
> +        Wrap !PLATFORM(CHROMIUM) around BlobRegistry::instance() so that we will

s/we will use/Chromium uses/


> diff --git a/WebKit/chromium/public/WebBlobData.h b/WebKit/chromium/public/WebBlobData.h

> +
> +#ifndef WebBlobData_h
> +#define WebBlobData_h
> +
> +#include "WebData.h"
> +#include "WebFileInfo.h"
> +#include "WebString.h"
> +
> +#if WEBKIT_IMPLEMENTATION
> +namespace WebCore { class BlobData; }
> +namespace WTF { template <typename T> class PassOwnPtr; }
> +#endif
> +
> +namespace WebKit {
> +
> +class WebBlobDataPrivate;
> +
> +class WebBlobData {
> +public:
> +    struct Item {
> +        enum { TypeData, TypeFile, TypeBlob } type;
> +        WebData data;
> +        WebString pathOrURL;
> +        long long offset;
> +        long long length; // -1 means to the end of the file/blob.

"-1 means *go* to the end of the file/blob."

> +        WebFileInfo fileInfo;
> +    };
> +


> +    WEBKIT_API void assign(const WebBlobData&);

Is this implemented anywhere?


> +    // Sets the values of the item at the given index.

"Sets" sounds like it changes the values at that index.

Perhaps:  "Retrieves the data at the given index."

> +    WEBKIT_API bool itemAt(size_t index, Item&) const;

I like the parameter name "result" here as it tells me that the function is filling that in.

I also thing the function name should be getItemAt (imo, itemAt would work better if it actually returned the item.)


> diff --git a/WebKit/chromium/public/WebBlobRegistry.h b/WebKit/chromium/public/WebBlobRegistry.h
> +#include "WebBlobStorageData.h"
> +#include "WebCommon.h"

Why do these headers need to be included? (It looks like fwd declarations would be good enough.)

> +    // Note that the ownership of the WebBlobData is passed to the function.
I don't under this since WebBlobData is passed by reference.

> +    virtual void registerBlobURL(const WebURL&, WebBlobData&) = 0;
> +    virtual void registerBlobURL(const WebURL& url, const WebURL& srcURL) = 0;

The parameter name |url| seems unnecessary here.

> diff --git a/WebKit/chromium/public/WebBlobStorageData.h b/WebKit/chromium/public/WebBlobStorageData.h

> +namespace WebKit {
> +
> +class WebBlobStorageDataPrivate;
> +
> +class WebBlobStorageData {
> +public:
> +    struct Item {
> +        enum { TypeData, TypeFile } type;
> +        WebData data;
> +        WebString path;
> +        long long offset;
> +        long long length; // -1 means to the end of the file/blob.

Same comment as before (about the comment).

> +        WebFileInfo fileInfo;
> +    };
>

> +    // Sets the values of the item at the given index.  Returns false if

Single space after "." (and same comment as before about the comment).

> +    // index is out of bounds.
> +    WEBKIT_API bool itemAt(size_t index, Item&) const;
> +
> +    WEBKIT_API WebString contentType() const;
> +    WEBKIT_API WebString contentDisposition() const;


> diff --git a/WebKit/chromium/public/WebKitClient.h b/WebKit/chromium/public/WebKitClient.h
> +    // Register a Blob object by its url.
> +    virtual void registerBlobURL(const WebURL&, const WebBlobData&) { }
> +    virtual void registerBlobURL(const WebURL& url, const WebURL& srcURL) { }

|url| param name.

> diff --git a/WebKit/chromium/src/BlobRegistryProxy.cpp b/WebKit/chromium/src/BlobRegistryProxy.cpp
> + * Copyright (C) 2009 Google Inc. All rights reserved.

2010

> +#include "WebBlobData.h"
> +#include "WebKit.h"
> +#include "WebKitClient.h"
> +#include "WebURL.h"
> +
> +#include "BlobData.h"
> +#include "KURL.h"
> +#include "ResourceHandle.h"
> +#include <wtf/StdLibExtras.h>

Why aren't these sorted?


> +
> +// We are part of the WebKit implementation.
> +using namespace WebKit;
> +
> +namespace WebCore {
> +
> +BlobRegistry& BlobRegistry::instance()
> +{
> +    DEFINE_STATIC_LOCAL(BlobRegistryProxy, instance, ());

Is threadsafety coming in the future?

> diff --git a/WebKit/chromium/src/BlobRegistryProxy.h b/WebKit/chromium/src/BlobRegistryProxy.h
> +    virtual void registerBlobURL(const KURL& url, const KURL& srcURL);

|url| param name.

> diff --git a/WebKit/chromium/src/WebBlobData.cpp b/WebKit/chromium/src/WebBlobData.cpp

> +class WebBlobDataPrivate : public BlobData {
> +};
> +
> +void WebBlobData::initialize()
> +{
> +    assign(static_cast<WebBlobDataPrivate*>(BlobData::create().leakPtr()));

Make assign take a PassOwnPtr and then avoid calling leakPtr here.


> +bool WebBlobData::itemAt(size_t index, Item& result) const

> +        result.offset = 0;
> +        result.length = 0;
> +        result.fileInfo.modificationTime = 0;

Why aren't the item.offset, item.length, and item.expectedModificationTime used here (even if they are always 0 in this case)?
If we do this, can we move the copying of offset, length, fileInfo.modificationTime to be outside of/before the switch statement?

> +    default:
> +        ASSERT_NOT_REACHED();

Better to not have this in the switch statement.
In this case, you can change the "break;" to "return true;" and then put the "ASSERT_NOT_REACHED();" after the switch statement. Then get the benefit of the compiler enum detection for switch statements plus the not reached assert as a runtime check.


> +void WebBlobData::assign(WebBlobDataPrivate* p)
> +{
> +    if (m_private)
> +        delete m_private;
> +    m_private = p;

Is there no equivalent of OwnPtr used in the chromium api layer?  (Why not use OwnPtr? Of course, you'll need to more the destructor into this file and then you can get rid of this method and lots of things in here become cleaner.)


> diff --git a/WebKit/chromium/src/WebBlobRegistryImpl.cpp b/WebKit/chromium/src/WebBlobRegistryImpl.cpp
> +namespace WebKit {
> +
> +WebBlobRegistry* WebBlobRegistry::instance()
> +{
> +    DEFINE_STATIC_LOCAL(WebBlobRegistryImpl, instance, ());

Thread safety?

> +    return &instance;

> diff --git a/WebKit/chromium/src/WebBlobRegistryImpl.h b/WebKit/chromium/src/WebBlobRegistryImpl.h

> +    virtual void registerBlobURL(const WebURL& url, const WebURL& srcURL);
|url| param name.

> diff --git a/WebKit/chromium/src/WebBlobStorageData.cpp b/WebKit/chromium/src/WebBlobStorageData.cpp

> +void WebBlobStorageData::initialize()
> +{
> +    assign(static_cast<WebBlobStorageDataPrivate*>(BlobStorageData::create().releaseRef()));

I find this confusing. It looks wrong to me b/c releaseRef will return a PassRefPtr (which will decrement the ref if nothing is done) and it is casted to a *, so the ref count is lost.

I think this should be a leakRef call, but I really believe that m_private should be a RefPtr and assign should go away.


> +bool WebBlobStorageData::itemAt(size_t index, Item& result) const
> +{
> +    ASSERT(!isNull());
> +
> +    if (index >= m_private->items().size())
> +        return false;
> +
> +    const BlobStorageDataItem& item = m_private->items()[index];
> +    switch (item.type) {
> +    case BlobStorageDataItem::Data:
> +        result.type = Item::TypeData;
> +        result.data.assign(item.data.data(), static_cast<size_t>(item.data.length()));
> +        result.offset = item.offset;
> +        result.length = item.length;
> +        break;
> +    case BlobStorageDataItem::File:
> +        result.type = Item::TypeFile;
> +        result.path = item.path;
> +        result.offset = item.offset;
> +        result.length = item.length;
> +        result.fileInfo.modificationTime = item.expectedModificationTime;
> +        break;

> +    default:
> +        ASSERT_NOT_REACHED();

Same comments as before on similar code.

[Jian Li]

Created attachment 64266 [details]
Proposed Patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/BlobRegistryProxy.h:31:  #ifndef header guard has wrong style, please use: BlobRegistryProxy_h  [build/header_guard] [5]
Total errors found: 1 in 14 files


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

[Jian Li]

(In reply to comment #5)
> There appear to be a lot of places in here that use blob but don't have #if ENABLE(BLOB) around them (like WebKit/chromium/public/WebBlobRegistry.h or WebKit/chromium/src/WebBlobData.cpp). Does it need to be added?

Other files under WebKit/chromium/public do not use feature guard. So I am following them.

> > +    // Sets the values of the item at the given index.
> 
> "Sets" sounds like it changes the values at that index.
> 
> Perhaps:  "Retrieves the data at the given index."
> 
> > +    WEBKIT_API bool itemAt(size_t index, Item&) const;
> 
> I like the parameter name "result" here as it tells me that the function is filling that in.
> 
> I also thing the function name should be getItemAt (imo, itemAt would work better if it actually returned the item.)
> 
I still use itemAt, in order to be consistent the similar function name elementAt in WebHTTPBody.h.

> 
> > diff --git a/WebKit/chromium/public/WebBlobRegistry.h b/WebKit/chromium/public/WebBlobRegistry.h
> > +#include "WebBlobStorageData.h"
> > +#include "WebCommon.h"
> 
> Why do these headers need to be included? (It looks like fwd declarations would be good enough.)
> 
I removed the include of WebCommon.h. However, I still need to include WebBlobStorageData.h since WebBlobStorageData is used as the return type in one function.

> > +// We are part of the WebKit implementation.
> > +using namespace WebKit;
> > +
> > +namespace WebCore {
> > +
> > +BlobRegistry& BlobRegistry::instance()
> > +{
> > +    DEFINE_STATIC_LOCAL(BlobRegistryProxy, instance, ());
> 
> Is threadsafety coming in the future?
I added "ASSERT(isMainThread())" here.

> > +class WebBlobDataPrivate : public BlobData {
> > +};
> > +
> > +void WebBlobData::initialize()
> > +{
> > +    assign(static_cast<WebBlobDataPrivate*>(BbloData::create().leakPtr()));
> 
> Make assign take a PassOwnPtr and then avoid calling leakPtr here.
I cannot do this because WebBlobDataPrivate is inherited from BlobData and BbloData::create() returns PassOwnPtr<BlobData> which is not compatible with PassOwnPtr<WebBlobDataPrivate>.


> > +void WebBlobData::assign(WebBlobDataPrivate* p)
> > +{
> > +    if (m_private)
> > +        delete m_private;
> > +    m_private = p;
> 
> Is there no equivalent of OwnPtr used in the chromium api layer?  (Why not use OwnPtr? Of course, you'll need to more the destructor into this file and then you can get rid of this method and lots of things in here become cleaner.)
We could use OwnPtr. However, I follow the similar thing done in chromium api layer.

> > +WebBlobRegistry* WebBlobRegistry::instance()
> > +{
> > +    DEFINE_STATIC_LOCAL(WebBlobRegistryImpl, instance, ());
> 
> Thread safety?
I added "ASSERT(isMainThread())".

> > +void WebBlobStorageData::initialize()
> > +{
> > +    assign(static_cast<WebBlobStorageDataPrivate*>(BlobStorageData::create().releaseRef()));
> 
> I find this confusing. It looks wrong to me b/c releaseRef will return a PassRefPtr (which will decrement the ref if nothing is done) and it is casted to a *, so the ref count is lost.
> 
> I think this should be a leakRef call, but I really believe that m_private should be a RefPtr and assign should go away.
I follow the similar thing done in chromium api layer, like in WebHTTPBody.cpp.

[Jian Li]

Created attachment 64269 [details]
Proposed Patch

Fixed style error.

[David Levin]

Comment on attachment 64269 [details]
Proposed Patch

As discussed.

[Jian Li]

Created attachment 64279 [details]
Proposed Patch

[David Levin]

Comment on attachment 64279 [details]
Proposed Patch

WebBlobStorageData::assign(const WebBlobStorageData& other) needs the ref fixed as discussed.

This looks good to me (with that minor fix), and I would r+, but we should have Darin look at the WebKit/chromium/public/* files before committing.

[Jian Li]

Created attachment 64283 [details]
Proposed Patch

Removed public assign method in WebBlobStorageData since it is not needed. This is to address the last comment from David.

Darin, could you please take a look?

[Jian Li]

Created attachment 64284 [details]
Proposed Patch

Correct patch.

[Darin Fisher (:fishd, Google)]

Comment on attachment 64284 [details]
Proposed Patch

WebCore/html/BlobRegistryImpl.cpp:75
 +  BlobRegistry& BlobRegistry::instance()
I'm surprised to see BlobRegistry::instance().  Convention for WebCore is
to have a global function named like the class.  I would have expected to
see a global WebCore::blobRegistry() function.  In fact, this seems like
it should be part of platform/network as I would imagine we'll need to
extend platform/network/FormData.h to support a "blob" type.

WebCore/html/BlobRegistryImpl.cpp:@
 +  bool BlobRegistryImpl::loadResourceSynchronously(const ResourceRequest& request,
it seems odd that WebCore/html/ contains the implementation of an
interface defined in WebCore/platform/.  why is it best for
BlobRegistryImpl.cpp to live here instead of in platform/?

WebKit/chromium/public/WebBlobRegistry.h:44
 +      WEBKIT_API static WebBlobRegistry* instance();
We normally just expose a static "create" function and let the
caller use it as a singleton.  see WebStorageNamespace.h and
WebIDBFactory.h for example.

WebKit/chromium/public/WebBlobRegistry.h:48
 +      virtual void registerBlobURL(const WebURL&, WebBlobData&) = 0;
please document this public API.  it is not obvious what this does.

WebKit/chromium/public/WebBlobRegistry.h:51
 +      virtual WebBlobStorageData getBlobDataFromURL(const WebURL&) = 0;
will getBlobDataFromURL require a synchronous IPC?  why do we need this method?
oh, i see... it is only meant to be used from the in-proc-webkit thread in the
browser process.

WebKit/chromium/public/WebBlobStorageData.h:47
 +  class WebBlobStorageData {
the duplication between WebBlobData, WebBlobStorageData and WebHTTPBody
is most unfortunate.  surely there is a way to avoid this duplication?

WebKit/chromium/public/WebKitClient.h:89
 +      // Register a Blob object by its url.
I would have expected a function here that returns a WebBlobRegistry instead
of adding all of the methods of WebBlobRegistry to WebKitClient.  I realize
that means exposing the getBlobDataFromURL function, but we can just not
implement that.

[Jian Li]

I will address your first 2 comments in 2 separated patches and then come back to this one.

(In reply to comment #15)
> (From update of attachment 64284 [details])
> WebCore/html/BlobRegistryImpl.cpp:75
>  +  BlobRegistry& BlobRegistry::instance()
> I'm surprised to see BlobRegistry::instance().  Convention for WebCore is
> to have a global function named like the class.  I would have expected to
> see a global WebCore::blobRegistry() function.  In fact, this seems like
> it should be part of platform/network as I would imagine we'll need to
> extend platform/network/FormData.h to support a "blob" type.
> 
> WebCore/html/BlobRegistryImpl.cpp:@
>  +  bool BlobRegistryImpl::loadResourceSynchronously(const ResourceRequest& request,
> it seems odd that WebCore/html/ contains the implementation of an
> interface defined in WebCore/platform/.  why is it best for
> BlobRegistryImpl.cpp to live here instead of in platform/?
> 
> WebKit/chromium/public/WebBlobRegistry.h:44
>  +      WEBKIT_API static WebBlobRegistry* instance();
> We normally just expose a static "create" function and let the
> caller use it as a singleton.  see WebStorageNamespace.h and
> WebIDBFactory.h for example.
> 
> WebKit/chromium/public/WebBlobRegistry.h:48
>  +      virtual void registerBlobURL(const WebURL&, WebBlobData&) = 0;
> please document this public API.  it is not obvious what this does.
> 
> WebKit/chromium/public/WebBlobRegistry.h:51
>  +      virtual WebBlobStorageData getBlobDataFromURL(const WebURL&) = 0;
> will getBlobDataFromURL require a synchronous IPC?  why do we need this method?
> oh, i see... it is only meant to be used from the in-proc-webkit thread in the
> browser process.
> 
> WebKit/chromium/public/WebBlobStorageData.h:47
>  +  class WebBlobStorageData {
> the duplication between WebBlobData, WebBlobStorageData and WebHTTPBody
> is most unfortunate.  surely there is a way to avoid this duplication?
> 
> WebKit/chromium/public/WebKitClient.h:89
>  +      // Register a Blob object by its url.
> I would have expected a function here that returns a WebBlobRegistry instead
> of adding all of the methods of WebBlobRegistry to WebKitClient.  I realize
> that means exposing the getBlobDataFromURL function, but we can just not
> implement that.

[Jian Li]

Created attachment 64761 [details]
Proposed Patch

[Jian Li]

Created attachment 64763 [details]
Proposed Patch

Updated 2 files that're not updated correctly last time.

[Jian Li]

Created attachment 64774 [details]
Proposed Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 64774 [details]
Proposed Patch

WebKit/chromium/public/WebBlobStorageData.h:49
 +      struct Item {
how about reusing WebBlobData::Item here?  either with a using
directive or just by typing WebBlobData::Item in place of Item.

WebKit/chromium/public/WebKitClient.h:95
 +      virtual WebBlobRegistry* blobRegistry() { return 0; }
please add a comment indicating if it is OK for a client to return null.
this impacts whether or not the webkit code should null test blobRegistry
before calling methods on it.

WebKit/chromium/public/WebBlobData.h:55
 +          WebFileInfo fileInfo;
i'm concerned that we will need to add more things to WebFileInfo
in the future to support WebFileSystem::getMetadata.  you may only
want to have a timestamp here.

R=me otherwise

[Kinuko Yasuda]

(In reply to comment #20)
> (From update of attachment 64774 [details])
> WebKit/chromium/public/WebBlobStorageData.h:49
>  +      struct Item {
> how about reusing WebBlobData::Item here?  either with a using
> directive or just by typing WebBlobData::Item in place of Item.
> 
> WebKit/chromium/public/WebKitClient.h:95
>  +      virtual WebBlobRegistry* blobRegistry() { return 0; }
> please add a comment indicating if it is OK for a client to return null.
> this impacts whether or not the webkit code should null test blobRegistry
> before calling methods on it.
> 
> WebKit/chromium/public/WebBlobData.h:55
>  +          WebFileInfo fileInfo;
> i'm concerned that we will need to add more things to WebFileInfo
> in the future to support WebFileSystem::getMetadata.  you may only
> want to have a timestamp here.

Should I define a new WebFileMetadata class that just inherits WebFileInfo for now?

> R=me otherwise

[Jian Li]

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

All issues fixed.

[WebKit Review Bot]

http://trac.webkit.org/changeset/65637 might have broken Chromium Mac Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/65635
http://trac.webkit.org/changeset/65636
http://trac.webkit.org/changeset/65637