Show Page Info from Form-Not-Secure 'Learn more' link

chrome/browser/ui/autofill/chrome_autofill_client.cc

Index: chrome/browser/ui/autofill/chrome_autofill_client.cc
diff --git a/chrome/browser/ui/autofill/chrome_autofill_client.cc b/chrome/browser/ui/autofill/chrome_autofill_client.cc
index 6b85038c7ef3f6ec306378270330550825c2d9fc..56bf2bcfe7407282132482edd594c0309789d77d 100644
--- a/chrome/browser/ui/autofill/chrome_autofill_client.cc
+++ b/chrome/browser/ui/autofill/chrome_autofill_client.cc
@@ -20,11 +20,13 @@
 #include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/signin/signin_promo_util.h"
+#include "chrome/browser/ssl/security_state_tab_helper.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/ui/autofill/autofill_popup_controller_impl.h"
 #include "chrome/browser/ui/autofill/create_card_unmask_prompt_view.h"
 #include "chrome/browser/ui/autofill/credit_card_scanner_controller.h"
 #include "chrome/browser/ui/autofill/save_card_bubble_controller_impl.h"
+#include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/chrome_pages.h"
@@ -39,6 +41,7 @@
 #include "components/browser_sync/profile_sync_service.h"
 #include "components/password_manager/content/browser/content_password_manager_driver.h"
 #include "components/prefs/pref_service.h"
+#include "components/security_state/core/security_state.h"

[elawrence, 2017/01/25 15:50:48]

Should/can the new #includes live inside the #else block so they're only pulled
in on platforms that use them?

[estark, 2017/01/25 16:50:01]

Moved into the !OS_ANDROID block on line 54.

 #include "components/signin/core/browser/profile_identity_provider.h"
 #include "components/signin/core/browser/signin_header_helper.h"
 #include "components/signin/core/browser/signin_metrics.h"
@@ -70,13 +73,6 @@ DEFINE_WEB_CONTENTS_USER_DATA_KEY(autofill::ChromeAutofillClient);
 
 namespace autofill {
 
-namespace {
-
-const char kSecurityIndicatorHelpCenterUrl[] =
-    "https://support.google.com/chrome/answer/95617";
-
-}  // anonymous namespace
-
 ChromeAutofillClient::ChromeAutofillClient(content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       unmask_controller_(
@@ -395,10 +391,36 @@ void ChromeAutofillClient::StartSigninFlow() {
 }
 
 void ChromeAutofillClient::ShowHttpNotSecureExplanation() {
+// On Android, open a Help Center URL in a new tab, since Page Info does not
+// (yet) contain a link to the Help Center. See https://crbug.com/679532. On
+// other platforms, open Page Info which briefly explains the HTTP warning
+// message and provides a link to the Help Center for more details.
+
+#if defined(OS_ANDROID)
+  const char kSecurityIndicatorHelpCenterUrl[] =
+      "https://support.google.com/chrome/answer/95617";

[elawrence, 2017/01/25 15:50:48]

Per https://bugs.chromium.org/p/chromium/issues/detail?id=678400#c6

this should be https://support.google.com/chrome/?p=ui_security_indicator

[Mathieu, 2017/01/25 15:55:56]

Would you like to verify with meggynwatkins@ about crbug.com/679462 ? Perhaps
the team is able to generate a proper URL for this. Otherwise we will have to
add a TODO and come back to it later. Thanks!

[estark, 2017/01/25 16:50:01]

Done per elawrence's similar comment.

[estark, 2017/01/25 16:50:01]

Done.

   web_contents()->OpenURL(content::OpenURLParams(
       GURL(kSecurityIndicatorHelpCenterUrl), content::Referrer(),
       WindowOpenDisposition::NEW_FOREGROUND_TAB, ui::PAGE_TRANSITION_LINK,
       false /* is_renderer_initiated */));
+#else
+  // Important to use GetVisibleEntry to match what's showing in the omnibox.
+  content::NavigationEntry* entry =
+      web_contents()->GetController().GetVisibleEntry();

[Mathieu, 2017/01/25 15:55:56]

Ideally there would be a function that shows the website settings for a given
|web_contents|. It feels wrong to copy this from location_icon_view.cc. What do
you think?

[estark, 2017/01/25 16:50:01]

I pushed the shared code into chrome::ShowWebsiteSettings, so that it no longer
takes a URL and security info but instead grabs it itself from the WebContents.
Does that work for you?

+  // The visible entry can be nullptr in the case of window.open("").
+  if (!entry)
+    return;
+
+  SecurityStateTabHelper* helper =
+      SecurityStateTabHelper::FromWebContents(web_contents());
+  DCHECK(helper);
+  security_state::SecurityInfo security_info;
+  helper->GetSecurityInfo(&security_info);
+
+  chrome::ShowWebsiteSettings(
+      chrome::FindBrowserWithWebContents(web_contents()), web_contents(),
+      entry->GetURL(), security_info);
+#endif

[elawrence, 2017/01/25 15:50:48]

Could it be written like this, so that we fall back to showing the Help topic
directly on Android OR when one of the calls above doesn't work as expected?

void ChromeAutofillClient::ShowHttpNotSecureExplanation() {
#if !defined(OS_ANDROID)
  content::NavigationEntry* entry =
web_contents()->GetController().GetVisibleEntry();
  if (entry) {
    SecurityStateTabHelper* helper =
        SecurityStateTabHelper::FromWebContents(web_contents());
    DCHECK(helper);
    if (helper)
    {
      security_state::SecurityInfo security_info;
      helper->GetSecurityInfo(&security_info);
     
chrome::ShowWebsiteSettings(chrome::FindBrowserWithWebContents(web_contents()),
web_contents(), entry->GetURL(), security_info);
      return;
    }
  }
#else
  // On Android or in corner cases where ShowWebsiteSettings won't work, just
  // launch the Help topic directly.
  web_contents()->OpenURL(content::OpenURLParams(
      GURL(kSecurityIndicatorHelpCenterUrl), content::Referrer(),
      WindowOpenDisposition::NEW_FOREGROUND_TAB, ui::PAGE_TRANSITION_LINK,
      false /* is_renderer_initiated */));
#endif
}

[estark, 2017/01/25 16:50:01]

Done. (With some modification because of mathp's comment above about duplicating
the code)

 }
 
 }  // namespace autofill