Refactor SecurityStateModel/Clients for simplicity and reusability.

components/security_state/content/web_contents_security_state_model.cc

Index: components/security_state/content/web_contents_security_state_model.cc
diff --git a/chrome/browser/ssl/chrome_security_state_model_client.cc b/components/security_state/content/web_contents_security_state_model.cc
similarity index 64%
copy from chrome/browser/ssl/chrome_security_state_model_client.cc
copy to components/security_state/content/web_contents_security_state_model.cc
index 5b7097e4b4e89c8b0c8ad8d31ffc030aa4cd8fbe..6ed7e55908b760a2363cdc7aee67f5779c1a0916 100644
--- a/chrome/browser/ssl/chrome_security_state_model_client.cc
+++ b/components/security_state/content/web_contents_security_state_model.cc
@@ -2,33 +2,24 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/ssl/chrome_security_state_model_client.h"
+#include "components/security_state/content/web_contents_security_state_model.h"
 
 #include <openssl/ssl.h>
 
+#include <string>
 #include <vector>
 
-#include "base/command_line.h"
-#include "base/metrics/field_trial.h"
-#include "base/metrics/histogram_macros.h"
 #include "base/strings/string16.h"
+#include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
-#include "build/build_config.h"
-#include "chrome/browser/browser_process.h"
-#include "chrome/browser/chromeos/policy/policy_cert_service.h"
-#include "chrome/browser/chromeos/policy/policy_cert_service_factory.h"
-#include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/safe_browsing/safe_browsing_service.h"
-#include "chrome/browser/safe_browsing/ui_manager.h"
-#include "chrome/grit/chromium_strings.h"
-#include "chrome/grit/generated_resources.h"
+#include "components/strings/grit/components_chromium_strings.h"
+#include "components/strings/grit/components_strings.h"
 #include "content/public/browser/navigation_entry.h"
-#include "content/public/browser/navigation_handle.h"
-#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/security_style_explanation.h"
 #include "content/public/browser/security_style_explanations.h"
 #include "content/public/browser/ssl_status.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/content_client.h"
 #include "content/public/common/origin_util.h"
 #include "net/base/net_errors.h"
 #include "net/cert/x509_certificate.h"
@@ -36,16 +27,12 @@
 #include "net/ssl/ssl_connection_status_flags.h"
 #include "ui/base/l10n/l10n_util.h"
 
