Add revocation checking field trial.

Add revocation checking field trial.

We wish to measure the impact of revocation checking on SSL connection times.
This change sets up a 50/50 field trial where members of the trial don't do
revocation checking on sites that we currently have certificate pins for.

BUG=none
TEST=none

[agl, 2011-06-06 16:58:55]

Note that this also moves several histograms out of a !using_spdy conditional. I
don't believe that they should have been in there.

[agl, 2011-06-07 21:47:42]

> Description:
> Add revocation checking field trial.

bump.

[Mike Belshe, 2011-06-08 04:50:38]

http://codereview.chromium.org/7113008/diff/1/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/7113008/diff/1/chrome/browser/browser_main.cc#...
chrome/browser/browser_main.cc:512: "RevCheckingImpact", kDivisor,
"RevChecking_control", 2011, 8, 30));
nit:  change "RevChecking_control" to "_control"?

I hope this will only be for pinned sites too, right?  Otherwise, this isn't a
strict A/B test.

http://codereview.chromium.org/7113008/diff/1/chrome/browser/browser_main.cc#...
chrome/browser/browser_main.cc:515: "RevChecking_disabled_for_pinned_sites",
probability);
nit:  change to "_disabled"

http://codereview.chromium.org/7113008/diff/1/net/socket/ssl_client_socket_po...
File net/socket/ssl_client_socket_pool.cc (right):

http://codereview.chromium.org/7113008/diff/1/net/socket/ssl_client_socket_po...
net/socket/ssl_client_socket_pool.cc:334:
UMA_HISTOGRAM_CUSTOM_TIMES("Net.SSL_Connection_Latency",
note:  you're changing the definition of these histograms... Before they were
"!spdy", but now they are everything - spdy or not.  I think this is ok....

http://codereview.chromium.org/7113008/diff/1/net/socket/ssl_client_socket_po...
net/socket/ssl_client_socket_pool.cc:354: if (rev_checking_trial) {
I think this is a problem.

You will compare:
a) sites which could have been pinned but which we overrode.
to 
b) all other sites that could have been pinned or not

[agl, 2011-06-08 18:57:12]

PTAL

http://codereview.chromium.org/7113008/diff/1/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/7113008/diff/1/chrome/browser/browser_main.cc#...
chrome/browser/browser_main.cc:512: "RevCheckingImpact", kDivisor,
"RevChecking_control", 2011, 8, 30));
On 2011/06/08 04:50:38, Mike Belshe wrote:
> nit:  change "RevChecking_control" to "_control"?

Have changed to "control" since none of the other users of FieldTrial in this
file prepend underscores. Please let me know if there's something that I missed.

http://codereview.chromium.org/7113008/diff/1/chrome/browser/browser_main.cc#...
chrome/browser/browser_main.cc:515: "RevChecking_disabled_for_pinned_sites",
probability);
On 2011/06/08 04:50:38, Mike Belshe wrote:
> nit:  change to "_disabled"

Done (with the same remark about the leading underscore).

http://codereview.chromium.org/7113008/diff/1/net/socket/ssl_client_socket_po...
File net/socket/ssl_client_socket_pool.cc (right):

