Bypass the DataReductionProxy for all POST requests

Bypass the DataReductionProxy for all POST requests

POST requests are non-dempotent, so clients using the Data Reduction Proxy (DRP) can often end up at an error page due to the fact that if the DRP has any problems connecting to the origin, DRP and the client will not be safely able to determine if the post successfully made it to the server. The impact of this change on data compression should be very limited, and it will improve the client's experience.

BUG=581750
Committed: https://crrev.com/4bab0683f19f749d1974944281c07abbc0d814cf
Cr-Commit-Position: refs/heads/master@{#379137}

[RyanSturm, 2016-02-11 22:02:21]

ryansturm@chromium.org changed reviewers:
+ bengr@chromium.org

[bengr, 2016-02-16 20:00:14]

https://codereview.chromium.org/1684123004/diff/20001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/1684123004/diff/20001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:1019: url, net::LOAD_NORMAL, &info, nullptr, "",
net::BoundNetLog());
Since this entire file uses NULL, I would keep the null here. Add std::string()
instead of "".

https://codereview.chromium.org/1684123004/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc
(right):

https://codereview.chromium.org/1684123004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:52:
const std::string& method,
Move the method to just after the url.

https://codereview.chromium.org/1684123004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:117:
const std::string& method,
Move the method to just after the url.

https://codereview.chromium.org/1684123004/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.h
(right):

https://codereview.chromium.org/1684123004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.h:53:
const std::string& method,
Move the method to just after the url.

https://codereview.chromium.org/1684123004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.h:91:
const std::string& method,
Move the method to just after the url.

https://codereview.chromium.org/1684123004/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc
(right):

https://codereview.chromium.org/1684123004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc:385:
// we will fail the test rather than hitting a crash from the DCHECK in
Don't personify code and use complete sentences. I don't think your comment is
useful btw.

https://codereview.chromium.org/1684123004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc:393:
// POST methods go direct
direct -> direct.

https://codereview.chromium.org/1684123004/diff/20001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_network_proxy_host.cc (right):

https://codereview.chromium.org/1684123004/diff/20001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_network_proxy_host.cc:153: nullptr,
Use NULL and std::string().

https://codereview.chromium.org/1684123004/diff/20001/content/browser/resolve...
File content/browser/resolve_proxy_msg_helper.cc (right):

https://codereview.chromium.org/1684123004/diff/20001/content/browser/resolve...
content/browser/resolve_proxy_msg_helper.cc:98: &req.pac_req, nullptr, "",
net::BoundNetLog());
NULL and std::string()

https://codereview.chromium.org/1684123004/diff/20001/google_apis/gcm/engine/...
File google_apis/gcm/engine/connection_factory_impl.cc (right):

https://codereview.chromium.org/1684123004/diff/20001/google_apis/gcm/engine/...
google_apis/gcm/engine/connection_factory_impl.cc:315: nullptr,
NULL and std::string() here and at 538.

https://codereview.chromium.org/1684123004/diff/20001/jingle/glue/proxy_resol...
File jingle/glue/proxy_resolving_client_socket.cc (right):

https://codereview.chromium.org/1684123004/diff/20001/jingle/glue/proxy_resol...
jingle/glue/proxy_resolving_client_socket.cc:148: nullptr,
NULL, std::string()

https://codereview.chromium.org/1684123004/diff/20001/jingle/glue/proxy_resol...
jingle/glue/proxy_resolving_client_socket.cc:297: proxy_resolve_callback_,
&pac_request_, nullptr, "", bound_net_log_);
NULL, std::string()

https://codereview.chromium.org/1684123004/diff/20001/net/base/proxy_delegate.h
File net/base/proxy_delegate.h (right):

https://codereview.chromium.org/1684123004/diff/20001/net/base/proxy_delegate...
net/base/proxy_delegate.h:38: const std::string& method,
Move the method to just after the url.

https://codereview.chromium.org/1684123004/diff/20001/net/base/test_proxy_del...
File net/base/test_proxy_delegate.cc (right):

