Support sending a sliced file in chromium....

Support sending a sliced file in chromium.

BUG=none
TEST=The WebKit Layout test.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=42559

[darin (slow to review), 2010-02-12 21:54:26]

http://codereview.chromium.org/594036/diff/4001/4021
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/594036/diff/4001/4021#newcode1326
chrome/browser/renderer_host/resource_message_filter.cc:1326: if
(file_util::GetFileInfo(path, &info))
file_util::FileInfo contains both size and last_modified time stamps.  this
means that your code here for proxying to the FILE thread could be reused to
implement OnGetFileSize.  maybe it would be better to share code instead.

file_util::GetFileSize just calls GetFileInfo under the covers.

maybe you could pass the address of the "WriteReplyParams" function to use.

http://codereview.chromium.org/594036/diff/4001/4026
File chrome/renderer/renderer_webkitclient_impl.cc (right):

http://codereview.chromium.org/594036/diff/4001/4026#newcode102
chrome/renderer/renderer_webkitclient_impl.cc:102: result =
static_cast<time_t>(modification_time);
The WebKitClient::getFileModificationTime should really be returning double
instead of time_t since "double seconds" is what webkit normally uses for a
timestamp.

http://codereview.chromium.org/594036/diff/4001/4012
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/594036/diff/4001/4012#newcode16
net/base/upload_data_stream.cc:16: if (stream->FillBuf())
been hacking webkit too much :)  in chromium code, if you use brackets on one
arm of the if/else, then use brackets on both.

however, in this case the "else" after "return" should just be deleted.

http://codereview.chromium.org/594036/diff/4001/4012#newcode86
net/base/upload_data_stream.cc:86: if
(file_util::GetFileInfo(element.file_path(), &info) &&
this is a synchronous file stat.  can we at least query this information from
next_element_stream_ so that we can use the open file descriptor instead of the
file path?

actually, instead of querying the information from FileStream, it would be
better to open the file yourself, stat it, and then pass the file descriptor to
FileStream.  I think you should wait for this CL to land: 
http://codereview.chromium.org/541022

It has all of those API changes, and should simplify this code.

http://codereview.chromium.org/594036/diff/4001/4016
File net/url_request/url_request.h (right):

http://codereview.chromium.org/594036/diff/4001/4016#newcode307
net/url_request/url_request.h:307: double modification_time);
in chromium code we prefer using base::Time for timestamps.  please convert to
base::Time at the WebKit <-> Chromium boundary.

also it would be good to document this parameter.  it is intended to be used as
a validator for the given file.

http://codereview.chromium.org/594036/diff/4001/4004
File webkit/glue/glue_serialize.cc (right):

http://codereview.chromium.org/594036/diff/4001/4004#newcode262
webkit/glue/glue_serialize.cc:262: WebString filePath = ReadString(obj);
please use google style for variable names.

http://codereview.chromium.org/594036/diff/4001/4005
File webkit/glue/mock_resource_loader_bridge.h (right):

http://codereview.chromium.org/594036/diff/4001/4005#newcode27
webkit/glue/mock_resource_loader_bridge.h:27: double modification_time));
expected_modification_time might be a good name for this parameter in general
since "expected" helps convey the intent of this being a validator.

[jianli, 2010-03-02 19:28:39]

http://codereview.chromium.org/594036/diff/4001/4021
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/594036/diff/4001/4021#newcode1326
chrome/browser/renderer_host/resource_message_filter.cc:1326: if
(file_util::GetFileInfo(path, &info))
On 2010/02/12 21:54:26, darin wrote:
> file_util::FileInfo contains both size and last_modified time stamps.  this
> means that your code here for proxying to the FILE thread could be reused to
> implement OnGetFileSize.  maybe it would be better to share code instead.
> 
> file_util::GetFileSize just calls GetFileInfo under the covers.
> 
> maybe you could pass the address of the "WriteReplyParams" function to use.

Done.

http://codereview.chromium.org/594036/diff/4001/4026
File chrome/renderer/renderer_webkitclient_impl.cc (right):

