Add additional UMA stats for remembering certificate decisions.

Add additional UMA stats for remembering certificate decisions.

In the new experiment for remembering certificate decisions by users, decisions
expire after a certain amount of time. This CL adds measures to explicitly
measure how ofter users change their mind after an expiration of their prior
decision.

BUG=262615
R=felt@chromium.org
TBR=gunsch@chromium.org,torne@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289787

[jww, 2014-08-07 23:48:56]

felt@, can you take a look at this as an initial run through? While we can use
the current interstitial stats to good effect for the cert memory experiment, I
want to add in an explicit one to see how often users change their mind when a
decision expires.

I decided to implement the stat as a separate UMA histogram rather than
redefining the general SSL reason one because of (a) stat bloat, and (b) this
will almost certainly be retired separately/first. Let me know if you disagree.

[felt, 2014-08-08 00:17:06]

https://codereview.chromium.org/450833002/diff/20001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/450833002/diff/20001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:105: enum SSLIsExpiredAndDecision {
Add a note warning people that this is a UMA histogram, e.g.,
// Events for UMA. Do not reorder or change!

https://codereview.chromium.org/450833002/diff/20001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:105: enum SSLIsExpiredAndDecision {
sort of a weird name for an enum, the "Is" makes it sound like it's a bool.
SSLDesicionAfterExpiration? SSLExpirationAndDecision?