https://codereview.chromium.org/1684123004/diff/20001/net/base/test_proxy_del...
net/base/test_proxy_delegate.cc:45: const std::string& method,
Move the method to just after the url.

https://codereview.chromium.org/1684123004/diff/20001/net/base/test_proxy_del...
File net/base/test_proxy_delegate.h (right):

https://codereview.chromium.org/1684123004/diff/20001/net/base/test_proxy_del...
net/base/test_proxy_delegate.h:56: const std::string& method,
Move the method to just after the url.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:775: const std::string& method)
Move the method to just after the url.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:924: std::string method_;
Move this up to before load_flags_ so as to group together all inputs to proxy
resolution.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1020: const std::string& method,
Move the method to just after the url.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1033: const std::string& method,
Move the method to just after the url.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1094: const std::string& method,
Move the method to just after the url.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1263: const std::string& method,
Move the method to just after the url.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1361: const std::string& method,
Move the method to just after the url.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.h:135: const std::string& method,
Move the method to just after the url.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.h:144: const std::string& method,
Move the method to just after the url.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.h:167: ProxyDelegate* proxy_delegate,
Move the method to just after the url.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.h:344: const std::string& method,
Move the method to just after the url.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.h:367: const std::string& method,
Move the method to just after the url.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
File net/proxy/proxy_service_mojo_unittest.cc (right):

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service_mojo_unittest.cc:161: "", BoundNetLog()));
std::string(). Ditto for line 183, 214, and 243.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:176: const std::string& method,
Move the method to just after the url.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:237: const std::string& method,
Move the method to just after the url.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:359: nullptr, nullptr, "", log.bound());
Use std::string() here and below.

https://codereview.chromium.org/1684123004/diff/20001/net/websockets/websocke...
File net/websockets/websocket_end_to_end_test.cc (right):

https://codereview.chromium.org/1684123004/diff/20001/net/websockets/websocke...
net/websockets/websocket_end_to_end_test.cc:215: const std::string& method,
Move the method to just after the url.

[RyanSturm, 2016-02-17 21:46:13]

https://codereview.chromium.org/1684123004/diff/20001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/1684123004/diff/20001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:1019: url, net::LOAD_NORMAL, &info, nullptr, "",
net::BoundNetLog());
On 2016/02/16 20:00:13, bengr wrote:
> Since this entire file uses NULL, I would keep the null here. Add
std::string()
> instead of "".

Done.

https://codereview.chromium.org/1684123004/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc
(right):

https://codereview.chromium.org/1684123004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:52:
const std::string& method,
On 2016/02/16 20:00:13, bengr wrote:
> Move the method to just after the url.

Done.

https://codereview.chromium.org/1684123004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:117:
const std::string& method,
On 2016/02/16 20:00:13, bengr wrote:
> Move the method to just after the url.

Done.

https://codereview.chromium.org/1684123004/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.h
(right):

https://codereview.chromium.org/1684123004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.h:53:
const std::string& method,
On 2016/02/16 20:00:13, bengr wrote:
> Move the method to just after the url.

Done.

https://codereview.chromium.org/1684123004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.h:91:
const std::string& method,
On 2016/02/16 20:00:13, bengr wrote:
> Move the method to just after the url.

Done.

https://codereview.chromium.org/1684123004/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc
(right):

https://codereview.chromium.org/1684123004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc:385:
// we will fail the test rather than hitting a crash from the DCHECK in
On 2016/02/16 20:00:13, bengr wrote:
> Don't personify code and use complete sentences. I don't think your comment is
> useful btw.

I removed it

https://codereview.chromium.org/1684123004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc:393:
// POST methods go direct
On 2016/02/16 20:00:13, bengr wrote:
> direct -> direct.

Done.

https://codereview.chromium.org/1684123004/diff/20001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_network_proxy_host.cc (right):

https://codereview.chromium.org/1684123004/diff/20001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_network_proxy_host.cc:153: nullptr,
On 2016/02/16 20:00:13, bengr wrote:
> Use NULL and std::string().

