Avoid jumping omnibox text in touch layouts caused by varying location bar icon sizes.

In the touch layouts, LOCATION_BAR_ICON_SIZE is 20, but kFaviconSize is 16, leading to different location bar icon image sizes depending on what icon is displayed and causing the omnibox text to jump when the icon size changes. For example, when typing a url, the 20px globe icon is displayed; wheras when typing a search url, the default search provider's favicon is displayed if available, which for Google is the 16px G icon. To avoid this, we set the location bar icon's size as LOCATION_BAR_ICON_SIZE, which is large enough to contain all icons. This is not an issue in other layouts, where both constants are 16.

Bug: 860295
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Ie9bc7a1b0b6b5a1f3dc6f4d2239d8fc2305c5063
Reviewed-on: https://chromium-review.googlesource.com/1135700
Commit-Queue: manuk hovanesian <manukh@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575675}

chrome/browser/ui/omnibox/chrome_omnibox_client.cc

@@ -61,6 +61,7 @@
 #include "net/traffic_annotation/network_traffic_annotation.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/base/window_open_disposition.h"
+#include "ui/gfx/image/canvas_image_source.h"
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/gfx/vector_icon_types.h"
 #include "url/gurl.h"
@@ -233,6 +234,25 @@ gfx::Image ChromeOmniboxClient::GetSizedIcon(
       vector_icon_color));
 }
 
