net: Return size of upload as well as position from HttpTransaction::GetUploadProgress()

net: Return size of upload as well as position from HttpTransaction::GetUploadProgress()

Add net::UploadProgress
Change return type of GetUploadSize() from uint64 to UploadProgress.

BUG=112607
TEST=net_unittests

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

[hashimoto, 2012-08-16 11:44:27]

Could you review this?
This change is needed to remove synchronous file operation from
ResourceDispatcherHostImpl (http://codereview.chromium.org/10825073/)

[willchan no longer on Chromium, 2012-08-16 22:49:06]

+mmenke, FYI, check the linked codereview for Darin's explanation of what's
going on.

LGTM mod some nits.

http://codereview.chromium.org/10834178/diff/7003/net/base/upload_progress.h
File net/base/upload_progress.h (right):

http://codereview.chromium.org/10834178/diff/7003/net/base/upload_progress.h#...
net/base/upload_progress.h:6: #define NET_URL_REQUEST_UPLOAD_PROGRESS_
Your include guards are wrong

http://codereview.chromium.org/10834178/diff/7003/net/base/upload_progress.h#...
net/base/upload_progress.h:14: uint64 size;
You need basictypes.h for this type.

[hashimoto, 2012-08-17 03:02:32]

http://codereview.chromium.org/10834178/diff/7003/net/base/upload_progress.h
File net/base/upload_progress.h (right):

http://codereview.chromium.org/10834178/diff/7003/net/base/upload_progress.h#...
net/base/upload_progress.h:6: #define NET_URL_REQUEST_UPLOAD_PROGRESS_
On 2012/08/16 22:49:06, willchan wrote:
> Your include guards are wrong
Oops, I thought this kind of error would be caught by presubmit check.
Done.

http://codereview.chromium.org/10834178/diff/7003/net/base/upload_progress.h#...
net/base/upload_progress.h:14: uint64 size;
On 2012/08/16 22:49:06, willchan wrote:
> You need basictypes.h for this type.

Done.

[darin (slow to review), 2012-08-29 22:05:35]

http://codereview.chromium.org/10834178/diff/13001/net/base/upload_progress.h
File net/base/upload_progress.h (right):

http://codereview.chromium.org/10834178/diff/13001/net/base/upload_progress.h...
net/base/upload_progress.h:16: uint64 size;
do these need to be public variables?  do they need to be mutable?  they do not
appear to need to be mutable for any of the code in this CL.

http://codereview.chromium.org/10834178/diff/13001/net/http/http_cache_transa...
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/10834178/diff/13001/net/http/http_cache_transa...
net/http/http_cache_transaction.cc:425: return final_upload_progress_;
nothing seems to ever assign a value to final_upload_progress_.  perhaps this
should just return UploadProgress()?

[hashimoto, 2012-08-29 22:27:07]

http://codereview.chromium.org/10834178/diff/13001/net/base/upload_progress.h
File net/base/upload_progress.h (right):

http://codereview.chromium.org/10834178/diff/13001/net/base/upload_progress.h...
net/base/upload_progress.h:16: uint64 size;
On 2012/08/29 22:05:35, darin wrote:
> do these need to be public variables?  do they need to be mutable?  they do
not
> appear to need to be mutable for any of the code in this CL.

I think UploadProgress is simple enough to be a struct, not a class.
Made a new change in which UploadProgress is a class.
Is this what you mean?

http://codereview.chromium.org/10834178/diff/13001/net/http/http_cache_transa...
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/10834178/diff/13001/net/http/http_cache_transa...
net/http/http_cache_transaction.cc:425: return final_upload_progress_;
On 2012/08/29 22:05:35, darin wrote:
> nothing seems to ever assign a value to final_upload_progress_.  perhaps this
> should just return UploadProgress()?

HttpCache::Transaction::DoUpdateCachedResponseComplete() is assigning to it in
line 1139.

[darin (slow to review), 2012-08-29 22:53:00]

Thanks for pointing out the final_upload_progress_ assignment.  That was buried
in the unmodified portions of the code! :-)

http://codereview.chromium.org/10834178/diff/28001/net/base/upload_progress.h
File net/base/upload_progress.h (right):

http://codereview.chromium.org/10834178/diff/28001/net/base/upload_progress.h...
net/base/upload_progress.h:18: uint64 size() const { return size_; }
yes, this is what i had in mind.  even though UploadProgress is a simple class,
it seems like the intent is for GetUploadProgress to return snapshots of
progress.  that's why I thought this might be good as a class w/ immutable
attributes.

[commit-bot: I haz the power, 2012-08-29 22:59:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/10834178/28001

[willchan no longer on Chromium, 2012-08-29 23:10:02]

http://codereview.chromium.org/10834178/diff/28001/net/base/upload_progress.h
File net/base/upload_progress.h (right):

http://codereview.chromium.org/10834178/diff/28001/net/base/upload_progress.h...
net/base/upload_progress.h:18: uint64 size() const { return size_; }
On 2012/08/29 22:53:00, darin wrote:
> yes, this is what i had in mind.  even though UploadProgress is a simple
class,
> it seems like the intent is for GetUploadProgress to return snapshots of
> progress.  that's why I thought this might be good as a class w/ immutable
> attributes.

But now we're using operator= and the copy constructor for UploadProgress, when
we generally want to DISALLOW_COPY_AND_ASSIGN(). If we wanted to make it
immutable, why not just make the member variables const and keep it as a struct?

[commit-bot: I haz the power, 2012-08-29 23:23:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/10834178/28001

[commit-bot: I haz the power, 2012-08-30 03:47:33]

Change committed as 154063