Allow no content-type for POST, PUT and PATCH methods.

Allow no content-type for POST, PUT and PATCH methods.

This CL allows to have no content type for UrlFetcher's POST, PUT and PATCH methods, if the body is empty.
At the same time, POST_WITHOUT_BODY is deprecated, as POST
should work such a case. The following CL will clean it up, after the code using it is fixed.

BUG=148632
TEST=Ran unit_tests.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181039

[hidehiko, 2013-02-05 15:13:52]

Hi Matt,

Could you kindly take a look at this CL?
The concept of this CL is as same as one of
https://codereview.chromium.org/12096093/.

Thank you for your review in advance,
- hidehiko

[mmenke, 2013-02-05 22:16:42]

On 2013/02/05 15:13:52, hidehiko wrote:
> Hi Matt,
> 
> Could you kindly take a look at this CL?
> The concept of this CL is as same as one of
> https://codereview.chromium.org/12096093/.
> 
> Thank you for your review in advance,
> - hidehiko

Sorry for changing my mind on you, but I've double checked both the Google Drive
docs and the relevant RFC, looks to me like 0-length body actually will work. 
Think the solution you proposed (Allowing empty content-type with an empty body,
and not setting Content-Type) may be the better solution.

If it turns out content-length: 0 really doesn't work, though, I still think
this is the way to go.

[hidehiko, 2013-02-06 05:34:43]

Hi Matt,

Thank you for your review.
I'm asking the Drive team if it is ok to send "Content-Length: 0" whenever, but
under my quick test, it looks working. Also, I found that "Content-Length: 0" is
added around net/http/http_network_transaction.cc:743, regardless of the enum.
That is, if it is not allowed to send the header, I would need to change in
another way.

So I updated (almost rewrote) this CL, and
also fixed the comment for the POST_WITHOUT_BODY.

Note that the value is getting to be used. So please let me send another CL to
clean it up, after the usage is fixed.

Again, thank you for your review!
- hidehiko

[mmenke, 2013-02-06 16:23:58]

https://codereview.chromium.org/12226008/diff/4002/net/url_request/url_fetcher.h
File net/url_request/url_fetcher.h (right):

https://codereview.chromium.org/12226008/diff/4002/net/url_request/url_fetche...
net/url_request/url_fetcher.h:293: base::FilePath* out_response_path) const = 0;
While you're here, could you fix the line with more than 80 characters?

https://codereview.chromium.org/12226008/diff/4002/net/url_request/url_fetche...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/12226008/diff/4002/net/url_request/url_fetche...
net/url_request/url_fetcher_core.cc:740: !upload_content_type_.empty());
Rather than this, I'd prefer a design that requires an explicit call to
SetUploadData("", "").

The simplest way to do this may be to move this DCHECK into the two SetUpload
routines, allowing content type to be empty in SetUploadData if the content is
empty, but never in SetChunkedUpload.

Then you DCHECK(is_chunked_upload_ || upload_content_) here.  Sound reasonable?

You could dig into upload_content_ here to figure out if it's 0-length, but
think that's needlessly complicated.

https://codereview.chromium.org/12226008/diff/4002/net/url_request/url_fetche...
net/url_request/url_fetcher_core.cc:747: upload_content_type_);
nit:  Use braces when body is more than one line.

[hidehiko, 2013-02-06 17:19:49]

Updated and running try-bot.
Could you take another look?

https://codereview.chromium.org/12226008/diff/4002/net/url_request/url_fetcher.h
File net/url_request/url_fetcher.h (right):

https://codereview.chromium.org/12226008/diff/4002/net/url_request/url_fetche...
net/url_request/url_fetcher.h:293: base::FilePath* out_response_path) const = 0;
On 2013/02/06 16:23:58, Matt Menke wrote:
> While you're here, could you fix the line with more than 80 characters?

Sure, done.

https://codereview.chromium.org/12226008/diff/4002/net/url_request/url_fetche...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/12226008/diff/4002/net/url_request/url_fetche...
net/url_request/url_fetcher_core.cc:740: !upload_content_type_.empty());
On 2013/02/06 16:23:58, Matt Menke wrote:
> Rather than this, I'd prefer a design that requires an explicit call to
> SetUploadData("", "").
> 
> The simplest way to do this may be to move this DCHECK into the two SetUpload
> routines, allowing content type to be empty in SetUploadData if the content is
> empty, but never in SetChunkedUpload.
> 
> Then you DCHECK(is_chunked_upload_ || upload_content_) here.  Sound
reasonable?
> 
> You could dig into upload_content_ here to figure out if it's 0-length, but
> think that's needlessly complicated.

Indeed. I just allowed SetUploadData("", "") for empty content type to be
conservative. This is because UploadDataStream given via SetUploadDataStream may
not be initialized yet, so it may be impossible to take the total size, that is,
we cannot check emptiness of the upload content there.

What do you think?

https://codereview.chromium.org/12226008/diff/4002/net/url_request/url_fetche...
net/url_request/url_fetcher_core.cc:747: upload_content_type_);
On 2013/02/06 16:23:58, Matt Menke wrote:
> nit:  Use braces when body is more than one line.

Done.

[mmenke, 2013-02-06 17:27:07]

LGTM, modulo comment.

https://codereview.chromium.org/12226008/diff/4003/net/url_request/url_fetche...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/12226008/diff/4003/net/url_request/url_fetche...
net/url_request/url_fetcher_core.cc:364: !upload_content_));
"DCHECK(!upload_content_type.empty());" here as well

[hidehiko, 2013-02-06 17:55:41]

Thank you for your review. Submitting...

https://chromiumcodereview.appspot.com/12226008/diff/4003/net/url_request/url...
File net/url_request/url_fetcher_core.cc (right):

https://chromiumcodereview.appspot.com/12226008/diff/4003/net/url_request/url...
net/url_request/url_fetcher_core.cc:364: !upload_content_));
On 2013/02/06 17:27:07, Matt Menke wrote:
> "DCHECK(!upload_content_type.empty());" here as well

Done.

[commit-bot: I haz the power, 2013-02-06 17:56:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/12226008/9003

[commit-bot: I haz the power, 2013-02-06 20:14:17]

Change committed as 181039