Done.

https://codereview.chromium.org/1684123004/diff/20001/content/browser/resolve...
File content/browser/resolve_proxy_msg_helper.cc (right):

https://codereview.chromium.org/1684123004/diff/20001/content/browser/resolve...
content/browser/resolve_proxy_msg_helper.cc:98: &req.pac_req, nullptr, "",
net::BoundNetLog());
On 2016/02/16 20:00:13, bengr wrote:
> NULL and std::string()

Done.

https://codereview.chromium.org/1684123004/diff/20001/google_apis/gcm/engine/...
File google_apis/gcm/engine/connection_factory_impl.cc (right):

https://codereview.chromium.org/1684123004/diff/20001/google_apis/gcm/engine/...
google_apis/gcm/engine/connection_factory_impl.cc:315: nullptr,
On 2016/02/16 20:00:13, bengr wrote:
> NULL and std::string() here and at 538.

Done.

https://codereview.chromium.org/1684123004/diff/20001/jingle/glue/proxy_resol...
File jingle/glue/proxy_resolving_client_socket.cc (right):

https://codereview.chromium.org/1684123004/diff/20001/jingle/glue/proxy_resol...
jingle/glue/proxy_resolving_client_socket.cc:148: nullptr,
On 2016/02/16 20:00:13, bengr wrote:
> NULL, std::string()

Done.

https://codereview.chromium.org/1684123004/diff/20001/jingle/glue/proxy_resol...
jingle/glue/proxy_resolving_client_socket.cc:297: proxy_resolve_callback_,
&pac_request_, nullptr, "", bound_net_log_);
On 2016/02/16 20:00:13, bengr wrote:
> NULL, std::string()

Done.

https://codereview.chromium.org/1684123004/diff/20001/net/base/proxy_delegate.h
File net/base/proxy_delegate.h (right):

https://codereview.chromium.org/1684123004/diff/20001/net/base/proxy_delegate...
net/base/proxy_delegate.h:38: const std::string& method,
On 2016/02/16 20:00:13, bengr wrote:
> Move the method to just after the url.

Done.

https://codereview.chromium.org/1684123004/diff/20001/net/base/test_proxy_del...
File net/base/test_proxy_delegate.cc (right):

https://codereview.chromium.org/1684123004/diff/20001/net/base/test_proxy_del...
net/base/test_proxy_delegate.cc:45: const std::string& method,
On 2016/02/16 20:00:13, bengr wrote:
> Move the method to just after the url.

Done.

https://codereview.chromium.org/1684123004/diff/20001/net/base/test_proxy_del...
File net/base/test_proxy_delegate.h (right):

https://codereview.chromium.org/1684123004/diff/20001/net/base/test_proxy_del...
net/base/test_proxy_delegate.h:56: const std::string& method,
On 2016/02/16 20:00:13, bengr wrote:
> Move the method to just after the url.

Done.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:775: const std::string& method)
On 2016/02/16 20:00:14, bengr wrote:
> Move the method to just after the url.

Done.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:924: std::string method_;
On 2016/02/16 20:00:14, bengr wrote:
> Move this up to before load_flags_ so as to group together all inputs to proxy
> resolution.

Done.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1020: const std::string& method,
On 2016/02/16 20:00:14, bengr wrote:
> Move the method to just after the url.

Done.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1033: const std::string& method,
On 2016/02/16 20:00:14, bengr wrote:
> Move the method to just after the url.

Done.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1094: const std::string& method,
On 2016/02/16 20:00:14, bengr wrote:
> Move the method to just after the url.

Done.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1263: const std::string& method,
On 2016/02/16 20:00:14, bengr wrote:
> Move the method to just after the url.

Done.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.cc:1361: const std::string& method,
On 2016/02/16 20:00:14, bengr wrote:
> Move the method to just after the url.

Done.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.h:135: const std::string& method,
On 2016/02/16 20:00:14, bengr wrote:
> Move the method to just after the url.

Done.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.h:167: ProxyDelegate* proxy_delegate,
On 2016/02/16 20:00:14, bengr wrote:
> Move the method to just after the url.

