[webkit-reviews] review denied: [Bug 44722] Add support for "downloadToFile" to ResourceRequest and ResourceResponse : [Attachment 70005] downloadAsBlob

Wed Oct 6 17:02:16 PDT 2010

Adam Barth <abarth at webkit.org> has denied Michael Nordman
<michaeln at google.com>'s request for review:
Bug 44722: Add support for "downloadToFile" to ResourceRequest and
ResourceResponse
https://bugs.webkit.org/show_bug.cgi?id=44722

Attachment 70005: downloadAsBlob
https://bugs.webkit.org/attachment.cgi?id=70005&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=70005&action=review

We need tests for this code.

> WebCore/loader/DocumentThreadableLoader.cpp:236
> +    // Ignore response body of preflight requests.

We can skip this comment.

> WebCore/loader/DocumentThreadableLoader.cpp:273
> +    if (m_actualRequest) {
> +	   ASSERT(!m_sameOriginRequest);
> +	   ASSERT(m_options.crossOriginRequestPolicy == UseAccessControl);
> +	   preflightSuccess();
> +    } else

Prefer early return.

> WebCore/loader/DocumentThreadableLoader.cpp:367
> +	       dataAsBlob =
m_document->frame()->loader()->loadResourceSynchronouslyAsBlob(request,
storedCredentials, error, response, identifier);
> +	   else
> +	       identifier =
m_document->frame()->loader()->loadResourceSynchronously(request,
storedCredentials, error, response, data);

Should these methods assert that request.downloadAsBlob is set to the right
thing?

> WebCore/loader/DocumentThreadableLoader.cpp:388
> +	  didFinishLoadingBlob(identifier, dataAsBlob.release());

bad indent.  prefer early return.

> WebCore/loader/FrameLoader.cpp:2740
>  unsigned long FrameLoader::loadResourceSynchronously(const ResourceRequest&
request, StoredCredentials storedCredentials, ResourceError& error,
ResourceResponse& response, Vector<char>& data)

This method shouldn't be on FrameLoader, but that's not your fault.

> WebCore/loader/FrameLoader.cpp:2742
> +    ASSERT(!request.downloadAsBlob());

Ah, I see it does.

> WebCore/loader/FrameLoader.cpp:2744
> +    unsigned long identifier = prepareSyncReqeust(request, error,
actualRequest);

prepareSyncReqeust => prepareSyncRequest

> WebCore/loader/FrameLoader.cpp:2761
> +PassRefPtr<Blob> FrameLoader::loadResourceSynchronouslyAsBlob(const
ResourceRequest& request, StoredCredentials storedCredentials, ResourceError&
error, ResourceResponse& response, unsigned long& identifier)

These two methods duplicate a lot of code.  Is it possible to share more? 
Perhaps pass which ResourceHandle static method to use as a template parameter?

> WebCore/loader/ResourceLoader.cpp:286
> +void ResourceLoader::didFinishLoadingBlob(double finishTime,
PassRefPtr<Blob>)

It seems strange to use a PassRefPtr and then to ignore the argument.  Did you
mean to pass a Blob* ?

> WebCore/loader/ResourceLoader.cpp:439
> +void ResourceLoader::didReceiveDataCommon(bool isBlobData, const char* data,
int length, int lengthReceived)

Can we use an enum instead of isBlobData ?  The call sites are hard to read.

> WebCore/loader/ResourceLoader.h:83
> +	   virtual void didReceiveBlobData(int) { }

Please don't declare virtual functions inline.

> WebCore/loader/ResourceLoader.h:106
> +	   virtual void didFinishLoadingBlob(ResourceHandle*, double
/*finishTime*/, PassRefPtr<Blob>);

There's not reason to comment out the formal parameter name.

> WebCore/loader/SubresourceLoader.cpp:195
> +void SubresourceLoader::didFinishLoadingCommon(bool asBlob, double
finishTime, PassRefPtr<Blob> blob)

Same comment w.r.t. enum here.	Also, please use the same variable names for
consistency.

