Add HTTP PUT request type to content:URLFetcher

content::URLFetcher: Add HTTP PUT request type and method to append header string.
PUT request is needed for uploading files to gdata.

BUG=chromium-os:25357
TEST=nothing yet.

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

[wtc, 2012-02-06 23:34:07]

Review comments on Patch Set 1:

Thank you for the patch.  I suggest some changes below.

http://codereview.chromium.org/9342007/diff/1/content/common/net/url_fetcher_...
File content/common/net/url_fetcher_impl.cc (right):

http://codereview.chromium.org/9342007/diff/1/content/common/net/url_fetcher_...
content/common/net/url_fetcher_impl.cc:821: case PUT:

Let's list the PUT case right below the POST case because
the two cases are very similar.

I wonder if it's possible to merge the code for the two
cases.

http://codereview.chromium.org/9342007/diff/1/content/common/net/url_fetcher_...
content/common/net/url_fetcher_impl.cc:822: DCHECK(!upload_content_.empty());

I studied the Section 9.6 PUT in the HTTP 1.1 RFC 2616.
I don't see anything that forbids the use of chunked
encoding.  So I think this DCHECK should look like the
DCHECK on line 801 for the POST case.

http://codereview.chromium.org/9342007/diff/1/content/public/common/url_fetch...
File content/public/common/url_fetcher.h (right):

http://codereview.chromium.org/9342007/diff/1/content/public/common/url_fetch...
content/public/common/url_fetcher.h:146: // Add header (with format $key ":"
$value) to the request headers.

Nit: I'd omit the dollar signs ($) in the format description.

You can consider using the exact wording from HTTP 1.1:

    field-name ":" [ field-value ]

http://codereview.chromium.org/9342007/diff/1/content/public/common/url_fetch...
content/public/common/url_fetcher.h:149: const base::StringPiece& header_line) =
0;

It's important to explain the difference between this
method and the SetExtraRequestHeaders method above.
One important difference (ignoring the std::string vs.
base::StringPiece difference) is that SetExtraRequestHeaders
replaces the extra request headers whereas this method
appends one header.

I suggest renaming this method AddExtraRequestHeader.

I also suggest using the same data type (either std::string
or base::StringPiece) for both methods.

[kuan, 2012-02-06 23:58:16]

i've addressed all ur comments.  ptal, thanks.

http://codereview.chromium.org/9342007/diff/1/content/common/net/url_fetcher_...
File content/common/net/url_fetcher_impl.cc (right):

http://codereview.chromium.org/9342007/diff/1/content/common/net/url_fetcher_...
content/common/net/url_fetcher_impl.cc:821: case PUT:
On 2012/02/06 23:34:07, wtc wrote:
> 
> Let's list the PUT case right below the POST case because
> the two cases are very similar.
> 
> I wonder if it's possible to merge the code for the two
> cases.

Done.

http://codereview.chromium.org/9342007/diff/1/content/common/net/url_fetcher_...
content/common/net/url_fetcher_impl.cc:822: DCHECK(!upload_content_.empty());
On 2012/02/06 23:34:07, wtc wrote:
> 
> I studied the Section 9.6 PUT in the HTTP 1.1 RFC 2616.
> I don't see anything that forbids the use of chunked
> encoding.  So I think this DCHECK should look like the
> DCHECK on line 801 for the POST case.

Done.  Since chunked encoding is also allowed for PUT, i've merged both POST and
PUT together.

http://codereview.chromium.org/9342007/diff/1/content/public/common/url_fetch...
File content/public/common/url_fetcher.h (right):

http://codereview.chromium.org/9342007/diff/1/content/public/common/url_fetch...
content/public/common/url_fetcher.h:146: // Add header (with format $key ":"
$value) to the request headers.
On 2012/02/06 23:34:07, wtc wrote:
> 
> Nit: I'd omit the dollar signs ($) in the format description.
> 
> You can consider using the exact wording from HTTP 1.1:
> 
>     field-name ":" [ field-value ]

Done.

http://codereview.chromium.org/9342007/diff/1/content/public/common/url_fetch...
content/public/common/url_fetcher.h:149: const base::StringPiece& header_line) =
0;
On 2012/02/06 23:34:07, wtc wrote:
> 
> It's important to explain the difference between this
> method and the SetExtraRequestHeaders method above.
> One important difference (ignoring the std::string vs.
> base::StringPiece difference) is that SetExtraRequestHeaders
> replaces the extra request headers whereas this method
> appends one header.
> 
> I suggest renaming this method AddExtraRequestHeader.
> 
> I also suggest using the same data type (either std::string
> or base::StringPiece) for both methods.

Done.

[wtc, 2012-02-07 01:25:11]

Patch Set 2 LGTM.

Are you working with satorux on this feature?

[commit-bot: I haz the power, 2012-02-07 02:14:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kuan@chromium.org/9342007/6001

[commit-bot: I haz the power, 2012-02-07 02:14:55]

Presubmit check for 9342007-6001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: content/public/common/url_fetcher.h

Presubmit checks took 1.8s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[kuan, 2012-02-07 02:18:24]

jam, could u provide owner's approval for added method in url_fetcher.h? 
thanks.

[jam, 2012-02-07 02:46:13]

lgtm for public

[commit-bot: I haz the power, 2012-02-07 02:53:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kuan@chromium.org/9342007/6001

[commit-bot: I haz the power, 2012-02-07 06:56:29]

Change committed as 120731