http://codereview.chromium.org/7113008/diff/1/net/socket/ssl_client_socket_po...
net/socket/ssl_client_socket_pool.cc:334:
UMA_HISTOGRAM_CUSTOM_TIMES("Net.SSL_Connection_Latency",
On 2011/06/08 04:50:38, Mike Belshe wrote:
> note:  you're changing the definition of these histograms... Before they were
> "!spdy", but now they are everything - spdy or not.  I think this is ok....

Yes, that's was intentional.

http://codereview.chromium.org/7113008/diff/1/net/socket/ssl_client_socket_po...
net/socket/ssl_client_socket_pool.cc:354: if (rev_checking_trial) {
On 2011/06/08 04:50:38, Mike Belshe wrote:
> I think this is a problem.

Yes, I meant to put the histogram in the |is_google| condition. However, another
case occurred: a user could be outside the trail but have manually disabled
revocation checking. Please see the code now that I've addressed that and check
that my assumptions about field trials are correct.

[mbelshe, 2011-06-08 23:02:17]

LGTM

On Wed, Jun 8, 2011 at 11:57 AM, <agl@chromium.org> wrote:

> PTAL
>
>
>
> http://codereview.chromium.**org/7113008/diff/1/chrome/**
>
browser/browser_main.cc<http://codereview.chromium.org/7113008/diff/1/chrome/browser/browser_main.cc>
> File chrome/browser/browser_main.cc (right):
>
> http://codereview.chromium.**org/7113008/diff/1/chrome/**
>
browser/browser_main.cc#**newcode512<http://codereview.chromium.org/7113008/diff/1/chrome/browser/browser_main.cc#newcode512>
> chrome/browser/browser_main.**cc:512: "RevCheckingImpact", kDivisor,
> "RevChecking_control", 2011, 8, 30));
> On 2011/06/08 04:50:38, Mike Belshe wrote:
>
>> nit:  change "RevChecking_control" to "_control"?
>>
>
> Have changed to "control" since none of the other users of FieldTrial in
> this file prepend underscores. Please let me know if there's something
> that I missed.
>
>
> http://codereview.chromium.**org/7113008/diff/1/chrome/**
>
browser/browser_main.cc#**newcode515<http://codereview.chromium.org/7113008/diff/1/chrome/browser/browser_main.cc#newcode515>
> chrome/browser/browser_main.**cc:515:
> "RevChecking_disabled_for_**pinned_sites", probability);
> On 2011/06/08 04:50:38, Mike Belshe wrote:
>
>> nit:  change to "_disabled"
>>
>
> Done (with the same remark about the leading underscore).
>
>
> http://codereview.chromium.**org/7113008/diff/1/net/socket/**
>
ssl_client_socket_pool.cc<http://codereview.chromium.org/7113008/diff/1/net/socket/ssl_client_socket_pool.cc>
> File net/socket/ssl_client_socket_**pool.cc (right):
>
> http://codereview.chromium.**org/7113008/diff/1/net/socket/**
>
ssl_client_socket_pool.cc#**newcode334<http://codereview.chromium.org/7113008/diff/1/net/socket/ssl_client_socket_pool.cc#newcode334>
> net/socket/ssl_client_socket_**pool.cc:334:
> UMA_HISTOGRAM_CUSTOM_TIMES("**Net.SSL_Connection_Latency",
> On 2011/06/08 04:50:38, Mike Belshe wrote:
>
>> note:  you're changing the definition of these histograms... Before
>>
> they were
>
>> "!spdy", but now they are everything - spdy or not.  I think this is
>>
> ok....
>
> Yes, that's was intentional.
>
>
> http://codereview.chromium.**org/7113008/diff/1/net/socket/**
>
ssl_client_socket_pool.cc#**newcode354<http://codereview.chromium.org/7113008/diff/1/net/socket/ssl_client_socket_pool.cc#newcode354>
> net/socket/ssl_client_socket_**pool.cc:354: if (rev_checking_trial) {
> On 2011/06/08 04:50:38, Mike Belshe wrote:
>
>> I think this is a problem.
>>
>
> Yes, I meant to put the histogram in the |is_google| condition. However,
> another case occurred: a user could be outside the trail but have
> manually disabled revocation checking. Please see the code now that I've
> addressed that and check that my assumptions about field trials are
> correct.
>
>
>
http://codereview.chromium.**org/7113008/<http://codereview.chromium.org/7113...
>

[commit-bot: I haz the power, 2011-06-09 14:26:50]

Presubmit check for 7113008-5001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
net/socket/ssl_client_socket_pool.cc,net/socket/ssl_client_socket_nss.cc,net/base/ssl_config_service.cc,net/base/ssl_config_service.h

Presubmit checks took 1.1s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!