Have SSLErrorHandler decide which type of interstitial to display

Have SSLErrorHandler decide which type of interstitial to display

Previously, the SSLBlockingPage would decide whether to show a clock
error or "regular" SSL error. That logic is now moved up into the
SSLErrorHandler.

This CL also moves metrics reporting to the bad clock interstitial.
The bad clock interstitial from now on will only record
interaction metrics, since decision metrics are moot.

BUG=516370
Committed: https://crrev.com/534cc9e7396c6f497f6fc11715adb3c0351a8b7b
Cr-Commit-Position: refs/heads/master@{#346726}

[felt, 2015-08-27 17:39:17]

Patchset #1 (id:1) has been deleted

[felt, 2015-08-27 17:39:22]

Patchset #1 (id:20001) has been deleted

[felt, 2015-08-28 02:12:21]

Patchset #1 (id:40001) has been deleted

[felt, 2015-08-28 02:12:34]

Patchset #1 (id:40001) has been deleted

[felt, 2015-08-28 04:21:37]

Patchset #3 (id:90001) has been deleted

[felt, 2015-08-28 04:40:34]

felt@chromium.org changed reviewers:
+ lgarron@chromium.org

[felt, 2015-08-28 04:40:48]

lgarron, can you please review?

[felt, 2015-08-28 04:54:05]

accidentally hit enter too soon:

I'm actually still struggling with the browser test, I think because of the
weirdnesses with using build time. would appreciate advice on that if anything
sticks out to you.

[felt, 2015-08-31 22:48:43]

felt@chromium.org changed reviewers:
+ estark@chromium.org

[felt, 2015-08-31 23:00:36]

estark:

PTAL?

Two notes:

* I'm not thrilled with needing to mock out the time in two places, but I don't
think that can be avoided.
* Right now the bad clock class doesn't send cert reports. This class is going
to be the first one I'm componentizing; I'll add reports back in after I do
that.

[estark, 2015-09-01 13:28:29]

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/bad...
File chrome/browser/ssl/bad_clock_blocking_page.cc (right):

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/bad...
chrome/browser/ssl/bad_clock_blocking_page.cc:188:
metrics_helper()->RecordUserInteraction(
In SSLBlockingPage we also record security_interstitials::MetricsHelper::SHOW
here. Is there a reason we don't do that here?

This also doesn't call SSLErrorClassification::RecordUMAStatistics() which would
have been used to be called for this interstitial from the SSLBlockingPage
constructor. Is that intentional?

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_blocking_page.cc (left):

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_blocking_page.cc:258: // We collapse the Rappor metric
name to just "ssl" so we don't leak
I found the comment on line 82 a little confusing, and this is more clear. Maybe
say something more like this on line 82? (i.e. be explicit that the point is to
avoid leaking the "overridable" bit)

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_blocking_page.cc:364: // Handled by the bad clock
interstitial.
Is it intentional that this falls through to the NOTREACHED() below?

I think it probably is. If so, to me it would look less accidental if written
like so (though this might be just a personal preference):

case CMD_OPEN_DATE_SETTINGS:
case CMD_OPEN_DIAGNOSTIC:
  // The date settings command is handled by the bad clock interstitial, and
Google doesn't currently have a transparency report for SSL.
  NOTREACHED();

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_blocking_page.cc:427:
SSLBlockingPage::GetCertReportInterstitialReason() {
You could now just inline this in the one place it's called.

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_blocking_page.cc:432: if (overridable_)
nit: `return overridable ? "ssl_overridable" : "ssl_nonoverridable"` ?

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:784: // The build time is only available
reliably on official builds.
Can you explain this comment more? It seems like a non-sequitur; I don't see
anything about official builds in the actual test.

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:796:
ASSERT_EQ(BadClockBlockingPage::kTypeForTesting,
nit: This should probably be EXPECT_EQ.

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_error_handler.cc (right):

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_error_handler.cc:141:
SSLErrorClassification::SetBuildTimeForTesting(
nit: I'm curious if you considered calling this from the test instead of calling
it here. The advantages I see of doing that are:
1. you can make both the Set*ForTesting clock methods private and
FRIEND_TEST_ALL_PREFIXES the test.
2. it's not obvious from the outset that calling
SSLErrorHandler::SetClockForTesting() also affects the build time that'll be
used. So maybe SetClockForTesting() should be renamed
SetClockAndBuildTimeForTesting() to make it more clear, or the test itself could
call SetBuildTimeForTesting() to make it more explicit.

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_error_handler.h (right):

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_error_handler.h:41: // If there is a name mismatch error
and a corresponding suggested URL available
Something in this sentence doesn't parse for me... maybe an extra "available" at
the end of this line?

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_error_handler.h:45: // causes match, an SSL intersittial
is shown.
typo in "interstitial"

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_error_handler.h:102: virtual void
ShowBadClockInterstitial(const base::Time& now);
It doesn't look like you actually override this anywhere, so maybe it doesn't
need to be virtual.

[felt, 2015-09-01 15:46:18]

Patchset #5 (id:150001) has been deleted

[felt, 2015-09-01 15:49:42]

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/bad...
File chrome/browser/ssl/bad_clock_blocking_page.cc (right):

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/bad...
chrome/browser/ssl/bad_clock_blocking_page.cc:188:
metrics_helper()->RecordUserInteraction(
On 2015/09/01 13:28:28, estark wrote:
> In SSLBlockingPage we also record security_interstitials::MetricsHelper::SHOW
> here. Is there a reason we don't do that here?

Yes. That's for the decision UMA metric which doesn't really make sense for the
bad ssl warning, where you can't proceed (meaning there is no decision to make).

> 
> This also doesn't call SSLErrorClassification::RecordUMAStatistics() which
would
> have been used to be called for this interstitial from the SSLBlockingPage
> constructor. Is that intentional?

SSLErrorClassification should probably be updated to keep separate stats for
different types of interstitials. With that being said, perhaps I should keep
the recording here and then make a change to how SSLErrorClassification works in
a follow-up CL. (Done that, and added a TODO.)

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_blocking_page.cc (left):

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_blocking_page.cc:258: // We collapse the Rappor metric
name to just "ssl" so we don't leak
On 2015/09/01 13:28:28, estark wrote:
> I found the comment on line 82 a little confusing, and this is more clear.
Maybe
> say something more like this on line 82? (i.e. be explicit that the point is
to
> avoid leaking the "overridable" bit)

Done.

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_blocking_page.cc:364: // Handled by the bad clock
interstitial.
On 2015/09/01 13:28:28, estark wrote:
> Is it intentional that this falls through to the NOTREACHED() below?
> 
> I think it probably is. If so, to me it would look less accidental if written
> like so (though this might be just a personal preference):
> 
> case CMD_OPEN_DATE_SETTINGS:
> case CMD_OPEN_DIAGNOSTIC:
>   // The date settings command is handled by the bad clock interstitial, and
> Google doesn't currently have a transparency report for SSL.
>   NOTREACHED();

Done.

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_blocking_page.cc:427:
SSLBlockingPage::GetCertReportInterstitialReason() {
On 2015/09/01 13:28:28, estark wrote:
> You could now just inline this in the one place it's called.

Done.

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_blocking_page.cc:432: if (overridable_)
On 2015/09/01 13:28:28, estark wrote:
> nit: `return overridable ? "ssl_overridable" : "ssl_nonoverridable"` ?

Done.

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:784: // The build time is only available
reliably on official builds.
On 2015/09/01 13:28:28, estark wrote:
> Can you explain this comment more? It seems like a non-sequitur; I don't see
> anything about official builds in the actual test.

Ah this was an oversight, I used to have the test run only on official builds
(but then mocked out the clock so that the tests could run everywhere). Deleted.

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_browser_tests.cc:796:
ASSERT_EQ(BadClockBlockingPage::kTypeForTesting,
On 2015/09/01 13:28:28, estark wrote:
> nit: This should probably be EXPECT_EQ.

Done.

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_error_handler.cc (right):

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_error_handler.cc:141:
SSLErrorClassification::SetBuildTimeForTesting(
On 2015/09/01 13:28:28, estark wrote:
> nit: I'm curious if you considered calling this from the test instead of
calling
> it here. The advantages I see of doing that are:
> 1. you can make both the Set*ForTesting clock methods private and
> FRIEND_TEST_ALL_PREFIXES the test.
> 2. it's not obvious from the outset that calling
> SSLErrorHandler::SetClockForTesting() also affects the build time that'll be
> used. So maybe SetClockForTesting() should be renamed
> SetClockAndBuildTimeForTesting() to make it more clear, or the test itself
could
> call SetBuildTimeForTesting() to make it more explicit.

Moved into the test.

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_error_handler.h (right):

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_error_handler.h:41: // If there is a name mismatch error
and a corresponding suggested URL available
On 2015/09/01 13:28:28, estark wrote:
> Something in this sentence doesn't parse for me... maybe an extra "available"
at
> the end of this line?

Done.

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_error_handler.h:45: // causes match, an SSL intersittial
is shown.
On 2015/09/01 13:28:28, estark wrote:
> typo in "interstitial"

Done.

https://codereview.chromium.org/1317593002/diff/130001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_error_handler.h:102: virtual void
ShowBadClockInterstitial(const base::Time& now);
On 2015/09/01 13:28:28, estark wrote:
> It doesn't look like you actually override this anywhere, so maybe it doesn't
> need to be virtual.

Done.

[estark, 2015-09-01 16:41:48]

LGTM % memory leak

https://codereview.chromium.org/1317593002/diff/170001/chrome/browser/ssl/bad...
File chrome/browser/ssl/bad_clock_blocking_page.cc (right):

https://codereview.chromium.org/1317593002/diff/170001/chrome/browser/ssl/bad...
chrome/browser/ssl/bad_clock_blocking_page.cc:192: (new
SSLErrorClassification(web_contents, time_triggered_, request_url,
memory leak... put it in a scoped_ptr or just do

SSLErrorClassification error_classification(...);
error_classification->RecordUMAStatistics(false);

[felt, 2015-09-01 19:13:15]

https://codereview.chromium.org/1317593002/diff/170001/chrome/browser/ssl/bad...
File chrome/browser/ssl/bad_clock_blocking_page.cc (right):

https://codereview.chromium.org/1317593002/diff/170001/chrome/browser/ssl/bad...
chrome/browser/ssl/bad_clock_blocking_page.cc:192: (new
SSLErrorClassification(web_contents, time_triggered_, request_url,
On 2015/09/01 16:41:48, estark wrote:
> memory leak... put it in a scoped_ptr or just do
> 
> SSLErrorClassification error_classification(...);
> error_classification->RecordUMAStatistics(false);

Done.

[felt, 2015-09-01 19:14:07]

felt@chromium.org changed reviewers:
+ isherman@chromium.org

[felt, 2015-09-01 19:14:43]

isherman, PTAL at histograms.xml?

[Ilya Sherman, 2015-09-01 20:32:41]

histograms.xml lgtm

[Ilya Sherman, 2015-09-01 20:33:03]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2015-09-01 20:33:04]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org
Link to the patchset: https://codereview.chromium.org/1317593002/#ps210001
(title: "typo")

[commit-bot: I haz the power, 2015-09-01 20:33:34]

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

[commit-bot: I haz the power, 2015-09-01 20:39:06]

Committed patchset #7 (id:210001)

[commit-bot: I haz the power, 2015-09-01 20:40:15]

Patchset 7 (id:??) landed as
https://crrev.com/534cc9e7396c6f497f6fc11715adb3c0351a8b7b
Cr-Commit-Position: refs/heads/master@{#346726}