https://codereview.chromium.org/450833002/diff/20001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:119: void
RecordSSLIsExpiredPageEventState(bool expired_but_previously_allowed,
it seems like this will count all non-overridable ones as
NOT_EXPRED_AND_DO_NOT_PROCEED? perhaps you ought to handle non-overridable ones
with a separate entry (or entries), or don't include them in the histogram at
all.

https://codereview.chromium.org/450833002/diff/20001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.h (right):

https://codereview.chromium.org/450833002/diff/20001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.h:124: // expried?
expired

[felt, 2014-08-08 00:18:01]

https://codereview.chromium.org/450833002/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/450833002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:9765: +    previous decision that had
expired or not.
this is rather long and hard to follow

[jww, 2014-08-08 15:55:45]

felt, please take another look and let me know if it looks okay. Feel free not
to lgtm but just that you think if it's okay to let others start reviewing.
Thanks!

https://codereview.chromium.org/450833002/diff/20001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/450833002/diff/20001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:105: enum SSLIsExpiredAndDecision {
On 2014/08/08 00:17:06, felt wrote:
> sort of a weird name for an enum, the "Is" makes it sound like it's a bool.
> SSLDesicionAfterExpiration? SSLExpirationAndDecision?

Done.

https://codereview.chromium.org/450833002/diff/20001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:105: enum SSLIsExpiredAndDecision {
On 2014/08/08 00:17:06, felt wrote:
> Add a note warning people that this is a UMA histogram, e.g.,
> // Events for UMA. Do not reorder or change!

Done.

https://codereview.chromium.org/450833002/diff/20001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:119: void
RecordSSLIsExpiredPageEventState(bool expired_but_previously_allowed,
On 2014/08/08 00:17:06, felt wrote:
> it seems like this will count all non-overridable ones as
> NOT_EXPRED_AND_DO_NOT_PROCEED? perhaps you ought to handle non-overridable
ones
> with a separate entry (or entries), or don't include them in the histogram at
> all.

Good call. I'll split them into
"interstitial.ssl.expired_and_decision.overridable" and
"interstitial.ssl.expired_and_decision.nonoverridable". I'd like to see if
there's a difference between the two.

https://codereview.chromium.org/450833002/diff/20001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.h (right):

https://codereview.chromium.org/450833002/diff/20001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.h:124: // expried?
On 2014/08/08 00:17:06, felt wrote:
> expired

Done.

https://codereview.chromium.org/450833002/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/450833002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:9765: +    previous decision that had
expired or not.
On 2014/08/08 00:18:00, felt wrote:
> this is rather long and hard to follow

I tried to simplify it, but I'm not sure if it's still confusing. Please check
out the new summary, and if you have any suggestions, I'm all ears.

[felt, 2014-08-08 17:55:34]

https://codereview.chromium.org/450833002/diff/40001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/450833002/diff/40001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:144:
UMA_HISTOGRAM_ENUMERATION(histogram,
Although this might compile, I don't think it will actually work because of how
the macros work. You'll need two separate UMA_HISTOGRAM_ENUMERATION invocations,
one for each histogram name.

https://codereview.chromium.org/450833002/diff/40001/content/public/browser/s...
File content/public/browser/ssl_host_state_delegate.h (right):

https://codereview.chromium.org/450833002/diff/40001/content/public/browser/s...
content/public/browser/ssl_host_state_delegate.h:39: bool*
expired_previous_decision) = 0;
why a bool* in some places and a bool in others?

[jww, 2014-08-12 04:51:03]

https://codereview.chromium.org/450833002/diff/40001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/450833002/diff/40001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:144:
UMA_HISTOGRAM_ENUMERATION(histogram,
On 2014/08/08 17:55:33, felt wrote:
> Although this might compile, I don't think it will actually work because of
how
> the macros work. You'll need two separate UMA_HISTOGRAM_ENUMERATION
invocations,
> one for each histogram name.

Done.

https://codereview.chromium.org/450833002/diff/40001/content/public/browser/s...
File content/public/browser/ssl_host_state_delegate.h (right):

https://codereview.chromium.org/450833002/diff/40001/content/public/browser/s...
content/public/browser/ssl_host_state_delegate.h:39: bool*
expired_previous_decision) = 0;
On 2014/08/08 17:55:33, felt wrote:
> why a bool* in some places and a bool in others?

The pointer ones are because they are return values. I'll clarify that in the
comment.

[felt, 2014-08-12 04:54:33]

https://codereview.chromium.org/450833002/diff/40001/content/public/browser/s...
File content/public/browser/ssl_host_state_delegate.h (right):

https://codereview.chromium.org/450833002/diff/40001/content/public/browser/s...
content/public/browser/ssl_host_state_delegate.h:39: bool*
expired_previous_decision) = 0;
On 2014/08/12 04:51:02, jww wrote:
> On 2014/08/08 17:55:33, felt wrote:
> > why a bool* in some places and a bool in others?
> 
> The pointer ones are because they are return values. I'll clarify that in the
> comment.

Oh, no, you don't need to. I see now.

[felt, 2014-08-12 04:56:13]

lgtm, onwards

https://codereview.chromium.org/450833002/diff/60001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/450833002/diff/60001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:133: if (overridable)
style nit: I would expect { } here now that it's a multi-line if-else

[jww, 2014-08-12 13:15:52]

jar@, can you look at tools/metrics?

sky@, can you owner review chrome and content?

https://codereview.chromium.org/450833002/diff/60001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/450833002/diff/60001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:133: if (overridable)
On 2014/08/12 04:56:12, felt wrote:
> style nit: I would expect { } here now that it's a multi-line if-else

Done.

[sky, 2014-08-12 16:47:32]

I'm not an owner of content/public.

https://codereview.chromium.org/450833002/diff/80001/chrome/browser/ssl/chrom...
File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right):

https://codereview.chromium.org/450833002/diff/80001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:191: expired = true;
Why did you move this AFAICT if it's only needed after the early return.

https://codereview.chromium.org/450833002/diff/80001/chrome/browser/ssl/chrom...
File chrome/browser/ssl/chrome_ssl_host_state_delegate.h (right):

https://codereview.chromium.org/450833002/diff/80001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_delegate.h:99: //
|expired_previous_decision| receives a true value if there had been a
nit: 'receives a true value' -> is set to true

https://codereview.chromium.org/450833002/diff/80001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.h (right):

https://codereview.chromium.org/450833002/diff/80001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.h:63: bool overridable,
A bitmask would make this code more readable.

