Change POST-exclusion to idempotent-inclusion

Change POST-exclusion to idempotent-inclusion

The data saver proxy should only be used for idempotent methods, but
right now it only checks if the method is POST, which leaves out
other non-idempotent methods like PATCH. Instead of excluding POST and
PATCH, this will explicitly check if the method is GET, OPTIONS, HEAD,
PUT, DELETE, or TRACE.

BUG=614078
Committed: https://crrev.com/fbc93e59b496ff6ea423527d1a3579fa8a2d061c
Cr-Commit-Position: refs/heads/master@{#395502}

[RyanSturm, 2016-05-23 18:23:27]

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

[RyanSturm, 2016-05-23 18:23:28]

tbansal: PTAL

Let me know if you think I should use the existing check
DataReductionProxyBypassProtocol::IsRequestIdempotent, and refactor it to take a
method string instead of a request. If you think I should do that, is there are
better place for that static method to be declared/defined?

[tbansal1, 2016-05-23 18:48:42]

https://codereview.chromium.org/2001243002/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc
(right):

https://codereview.chromium.org/2001243002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:127:
method == "PUT" || method == "DELETE" || method == "TRACE")) {
This relies on //net giving us methods in upper case. So, it might be useful to
add a DCHECK:
DCHECK_EQ(method,  base::ToUpperASCII(method));

[RyanSturm, 2016-05-23 20:16:35]

https://codereview.chromium.org/2001243002/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc
(right):

https://codereview.chromium.org/2001243002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:127:
method == "PUT" || method == "DELETE" || method == "TRACE")) {
On 2016/05/23 18:48:42, tbansal1 wrote:
> This relies on //net giving us methods in upper case. So, it might be useful
to
> add a DCHECK:
> DCHECK_EQ(method,  base::ToUpperASCII(method));

This will break some distant tests that want to verify that only capitalized
methods have certain functionality. I think those probably give us enough
coverage to leave the DCHECK off here.

[tbansal1, 2016-05-23 20:34:48]

couple of nits.

https://codereview.chromium.org/2001243002/diff/40001/components/data_reducti...
File components/data_reduction_proxy/core/common/data_reduction_proxy_util.cc
(right):

https://codereview.chromium.org/2001243002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_util.cc:7:
#include "base/strings/string_util.h"
why is string_util.h needed?

https://codereview.chromium.org/2001243002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_util.cc:12: if
(method == "GET" || method == "OPTIONS" || method == "HEAD" ||
how about just: return method == "GET" || method == "..."...

[RyanSturm, 2016-05-23 20:38:26]

https://codereview.chromium.org/2001243002/diff/40001/components/data_reducti...
File components/data_reduction_proxy/core/common/data_reduction_proxy_util.cc
(right):

https://codereview.chromium.org/2001243002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_util.cc:7:
#include "base/strings/string_util.h"
On 2016/05/23 20:34:48, tbansal1 wrote:
> why is string_util.h needed?

Done.

https://codereview.chromium.org/2001243002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_util.cc:12: if
(method == "GET" || method == "OPTIONS" || method == "HEAD" ||
On 2016/05/23 20:34:48, tbansal1 wrote:
> how about just: return method == "GET" || method == "..."...

Done.

[tbansal1, 2016-05-23 20:41:06]

lgtm

[tbansal1, 2016-05-23 20:41:07]

lgtm

lgtm

[RyanSturm, 2016-05-23 21:38:49]

The CQ bit was checked by ryansturm@chromium.org

[commit-bot: I haz the power, 2016-05-23 21:39:28]

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

[RyanSturm, 2016-05-23 21:39:40]

The CQ bit was unchecked by ryansturm@chromium.org

[RyanSturm, 2016-05-23 21:40:48]

The CQ bit was checked by ryansturm@chromium.org

[commit-bot: I haz the power, 2016-05-23 21:41:31]

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

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

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-23 23:41:43]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[RyanSturm, 2016-05-24 01:13:33]

The CQ bit was checked by ryansturm@chromium.org

[commit-bot: I haz the power, 2016-05-24 01:13:58]

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

[commit-bot: I haz the power, 2016-05-24 01:18:20]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-24 01:19:47]

Description was changed from

==========
Change POST-exclusion to idempotent-inclusion

The data saver proxy should only be used for idempotent methods, but
right now it only checks if the method is POST, which leaves out
other non-idempotent methods like PATCH. Instead of excluding POST and
PATCH, this will explicitly check if the method is GET, OPTIONS, HEAD,
PUT, DELETE, or TRACE.

BUG=614078
==========

to

==========
Change POST-exclusion to idempotent-inclusion

The data saver proxy should only be used for idempotent methods, but
right now it only checks if the method is POST, which leaves out
other non-idempotent methods like PATCH. Instead of excluding POST and
PATCH, this will explicitly check if the method is GET, OPTIONS, HEAD,
PUT, DELETE, or TRACE.

BUG=614078

Committed: https://crrev.com/fbc93e59b496ff6ea423527d1a3579fa8a2d061c
Cr-Commit-Position: refs/heads/master@{#395502}
==========

[commit-bot: I haz the power, 2016-05-24 01:19:50]

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