Done.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.h:344: const std::string& method,
On 2016/02/16 20:00:14, bengr wrote:
> Move the method to just after the url.

Done.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service.h:367: const std::string& method,
On 2016/02/16 20:00:14, bengr wrote:
> Move the method to just after the url.

Done.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:176: const std::string& method,
On 2016/02/16 20:00:14, bengr wrote:
> Move the method to just after the url.

Done.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:237: const std::string& method,
On 2016/02/16 20:00:14, bengr wrote:
> Move the method to just after the url.

Done.

https://codereview.chromium.org/1684123004/diff/20001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:359: nullptr, nullptr, "", log.bound());
On 2016/02/16 20:00:14, bengr wrote:
> Use std::string() here and below.

Done.

https://codereview.chromium.org/1684123004/diff/20001/net/websockets/websocke...
File net/websockets/websocket_end_to_end_test.cc (right):

https://codereview.chromium.org/1684123004/diff/20001/net/websockets/websocke...
net/websockets/websocket_end_to_end_test.cc:215: const std::string& method,
On 2016/02/16 20:00:14, bengr wrote:
> Move the method to just after the url.

Done.

[bengr, 2016-02-21 21:23:13]

Description was changed from

==========
Bypass the DataReductionProxy for all POST requests

POST requests are non-dempotent, so clients using flywheel can often end up at
an error page due to the fact that if the flywheel proxy has any problems
connecting to the origin, flywheel and the client will not be safely able to
determine if the post successfully made it to the server. The impact of this
change on data compression should be very limited, and it will improve the
client's experience.

BUG=581750
==========

to

==========
Bypass the DataReductionProxy for all POST requests

POST requests are non-dempotent, so clients using the Data Reduction Proxy (DRP)
can often end up at an error page due to the fact that if the DRP has any
problems connecting to the origin, DRP and the client will not be safely able to
determine if the post successfully made it to the server. The impact of this
change on data compression should be very limited, and it will improve the
client's experience.

BUG=581750
==========

[bengr, 2016-02-21 21:36:40]

https://codereview.chromium.org/1684123004/diff/40001/content/browser/resolve...
File content/browser/resolve_proxy_msg_helper.cc (right):

https://codereview.chromium.org/1684123004/diff/40001/content/browser/resolve...
content/browser/resolve_proxy_msg_helper.cc:95: req.url, std::string(),
net::LOAD_NORMAL, &proxy_info_,
Why not always pass in the method?

https://codereview.chromium.org/1684123004/diff/40001/net/base/proxy_delegate.h
File net/base/proxy_delegate.h (right):

https://codereview.chromium.org/1684123004/diff/40001/net/base/proxy_delegate...
net/base/proxy_delegate.h:36: const std::string& method,
Say something in the comment about what |method| is.

https://codereview.chromium.org/1684123004/diff/40001/net/base/test_proxy_del...
File net/base/test_proxy_delegate.cc (right):

https://codereview.chromium.org/1684123004/diff/40001/net/base/test_proxy_del...
net/base/test_proxy_delegate.cc:87: }  // namespace net
And here?

https://codereview.chromium.org/1684123004/diff/40001/net/base/test_proxy_del...
File net/base/test_proxy_delegate.h (right):

https://codereview.chromium.org/1684123004/diff/40001/net/base/test_proxy_del...
net/base/test_proxy_delegate.h:58: 
Remove the blank line.

https://codereview.chromium.org/1684123004/diff/40001/net/base/test_proxy_del...
net/base/test_proxy_delegate.h:88: #endif  // NET_BASE_TEST_PROXY_DELEGATE_H_
Did you add whitespace here?

https://codereview.chromium.org/1684123004/diff/40001/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

https://codereview.chromium.org/1684123004/diff/40001/net/proxy/proxy_service...
net/proxy/proxy_service.h:130: const std::string& method,
Add a description of |method| to the comment.

https://codereview.chromium.org/1684123004/diff/40001/net/proxy/proxy_service...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/1684123004/diff/40001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:360: service.ResolveProxy(url,
std::string(), LOAD_NORMAL, &info,
#include <string>

https://codereview.chromium.org/1684123004/diff/40001/net/websockets/websocke...
File net/websockets/websocket_end_to_end_test.cc (right):

https://codereview.chromium.org/1684123004/diff/40001/net/websockets/websocke...
net/websockets/websocket_end_to_end_test.cc:11: #include <stdint.h>
Add a blank line after this.

[RyanSturm, 2016-02-26 22:16:19]

https://codereview.chromium.org/1684123004/diff/40001/content/browser/resolve...
File content/browser/resolve_proxy_msg_helper.cc (right):

https://codereview.chromium.org/1684123004/diff/40001/content/browser/resolve...
content/browser/resolve_proxy_msg_helper.cc:95: req.url, std::string(),
net::LOAD_NORMAL, &proxy_info_,
On 2016/02/21 21:36:39, bengr wrote:
> Why not always pass in the method?

This would involve massive refactoring to get the method from either Pepper or
WebPlugin, and it would provide little benefit right now, as this call does not
pass in a ProxyDelegate, which is the only place that the method is consumed.

https://codereview.chromium.org/1684123004/diff/40001/net/base/proxy_delegate.h
File net/base/proxy_delegate.h (right):

https://codereview.chromium.org/1684123004/diff/40001/net/base/proxy_delegate...
net/base/proxy_delegate.h:36: const std::string& method,
On 2016/02/21 21:36:39, bengr wrote:
> Say something in the comment about what |method| is.

Done.

https://codereview.chromium.org/1684123004/diff/40001/net/base/test_proxy_del...
File net/base/test_proxy_delegate.cc (right):

https://codereview.chromium.org/1684123004/diff/40001/net/base/test_proxy_del...
net/base/test_proxy_delegate.cc:87: }  // namespace net
On 2016/02/21 21:36:39, bengr wrote:
> And here?

Done.

https://codereview.chromium.org/1684123004/diff/40001/net/base/test_proxy_del...
File net/base/test_proxy_delegate.h (right):

https://codereview.chromium.org/1684123004/diff/40001/net/base/test_proxy_del...
net/base/test_proxy_delegate.h:58: 
On 2016/02/21 21:36:39, bengr wrote:
> Remove the blank line.

Done.

https://codereview.chromium.org/1684123004/diff/40001/net/base/test_proxy_del...
net/base/test_proxy_delegate.h:88: #endif  // NET_BASE_TEST_PROXY_DELEGATE_H_
On 2016/02/21 21:36:39, bengr wrote:
> Did you add whitespace here?

Now that I'm not using Eclipse the auto-formatting should stop.

https://codereview.chromium.org/1684123004/diff/40001/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

https://codereview.chromium.org/1684123004/diff/40001/net/proxy/proxy_service...
net/proxy/proxy_service.h:130: const std::string& method,
On 2016/02/21 21:36:40, bengr wrote:
> Add a description of |method| to the comment.

Done.

https://codereview.chromium.org/1684123004/diff/40001/net/proxy/proxy_service...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/1684123004/diff/40001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:360: service.ResolveProxy(url,
std::string(), LOAD_NORMAL, &info,
On 2016/02/21 21:36:40, bengr wrote:
> #include <string>

Done.

https://codereview.chromium.org/1684123004/diff/40001/net/websockets/websocke...
File net/websockets/websocket_end_to_end_test.cc (right):

https://codereview.chromium.org/1684123004/diff/40001/net/websockets/websocke...
net/websockets/websocket_end_to_end_test.cc:11: #include <stdint.h>
On 2016/02/21 21:36:40, bengr wrote:
> Add a blank line after this.

Done.

[bengr, 2016-02-26 23:43:33]

Lgtm, but please add the comment and the includes.

https://codereview.chromium.org/1684123004/diff/60001/google_apis/gcm/engine/...
File google_apis/gcm/engine/connection_factory_impl.cc (right):