+gfx::Image ChromeOmniboxClient::GetSizedIcon(const gfx::Image& icon) const {
+  if (icon.IsEmpty())
+    return icon;
+
+  const int icon_size = GetLayoutConstant(LOCATION_BAR_ICON_SIZE);
+  // In touch mode, icons are 20x20. FaviconCache and ExtensionIconManager both
+  // guarantee favicons and extension icons will be 16x16, so add extra padding
+  // around them to align them vertically with the other vector icons.
+  DCHECK_GE(icon_size, icon.Height());
+  DCHECK_GE(icon_size, icon.Width());
+  gfx::Insets padding_border((icon_size - icon.Height()) / 2,
+                             (icon_size - icon.Width()) / 2);
+  if (!padding_border.IsEmpty()) {
+    return gfx::Image(gfx::CanvasImageSource::CreatePadded(*icon.ToImageSkia(),
+                                                           padding_border));
+  }
+  return icon;
+}
+
 bool ChromeOmniboxClient::ProcessExtensionKeyword(
     const TemplateURL* template_url,
     const AutocompleteMatch& match,

chrome/browser/ui/omnibox/chrome_omnibox_client.h

@@ -52,6 +52,7 @@ class ChromeOmniboxClient : public OmniboxClient {
       const AutocompleteMatch& match) const override;
   gfx::Image GetSizedIcon(const gfx::VectorIcon& vector_icon_type,
                           SkColor vector_icon_color) const override;
+  gfx::Image GetSizedIcon(const gfx::Image& icon) const override;
   bool ProcessExtensionKeyword(const TemplateURL* template_url,
                                const AutocompleteMatch& match,
                                WindowOpenDisposition disposition,

chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc

@@ -248,23 +248,7 @@ void OmniboxPopupContentsView::OpenMatch(WindowOpenDisposition disposition) {
 gfx::Image OmniboxPopupContentsView::GetMatchIcon(
     const AutocompleteMatch& match,
     SkColor vector_icon_color) const {
-  gfx::Image icon = model_->GetMatchIcon(match, vector_icon_color);
-  if (icon.IsEmpty())
-    return icon;
-
-  const int icon_size = GetLayoutConstant(LOCATION_BAR_ICON_SIZE);
-  // In touch mode, icons are 20x20. FaviconCache and ExtensionIconManager both
-  // guarantee favicons and extension icons will be 16x16, so add extra padding
-  // around them to align them vertically with the other vector icons.
-  DCHECK_GE(icon_size, icon.Height());
-  DCHECK_GE(icon_size, icon.Width());
-  gfx::Insets padding_border((icon_size - icon.Height()) / 2,
-                             (icon_size - icon.Width()) / 2);
-  if (!padding_border.IsEmpty()) {
-    return gfx::Image(gfx::CanvasImageSource::CreatePadded(*icon.ToImageSkia(),
-                                                           padding_border));
-  }
-  return icon;
+  return model_->GetMatchIcon(match, vector_icon_color);
 }
 
 OmniboxTint OmniboxPopupContentsView::GetTint() const {

components/omnibox/browser/omnibox_client.cc

@@ -79,6 +79,10 @@ gfx::Image OmniboxClient::GetSizedIcon(const gfx::VectorIcon& vector_icon_type,
   return gfx::Image();
 }
 
+gfx::Image OmniboxClient::GetSizedIcon(const gfx::Image& icon) const {
+  return gfx::Image();
+}
+
 bool OmniboxClient::ProcessExtensionKeyword(
     const TemplateURL* template_url,
     const AutocompleteMatch& match,

components/omnibox/browser/omnibox_client.h

@@ -100,6 +100,9 @@ class OmniboxClient {
   virtual gfx::Image GetSizedIcon(const gfx::VectorIcon& vector_icon_type,
                                   SkColor vector_icon_color) const;
 
+  // Returns the given |icon| with the correct size.
+  virtual gfx::Image GetSizedIcon(const gfx::Image& icon) const;
+
   // Checks whether |template_url| is an extension keyword; if so, asks the
   // ExtensionOmniboxEventRouter to process |match| for it and returns true.
   // Otherwise returns false. |observer| is the OmniboxNavigationObserver

components/omnibox/browser/omnibox_popup_model.cc

@@ -288,8 +288,10 @@ gfx::Image OmniboxPopupModel::GetMatchIcon(const AutocompleteMatch& match,
                                            SkColor vector_icon_color) {
   gfx::Image extension_icon =
       edit_model_->client()->GetIconIfExtensionMatch(match);
+  // Extension icons are the correct size for non-touch UI but need to be
+  // adjusted to be the correct size for touch mode.
   if (!extension_icon.IsEmpty())
-    return extension_icon;
+    return edit_model_->client()->GetSizedIcon(extension_icon);
 
   if (OmniboxFieldTrial::IsShowSuggestionFaviconsEnabled() &&
       !AutocompleteMatch::IsSearchType(match.type)) {
@@ -303,8 +305,10 @@ gfx::Image OmniboxPopupModel::GetMatchIcon(const AutocompleteMatch& match,
         base::Bind(&OmniboxPopupModel::OnFaviconFetched,
                    weak_factory_.GetWeakPtr(), match.destination_url));
 
+    // Extension icons are the correct size for non-touch UI but need to be
+    // adjusted to be the correct size for touch mode.
     if (!favicon.IsEmpty())
-      return favicon;
+      return edit_model_->client()->GetSizedIcon(favicon);
   }
 
   const auto& vector_icon_type = AutocompleteMatch::TypeToVectorIcon(

components/omnibox/browser/omnibox_view.cc

@@ -102,6 +102,13 @@ bool OmniboxView::IsEditingOrEmpty() const {
       (GetOmniboxTextLength() == 0);
 }
 
+// TODO (manukh) OmniboxView::GetIcon is very similar to
+// OmniboxPopupModel::GetMatchIcon. They contain certain inconsistencies
+// concerning what flags are required to display url favicons and bookmark star
+// icons. OmniboxPopupModel::GetMatchIcon also doesn't display default search
+// provider icons. It's possible they have other inconsistencies as well. In the
+// future, once the Material and Favicon flags are always enabled, we may want
+// to consider reusing the same code for both the popup and omnibox icons.
 gfx::ImageSkia OmniboxView::GetIcon(int dip_size,
                                     SkColor color,
                                     IconFetchedCallback on_icon_fetched) const {
@@ -141,7 +148,7 @@ gfx::ImageSkia OmniboxView::GetIcon(int dip_size,
     }
 
     if (!favicon.IsEmpty())
-      return favicon.AsImageSkia();
+      return model_->client()->GetSizedIcon(favicon).AsImageSkia();
     // If the client returns an empty favicon, fall through to provide the
     // generic vector icon. |on_icon_fetched| may or may not be called later.
     // If it's never called, the vector icon we provide below should remain.