Support handling blob URL and resolve blob references in upload data....

Support handling blob URL and resolve blob references in upload data.

BUG=none
TEST=unittest

[michaeln, 2010-08-28 00:55:14]

Some preliminary comments. Many/most of these are me just thinking out loud a
bit. I have to look more closely, there's a lot of code here to review. I'll
give you better comments tomorrow.

I do think you should get rid of the special case for "single non-UNC file" and
just go down the same code path in all cases on all platforms.

http://codereview.chromium.org/3282003/diff/1/2
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/3282003/diff/1/2#newcode582
chrome/browser/browser_init.cc:582: &BlobURLRequestJobFactory);
Would be good to do this registration on the IO thread.

http://codereview.chromium.org/3282003/diff/1/11
File webkit/blob/blob_storage_controller.cc (right):

http://codereview.chromium.org/3282003/diff/1/11#newcode102
webkit/blob/blob_storage_controller.cc:102: // It is OK that we cannot find the
blob corresponding to src_url.
Under what circumstances would that be expected?

http://codereview.chromium.org/3282003/diff/1/11#newcode138
webkit/blob/blob_storage_controller.cc:138: new_upload_data->AppendBytes(
Makes me sad to see copies being.

http://codereview.chromium.org/3282003/diff/1/11#newcode151
webkit/blob/blob_storage_controller.cc:151: if (!blob_data)
Is this an error condition? At every site where !blob_data is found i have this
same quesion. Under what circumstance is this expected to be NULL? If the answer
is "only if we get bad inputs" from the child process how we deal with it could
be different than an actual expected case.

As written are we going to upload a data stream missing something that the
caller was expecting to be there... if so that looks like a bug.

http://codereview.chromium.org/3282003/diff/1/11#newcode162
webkit/blob/blob_storage_controller.cc:162: new_upload_data->AppendFileRange(
Should be fun to get temp files backing blobs lifetime right :)

Can you add a TODO(michaeln) here to ensure that any temp files survive the
URLRequest performing the upload.

http://codereview.chromium.org/3282003/diff/1/12
File webkit/blob/blob_storage_controller.h (right):

http://codereview.chromium.org/3282003/diff/1/12#newcode34
webkit/blob/blob_storage_controller.h:34: // blob references found, return the
passing UploadData.
What are the ownership semantics of the UploadData instances involved here?

http://codereview.chromium.org/3282003/diff/1/14
File webkit/blob/blob_url_request_job.cc (right):

http://codereview.chromium.org/3282003/diff/1/14#newcode144
webkit/blob/blob_url_request_job.cc:144: resolve_on_background = true;
A little simpler...

bool resolve_on_background = (file_path_list.size() > 1);

http://codereview.chromium.org/3282003/diff/1/14#newcode146
webkit/blob/blob_url_request_job.cc:146: #if defined(OS_WIN)
oh... some trickiness?

http://codereview.chromium.org/3282003/diff/1/14#newcode161
webkit/blob/blob_url_request_job.cc:161: // For single non-UNC file, do it now.
Is there compelling reason to have multiple code paths for this? A "mapped"
drive can still be on the network. Unless there is compelling reason, we should
probably start out with simpler code.

if (!file_path_list.empty()) {
  // fire up an async resolver
  return;
}

http://codereview.chromium.org/3282003/diff/1/14#newcode213
webkit/blob/blob_url_request_job.cc:213: NotifyFailure(net::ERR_FILE_NOT_FOUND);
i wonder if theres a better error code

http://codereview.chromium.org/3282003/diff/1/15
File webkit/blob/blob_url_request_job.h (right):

http://codereview.chromium.org/3282003/diff/1/15#newcode6
webkit/blob/blob_url_request_job.h:6: #define WEBKIT_BLOB_BLOB_URL_REQUEST_OB_H_
I think its nice that this is factored out of the chrome library so we can use
it in test_shell and DRT too.

http://codereview.chromium.org/3282003/diff/1/16
File webkit/blob/blob_url_request_job_unittest.cc (right):

http://codereview.chromium.org/3282003/diff/1/16#newcode34
webkit/blob/blob_url_request_job_unittest.cc:34: class BlobURLRequestJobTest :
public testing::Test {
very nice to see the test cases!

http://codereview.chromium.org/3282003/diff/27001/28010
File webkit/blob/blob_storage_controller.cc (right):

http://codereview.chromium.org/3282003/diff/27001/28010#newcode139
webkit/blob/blob_storage_controller.cc:139: case net::UploadData::TYPE_BYTES:
pains me to see copies :(

Could this method mutate the 'update_data' just replacing that one 'blob'
reference elements with representations of blob data items?

Hmmm... if BlobData and UploadData understood the same type of DataItem and they
were refcounted.

[darin (slow to review), 2010-08-28 06:08:46]

I know michael mentioned doing registration on the IO thread, but that's not
necessary if it is done early enough at startup, which is the case for all of
the other special URLRequest factories.

http://codereview.chromium.org/3282003/diff/1/2
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/3282003/diff/1/2#newcode581
chrome/browser/browser_init.cc:581:
URLRequest::RegisterProtocolFactory(chrome::kBlobScheme,
seems like this would better placed along side the call to
RegisterExtensionProtocols here:
https://cs.corp.google.com/p#chrome/trunk/src/chrome/browser/browser_main.cc&...

[jianli, 2010-08-28 06:55:42]

http://codereview.chromium.org/3282003/diff/1/2
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/3282003/diff/1/2#newcode582
chrome/browser/browser_init.cc:582: &BlobURLRequestJobFactory);
On 2010/08/28 00:55:14, michaeln wrote:
> Would be good to do this registration on the IO thread.

I think it is fine to register it from any thread. I moved it to browser_main.cc
where other URL handlers are registered.

http://codereview.chromium.org/3282003/diff/1/11
File webkit/blob/blob_storage_controller.cc (right):

http://codereview.chromium.org/3282003/diff/1/11#newcode102
webkit/blob/blob_storage_controller.cc:102: // It is OK that we cannot find the
blob corresponding to src_url.
On 2010/08/28 00:55:14, michaeln wrote:
> Under what circumstances would that be expected?

Yes, it is not really possible. Added DCHECK.

http://codereview.chromium.org/3282003/diff/1/11#newcode138
webkit/blob/blob_storage_controller.cc:138: new_upload_data->AppendBytes(
On 2010/08/28 00:55:14, michaeln wrote:
> Makes me sad to see copies being.

I agree. We need to figure out how to better represent BlobData and
UploadData/FormData. Added a TODO here.

http://codereview.chromium.org/3282003/diff/1/11#newcode151
webkit/blob/blob_storage_controller.cc:151: if (!blob_data)
On 2010/08/28 00:55:14, michaeln wrote:
> Is this an error condition? At every site where !blob_data is found i have
this
> same quesion. Under what circumstance is this expected to be NULL? If the
answer
> is "only if we get bad inputs" from the child process how we deal with it
could
> be different than an actual expected case.
> 
> As written are we going to upload a data stream missing something that the
> caller was expecting to be there... if so that looks like a bug.
> 

Added DCHECK here.

http://codereview.chromium.org/3282003/diff/1/11#newcode162
webkit/blob/blob_storage_controller.cc:162: new_upload_data->AppendFileRange(
On 2010/08/28 00:55:14, michaeln wrote:
> Should be fun to get temp files backing blobs lifetime right :)
> 
> Can you add a TODO(michaeln) here to ensure that any temp files survive the
> URLRequest performing the upload.

Done.

http://codereview.chromium.org/3282003/diff/1/12
File webkit/blob/blob_storage_controller.h (right):

http://codereview.chromium.org/3282003/diff/1/12#newcode34
webkit/blob/blob_storage_controller.h:34: // blob references found, return the
passing UploadData.
On 2010/08/28 00:55:14, michaeln wrote:
> What are the ownership semantics of the UploadData instances involved here?

Changed to update the elements in place.

http://codereview.chromium.org/3282003/diff/1/14
File webkit/blob/blob_url_request_job.cc (right):

http://codereview.chromium.org/3282003/diff/1/14#newcode144
webkit/blob/blob_url_request_job.cc:144: resolve_on_background = true;
On 2010/08/28 00:55:14, michaeln wrote:
> A little simpler...
> 
> bool resolve_on_background = (file_path_list.size() > 1);

Done.

http://codereview.chromium.org/3282003/diff/1/14#newcode146
webkit/blob/blob_url_request_job.cc:146: #if defined(OS_WIN)
On 2010/08/28 00:55:14, michaeln wrote:
> oh... some trickiness?

Removed.

http://codereview.chromium.org/3282003/diff/1/14#newcode161
webkit/blob/blob_url_request_job.cc:161: // For single non-UNC file, do it now.
On 2010/08/28 00:55:14, michaeln wrote:
> Is there compelling reason to have multiple code paths for this? A "mapped"
> drive can still be on the network. Unless there is compelling reason, we
should
> probably start out with simpler code.
> 
> if (!file_path_list.empty()) {
>   // fire up an async resolver
>   return;
> }

I am following the similar logic in URLRequestFileJob. I agree with your
suggestion. So I changed to do background resolving as long as there is a file
in the blob data.

http://codereview.chromium.org/3282003/diff/1/14#newcode213
webkit/blob/blob_url_request_job.cc:213: NotifyFailure(net::ERR_FILE_NOT_FOUND);
On 2010/08/28 00:55:14, michaeln wrote:
> i wonder if theres a better error code

Probably we need better error code. However, I think it does not matter because
eventually we will return HTTP 404 for file not found or this type of error, per
File API.

http://codereview.chromium.org/3282003/diff/1/15
File webkit/blob/blob_url_request_job.h (right):

http://codereview.chromium.org/3282003/diff/1/15#newcode6
webkit/blob/blob_url_request_job.h:6: #define WEBKIT_BLOB_BLOB_URL_REQUEST_OB_H_
On 2010/08/28 00:55:14, michaeln wrote:
> I think its nice that this is factored out of the chrome library so we can use
> it in test_shell and DRT too.

What do you mean? I think this class is used in both Chrome and test_shell/DRT.

http://codereview.chromium.org/3282003/diff/27001/28010
File webkit/blob/blob_storage_controller.cc (right):

http://codereview.chromium.org/3282003/diff/27001/28010#newcode139
webkit/blob/blob_storage_controller.cc:139: case net::UploadData::TYPE_BYTES:
On 2010/08/28 00:55:15, michaeln wrote:
> pains me to see copies :(
> 
> Could this method mutate the 'update_data' just replacing that one 'blob'
> reference elements with representations of blob data items?
> 
> Hmmm... if BlobData and UploadData understood the same type of DataItem and
they
> were refcounted.

Changed to update the elements in place.

Yes, it would be much better if BlobData and UploadData understood the same type
of DataItem and they're ref counted.

[jianli, 2010-08-29 03:35:16]

Updated the patch to use "#if 0/#endif" to temporarily disable
BlobStorageController usages in test_shell. This is because we need to
change
WebKit.gyp to include blob dependency and pull it downstream. To expedite
the
process, I use this as a workaround so that I can land this patch first.

On Fri, Aug 27, 2010 at 11:55 PM, <jianli@chromium.org> wrote:

>
> http://codereview.chromium.org/3282003/diff/1/2
> File chrome/browser/browser_init.cc (right):
>
> http://codereview.chromium.org/3282003/diff/1/2#newcode582
> chrome/browser/browser_init.cc:582: &BlobURLRequestJobFactory);
> On 2010/08/28 00:55:14, michaeln wrote:
>
>> Would be good to do this registration on the IO thread.
>>
>
> I think it is fine to register it from any thread. I moved it to
> browser_main.cc where other URL handlers are registered.
>
>
> http://codereview.chromium.org/3282003/diff/1/11
> File webkit/blob/blob_storage_controller.cc (right):
>
> http://codereview.chromium.org/3282003/diff/1/11#newcode102
> webkit/blob/blob_storage_controller.cc:102: // It is OK that we cannot
> find the blob corresponding to src_url.
> On 2010/08/28 00:55:14, michaeln wrote:
>
>> Under what circumstances would that be expected?
>>
>
> Yes, it is not really possible. Added DCHECK.
>
>
> http://codereview.chromium.org/3282003/diff/1/11#newcode138
> webkit/blob/blob_storage_controller.cc:138:
> new_upload_data->AppendBytes(
> On 2010/08/28 00:55:14, michaeln wrote:
>
>> Makes me sad to see copies being.
>>
>
> I agree. We need to figure out how to better represent BlobData and
> UploadData/FormData. Added a TODO here.
>
>
> http://codereview.chromium.org/3282003/diff/1/11#newcode151
> webkit/blob/blob_storage_controller.cc:151: if (!blob_data)
> On 2010/08/28 00:55:14, michaeln wrote:
>
>> Is this an error condition? At every site where !blob_data is found i
>>
> have this
>
>> same quesion. Under what circumstance is this expected to be NULL? If
>>
> the answer
>
>> is "only if we get bad inputs" from the child process how we deal with
>>
> it could
>
>> be different than an actual expected case.
>>
>
>  As written are we going to upload a data stream missing something that
>>
> the
>
>> caller was expecting to be there... if so that looks like a bug.
>>
>
>
> Added DCHECK here.
>
>
> http://codereview.chromium.org/3282003/diff/1/11#newcode162
> webkit/blob/blob_storage_controller.cc:162:
> new_upload_data->AppendFileRange(
> On 2010/08/28 00:55:14, michaeln wrote:
>
>> Should be fun to get temp files backing blobs lifetime right :)
>>
>
>  Can you add a TODO(michaeln) here to ensure that any temp files
>>
> survive the
>
>> URLRequest performing the upload.
>>
>
> Done.
>
>
> http://codereview.chromium.org/3282003/diff/1/12
> File webkit/blob/blob_storage_controller.h (right):
>
> http://codereview.chromium.org/3282003/diff/1/12#newcode34
> webkit/blob/blob_storage_controller.h:34: // blob references found,
> return the passing UploadData.
> On 2010/08/28 00:55:14, michaeln wrote:
>
>> What are the ownership semantics of the UploadData instances involved
>>
> here?
>
> Changed to update the elements in place.
>
>
> http://codereview.chromium.org/3282003/diff/1/14
> File webkit/blob/blob_url_request_job.cc (right):
>
> http://codereview.chromium.org/3282003/diff/1/14#newcode144
> webkit/blob/blob_url_request_job.cc:144: resolve_on_background = true;
> On 2010/08/28 00:55:14, michaeln wrote:
>
>> A little simpler...
>>
>
>  bool resolve_on_background = (file_path_list.size() > 1);
>>
>
> Done.
>
>
> http://codereview.chromium.org/3282003/diff/1/14#newcode146
> webkit/blob/blob_url_request_job.cc:146: #if defined(OS_WIN)
> On 2010/08/28 00:55:14, michaeln wrote:
>
>> oh... some trickiness?
>>
>
> Removed.
>
>
> http://codereview.chromium.org/3282003/diff/1/14#newcode161
> webkit/blob/blob_url_request_job.cc:161: // For single non-UNC file, do
> it now.
> On 2010/08/28 00:55:14, michaeln wrote:
>
>> Is there compelling reason to have multiple code paths for this? A
>>
> "mapped"
>
>> drive can still be on the network. Unless there is compelling reason,
>>
> we should
>
>> probably start out with simpler code.
>>
>
>  if (!file_path_list.empty()) {
>>   // fire up an async resolver
>>   return;
>> }
>>
>
> I am following the similar logic in URLRequestFileJob. I agree with your
> suggestion. So I changed to do background resolving as long as there is
> a file in the blob data.
>
>
> http://codereview.chromium.org/3282003/diff/1/14#newcode213
> webkit/blob/blob_url_request_job.cc:213:
> NotifyFailure(net::ERR_FILE_NOT_FOUND);
> On 2010/08/28 00:55:14, michaeln wrote:
>
>> i wonder if theres a better error code
>>
>
> Probably we need better error code. However, I think it does not matter
> because eventually we will return HTTP 404 for file not found or this
> type of error, per File API.
>
>
> http://codereview.chromium.org/3282003/diff/1/15
> File webkit/blob/blob_url_request_job.h (right):
>
> http://codereview.chromium.org/3282003/diff/1/15#newcode6
> webkit/blob/blob_url_request_job.h:6: #define
> WEBKIT_BLOB_BLOB_URL_REQUEST_OB_H_
> On 2010/08/28 00:55:14, michaeln wrote:
>
>> I think its nice that this is factored out of the chrome library so we
>>
> can use
>
>> it in test_shell and DRT too.
>>
>
> What do you mean? I think this class is used in both Chrome and
> test_shell/DRT.
>
>
> http://codereview.chromium.org/3282003/diff/27001/28010
> File webkit/blob/blob_storage_controller.cc (right):
>
> http://codereview.chromium.org/3282003/diff/27001/28010#newcode139
> webkit/blob/blob_storage_controller.cc:139: case
> net::UploadData::TYPE_BYTES:
> On 2010/08/28 00:55:15, michaeln wrote:
>
>> pains me to see copies :(
>>
>
>  Could this method mutate the 'update_data' just replacing that one
>>
> 'blob'
>
>> reference elements with representations of blob data items?
>>
>
>  Hmmm... if BlobData and UploadData understood the same type of
>>
> DataItem and they
>
>> were refcounted.
>>
>
> Changed to update the elements in place.
>
> Yes, it would be much better if BlobData and UploadData understood the
> same type of DataItem and they're ref counted.
>
>
> http://codereview.chromium.org/3282003/show
>

[michaeln, 2010-08-29 22:41:43]

http://codereview.chromium.org/3282003/diff/23003/7003
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/3282003/diff/23003/7003#newcode1341
chrome/browser/browser_main.cc:1341: RegisterBlobURLRequestHandler();
where the others are is good... thnx for moving this.

http://codereview.chromium.org/3282003/diff/23003/7005
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/3282003/diff/23003/7005#newcode72
chrome/browser/renderer_host/resource_dispatcher_host.cc:72: #include
"webkit/blob/blob_data.h"
needed?

http://codereview.chromium.org/3282003/diff/23003/7011
File net/base/upload_data.h (right):

http://codereview.chromium.org/3282003/diff/23003/7011#newcode100
net/base/upload_data.h:100: if (type_ == TYPE_BYTES) {
nit: no {}'s needed here

http://codereview.chromium.org/3282003/diff/23003/7011#newcode112
net/base/upload_data.h:112: return false;
is this return false reachable... if not, NOTREACHED?

http://codereview.chromium.org/3282003/diff/23003/7013
File webkit/blob/blob_storage_controller.cc (right):

http://codereview.chromium.org/3282003/diff/23003/7013#newcode135
webkit/blob/blob_storage_controller.cc:135: if (!blob_data) {
Maybe a TODO here about how to deal with this error?

http://codereview.chromium.org/3282003/diff/23003/7013#newcode153
webkit/blob/blob_storage_controller.cc:153: iter = uploads->insert(iter,
net::UploadData::Element());
Looks like this insert may cause additional copies of each element to be made to
shift elements down in the vector

Maybe insert <n> empty elements in one call before this loop
 uploads->insert(iter, n, Element());
and then populate the newly inserted elements within the loop?

I don't think its possible to avoid copies with our current data structures for
BlobData/Item and UploadData/Element.

http://codereview.chromium.org/3282003/diff/23003/7013#newcode173
webkit/blob/blob_storage_controller.cc:173: DCHECK(false);
would some kind of NOTREACHABLE be a better fit here?

http://codereview.chromium.org/3282003/diff/23003/7016
File webkit/blob/blob_url_request_job.cc (right):

http://codereview.chromium.org/3282003/diff/23003/7016#newcode54
webkit/blob/blob_url_request_job.cc:54: if
(!file_util::GetFileInfo(file_path_list_[i], &file_info)) {
This might be a good addition to FileUtilProxy, the ability to stat a file or an
array of files.

http://codereview.chromium.org/3282003/diff/23003/7016#newcode88
webkit/blob/blob_url_request_job.cc:88: MessageLoop* owner_loop_;
I think you could remove the lock if a MessageLoopProxy was used here instead lf
a MessageLoop ptr.

http://codereview.chromium.org/3282003/diff/23003/7016#newcode93
webkit/blob/blob_url_request_job.cc:93: /*static */
missing space in the comment, // static is maybe more common?

http://codereview.chromium.org/3282003/diff/23003/7016#newcode99
webkit/blob/blob_url_request_job.cc:99: if (request->method() == "GET") {
nit: don't need the {}'s

http://codereview.chromium.org/3282003/diff/23003/7016#newcode144
webkit/blob/blob_url_request_job.cc:144: WorkerPool::PostTask(FROM_HERE,
NewRunnableMethod(
It might be good to arrange to use the FILE thread in chrome for this (and to do
this directly on the IO thread in test_shell).

Here's a comment from WorkerPool.h

// WARNING: This shouldn't be used unless absolutely necessary. We don't wait
// for the worker pool threads to finish on shutdown, so the tasks running
// inside the pool must be extremely careful about other objects they access
// (MessageLoops, Singletons, etc). During shutdown these object may no longer
// exist.

http://codereview.chromium.org/3282003/diff/23003/7016#newcode263
webkit/blob/blob_url_request_job.cc:263: // Keep track of the buffer.
do we need to addref 'dest' here?

http://codereview.chromium.org/3282003/diff/23003/7016#newcode382
webkit/blob/blob_url_request_job.cc:382: SetStatus(URLRequestStatus());  //
Clear the IO_PENDING status
if result == 0, do we also need to clear the IO_PENDING status?

http://codereview.chromium.org/3282003/diff/23003/7017
File webkit/blob/blob_url_request_job.h (right):

http://codereview.chromium.org/3282003/diff/23003/7017#newcode45
webkit/blob/blob_url_request_job.h:45: void Seek(int64 offset);
This logic to asyncly read a stream of bytes out of a 'blob' is going to want to
be reused in at least 2 (ideally 3) places.
1) urlRequests
2) fileWriter
3) (ideally) uploadData

Feels like the structures to represent composite 'data', and the logic to
asyncly read it into an ioBuffer ultimately belong in the net/io library.

http://codereview.chromium.org/3282003/diff/47002/7049
File webkit/blob/blob_url_request_job_unittest.cc (right):

http://codereview.chromium.org/3282003/diff/47002/7049#newcode245
webkit/blob/blob_url_request_job_unittest.cc:245: this,
&BlobURLRequestJobTest::VerifyErrorResponse));
It looks like TestSuccessRequest will go thru this code path since we're
expecting a status code of 200. Its not clear that the expected_response is
actually being examined.

[jianli, 2010-08-30 01:30:17]

http://codereview.chromium.org/3282003/diff/23003/7005
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/3282003/diff/23003/7005#newcode72
chrome/browser/renderer_host/resource_dispatcher_host.cc:72: #include
"webkit/blob/blob_data.h"
On 2010/08/29 22:41:43, michaeln wrote:
> needed?

Removed.

http://codereview.chromium.org/3282003/diff/23003/7011
File net/base/upload_data.h (right):

http://codereview.chromium.org/3282003/diff/23003/7011#newcode100
net/base/upload_data.h:100: if (type_ == TYPE_BYTES) {
On 2010/08/29 22:41:43, michaeln wrote:
> nit: no {}'s needed here

Done.

http://codereview.chromium.org/3282003/diff/23003/7011#newcode112
net/base/upload_data.h:112: return false;
On 2010/08/29 22:41:43, michaeln wrote:
> is this return false reachable... if not, NOTREACHED?

It is not reachable. However, I want to avoid include ASSERT in header file.
Also since this is only used in unittest and returning false should be enough to
fail the test, I think probably we do not want to add the check.

http://codereview.chromium.org/3282003/diff/23003/7013
File webkit/blob/blob_storage_controller.cc (right):

http://codereview.chromium.org/3282003/diff/23003/7013#newcode135
webkit/blob/blob_storage_controller.cc:135: if (!blob_data) {
On 2010/08/29 22:41:43, michaeln wrote:
> Maybe a TODO here about how to deal with this error?

Done.

http://codereview.chromium.org/3282003/diff/23003/7013#newcode153
webkit/blob/blob_storage_controller.cc:153: iter = uploads->insert(iter,
net::UploadData::Element());
On 2010/08/29 22:41:43, michaeln wrote:
> Looks like this insert may cause additional copies of each element to be made
to
> shift elements down in the vector
> 
> Maybe insert <n> empty elements in one call before this loop
>  uploads->insert(iter, n, Element());
> and then populate the newly inserted elements within the loop?
> 
> I don't think its possible to avoid copies with our current data structures
for
> BlobData/Item and UploadData/Element.

Yes, I agree that it is not possible to avoid copies.

As to your suggestions, I think it is a bit complicated to apply it. This is
because vector<>::insert(, n, ) does not return any value. So if we want to use
it, we will have to iterate from the beginning again.

http://codereview.chromium.org/3282003/diff/23003/7013#newcode173
webkit/blob/blob_storage_controller.cc:173: DCHECK(false);
On 2010/08/29 22:41:43, michaeln wrote:
> would some kind of NOTREACHABLE be a better fit here?

Done.

http://codereview.chromium.org/3282003/diff/23003/7016
File webkit/blob/blob_url_request_job.cc (right):

http://codereview.chromium.org/3282003/diff/23003/7016#newcode54
webkit/blob/blob_url_request_job.cc:54: if
(!file_util::GetFileInfo(file_path_list_[i], &file_info)) {
On 2010/08/29 22:41:43, michaeln wrote:
> This might be a good addition to FileUtilProxy, the ability to stat a file or
an
> array of files.

Changed to use FileUtilProxy that runs on FILE thread for chrome or IO thread
for test.

http://codereview.chromium.org/3282003/diff/23003/7016#newcode88
webkit/blob/blob_url_request_job.cc:88: MessageLoop* owner_loop_;
On 2010/08/29 22:41:43, michaeln wrote:
> I think you could remove the lock if a MessageLoopProxy was used here instead
lf
> a MessageLoop ptr.

Changed to use FileUtilProxy that runs on FILE thread for chrome or IO thread
for test.

http://codereview.chromium.org/3282003/diff/23003/7016#newcode93
webkit/blob/blob_url_request_job.cc:93: /*static */
On 2010/08/29 22:41:43, michaeln wrote:
> missing space in the comment, // static is maybe more common?

Done.

http://codereview.chromium.org/3282003/diff/23003/7016#newcode99
webkit/blob/blob_url_request_job.cc:99: if (request->method() == "GET") {
On 2010/08/29 22:41:43, michaeln wrote:
> nit: don't need the {}'s

But I got the impression {} is also needed if the statement across more than one
line.

http://codereview.chromium.org/3282003/diff/23003/7016#newcode144
webkit/blob/blob_url_request_job.cc:144: WorkerPool::PostTask(FROM_HERE,
NewRunnableMethod(
On 2010/08/29 22:41:43, michaeln wrote:
> It might be good to arrange to use the FILE thread in chrome for this (and to
do
> this directly on the IO thread in test_shell).
> 
> Here's a comment from WorkerPool.h
> 
> // WARNING: This shouldn't be used unless absolutely necessary. We don't wait
> // for the worker pool threads to finish on shutdown, so the tasks running
> // inside the pool must be extremely careful about other objects they access
> // (MessageLoops, Singletons, etc). During shutdown these object may no longer
> // exist.

Changed to use FileUtilProxy that runs on FILE thread for chrome or IO thread
for test.

http://codereview.chromium.org/3282003/diff/23003/7016#newcode263
webkit/blob/blob_url_request_job.cc:263: // Keep track of the buffer.
On 2010/08/29 22:41:43, michaeln wrote:
> do we need to addref 'dest' here?

Done.

http://codereview.chromium.org/3282003/diff/23003/7016#newcode382
webkit/blob/blob_url_request_job.cc:382: SetStatus(URLRequestStatus());  //
Clear the IO_PENDING status
On 2010/08/29 22:41:43, michaeln wrote:
> if result == 0, do we also need to clear the IO_PENDING status?

Done.

http://codereview.chromium.org/3282003/diff/47002/7049
File webkit/blob/blob_url_request_job_unittest.cc (right):

http://codereview.chromium.org/3282003/diff/47002/7049#newcode245
webkit/blob/blob_url_request_job_unittest.cc:245: this,
&BlobURLRequestJobTest::VerifyErrorResponse));
On 2010/08/29 22:41:43, michaeln wrote:
> It looks like TestSuccessRequest will go thru this code path since we're
> expecting a status code of 200. Its not clear that the expected_response is
> actually being examined.

Fixed.

[darin (slow to review), 2010-08-30 06:41:57]

overall, looks great to me.

http://codereview.chromium.org/3282003/diff/19004/48002
File base/file_util_proxy.cc (right):

http://codereview.chromium.org/3282003/diff/19004/48002#newcode208
base/file_util_proxy.cc:208: class RelayGetFileListInfo : public
MessageLoopRelay {
this anonymous namespace should really just be within the base namespace so that
all of the base:: prefixes would be unnecessary.  this can be part of a
different CL since the issue already existed.

http://codereview.chromium.org/3282003/diff/19004/48003
File base/file_util_proxy.h (right):

http://codereview.chromium.org/3282003/diff/19004/48003#newcode65
base/file_util_proxy.h:65: typedef Callback2<bool /*exists*/,
i think this makes for an awkward low-level API.  the exists parameter if false
really indicates that the results vector is truncated.  that is reasonable for
your usage, but in the general case, i can imagine someone wanting to skip past
non-existent files and keep going.  at this level, it feels like we should just
have a GetFileInfo that takes a single path and returns a single result.  i
realize that would complicate your consumer code a bit, but it seems like the
right low level method.

http://codereview.chromium.org/3282003/diff/19004/48004
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/3282003/diff/19004/48004#newcode172
chrome/browser/browser_main.cc:172: return
ChromeThread::GetMessageLoopProxyForThread(ChromeThread::FILE);
this function returns a constant value, so perhaps you should just pass a
reference to the FILE thread's MessageLoopProxy to the Factory method below? 
then you wouldn't need to define BlobURLRequestJobClient.

http://codereview.chromium.org/3282003/diff/19004/48004#newcode187
chrome/browser/browser_main.cc:187: void RegisterBlobURLRequestHandler() {
browser_main.cc is already big enough.  i'd prefer to see this small bit of code
hoisted out into its own .h and .cc files. 
chrome/browser/net/blob_url_request_job_factory.{h,cc} seems good.

it'd probably be good for the function to be named similarly: 
RegisterBlobURLRequestJobFactory

http://codereview.chromium.org/3282003/diff/19004/48012
File net/base/upload_data.h (right):

http://codereview.chromium.org/3282003/diff/19004/48012#newcode97
net/base/upload_data.h:97: bool operator==(const Element& other) const {
nit: it is usually better to define operator== and operator!= at global scope. 
that is just a good convention so that C++ type coercion will work properly for
both lhs and rhs operands.

http://codereview.chromium.org/3282003/diff/19004/48017
File webkit/blob/blob_url_request_job.cc (right):

http://codereview.chromium.org/3282003/diff/19004/48017#newcode102
webkit/blob/blob_url_request_job.cc:102: message_loop_proxy =
base::MessageLoopProxy::CreateForCurrentThread();
it seems like you meant to null check message_loop_proxy here before overwriting
it.

http://codereview.chromium.org/3282003/diff/19004/48017#newcode516
webkit/blob/blob_url_request_job.cc:516:
NotifyFailure(net::ERR_REQUEST_RANGE_NOT_SATISFIABLE);
this shouldn't cause too much trouble.  some pepper plugins might try to perform
multipart byte range requests though.  please add a TODO here and be sure to
file a bug.

[jianli, 2010-08-30 18:17:11]

http://codereview.chromium.org/3282003/diff/19004/48002
File base/file_util_proxy.cc (right):

http://codereview.chromium.org/3282003/diff/19004/48002#newcode208
base/file_util_proxy.cc:208: class RelayGetFileListInfo : public
MessageLoopRelay {
On 2010/08/30 06:41:58, darin wrote:
> this anonymous namespace should really just be within the base namespace so
that
> all of the base:: prefixes would be unnecessary.  this can be part of a
> different CL since the issue already existed.

Added TODO to clean it up.

http://codereview.chromium.org/3282003/diff/19004/48003
File base/file_util_proxy.h (right):

http://codereview.chromium.org/3282003/diff/19004/48003#newcode65
base/file_util_proxy.h:65: typedef Callback2<bool /*exists*/,
On 2010/08/30 06:41:58, darin wrote:
> i think this makes for an awkward low-level API.  the exists parameter if
false
> really indicates that the results vector is truncated.  that is reasonable for
> your usage, but in the general case, i can imagine someone wanting to skip
past
> non-existent files and keep going.  at this level, it feels like we should
just
> have a GetFileInfo that takes a single path and returns a single result.  i
> realize that would complicate your consumer code a bit, but it seems like the
> right low level method.

Done.

http://codereview.chromium.org/3282003/diff/19004/48004
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/3282003/diff/19004/48004#newcode172
chrome/browser/browser_main.cc:172: return
ChromeThread::GetMessageLoopProxyForThread(ChromeThread::FILE);
On 2010/08/30 06:41:58, darin wrote:
> this function returns a constant value, so perhaps you should just pass a
> reference to the FILE thread's MessageLoopProxy to the Factory method below? 
> then you wouldn't need to define BlobURLRequestJobClient.

Done.

http://codereview.chromium.org/3282003/diff/19004/48004#newcode187
chrome/browser/browser_main.cc:187: void RegisterBlobURLRequestHandler() {
On 2010/08/30 06:41:58, darin wrote:
> browser_main.cc is already big enough.  i'd prefer to see this small bit of
code
> hoisted out into its own .h and .cc files. 
> chrome/browser/net/blob_url_request_job_factory.{h,cc} seems good.
> 
> it'd probably be good for the function to be named similarly: 
> RegisterBlobURLRequestJobFactory

Done.

http://codereview.chromium.org/3282003/diff/19004/48012
File net/base/upload_data.h (right):

http://codereview.chromium.org/3282003/diff/19004/48012#newcode97
net/base/upload_data.h:97: bool operator==(const Element& other) const {
On 2010/08/30 06:41:58, darin wrote:
> nit: it is usually better to define operator== and operator!= at global scope.

> that is just a good convention so that C++ type coercion will work properly
for
> both lhs and rhs operands.

Done.

http://codereview.chromium.org/3282003/diff/19004/48017
File webkit/blob/blob_url_request_job.cc (right):

http://codereview.chromium.org/3282003/diff/19004/48017#newcode102
webkit/blob/blob_url_request_job.cc:102: message_loop_proxy =
base::MessageLoopProxy::CreateForCurrentThread();
On 2010/08/30 06:41:58, darin wrote:
> it seems like you meant to null check message_loop_proxy here before
overwriting
> it.

Changed to do the resolving in current thread directly if message_loop_proxy is
not provided, as in test shell or unittest.

http://codereview.chromium.org/3282003/diff/19004/48017#newcode516
webkit/blob/blob_url_request_job.cc:516:
NotifyFailure(net::ERR_REQUEST_RANGE_NOT_SATISFIABLE);
On 2010/08/30 06:41:58, darin wrote:
> this shouldn't cause too much trouble.  some pepper plugins might try to
perform
> multipart byte range requests though.  please add a TODO here and be sure to
> file a bug.

Done.

[Paweł Hajdan Jr., 2010-08-30 18:18:40]

Drive-by with some test comments. Please let me take another look at the
ScopedTempDir bits before you commit.

http://codereview.chromium.org/3282003/diff/19004/48016
File webkit/blob/blob_storage_controller_unittest.cc (right):

http://codereview.chromium.org/3282003/diff/19004/48016#newcode136
webkit/blob/blob_storage_controller_unittest.cc:136:
ASSERT_EQ(upload_data->elements()->size(), static_cast<size_t>(2));
nit: Just 2U instead of the static_cast. Please fix all occurrences in this
file.

http://codereview.chromium.org/3282003/diff/19004/48019
File webkit/blob/blob_url_request_job_unittest.cc (right):

http://codereview.chromium.org/3282003/diff/19004/48019#newcode127
webkit/blob/blob_url_request_job_unittest.cc:127:
file_util::CreateTemporaryFile(&temp_file1_);
Please use ScopedTempDir.

[jianli, 2010-08-30 18:39:09]

http://codereview.chromium.org/3282003/diff/19004/48016
File webkit/blob/blob_storage_controller_unittest.cc (right):

http://codereview.chromium.org/3282003/diff/19004/48016#newcode136
webkit/blob/blob_storage_controller_unittest.cc:136:
ASSERT_EQ(upload_data->elements()->size(), static_cast<size_t>(2));
On 2010/08/30 18:18:40, Paweł Hajdan Jr. wrote:
> nit: Just 2U instead of the static_cast. Please fix all occurrences in this
> file.

Done.

http://codereview.chromium.org/3282003/diff/19004/48019
File webkit/blob/blob_url_request_job_unittest.cc (right):

http://codereview.chromium.org/3282003/diff/19004/48019#newcode127
webkit/blob/blob_url_request_job_unittest.cc:127:
file_util::CreateTemporaryFile(&temp_file1_);
On 2010/08/30 18:18:40, Paweł Hajdan Jr. wrote:
> Please use ScopedTempDir.

Done.

[Paweł Hajdan Jr., 2010-08-30 18:42:15]

Please note that gtest creates a fresh test fixture for each test.

http://codereview.chromium.org/3282003/diff/32005/51026
File webkit/blob/blob_url_request_job_unittest.cc (right):

http://codereview.chromium.org/3282003/diff/32005/51026#newcode128
webkit/blob/blob_url_request_job_unittest.cc:128: temp_dir_.reset(new
ScopedTempDir());
Why not just stack-allocated?

http://codereview.chromium.org/3282003/diff/32005/51026#newcode152
webkit/blob/blob_url_request_job_unittest.cc:152: temp_dir_.reset(NULL);
This shouldn't be needed.

[jianli, 2010-08-30 18:59:18]

http://codereview.chromium.org/3282003/diff/32005/51026
File webkit/blob/blob_url_request_job_unittest.cc (right):

http://codereview.chromium.org/3282003/diff/32005/51026#newcode128
webkit/blob/blob_url_request_job_unittest.cc:128: temp_dir_.reset(new
ScopedTempDir());
On 2010/08/30 18:42:16, Paweł Hajdan Jr. wrote:
> Why not just stack-allocated?

Just want to make sure that temp_dir_ is only created when gtest starts to set
up test case and it is more deterministic.

http://codereview.chromium.org/3282003/diff/32005/51026#newcode152
webkit/blob/blob_url_request_job_unittest.cc:152: temp_dir_.reset(NULL);
On 2010/08/30 18:42:16, Paweł Hajdan Jr. wrote:
> This shouldn't be needed.

Just want to make sure that temp_dir_ can be destructed right in
TearDownTestCase(), instead of some time later.

[Paweł Hajdan Jr., 2010-08-30 20:22:39]

Oh, I have just noticed it re-uses Thread and ScopedTempDir between tests.
Please don't do that, we should avoid one test possibly interfering with another
this way. Especially re-using Thread is very suspicious.

[michaeln, 2010-08-30 22:50:38]

http://codereview.chromium.org/3282003/diff/32005/51007
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/3282003/diff/32005/51007#newcode1325
chrome/browser/browser_main.cc:1325: 
extra blank line got inserted here

http://codereview.chromium.org/3282003/diff/32005/51009
File chrome/browser/net/blob_url_request_job_factory.cc (right):

http://codereview.chromium.org/3282003/diff/32005/51009#newcode17
chrome/browser/net/blob_url_request_job_factory.cc:17: return
webkit_blob::BlobURLRequestJob::Factory(
Maybe just call new webkit_blob::BlobURLRequestJob() directly here since this is
the function that is registered as the factory method.

http://codereview.chromium.org/3282003/diff/32005/51021
File webkit/blob/blob_storage_controller.cc (right):

http://codereview.chromium.org/3282003/diff/32005/51021#newcode79
webkit/blob/blob_storage_controller.cc:79: } case BlobData::TYPE_FILE:
new a newline here

http://codereview.chromium.org/3282003/diff/32005/51024
File webkit/blob/blob_url_request_job.cc (right):

http://codereview.chromium.org/3282003/diff/32005/51024#newcode45
webkit/blob/blob_url_request_job.cc:45: base::MessageLoopProxy*
resolving_message_loop_proxy) {
Can you call this the 'file_thread_proxy_' for clarity.

http://codereview.chromium.org/3282003/diff/32005/51024#newcode50
webkit/blob/blob_url_request_job.cc:50:
blob_storage_controller->GetBlobDataFromUrl(request->url()),
The only dependency this class has on the 'controller' is this call to lookup
the blob given the url, consider having the caller perform that function and
pass in the BlobData instead as is done in the ctor.

http://codereview.chromium.org/3282003/diff/32005/51024#newcode53
webkit/blob/blob_url_request_job.cc:53: return new URLRequestErrorJob(request,
net::ERR_METHOD_NOT_SUPPORTED);
Oh... i see you have this runtime check here... the Start() method could also
return an error response for non supported methods, what about HEAD requests.

I still think you could nix this Factory() method and just use the ctro at
callsites to avoid a little confusion... this method can't be registered with
network lib as a THE factory since it has the wrong signature.

http://codereview.chromium.org/3282003/diff/32005/51024#newcode113
webkit/blob/blob_url_request_job.cc:113: // we run the unittest or test shell.
Might be good to always have the same code path. Instead of allowing a NULL mlp
have test_shell pass in a proxy for the 'current' thread.

http://codereview.chromium.org/3282003/diff/32005/51026
File webkit/blob/blob_url_request_job_unittest.cc (right):

http://codereview.chromium.org/3282003/diff/32005/51026#newcode63
webkit/blob/blob_url_request_job_unittest.cc:63: ReadSome(request);
I think you've got an extra call to ReadSome in here since ReceiveData() will
also call ReadSome() in this case (which will make an extra call to
ScheduleNextTask())

http://codereview.chromium.org/3282003/diff/32005/51026#newcode162
webkit/blob/blob_url_request_job_unittest.cc:162: return new
URLRequestErrorJob(request, net::ERR_INTERNET_DISCONNECTED);
I don't think any of your tests use this code path. If that's right, please just
ASSERT(blob_url_request_job_) instead, having this code path here makes me
think... "oh, in some cases maybe its supposed to be NULL".

[jianli, 2010-08-30 23:30:51]

http://codereview.chromium.org/3282003/diff/32005/51007
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/3282003/diff/32005/51007#newcode1325
chrome/browser/browser_main.cc:1325: 
On 2010/08/30 22:50:38, michaeln wrote:
> extra blank line got inserted here

Done.

http://codereview.chromium.org/3282003/diff/32005/51009
File chrome/browser/net/blob_url_request_job_factory.cc (right):

http://codereview.chromium.org/3282003/diff/32005/51009#newcode17
chrome/browser/net/blob_url_request_job_factory.cc:17: return
webkit_blob::BlobURLRequestJob::Factory(
On 2010/08/30 22:50:38, michaeln wrote:
> Maybe just call new webkit_blob::BlobURLRequestJob() directly here since this
is
> the function that is registered as the factory method.

Done.

http://codereview.chromium.org/3282003/diff/32005/51021
File webkit/blob/blob_storage_controller.cc (right):

http://codereview.chromium.org/3282003/diff/32005/51021#newcode79
webkit/blob/blob_storage_controller.cc:79: } case BlobData::TYPE_FILE:
On 2010/08/30 22:50:38, michaeln wrote:
> new a newline here

Done.

http://codereview.chromium.org/3282003/diff/32005/51024
File webkit/blob/blob_url_request_job.cc (right):

http://codereview.chromium.org/3282003/diff/32005/51024#newcode45
webkit/blob/blob_url_request_job.cc:45: base::MessageLoopProxy*
resolving_message_loop_proxy) {
On 2010/08/30 22:50:38, michaeln wrote:
> Can you call this the 'file_thread_proxy_' for clarity.

Done.

http://codereview.chromium.org/3282003/diff/32005/51024#newcode50
webkit/blob/blob_url_request_job.cc:50:
blob_storage_controller->GetBlobDataFromUrl(request->url()),
On 2010/08/30 22:50:38, michaeln wrote:
> The only dependency this class has on the 'controller' is this call to lookup
> the blob given the url, consider having the caller perform that function and
> pass in the BlobData instead as is done in the ctor.

Done.

http://codereview.chromium.org/3282003/diff/32005/51024#newcode53
webkit/blob/blob_url_request_job.cc:53: return new URLRequestErrorJob(request,
net::ERR_METHOD_NOT_SUPPORTED);
On 2010/08/30 22:50:38, michaeln wrote:
> Oh... i see you have this runtime check here... the Start() method could also
> return an error response for non supported methods, what about HEAD requests.
> 
> I still think you could nix this Factory() method and just use the ctro at
> callsites to avoid a little confusion... this method can't be registered with
> network lib as a THE factory since it has the wrong signature.

Done.

http://codereview.chromium.org/3282003/diff/32005/51024#newcode113
webkit/blob/blob_url_request_job.cc:113: // we run the unittest or test shell.
On 2010/08/30 22:50:38, michaeln wrote:
> Might be good to always have the same code path. Instead of allowing a NULL
mlp
> have test_shell pass in a proxy for the 'current' thread.

I got some wield errors when I use the proxy of current thread for unittest. I
think it has something to do with how IO thread is setup in blob url unittest. I
adds a TODO here and will investigate the cause.

http://codereview.chromium.org/3282003/diff/32005/51026
File webkit/blob/blob_url_request_job_unittest.cc (right):

http://codereview.chromium.org/3282003/diff/32005/51026#newcode63
webkit/blob/blob_url_request_job_unittest.cc:63: ReadSome(request);
On 2010/08/30 22:50:38, michaeln wrote:
> I think you've got an extra call to ReadSome in here since ReceiveData() will
> also call ReadSome() in this case (which will make an extra call to
> ScheduleNextTask())

Done.

http://codereview.chromium.org/3282003/diff/32005/51026#newcode162
webkit/blob/blob_url_request_job_unittest.cc:162: return new
URLRequestErrorJob(request, net::ERR_INTERNET_DISCONNECTED);
On 2010/08/30 22:50:38, michaeln wrote:
> I don't think any of your tests use this code path. If that's right, please
just
> ASSERT(blob_url_request_job_) instead, having this code path here makes me
> think... "oh, in some cases maybe its supposed to be NULL".

Done.

[michaeln, 2010-08-31 01:11:38]

> I got some wield errors when I use the proxy of current thread for unittest. I
> think it has something to do with how IO thread is setup in blob url unittest.
I
> adds a TODO here and will investigate the cause.

Any idea whats up with that? Could it have been related to multiple calls to
'done'?

[jianli, 2010-08-31 01:16:22]

It could be, but I can't repro this locally. I only saw this in try jobs. I
hate to submit try jobs and take several hours to verify and investigate.

Could I keep what currently it is now so that I can land it to unblocking
landing WebKit patches? Then I will work on this. Thanks.

On Mon, Aug 30, 2010 at 6:11 PM, <michaeln@chromium.org> wrote:

> I got some wield errors when I use the proxy of current thread for
>> unittest. I
>> think it has something to do with how IO thread is setup in blob url
>> unittest.
>>
> I
>
>> adds a TODO here and will investigate the cause.
>>
>
> Any idea whats up with that? Could it have been related to multiple calls
> to
> 'done'?
>
>
> http://codereview.chromium.org/3282003/show
>

[michaeln, 2010-08-31 01:27:22]

Lgtm

I can tell we're going to want to refactor the BlobUrlRequestJob class so the
logic that delivers a blob as a stream of bytes can be reused. And also probably
want to revisit the BlobData structures to make it easier to manage slicing and
such... but this is a great start at it... thanx jian!

I don't know if darin what's to take another look or not, please ping him.

http://codereview.chromium.org/3282003/diff/13005/1060
File webkit/blob/blob_url_request_job.cc (right):

http://codereview.chromium.org/3282003/diff/13005/1060#newcode348
webkit/blob/blob_url_request_job.cc:348: }
i think this can be simplified as...

 if (result < 0) {
   NotifyFailure(err);
   return;
 }
 SetStatus(...);  // Clear IO pending status

http://codereview.chromium.org/3282003/diff/13005/1073
File webkit/tools/test_shell/test_shell_webblobregistry_impl.cc (right):

http://codereview.chromium.org/3282003/diff/13005/1073#newcode75
webkit/tools/test_shell/test_shell_webblobregistry_impl.cc:75: const GURL& url,
webkit_blob::BlobData* blob_data) {
If the blob_data param was a scoped_refptr<BlobData>, would you need to do the
manual refcounting?

http://codereview.chromium.org/3282003/diff/13005/1076
File webkit/tools/test_shell/test_shell_webkit_init.h (right):

http://codereview.chromium.org/3282003/diff/13005/1076#newcode149
webkit/tools/test_shell/test_shell_webkit_init.h:149:
scoped_refptr<TestShellWebBlobRegistryImpl> web_blob_registry_;
just blob_registry_ no need for the "web" prefix

[Paweł Hajdan Jr., 2010-08-31 03:39:34]

Why has this change been committed without proper review?

I have raised concerns about re-using Thread and ScopedTempDir for multiple
tests, and discussed it on IM with you, repeating that you should not do that.

[jianli, 2010-08-31 03:42:31]

Yes, we have a discussion. But I think it is not a wrong way to do things.
If this issue is discussed and the conclusion is reached in a wide scope, I
am willing to make a change. Thanks.

On Mon, Aug 30, 2010 at 8:39 PM, <phajdan.jr@chromium.org> wrote:

> Why has this change been committed without proper review?
>
> I have raised concerns about re-using Thread and ScopedTempDir for multiple
> tests, and discussed it on IM with you, repeating that you should not do
> that.
>
>
> http://codereview.chromium.org/3282003/show
>

[michaeln, 2010-08-31 05:46:53]

I'm sorry, I meant to comment on that. Jian's reusing a technique from other
test harnesses that spin up a thread for a suite of tests. Reusing a thread in
an of itself shouldn't be an issue, after all, most tests are all run on the
same thread. At some point it might be nice to actually reuse the harness code
too, so keeping the harness behaving this way for these tests i think is a good
thing.

The data in the scoped_dir are inputs to  some of the test cases, the tests
don't produce outputs in that directory, just the harness drops some inputs in
there which are used multiple times. I didn't see any issue with that. And on
the plus side, the tests run faster.


On 2010/08/31 03:42:31, jianli wrote:
> Yes, we have a discussion. But I think it is not a wrong way to do things.
> If this issue is discussed and the conclusion is reached in a wide scope, I
> am willing to make a change. Thanks.
> 
> On Mon, Aug 30, 2010 at 8:39 PM, <mailto:phajdan.jr@chromium.org> wrote:
> 
> > Why has this change been committed without proper review?
> >
> > I have raised concerns about re-using Thread and ScopedTempDir for multiple
> > tests, and discussed it on IM with you, repeating that you should not do
> > that.

[michaeln, 2010-08-31 06:02:15]

Oh... i meant to publish this comment but apparently didn't :(

I've cc'd Kavita so she can reconcile the difference.

http://codereview.chromium.org/3282003/diff/13005/1042
File base/file_util_proxy.h (right):

http://codereview.chromium.org/3282003/diff/13005/1042#newcode67
base/file_util_proxy.h:67: static bool GetFileInfo(
Oh... it looks like kavita is adding a very similar method in this CL...
http://codereview.chromium.org/3212002/diff/37001/38002?context=50&column_wid...

The only difference being the first param to the callback...

  61   typedef Callback2<int /* error code */,
  62       file_util::FileInfo>::Type GetFileInfoCallback;
  63   static bool GetFileInfo(scoped_refptr<MessageLoopProxy>
message_loop_proxy,
  64                           const FilePath& file_path,
  65                           GetFileInfoCallback* callback);

Can you update your CL to match this sig instead.