https://codereview.chromium.org/1684123004/diff/60001/google_apis/gcm/engine/...
google_apis/gcm/engine/connection_factory_impl.cc:310: std::string(),
#include <string>

https://codereview.chromium.org/1684123004/diff/60001/jingle/glue/proxy_resol...
File jingle/glue/proxy_resolving_client_socket.cc (right):

https://codereview.chromium.org/1684123004/diff/60001/jingle/glue/proxy_resol...
jingle/glue/proxy_resolving_client_socket.cc:142: std::string(),
#include <string>

https://codereview.chromium.org/1684123004/diff/60001/net/base/proxy_delegate.h
File net/base/proxy_delegate.h (right):

https://codereview.chromium.org/1684123004/diff/60001/net/base/proxy_delegate...
net/base/proxy_delegate.h:32: // Called as the proxy is being resolved for |url|
for a |method| request.
You need to say somewhere (here, but maybe also other places) that the caller
may pass in an empty string for the method to request method-agnostic
resolution.

https://codereview.chromium.org/1684123004/diff/60001/net/base/proxy_delegate...
net/base/proxy_delegate.h:37: const std::string& method,
#include <string>

https://codereview.chromium.org/1684123004/diff/60001/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

https://codereview.chromium.org/1684123004/diff/60001/net/proxy/proxy_service...
net/proxy/proxy_service.h:133: const std::string& method,
#include <string>

[RyanSturm, 2016-02-29 21:03:33]

On 2016/02/26 23:43:33, bengr wrote:
> Lgtm, but please add the comment and the includes.
> 
>
https://codereview.chromium.org/1684123004/diff/60001/google_apis/gcm/engine/...
> File google_apis/gcm/engine/connection_factory_impl.cc (right):
> 
>
https://codereview.chromium.org/1684123004/diff/60001/google_apis/gcm/engine/...
> google_apis/gcm/engine/connection_factory_impl.cc:310: std::string(),
> #include <string>
> 
>
https://codereview.chromium.org/1684123004/diff/60001/jingle/glue/proxy_resol...
> File jingle/glue/proxy_resolving_client_socket.cc (right):
> 
>
https://codereview.chromium.org/1684123004/diff/60001/jingle/glue/proxy_resol...
> jingle/glue/proxy_resolving_client_socket.cc:142: std::string(),
> #include <string>
> 
>
https://codereview.chromium.org/1684123004/diff/60001/net/base/proxy_delegate.h
> File net/base/proxy_delegate.h (right):
> 
>
https://codereview.chromium.org/1684123004/diff/60001/net/base/proxy_delegate...
> net/base/proxy_delegate.h:32: // Called as the proxy is being resolved for
|url|
> for a |method| request.
> You need to say somewhere (here, but maybe also other places) that the caller
> may pass in an empty string for the method to request method-agnostic
> resolution.
> 
>
https://codereview.chromium.org/1684123004/diff/60001/net/base/proxy_delegate...
> net/base/proxy_delegate.h:37: const std::string& method,
> #include <string>
> 
>
https://codereview.chromium.org/1684123004/diff/60001/net/proxy/proxy_service.h
> File net/proxy/proxy_service.h (right):
> 
>
https://codereview.chromium.org/1684123004/diff/60001/net/proxy/proxy_service...
> net/proxy/proxy_service.h:133: const std::string& method,
> #include <string>

done

[RyanSturm, 2016-03-01 00:00:34]

ryansturm@chromium.org changed reviewers:
+ hashimoto@chromium.org, jochen@chromium.org, mmenke@chromium.org,
zea@chromium.org

[RyanSturm, 2016-03-01 00:00:35]

Adding code reviewers for specific areas.

[Nicolas Zea, 2016-03-01 00:05:46]

jingle and gcm LGTM

[jochen (gone - plz use gerrit), 2016-03-01 14:39:51]

i'm not a very specific reviewer. what do you want me to look at?

[RyanSturm, 2016-03-01 15:20:58]

