From 00ba26eacd90c64f91bf93f6de684aad77333f92 Mon Sep 17 00:00:00 2001 From: Veronique Nguyen Date: Fri, 25 Feb 2022 14:57:17 +0000 Subject: [PATCH] [iOS] Add Favicon in the Password Manager - Add feature flag to enable favicons for the Password Manager. - Extract favicon size to a new file called favicon_constants. - Add favicon in the Password Manager. - Add unit tests. Demo: https://drive.google.com/drive/folders/14K_ipBwlOUpfnABYIXPKh9lL2IcVUODq?resourcekey=0-gyEfovXg7lx9I8tDiEhF_Q&usp=sharing Bug: 1297226 Change-Id: Iab3587442556e8b63231f59208b5efa1668297a2 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3472921 Reviewed-by: Vasilii Sukhanov Reviewed-by: Gauthier Ambard Commit-Queue: Veronique Nguyen Cr-Commit-Position: refs/heads/main@{#975120} --- chrome/browser/flag-metadata.json | 5 + .../core/common/password_manager_features.cc | 4 + .../core/common/password_manager_features.h | 1 + ios/chrome/browser/flags/about_flags.mm | 5 + .../flags/ios_chrome_flag_descriptions.cc | 6 + .../flags/ios_chrome_flag_descriptions.h | 5 + ios/chrome/browser/ui/bookmarks/BUILD.gn | 1 + .../bookmarks/bookmark_home_shared_state.mm | 9 +- ios/chrome/browser/ui/context_menu/BUILD.gn | 1 + .../context_menu_configuration_provider.mm | 6 +- ios/chrome/browser/ui/favicon/BUILD.gn | 8 + .../browser/ui/favicon/favicon_constants.h | 18 ++ .../browser/ui/favicon/favicon_constants.mm | 13 ++ ios/chrome/browser/ui/history/BUILD.gn | 1 + .../browser/ui/history/history_mediator.mm | 10 +- ios/chrome/browser/ui/omnibox/BUILD.gn | 1 + .../browser/ui/omnibox/omnibox_mediator.mm | 13 +- ios/chrome/browser/ui/reading_list/BUILD.gn | 1 + .../ui/reading_list/reading_list_mediator.mm | 8 +- ios/chrome/browser/ui/recent_tabs/BUILD.gn | 1 + .../ui/recent_tabs/recent_tabs_mediator.mm | 10 +- ios/chrome/browser/ui/settings/BUILD.gn | 1 + .../browser/ui/settings/password/BUILD.gn | 6 + .../password/passwords_coordinator.mm | 9 +- .../ui/settings/password/passwords_mediator.h | 6 +- .../settings/password/passwords_mediator.mm | 26 ++- .../password/passwords_mediator_unittest.mm | 11 +- .../passwords_table_view_controller.h | 4 + .../passwords_table_view_controller.mm | 177 +++++++++++++++--- ...asswords_table_view_controller_unittest.mm | 45 ++++- .../search_engine_table_view_controller.mm | 7 +- .../ui/table_view/cells/table_view_url_item.h | 2 + .../table_view/cells/table_view_url_item.mm | 3 + .../chrome_table_view_controller_test.h | 12 ++ .../chrome_table_view_controller_test.mm | 20 ++ 35 files changed, 380 insertions(+), 76 deletions(-) create mode 100644 ios/chrome/browser/ui/favicon/favicon_constants.h create mode 100644 ios/chrome/browser/ui/favicon/favicon_constants.mm diff --git a/chrome/browser/flag-metadata.json b/chrome/browser/flag-metadata.json index c4b25400ffbf42..b4f5d6e92f5992 100644 --- a/chrome/browser/flag-metadata.json +++ b/chrome/browser/flag-metadata.json @@ -2085,6 +2085,11 @@ // features. "expiry_milestone": -1 }, + { + "name": "enable-favicon-passwords", + "owners": [ "veronguyen", "gambard" ], + "expiry_milestone": 105 + }, { "name": "enable-fenced-frames", "owners": [ "dom", "shivanisha", "chrome-fenced-frames@google.com" ], diff --git a/components/password_manager/core/common/password_manager_features.cc b/components/password_manager/core/common/password_manager_features.cc index 2ed6eddf91e6b8..dbd0db7e6b839d 100644 --- a/components/password_manager/core/common/password_manager_features.cc +++ b/components/password_manager/core/common/password_manager_features.cc @@ -27,6 +27,10 @@ const base::Feature kDetectFormSubmissionOnFormClear = { #endif }; +// Enables favicons in Password Manager. +const base::Feature kEnableFaviconForPasswords{ + "EnableFaviconForPasswords", base::FEATURE_DISABLED_BY_DEFAULT}; + // Enables UI that allows the user to create a strong password even if the field // wasn't parsed as a new password field. // TODO(crbug/1181254): Remove once it's launched. diff --git a/components/password_manager/core/common/password_manager_features.h b/components/password_manager/core/common/password_manager_features.h index 266fa016f35a5d..d45cba6de4480c 100644 --- a/components/password_manager/core/common/password_manager_features.h +++ b/components/password_manager/core/common/password_manager_features.h @@ -21,6 +21,7 @@ namespace features { extern const base::Feature kBiometricTouchToFill; extern const base::Feature kDetectFormSubmissionOnFormClear; +extern const base::Feature kEnableFaviconForPasswords; extern const base::Feature kEnableManualPasswordGeneration; extern const base::Feature kEnableOverwritingPlaceholderUsernames; extern const base::Feature kEnablePasswordsAccountStorage; diff --git a/ios/chrome/browser/flags/about_flags.mm b/ios/chrome/browser/flags/about_flags.mm index 98827675f05f9e..6a468aa0a3bb53 100644 --- a/ios/chrome/browser/flags/about_flags.mm +++ b/ios/chrome/browser/flags/about_flags.mm @@ -778,6 +778,11 @@ flag_descriptions::kMuteCompromisedPasswordsName, flag_descriptions::kMuteCompromisedPasswordsDescription, flags_ui::kOsIos, FEATURE_VALUE_TYPE(password_manager::features::kMuteCompromisedPasswords)}, + {"enable-favicon-passwords", + flag_descriptions::kEnableFaviconForPasswordsName, + flag_descriptions::kEnableFaviconForPasswordsDescription, flags_ui::kOsIos, + FEATURE_VALUE_TYPE( + password_manager::features::kEnableFaviconForPasswords)}, {"autofill-enable-sending-bcn-in-get-upload-details", flag_descriptions::kAutofillEnableSendingBcnInGetUploadDetailsName, flag_descriptions::kAutofillEnableSendingBcnInGetUploadDetailsDescription, diff --git a/ios/chrome/browser/flags/ios_chrome_flag_descriptions.cc b/ios/chrome/browser/flags/ios_chrome_flag_descriptions.cc index 049028dfd86e4e..5faf7fe36e2dcd 100644 --- a/ios/chrome/browser/flags/ios_chrome_flag_descriptions.cc +++ b/ios/chrome/browser/flags/ios_chrome_flag_descriptions.cc @@ -186,6 +186,12 @@ const char kEnableFREDefaultBrowserScreenTestingDescription[] = "This test display the FRE default browser screen and other default " "browser promo depending on experiment."; +const char kEnableFaviconForPasswordsName[] = + "Enable favicons in the Password Manager"; +const char kEnableFaviconForPasswordsDescription[] = + "Show favicons in the Password Manager settings for the Saved Passwords " + "and Never Saved sections."; + const char kEnableFREUIModuleIOSName[] = "Enable FRE UI module with options"; const char kEnableFREUIModuleIOSDescription[] = "Use the new FRE UI module for first run. There are 4 UI options: Identity " diff --git a/ios/chrome/browser/flags/ios_chrome_flag_descriptions.h b/ios/chrome/browser/flags/ios_chrome_flag_descriptions.h index 733f97e330b348..0a3a449962dffe 100644 --- a/ios/chrome/browser/flags/ios_chrome_flag_descriptions.h +++ b/ios/chrome/browser/flags/ios_chrome_flag_descriptions.h @@ -168,6 +168,11 @@ extern const char kEnableDiscoverFeedShorterCacheDescription[]; extern const char kEnableDiscoverFeedStaticResourceServingName[]; extern const char kEnableDiscoverFeedStaticResourceServingDescription[]; +// Title and description for the flag to enable favicon for the Password +// Manager. +extern const char kEnableFaviconForPasswordsName[]; +extern const char kEnableFaviconForPasswordsDescription[]; + // Title and description for the flag to test the FRE default browser promo // experiment. extern const char kEnableFREDefaultBrowserScreenTestingName[]; diff --git a/ios/chrome/browser/ui/bookmarks/BUILD.gn b/ios/chrome/browser/ui/bookmarks/BUILD.gn index 94a2fa33612fc0..873db424ed207f 100644 --- a/ios/chrome/browser/ui/bookmarks/BUILD.gn +++ b/ios/chrome/browser/ui/bookmarks/BUILD.gn @@ -71,6 +71,7 @@ source_set("bookmarks") { "//ios/chrome/browser/ui/commands", "//ios/chrome/browser/ui/default_promo:utils", "//ios/chrome/browser/ui/elements", + "//ios/chrome/browser/ui/favicon:constants", "//ios/chrome/browser/ui/incognito_reauth:incognito_reauth_scene_agent", "//ios/chrome/browser/ui/keyboard", "//ios/chrome/browser/ui/list_model", diff --git a/ios/chrome/browser/ui/bookmarks/bookmark_home_shared_state.mm b/ios/chrome/browser/ui/bookmarks/bookmark_home_shared_state.mm index b2e4c7a3bd701b..9c45a783349ad2 100644 --- a/ios/chrome/browser/ui/bookmarks/bookmark_home_shared_state.mm +++ b/ios/chrome/browser/ui/bookmarks/bookmark_home_shared_state.mm @@ -6,6 +6,7 @@ #include "base/check.h" #import "ios/chrome/browser/ui/bookmarks/cells/bookmark_table_cell_title_editing.h" +#import "ios/chrome/browser/ui/favicon/favicon_constants.h" #import "ios/chrome/browser/ui/table_view/table_view_model.h" #if !defined(__has_feature) || !__has_feature(objc_arc) @@ -13,12 +14,6 @@ #endif namespace { -// Minimal acceptable favicon size, in points. -const CGFloat kMinFaviconSizePt = 16.0; - -// Desired favicon size, in points. -const CGFloat kDesiredFaviconSizePt = 32.0; - // Minimium spacing between keyboard and the titleText when creating new folder, // in points. const CGFloat kKeyboardSpacingPt = 16.0; @@ -80,7 +75,7 @@ + (CGFloat)minFaviconSizePt { } + (CGFloat)desiredFaviconSizePt { - return kDesiredFaviconSizePt; + return kDesiredMediumFaviconSizePt; } + (CGFloat)keyboardSpacingPt { diff --git a/ios/chrome/browser/ui/context_menu/BUILD.gn b/ios/chrome/browser/ui/context_menu/BUILD.gn index 0a6036b28bc880..4155b741dab7ac 100644 --- a/ios/chrome/browser/ui/context_menu/BUILD.gn +++ b/ios/chrome/browser/ui/context_menu/BUILD.gn @@ -22,6 +22,7 @@ source_set("context_menu") { "//ios/chrome/browser/ui/alert_coordinator", "//ios/chrome/browser/ui/commands", "//ios/chrome/browser/ui/context_menu/link_preview", + "//ios/chrome/browser/ui/favicon:constants", "//ios/chrome/browser/ui/image_util:web", "//ios/chrome/browser/ui/incognito_reauth:incognito_reauth_commands", "//ios/chrome/browser/ui/incognito_reauth:incognito_reauth_scene_agent", diff --git a/ios/chrome/browser/ui/context_menu/context_menu_configuration_provider.mm b/ios/chrome/browser/ui/context_menu/context_menu_configuration_provider.mm index 2bd1443ab4c9c4..c72438bb4e233e 100644 --- a/ios/chrome/browser/ui/context_menu/context_menu_configuration_provider.mm +++ b/ios/chrome/browser/ui/context_menu/context_menu_configuration_provider.mm @@ -26,6 +26,7 @@ #import "ios/chrome/browser/ui/context_menu/context_menu_utils.h" #import "ios/chrome/browser/ui/context_menu/image_preview_view_controller.h" #import "ios/chrome/browser/ui/context_menu/link_no_preview_view_controller.h" +#import "ios/chrome/browser/ui/favicon/favicon_constants.h" #import "ios/chrome/browser/ui/image_util/image_copier.h" #import "ios/chrome/browser/ui/image_util/image_saver.h" #import "ios/chrome/browser/ui/incognito_reauth/incognito_reauth_commands.h" @@ -62,9 +63,6 @@ // Character to append to context menut titles that are truncated. NSString* const kContextMenuEllipsis = @"…"; -// Desired width and height of favicon. -const CGFloat kFaviconWidthHeight = 24; - } // namespace @interface ContextMenuConfigurationProvider () @@ -330,7 +328,7 @@ - (instancetype)initWithBrowser:(Browser*)browser IOSChromeFaviconLoaderFactory::GetForBrowserState( self.browser->GetBrowserState()); faviconLoader->FaviconForPageUrl( - linkURL, kFaviconWidthHeight, kFaviconWidthHeight, + linkURL, kDesiredSmallFaviconSizePt, kDesiredSmallFaviconSizePt, /*fallback_to_google_server=*/false, ^(FaviconAttributes* attributes) { [weakPreview configureFaviconWithAttributes:attributes]; diff --git a/ios/chrome/browser/ui/favicon/BUILD.gn b/ios/chrome/browser/ui/favicon/BUILD.gn index adb21cff03c2e2..32e85471cf299b 100644 --- a/ios/chrome/browser/ui/favicon/BUILD.gn +++ b/ios/chrome/browser/ui/favicon/BUILD.gn @@ -24,3 +24,11 @@ source_set("favicon") { ] configs += [ "//build/config/compiler:enable_arc" ] } + +source_set("constants") { + sources = [ + "favicon_constants.h", + "favicon_constants.mm", + ] + configs += [ "//build/config/compiler:enable_arc" ] +} diff --git a/ios/chrome/browser/ui/favicon/favicon_constants.h b/ios/chrome/browser/ui/favicon/favicon_constants.h new file mode 100644 index 00000000000000..085b53a39b55bd --- /dev/null +++ b/ios/chrome/browser/ui/favicon/favicon_constants.h @@ -0,0 +1,18 @@ +// Copyright 2022 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef IOS_CHROME_BROWSER_UI_FAVICON_FAVICON_CONSTANTS_H_ +#define IOS_CHROME_BROWSER_UI_FAVICON_FAVICON_CONSTANTS_H_ + +#import +#import + +// Minimal acceptable favicon size, in points. +extern CGFloat const kMinFaviconSizePt; +// Desired small favicon size, in points. +extern CGFloat const kDesiredSmallFaviconSizePt; +// Desired medium favicon size, in points. +extern CGFloat const kDesiredMediumFaviconSizePt; + +#endif // IOS_CHROME_BROWSER_UI_FAVICON_FAVICON_CONSTANTS_H_ diff --git a/ios/chrome/browser/ui/favicon/favicon_constants.mm b/ios/chrome/browser/ui/favicon/favicon_constants.mm new file mode 100644 index 00000000000000..c26917cbd8408f --- /dev/null +++ b/ios/chrome/browser/ui/favicon/favicon_constants.mm @@ -0,0 +1,13 @@ +// Copyright 2022 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#import "ios/chrome/browser/ui/favicon/favicon_constants.h" + +#if !defined(__has_feature) || !__has_feature(objc_arc) +#error "This file requires ARC support." +#endif + +CGFloat const kMinFaviconSizePt = 16.0; +CGFloat const kDesiredSmallFaviconSizePt = 24.0; +CGFloat const kDesiredMediumFaviconSizePt = 32.0; diff --git a/ios/chrome/browser/ui/history/BUILD.gn b/ios/chrome/browser/ui/history/BUILD.gn index cad8cdb7769455..7121f3b0246935 100644 --- a/ios/chrome/browser/ui/history/BUILD.gn +++ b/ios/chrome/browser/ui/history/BUILD.gn @@ -28,6 +28,7 @@ source_set("history") { "//ios/chrome/browser/ui/activity_services", "//ios/chrome/browser/ui/alert_coordinator", "//ios/chrome/browser/ui/coordinators:chrome_coordinators", + "//ios/chrome/browser/ui/favicon:constants", "//ios/chrome/browser/ui/history/public", "//ios/chrome/browser/ui/menu", "//ios/chrome/browser/ui/sharing", diff --git a/ios/chrome/browser/ui/history/history_mediator.mm b/ios/chrome/browser/ui/history/history_mediator.mm index 70b0613ded8cf0..dcc278bbe3365f 100644 --- a/ios/chrome/browser/ui/history/history_mediator.mm +++ b/ios/chrome/browser/ui/history/history_mediator.mm @@ -7,18 +7,12 @@ #import "ios/chrome/browser/favicon/favicon_loader.h" #include "ios/chrome/browser/favicon/ios_chrome_favicon_loader_factory.h" #import "ios/chrome/browser/net/crurl.h" +#import "ios/chrome/browser/ui/favicon/favicon_constants.h" #if !defined(__has_feature) || !__has_feature(objc_arc) #error "This file requires ARC support." #endif -namespace { -// Desired width and height of favicon. -const CGFloat kFaviconWidthHeight = 24; -// Minimum favicon size to retrieve. -const CGFloat kFaviconMinWidthHeight = 16; -} // namespace - @interface HistoryMediator () // FaviconLoader is a keyed service that uses LargeIconService to retrieve // favicon images. @@ -44,7 +38,7 @@ - (instancetype)initWithBrowserState:(ChromeBrowserState*)browserState { - (void)faviconForURL:(CrURL*)URL completion:(void (^)(FaviconAttributes*))completion { self.faviconLoader->FaviconForPageUrl( - URL.gurl, kFaviconWidthHeight, kFaviconMinWidthHeight, + URL.gurl, kDesiredSmallFaviconSizePt, kMinFaviconSizePt, /*fallback_to_google_server=*/false, ^(FaviconAttributes* attributes) { completion(attributes); }); diff --git a/ios/chrome/browser/ui/omnibox/BUILD.gn b/ios/chrome/browser/ui/omnibox/BUILD.gn index 97976dda09cea0..55865c0d29b365 100644 --- a/ios/chrome/browser/ui/omnibox/BUILD.gn +++ b/ios/chrome/browser/ui/omnibox/BUILD.gn @@ -152,6 +152,7 @@ source_set("omnibox_internal") { "//ios/chrome/browser/ui/coordinators:chrome_coordinators", "//ios/chrome/browser/ui/default_promo", "//ios/chrome/browser/ui/default_promo:utils", + "//ios/chrome/browser/ui/favicon:constants", "//ios/chrome/browser/ui/fullscreen", "//ios/chrome/browser/ui/gestures", "//ios/chrome/browser/ui/location_bar:constants", diff --git a/ios/chrome/browser/ui/omnibox/omnibox_mediator.mm b/ios/chrome/browser/ui/omnibox/omnibox_mediator.mm index 3ba087f9af6087..84e63fcc721caa 100644 --- a/ios/chrome/browser/ui/omnibox/omnibox_mediator.mm +++ b/ios/chrome/browser/ui/omnibox/omnibox_mediator.mm @@ -9,6 +9,7 @@ #import "ios/chrome/browser/favicon/favicon_loader.h" #import "ios/chrome/browser/search_engines/search_engine_observer_bridge.h" #import "ios/chrome/browser/search_engines/search_engines_util.h" +#import "ios/chrome/browser/ui/favicon/favicon_constants.h" #import "ios/chrome/browser/ui/omnibox/omnibox_consumer.h" #import "ios/chrome/browser/ui/omnibox/omnibox_util.h" #include "ios/chrome/browser/ui/ui_feature_flags.h" @@ -20,10 +21,6 @@ #error "This file requires ARC support." #endif -namespace { -const CGFloat kOmniboxIconSize = 16; -} // namespace - @interface OmniboxMediator () // Whether the current default search engine supports search-by-image. @@ -156,7 +153,7 @@ - (void)loadFaviconByPageURL:(GURL)pageURL // Download the favicon. // The code below mimics that in OmniboxPopupMediator. self.faviconLoader->FaviconForPageUrl( - pageURL, kOmniboxIconSize, kOmniboxIconSize, + pageURL, kMinFaviconSizePt, kMinFaviconSizePt, /*fallback_to_google_server=*/false, handleFaviconResult); } @@ -206,7 +203,7 @@ - (void)loadDefaultSearchEngineFaviconWithCompletion: __weak __typeof(self) weakSelf = self; self.latestDefaultSearchEngine = defaultProvider; auto handleFaviconResult = ^void(FaviconAttributes* faviconCacheResult) { - DCHECK_LE(faviconCacheResult.faviconImage.size.width, kOmniboxIconSize); + DCHECK_LE(faviconCacheResult.faviconImage.size.width, kMinFaviconSizePt); if (weakSelf.latestDefaultSearchEngine != defaultProvider || !faviconCacheResult.faviconImage || faviconCacheResult.usesDefaultImage) { @@ -229,13 +226,13 @@ - (void)loadDefaultSearchEngineFaviconWithCompletion: TemplateURLRef::SearchTermsArgs(std::u16string()), _templateURLService->search_terms_data()); self.faviconLoader->FaviconForPageUrl( - GURL(emptyPageUrl), kOmniboxIconSize, kOmniboxIconSize, + GURL(emptyPageUrl), kMinFaviconSizePt, kMinFaviconSizePt, /*fallback_to_google_server=*/YES, handleFaviconResult); } else { // Download the favicon. // The code below mimics that in OmniboxPopupMediator. self.faviconLoader->FaviconForIconUrl(defaultProvider->favicon_url(), - kOmniboxIconSize, kOmniboxIconSize, + kMinFaviconSizePt, kMinFaviconSizePt, handleFaviconResult); } } diff --git a/ios/chrome/browser/ui/reading_list/BUILD.gn b/ios/chrome/browser/ui/reading_list/BUILD.gn index b7f968e63a2ad6..29bbb49fe8ae6e 100644 --- a/ios/chrome/browser/ui/reading_list/BUILD.gn +++ b/ios/chrome/browser/ui/reading_list/BUILD.gn @@ -50,6 +50,7 @@ source_set("reading_list") { "//ios/chrome/browser/ui/commands", "//ios/chrome/browser/ui/coordinators:chrome_coordinators", "//ios/chrome/browser/ui/favicon", + "//ios/chrome/browser/ui/favicon:constants", "//ios/chrome/browser/ui/incognito_reauth:incognito_reauth_scene_agent", "//ios/chrome/browser/ui/main:scene_state_header", "//ios/chrome/browser/ui/menu", diff --git a/ios/chrome/browser/ui/reading_list/reading_list_mediator.mm b/ios/chrome/browser/ui/reading_list/reading_list_mediator.mm index ac6dfbea188beb..2d1884701737eb 100644 --- a/ios/chrome/browser/ui/reading_list/reading_list_mediator.mm +++ b/ios/chrome/browser/ui/reading_list/reading_list_mediator.mm @@ -14,6 +14,7 @@ #include "components/url_formatter/url_formatter.h" #import "ios/chrome/browser/favicon/favicon_loader.h" #include "ios/chrome/browser/favicon/ios_chrome_favicon_loader_factory.h" +#import "ios/chrome/browser/ui/favicon/favicon_constants.h" #import "ios/chrome/browser/ui/reading_list/reading_list_data_sink.h" #import "ios/chrome/browser/ui/reading_list/reading_list_list_item.h" #import "ios/chrome/browser/ui/reading_list/reading_list_list_item_factory.h" @@ -32,11 +33,6 @@ bool EntrySorter(const ReadingListEntry* rhs, const ReadingListEntry* lhs) { return rhs->UpdateTime() > lhs->UpdateTime(); } -// Desired width and height of favicon. -const CGFloat kFaviconWidthHeight = 24; -// Minimum favicon size to retrieve. -const CGFloat kFaviconMinWidthHeight = 16; - } // namespace @interface ReadingListMediator () { @@ -169,7 +165,7 @@ - (void)fetchFaviconForItem:(id)item { [strongSelf.dataSink itemHasChangedAfterDelay:strongItem]; }; self.faviconLoader->FaviconForPageUrl( - item.faviconPageURL, kFaviconWidthHeight, kFaviconMinWidthHeight, + item.faviconPageURL, kDesiredSmallFaviconSizePt, kMinFaviconSizePt, /*fallback_to_google_server=*/false, completionBlock); } diff --git a/ios/chrome/browser/ui/recent_tabs/BUILD.gn b/ios/chrome/browser/ui/recent_tabs/BUILD.gn index c8fe6ecb1f4f35..0e91a0b8c1be59 100644 --- a/ios/chrome/browser/ui/recent_tabs/BUILD.gn +++ b/ios/chrome/browser/ui/recent_tabs/BUILD.gn @@ -35,6 +35,7 @@ source_set("recent_tabs") { "//ios/chrome/browser/ui/activity_services", "//ios/chrome/browser/ui/commands", "//ios/chrome/browser/ui/coordinators:chrome_coordinators", + "//ios/chrome/browser/ui/favicon:constants", "//ios/chrome/browser/ui/menu", "//ios/chrome/browser/ui/menu:context_menu_delegate", "//ios/chrome/browser/ui/ntp", diff --git a/ios/chrome/browser/ui/recent_tabs/recent_tabs_mediator.mm b/ios/chrome/browser/ui/recent_tabs/recent_tabs_mediator.mm index 2574fdd42ac16e..157c48f61e558f 100644 --- a/ios/chrome/browser/ui/recent_tabs/recent_tabs_mediator.mm +++ b/ios/chrome/browser/ui/recent_tabs/recent_tabs_mediator.mm @@ -16,6 +16,7 @@ #include "ios/chrome/browser/sync/session_sync_service_factory.h" #include "ios/chrome/browser/sync/sync_setup_service.h" #include "ios/chrome/browser/sync/sync_setup_service_factory.h" +#import "ios/chrome/browser/ui/favicon/favicon_constants.h" #import "ios/chrome/browser/ui/recent_tabs/recent_tabs_consumer.h" #import "ios/chrome/browser/ui/recent_tabs/sessions_sync_user_state.h" #import "ios/chrome/browser/web_state_list/web_state_list.h" @@ -26,13 +27,6 @@ #error "This file requires ARC support." #endif -namespace { -// Desired width and height of favicon. -const CGFloat kFaviconWidthHeight = 24; -// Minimum favicon size to retrieve. -const CGFloat kFaviconMinWidthHeight = 16; -} // namespace - @interface RecentTabsMediator () { std::unique_ptr @@ -168,7 +162,7 @@ - (void)faviconForURL:(CrURL*)URL FaviconLoader* faviconLoader = IOSChromeFaviconLoaderFactory::GetForBrowserState(self.browserState); faviconLoader->FaviconForPageUrl( - URL.gurl, kFaviconWidthHeight, kFaviconMinWidthHeight, + URL.gurl, kDesiredSmallFaviconSizePt, kMinFaviconSizePt, /*fallback_to_google_server=*/false, ^(FaviconAttributes* attributes) { completion(attributes); }); diff --git a/ios/chrome/browser/ui/settings/BUILD.gn b/ios/chrome/browser/ui/settings/BUILD.gn index 8f7427d01c4c5e..b6b71fc5eb6b71 100644 --- a/ios/chrome/browser/ui/settings/BUILD.gn +++ b/ios/chrome/browser/ui/settings/BUILD.gn @@ -153,6 +153,7 @@ source_set("settings") { "//ios/chrome/browser/ui/content_suggestions/cells", "//ios/chrome/browser/ui/coordinators:chrome_coordinators", "//ios/chrome/browser/ui/elements:elements_internal", + "//ios/chrome/browser/ui/favicon:constants", "//ios/chrome/browser/ui/icons", "//ios/chrome/browser/ui/keyboard", "//ios/chrome/browser/ui/list_model", diff --git a/ios/chrome/browser/ui/settings/password/BUILD.gn b/ios/chrome/browser/ui/settings/password/BUILD.gn index 30b7fad4c9210f..b009e9b9ba3c34 100644 --- a/ios/chrome/browser/ui/settings/password/BUILD.gn +++ b/ios/chrome/browser/ui/settings/password/BUILD.gn @@ -26,7 +26,9 @@ source_set("password") { "//ios/chrome/app/strings", "//ios/chrome/browser", "//ios/chrome/browser/browser_state", + "//ios/chrome/browser/favicon", "//ios/chrome/browser/main:public", + "//ios/chrome/browser/net:crurl", "//ios/chrome/browser/passwords", "//ios/chrome/browser/passwords:save_passwords_consumer", "//ios/chrome/browser/signin", @@ -34,9 +36,11 @@ source_set("password") { "//ios/chrome/browser/ui:feature_flags", "//ios/chrome/browser/ui/commands", "//ios/chrome/browser/ui/coordinators:chrome_coordinators", + "//ios/chrome/browser/ui/favicon:constants", "//ios/chrome/browser/ui/settings/password/password_details", "//ios/chrome/browser/ui/settings/password/passwords_in_other_apps", "//ios/chrome/browser/ui/settings/utils", + "//ios/chrome/browser/ui/table_view", "//ios/chrome/browser/ui/table_view:utils", "//ios/chrome/common", "//ios/chrome/common/ui/colors", @@ -103,6 +107,7 @@ source_set("password_ui") { "//ios/chrome/common:constants", "//ios/chrome/common/ui/colors", "//ios/chrome/common/ui/elements:popover_label_view_controller", + "//ios/chrome/common/ui/favicon", "//ios/chrome/common/ui/reauthentication", "//ios/chrome/common/ui/util", "//ios/third_party/material_components_ios", @@ -153,6 +158,7 @@ source_set("unit_tests") { "//ios/chrome/app/strings", "//ios/chrome/app/strings", "//ios/chrome/browser/browser_state:test_support", + "//ios/chrome/browser/favicon", "//ios/chrome/browser/main:test_support", "//ios/chrome/browser/passwords", "//ios/chrome/browser/passwords:save_passwords_consumer", diff --git a/ios/chrome/browser/ui/settings/password/passwords_coordinator.mm b/ios/chrome/browser/ui/settings/password/passwords_coordinator.mm index 197f00bffd27d4..762c3a5fd01209 100644 --- a/ios/chrome/browser/ui/settings/password/passwords_coordinator.mm +++ b/ios/chrome/browser/ui/settings/password/passwords_coordinator.mm @@ -8,6 +8,8 @@ #include "components/keyed_service/core/service_access_type.h" #include "components/password_manager/core/browser/password_manager_metrics_util.h" #include "components/password_manager/core/common/password_manager_features.h" +#import "ios/chrome/browser/favicon/favicon_loader.h" +#include "ios/chrome/browser/favicon/ios_chrome_favicon_loader_factory.h" #import "ios/chrome/browser/main/browser.h" #include "ios/chrome/browser/passwords/ios_chrome_password_check_manager.h" #include "ios/chrome/browser/passwords/ios_chrome_password_check_manager_factory.h" @@ -105,10 +107,14 @@ - (UIViewController*)viewController { #pragma mark - ChromeCoordinator - (void)start { + ChromeBrowserState* browserState = self.browser->GetBrowserState(); + FaviconLoader* faviconLoader = + IOSChromeFaviconLoaderFactory::GetForBrowserState(browserState); self.mediator = [[PasswordsMediator alloc] initWithPasswordCheckManager:[self passwordCheckManager] syncService:SyncSetupServiceFactory::GetForBrowserState( - self.browser->GetBrowserState())]; + browserState) + faviconLoader:faviconLoader]; self.reauthModule = [[ReauthenticationModule alloc] initWithSuccessfulReauthTimeAccessor:self.mediator]; @@ -120,6 +126,7 @@ - (void)start { self.passwordsViewController.dispatcher = self.dispatcher; self.passwordsViewController.presentationDelegate = self; self.passwordsViewController.reauthenticationModule = self.reauthModule; + self.passwordsViewController.imageDataSource = self.mediator; self.mediator.consumer = self.passwordsViewController; diff --git a/ios/chrome/browser/ui/settings/password/passwords_mediator.h b/ios/chrome/browser/ui/settings/password/passwords_mediator.h index 5f3912e59e69c7..e1fa88671374da 100644 --- a/ios/chrome/browser/ui/settings/password/passwords_mediator.h +++ b/ios/chrome/browser/ui/settings/password/passwords_mediator.h @@ -10,8 +10,10 @@ #include "base/memory/scoped_refptr.h" #import "ios/chrome/browser/ui/settings/password/passwords_table_view_controller_delegate.h" #import "ios/chrome/browser/ui/settings/utils/password_auto_fill_status_observer.h" +#import "ios/chrome/browser/ui/table_view/table_view_favicon_data_source.h" #import "ios/chrome/common/ui/reauthentication/reauthentication_module.h" +class FaviconLoader; class IOSChromePasswordCheckManager; @protocol PasswordsConsumer; class SyncSetupService; @@ -19,12 +21,14 @@ class SyncSetupService; // This mediator fetches and organises the passwords for its consumer. @interface PasswordsMediator : NSObject + SuccessfulReauthTimeAccessor, + TableViewFaviconDataSource> - (instancetype)initWithPasswordCheckManager: (scoped_refptr) passwordCheckManager syncService:(SyncSetupService*)syncService + faviconLoader:(FaviconLoader*)faviconLoader NS_DESIGNATED_INITIALIZER; - (instancetype)init NS_UNAVAILABLE; diff --git a/ios/chrome/browser/ui/settings/password/passwords_mediator.mm b/ios/chrome/browser/ui/settings/password/passwords_mediator.mm index debcc23e6a5a1a..b6155f173f21f8 100644 --- a/ios/chrome/browser/ui/settings/password/passwords_mediator.mm +++ b/ios/chrome/browser/ui/settings/password/passwords_mediator.mm @@ -9,10 +9,13 @@ #include "base/time/time.h" #include "components/password_manager/core/browser/leak_detection_dialog_utils.h" #include "components/password_manager/core/common/password_manager_features.h" +#import "ios/chrome/browser/favicon/favicon_loader.h" +#import "ios/chrome/browser/net/crurl.h" #include "ios/chrome/browser/passwords/password_check_observer_bridge.h" #import "ios/chrome/browser/passwords/save_passwords_consumer.h" #import "ios/chrome/browser/signin/authentication_service.h" #include "ios/chrome/browser/sync/sync_setup_service.h" +#import "ios/chrome/browser/ui/favicon/favicon_constants.h" #import "ios/chrome/browser/ui/settings/password/passwords_consumer.h" #import "ios/chrome/browser/ui/settings/password/saved_passwords_presenter_observer.h" #import "ios/chrome/browser/ui/settings/utils/password_auto_fill_status_manager.h" @@ -64,6 +67,13 @@ @interface PasswordsMediator () ) passwordCheckManager - syncService:(SyncSetupService*)syncService { + syncService:(SyncSetupService*)syncService + faviconLoader:(FaviconLoader*)faviconLoader { self = [super init]; if (self) { + _faviconLoader = faviconLoader; + _syncService = syncService; _passwordCheckManager = passwordCheckManager; @@ -324,4 +337,15 @@ - (NSDate*)lastSuccessfulReauthTime { return [self successfulReauthTime]; } +#pragma mark - TableViewFaviconDataSource + +- (void)faviconForURL:(CrURL*)URL + completion:(void (^)(FaviconAttributes*))completion { + self.faviconLoader->FaviconForPageUrl( + URL.gurl, kDesiredMediumFaviconSizePt, kMinFaviconSizePt, + /*fallback_to_google_server=*/false, ^(FaviconAttributes* attributes) { + completion(attributes); + }); +} + @end diff --git a/ios/chrome/browser/ui/settings/password/passwords_mediator_unittest.mm b/ios/chrome/browser/ui/settings/password/passwords_mediator_unittest.mm index dbc4b1c856e878..479bdd8b1dc33c 100644 --- a/ios/chrome/browser/ui/settings/password/passwords_mediator_unittest.mm +++ b/ios/chrome/browser/ui/settings/password/passwords_mediator_unittest.mm @@ -15,6 +15,8 @@ #include "components/prefs/testing_pref_service.h" #include "ios/chrome/browser/browser_state/chrome_browser_state.h" #include "ios/chrome/browser/browser_state/test_chrome_browser_state.h" +#import "ios/chrome/browser/favicon/favicon_loader.h" +#include "ios/chrome/browser/favicon/ios_chrome_favicon_loader_factory.h" #import "ios/chrome/browser/main/test_browser.h" #include "ios/chrome/browser/passwords/ios_chrome_password_check_manager.h" #include "ios/chrome/browser/passwords/ios_chrome_password_check_manager_factory.h" @@ -116,9 +118,12 @@ void SetUp() override { consumer_ = [[FakePasswordsConsumer alloc] init]; - mediator_ = - [[PasswordsMediator alloc] initWithPasswordCheckManager:password_check_ - syncService:syncService()]; + mediator_ = [[PasswordsMediator alloc] + initWithPasswordCheckManager:password_check_ + syncService:syncService() + faviconLoader:IOSChromeFaviconLoaderFactory:: + GetForBrowserState( + browser_state_.get())]; mediator_.consumer = consumer_; } diff --git a/ios/chrome/browser/ui/settings/password/passwords_table_view_controller.h b/ios/chrome/browser/ui/settings/password/passwords_table_view_controller.h index 272e9d730efc23..273097206dc3e7 100644 --- a/ios/chrome/browser/ui/settings/password/passwords_table_view_controller.h +++ b/ios/chrome/browser/ui/settings/password/passwords_table_view_controller.h @@ -9,6 +9,7 @@ #import "ios/chrome/browser/ui/settings/settings_controller_protocol.h" #import "ios/chrome/browser/ui/settings/settings_navigation_controller.h" #import "ios/chrome/browser/ui/settings/settings_root_table_view_controller.h" +#import "ios/chrome/browser/ui/table_view/table_view_favicon_data_source.h" #import "ios/chrome/common/ui/reauthentication/reauthentication_module.h" class Browser; @@ -42,6 +43,9 @@ class Browser; @property(nonatomic, strong) id reauthenticationModule; +// Data source for favicon images. +@property(nonatomic, weak) id imageDataSource; + @end #endif // IOS_CHROME_BROWSER_UI_SETTINGS_PASSWORD_PASSWORDS_TABLE_VIEW_CONTROLLER_H_ diff --git a/ios/chrome/browser/ui/settings/password/passwords_table_view_controller.mm b/ios/chrome/browser/ui/settings/password/passwords_table_view_controller.mm index c4c2eea4367bdd..3f15c509ddc98c 100644 --- a/ios/chrome/browser/ui/settings/password/passwords_table_view_controller.mm +++ b/ios/chrome/browser/ui/settings/password/passwords_table_view_controller.mm @@ -58,12 +58,15 @@ #import "ios/chrome/browser/ui/table_view/cells/table_view_switch_item.h" #import "ios/chrome/browser/ui/table_view/cells/table_view_text_header_footer_item.h" #import "ios/chrome/browser/ui/table_view/cells/table_view_text_item.h" +#import "ios/chrome/browser/ui/table_view/cells/table_view_url_item.h" +#import "ios/chrome/browser/ui/table_view/table_view_favicon_data_source.h" #import "ios/chrome/browser/ui/table_view/table_view_navigation_controller_constants.h" #import "ios/chrome/browser/ui/table_view/table_view_utils.h" #include "ios/chrome/browser/ui/ui_feature_flags.h" #import "ios/chrome/browser/ui/util/uikit_ui_util.h" #import "ios/chrome/common/ui/colors/semantic_color_names.h" #import "ios/chrome/common/ui/elements/popover_label_view_controller.h" +#import "ios/chrome/common/ui/favicon/favicon_view.h" #import "ios/chrome/common/ui/reauthentication/reauthentication_module.h" #import "ios/chrome/common/ui/util/constraints_ui_util.h" #include "ios/chrome/grit/ios_chromium_strings.h" @@ -177,9 +180,25 @@ void RemoveFormsToBeDeleted( }); } +// Return if the feature flag for the favicon is enabled. +// TODO(crbug.com/1300569): Remove this when kEnableFaviconForPasswords flag is +// removed. +bool IsFaviconEnabled() { + return base::FeatureList::IsEnabled( + password_manager::features::kEnableFaviconForPasswords); +} + } // namespace -@interface PasswordFormContentItem : TableViewDetailTextItem +// TODO(crbug.com/1300569): Remove this when kEnableFaviconForPasswords flag is +// removed. +@interface LegacyPasswordFormContentItem : TableViewDetailTextItem +@property(nonatomic) password_manager::PasswordForm form; +@end +@implementation LegacyPasswordFormContentItem +@end + +@interface PasswordFormContentItem : TableViewURLItem @property(nonatomic) password_manager::PasswordForm form; @end @implementation PasswordFormContentItem @@ -316,6 +335,13 @@ @interface PasswordsTableViewController () < // site equivalent to that of |mostRecentlyUpdatedPassword|. @property(nonatomic, weak) PasswordFormContentItem* mostRecentlyUpdatedItem; +// Stores the PasswordFormContentItem which has form attribute's username and +// site equivalent to that of |legacyMostRecentlyUpdatedItem|. +// TODO(crbug.com/1300569): Remove this when kEnableFaviconForPasswords flag is +// removed. +@property(nonatomic, weak) + LegacyPasswordFormContentItem* legacyMostRecentlyUpdatedItem; + // YES, if the user has tapped on the "Check Now" button. @property(nonatomic, assign) BOOL shouldFocusAccessibilityOnPasswordCheckStatus; @@ -895,9 +921,10 @@ - (TableViewTextItem*)exportPasswordsItem { forForm:(const password_manager::PasswordForm&)form { PasswordFormContentItem* passwordItem = [[PasswordFormContentItem alloc] initWithType:ItemTypeSavedPassword]; - passwordItem.text = text; + passwordItem.title = text; passwordItem.form = form; passwordItem.detailText = detailText; + passwordItem.URL = [[CrURL alloc] initWithGURL:GURL(form.url)]; passwordItem.accessibilityTraits |= UIAccessibilityTraitButton; passwordItem.accessoryType = UITableViewCellAccessoryDisclosureIndicator; if (self.mostRecentlyUpdatedPassword) { @@ -916,6 +943,46 @@ - (TableViewTextItem*)exportPasswordsItem { forForm:(const password_manager::PasswordForm&)form { PasswordFormContentItem* passwordItem = [[PasswordFormContentItem alloc] initWithType:ItemTypeBlocked]; + passwordItem.title = text; + passwordItem.form = form; + passwordItem.URL = [[CrURL alloc] initWithGURL:GURL(form.url)]; + passwordItem.accessibilityTraits |= UIAccessibilityTraitButton; + passwordItem.accessoryType = UITableViewCellAccessoryDisclosureIndicator; + return passwordItem; +} + +// TODO(crbug.com/1300569): Remove this when kEnableFaviconForPasswords flag is +// removed. +- (LegacyPasswordFormContentItem*) + legacySavedFormItemWithText:(NSString*)text + andDetailText:(NSString*)detailText + forForm:(const password_manager::PasswordForm&)form { + LegacyPasswordFormContentItem* passwordItem = + [[LegacyPasswordFormContentItem alloc] + initWithType:ItemTypeSavedPassword]; + passwordItem.text = text; + passwordItem.form = form; + passwordItem.detailText = detailText; + passwordItem.accessibilityTraits |= UIAccessibilityTraitButton; + passwordItem.accessoryType = UITableViewCellAccessoryDisclosureIndicator; + if (self.mostRecentlyUpdatedPassword) { + if (self.mostRecentlyUpdatedPassword->username_value == + form.username_value && + self.mostRecentlyUpdatedPassword->signon_realm == form.signon_realm) { + self.legacyMostRecentlyUpdatedItem = passwordItem; + self.mostRecentlyUpdatedPassword = absl::nullopt; + } + } + return passwordItem; +} + +// TODO(crbug.com/1300569): Remove this when kEnableFaviconForPasswords flag is +// removed. +- (LegacyPasswordFormContentItem*) + legacyBlockedFormItemWithText:(NSString*)text + forForm:(const password_manager::PasswordForm&)form { + LegacyPasswordFormContentItem* passwordItem = + [[LegacyPasswordFormContentItem alloc] initWithType:ItemTypeBlocked]; passwordItem.text = text; passwordItem.form = form; passwordItem.accessibilityTraits |= UIAccessibilityTraitButton; @@ -1355,9 +1422,13 @@ - (void)filterItems:(NSString*)searchTerm { ![detailText localizedCaseInsensitiveContainsString:searchTerm]; if (hidden) continue; - [model addItem:[self savedFormItemWithText:text - andDetailText:detailText - forForm:form] + [model addItem:(IsFaviconEnabled() + ? [self savedFormItemWithText:text + andDetailText:detailText + forForm:form] + : [self legacySavedFormItemWithText:text + andDetailText:detailText + forForm:form]) toSectionWithIdentifier:SectionIdentifierSavedPasswords]; } } @@ -1371,7 +1442,10 @@ - (void)filterItems:(NSString*)searchTerm { ![text localizedCaseInsensitiveContainsString:searchTerm]; if (hidden) continue; - [model addItem:[self blockedFormItemWithText:text forForm:form] + [model addItem:(IsFaviconEnabled() + ? [self blockedFormItemWithText:text forForm:form] + : [self legacyBlockedFormItemWithText:text + forForm:form]) toSectionWithIdentifier:SectionIdentifierBlocked]; } } @@ -1633,16 +1707,22 @@ - (void)deleteItemAtIndexPaths:(NSArray*)indexPaths { std::vector blockedToDelete; for (NSIndexPath* indexPath in indexPaths) { + // TODO(crbug.com/1300569): Remove this when kEnableFaviconForPasswords flag + // is removed. + password_manager::PasswordForm form = + IsFaviconEnabled() + ? base::mac::ObjCCastStrict( + [self.tableViewModel itemAtIndexPath:indexPath]) + .form + : base::mac::ObjCCastStrict( + [self.tableViewModel itemAtIndexPath:indexPath]) + .form; // Only form items are editable. - PasswordFormContentItem* item = - base::mac::ObjCCastStrict( - [self.tableViewModel itemAtIndexPath:indexPath]); NSInteger itemType = [self.tableViewModel itemTypeForIndexPath:indexPath]; BOOL blocked = (itemType == ItemTypeBlocked); - blocked ? blockedToDelete.push_back(item.form) - : passwordsToDelete.push_back(item.form); + blocked ? blockedToDelete.push_back(form) + : passwordsToDelete.push_back(form); } - RemoveFormsToBeDeleted(_savedForms, passwordsToDelete); RemoveFormsToBeDeleted(_blockedForms, blockedToDelete); @@ -1710,6 +1790,15 @@ - (void)scrollToLastUpdatedItem { atScrollPosition:UITableViewScrollPositionTop animated:NO]; self.mostRecentlyUpdatedItem = nil; + } else if (self.legacyMostRecentlyUpdatedItem) { + // TODO(crbug.com/1300569): Remove this when kEnableFaviconForPasswords flag + // is removed. + NSIndexPath* indexPath = [self.tableViewModel + indexPathForItem:self.legacyMostRecentlyUpdatedItem]; + [self.tableView scrollToRowAtIndexPath:indexPath + atScrollPosition:UITableViewScrollPositionTop + animated:NO]; + self.legacyMostRecentlyUpdatedItem = nil; } } @@ -1767,19 +1856,29 @@ - (void)tableView:(UITableView*)tableView case ItemTypeSavedPassword: { DCHECK_EQ(SectionIdentifierSavedPasswords, [model sectionIdentifierForSection:indexPath.section]); - PasswordFormContentItem* saveFormItem = - base::mac::ObjCCastStrict( - [model itemAtIndexPath:indexPath]); - [self.handler showDetailedViewForForm:saveFormItem.form]; + password_manager::PasswordForm form = + IsFaviconEnabled() + ? base::mac::ObjCCastStrict( + [model itemAtIndexPath:indexPath]) + .form + : base::mac::ObjCCastStrict( + [model itemAtIndexPath:indexPath]) + .form; + [self.handler showDetailedViewForForm:form]; break; } case ItemTypeBlocked: { DCHECK_EQ(SectionIdentifierBlocked, [model sectionIdentifierForSection:indexPath.section]); - PasswordFormContentItem* blockedItem = - base::mac::ObjCCastStrict( - [model itemAtIndexPath:indexPath]); - [self.handler showDetailedViewForForm:blockedItem.form]; + password_manager::PasswordForm form = + IsFaviconEnabled() + ? base::mac::ObjCCastStrict( + [model itemAtIndexPath:indexPath]) + .form + : base::mac::ObjCCastStrict( + [model itemAtIndexPath:indexPath]) + .form; + [self.handler showDetailedViewForForm:form]; break; } case ItemTypeExportPasswordsButton: @@ -1916,15 +2015,47 @@ - (UITableViewCell*)tableView:(UITableView*)tableView } case ItemTypeSavedPassword: case ItemTypeBlocked: { - TableViewDetailTextCell* textCell = - base::mac::ObjCCastStrict(cell); - textCell.textLabel.lineBreakMode = NSLineBreakByTruncatingHead; + if (IsFaviconEnabled()) { + TableViewURLCell* urlCell = + base::mac::ObjCCastStrict(cell); + urlCell.textLabel.lineBreakMode = NSLineBreakByTruncatingHead; + // Load the favicon from cache. + [self loadFaviconAtIndexPath:indexPath forCell:cell]; + } else { + TableViewDetailTextCell* textCell = + base::mac::ObjCCastStrict(cell); + textCell.textLabel.lineBreakMode = NSLineBreakByTruncatingHead; + } break; } } return cell; } +// Asynchronously loads favicon for given index path that is of type +// `ItemTypeSavedPassword` or `ItemTypeBlocked`. The loads are cancelled upon +// cell reuse automatically. +- (void)loadFaviconAtIndexPath:(NSIndexPath*)indexPath + forCell:(UITableViewCell*)cell { + TableViewItem* item = [self.tableViewModel itemAtIndexPath:indexPath]; + DCHECK(item); + DCHECK(cell); + + TableViewURLItem* URLItem = base::mac::ObjCCastStrict(item); + TableViewURLCell* URLCell = base::mac::ObjCCastStrict(cell); + + NSString* itemIdentifier = URLItem.uniqueIdentifier; + [self.imageDataSource + faviconForURL:URLItem.URL + completion:^(FaviconAttributes* attributes) { + // Only set favicon if the cell hasn't been reused. + if ([URLCell.cellUniqueIdentifier isEqualToString:itemIdentifier]) { + DCHECK(attributes); + [URLCell.faviconView configureWithAttributes:attributes]; + } + }]; +} + #pragma mark PasswordExporterDelegate - (void)showSetPasscodeDialog { diff --git a/ios/chrome/browser/ui/settings/password/passwords_table_view_controller_unittest.mm b/ios/chrome/browser/ui/settings/password/passwords_table_view_controller_unittest.mm index 4958b74b990d76..7fdc5abb6c058a 100644 --- a/ios/chrome/browser/ui/settings/password/passwords_table_view_controller_unittest.mm +++ b/ios/chrome/browser/ui/settings/password/passwords_table_view_controller_unittest.mm @@ -19,6 +19,8 @@ #include "components/password_manager/core/browser/test_password_store.h" #include "components/password_manager/core/common/password_manager_features.h" #include "ios/chrome/browser/browser_state/test_chrome_browser_state.h" +#import "ios/chrome/browser/favicon/favicon_loader.h" +#include "ios/chrome/browser/favicon/ios_chrome_favicon_loader_factory.h" #include "ios/chrome/browser/main/test_browser.h" #include "ios/chrome/browser/passwords/ios_chrome_bulk_leak_check_service_factory.h" #include "ios/chrome/browser/passwords/ios_chrome_password_check_manager.h" @@ -104,7 +106,10 @@ void SetUp() override { initWithPasswordCheckManager:IOSChromePasswordCheckManagerFactory:: GetForBrowserState( browser_->GetBrowserState()) - syncService:nil]; + syncService:nil + faviconLoader:IOSChromeFaviconLoaderFactory:: + GetForBrowserState( + browser_->GetBrowserState())]; // Inject some fake passwords to pass the loading state. PasswordsTableViewController* passwords_controller = @@ -320,6 +325,40 @@ void SetEditing(bool editing) { // Tests the order in which the saved passwords are displayed. TEST_F(PasswordsTableViewControllerTest, TestSavedPasswordsOrder) { + base::test::ScopedFeatureList scoped_feature_list; + scoped_feature_list.InitAndEnableFeature( + password_manager::features::kEnableFaviconForPasswords); + + AddSavedForm2(); + + CheckURLCellTitleAndDetailText(@"example2.com", @"test@egmail.com", + GetSectionIndex(SavedPasswords), 0); + + AddSavedForm1(); + CheckURLCellTitleAndDetailText(@"example.com", @"test@egmail.com", + GetSectionIndex(SavedPasswords), 0); + CheckURLCellTitleAndDetailText(@"example2.com", @"test@egmail.com", + GetSectionIndex(SavedPasswords), 1); +} + +// Tests the order in which the blocked passwords are displayed. +TEST_F(PasswordsTableViewControllerTest, TestBlockedPasswordsOrder) { + base::test::ScopedFeatureList scoped_feature_list; + scoped_feature_list.InitAndEnableFeature( + password_manager::features::kEnableFaviconForPasswords); + + AddBlockedForm2(); + CheckURLCellTitle(@"secret2.com", GetSectionIndex(SavedPasswords), 0); + + AddBlockedForm1(); + CheckURLCellTitle(@"secret.com", GetSectionIndex(SavedPasswords), 0); + CheckURLCellTitle(@"secret2.com", GetSectionIndex(SavedPasswords), 1); +} + +// Tests the order in which the saved passwords are displayed. +// TODO(crbug.com/1300569): Remove this when kEnableFaviconForPasswords flag is +// removed. +TEST_F(PasswordsTableViewControllerTest, TestSavedPasswordsOrderLegacy) { AddSavedForm2(); CheckTextCellTextAndDetailText(@"example2.com", @"test@egmail.com", @@ -333,7 +372,9 @@ void SetEditing(bool editing) { } // Tests the order in which the blocked passwords are displayed. -TEST_F(PasswordsTableViewControllerTest, TestBlockedPasswordsOrder) { +// TODO(crbug.com/1300569): Remove this when kEnableFaviconForPasswords flag is +// removed. +TEST_F(PasswordsTableViewControllerTest, TestBlockedPasswordsOrderLegacy) { AddBlockedForm2(); CheckTextCellText(@"secret2.com", GetSectionIndex(SavedPasswords), 0); diff --git a/ios/chrome/browser/ui/settings/search_engine_table_view_controller.mm b/ios/chrome/browser/ui/settings/search_engine_table_view_controller.mm index e5e6ed1849c69c..24943a27fb583c 100644 --- a/ios/chrome/browser/ui/settings/search_engine_table_view_controller.mm +++ b/ios/chrome/browser/ui/settings/search_engine_table_view_controller.mm @@ -19,6 +19,7 @@ #include "ios/chrome/browser/favicon/ios_chrome_favicon_loader_factory.h" #import "ios/chrome/browser/search_engines/search_engine_observer_bridge.h" #include "ios/chrome/browser/search_engines/template_url_service_factory.h" +#import "ios/chrome/browser/ui/favicon/favicon_constants.h" #import "ios/chrome/browser/ui/settings/cells/search_engine_item.h" #import "ios/chrome/browser/ui/table_view/cells/table_view_text_header_footer_item.h" #import "ios/chrome/browser/ui/table_view/cells/table_view_url_item.h" @@ -45,8 +46,6 @@ typedef NS_ENUM(NSInteger, ItemType) { }; const CGFloat kTableViewSeparatorLeadingInset = 56; -const int kFaviconDesiredSizeInPoint = 32; -const int kFaviconMinSizeInPoint = 16; constexpr base::TimeDelta kMaxVisitAge = base::Days(2); const size_t kMaxcustomSearchEngines = 3; const char kUmaSelectDefaultSearchEngine[] = @@ -397,7 +396,7 @@ - (UITableViewCell*)tableView:(UITableView*)tableView if (item.type == ItemTypePrepopulatedEngine) { _faviconLoader->FaviconForPageUrl( - engineItem.URL, kFaviconDesiredSizeInPoint, kFaviconMinSizeInPoint, + engineItem.URL, kDesiredMediumFaviconSizePt, kMinFaviconSizePt, /*fallback_to_google_server=*/YES, ^(FaviconAttributes* attributes) { // Only set favicon if the cell hasn't been reused. if (urlCell.cellUniqueIdentifier == engineItem.uniqueIdentifier) { @@ -407,7 +406,7 @@ - (UITableViewCell*)tableView:(UITableView*)tableView }); } else { _faviconLoader->FaviconForIconUrl( - engineItem.URL, kFaviconDesiredSizeInPoint, kFaviconMinSizeInPoint, + engineItem.URL, kDesiredMediumFaviconSizePt, kMinFaviconSizePt, ^(FaviconAttributes* attributes) { // Only set favicon if the cell hasn't been reused. if (urlCell.cellUniqueIdentifier == engineItem.uniqueIdentifier) { diff --git a/ios/chrome/browser/ui/table_view/cells/table_view_url_item.h b/ios/chrome/browser/ui/table_view/cells/table_view_url_item.h index 088a7819d8d519..9039fb8c868006 100644 --- a/ios/chrome/browser/ui/table_view/cells/table_view_url_item.h +++ b/ios/chrome/browser/ui/table_view/cells/table_view_url_item.h @@ -25,6 +25,8 @@ @property(nonatomic, readwrite, copy) NSString* supplementalURLText; // Delimiter used to separate the URL hostname and the supplemental text. @property(nonatomic, readwrite, copy) NSString* supplementalURLTextDelimiter; +// Detail text to be displayed instead of the URL. +@property(nonatomic, strong) NSString* detailText; // Metadata text displayed at the trailing edge of the cell. @property(nonatomic, readwrite, copy) NSString* metadata; // The image for the badge view added over the favicon. diff --git a/ios/chrome/browser/ui/table_view/cells/table_view_url_item.mm b/ios/chrome/browser/ui/table_view/cells/table_view_url_item.mm index 9e27e6e83264d4..eb35a03559c977 100644 --- a/ios/chrome/browser/ui/table_view/cells/table_view_url_item.mm +++ b/ios/chrome/browser/ui/table_view/cells/table_view_url_item.mm @@ -94,6 +94,9 @@ - (NSString*)titleLabelText { // Returns the text to use when configuring a TableViewURLCell's URL label. - (NSString*)URLLabelText { + // Use detail text instead of the URL if there is one set. + if (self.detailText) + return self.detailText; // If there's no title text, the URL is used as the cell title. Add the // supplemental text to the URL label below if it exists. if (!self.title.length) diff --git a/ios/chrome/browser/ui/table_view/chrome_table_view_controller_test.h b/ios/chrome/browser/ui/table_view/chrome_table_view_controller_test.h index fad2bb2e64c790..416dfbd22efa12 100644 --- a/ios/chrome/browser/ui/table_view/chrome_table_view_controller_test.h +++ b/ios/chrome/browser/ui/table_view/chrome_table_view_controller_test.h @@ -82,6 +82,18 @@ class ChromeTableViewControllerTest : public BlockCleanupTest { int section, int item); + // Verifies that the URL cell at |item| in |section| has a title property + // which matches |expected_title|. + void CheckURLCellTitle(NSString* expected_title, int section, int item); + + // Verifies that the URL cell at |item| in |section| has a title and + // detailText properties which match strings for |expected_title| and + // |expected_detail_text|, respectively. + void CheckURLCellTitleAndDetailText(NSString* expected_title, + NSString* expected_detail_text, + int section, + int item); + // Verifies that the text cell at |item| in |section| has a text and // detailText properties which match strings for |expected_text| and // |expected_detail_text|, respectively. diff --git a/ios/chrome/browser/ui/table_view/chrome_table_view_controller_test.mm b/ios/chrome/browser/ui/table_view/chrome_table_view_controller_test.mm index 0b6fd30956272f..2c9a4c1918362b 100644 --- a/ios/chrome/browser/ui/table_view/chrome_table_view_controller_test.mm +++ b/ios/chrome/browser/ui/table_view/chrome_table_view_controller_test.mm @@ -123,6 +123,14 @@ - (NSString*)detailText; EXPECT_NSEQ(expected_text, [cell text]); } +void ChromeTableViewControllerTest::CheckURLCellTitle(NSString* expected_title, + int section, + int item) { + id cell = GetTableViewItem(section, item); + ASSERT_TRUE([cell respondsToSelector:@selector(title)]); + EXPECT_NSEQ(expected_title, [cell title]); +} + void ChromeTableViewControllerTest::CheckTextCellTextWithId( int expected_text_id, int section, @@ -142,6 +150,18 @@ - (NSString*)detailText; EXPECT_NSEQ(expected_detail_text, [cell detailText]); } +void ChromeTableViewControllerTest::CheckURLCellTitleAndDetailText( + NSString* expected_title, + NSString* expected_detail_text, + int section, + int item) { + id cell = GetTableViewItem(section, item); + ASSERT_TRUE([cell respondsToSelector:@selector(title)]); + ASSERT_TRUE([cell respondsToSelector:@selector(detailText)]); + EXPECT_NSEQ(expected_title, [cell title]); + EXPECT_NSEQ(expected_detail_text, [cell detailText]); +} + void ChromeTableViewControllerTest::CheckDetailItemTextWithIds( int expected_text_id, int expected_detail_text_id,