POSTs to HSTS domains are no longer converted to GETs

POSTs to HSTS domains are no longer converted to GETs 
on upgrade to https. 

URLRequestRedirectJobs now take a status code on construction, 
to reduce the change of regressions, and the status code can 
no longer be changed after construction. 

TBR=jhawkins@chromium.org 
BUG=159343
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170029

[mmenke, 2012-11-16 19:10:28]

Out of paranoia, seemed best to make the status code a constructor parameter. 
We could probably just swap the default to 307, and everything would be fine
except the unit test that needs a 302, but I think making it an explicit
parameter is safer.  Also unclear if
"ProtocolHandlerRegistry::Core::MaybeCreateJob" should really be using 307s.

[willchan no longer on Chromium, 2012-11-17 21:43:08]

Please update this changelist description to describe the refactoring changes
too.

LGTM

https://codereview.chromium.org/11420013/diff/14021/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

https://codereview.chromium.org/11420013/diff/14021/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.cc:155:
net::URLRequestRedirectJob::REDIRECT_302_FOUND);
I think you're right that this is wrong. May want to file a bug about it to
doublecheck and check with koz@ to see what's desired here. Let's not solve it
in this CL.

https://codereview.chromium.org/11420013/diff/14021/net/url_request/url_reque...
File net/url_request/url_request_redirect_job.h (right):

https://codereview.chromium.org/11420013/diff/14021/net/url_request/url_reque...
net/url_request/url_request_redirect_job.h:46: int http_status_code_;
Nit: Make these member variables const?
Up to you, but I like documenting that these can't change.

[mmenke, 2012-11-19 16:00:34]

Description updated as well.

https://codereview.chromium.org/11420013/diff/14021/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

https://codereview.chromium.org/11420013/diff/14021/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.cc:155:
net::URLRequestRedirectJob::REDIRECT_302_FOUND);
On 2012/11/17 21:43:08, willchan wrote:
> I think you're right that this is wrong. May want to file a bug about it to
> doublecheck and check with koz@ to see what's desired here. Let's not solve it
> in this CL.

Bug filed - http://www.crbug.com/161769

https://codereview.chromium.org/11420013/diff/14021/net/url_request/url_reque...
File net/url_request/url_request_redirect_job.h (right):

https://codereview.chromium.org/11420013/diff/14021/net/url_request/url_reque...
net/url_request/url_request_redirect_job.h:46: int http_status_code_;
On 2012/11/17 21:43:08, willchan wrote:
> Nit: Make these member variables const?
> Up to you, but I like documenting that these can't change.

Done - I agree.

[commit-bot: I haz the power, 2012-11-19 16:00:50]

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

[commit-bot: I haz the power, 2012-11-19 16:00:58]

Presubmit check for 11420013-10016 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome_frame
    chrome
    chrome/browser/chromeos

Presubmit checks took 1.2s to calculate.

[mmenke, 2012-11-19 16:13:44]

davemoore: chrome/browser/chromeos/
erikwright: chrome_frame/

[mmenke, 2012-11-19 16:14:23]

And TBRing jhawkins for the refactoring change to
chrome/browser/custom_handlers/protocol_handler_registry.cc

[mmenke, 2012-11-19 16:21:03]

Oh, and davemoore - HSTS isn't supported on Chrome Frame, which is why this and
the other HSTS tests aren't run there.

[erikwright (departed), 2012-11-19 16:52:26]

chrome_frame LGTM.

[mmenke, 2012-11-19 16:53:19]

On 2012/11/19 16:21:03, Matt Menke wrote:
> Oh, and davemoore - HSTS isn't supported on Chrome Frame, which is why this
and
> the other HSTS tests aren't run there.

Sorry, that was meant to be addressed to erik.

[mmenke, 2012-11-27 16:36:27]

davemoore:  Ping.

[commit-bot: I haz the power, 2012-11-28 17:03:41]

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

[commit-bot: I haz the power, 2012-11-28 17:45:35]

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

[szym, 2012-11-28 18:04:16]

https://codereview.chromium.org/11420013/diff/11029/net/url_request/url_reque...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/11420013/diff/11029/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:4197:
host_resolver.rules()->AddRule("www.somewhere.com", "127.0.0.1");
Drive-by: MockHostResolver redirects everything to 127.0.0.1 by default
(exceptions are allowed). I guess the header doc in mock_host_resolver.h should
explain that.

[mmenke, 2012-11-28 18:34:45]

https://codereview.chromium.org/11420013/diff/11029/net/url_request/url_reque...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/11420013/diff/11029/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:4197:
host_resolver.rules()->AddRule("www.somewhere.com", "127.0.0.1");
On 2012/11/28 18:04:16, szym wrote:
> Drive-by: MockHostResolver redirects everything to 127.0.0.1 by default
> (exceptions are allowed). I guess the header doc in mock_host_resolver.h
should
> explain that.

Thanks.  This was copy/pasted directly from another test in this file.  I'll go
ahead land this as-is, and do a followup CL that removes them from this file,
and updates their (identical) comments as appropriate.

[commit-bot: I haz the power, 2012-11-28 19:52:16]

Change committed as 170029