On 2016/03/01 14:39:51, jochen wrote:
> i'm not a very specific reviewer. what do you want me to look at?

I added you (jochen) for pepper_network_proxy_host and resolve_proxy_msg_helper.

[jochen (gone - plz use gerrit), 2016-03-01 15:43:11]

On 2016/03/01 at 15:20:58, ryansturm wrote:
> On 2016/03/01 14:39:51, jochen wrote:
> > i'm not a very specific reviewer. what do you want me to look at?
> 
> I added you (jochen) for pepper_network_proxy_host and
resolve_proxy_msg_helper.

content/ lgtm

[hashimoto, 2016-03-02 05:36:26]

I assume you are asking me to review chromeos/dbus.
It'd be appreciated if you could tell each reviewer which directory to review
next time :)

chromeos/dbus lgtm

[hashimoto, 2016-03-02 05:47:59]

Actually, lgtm with a nit.

https://codereview.chromium.org/1684123004/diff/100001/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

https://codereview.chromium.org/1684123004/diff/100001/net/proxy/proxy_servic...
net/proxy/proxy_service.h:110: // Determines the appropriate proxy for |url| for
a |method| request and
nit: How about adding a description about what happens when |method| is empty
like the one in proxy_delegate.h?
It seems almost all ProxyService users are passing empty strings.

[mmenke, 2016-03-02 22:53:45]

Sorry for slowness.  A lot of reviews this week, as usual.  I'll do a second
quick pass tomorrow, and then sign off.

https://codereview.chromium.org/1684123004/diff/120001/net/proxy/proxy_servic...
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/1684123004/diff/120001/net/proxy/proxy_servic...
net/proxy/proxy_service.cc:1018: DCHECK((proxy_delegate && !method.empty()) ||
!proxy_delegate);
Why this DCHECK?  The docs say it can be empty.

https://codereview.chromium.org/1684123004/diff/120001/net/proxy/proxy_servic...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/1684123004/diff/120001/net/proxy/proxy_servic...
net/proxy/proxy_service_unittest.cc:185: result->UseDirect();
Should record the method, and have a test that makes sure we send it.  Can just
modify OnResolveProxyCallbackAddProxy(), or add a new one.

https://codereview.chromium.org/1684123004/diff/120001/net/url_request/url_re...
File net/url_request/url_request_ftp_job.cc (right):

https://codereview.chromium.org/1684123004/diff/120001/net/url_request/url_re...
net/url_request/url_request_ftp_job.cc:104: request_->url(), request_->method(),
request_->load_flags(),
Methods are meaningless for FTP requests.  Should just send a GET, since that's
the only method we use support for proxied FTP requests.

[RyanSturm, 2016-03-03 18:54:31]

https://codereview.chromium.org/1684123004/diff/120001/net/proxy/proxy_servic...
File net/proxy/proxy_service.cc (right):

https://codereview.chromium.org/1684123004/diff/120001/net/proxy/proxy_servic...
net/proxy/proxy_service.cc:1018: DCHECK((proxy_delegate && !method.empty()) ||
!proxy_delegate);
On 2016/03/02 22:53:45, mmenke wrote:
> Why this DCHECK?  The docs say it can be empty.

Done.

https://codereview.chromium.org/1684123004/diff/120001/net/proxy/proxy_servic...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/1684123004/diff/120001/net/proxy/proxy_servic...
net/proxy/proxy_service_unittest.cc:185: result->UseDirect();
On 2016/03/02 22:53:45, mmenke wrote:
> Should record the method, and have a test that makes sure we send it.  Can
just
> modify OnResolveProxyCallbackAddProxy(), or add a new one.

Done.

https://codereview.chromium.org/1684123004/diff/120001/net/url_request/url_re...
File net/url_request/url_request_ftp_job.cc (right):

https://codereview.chromium.org/1684123004/diff/120001/net/url_request/url_re...
net/url_request/url_request_ftp_job.cc:104: request_->url(), request_->method(),
request_->load_flags(),
On 2016/03/02 22:53:45, mmenke wrote:
> Methods are meaningless for FTP requests.  Should just send a GET, since
that's
> the only method we use support for proxied FTP requests.