-DEFINE_WEB_CONTENTS_USER_DATA_KEY(ChromeSecurityStateModelClient);
-
-using safe_browsing::SafeBrowsingUIManager;
-using security_state::SecurityStateModel;
+namespace security_state {
 
 namespace {
 
 // Note: This is a lossy operation. Not all of the policies that can be
-// expressed by a SecurityLevel (a //chrome concept) can be expressed by
-// a blink::WebSecurityStyle.
+// expressed by a SecurityLevel can be expressed by a blink::WebSecurityStyle.
 blink::WebSecurityStyle SecurityLevelToSecurityStyle(
     SecurityStateModel::SecurityLevel security_level) {
   switch (security_level) {
@@ -67,9 +54,8 @@ blink::WebSecurityStyle SecurityLevelToSecurityStyle(
 }
 
 void AddConnectionExplanation(
-    const security_state::SecurityStateModel::SecurityInfo& security_info,
+    const SecurityStateModel::SecurityInfo& security_info,
     content::SecurityStyleExplanations* security_style_explanations) {
-
   // Avoid showing TLS details when we couldn't even establish a TLS connection
   // (e.g. for net errors) or if there was no real connection (some tests). We
   // check the |connection_status| to see if there was a connection.
@@ -149,44 +135,118 @@ void AddConnectionExplanation(
                                          description_replacements, nullptr))));
 }
 
-// Check to see whether the security state should be downgraded to reflect
-// a Safe Browsing verdict.
-void CheckSafeBrowsingStatus(content::NavigationEntry* entry,
-                             content::WebContents* web_contents,
-                             SecurityStateModel::VisibleSecurityState* state) {
-  safe_browsing::SafeBrowsingService* sb_service =
-      g_browser_process->safe_browsing_service();
-  if (!sb_service)
-    return;
-  scoped_refptr<SafeBrowsingUIManager> sb_ui_manager = sb_service->ui_manager();
-  if (sb_ui_manager->IsUrlWhitelistedOrPendingForWebContents(
-          entry->GetURL(), false, entry, web_contents, false)) {
-    state->fails_malware_check = true;
+}  // namespace
+
+class WebContentsSecurityStateModel::Client : public SecurityStateModelClient {

[blundell, 2016/10/26 18:30:47]

This is still implementing the core Client interface within the component. I
would do the following:

WebContentsSecurityStateModel (which I would personally rename to something like
WebContentsSecurityStateModelHelper since it has-a SecurityStateModel rather
than is-a) doesn't implement SecurityStateModelClient at all. Instead it has
parallel interfaces for the functionality that you want to share between the
Chrome and Headless impls.

ChromeSecurityStateModelClient has a WebContentsSecurityStateModelHelper
instance and simply delegates its impls of ChromeSecurityStateModelClient
methods to that interface as necessary. For the functionality that's
embedder-specific it would naturally still be completely contained in
ChromeSecurityStateModelClient (rather than needing to call out to
model_->client from within the component as you're doing here).

This is a little different from what Emily has suggested, but it avoids needing
two separate Client interfaces and seems like the most natural structure to me.

+ public:
+  Client(WebContentsSecurityStateModel* model) : model_(model) {}
+
+  ~Client() override {}
+
+  // SecurityStateModelClient:
+  void GetVisibleSecurityState(
+      SecurityStateModel::VisibleSecurityState* state) override {
+    content::NavigationEntry* entry =
+        model_->web_contents_->GetController().GetVisibleEntry();
+    if (!entry) {
+      *state = SecurityStateModel::VisibleSecurityState();
+      return;
+    }
+
+    if (!entry->GetSSL().initialized) {
+      *state = SecurityStateModel::VisibleSecurityState();
+      // Connection security information is still being initialized, but malware
+      // status might already be known.
+      if (model_->client_) {
+        state->fails_malware_check =
+            model_->client_->GetMalwareStatus(entry, model_->web_contents_);
+      }
+      return;
+    }
+
+    state->connection_info_initialized = true;
+    state->url = entry->GetURL();
+    const content::SSLStatus& ssl = entry->GetSSL();
+    state->certificate = ssl.certificate;
+    state->cert_status = ssl.cert_status;
+    state->connection_status = ssl.connection_status;
+    state->key_exchange_group = ssl.key_exchange_group;
+    state->security_bits = ssl.security_bits;
+    state->pkp_bypassed = ssl.pkp_bypassed;
+    state->sct_verify_statuses.clear();
+    state->sct_verify_statuses.insert(state->sct_verify_statuses.begin(),
+                                      ssl.sct_statuses.begin(),
+                                      ssl.sct_statuses.end());
+    state->displayed_mixed_content =
+        !!(ssl.content_status & content::SSLStatus::DISPLAYED_INSECURE_CONTENT);
+    state->ran_mixed_content =
+        !!(ssl.content_status & content::SSLStatus::RAN_INSECURE_CONTENT);
+    state->displayed_content_with_cert_errors =
+        !!(ssl.content_status &
+           content::SSLStatus::DISPLAYED_CONTENT_WITH_CERT_ERRORS);
+    state->ran_content_with_cert_errors = !!(
+        ssl.content_status & content::SSLStatus::RAN_CONTENT_WITH_CERT_ERRORS);
+    state->displayed_password_field_on_http =
+        !!(ssl.content_status &
+           content::SSLStatus::DISPLAYED_PASSWORD_FIELD_ON_HTTP);
+    state->displayed_credit_card_field_on_http =
+        !!(ssl.content_status &
+           content::SSLStatus::DISPLAYED_CREDIT_CARD_FIELD_ON_HTTP);
+
+    if (model_->client_) {
+      state->fails_malware_check =
+          model_->client_->GetMalwareStatus(entry, model_->web_contents_);
+    }
   }
-}
 
-}  // namespace
+  bool UsedPolicyInstalledCertificate() override {
+    if (model_->client_)
+      return model_->client_->UsedPolicyInstalledCertificate();
+    return false;
+  }
+
+  bool IsOriginSecure(const GURL& url) override {
+    return content::IsOriginSecure(url);
+  }
 
-ChromeSecurityStateModelClient::ChromeSecurityStateModelClient(
+ private:
+  WebContentsSecurityStateModel* model_;
+};
+
+WebContentsSecurityStateModel::WebContentsSecurityStateModel(
     content::WebContents* web_contents)
-    : content::WebContentsObserver(web_contents),
-      web_contents_(web_contents),
+    : web_contents_(web_contents),
       security_state_model_(new SecurityStateModel()),
-      logged_http_warning_on_current_navigation_(false) {
-  security_state_model_->SetClient(this);
+      client_(nullptr),
+      internal_client_(new Client(this)) {
+  security_state_model_->SetClient(internal_client_.get());
+}
+
+WebContentsSecurityStateModel::~WebContentsSecurityStateModel() {}
+
+void WebContentsSecurityStateModel::SetClient(
+    WebContentsSecurityStateModelClient* client) {
+  client_ = client;
 }
 
-ChromeSecurityStateModelClient::~ChromeSecurityStateModelClient() {}
+void WebContentsSecurityStateModel::GetSecurityInfo(
+    SecurityStateModel::SecurityInfo* result) const {
+  security_state_model_->GetSecurityInfo(result);
+}
+
+void WebContentsSecurityStateModel::GetVisibleSecurityState(
+    SecurityStateModel::VisibleSecurityState* state) {
+  internal_client_->GetVisibleSecurityState(state);
+}
 
 // static
-blink::WebSecurityStyle ChromeSecurityStateModelClient::GetSecurityStyle(
-    const security_state::SecurityStateModel::SecurityInfo& security_info,
+blink::WebSecurityStyle WebContentsSecurityStateModel::GetSecurityStyle(
+    const SecurityStateModel::SecurityInfo& security_info,
     content::SecurityStyleExplanations* security_style_explanations) {
   const blink::WebSecurityStyle security_style =
       SecurityLevelToSecurityStyle(security_info.security_level);
 
-  if (security_info.security_level ==
-      security_state::SecurityStateModel::HTTP_SHOW_WARNING) {
+  if (security_info.security_level == SecurityStateModel::HTTP_SHOW_WARNING) {
     // If the HTTP_SHOW_WARNING field trial is in use, display an
     // unauthenticated explanation explaining why the omnibox warning is
     // present.
@@ -194,8 +254,7 @@ blink::WebSecurityStyle ChromeSecurityStateModelClient::GetSecurityStyle(
         content::SecurityStyleExplanation(
             l10n_util::GetStringUTF8(IDS_PRIVATE_USER_DATA_INPUT),
             l10n_util::GetStringUTF8(IDS_PRIVATE_USER_DATA_INPUT_DESCRIPTION)));
-  } else if (security_info.security_level ==
-                 security_state::SecurityStateModel::NONE &&
+  } else if (security_info.security_level == SecurityStateModel::NONE &&
              security_info.displayed_private_user_data_input_on_http) {
     // If the HTTP_SHOW_WARNING field trial isn't in use yet, display an
     // informational note that the omnibox will contain a warning for
@@ -323,114 +382,4 @@ blink::WebSecurityStyle ChromeSecurityStateModelClient::GetSecurityStyle(
   return security_style;
 }
 
-void ChromeSecurityStateModelClient::GetSecurityInfo(
-    SecurityStateModel::SecurityInfo* result) const {
-  security_state_model_->GetSecurityInfo(result);
-}
-
-void ChromeSecurityStateModelClient::VisibleSSLStateChanged() {
-  if (logged_http_warning_on_current_navigation_)
-    return;
-
-  security_state::SecurityStateModel::SecurityInfo security_info;
-  GetSecurityInfo(&security_info);
-  if (!security_info.displayed_private_user_data_input_on_http)
-    return;
-
-  std::string warning;
-  switch (security_info.security_level) {
-    case security_state::SecurityStateModel::HTTP_SHOW_WARNING:
-      warning =
-          "This page includes a password or credit card input in a non-secure "
-          "context. A warning has been added to the URL bar. For more "
-          "information, see https://goo.gl/zmWq3m.";
-      break;
-    case security_state::SecurityStateModel::NONE:
-      warning =
-          "This page includes a password or credit card input in a non-secure "
-          "context. A warning will be added to the URL bar in Chrome 56 (Jan "
-          "2017). For more information, see https://goo.gl/zmWq3m.";
-      break;
-    default:
-      return;
-  }
-
-  logged_http_warning_on_current_navigation_ = true;
-  web_contents_->GetMainFrame()->AddMessageToConsole(
-      content::CONSOLE_MESSAGE_LEVEL_WARNING, warning);
-}
-
-void ChromeSecurityStateModelClient::DidFinishNavigation(
-    content::NavigationHandle* navigation_handle) {
-  if (navigation_handle->IsInMainFrame() &&
-      !navigation_handle->IsSynchronousNavigation()) {
-    // Only reset the console message flag for main-frame navigations,
-    // and not for synchronous navigations like reference fragments and
-    // pushState.
-    logged_http_warning_on_current_navigation_ = false;
-  }
-}
-
-bool ChromeSecurityStateModelClient::UsedPolicyInstalledCertificate() {
-#if defined(OS_CHROMEOS)
-  policy::PolicyCertService* service =
-      policy::PolicyCertServiceFactory::GetForProfile(
-          Profile::FromBrowserContext(web_contents_->GetBrowserContext()));
-  if (service && service->UsedPolicyCertificates())
-    return true;
-#endif
-  return false;
-}
-
-bool ChromeSecurityStateModelClient::IsOriginSecure(const GURL& url) {
-  return content::IsOriginSecure(url);
-}
-
-void ChromeSecurityStateModelClient::GetVisibleSecurityState(
-    SecurityStateModel::VisibleSecurityState* state) {
-  content::NavigationEntry* entry =
-      web_contents_->GetController().GetVisibleEntry();
-  if (!entry) {
-    *state = SecurityStateModel::VisibleSecurityState();
-    return;
-  }
-
-  if (!entry->GetSSL().initialized) {
-    *state = SecurityStateModel::VisibleSecurityState();
-    // Connection security information is still being initialized, but malware
-    // status might already be known.
-    CheckSafeBrowsingStatus(entry, web_contents_, state);
-    return;
-  }
-
-  state->connection_info_initialized = true;
-  state->url = entry->GetURL();
-  const content::SSLStatus& ssl = entry->GetSSL();
-  state->certificate = ssl.certificate;
-  state->cert_status = ssl.cert_status;
-  state->connection_status = ssl.connection_status;
-  state->key_exchange_group = ssl.key_exchange_group;
-  state->security_bits = ssl.security_bits;
-  state->pkp_bypassed = ssl.pkp_bypassed;
-  state->sct_verify_statuses.clear();
-  state->sct_verify_statuses.insert(state->sct_verify_statuses.begin(),
-                                    ssl.sct_statuses.begin(),
-                                    ssl.sct_statuses.end());
-  state->displayed_mixed_content =
-      !!(ssl.content_status & content::SSLStatus::DISPLAYED_INSECURE_CONTENT);
-  state->ran_mixed_content =
-      !!(ssl.content_status & content::SSLStatus::RAN_INSECURE_CONTENT);
-  state->displayed_content_with_cert_errors =
-      !!(ssl.content_status &
-         content::SSLStatus::DISPLAYED_CONTENT_WITH_CERT_ERRORS);
-  state->ran_content_with_cert_errors =
-      !!(ssl.content_status & content::SSLStatus::RAN_CONTENT_WITH_CERT_ERRORS);
-  state->displayed_password_field_on_http =
-      !!(ssl.content_status &
-         content::SSLStatus::DISPLAYED_PASSWORD_FIELD_ON_HTTP);
-  state->displayed_credit_card_field_on_http =
-      !!(ssl.content_status &
-         content::SSLStatus::DISPLAYED_CREDIT_CARD_FIELD_ON_HTTP);
-
-  CheckSafeBrowsingStatus(entry, web_contents_, state);
-}
+}  // namespace security_state