http://codereview.chromium.org/594036/diff/4001/4026#newcode102
chrome/renderer/renderer_webkitclient_impl.cc:102: result =
static_cast<time_t>(modification_time);
On 2010/02/12 21:54:26, darin wrote:
> The WebKitClient::getFileModificationTime should really be returning double
> instead of time_t since "double seconds" is what webkit normally uses for a
> timestamp.

Done.

http://codereview.chromium.org/594036/diff/4001/4012
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/594036/diff/4001/4012#newcode16
net/base/upload_data_stream.cc:16: if (stream->FillBuf())
On 2010/02/12 21:54:26, darin wrote:
> been hacking webkit too much :)  in chromium code, if you use brackets on one
> arm of the if/else, then use brackets on both.
> 
> however, in this case the "else" after "return" should just be deleted.

Done.

http://codereview.chromium.org/594036/diff/4001/4012#newcode86
net/base/upload_data_stream.cc:86: if
(file_util::GetFileInfo(element.file_path(), &info) &&
On 2010/02/12 21:54:26, darin wrote:
> this is a synchronous file stat.  can we at least query this information from
> next_element_stream_ so that we can use the open file descriptor instead of
the
> file path?
> 
> actually, instead of querying the information from FileStream, it would be
> better to open the file yourself, stat it, and then pass the file descriptor
to
> FileStream.  I think you should wait for this CL to land: 
> http://codereview.chromium.org/541022
> 
> It has all of those API changes, and should simplify this code.

Patch 541022 is not going to land soon. What should I do here?

http://codereview.chromium.org/594036/diff/4001/4016
File net/url_request/url_request.h (right):

http://codereview.chromium.org/594036/diff/4001/4016#newcode307
net/url_request/url_request.h:307: double modification_time);
On 2010/02/12 21:54:26, darin wrote:
> in chromium code we prefer using base::Time for timestamps.  please convert to
> base::Time at the WebKit <-> Chromium boundary.
> 
> also it would be good to document this parameter.  it is intended to be used
as
> a validator for the given file.

Done.

http://codereview.chromium.org/594036/diff/4001/4004
File webkit/glue/glue_serialize.cc (right):

http://codereview.chromium.org/594036/diff/4001/4004#newcode262
webkit/glue/glue_serialize.cc:262: WebString filePath = ReadString(obj);
On 2010/02/12 21:54:26, darin wrote:
> please use google style for variable names.

Done.

http://codereview.chromium.org/594036/diff/4001/4005
File webkit/glue/mock_resource_loader_bridge.h (right):

http://codereview.chromium.org/594036/diff/4001/4005#newcode27
webkit/glue/mock_resource_loader_bridge.h:27: double modification_time));
On 2010/02/12 21:54:26, darin wrote:
> expected_modification_time might be a good name for this parameter in general
> since "expected" helps convey the intent of this being a validator.

Done.

[darin (slow to review), 2010-03-05 09:07:33]