Done.

[mmenke, 2016-03-03 19:05:53]

net/, chrome/browser/net LGTM

https://codereview.chromium.org/1684123004/diff/140001/net/proxy/proxy_servic...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/1684123004/diff/140001/net/proxy/proxy_servic...
net/proxy/proxy_service_unittest.cc:172: method_(),
nit:  Explicit initialization not needed for std::string.

https://codereview.chromium.org/1684123004/diff/140001/net/proxy/proxy_servic...
net/proxy/proxy_service_unittest.cc:195: std::string method() const { return
method_; }
nit:  const std::string&

[RyanSturm, 2016-03-03 19:33:08]

https://codereview.chromium.org/1684123004/diff/140001/net/proxy/proxy_servic...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/1684123004/diff/140001/net/proxy/proxy_servic...
net/proxy/proxy_service_unittest.cc:172: method_(),
On 2016/03/03 19:05:53, mmenke wrote:
> nit:  Explicit initialization not needed for std::string.

Done.

https://codereview.chromium.org/1684123004/diff/140001/net/proxy/proxy_servic...
net/proxy/proxy_service_unittest.cc:195: std::string method() const { return
method_; }
On 2016/03/03 19:05:53, mmenke wrote:
> nit:  const std::string&

Done.

[RyanSturm, 2016-03-03 23:30:27]

The CQ bit was checked by ryansturm@chromium.org

[RyanSturm, 2016-03-03 23:30:28]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org,
hashimoto@chromium.org, jochen@chromium.org, zea@chromium.org,
mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1684123004/#ps200001
(title: "Fix Rebase issue")

[commit-bot: I haz the power, 2016-03-03 23:31:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1684123004/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1684123004/200001

[commit-bot: I haz the power, 2016-03-03 23:41:15]

Description was changed from

==========
Bypass the DataReductionProxy for all POST requests

POST requests are non-dempotent, so clients using the Data Reduction Proxy (DRP)
can often end up at an error page due to the fact that if the DRP has any
problems connecting to the origin, DRP and the client will not be safely able to
determine if the post successfully made it to the server. The impact of this
change on data compression should be very limited, and it will improve the
client's experience.

BUG=581750
==========

to

==========
Bypass the DataReductionProxy for all POST requests

POST requests are non-dempotent, so clients using the Data Reduction Proxy (DRP)
can often end up at an error page due to the fact that if the DRP has any
problems connecting to the origin, DRP and the client will not be safely able to
determine if the post successfully made it to the server. The impact of this
change on data compression should be very limited, and it will improve the
client's experience.

BUG=581750
==========

[commit-bot: I haz the power, 2016-03-03 23:41:17]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-03-03 23:43:59]

Description was changed from

==========
Bypass the DataReductionProxy for all POST requests

POST requests are non-dempotent, so clients using the Data Reduction Proxy (DRP)
can often end up at an error page due to the fact that if the DRP has any
problems connecting to the origin, DRP and the client will not be safely able to
determine if the post successfully made it to the server. The impact of this
change on data compression should be very limited, and it will improve the
client's experience.

BUG=581750
==========

to

==========
Bypass the DataReductionProxy for all POST requests

POST requests are non-dempotent, so clients using the Data Reduction Proxy (DRP)
can often end up at an error page due to the fact that if the DRP has any
problems connecting to the origin, DRP and the client will not be safely able to
determine if the post successfully made it to the server. The impact of this
change on data compression should be very limited, and it will improve the
client's experience.

BUG=581750

Committed: https://crrev.com/4bab0683f19f749d1974944281c07abbc0d814cf
Cr-Commit-Position: refs/heads/master@{#379137}
==========

[commit-bot: I haz the power, 2016-03-03 23:44:01]

Patchset 11 (id:??) landed as
https://crrev.com/4bab0683f19f749d1974944281c07abbc0d814cf
Cr-Commit-Position: refs/heads/master@{#379137}