Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2650033004:
Show Page Info from Form-Not-Secure 'Learn more' link (Closed)
|
Created:
3 years, 11 months ago by estark Modified:
3 years, 11 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, estade+watch_chromium.org, sebsg+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, mathp+autofillwatch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShow Page Info from Form-Not-Secure 'Learn more' link
Instead of directly opening a tab to the Help Center, the "Learn more" link
should open the Page Info bubble. From there the user can click another link to
open the Help Center, if they want.
BUG=684682
TEST=Enable #enable-http-form-warning. Visit http://rsolomakhin.github.io.
Fill out the Name/Password form and submit it and save the password. Go back
to http://rsolomakhin.github.io and focus the Password field. Observe the
"Login not secure" warning in the autofill dropdown and click it. Observe that
the Page Info bubble opens down from the omnibox.
Review-Url: https://codereview.chromium.org/2650033004
Cr-Commit-Position: refs/heads/master@{#446469}
Committed: https://chromium.googlesource.com/chromium/src/+/c83163e85da7ed3dc54a0dd21bf1954f2643459c
Patch Set 1 #Patch Set 2 : fix Android #Patch Set 3 : android fix #Patch Set 4 : fix unused var #
Total comments: 10
Patch Set 5 : elawrence, mathp comments #Patch Set 6 : Remove the 'trigger a call to open the url' references #
Total comments: 6
Patch Set 7 : mathp comments #
Total comments: 23
Patch Set 8 : msw comments #Patch Set 9 : rebase #Messages
Total messages: 53 (35 generated)
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Show Page Info from Form-Not-Secure 'Learn more' link Instead of directly opening a tab to the Help Center, the "Learn more" link should open the Page Info bubble. From there the user can click another link to open the Help Center, if they want. BUG=684682 ========== to ========== Show Page Info from Form-Not-Secure 'Learn more' link Instead of directly opening a tab to the Help Center, the "Learn more" link should open the Page Info bubble. From there the user can click another link to open the Help Center, if they want. BUG=684682 TEST=Enable #enable-http-form-warning. Visit http://rsolomakhin.github.io. Fill out the Name/Password form and submit it and save the password. Go back to http://rsolomakhin.github.io and focus the Password field. Observe the "Login not secure" warning in the autofill dropdown and click it. Observe that the Page Info bubble opens down from the omnibox. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
estark@chromium.org changed reviewers: + mathp@chromium.org
mathp, PTAL? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
elawrence@chromium.org changed reviewers: + elawrence@chromium.org
https://codereview.chromium.org/2650033004/diff/60001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/chrome_autofill_client.cc (right): https://codereview.chromium.org/2650033004/diff/60001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client.cc:44: #include "components/security_state/core/security_state.h" Should/can the new #includes live inside the #else block so they're only pulled in on platforms that use them? https://codereview.chromium.org/2650033004/diff/60001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client.cc:401: "https://support.google.com/chrome/answer/95617"; Per https://bugs.chromium.org/p/chromium/issues/detail?id=678400#c6 this should be https://support.google.com/chrome/?p=ui_security_indicator https://codereview.chromium.org/2650033004/diff/60001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client.cc:423: #endif 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 }
On 2017/01/25 15:50:48, elawrence wrote: > https://codereview.chromium.org/2650033004/diff/60001/chrome/browser/ui/autof... > File chrome/browser/ui/autofill/chrome_autofill_client.cc (right): > > https://codereview.chromium.org/2650033004/diff/60001/chrome/browser/ui/autof... > chrome/browser/ui/autofill/chrome_autofill_client.cc:44: #include > "components/security_state/core/security_state.h" > Should/can the new #includes live inside the #else block so they're only pulled > in on platforms that use them? > > https://codereview.chromium.org/2650033004/diff/60001/chrome/browser/ui/autof... > chrome/browser/ui/autofill/chrome_autofill_client.cc:401: > "https://support.google.com/chrome/answer/95617"; > Per https://bugs.chromium.org/p/chromium/issues/detail?id=678400#c6 > > this should be https://support.google.com/chrome/?p=ui_security_indicator > > https://codereview.chromium.org/2650033004/diff/60001/chrome/browser/ui/autof... > chrome/browser/ui/autofill/chrome_autofill_client.cc:423: #endif > 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 > } Oh, also should we clean up the three comments that describe the old behavior? https://cs.chromium.org/search/?q=%22//+Accepting+the+warning+message+should+...
https://codereview.chromium.org/2650033004/diff/60001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/chrome_autofill_client.cc (right): https://codereview.chromium.org/2650033004/diff/60001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client.cc:401: "https://support.google.com/chrome/answer/95617"; 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! https://codereview.chromium.org/2650033004/diff/60001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client.cc:409: web_contents()->GetController().GetVisibleEntry(); 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?
On 2017/01/25 15:55:56, Mathieu Perreault wrote: > https://codereview.chromium.org/2650033004/diff/60001/chrome/browser/ui/autof... > File chrome/browser/ui/autofill/chrome_autofill_client.cc (right): > > https://codereview.chromium.org/2650033004/diff/60001/chrome/browser/ui/autof... > chrome/browser/ui/autofill/chrome_autofill_client.cc:401: > "https://support.google.com/chrome/answer/95617"; > 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! > > https://codereview.chromium.org/2650033004/diff/60001/chrome/browser/ui/autof... > chrome/browser/ui/autofill/chrome_autofill_client.cc:409: > web_contents()->GetController().GetVisibleEntry(); > 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? Simultaneous review! Thankfully, we're not too far off :)
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2650033004/diff/60001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/chrome_autofill_client.cc (right): https://codereview.chromium.org/2650033004/diff/60001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client.cc:44: #include "components/security_state/core/security_state.h" On 2017/01/25 15:50:48, elawrence wrote: > Should/can the new #includes live inside the #else block so they're only pulled > in on platforms that use them? Moved into the !OS_ANDROID block on line 54. https://codereview.chromium.org/2650033004/diff/60001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client.cc:401: "https://support.google.com/chrome/answer/95617"; On 2017/01/25 15:50:48, elawrence wrote: > Per https://bugs.chromium.org/p/chromium/issues/detail?id=678400#c6 > > this should be https://support.google.com/chrome/?p=ui_security_indicator Done. https://codereview.chromium.org/2650033004/diff/60001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client.cc:401: "https://support.google.com/chrome/answer/95617"; On 2017/01/25 15:55:56, Mathieu Perreault wrote: > 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! Done per elawrence's similar comment. https://codereview.chromium.org/2650033004/diff/60001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client.cc:409: web_contents()->GetController().GetVisibleEntry(); On 2017/01/25 15:55:56, Mathieu Perreault wrote: > 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? 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? https://codereview.chromium.org/2650033004/diff/60001/chrome/browser/ui/autof... chrome/browser/ui/autofill/chrome_autofill_client.cc:423: #endif On 2017/01/25 15:50:48, elawrence wrote: > 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 > } Done. (With some modification because of mathp's comment above about duplicating the code)
autofill lgtm https://codereview.chromium.org/2650033004/diff/100001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/chrome_autofill_client.cc (right): https://codereview.chromium.org/2650033004/diff/100001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:395: if (chrome::ShowWebsiteSettings( nit: do we need the if and return, given we are effectively at the end of the function? https://codereview.chromium.org/2650033004/diff/100001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:396: chrome::FindBrowserWithWebContents(web_contents()), web_contents())) { nit: if seems to be good practice (at least in this file) to first check the result from FindBrowserWithWebContents before continuing https://codereview.chromium.org/2650033004/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/2650033004/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_commands.cc:904: if (!entry) I think it's good to carry over the comment about how this can be null
> https://codereview.chromium.org/2650033004/diff/100001/chrome/browser/ui/auto... > chrome/browser/ui/autofill/chrome_autofill_client.cc:395: if > (chrome::ShowWebsiteSettings( > nit: do we need the if and return, given we are effectively at the end of the > function? I made a bad suggestion in CL #4. The #else should be #endif so fallthrough works . > https://codereview.chromium.org/2650033004/diff/100001/chrome/browser/ui/brow... > chrome/browser/ui/browser_commands.cc:904: if (!entry) > I think it's good to carry over the comment about how this can be null The original comment about the conditions under which this could be null was a bit misleading.
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
estark@chromium.org changed reviewers: + achuith@chromium.org, engedy@chromium.org, msw@chromium.org
+achuith for chrome/browser/chromeos +engedy for components/password_manager +msw for chrome/browser/ui The chrome/browser/ui change is to move some logic into chrome::ShowWebsiteSettings to avoid callers from having to duplicate it. PTAL, thanks! https://codereview.chromium.org/2650033004/diff/100001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/chrome_autofill_client.cc (right): https://codereview.chromium.org/2650033004/diff/100001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:395: if (chrome::ShowWebsiteSettings( On 2017/01/25 17:03:41, Mathieu Perreault wrote: > nit: do we need the if and return, given we are effectively at the end of the > function? Fixed the fallthrough and added comment. This is set up so that if ShowWebsiteSettings returns false (e.g. because of no navigation entry) we fall through to the section below where we open the URL directly. https://codereview.chromium.org/2650033004/diff/100001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:396: chrome::FindBrowserWithWebContents(web_contents()), web_contents())) { On 2017/01/25 17:03:41, Mathieu Perreault wrote: > nit: if seems to be good practice (at least in this file) to first check the > result from FindBrowserWithWebContents before continuing Done. https://codereview.chromium.org/2650033004/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/2650033004/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_commands.cc:904: if (!entry) On 2017/01/25 17:03:41, Mathieu Perreault wrote: > I think it's good to carry over the comment about how this can be null As elawrence mentioned, I don't think we should try to document why GetVisibleEntry() can be null; the method documentation is clear that it can be null, so I don't think we have to explain why we are handling that case.
components/password_manager LGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/25 17:23:15, estark wrote: > +achuith for chrome/browser/chromeos rubberstamp lgtm
c/b/ui lgtm with nits https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/chrome_autofill_client.cc (right): https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:393: // On desktop platform, open Page Info, which briefly explains the HTTP nit: 'platforms' https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:396: if (browser && chrome::ShowWebsiteSettings(browser, web_contents())) { nit: curlies not needed https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:399: // Otherwise fall through to the section below that opens the URL directly. optional nit: this comment seems unnecessary https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/location_icon_decoration.mm (right): https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_icon_decoration.mm:17: #include "content/public/browser/navigation_controller.h" nit: remove https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_icon_decoration.mm:18: #include "content/public/browser/navigation_entry.h" nit: remove https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_icon_decoration.mm:25: using content::NavigationController; nit: remove https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_icon_decoration.mm:26: using content::NavigationEntry; nit: remove https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_icon_view.cc (right): https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_icon_view.cc:15: #include "components/security_state/core/security_state.h" nit: remove https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_icon_view.cc:16: #include "content/public/browser/navigation_controller.h" nit: remove https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_icon_view.cc:17: #include "content/public/browser/navigation_entry.h" nit: remove https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_icon_view.cc:22: using content::NavigationEntry; nit: remove
lgtm https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/chrome_autofill_client.cc (right): https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:399: // Otherwise fall through to the section below that opens the URL directly. I like this comment here, since it's rather easy to overlook the fallthrough, as evidenced by the fact that we did in an earlier CL. :)
Thanks all. https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/chrome_autofill_client.cc (right): https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:393: // On desktop platform, open Page Info, which briefly explains the HTTP On 2017/01/25 20:22:40, msw wrote: > nit: 'platforms' Done. https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:396: if (browser && chrome::ShowWebsiteSettings(browser, web_contents())) { On 2017/01/25 20:22:40, msw wrote: > nit: curlies not needed Done. https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:399: // Otherwise fall through to the section below that opens the URL directly. On 2017/01/25 20:22:40, msw wrote: > optional nit: this comment seems unnecessary Acknowledged, decided to keep. Per Eric's comment we've gotten confused a couple of times about the intended flow in the course of reviewing this CL. https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/location_icon_decoration.mm (right): https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_icon_decoration.mm:17: #include "content/public/browser/navigation_controller.h" On 2017/01/25 20:22:40, msw wrote: > nit: remove Done. https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_icon_decoration.mm:18: #include "content/public/browser/navigation_entry.h" On 2017/01/25 20:22:40, msw wrote: > nit: remove Done. https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_icon_decoration.mm:25: using content::NavigationController; On 2017/01/25 20:22:40, msw wrote: > nit: remove Done. https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/location_icon_decoration.mm:26: using content::NavigationEntry; On 2017/01/25 20:22:40, msw wrote: > nit: remove Done. https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/location_bar/location_icon_view.cc (right): https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_icon_view.cc:15: #include "components/security_state/core/security_state.h" On 2017/01/25 20:22:40, msw wrote: > nit: remove Done. https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_icon_view.cc:16: #include "content/public/browser/navigation_controller.h" On 2017/01/25 20:22:40, msw wrote: > nit: remove Done. https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_icon_view.cc:17: #include "content/public/browser/navigation_entry.h" On 2017/01/25 20:22:40, msw wrote: > nit: remove Done. https://codereview.chromium.org/2650033004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/location_bar/location_icon_view.cc:22: using content::NavigationEntry; On 2017/01/25 20:22:40, msw wrote: > nit: remove Done.
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, msw@chromium.org, achuith@chromium.org, engedy@chromium.org, elawrence@chromium.org Link to the patchset: https://codereview.chromium.org/2650033004/#ps140001 (title: "msw comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for chrome/browser/ui/browser_commands.cc:
While running git apply --index -p1;
error: patch failed: chrome/browser/ui/browser_commands.cc:25
error: chrome/browser/ui/browser_commands.cc: patch does not apply
Patch: chrome/browser/ui/browser_commands.cc
Index: chrome/browser/ui/browser_commands.cc
diff --git a/chrome/browser/ui/browser_commands.cc
b/chrome/browser/ui/browser_commands.cc
index
49c5ad4ae3e8bdfa290552ad415ad50505e79f48..7dc18933553425ab72e281ba280e9af5177f5b26
100644
--- a/chrome/browser/ui/browser_commands.cc
+++ b/chrome/browser/ui/browser_commands.cc
@@ -25,6 +25,7 @@
#include "chrome/browser/search/search.h"
#include "chrome/browser/sessions/session_service_factory.h"
#include "chrome/browser/sessions/tab_restore_service_factory.h"
+#include "chrome/browser/ssl/security_state_tab_helper.h"
#include "chrome/browser/translate/chrome_translate_client.h"
#include "chrome/browser/ui/accelerator_utils.h"
#include "chrome/browser/ui/autofill/save_card_bubble_controller_impl.h"
@@ -62,6 +63,7 @@
#include "components/favicon/content/content_favicon_driver.h"
#include "components/google/core/browser/google_util.h"
#include "components/prefs/pref_service.h"
+#include "components/security_state/core/security_state.h"
#include "components/sessions/core/live_tab_context.h"
#include "components/sessions/core/tab_restore_service.h"
#include "components/signin/core/browser/signin_header_helper.h"
@@ -896,13 +898,21 @@ void ShowFindBar(Browser* browser) {
browser->GetFindBarController()->Show();
}
-void ShowWebsiteSettings(Browser* browser,
- content::WebContents* web_contents,
- const GURL& url,
- const security_state::SecurityInfo& security_info) {
+bool ShowWebsiteSettings(Browser* browser, content::WebContents* web_contents)
{
+ content::NavigationEntry* entry =
+ web_contents->GetController().GetVisibleEntry();
+ if (!entry)
+ return false;
+
+ SecurityStateTabHelper* helper =
+ SecurityStateTabHelper::FromWebContents(web_contents);
+ security_state::SecurityInfo security_info;
+ helper->GetSecurityInfo(&security_info);
+
browser->window()->ShowWebsiteSettings(
Profile::FromBrowserContext(web_contents->GetBrowserContext()),
- web_contents, url, security_info);
+ web_contents, entry->GetVirtualURL(), security_info);
+ return true;
}
void Print(Browser* browser) {
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, msw@chromium.org, achuith@chromium.org, engedy@chromium.org, elawrence@chromium.org Link to the patchset: https://codereview.chromium.org/2650033004/#ps160001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1485466261956980,
"parent_rev": "7563d11df963b1807dc96bc530a6a870fb1db222", "commit_rev":
"c83163e85da7ed3dc54a0dd21bf1954f2643459c"}
Message was sent while issue was closed.
Description was changed from ========== Show Page Info from Form-Not-Secure 'Learn more' link Instead of directly opening a tab to the Help Center, the "Learn more" link should open the Page Info bubble. From there the user can click another link to open the Help Center, if they want. BUG=684682 TEST=Enable #enable-http-form-warning. Visit http://rsolomakhin.github.io. Fill out the Name/Password form and submit it and save the password. Go back to http://rsolomakhin.github.io and focus the Password field. Observe the "Login not secure" warning in the autofill dropdown and click it. Observe that the Page Info bubble opens down from the omnibox. ========== to ========== Show Page Info from Form-Not-Secure 'Learn more' link Instead of directly opening a tab to the Help Center, the "Learn more" link should open the Page Info bubble. From there the user can click another link to open the Help Center, if they want. BUG=684682 TEST=Enable #enable-http-form-warning. Visit http://rsolomakhin.github.io. Fill out the Name/Password form and submit it and save the password. Go back to http://rsolomakhin.github.io and focus the Password field. Observe the "Login not secure" warning in the autofill dropdown and click it. Observe that the Page Info bubble opens down from the omnibox. Review-Url: https://codereview.chromium.org/2650033004 Cr-Commit-Position: refs/heads/master@{#446469} Committed: https://chromium.googlesource.com/chromium/src/+/c83163e85da7ed3dc54a0dd21bf1... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/c83163e85da7ed3dc54a0dd21bf1... |