http://codereview.chromium.org/594036/diff/6001/6012
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/594036/diff/6001/6012#newcode33
net/base/upload_data_stream.cc:33: success_(false) {
i never see success_ getting set to true.  maybe you should actually use this
field to convey error information instead of making UpdateDataStream return
null?  that is, maybe the default mode should still be to create a stream that
just skips files if they are not present (or if they do not validate)?  or maybe
just having Create return a network error code is better.  hmm...

http://codereview.chromium.org/594036/diff/6001/6010
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/594036/diff/6001/6010#newcode848
net/http/http_network_transaction.cc:848: return ERR_FAILED;
i think we want a more specific error code here.  if we see this error on a
chrome network error page, it should be obvious from that what the cause of the
problem was.  it might therefore be nice if UpdateDataStream::Create returned a
network error code.

[jianli, 2010-03-19 23:14:43]

http://codereview.chromium.org/594036/diff/6001/6012
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/594036/diff/6001/6012#newcode33
net/base/upload_data_stream.cc:33: success_(false) {
On 2010/03/05 09:07:33, darin wrote:
> i never see success_ getting set to true.  maybe you should actually use this
> field to convey error information instead of making UpdateDataStream return
> null?  that is, maybe the default mode should still be to create a stream that
> just skips files if they are not present (or if they do not validate)?  or
maybe
> just having Create return a network error code is better.  hmm...

The variable success_ is a leftover. Removed.

I changed to have Create return a network error code in the output parameter.

http://codereview.chromium.org/594036/diff/6001/6010
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/594036/diff/6001/6010#newcode848
net/http/http_network_transaction.cc:848: return ERR_FAILED;
On 2010/03/05 09:07:33, darin wrote:
> i think we want a more specific error code here.  if we see this error on a
> chrome network error page, it should be obvious from that what the cause of
the
> problem was.  it might therefore be nice if UpdateDataStream::Create returned
a
> network error code.

I added a new error code in net_error_list.h.

[darin (slow to review), 2010-03-23 18:17:52]

I've asked Eric Roman to also review the net/ changes.

[darin (slow to review), 2010-03-23 19:53:01]

http://codereview.chromium.org/594036/diff/9001/10020
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/594036/diff/9001/10020#newcode1307
chrome/browser/renderer_host/resource_message_filter.cc:1307: void
WriteFileSize(IPC::Message* reply_msg,
this function should be moved up into the anonymous namespace section

http://codereview.chromium.org/594036/diff/9001/10020#newcode1332
chrome/browser/renderer_host/resource_message_filter.cc:1332: void
WriteFileModificationTime(IPC::Message* reply_msg,
this function should be moved up into the anonymous namespace section

http://codereview.chromium.org/594036/diff/9001/10019
File chrome/browser/renderer_host/resource_message_filter.h (right):

http://codereview.chromium.org/594036/diff/9001/10019#newcode317
chrome/browser/renderer_host/resource_message_filter.h:317:
void(*write_func)(IPC::Message* reply_msg,
nit: insert space after "void"

nit: can you create a typedef for this function pointer?

http://codereview.chromium.org/594036/diff/9001/10022
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/594036/diff/9001/10022#newcode2106
chrome/common/render_messages_internal.h:2106: double /* result */)
please use base::Time instead of double for timestamps in chromium code

http://codereview.chromium.org/594036/diff/9001/10013
File net/base/upload_data_stream_unittest.cc (right):

http://codereview.chromium.org/594036/diff/9001/10013#newcode79
net/base/upload_data_stream_unittest.cc:79: 
please add a test to verify that ERR_FILE_CHANGED is generated properly

http://codereview.chromium.org/594036/diff/9001/10009
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/594036/diff/9001/10009#newcode921
net/http/http_network_transaction.cc:921: int error_code = OK;
nit: no need to initialize the output parameter

http://codereview.chromium.org/594036/diff/9001/10010
File net/spdy/spdy_network_transaction.cc (right):

http://codereview.chromium.org/594036/diff/9001/10010#newcode251
net/spdy/spdy_network_transaction.cc:251: int error_code = OK;
nit: no need to initialize the output parameter

http://codereview.chromium.org/594036/diff/9001/10003
File webkit/glue/glue_serialize.cc (right):

http://codereview.chromium.org/594036/diff/9001/10003#newcode274
webkit/glue/glue_serialize.cc:274: #if ENABLE_BLOB_SLICE
I don't think we need to have this ENABLE_ option here.  that should only be
used within the WebKit / WebCore implementation files.  The WebKit API should
not be parameterized by this define.

[jianli, 2010-03-24 01:28:14]

http://codereview.chromium.org/594036/diff/9001/10020
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/594036/diff/9001/10020#newcode1307
chrome/browser/renderer_host/resource_message_filter.cc:1307: void
WriteFileSize(IPC::Message* reply_msg,
On 2010/03/23 19:53:02, darin wrote:
> this function should be moved up into the anonymous namespace section

Done.

http://codereview.chromium.org/594036/diff/9001/10020#newcode1332
chrome/browser/renderer_host/resource_message_filter.cc:1332: void
WriteFileModificationTime(IPC::Message* reply_msg,
On 2010/03/23 19:53:02, darin wrote:
> this function should be moved up into the anonymous namespace section

Done.

http://codereview.chromium.org/594036/diff/9001/10019
File chrome/browser/renderer_host/resource_message_filter.h (right):

http://codereview.chromium.org/594036/diff/9001/10019#newcode317
chrome/browser/renderer_host/resource_message_filter.h:317:
void(*write_func)(IPC::Message* reply_msg,
On 2010/03/23 19:53:02, darin wrote:
> nit: insert space after "void"
> 
> nit: can you create a typedef for this function pointer?

Done. Used typedef for it.

http://codereview.chromium.org/594036/diff/9001/10022
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/594036/diff/9001/10022#newcode2106
chrome/common/render_messages_internal.h:2106: double /* result */)
On 2010/03/23 19:53:02, darin wrote:
> please use base::Time instead of double for timestamps in chromium code

Done.

http://codereview.chromium.org/594036/diff/9001/10013
File net/base/upload_data_stream_unittest.cc (right):

http://codereview.chromium.org/594036/diff/9001/10013#newcode79
net/base/upload_data_stream_unittest.cc:79: 
On 2010/03/23 19:53:02, darin wrote:
> please add a test to verify that ERR_FILE_CHANGED is generated properly

Done.

http://codereview.chromium.org/594036/diff/9001/10009
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/594036/diff/9001/10009#newcode921
net/http/http_network_transaction.cc:921: int error_code = OK;
On 2010/03/23 19:53:02, darin wrote:
> nit: no need to initialize the output parameter

Done.

http://codereview.chromium.org/594036/diff/9001/10010
File net/spdy/spdy_network_transaction.cc (right):

http://codereview.chromium.org/594036/diff/9001/10010#newcode251
net/spdy/spdy_network_transaction.cc:251: int error_code = OK;
On 2010/03/23 19:53:02, darin wrote:
> nit: no need to initialize the output parameter

Done.

http://codereview.chromium.org/594036/diff/9001/10003
File webkit/glue/glue_serialize.cc (right):

http://codereview.chromium.org/594036/diff/9001/10003#newcode274
webkit/glue/glue_serialize.cc:274: #if ENABLE_BLOB_SLICE
On 2010/03/23 19:53:02, darin wrote:
> I don't think we need to have this ENABLE_ option here.  that should only be
> used within the WebKit / WebCore implementation files.  The WebKit API should
> not be parameterized by this define.

Done.

[darin (slow to review), 2010-03-24 18:12:15]

LGTM, but I would still like Eric Roman to sign off on the net/ changes. 
Thanks!

[eroman, 2010-03-24 18:47:47]

Can you upload the latest patch? I don't see some of the changes described in
the comments.

[eroman, 2010-03-24 20:56:04]

> Support sending a sliced file in chromium. 

Can you be a bit more verbose in the changelist description? Perhaps link to the
webkit issue (https://bugs.webkit.org/show_bug.cgi?id=32993).  I for example
didn't know what a "sliced file" was.

> TEST=The WebKit Layout test. 

I suggest listing the actual name:
http/tests/local/send-sliced-dragged-file.html

http://codereview.chromium.org/594036/diff/38001/39020
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/594036/diff/38001/39020#newcode1349
chrome/browser/renderer_host/resource_message_filter.cc:1349: // Getting file
modification time could take long time if it lives on a
nit: "take long time" --> "take a long time"

http://codereview.chromium.org/594036/diff/38001/39020#newcode1350
chrome/browser/renderer_host/resource_message_filter.cc:1350: // network share,
so run it on FILE thread.
nit: "run it on FILE thread" --> "run it on the FILE thread".

http://codereview.chromium.org/594036/diff/38001/39020#newcode1368
chrome/browser/renderer_host/resource_message_filter.cc:1368:
(*write_func)(reply_msg, file_info);
nit: why pass this in as a function pointer instead of just calling it directly?

http://codereview.chromium.org/594036/diff/38001/39017
File chrome_frame/urlmon_upload_data_stream.cc (right):

http://codereview.chromium.org/594036/diff/38001/39017#newcode22
chrome_frame/urlmon_upload_data_stream.cc:22: return E_FAIL;
I am not familiar with urlmon_upload_data_stream.cc, but I wander if it is
possible to fail during Initialize() step instead, and avoid these NULL checks.

http://codereview.chromium.org/594036/diff/38001/39030
File net/base/net_error_list.h (right):

http://codereview.chromium.org/594036/diff/38001/39030#newcode61
net/base/net_error_list.h:61: // The file has been changed from the expectation.
Can you make the comment more detailed? Perhaps:

The file upload failed because the file's modification time was different from
the expectation.

Similarly for the error name, I suggest having "UPLOAD" somewhere in it.

http://codereview.chromium.org/594036/diff/38001/39012
File net/base/upload_data.h (right):

http://codereview.chromium.org/594036/diff/38001/39012#newcode39
net/base/upload_data.h:39: const base::Time& expected_file_modification_time()
const {
Can you mention in a comment here that a null time means "no expectation".

http://codereview.chromium.org/594036/diff/38001/39012#newcode54
net/base/upload_data.h:54: const base::Time& expected_modification_time) {
Please add a comment explaining what expected_modification_time means.

In particular:

  * null means no expectation
  * the granularity for comparison is time_t, not the full precision.

http://codereview.chromium.org/594036/diff/38001/39011
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/594036/diff/38001/39011#newcode23
net/base/upload_data_stream.cc:23: delete stream;
I suggest avoiding manual calls to |delete|. Perhaps something like this:

scoped_ptr<UploadDataStream> stream(new ...);

...

if (rv != OK) {
  return NULL;
}

return stream.release();

http://codereview.chromium.org/594036/diff/38001/39011#newcode83
net/base/upload_data_stream.cc:83: // Note that the expected modification time
from WebKit is based on
It is unfortunate to have to mention WebKit in the net module.

It might be clearer to make the expected_file_modification_time into a time_t
parameter, so the granularity matches automatically, although I am torn. I guess
it is fine as is.

http://codereview.chromium.org/594036/diff/38001/39014
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/594036/diff/38001/39014#newcode20
net/base/upload_data_stream.h:20: static UploadDataStream* Create(const
UploadData* data, int* error_code);
Typically in the network module we make the error code be the return parameter
for functions, rather than an out parameter.

I suggest re-ordering this, such that the |UploadDataStream*| is an output
parameter. And similarly changing the contract so the out |UploadDataStream*| is
only valid when return value != OK.

http://codereview.chromium.org/594036/diff/38001/39014#newcode45
net/base/upload_data_stream.h:45: explicit UploadDataStream(const UploadData*
data);
Can you add a brief comment?

http://codereview.chromium.org/594036/diff/38001/39015
File net/url_request/url_request.h (right):

http://codereview.chromium.org/594036/diff/38001/39015#newcode304
net/url_request/url_request.h:304: // check if the underlying file has been
changed or not.
Please also describe the granularity of the time check (it is clipped to
time_t).

[jianli, 2010-03-24 22:42:27]

http://codereview.chromium.org/594036/diff/38001/39020
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/594036/diff/38001/39020#newcode1349
chrome/browser/renderer_host/resource_message_filter.cc:1349: // Getting file
modification time could take long time if it lives on a
On 2010/03/24 20:56:05, eroman wrote:
> nit: "take long time" --> "take a long time"

Done.

http://codereview.chromium.org/594036/diff/38001/39020#newcode1350
chrome/browser/renderer_host/resource_message_filter.cc:1350: // network share,
so run it on FILE thread.
On 2010/03/24 20:56:05, eroman wrote:
> nit: "run it on FILE thread" --> "run it on the FILE thread".

Done.

http://codereview.chromium.org/594036/diff/38001/39020#newcode1368
chrome/browser/renderer_host/resource_message_filter.cc:1368:
(*write_func)(reply_msg, file_info);
On 2010/03/24 20:56:05, eroman wrote:
> nit: why pass this in as a function pointer instead of just calling it
directly?

This is because we want share the above same code as much as possible for both
getting file size and modification time, as suggested by darin.

http://codereview.chromium.org/594036/diff/38001/39017
File chrome_frame/urlmon_upload_data_stream.cc (right):

http://codereview.chromium.org/594036/diff/38001/39017#newcode22
chrome_frame/urlmon_upload_data_stream.cc:22: return E_FAIL;
On 2010/03/24 20:56:05, eroman wrote:
> I am not familiar with urlmon_upload_data_stream.cc, but I wander if it is
> possible to fail during Initialize() step instead, and avoid these NULL
checks.

Yes, it is not possible. Remove the check here and add DCHECK on Initialize().

http://codereview.chromium.org/594036/diff/38001/39030
File net/base/net_error_list.h (right):

http://codereview.chromium.org/594036/diff/38001/39030#newcode61
net/base/net_error_list.h:61: // The file has been changed from the expectation.
On 2010/03/24 20:56:05, eroman wrote:
> Can you make the comment more detailed? Perhaps:
> 
> The file upload failed because the file's modification time was different from
> the expectation.
> 
> Similarly for the error name, I suggest having "UPLOAD" somewhere in it.

Done.

http://codereview.chromium.org/594036/diff/38001/39012
File net/base/upload_data.h (right):

http://codereview.chromium.org/594036/diff/38001/39012#newcode39
net/base/upload_data.h:39: const base::Time& expected_file_modification_time()
const {
On 2010/03/24 20:56:05, eroman wrote:
> Can you mention in a comment here that a null time means "no expectation".

Done.

http://codereview.chromium.org/594036/diff/38001/39012#newcode54
net/base/upload_data.h:54: const base::Time& expected_modification_time) {
On 2010/03/24 20:56:05, eroman wrote:
> Please add a comment explaining what expected_modification_time means.
> 
> In particular:
> 
>   * null means no expectation
>   * the granularity for comparison is time_t, not the full precision.

Done.

http://codereview.chromium.org/594036/diff/38001/39011
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/594036/diff/38001/39011#newcode23
net/base/upload_data_stream.cc:23: delete stream;
On 2010/03/24 20:56:05, eroman wrote:
> I suggest avoiding manual calls to |delete|. Perhaps something like this:
> 
> scoped_ptr<UploadDataStream> stream(new ...);
> 
> ...
> 
> if (rv != OK) {
>   return NULL;
> }
> 
> return stream.release();

Done.

http://codereview.chromium.org/594036/diff/38001/39011#newcode83
net/base/upload_data_stream.cc:83: // Note that the expected modification time
from WebKit is based on
On 2010/03/24 20:56:05, eroman wrote:
> It is unfortunate to have to mention WebKit in the net module.
> 
> It might be clearer to make the expected_file_modification_time into a time_t
> parameter, so the granularity matches automatically, although I am torn. I
guess
> it is fine as is.

I sure understand what you feel when I try to make the balance/consistency among
base::Time, double and time_t.

http://codereview.chromium.org/594036/diff/38001/39014
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/594036/diff/38001/39014#newcode20
net/base/upload_data_stream.h:20: static UploadDataStream* Create(const
UploadData* data, int* error_code);
On 2010/03/24 20:56:05, eroman wrote:
> Typically in the network module we make the error code be the return parameter
> for functions, rather than an out parameter.
> 
> I suggest re-ordering this, such that the |UploadDataStream*| is an output
> parameter. And similarly changing the contract so the out |UploadDataStream*|
is
> only valid when return value != OK.

I've thought about this first when I introduced the creator. The reason I choose
the other alternative is to accommodate the caller code. If I define the creator
as: static int Create(const UploadData* data, UploadDataStream** stream), we
will have to change the following calling code from:
  request_body_stream_.reset(net::UploadDataStream::Create(upload_data, NULL));
to:
  UploadDataStream* stream = NULL;
  net::UploadDataStream::Create(upload_data, &stream);
  request_body_stream_.reset(stream);

This is because the scoped_ptr does not have any method to get the address of
the underlying pointer.

How do you think? I can change it if you feel strongly about this.

http://codereview.chromium.org/594036/diff/38001/39014#newcode45
net/base/upload_data_stream.h:45: explicit UploadDataStream(const UploadData*
data);
On 2010/03/24 20:56:05, eroman wrote:
> Can you add a brief comment?

Done.

http://codereview.chromium.org/594036/diff/38001/39015
File net/url_request/url_request.h (right):

http://codereview.chromium.org/594036/diff/38001/39015#newcode304
net/url_request/url_request.h:304: // check if the underlying file has been
changed or not.
On 2010/03/24 20:56:05, eroman wrote:
> Please also describe the granularity of the time check (it is clipped to
> time_t).

Done.

[eroman, 2010-03-24 22:56:23]

Thanks for making those changes. LGTM.

The only other thing I wanted to bring up, is the slight race between when we
test the file's modification time, and when we read from it.

It looks like it is possible for us to test the modification time, it passes,
and then the file changes, and then we upload it.

http://codereview.chromium.org/594036/diff/38001/39014
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/594036/diff/38001/39014#newcode20
net/base/upload_data_stream.h:20: static UploadDataStream* Create(const
UploadData* data, int* error_code);
On 2010/03/24 22:42:27, jianli wrote:
> On 2010/03/24 20:56:05, eroman wrote:
> > Typically in the network module we make the error code be the return
parameter
> > for functions, rather than an out parameter.
> > 
> > I suggest re-ordering this, such that the |UploadDataStream*| is an output
> > parameter. And similarly changing the contract so the out
|UploadDataStream*|
> is
> > only valid when return value != OK.
> 
> I've thought about this first when I introduced the creator. The reason I
choose
> the other alternative is to accommodate the caller code. If I define the
creator
> as: static int Create(const UploadData* data, UploadDataStream** stream), we
> will have to change the following calling code from:
>   request_body_stream_.reset(net::UploadDataStream::Create(upload_data,
NULL));
> to:
>   UploadDataStream* stream = NULL;
>   net::UploadDataStream::Create(upload_data, &stream);
>   request_body_stream_.reset(stream);
> 
> This is because the scoped_ptr does not have any method to get the address of
> the underlying pointer.
> 
> How do you think? I can change it if you feel strongly about this.

I see. Well I'll leave it up to your preference then. One way would be to make
the out parameter a scoped_ptr*.

[jianli, 2010-03-25 00:45:57]

Yes, thanks for reminding this. I think this is only the very edge case with
which we can tolerate. We do not want to be 100% bullet proof by making
things too complicated.


On Wed, Mar 24, 2010 at 3:56 PM, <eroman@chromium.org> wrote:

> Thanks for making those changes. LGTM.
>
> The only other thing I wanted to bring up, is the slight race between when
> we
> test the file's modification time, and when we read from it.
>
> It looks like it is possible for us to test the modification time, it
> passes,
> and then the file changes, and then we upload it.
>
>
>
> http://codereview.chromium.org/594036/diff/38001/39014
> File net/base/upload_data_stream.h (right):
>
> http://codereview.chromium.org/594036/diff/38001/39014#newcode20
> net/base/upload_data_stream.h:20: static UploadDataStream* Create(const
> UploadData* data, int* error_code);
> On 2010/03/24 22:42:27, jianli wrote:
>
>> On 2010/03/24 20:56:05, eroman wrote:
>> > Typically in the network module we make the error code be the return
>>
> parameter
>
>> > for functions, rather than an out parameter.
>> >
>> > I suggest re-ordering this, such that the |UploadDataStream*| is an
>>
> output
>
>> > parameter. And similarly changing the contract so the out
>>
> |UploadDataStream*|
>
>> is
>> > only valid when return value != OK.
>>
>
>  I've thought about this first when I introduced the creator. The
>>
> reason I choose
>
>> the other alternative is to accommodate the caller code. If I define
>>
> the creator
>
>> as: static int Create(const UploadData* data, UploadDataStream**
>>
> stream), we
>
>> will have to change the following calling code from:
>>
>
> request_body_stream_.reset(net::UploadDataStream::Create(upload_data,
> NULL));
>
>> to:
>>   UploadDataStream* stream = NULL;
>>   net::UploadDataStream::Create(upload_data, &stream);
>>   request_body_stream_.reset(stream);
>>
>
>  This is because the scoped_ptr does not have any method to get the
>>
> address of
>
>> the underlying pointer.
>>
>
>  How do you think? I can change it if you feel strongly about this.
>>
>
> I see. Well I'll leave it up to your preference then. One way would be
> to make the out parameter a scoped_ptr*.
>
>
> http://codereview.chromium.org/594036
>

[darin (slow to review), 2010-03-25 02:57:20]

That is the edge case I was hoping to avoid by leveraging agl's patch, but
he was never able to get that one to stick due to some obscure errors on the
bots.

-Darin



On Wed, Mar 24, 2010 at 5:45 PM, Jian Li <jianli@chromium.org> wrote:

> Yes, thanks for reminding this. I think this is only the very edge case
> with which we can tolerate. We do not want to be 100% bullet proof by making
> things too complicated.
>
>
> On Wed, Mar 24, 2010 at 3:56 PM, <eroman@chromium.org> wrote:
>
>> Thanks for making those changes. LGTM.
>>
>> The only other thing I wanted to bring up, is the slight race between when
>> we
>> test the file's modification time, and when we read from it.
>>
>> It looks like it is possible for us to test the modification time, it
>> passes,
>> and then the file changes, and then we upload it.
>>
>>
>>
>> http://codereview.chromium.org/594036/diff/38001/39014
>> File net/base/upload_data_stream.h (right):
>>
>> http://codereview.chromium.org/594036/diff/38001/39014#newcode20
>> net/base/upload_data_stream.h:20: static UploadDataStream* Create(const
>> UploadData* data, int* error_code);
>> On 2010/03/24 22:42:27, jianli wrote:
>>
>>> On 2010/03/24 20:56:05, eroman wrote:
>>> > Typically in the network module we make the error code be the return
>>>
>> parameter
>>
>>> > for functions, rather than an out parameter.
>>> >
>>> > I suggest re-ordering this, such that the |UploadDataStream*| is an
>>>
>> output
>>
>>> > parameter. And similarly changing the contract so the out
>>>
>> |UploadDataStream*|
>>
>>> is
>>> > only valid when return value != OK.
>>>
>>
>>  I've thought about this first when I introduced the creator. The
>>>
>> reason I choose
>>
>>> the other alternative is to accommodate the caller code. If I define
>>>
>> the creator
>>
>>> as: static int Create(const UploadData* data, UploadDataStream**
>>>
>> stream), we
>>
>>> will have to change the following calling code from:
>>>
>>
>> request_body_stream_.reset(net::UploadDataStream::Create(upload_data,
>> NULL));
>>
>>> to:
>>>   UploadDataStream* stream = NULL;
>>>   net::UploadDataStream::Create(upload_data, &stream);
>>>   request_body_stream_.reset(stream);
>>>
>>
>>  This is because the scoped_ptr does not have any method to get the
>>>
>> address of
>>
>>> the underlying pointer.
>>>
>>
>>  How do you think? I can change it if you feel strongly about this.
>>>
>>
>> I see. Well I'll leave it up to your preference then. One way would be
>> to make the out parameter a scoped_ptr*.
>>
>>
>> http://codereview.chromium.org/594036
>>
>
>