https://codereview.chromium.org/450833002/diff/80001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.h:125: bool
expired_but_previously_allowed_;
const? Probably on all similar members.

https://codereview.chromium.org/450833002/diff/80001/content/browser/ssl/ssl_...
File content/browser/ssl/ssl_policy.h (right):

https://codereview.chromium.org/450833002/diff/80001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_policy.h:61: bool overridable,
Same comment about a bitmask here.

[felt, 2014-08-12 16:50:29]

https://codereview.chromium.org/450833002/diff/80001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.h (right):

https://codereview.chromium.org/450833002/diff/80001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.h:63: bool overridable,
On 2014/08/12 16:47:31, sky wrote:
> A bitmask would make this code more readable.

Do you mean condense overridable, strict_enforcement, and
expired_but_previously_allowed into a bitmask? They're held in separate member
variables so I'm not sure how much we'd gain from that?

[sky, 2014-08-12 17:16:51]

On Tue, Aug 12, 2014 at 9:50 AM,  <felt@chromium.org> wrote:
>
>
https://codereview.chromium.org/450833002/diff/80001/chrome/browser/ssl/ssl_b...
> File chrome/browser/ssl/ssl_blocking_page.h (right):
>
>
https://codereview.chromium.org/450833002/diff/80001/chrome/browser/ssl/ssl_b...
> chrome/browser/ssl/ssl_blocking_page.h:63: bool overridable,
> On 2014/08/12 16:47:31, sky wrote:
>>
>> A bitmask would make this code more readable.
>
>
> Do you mean condense overridable, strict_enforcement, and
> expired_but_previously_allowed into a bitmask?

Yes.

> They're held in separate
> member variables so I'm not sure how much we'd gain from that?

That's ok. The benefit of bitmasks is it makes code like this
foo(true, false, true) more readable, eg foo(ENABLE_1 | ENABLE_2).
Especially when you see people right the multi-boolean variants as:
  foo(true, // ENABLE_1
       false, // DISABLE_2
       true); // ENABLE_2

  -Scott
>
> https://codereview.chromium.org/450833002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jww, 2014-08-12 19:23:42]

jam@, can you check out the changes to content/public? When the ssl_host_state.h
changes are merged with https://codereview.chromium.org/418133012/, they will
actually end up in ssl_host_state_delegate.h, I believe.

Thanks for the comments, sky. And sorry for asking for content/public review :-)

https://codereview.chromium.org/450833002/diff/80001/chrome/browser/ssl/chrom...
File chrome/browser/ssl/chrome_ssl_host_state_delegate.cc (right):

https://codereview.chromium.org/450833002/diff/80001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_delegate.cc:191: expired = true;
On 2014/08/12 16:47:31, sky wrote:
> Why did you move this AFAICT if it's only needed after the early return.
It seemed cleaner to set expired and expired_previous_decision at the same time.
I'll move it back, though, since it is indeed an unnecessary change.

https://codereview.chromium.org/450833002/diff/80001/chrome/browser/ssl/chrom...
File chrome/browser/ssl/chrome_ssl_host_state_delegate.h (right):

https://codereview.chromium.org/450833002/diff/80001/chrome/browser/ssl/chrom...
chrome/browser/ssl/chrome_ssl_host_state_delegate.h:99: //
|expired_previous_decision| receives a true value if there had been a
On 2014/08/12 16:47:31, sky wrote:
> nit: 'receives a true value' -> is set to true

Done.

https://codereview.chromium.org/450833002/diff/80001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.h (right):

https://codereview.chromium.org/450833002/diff/80001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.h:63: bool overridable,
On 2014/08/12 16:50:28, felt wrote:
> On 2014/08/12 16:47:31, sky wrote:
> > A bitmask would make this code more readable.
> 
> Do you mean condense overridable, strict_enforcement, and
> expired_but_previously_allowed into a bitmask? They're held in separate member
> variables so I'm not sure how much we'd gain from that?

I think sky is suggesting that call sites will be more readable. I'll give it a
whirl.

