ResourceFetcher: Add POST support and the ability to set headers

ResourceFetcher:  Add POST support and the ability to set headers

These are needed to use the new Link Doctor API.

BUG=64832
R=darin@chromium.org, ttuttle@chromium.org

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

[mmenke, 2014-01-17 18:03:24]

Plan to remove alt_error_page_resource_fetcher.cc from the CL and land the other
one first, just wanted to run it on the trybots.

[mmenke, 2014-01-17 18:24:03]

[+ttuttle]:  PTAL

[mmenke, 2014-01-27 15:34:53]

ttuttle:  Ping

[Deprecated (see juliatuttle), 2014-01-27 16:20:51]

On 2014/01/27 15:34:53, mmenke wrote:
> ttuttle:  Ping

lgtm.

[mmenke, 2014-01-27 16:27:40]

[+darin]:  PTAL.  Should be the last of these I send your way.

[darin (slow to review), 2014-01-27 17:23:15]

LGTM

(I wonder if we should do anything to help alert developers who mistakenly set a
body with a method that does not support a body. That's a difficult thing to do
in general, but just ensuring that the method is not "GET" when the body is set
might help.)

https://codereview.chromium.org/140823010/diff/800001/content/renderer/fetche...
File content/renderer/fetchers/image_resource_fetcher.cc (right):

https://codereview.chromium.org/140823010/diff/800001/content/renderer/fetche...
content/renderer/fetchers/image_resource_fetcher.cc:34:
fetcher_.reset(ResourceFetcher::Create(image_url));
I really like that you broke out the Start method. Always good to limit the
side-effects of constructors!

[mmenke, 2014-01-27 17:41:15]

Thanks for the reviews!

I went ahead and add a DCHECK when setting the body if the method is still
"GET".  Also added a comment in the public API that the method must be set
before the body.

On 2014/01/27 17:23:15, darin wrote:
> LGTM
> 
> (I wonder if we should do anything to help alert developers who mistakenly set
a
> body with a method that does not support a body. That's a difficult thing to
do
> in general, but just ensuring that the method is not "GET" when the body is
set
> might help.)
> 
>
https://codereview.chromium.org/140823010/diff/800001/content/renderer/fetche...
> File content/renderer/fetchers/image_resource_fetcher.cc (right):
> 
>
https://codereview.chromium.org/140823010/diff/800001/content/renderer/fetche...
> content/renderer/fetchers/image_resource_fetcher.cc:34:
> fetcher_.reset(ResourceFetcher::Create(image_url));
> I really like that you broke out the Start method. Always good to limit the
> side-effects of constructors!

[commit-bot: I haz the power, 2014-01-27 19:31:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/140823010/800002

[commit-bot: I haz the power, 2014-01-27 19:50:14]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[mmenke, 2014-01-27 19:54:23]

On 2014/01/27 19:50:14, I haz the power (commit-bot) wrote:
> Retried try job too often on mac_rel for step(s) base_unittests,
browser_tests,
> interactive_ui_tests, net_unittests, unit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

Didn't like my DCHECK, since it was using a WebString.  Will fix and retry.

[commit-bot: I haz the power, 2014-01-27 21:03:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/140823010/930001

[darin (slow to review), 2014-01-27 21:26:52]

Are you sure you want to assert that in SetBody? What if the next line
after SetBody is SetMethod("POST")? It seems like you should assert in
Start rather than in the setters.

-Darin


On Mon, Jan 27, 2014 at 9:41 AM, <mmenke@chromium.org> wrote:

> Thanks for the reviews!
>
> I went ahead and add a DCHECK when setting the body if the method is still
> "GET".  Also added a comment in the public API that the method must be set
> before the body.
>
>
> On 2014/01/27 17:23:15, darin wrote:
>
>> LGTM
>>
>
>  (I wonder if we should do anything to help alert developers who
>> mistakenly set
>>
> a
>
>> body with a method that does not support a body. That's a difficult thing
>> to
>>
> do
>
>> in general, but just ensuring that the method is not "GET" when the body
>> is
>>
> set
>
>> might help.)
>>
>
>
> https://codereview.chromium.org/140823010/diff/800001/
> content/renderer/fetchers/image_resource_fetcher.cc
>
>> File content/renderer/fetchers/image_resource_fetcher.cc (right):
>>
>
>
> https://codereview.chromium.org/140823010/diff/800001/
> content/renderer/fetchers/image_resource_fetcher.cc#newcode34
>
>> content/renderer/fetchers/image_resource_fetcher.cc:34:
>> fetcher_.reset(ResourceFetcher::Create(image_url));
>> I really like that you broke out the Start method. Always good to limit
>> the
>> side-effects of constructors!
>>
>
>
>
> https://codereview.chromium.org/140823010/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mmenke, 2014-01-27 21:27:11]

Good point.  I have no idea why that didn't occur to me.


On Mon, Jan 27, 2014 at 4:26 PM, Darin Fisher <darin@chromium.org> wrote:

> Are you sure you want to assert that in SetBody? What if the next line
> after SetBody is SetMethod("POST")? It seems like you should assert in
> Start rather than in the setters.
>
> -Darin
>
>
> On Mon, Jan 27, 2014 at 9:41 AM, <mmenke@chromium.org> wrote:
>
>> Thanks for the reviews!
>>
>> I went ahead and add a DCHECK when setting the body if the method is still
>> "GET".  Also added a comment in the public API that the method must be set
>> before the body.
>>
>>
>> On 2014/01/27 17:23:15, darin wrote:
>>
>>> LGTM
>>>
>>
>>  (I wonder if we should do anything to help alert developers who
>>> mistakenly set
>>>
>> a
>>
>>> body with a method that does not support a body. That's a difficult
>>> thing to
>>>
>> do
>>
>>> in general, but just ensuring that the method is not "GET" when the body
>>> is
>>>
>> set
>>
>>> might help.)
>>>
>>
>>
>> https://codereview.chromium.org/140823010/diff/800001/
>> content/renderer/fetchers/image_resource_fetcher.cc
>>
>>> File content/renderer/fetchers/image_resource_fetcher.cc (right):
>>>
>>
>>
>> https://codereview.chromium.org/140823010/diff/800001/
>> content/renderer/fetchers/image_resource_fetcher.cc#newcode34
>>
>>> content/renderer/fetchers/image_resource_fetcher.cc:34:
>>> fetcher_.reset(ResourceFetcher::Create(image_url));
>>> I really like that you broke out the Start method. Always good to limit
>>> the
>>> side-effects of constructors!
>>>
>>
>>
>>
>> https://codereview.chromium.org/140823010/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-01-27 21:37:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/140823010/950001

[commit-bot: I haz the power, 2014-01-27 22:10:44]

Retried try job too often on linux_rel for step(s) content_browsertests,
interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-28 00:06:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/140823010/960001

[commit-bot: I haz the power, 2014-01-28 01:48:05]

Retried try job too often on win_rel for step(s) telemetry_perf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-28 01:52:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/140823010/960001

[mmenke, 2014-01-28 13:32:06]

Committed patchset #8 manually as r247445 (presubmit successful).