Omnibox: Make OmniboxView::GetIcon more robust for steady state elisions

Whenever the user unelides the URL, the omnibox also enters
user_input_in_progress_ mode.

This is was harmless before, but since then we have started displaying
suggestion favicons in the omnibox, the current page's security
indicator is being clobbered by the page's favicon during unelision.

This is not correct, since unelision is supposed to be as unobtrusive
as possible.

This is going to become more noticable now that Ctrl+L triggers
unelision, as well as our future planned work for One-Click-Unelide.

This CL replaces the old logic, and keeps showing the current page's
security indicator until the user actually modifies the user text.

Bug: 874592, 906223, 910145
Change-Id: I8a5619d4408b09d2e9f98fd5ce011ecbd2cab085
Reviewed-on: https://chromium-review.googlesource.com/c/1351564
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: Kevin Bailey <krb@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Reviewed-by: Ben Wells <benwells@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613203}

chrome/browser/ui/views/location_bar/location_bar_view.cc

@@ -1087,6 +1087,9 @@ void LocationBarView::OnChanged() {
 void LocationBarView::OnPopupVisibilityChanged() {
   RefreshBackground();
 
+  // The location icon may change when the popup visibility changes.
+  location_icon_view_->Update(/*suppress_animations=*/false);
+
   // The focus ring may be hidden or shown when the popup visibility changes.
   if (focus_ring_)
     focus_ring_->SchedulePaint();

components/omnibox/browser/location_bar_model.h

@@ -54,8 +54,9 @@ class LocationBarModel {
 
   // Returns the id of the icon to show to the left of the address, based on the
   // current URL.  When search term replacement is active, this returns a search
-  // icon.  This doesn't cover specialized icons while the user is editing; see
-  // OmniboxView::GetIcon().
+  // icon.  This always shows the icon based on the current page's security
+  // state, and doesn't account for user editing, or show any specialized icons
+  // for input in progress. See OmniboxView::GetIcon() for those.
   virtual const gfx::VectorIcon& GetVectorIcon() const = 0;
 
   // Returns text for the omnibox secure verbose chip, displayed next to the

components/omnibox/browser/location_bar_model_impl.cc

@@ -118,7 +118,7 @@ const gfx::VectorIcon& LocationBarModelImpl::GetVectorIcon() const {
   if (IsOfflinePage())
     return omnibox::kOfflinePinIcon;
 
-  switch (GetSecurityLevel(false)) {
+  switch (GetSecurityLevel(true /* ignore_editing */)) {
     case security_state::NONE:
     case security_state::HTTP_SHOW_WARNING:
       return omnibox::kHttpIcon;

components/omnibox/browser/omnibox_edit_model.cc

@@ -426,6 +426,28 @@ void OmniboxEditModel::AdjustTextForCopy(int sel_min,
   }
 }
 
+bool OmniboxEditModel::ShouldShowCurrentPageIcon() const {
+  // If the popup is open, don't show the current page's icon. The caller is
+  // instead expected to show the current match's icon.
+  if (PopupIsOpen())
+    return false;
+
+  // On the New Tab Page, the omnibox textfield is empty. We want to display
+  // the default search provider favicon instead of the NTP security icon.
+  if (view_->GetText().empty())
+    return false;
+
+  // If user input is not in progress, always show the current page's icon.
+  if (!user_input_in_progress())
+    return true;
+
+  // If user input is in progress, keep showing the current page's icon so long
+  // as the text matches the current page's URL, elided or unelided. This logic
+  // also works for Query in Omnibox, since the query is in |display_text_|.
+  return view_->GetText() == display_text_ ||
+         view_->GetText() == url_for_editing_;
+}
+
 void OmniboxEditModel::UpdateInput(bool has_selected_text,
                                    bool prevent_inline_autocomplete) {
   bool changed = SetInputInProgressNoNotify(true);

components/omnibox/browser/omnibox_edit_model.h

@@ -140,6 +140,11 @@ class OmniboxEditModel {
 
   bool user_input_in_progress() const { return user_input_in_progress_; }
 
+  // Encapsulates all the varied conditions for whether to override the
+  // permanent page icon (associated with the currently displayed page),
+  // with a temporary icon (associated with the current match or user text).
+  bool ShouldShowCurrentPageIcon() const;
+
   // Sets the state of user_input_in_progress_, and notifies the observer if
   // that state has changed.
   void SetInputInProgress(bool in_progress);

components/omnibox/browser/omnibox_edit_model_unittest.cc

@@ -313,6 +313,11 @@ TEST_F(OmniboxEditModelTest, DisplayText) {
   EXPECT_TRUE(model()->user_input_in_progress());
   EXPECT_TRUE(view()->IsSelectAll());
   EXPECT_TRUE(model()->CurrentTextIsURL());
+
+  // We should still show the current page's icon until the URL is modified.
+  EXPECT_TRUE(model()->ShouldShowCurrentPageIcon());
+  view()->SetUserText(base::ASCIIToUTF16("something else"));
+  EXPECT_FALSE(model()->ShouldShowCurrentPageIcon());
 }
 
 TEST_F(OmniboxEditModelTest, DisplayAndExitQueryInOmnibox) {
@@ -323,14 +328,17 @@ TEST_F(OmniboxEditModelTest, DisplayAndExitQueryInOmnibox) {
   TestOmniboxClient* client =
       static_cast<TestOmniboxClient*>(model()->client());
   client->SetFakeSearchTermsForQueryInOmnibox(base::ASCIIToUTF16("foobar"));
-  model()->ResetDisplayTexts();
+  EXPECT_TRUE(model()->ResetDisplayTexts());
+  model()->Revert();
   EXPECT_EQ(base::ASCIIToUTF16("foobar"), model()->GetPermanentDisplayText());
+  EXPECT_EQ(base::ASCIIToUTF16("foobar"), view()->GetText());
 
   base::string16 search_terms;
   EXPECT_TRUE(model()->GetQueryInOmniboxSearchTerms(&search_terms));
   EXPECT_FALSE(search_terms.empty());
   EXPECT_EQ(base::ASCIIToUTF16("foobar"), search_terms);
   EXPECT_FALSE(model()->CurrentTextIsURL());
+  EXPECT_TRUE(model()->ShouldShowCurrentPageIcon());
 
   // Verify we can exit Query in Omnibox mode properly.
   model()->Unelide(true /* exit_query_in_omnibox */);
@@ -338,6 +346,11 @@ TEST_F(OmniboxEditModelTest, DisplayAndExitQueryInOmnibox) {
   EXPECT_TRUE(model()->user_input_in_progress());
   EXPECT_TRUE(view()->IsSelectAll());
   EXPECT_TRUE(model()->CurrentTextIsURL());
+
+  // We should still show the current page's icon until the URL is modified.
+  EXPECT_TRUE(model()->ShouldShowCurrentPageIcon());
+  view()->SetUserText(base::ASCIIToUTF16("something else"));
+  EXPECT_FALSE(model()->ShouldShowCurrentPageIcon());
 }
 
 TEST_F(OmniboxEditModelTest, DisablePasteAndGoForLongTexts) {

components/omnibox/browser/omnibox_view.cc

@@ -117,10 +117,18 @@ gfx::ImageSkia OmniboxView::GetIcon(int dip_size,
   NOTREACHED();
   return gfx::ImageSkia();
 #else
-  if (!IsEditingOrEmpty()) {
+
+  // For tests, model_ will be null.
+  if (!model_) {
+    AutocompleteMatch fake_match;
+    fake_match.type = AutocompleteMatchType::URL_WHAT_YOU_TYPED;
+    const gfx::VectorIcon& vector_icon = fake_match.GetVectorIcon(false);
+    return gfx::CreateVectorIcon(vector_icon, dip_size, color);
+  }
+
+  if (model_->ShouldShowCurrentPageIcon()) {
     // Query in Omnibox.
-    if (model_ &&
-        model_->GetQueryInOmniboxSearchTerms(nullptr /* search_terms */)) {
+    if (model_->GetQueryInOmniboxSearchTerms(nullptr /* search_terms */)) {
       gfx::Image icon = model_->client()->GetFaviconForDefaultSearchProvider(
           std::move(on_icon_fetched));
       if (!icon.IsEmpty())
@@ -131,16 +139,7 @@ gfx::ImageSkia OmniboxView::GetIcon(int dip_size,
         controller_->GetLocationBarModel()->GetVectorIcon(), dip_size, color);
   }
 
-  // For tests, model_ will be null.
-  if (!model_) {
-    AutocompleteMatch fake_match;
-    fake_match.type = AutocompleteMatchType::URL_WHAT_YOU_TYPED;
-    const gfx::VectorIcon& vector_icon = fake_match.GetVectorIcon(false);
-    return gfx::CreateVectorIcon(vector_icon, dip_size, color);
-  }
-
   gfx::Image favicon;
-
   AutocompleteMatch match = model_->CurrentMatch(nullptr);
   if (AutocompleteMatch::IsSearchType(match.type)) {
     // For search queries, display default search engine's favicon.