Committed interstitials: move metrics and reporting out of Show, OnProceed, OnDontProceed |
|||
Issue descriptionThe blocking page's Show, OnProceed, and OnDontProceed methods currently do a bit of work around metrics and certificate reporting. These methods, however, aren't called when committed interstitials are enabled. (Show is never called because we never show an interstitial, just compute its HTML contents. OnProceed and OnDontProceed are InterstitialPageDelegate methods and the InterstitialPage classes are not used with committed interstitials.) For OnProceed and OnDontProceed, we can pull the metrics/reporting code out into a helper function and call that from the blocking page's CommandReceived for PROCEED/GO_BACK commands. For Show(), we should wrap GetHTMLContents() in a method that does the metrics setup and call that instead of calling GetHTMLContents() directly in the committed interstitials path.
,
Dec 8 2017
,
Dec 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2f8227fb2382f812203f715837f953e9ffef8410 commit 2f8227fb2382f812203f715837f953e9ffef8410 Author: Emily Stark <estark@google.com> Date: Wed Dec 13 18:07:06 2017 Adapt cert reporting, metrics, tests for committed interstitials The content::InterstitialPage hierarchy makes sure that blocking page's Show() method is called when the interstitial is shown, and OnProceed/OnDontProceed (from InterstitialPageDelegate) when the interstitial is hidden. However, with committed interstitials, the content::InterstitialPage hierachy is no longer used, so we can no longer rely on Show(), OnProceed(), and OnDontProceed() to be called to trigger metrics set up, recording, and cert reporting. This CL makes the following changes to account for this: - Introduce a new SSLBlockingPageBase superclass that handles cert reporting and metrics collection when an interstitial is closing. In the old code path, this is all triggered from OnProceed/OnDontProceed. In the new code path, it's triggered when SSLErrorTabHelper cleans up a blocking page, which happens when we navigate away or (newly in this CL) when the tab is closed. We can later move more shared code from the various SSL-related blocking pages into SSLBlockingPageBase. - Adjust how a blocking page's HTML is generated in the committed interstitials code path: instead of calling GetHTMLContents() directly, we now call a wrapper method that does the Show() metrics setup. - Deprecates an old metric that isn't actively used and was going to be a pain to adapt to the new code path. I've also enabled certificate-reporting-related browser tests for committed interstitials. Bug: 792319 , 792324 , 792324 Change-Id: I1fcbdc18085434e6c65139350f159fc8c06419c9 Reviewed-on: https://chromium-review.googlesource.com/811604 Commit-Queue: Emily Stark <estark@chromium.org> Reviewed-by: Jesse Doherty <jwd@chromium.org> Reviewed-by: Nathan Parker <nparker@chromium.org> Reviewed-by: Mustafa Emre Acer <meacer@chromium.org> Cr-Commit-Position: refs/heads/master@{#523816} [modify] https://crrev.com/2f8227fb2382f812203f715837f953e9ffef8410/chrome/browser/BUILD.gn [modify] https://crrev.com/2f8227fb2382f812203f715837f953e9ffef8410/chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc [modify] https://crrev.com/2f8227fb2382f812203f715837f953e9ffef8410/chrome/browser/ssl/bad_clock_blocking_page.cc [modify] https://crrev.com/2f8227fb2382f812203f715837f953e9ffef8410/chrome/browser/ssl/bad_clock_blocking_page.h [modify] https://crrev.com/2f8227fb2382f812203f715837f953e9ffef8410/chrome/browser/ssl/captive_portal_blocking_page.cc [modify] https://crrev.com/2f8227fb2382f812203f715837f953e9ffef8410/chrome/browser/ssl/captive_portal_blocking_page.h [modify] https://crrev.com/2f8227fb2382f812203f715837f953e9ffef8410/chrome/browser/ssl/cert_report_helper.cc [modify] https://crrev.com/2f8227fb2382f812203f715837f953e9ffef8410/chrome/browser/ssl/cert_report_helper.h [modify] https://crrev.com/2f8227fb2382f812203f715837f953e9ffef8410/chrome/browser/ssl/mitm_software_blocking_page.cc [modify] https://crrev.com/2f8227fb2382f812203f715837f953e9ffef8410/chrome/browser/ssl/mitm_software_blocking_page.h [modify] https://crrev.com/2f8227fb2382f812203f715837f953e9ffef8410/chrome/browser/ssl/ssl_blocking_page.cc [modify] https://crrev.com/2f8227fb2382f812203f715837f953e9ffef8410/chrome/browser/ssl/ssl_blocking_page.h [add] https://crrev.com/2f8227fb2382f812203f715837f953e9ffef8410/chrome/browser/ssl/ssl_blocking_page_base.cc [add] https://crrev.com/2f8227fb2382f812203f715837f953e9ffef8410/chrome/browser/ssl/ssl_blocking_page_base.h [modify] https://crrev.com/2f8227fb2382f812203f715837f953e9ffef8410/chrome/browser/ssl/ssl_browsertest.cc [modify] https://crrev.com/2f8227fb2382f812203f715837f953e9ffef8410/chrome/browser/ssl/ssl_error_tab_helper.cc [modify] https://crrev.com/2f8227fb2382f812203f715837f953e9ffef8410/chrome/browser/ssl/ssl_error_tab_helper.h [modify] https://crrev.com/2f8227fb2382f812203f715837f953e9ffef8410/components/safe_browsing/base_blocking_page.cc [modify] https://crrev.com/2f8227fb2382f812203f715837f953e9ffef8410/components/safe_browsing/base_blocking_page.h [modify] https://crrev.com/2f8227fb2382f812203f715837f953e9ffef8410/components/security_interstitials/content/security_interstitial_page.cc [modify] https://crrev.com/2f8227fb2382f812203f715837f953e9ffef8410/components/security_interstitials/content/security_interstitial_page.h [modify] https://crrev.com/2f8227fb2382f812203f715837f953e9ffef8410/tools/metrics/histograms/histograms.xml
,
Dec 13 2017
|
|||
►
Sign in to add a comment |
|||
Comment 1 by est...@chromium.org
, Dec 6 2017