Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(351)

Issue 2949003003: Implement a skeleton of the Superfish interstitial (Closed)

Created:
3 years, 6 months ago by estark
Modified:
3 years, 6 months ago
Reviewers:
meacer, Steven Holte
CC:
chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Implement a skeleton of the Superfish interstitial This CL adds SuperfishBlockingPage and SuperfishErrorUI classes to display the Superfish interstitial. SSLErrorHandler checks for the Superfish certificate fingerprint and uses a SuperfishBlockingPage (a subclass of SSLBlockingPage) when it's present. The strings displayed by SuperfishErrorUI are stubs for now; those will be filled in to match the mocks in a follow-up. SSLBlockingPage is modified a bit to allow a subclass that uses its own subclass of SSLErrorUI. And SSLErrorUI is modified a bit to allow a subclass that uses special strings. The certificate error report format is updated to report when the Superfish interstitial is shown (similar to how clock and captive portal interstitials are reported). Certificate reporting for the Superfish interstitial is missing a test; see https://crbug.com/735803. This is all gated behind the SuperfishInterstitial Finch feature. BUG=734590 Review-Url: https://codereview.chromium.org/2949003003 Cr-Commit-Position: refs/heads/master@{#481781} Committed: https://chromium.googlesource.com/chromium/src/+/5cbbc0943d80dbd7ba1e89694f921d61ff12e3ab

Patch Set 1 #

Total comments: 17

Patch Set 2 : meacer comments #

Total comments: 18

Patch Set 3 : meacer comments rd 2 #

Patch Set 4 : fix excessive output test problem #

Patch Set 5 : actually fix excessive output problem #

Unified diffs Side-by-side diffs Delta from patch set Stats (+296 lines, -46 lines) Patch
M chrome/browser/ssl/ssl_blocking_page.h View 1 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/ssl/ssl_blocking_page.cc View 1 2 7 chunks +29 lines, -14 lines 0 comments Download
M chrome/browser/ssl/ssl_browser_tests.cc View 1 2 4 chunks +57 lines, -3 lines 0 comments Download
M chrome/browser/ssl/ssl_error_handler.cc View 1 4 chunks +35 lines, -21 lines 0 comments Download
M chrome/browser/ui/webui/interstitials/interstitial_ui.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M components/certificate_reporting/cert_logger.proto View 1 chunk +6 lines, -3 lines 0 comments Download
M components/certificate_reporting/error_report.h View 1 chunk +2 lines, -1 line 0 comments Download
M components/certificate_reporting/error_report.cc View 1 chunk +4 lines, -0 lines 0 comments Download
M components/security_interstitials/core/BUILD.gn View 1 chunk +2 lines, -0 lines 0 comments Download
M components/security_interstitials/core/ssl_error_ui.h View 1 2 3 4 1 chunk +9 lines, -3 lines 0 comments Download
M components/security_interstitials/core/ssl_error_ui.cc View 1 2 3 4 1 chunk +16 lines, -0 lines 0 comments Download
A components/security_interstitials/core/superfish_error_ui.h View 1 chunk +36 lines, -0 lines 0 comments Download
A components/security_interstitials/core/superfish_error_ui.cc View 1 2 3 4 1 chunk +74 lines, -0 lines 0 comments Download
M components/security_interstitials_strings.grdp View 1 chunk +8 lines, -0 lines 0 comments Download
M testing/variations/fieldtrial_testing_config.json View 1 chunk +15 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 29 (20 generated)
estark
meacer, ptal? https://codereview.chromium.org/2949003003/diff/1/components/security_interstitials_strings.grdp File components/security_interstitials_strings.grdp (right): https://codereview.chromium.org/2949003003/diff/1/components/security_interstitials_strings.grdp#newcode65 components/security_interstitials_strings.grdp:65: Software on your computer is stopping Chrome ...
3 years, 6 months ago (2017-06-22 05:45:03 UTC) #6
meacer
https://codereview.chromium.org/2949003003/diff/1/chrome/browser/ssl/ssl_blocking_page.cc File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/2949003003/diff/1/chrome/browser/ssl/ssl_blocking_page.cc#newcode177 chrome/browser/ssl/ssl_blocking_page.cc:177: // Set up the metrics helper for the SSLErrorUI. ...
3 years, 6 months ago (2017-06-22 19:06:19 UTC) #9
estark
+holte to review testing/, please Thanks Mustafa! The only downside of getting rid of the ...
3 years, 6 months ago (2017-06-22 22:01:05 UTC) #13
Steven Holte
testing/ lgtm
3 years, 6 months ago (2017-06-22 22:34:08 UTC) #14
meacer
Looking good. Some minor comments. https://codereview.chromium.org/2949003003/diff/20001/chrome/browser/ssl/ssl_blocking_page.cc File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/2949003003/diff/20001/chrome/browser/ssl/ssl_blocking_page.cc#newcode136 chrome/browser/ssl/ssl_blocking_page.cc:136: bool overridable = nit: ...
3 years, 6 months ago (2017-06-22 22:35:38 UTC) #15
estark
https://codereview.chromium.org/2949003003/diff/20001/chrome/browser/ssl/ssl_blocking_page.cc File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/2949003003/diff/20001/chrome/browser/ssl/ssl_blocking_page.cc#newcode136 chrome/browser/ssl/ssl_blocking_page.cc:136: bool overridable = On 2017/06/22 22:35:37, meacer wrote: > ...
3 years, 6 months ago (2017-06-22 23:17:49 UTC) #20
meacer
LGTM, go go go. https://codereview.chromium.org/2949003003/diff/20001/components/certificate_reporting/cert_logger.proto File components/certificate_reporting/cert_logger.proto (right): https://codereview.chromium.org/2949003003/diff/20001/components/certificate_reporting/cert_logger.proto#newcode39 components/certificate_reporting/cert_logger.proto:39: INTERSTITIAL_SUPERFISH = 4; On 2017/06/22 ...
3 years, 6 months ago (2017-06-22 23:22:02 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2949003003/80001
3 years, 6 months ago (2017-06-23 00:26:30 UTC) #26
commit-bot: I haz the power
3 years, 6 months ago (2017-06-23 02:01:17 UTC) #29
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/5cbbc0943d80dbd7ba1e89694f92...

Powered by Google App Engine
This is Rietveld 408576698