> WebCore/loader/SubresourceLoader.cpp:206
> +	       m_client->didFinishLoadingBlob(this, blob);

I'm confused.  Does didFinishLoadingBlob take a PassRefPtr?  If so, won't blob
be null below?

> WebCore/loader/WorkerThreadableLoader.cpp:235
> +	   createCallbackTask(&workerContextDidFinishLoadingBlob,
m_workerClientWrapper, identifier, blob->url(), blob->type(), blob->size()),
> +	   m_taskMode);

We don't usually put in this kind of line break.

> WebCore/loader/WorkerThreadableLoader.h:137
> +	       // Only to be used on the main thread.

This comment doesn't match the one above.  Is that an intentional difference?

> WebCore/platform/network/ResourceHandle.cpp:196
> +PassRefPtr<Blob> ResourceHandle::loadResourceSynchronouslyAsBlob(
> +						  NetworkingContext* context,
> +						  const ResourceRequest&
request,
> +						  StoredCredentials
storedCredentials,
> +						  ResourceError& error,
> +						  ResourceResponse& response)

This all goes on one line.  Yes, it's ugly.

> WebCore/platform/network/ResourceHandle.cpp:214
> +	   blobData->appendData(CString(data.data(), data.size())); // FIXME:
Don't copy the data.

Woah, that's a big memcpy.  This change is already huge, but please follow up
and fix this!!!

> WebCore/platform/network/ResourceRequestBase.h:228
> +	   bool m_downloadAsBlob;

There's a general problem that we can't add members to ResourceRequestBase that
can't be represented in an NSURLRequest (or something like that).  I'm not sure
if this issue has been resolved.  We should check with some Apple folks.

> WebCore/platform/network/chromium/ResourceRequest.h:87
> +	   // If true, the response body will be provided as a file.

Remove this comment.

> WebCore/platform/network/chromium/ResourceRequest.h:89
> +	   bool downloadToFile() const { return downloadAsBlob(); }
> +	   void setDownloadToFile(bool download) { setDownloadAsBlob(download);
}

Why are we switching names from "AsBlob" to "ToFile" ?

> WebCore/platform/network/chromium/ResourceResponse.cpp:40
> +    m_downloadFilePath  = path;

extra space before =

> WebCore/xml/XMLHttpRequest.cpp:1041
> +void XMLHttpRequest::didReceiveDataCommon(bool isBlobData, const char* data,
int len)

isBlobData => enum
len => length

> WebCore/xml/XMLHttpRequest.cpp:1069
> +	   if (len == -1) 
> +	       len = strlen(data);

wow, that's terrible.  I had no idea this was here.

> WebKit/chromium/src/ResourceHandle.cpp:89
> +    static PassRefPtr<Blob> loadResourceSynchronously(NetworkingContext*
context,
> +							 const ResourceRequest&
request,
> +							 StoredCredentials
storedCredentials,
> +							 ResourceError& error,
> +							 ResourceResponse&
response,
> +							 Vector<char>* data);

one line

> WebKit/chromium/src/ResourceHandle.cpp:247
> +String ResourceHandleInternal::determineContentType(const ResourceResponse&
response)
> +{
> +    if (response.isHTTP())
> +	   return
extractMIMETypeFromMediaType(response.httpHeaderField("Content-Type"));
> +    return response.mimeType();
> +}

Why is this implemented here instead of as a member of ResourceResponse?

> WebKit/chromium/src/ResourceHandle.cpp:255
> +PassRefPtr<Blob>
ResourceHandleInternal::loadResourceSynchronously(NetworkingContext* context,
> +								      const
ResourceRequest& request,
> +								     
StoredCredentials storedCredentials,
> +								     
ResourceError& error,
> +								     
ResourceResponse& response,
> +								     
Vector<char>* data)

one line.

> WebKit/chromium/src/ResourceHandle.cpp:266
> +    long long lengthOut = 7777;

7777?

> WebKit/chromium/src/ResourceHandle.cpp:268
> +    // TODO(michaeln): Some way to get the lengthOut in the asFile case.

TODO(michaeln) => FIXME