Shows the info bubble when the location bar icon is clicked in the origin chip.

chrome/browser/ui/toolbar/toolbar_model_impl.cc

Index: chrome/browser/ui/toolbar/toolbar_model_impl.cc
diff --git a/chrome/browser/ui/toolbar/toolbar_model_impl.cc b/chrome/browser/ui/toolbar/toolbar_model_impl.cc
index 50df4f2e268ce70cf1ae3ba36a9ec5fd7632466f..e46fb0f747bce525847396a7f9ad54e02d5575f7 100644
--- a/chrome/browser/ui/toolbar/toolbar_model_impl.cc
+++ b/chrome/browser/ui/toolbar/toolbar_model_impl.cc
@@ -51,6 +51,7 @@ ToolbarModelImpl::ToolbarModelImpl(ToolbarModelDelegate* delegate)
 ToolbarModelImpl::~ToolbarModelImpl() {
 }
 
+// static
 ToolbarModel::SecurityLevel ToolbarModelImpl::GetSecurityLevelForWebContents(
       content::WebContents* web_contents) {
   if (!web_contents)
@@ -94,6 +95,22 @@ ToolbarModel::SecurityLevel ToolbarModelImpl::GetSecurityLevelForWebContents(
   }
 }
 
+// static
+base::string16 ToolbarModelImpl::GetEVCertName(
+    const net::X509Certificate& cert) {
+  // EV are required to have an organization name and country.
+  if (cert.subject().organization_names.empty() ||
+      cert.subject().country_name.empty()) {
+    NOTREACHED();

[Peter Kasting, 2014/05/09 20:35:40]

Nit: This violates the Chrome style guide rule about not handling DCHECK
failure.  This should just be:

  DCHECK(!cert.subject().organization_names.empty());
  DCHECK(!cert.subject().country_name.empty());

(Unless for some reason these really can be empty, in which case we should
remove the NOTREACHED() instead.  I wouldn't do that unless you know you need
to.)

[macourteau, 2014/05/09 20:53:23]

Isn't the behavior of the two DCHECK's different though? I'm not the author of
this code, but what I see here is that in debug, it will trigger the NOTREACHED,
but in release it will log an error and return an empty string.

With the two DCHECK's, on the other hand, the debug behavior would be the same,
but control would reach the GetStringFUTF16 call in release, which is different
from returning an empty string (and might crash due to accessing index 0 of
organization_name, if it's empty).

[Peter Kasting, 2014/05/09 21:15:47]

NOTREACHED/DCHECK is an assertion that the check in question cannot possibly
fail, ever, in any mode, even for unusual reasons (e.g. disk corruption or
out-of-memory should still not trigger DCHECK failures).

Code is not supposed to speculate about what might happen if the assertion is
violated, and how to handle that.  In other words, and code that says, "but in
release mode, if this fails, then" is wrong, because the entire point of a
NOTREACHED/DCHECK is to say that the conditional literally never fails.

[macourteau, 2014/05/09 22:23:51]

SGTM. Done.

+    return base::string16();
+  }
+
+  return l10n_util::GetStringFUTF16(
+      IDS_SECURE_CONNECTION_EV,
+      base::UTF8ToUTF16(cert.subject().organization_names[0]),
+      base::UTF8ToUTF16(cert.subject().country_name));
+}
+
 // ToolbarModelImpl Implementation.
 base::string16 ToolbarModelImpl::GetText() const {
   base::string16 search_terms(GetSearchTerms(false));
@@ -154,77 +171,11 @@ GURL ToolbarModelImpl::GetURL() const {
   return GURL(content::kAboutBlankURL);
 }
 
-bool ToolbarModelImpl::WouldOmitURLDueToOriginChip() const {
-  const char kInterstitialShownKey[] = "interstitial_shown";
-
-  // When users type URLs and hit enter, continue to show those URLs until
-  // the navigation commits or an interstitial is shown, because having the
-  // omnibox clear immediately feels like the input was ignored.
-  NavigationController* navigation_controller = GetNavigationController();
-  if (navigation_controller) {
-    NavigationEntry* pending_entry = navigation_controller->GetPendingEntry();
-    if (pending_entry) {
-      const NavigationEntry* visible_entry =
-          navigation_controller->GetVisibleEntry();
-      base::string16 unused;
-      // Keep track that we've shown the origin chip on an interstitial so it
-      // can be shown even after the interstitial was dismissed, to avoid
-      // showing the chip, removing it and then showing it again.
-      if (visible_entry &&
-          visible_entry->GetPageType() == content::PAGE_TYPE_INTERSTITIAL &&
-          !pending_entry->GetExtraData(kInterstitialShownKey, &unused))
-        pending_entry->SetExtraData(kInterstitialShownKey, base::string16());
-      const content::PageTransition transition_type =
-          pending_entry->GetTransitionType();
-      if ((transition_type & content::PAGE_TRANSITION_TYPED) != 0 &&
-          !pending_entry->GetExtraData(kInterstitialShownKey, &unused))
-        return false;
-    }
-  }
-
-  bool should_display_origin_chip =
-      chrome::ShouldDisplayOriginChip() || chrome::ShouldDisplayOriginChipV2();
-  return should_display_origin_chip && delegate_->InTabbedBrowser() &&
-      ShouldDisplayURL() && url_replacement_enabled();
-}
-
 bool ToolbarModelImpl::WouldPerformSearchTermReplacement(
     bool ignore_editing) const {
   return !GetSearchTerms(ignore_editing).empty();
 }
 
-bool ToolbarModelImpl::ShouldDisplayURL() const {
-  // Note: The order here is important.
-  // - The WebUI test must come before the extension scheme test because there
-  //   can be WebUIs that have extension schemes (e.g. the bookmark manager). In
-  //   that case, we should prefer what the WebUI instance says.
-  // - The view-source test must come before the NTP test because of the case
-  //   of view-source:chrome://newtab, which should display its URL despite what
-  //   chrome://newtab says.
-  NavigationController* controller = GetNavigationController();
-  NavigationEntry* entry = controller ? controller->GetVisibleEntry() : NULL;
-  if (entry) {
-    if (entry->IsViewSourceMode() ||
-        entry->GetPageType() == content::PAGE_TYPE_INTERSTITIAL) {
-      return true;
-    }
-
-    GURL url = entry->GetURL();
-    GURL virtual_url = entry->GetVirtualURL();
-    if (url.SchemeIs(content::kChromeUIScheme) ||
-        virtual_url.SchemeIs(content::kChromeUIScheme)) {
-      if (!url.SchemeIs(content::kChromeUIScheme))
-        url = virtual_url;
-      return url.host() != chrome::kChromeUINewTabHost;
-    }
-  }
-
-  if (chrome::IsInstantNTP(delegate_->GetActiveWebContents()))
-    return false;
-
-  return true;
-}
-
 ToolbarModel::SecurityLevel ToolbarModelImpl::GetSecurityLevel(
     bool ignore_editing) const {
   // When editing, assume no security style.
@@ -273,20 +224,70 @@ base::string16 ToolbarModelImpl::GetEVCertName() const {
   return GetEVCertName(*cert.get());
 }
 
-// static
-base::string16 ToolbarModelImpl::GetEVCertName(
-    const net::X509Certificate& cert) {
-  // EV are required to have an organization name and country.
-  if (cert.subject().organization_names.empty() ||
-      cert.subject().country_name.empty()) {
-    NOTREACHED();
-    return base::string16();
+bool ToolbarModelImpl::ShouldDisplayURL() const {
+  // Note: The order here is important.
+  // - The WebUI test must come before the extension scheme test because there
+  //   can be WebUIs that have extension schemes (e.g. the bookmark manager). In
+  //   that case, we should prefer what the WebUI instance says.
+  // - The view-source test must come before the NTP test because of the case
+  //   of view-source:chrome://newtab, which should display its URL despite what
+  //   chrome://newtab says.
+  NavigationController* controller = GetNavigationController();
+  NavigationEntry* entry = controller ? controller->GetVisibleEntry() : NULL;
+  if (entry) {
+    if (entry->IsViewSourceMode() ||
+        entry->GetPageType() == content::PAGE_TYPE_INTERSTITIAL) {
+      return true;
+    }
+
+    GURL url = entry->GetURL();
+    GURL virtual_url = entry->GetVirtualURL();
+    if (url.SchemeIs(content::kChromeUIScheme) ||
+        virtual_url.SchemeIs(content::kChromeUIScheme)) {
+      if (!url.SchemeIs(content::kChromeUIScheme))
+        url = virtual_url;
+      return url.host() != chrome::kChromeUINewTabHost;
+    }
   }
 
-  return l10n_util::GetStringFUTF16(
-      IDS_SECURE_CONNECTION_EV,
-      base::UTF8ToUTF16(cert.subject().organization_names[0]),
-      base::UTF8ToUTF16(cert.subject().country_name));
+  if (chrome::IsInstantNTP(delegate_->GetActiveWebContents()))

[Peter Kasting, 2014/05/09 20:35:40]

Nit: Just:

  return !chrome::IsInstantNTP(delegate_->GetActiveWebContents());

[macourteau, 2014/05/09 20:53:23]

Done.

+    return false;
+
+  return true;
+}
+
+bool ToolbarModelImpl::WouldOmitURLDueToOriginChip() const {
+  const char kInterstitialShownKey[] = "interstitial_shown";
+
+  // When users type URLs and hit enter, continue to show those URLs until
+  // the navigation commits or an interstitial is shown, because having the
+  // omnibox clear immediately feels like the input was ignored.
+  NavigationController* navigation_controller = GetNavigationController();
+  if (navigation_controller) {
+    NavigationEntry* pending_entry = navigation_controller->GetPendingEntry();
+    if (pending_entry) {
+      const NavigationEntry* visible_entry =
+          navigation_controller->GetVisibleEntry();
+      base::string16 unused;
+      // Keep track that we've shown the origin chip on an interstitial so it
+      // can be shown even after the interstitial was dismissed, to avoid
+      // showing the chip, removing it and then showing it again.
+      if (visible_entry &&
+          visible_entry->GetPageType() == content::PAGE_TYPE_INTERSTITIAL &&
+          !pending_entry->GetExtraData(kInterstitialShownKey, &unused))
+        pending_entry->SetExtraData(kInterstitialShownKey, base::string16());
+      const content::PageTransition transition_type =
+          pending_entry->GetTransitionType();
+      if ((transition_type & content::PAGE_TRANSITION_TYPED) != 0 &&
+          !pending_entry->GetExtraData(kInterstitialShownKey, &unused))
+        return false;
+    }
+  }
+
+  bool should_display_origin_chip =
+      chrome::ShouldDisplayOriginChip() || chrome::ShouldDisplayOriginChipV2();
+  return should_display_origin_chip && delegate_->InTabbedBrowser() &&
+      ShouldDisplayURL() && url_replacement_enabled();
 }
 
 NavigationController* ToolbarModelImpl::GetNavigationController() const {