Enable an option to only use the secure Data Reduction Proxy when the secure proxy check succeeds.

Enable an option to only use the secure Data Reduction Proxy when the secure proxy check succeeds.

Instead of being optimistic about using the secure proxy (i.e. use it unless
the check fails), flip the logic so that the secure proxy is not used unless
the check succeeds.

BUG=466753
Committed: https://crrev.com/2f0a5eba99e3d233b0c2a6a65402a5dbefa5537d
Cr-Commit-Position: refs/heads/master@{#329961}

[jeremyim, 2015-05-12 03:37:33]

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

[jeremyim, 2015-05-12 03:37:34]

PTAL!

[bengr, 2015-05-12 17:16:15]

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

https://codereview.chromium.org/1132083004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:109:
DataReductionProxyParams::ShouldUseSecureProxyByDefault()),
Names. Maybe this should be called secure_proxy_check_required_ and the method
should be called ShouldDoSecureProxyCheck().

https://codereview.chromium.org/1132083004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:364:
// This must stay a LOG(WARNING); the output is used in processing customer
While you're here, please move this comment down to 376.

https://codereview.chromium.org/1132083004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:407:
secure_proxy_allowed_ = true;
Why would we change this to allowed?

https://codereview.chromium.org/1132083004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:447:
bool should_use_secure_proxy =
This logic seems too complicated. Can't we just have a state variable that
tracks where we are in doing the secure proxy check and a policy variable that
lets us know if we're allowed to do the check? Alternatively, the state variable
can hold the policy too:

SECURE_PROXY_CHECK_DISALLOWED
SECURE_PROXY_CHECK_BEFORE_REQUEST
SECURE_PROXY_CHECK_PENDING
SECURE_PROXY_CHECK_COMPLETED

or something like that.

https://codereview.chromium.org/1132083004/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_switches.cc
(right):

https://codereview.chromium.org/1132083004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_switches.cc:43:
"data-reduction-proxy-secure-proxy-disabled";
Will this be tied to your larger experiment?

[jeremyim, 2015-05-12 20:57:59]

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

https://codereview.chromium.org/1132083004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:109:
DataReductionProxyParams::ShouldUseSecureProxyByDefault()),
On 2015/05/12 17:16:14, bengr wrote:
> Names. Maybe this should be called secure_proxy_check_required_ and the method
> should be called ShouldDoSecureProxyCheck().

Added a comment in the header:
"Indicates if the secure Data Reduction Proxy can be used or not."

As such, it does not indicate whether the check is required (it is always
required), but whether we are permitted to use it.

https://codereview.chromium.org/1132083004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:364:
// This must stay a LOG(WARNING); the output is used in processing customer
On 2015/05/12 17:16:15, bengr wrote:
> While you're here, please move this comment down to 376.

Done.

https://codereview.chromium.org/1132083004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:407:
secure_proxy_allowed_ = true;
On 2015/05/12 17:16:15, bengr wrote:
> Why would we change this to allowed?

If the secure proxy check succeeds, we are now "allowed" to use it.

https://codereview.chromium.org/1132083004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:447:
bool should_use_secure_proxy =
On 2015/05/12 17:16:15, bengr wrote:
> This logic seems too complicated. Can't we just have a state variable that
> tracks where we are in doing the secure proxy check and a policy variable that
> lets us know if we're allowed to do the check? Alternatively, the state
variable
> can hold the policy too:
> 
> SECURE_PROXY_CHECK_DISALLOWED
> SECURE_PROXY_CHECK_BEFORE_REQUEST
> SECURE_PROXY_CHECK_PENDING
> SECURE_PROXY_CHECK_COMPLETED
> 
> or something like that.

The nuances take place because of:
- What was the previous state of the secure proxy? (If it was previously allowed
but it is disabled until the check succeeds, then we must reload the config and
disable the secure proxy)
- What is the desired behavior? (If the secure proxy check is enabled until the
check fails, then we continue to use the configuration as before)

https://codereview.chromium.org/1132083004/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_switches.cc
(right):

https://codereview.chromium.org/1132083004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_switches.cc:43:
"data-reduction-proxy-secure-proxy-disabled";
On 2015/05/12 17:16:15, bengr wrote:
> Will this be tied to your larger experiment?

It will be tied to a separate experiment.

[bengr, 2015-05-14 15:50:34]

https://codereview.chromium.org/1132083004/diff/40001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://codereview.chromium.org/1132083004/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:268:
// Indicates if the secure Data Reduction Proxy can be used or not.
Oy. So iiuc, this can be true even after the check fails, at which time the
secure proxy is not allowed to be used.

[jeremyim, 2015-05-14 15:55:33]

https://codereview.chromium.org/1132083004/diff/40001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://codereview.chromium.org/1132083004/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:268:
// Indicates if the secure Data Reduction Proxy can be used or not.
On 2015/05/14 15:50:33, bengr wrote:
> Oy. So iiuc, this can be true even after the check fails, at which time the
> secure proxy is not allowed to be used.

If the check fails, it is set to false (if not already false); it's basically a
rename of "restricted_by_carrier_" to its inverse (since with this change, it
now means "not restricted by carrier" since it depends on the default state).

[bengr, 2015-05-14 18:25:08]

lgtm

[jeremyim, 2015-05-14 18:34:28]

jeremyim@chromium.org changed reviewers:
+ asvitkine@chromium.org

[jeremyim, 2015-05-14 18:34:28]

asvitkine: Please review changes in histograms.xml

Thanks!

[Alexei Svitkine (slow), 2015-05-14 20:40:41]

lgtm

[jeremyim, 2015-05-14 21:20:57]

The CQ bit was checked by jeremyim@chromium.org

[commit-bot: I haz the power, 2015-05-14 21:21:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1132083004/40001

[commit-bot: I haz the power, 2015-05-14 21:30:46]

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

[commit-bot: I haz the power, 2015-05-14 21:30:46]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jeremyim, 2015-05-14 21:37:40]

The CQ bit was checked by jeremyim@chromium.org

[jeremyim, 2015-05-14 21:37:41]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1132083004/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2015-05-14 21:39:53]

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

[commit-bot: I haz the power, 2015-05-14 22:40:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-14 22:40:52]

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