Experience sampling insturmentation for SSL and Safe Browsing interstitials

Experience sampling instrumentation for SSL and Safe Browsing interstitials

This CL instruments the interstitial pages to generate the experience sampling API onDecision event.

BUG=384635

[Chris Thompson, 2014-07-16 22:49:44]

felt@ could you PTAL?

[felt, 2014-07-18 21:13:59]

preliminary review, I know you're still planning to make a few more changes to
this

https://codereview.chromium.org/399773002/diff/20001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/399773002/diff/20001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:319: event_name =
"malware_and_phishing_interstitial";
use a switch statement for this, and also put strings as const strings up at the
top

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

https://codereview.chromium.org/399773002/diff/20001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:284: std::string
event_name("ssl_interstitial_");
these should be constants too

you might even want to add these constants to the experience sampling API class
and then refer to them here, so that there is a single place with all constants

https://codereview.chromium.org/399773002/diff/20001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:286: event_name.append("nonblocking_");
use "overridable" here to match the boolean; also, you need to check

overridable_ && !strict_enforcement_

(I know, this is messy, I need to refactor at some point to create a single
boolean.)

https://codereview.chromium.org/399773002/diff/20001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:312: 
don't add spurious blank lines please

[Chris Thompson, 2014-07-23 22:45:38]

https://codereview.chromium.org/399773002/diff/20001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/399773002/diff/20001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:319: event_name =
"malware_and_phishing_interstitial";
On 2014/07/18 21:13:59, felt wrote:
> use a switch statement for this, and also put strings as const strings up at
the
> top

Done.

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

https://codereview.chromium.org/399773002/diff/20001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:284: std::string
event_name("ssl_interstitial_");
On 2014/07/18 21:13:59, felt wrote:
> these should be constants too
> 
> you might even want to add these constants to the experience sampling API
class
> and then refer to them here, so that there is a single place with all
constants

Done. I've put the context-specific strings at the top of each file/context, and
the general ones (such as "proceed", "deny") into the SamplingEvent class.

https://codereview.chromium.org/399773002/diff/20001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:286: event_name.append("nonblocking_");
On 2014/07/18 21:13:59, felt wrote:
> use "overridable" here to match the boolean; also, you need to check
> 
> overridable_ && !strict_enforcement_
> 
> (I know, this is messy, I need to refactor at some point to create a single
> boolean.)

Done.

https://codereview.chromium.org/399773002/diff/20001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:312: 
On 2014/07/18 21:13:59, felt wrote:
> don't add spurious blank lines please

Done. Accidentally left this in from previous version of the instrumentation.

[felt, 2014-07-23 22:49:03]

are there other changes i should wait for, or should i do a full review now?

[Chris Thompson, 2014-07-23 23:05:11]

On 2014/07/23 22:49:03, felt wrote:
> are there other changes i should wait for, or should i do a full review now?

I still need to add the instrumentation for the details and learn more. I'll
ping this CL when that's done and this is ready for full review.

[Chris Thompson, 2014-07-23 23:33:50]

On 2014/07/23 23:05:11, Chris Thompson wrote:
> On 2014/07/23 22:49:03, felt wrote:
> > are there other changes i should wait for, or should i do a full review now?
> 
> I still need to add the instrumentation for the details and learn more. I'll
> ping this CL when that's done and this is ready for full review.

Alright done and ready for full review.

[felt, 2014-07-25 01:05:51]

lgtm, please send to owners for reviews

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

https://codereview.chromium.org/399773002/diff/100001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:765: 
nit: don't need this empty line

[Chris Thompson, 2014-07-25 01:22:00]

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

https://codereview.chromium.org/399773002/diff/100001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:765: 
On 2014/07/25 01:05:51, felt wrote:
> nit: don't need this empty line

Done.

[Chris Thompson, 2014-07-25 16:34:04]

palmer@ and noelutz@, could you give OWNERS review for the SSL and SafeBrowsing
blocking pages? Thanks.

[palmer, 2014-07-25 18:05:00]

LGTM after you fix the typos. You also have a typo in the commit message. :)

https://codereview.chromium.org/399773002/diff/140001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/399773002/diff/140001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:124: const char
kEventNameOther[] = "safebrowsing_other_instertitial_";
Typo: Should be "interstitial".

https://codereview.chromium.org/399773002/diff/140001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:323: // This needs
to handle all types of warnings this insterstitial can show.
Typo again :)

[Chris Thompson, 2014-07-25 18:19:47]

Apparently my hands don't know how to type "interstitial". CL description also
fixed :-)

https://codereview.chromium.org/399773002/diff/140001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/399773002/diff/140001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:124: const char
kEventNameOther[] = "safebrowsing_other_instertitial_";
On 2014/07/25 18:05:00, Chromium Palmer wrote:
> Typo: Should be "interstitial".

Done.

https://codereview.chromium.org/399773002/diff/140001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:323: // This needs
to handle all types of warnings this insterstitial can show.
On 2014/07/25 18:05:00, Chromium Palmer wrote:
> Typo again :)

Done.

[noelutz, 2014-07-25 19:58:13]

The CQ bit was checked by noelutz@google.com

[noelutz, 2014-07-25 19:58:15]

The CQ bit was unchecked by noelutz@google.com

[noelutz, 2014-07-25 19:58:19]

lgtm

[Chris Thompson, 2014-07-30 21:04:33]

The CQ bit was checked by cthomp@chromium.org

[commit-bot: I haz the power, 2014-07-30 21:06:41]

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

[Chris Thompson, 2014-07-30 23:41:19]

The CQ bit was checked by cthomp@chromium.org

[commit-bot: I haz the power, 2014-07-30 23:44:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/cthomp@chromium.org/399773002/280001

[commit-bot: I haz the power, 2014-07-31 07:50:05]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel 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_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
  win_chromium_rel 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-07-31 08:25:52]

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

[commit-bot: I haz the power, 2014-07-31 08:25:53]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Chris Thompson, 2014-08-07 17:21:05]

The CQ bit was checked by cthomp@chromium.org

[commit-bot: I haz the power, 2014-08-07 17:22:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/cthomp@chromium.org/399773002/400001

[commit-bot: I haz the power, 2014-08-07 18:49:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/43830)
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
  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 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-07 19:10:52]

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

[commit-bot: I haz the power, 2014-08-07 19:10:54]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)

[Chris Thompson, 2014-08-07 19:18:35]

The CQ bit was checked by cthomp@chromium.org

[commit-bot: I haz the power, 2014-08-07 19:22:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/cthomp@chromium.org/399773002/400001

[commit-bot: I haz the power, 2014-08-08 00:41:33]

Change committed as 288177