[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 <vasilii@chromium.org> Reviewed-by: Gauthier Ambard <gambard@chromium.org> Commit-Queue: Veronique Nguyen <veronguyen@google.com> Cr-Commit-Position: refs/heads/main@{#975120}
Denger-Network · Feb 25, 2022 · 00ba26e · 00ba26e
1 parent b12f8d8
commit 00ba26e
Show file tree

Hide file tree

Showing 35 changed files with 380 additions and 76 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -6,19 +6,14 @@
 
 #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)
 #error "This file requires ARC support."
 #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
@@ -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
@@ -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
@@ -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
@@ -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 <Foundation/Foundation.h>
+#import <UIKit/UIKit.h>
+
+// 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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 () <SearchEngineObserving>
 
 // 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
@@ -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
@@ -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 ()<ReadingListModelBridgeObserver> {
@@ -169,7 +165,7 @@ - (void)fetchFaviconForItem:(id<ReadingListListItem>)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
@@ -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
@@ -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 () <SyncedSessionsObserver,
                                   WebStateListObserving> {
   std::unique_ptr<synced_sessions::SyncedSessionsObserverBridge>
@@ -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
@@ -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",