https://codereview.chromium.org/450833002/diff/80001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.h:125: bool
expired_but_previously_allowed_;
On 2014/08/12 16:47:31, sky wrote:
> const? Probably on all similar members.

Done.

https://codereview.chromium.org/450833002/diff/80001/content/browser/ssl/ssl_...
File content/browser/ssl/ssl_policy.h (right):

https://codereview.chromium.org/450833002/diff/80001/content/browser/ssl/ssl_...
content/browser/ssl/ssl_policy.h:61: bool overridable,
On 2014/08/12 16:47:31, sky wrote:
> Same comment about a bitmask here.

Done.

[felt, 2014-08-12 19:37:35]

https://codereview.chromium.org/450833002/diff/100001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.h (right):

https://codereview.chromium.org/450833002/diff/100001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.h:110: const bool overridable_;
overridable_ can't be const, its value might change in the constructor:

if (strict_enforcement_) overridable_ = false;

(You might need a rebase, since I don't see this line in your CL.)

[jww, 2014-08-12 20:30:43]

On 2014/08/12 19:37:35, felt wrote:
>
https://codereview.chromium.org/450833002/diff/100001/chrome/browser/ssl/ssl_...
> File chrome/browser/ssl/ssl_blocking_page.h (right):
> 
>
https://codereview.chromium.org/450833002/diff/100001/chrome/browser/ssl/ssl_...
> chrome/browser/ssl/ssl_blocking_page.h:110: const bool overridable_;
> overridable_ can't be const, its value might change in the constructor:
> 
> if (strict_enforcement_) overridable_ = false;
> 
> (You might need a rebase, since I don't see this line in your CL.)

Yup, a rebase on ToT brought that up. I just uploaded the rebase which moves the
"if (strict_enforcement_) overridable_ = false;" check so that it is done in the
contstructor's initialization so overridable_ can remain const.

[felt, 2014-08-12 21:20:35]

On 2014/08/12 20:30:43, jww wrote:
> On 2014/08/12 19:37:35, felt wrote:
> >
>
https://codereview.chromium.org/450833002/diff/100001/chrome/browser/ssl/ssl_...
> > File chrome/browser/ssl/ssl_blocking_page.h (right):
> > 
> >
>
https://codereview.chromium.org/450833002/diff/100001/chrome/browser/ssl/ssl_...
> > chrome/browser/ssl/ssl_blocking_page.h:110: const bool overridable_;
> > overridable_ can't be const, its value might change in the constructor:
> > 
> > if (strict_enforcement_) overridable_ = false;
> > 
> > (You might need a rebase, since I don't see this line in your CL.)
> 
> Yup, a rebase on ToT brought that up. I just uploaded the rebase which moves
the
> "if (strict_enforcement_) overridable_ = false;" check so that it is done in
the
> contstructor's initialization so overridable_ can remain const.

perfecto

[jar (doing other things), 2014-08-12 21:21:50]

Patch set 7, histograms.xml LGTM

[jam, 2014-08-13 06:43:58]

lgtm

[sky, 2014-08-13 20:02:27]

LGTM

[jww, 2014-08-13 23:55:34]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-08-13 23:58:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jww@chromium.org/450833002/140001

[jww, 2014-08-14 01:10:18]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-08-14 01:16:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jww@chromium.org/450833002/160001

[commit-bot: I haz the power, 2014-08-14 07:21:20]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/46788)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-14 07:33:18]

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

[commit-bot: I haz the power, 2014-08-14 07:33:19]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jww, 2014-08-14 13:23:08]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-08-14 13:25:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jww@chromium.org/450833002/180001

[commit-bot: I haz the power, 2014-08-14 17:39:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/41752)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/46967)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-14 17:52:27]

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

[commit-bot: I haz the power, 2014-08-14 17:52:28]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)

[jww, 2014-08-14 21:46:26]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-08-14 21:48:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jww@chromium.org/450833002/200001

[commit-bot: I haz the power, 2014-08-15 04:49:17]

Committed patchset #11 (200001) as 289787