Commit f22e0300 authored by Vasilii Sukhanov's avatar Vasilii Sukhanov Committed by Commit Bot

Implement favicons for the password dropdown on desktop.

TBR=pkotwicz@chromium.org

Bug: 872847
Change-Id: Iae5cb8c652618511101b6516c918ee723712fe64
Reviewed-on: https://chromium-review.googlesource.com/1169803
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: default avatarVasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: default avatarVadym Doroshenko <dvadym@chromium.org>
Reviewed-by: default avatarTommy Martino <tmartino@chromium.org>
Reviewed-by: default avatarFabio Tirelo <ftirelo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582546}
parent bb4f6832
......@@ -18,6 +18,7 @@
#include "build/build_config.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/browsing_data/browsing_data_helper.h"
#include "chrome/browser/favicon/favicon_service_factory.h"
#include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
#include "chrome/browser/password_manager/password_store_factory.h"
#include "chrome/browser/prerender/prerender_contents.h"
......@@ -908,6 +909,11 @@ ChromePasswordManagerClient::GetPasswordRequirementsService() {
Profile::FromBrowserContext(web_contents()->GetBrowserContext()));
}
favicon::FaviconService* ChromePasswordManagerClient::GetFaviconService() {
return FaviconServiceFactory::GetForProfile(
profile_, ServiceAccessType::EXPLICIT_ACCESS);
}
void ChromePasswordManagerClient::UpdateFormManagers() {
password_manager_.UpdateFormManagers();
}
......
......@@ -109,6 +109,7 @@ class ChromePasswordManagerClient
const password_manager::LogManager* GetLogManager() const override;
password_manager::PasswordRequirementsService*
GetPasswordRequirementsService() override;
favicon::FaviconService* GetFaviconService() override;
void UpdateFormManagers() override;
// autofill::mojom::PasswordManagerClient overrides.
......
......@@ -209,6 +209,9 @@ ui::NativeTheme::ColorId AutofillPopupLayoutModel::GetValueFontColorIDForRow(
gfx::ImageSkia AutofillPopupLayoutModel::GetIconImage(size_t index) const {
std::vector<autofill::Suggestion> suggestions = delegate_->GetSuggestions();
if (!suggestions[index].custom_icon.IsEmpty())
return suggestions[index].custom_icon.AsImageSkia();
const base::string16& icon_str = suggestions[index].icon;
if (icon_str.empty())
return gfx::ImageSkia();
......@@ -225,6 +228,10 @@ gfx::ImageSkia AutofillPopupLayoutModel::GetIconImage(size_t index) const {
return gfx::CreateVectorIcon(toolbar::kHttpsInvalidIcon, kIconSize,
gfx::kGoogleRed700);
}
if (icon_str == base::ASCIIToUTF16("keyIcon"))
return gfx::CreateVectorIcon(kKeyIcon, kIconSize, gfx::kChromeIconGrey);
if (icon_str == base::ASCIIToUTF16("globeIcon"))
return gfx::CreateVectorIcon(kGlobeIcon, kIconSize, gfx::kChromeIconGrey);
// For other suggestion entries, get icon from PNG files.
int icon_id = GetIconResourceID(icon_str);
......
......@@ -69,6 +69,25 @@ namespace autofill {
namespace {
// Describes the possible layouts which can be applied to the rows in the popup.
enum class PopupItemLayoutType {
kLeadingIcon, // Icon (if any) shown on the leading (left in LTR) side.
kTrailingIcon // Icon (if any) shown on the trailing (right in LTR) side.
};
// Returns the icon alignment for the drop down of type |frontend_id|. See
// PopupItemId for allowed values.
PopupItemLayoutType GetLayoutType(int frontend_id) {
switch (frontend_id) {
case autofill::PopupItemId::POPUP_ITEM_ID_USERNAME_ENTRY:
case autofill::PopupItemId::POPUP_ITEM_ID_PASSWORD_ENTRY:
case autofill::PopupItemId::POPUP_ITEM_ID_GENERATE_PASSWORD_ENTRY:
return PopupItemLayoutType::kLeadingIcon;
default:
return PopupItemLayoutType::kTrailingIcon;
}
}
// Container view that holds one child view and limits its width to the
// specified maximum.
class ConstrainedWidthView : public views::View {
......@@ -122,7 +141,6 @@ class AutofillPopupItemView : public AutofillPopupRowView {
void CreateContent() override;
void RefreshStyle() override;
protected:
virtual int GetPrimaryTextStyle() = 0;
virtual views::View* CreateValueLabel();
// The description view can be nullptr.
......@@ -132,6 +150,7 @@ class AutofillPopupItemView : public AutofillPopupRowView {
views::Label* CreateDescriptionLabelInternal() const;
private:
void AddIcon(gfx::ImageSkia icon);
void AddSpacerWithSize(int spacer_width,
bool resize,
views::BoxLayout* layout);
......@@ -333,6 +352,17 @@ void AutofillPopupItemView::CreateContent() {
layout->set_minimum_cross_axis_size(
views::MenuConfig::instance().touchable_menu_height + extra_height_);
const gfx::ImageSkia icon =
controller->layout_model().GetIconImage(line_number_);
int frontend_id = controller->GetSuggestionAt(line_number_).frontend_id;
if (!icon.isNull() &&
GetLayoutType(frontend_id) == PopupItemLayoutType::kLeadingIcon) {
AddIcon(icon);
AddSpacerWithSize(views::MenuConfig::instance().item_horizontal_padding,
/*resize=*/false, layout);
}
AddChildView(CreateValueLabel());
AddSpacerWithSize(views::MenuConfig::instance().item_horizontal_padding,
......@@ -342,16 +372,11 @@ void AutofillPopupItemView::CreateContent() {
if (description_label)
AddChildView(description_label);
const gfx::ImageSkia icon =
controller->layout_model().GetIconImage(line_number_);
if (!icon.isNull()) {
if (!icon.isNull() &&
GetLayoutType(frontend_id) == PopupItemLayoutType::kTrailingIcon) {
AddSpacerWithSize(views::MenuConfig::instance().item_horizontal_padding,
/*resize=*/false, layout);
auto* image_view = new views::ImageView();
image_view->SetImage(icon);
AddChildView(image_view);
AddIcon(icon);
}
}
......@@ -393,6 +418,12 @@ views::Label* AutofillPopupItemView::CreateDescriptionLabelInternal() const {
return nullptr;
}
void AutofillPopupItemView::AddIcon(gfx::ImageSkia icon) {
auto* image_view = new views::ImageView();
image_view->SetImage(icon);
AddChildView(image_view);
}
void AutofillPopupItemView::AddSpacerWithSize(int spacer_width,
bool resize,
views::BoxLayout* layout) {
......
......@@ -19,10 +19,10 @@ Suggestion::Suggestion(const Suggestion& other)
frontend_id(other.frontend_id),
value(other.value),
label(other.label),
custom_icon(other.custom_icon),
icon(other.icon),
match(other.match),
is_value_bold(other.is_value_bold) {
}
is_value_bold(other.is_value_bold) {}
Suggestion::Suggestion(const base::string16& v)
: frontend_id(0),
......@@ -43,7 +43,6 @@ Suggestion::Suggestion(const std::string& v,
is_value_bold(false) {
}
Suggestion::~Suggestion() {
}
Suggestion::~Suggestion() = default;
} // namespace autofill
......@@ -8,6 +8,7 @@
#include <string>
#include "base/strings/string16.h"
#include "ui/gfx/image/image.h"
namespace autofill {
......@@ -45,6 +46,9 @@ struct Suggestion {
base::string16 value;
base::string16 label;
// Contains an image to display for the suggestion.
gfx::Image custom_icon;
// If |custom_icon| is empty, the name of the fallback built-in icon.
base::string16 icon;
MatchMode match;
bool is_value_bold; // true if |value| should be displayed in bold type face.
......
......@@ -188,6 +188,7 @@ jumbo_static_library("browser") {
"//components/autofill/core/browser",
"//components/autofill/core/browser/proto",
"//components/autofill/core/common",
"//components/favicon/core",
"//components/keyed_service/core",
"//components/os_crypt",
"//components/password_manager/core/browser/form_parsing",
......@@ -426,6 +427,7 @@ source_set("unit_tests") {
"//components/autofill/core/browser:test_support",
"//components/autofill/core/browser/proto",
"//components/autofill/core/common",
"//components/favicon/core/test:test_support",
"//components/os_crypt",
"//components/os_crypt:test_support",
"//components/password_manager/core/browser:proto",
......@@ -449,6 +451,7 @@ source_set("unit_tests") {
"//testing/gtest",
"//third_party/sqlite",
"//ui/base",
"//ui/gfx:test_support",
"//url",
]
......
include_rules = [
"+components/autofill/core/browser",
"+components/favicon/core",
"+components/keyed_service/core",
"+components/pref_registry",
"+components/security_state",
......
......@@ -26,6 +26,7 @@
#include "components/autofill/core/common/autofill_constants.h"
#include "components/autofill/core/common/autofill_data_validation.h"
#include "components/autofill/core/common/autofill_util.h"
#include "components/favicon/core/favicon_util.h"
#include "components/password_manager/core/browser/android_affiliation/affiliation_utils.h"
#include "components/password_manager/core/browser/password_manager_client.h"
#include "components/password_manager/core/browser/password_manager_driver.h"
......@@ -81,6 +82,7 @@ base::string16 GetUsernameFromSuggestion(const base::string16& suggestion) {
void AppendSuggestionIfMatching(
const base::string16& field_suggestion,
const base::string16& field_contents,
const gfx::Image& custom_icon,
const std::string& signon_realm,
bool show_all,
bool is_password_field,
......@@ -103,6 +105,9 @@ void AppendSuggestionIfMatching(
base::CompareCase::SENSITIVE)
? autofill::Suggestion::PREFIX_MATCH
: autofill::Suggestion::SUBSTRING_MATCH;
suggestion.custom_icon = custom_icon;
// The UI code will pick up an icon from the resources based on the string.
suggestion.icon = base::ASCIIToUTF16("globeIcon");
suggestions->push_back(suggestion);
}
}
......@@ -114,16 +119,17 @@ void AppendSuggestionIfMatching(
// substring or a prefix based on the flag.
void GetSuggestions(const autofill::PasswordFormFillData& fill_data,
const base::string16& current_username,
const gfx::Image& custom_icon,
bool show_all,
bool is_password_field,
std::vector<autofill::Suggestion>* suggestions) {
AppendSuggestionIfMatching(
fill_data.username_field.value, current_username,
fill_data.username_field.value, current_username, custom_icon,
fill_data.preferred_realm, show_all, is_password_field,
fill_data.password_field.value.size(), suggestions);
for (const auto& login : fill_data.additional_logins) {
AppendSuggestionIfMatching(login.first, current_username,
AppendSuggestionIfMatching(login.first, current_username, custom_icon,
login.second.realm, show_all, is_password_field,
login.second.password.size(), suggestions);
}
......@@ -207,6 +213,7 @@ void PasswordAutofillManager::OnAddPasswordFormMapping(
return;
login_to_password_info_[key] = fill_data;
RequestFavicon(fill_data.origin);
}
void PasswordAutofillManager::OnShowPasswordSuggestions(
......@@ -223,7 +230,7 @@ void PasswordAutofillManager::OnShowPasswordSuggestions(
NOTREACHED();
return;
}
GetSuggestions(fill_data_it->second, typed_username,
GetSuggestions(fill_data_it->second, typed_username, page_favicon_,
(options & autofill::SHOW_ALL) != 0,
(options & autofill::IS_PASSWORD_FIELD) != 0, &suggestions);
......@@ -273,14 +280,15 @@ bool PasswordAutofillManager::MaybeShowPasswordSuggestionsWithGeneration(
return false;
std::vector<autofill::Suggestion> suggestions;
GetSuggestions(login_to_password_info_.begin()->second, base::string16(),
true /* show_all */, true /* is_password_field */,
&suggestions);
page_favicon_, true /* show_all */,
true /* is_password_field */, &suggestions);
form_data_key_ = login_to_password_info_.begin()->first;
// Add 'Generation' option.
// The UI code will pick up an icon from the resources based on the string.
autofill::Suggestion suggestion(
l10n_util::GetStringUTF8(IDS_PASSWORD_MANAGER_GENERATE_PASSWORD),
std::string(), std::string(),
std::string(), std::string("keyIcon"),
autofill::POPUP_ITEM_ID_GENERATE_PASSWORD_ENTRY);
suggestions.push_back(suggestion);
......@@ -304,6 +312,8 @@ bool PasswordAutofillManager::MaybeShowPasswordSuggestionsWithGeneration(
void PasswordAutofillManager::DidNavigateMainFrame() {
login_to_password_info_.clear();
favicon_tracker_.TryCancelAll();
page_favicon_ = gfx::Image();
}
bool PasswordAutofillManager::FillSuggestionForTest(
......@@ -434,4 +444,21 @@ bool PasswordAutofillManager::FindLoginInfo(
return true;
}
void PasswordAutofillManager::RequestFavicon(const GURL& url) {
if (!password_client_)
return;
favicon::GetFaviconImageForPageURL(
password_client_->GetFaviconService(), url,
favicon_base::IconType::kFavicon,
base::BindRepeating(&PasswordAutofillManager::OnFaviconReady,
weak_ptr_factory_.GetWeakPtr()),
&favicon_tracker_);
}
void PasswordAutofillManager::OnFaviconReady(
const favicon_base::FaviconImageResult& result) {
if (!result.image.IsEmpty())
page_favicon_ = result.image;
}
} // namespace password_manager
......@@ -10,10 +10,16 @@
#include "base/callback.h"
#include "base/i18n/rtl.h"
#include "base/macros.h"
#include "base/task/cancelable_task_tracker.h"
#include "components/autofill/core/browser/autofill_client.h"
#include "components/autofill/core/browser/autofill_popup_delegate.h"
#include "components/autofill/core/common/password_form_fill_data.h"
#include "components/password_manager/core/browser/password_manager_metrics_util.h"
#include "ui/gfx/image/image.h"
namespace favicon_base {
struct FaviconImageResult;
}
namespace gfx {
class RectF;
......@@ -118,13 +124,20 @@ class PasswordAutofillManager : public autofill::AutofillPopupDelegate {
// Finds login information for a |node| that was previously filled.
bool FindLoginInfo(int key, autofill::PasswordFormFillData* found_password);
// Creates suggestion and records the metrics for the "Form not secure
// warning".
autofill::Suggestion CreateFormNotSecureWarning();
// Makes a request to the favicon service for the icon of |url|.
void RequestFavicon(const GURL& url);
// Called when the favicon was retrieved. When the icon is not ready or
// unavailable a fallback globe icon is used. The request to the favicon
// store is canceled on navigation.
void OnFaviconReady(const favicon_base::FaviconImageResult& result);
// The logins we have filled so far with their associated info.
LoginToPasswordInfoMap login_to_password_info_;
// Contains the favicon for the credentials offered on the current page.
gfx::Image page_favicon_;
// When the autofill popup should be shown, |form_data_key_| identifies the
// right password info in |login_to_password_info_|.
int form_data_key_;
......@@ -139,6 +152,9 @@ class PasswordAutofillManager : public autofill::AutofillPopupDelegate {
// If not null then it will be called in destructor.
base::OnceClosure deletion_callback_;
// Used to track a requested favicon.
base::CancelableTaskTracker favicon_tracker_;
base::WeakPtrFactory<PasswordAutofillManager> weak_ptr_factory_;
DISALLOW_COPY_AND_ASSIGN(PasswordAutofillManager);
......
......@@ -22,6 +22,7 @@
#include "components/autofill/core/common/autofill_switches.h"
#include "components/autofill/core/common/form_field_data.h"
#include "components/autofill/core/common/password_form_fill_data.h"
#include "components/favicon/core/test/mock_favicon_service.h"
#include "components/password_manager/core/browser/password_manager.h"
#include "components/password_manager/core/browser/password_manager_metrics_util.h"
#include "components/password_manager/core/browser/stub_password_manager_client.h"
......@@ -36,6 +37,7 @@
#include "testing/gtest/include/gtest/gtest.h"
#include "ui/base/l10n/l10n_util.h"
#include "ui/gfx/geometry/rect_f.h"
#include "ui/gfx/image/image_unittest_util.h"
#if defined(OS_ANDROID)
#include "base/android/build_info.h"
......@@ -50,10 +52,13 @@ const char kAliceUsername[] = "alice";
const char kAlicePassword[] = "password";
using autofill::Suggestion;
using autofill::SuggestionVectorIconsAre;
using autofill::SuggestionVectorIdsAre;
using autofill::SuggestionVectorValuesAre;
using autofill::SuggestionVectorLabelsAre;
using testing::_;
using testing::ElementsAreArray;
using testing::Return;
using UkmEntry = ukm::builders::PageWithPassword;
......@@ -85,6 +90,7 @@ class TestPasswordManagerClient : public StubPasswordManagerClient {
const GURL& GetMainFrameURL() const override { return main_frame_url_; }
MOCK_METHOD0(GeneratePassword, void());
MOCK_METHOD0(GetFaviconService, favicon::FaviconService*());
private:
MockPasswordManagerDriver driver_;
......@@ -141,6 +147,15 @@ std::vector<base::string16> GetSuggestionList(
return credentials;
}
std::vector<base::string16> GetIconsList(std::vector<std::string> icons) {
std::vector<base::string16> ret(icons.size());
std::transform(icons.begin(), icons.end(), ret.begin(), &base::ASCIIToUTF16);
// On older Android versions the item "Manage passwords" is absent.
if (!IsPreLollipopAndroid())
ret.push_back(base::string16());
return ret;
}
} // namespace
class PasswordAutofillManagerTest : public testing::Test {
......@@ -168,8 +183,16 @@ class PasswordAutofillManagerTest : public testing::Test {
autofill::AutofillClient* autofill_client) {
password_autofill_manager_.reset(new PasswordAutofillManager(
client->mock_driver(), autofill_client, client));
favicon::MockFaviconService favicon_service;
EXPECT_CALL(*client, GetFaviconService())
.WillOnce(Return(&favicon_service));
EXPECT_CALL(favicon_service,
GetFaviconImageForPageURL(fill_data_.origin, _, _));
password_autofill_manager_->OnAddPasswordFormMapping(fill_data_id_,
fill_data_);
testing::Mock::VerifyAndClearExpectations(client);
// Suppress the warnings in the tests.
EXPECT_CALL(*client, GetFaviconService()).WillRepeatedly(Return(nullptr));
}
protected:
......@@ -240,7 +263,7 @@ TEST_F(PasswordAutofillManagerTest, PreviewSuggestion) {
fill_data_id(), test_username_));
}
// Test that the popup is marked as visible after recieving password
// Test that the popup is marked as visible after receiving password
// suggestions.
TEST_F(PasswordAutofillManagerTest, ExternalDelegatePasswordSuggestions) {
for (bool is_suggestion_on_password_field : {false, true}) {
......@@ -257,10 +280,18 @@ TEST_F(PasswordAutofillManagerTest, ExternalDelegatePasswordSuggestions) {
data.password_field.value = test_password_;
data.preferred_realm = "http://foo.com/";
int dummy_key = 0;
favicon::MockFaviconService favicon_service;
EXPECT_CALL(*client, GetFaviconService())
.WillOnce(Return(&favicon_service));
favicon_base::FaviconImageCallback callback;
EXPECT_CALL(favicon_service, GetFaviconImageForPageURL(data.origin, _, _))
.WillOnce(DoAll(testing::SaveArg<1>(&callback), Return(1)));
password_autofill_manager_->OnAddPasswordFormMapping(dummy_key, data);
EXPECT_CALL(*client->mock_driver(),
FillSuggestion(test_username_, test_password_));
// Resolve the favicon.
favicon_base::FaviconImageResult image_result;
image_result.image = gfx::test::CreateImage(16, 16);
callback.Run(image_result);
std::vector<autofill::PopupItemId> ids = {
is_suggestion_on_password_field
......@@ -269,18 +300,25 @@ TEST_F(PasswordAutofillManagerTest, ExternalDelegatePasswordSuggestions) {
if (!IsPreLollipopAndroid()) {
ids.push_back(autofill::POPUP_ITEM_ID_ALL_SAVED_PASSWORDS_ENTRY);
}
std::vector<Suggestion> suggestions;
EXPECT_CALL(
*autofill_client,
ShowAutofillPopup(
_, _, SuggestionVectorIdsAre(testing::ElementsAreArray(ids)), false,
_));
_))
.WillOnce(testing::SaveArg<2>(&suggestions));
int show_suggestion_options =
is_suggestion_on_password_field ? autofill::IS_PASSWORD_FIELD : 0;
password_autofill_manager_->OnShowPasswordSuggestions(
dummy_key, base::i18n::RIGHT_TO_LEFT, base::string16(),
show_suggestion_options, element_bounds);
ASSERT_GE(suggestions.size(), 1u);
EXPECT_TRUE(gfx::test::AreImagesEqual(suggestions[0].custom_icon,
image_result.image));
EXPECT_CALL(*client->mock_driver(),
FillSuggestion(test_username_, test_password_));
// Accepting a suggestion should trigger a call to hide the popup.
EXPECT_CALL(*autofill_client, HideAutofillPopup());
password_autofill_manager_->DidAcceptSuggestion(
......@@ -757,16 +795,22 @@ TEST_F(PasswordAutofillManagerTest,
data.origin = GURL("https://foo.test");
int dummy_key = 0;
favicon::MockFaviconService favicon_service;
EXPECT_CALL(*client, GetFaviconService()).WillOnce(Return(&favicon_service));
EXPECT_CALL(favicon_service, GetFaviconImageForPageURL(data.origin, _, _));
password_autofill_manager_->OnAddPasswordFormMapping(dummy_key, data);
// Bring up the drop-down with the generaion option.
base::string16 generation_string =
l10n_util::GetStringUTF16(IDS_PASSWORD_MANAGER_GENERATE_PASSWORD);
EXPECT_CALL(*autofill_client,
EXPECT_CALL(
*autofill_client,
ShowAutofillPopup(
element_bounds, base::i18n::RIGHT_TO_LEFT,
SuggestionVectorValuesAre(testing::ElementsAreArray(
AllOf(SuggestionVectorValuesAre(ElementsAreArray(
GetSuggestionList({test_username_, generation_string}))),
SuggestionVectorIconsAre(
ElementsAreArray(GetIconsList({"globeIcon", "keyIcon"})))),
false, _));
EXPECT_TRUE(
password_autofill_manager_->MaybeShowPasswordSuggestionsWithGeneration(
......
......@@ -92,4 +92,8 @@ PasswordManagerClient::GetPasswordRequirementsService() {
return nullptr;
}
favicon::FaviconService* PasswordManagerClient::GetFaviconService() {
return nullptr;
}
} // namespace password_manager
......@@ -21,6 +21,10 @@ namespace autofill {
class AutofillManager;
}
namespace favicon {
class FaviconService;
}
class GURL;
#if defined(SAFE_BROWSING_DB_LOCAL)
......@@ -257,6 +261,9 @@ class PasswordManagerClient {
// incognito context. Callers should guard against this.
virtual PasswordRequirementsService* GetPasswordRequirementsService();
// Returns the favicon service used to retrieve icons for an origin.
virtual favicon::FaviconService* GetFaviconService();
// Causes all live PasswordFormManager objects to query the password store
// again. Results in updating the fill information on the page.
virtual void UpdateFormManagers() {}
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment