[webkit-reviews] review denied: [Bug 43871] [chromium] Chromium side implementation of blob data and blob registry : [Attachment 64284] Proposed Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Jian Li
<jianli at chromium.org>'s request for review:
Bug 43871: [chromium] Chromium side implementation of blob data and blob
registry
https://bugs.webkit.org/show_bug.cgi?id=43871

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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.