Omnibox: Make a single Ctrl+L reveal the full URL

After this CL, a single Ctrl+L will reveal the full URL.

This applies to beth Query in Omnibox and Steady State Elisions.

This CL also adds a unit test, as well as an internal-only killswitch
in case we need to remotely disable this feature.

Bug: 874592, 882348
Change-Id: I6bce176e613b46bfc9b6ce524ddf855bdb14f399
Reviewed-on: https://chromium-review.googlesource.com/c/1285511
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603584}

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

@@ -111,6 +111,16 @@
 #include "chrome/browser/ui/views/location_bar/intent_picker_view.h"
 #endif
 
+namespace {
+
+// This feature shows the full URL when the user focuses the omnibox via
+// keyboard shortcut. This feature flag only exists so we have a remote
+// killswitch for this behavior.
+base::Feature kOmniboxShowFullUrlOnKeyboardShortcut{
+    "OmniboxShowFullUrlOnKeyboardShortcut", base::FEATURE_ENABLED_BY_DEFAULT};
+
+}  // namespace
+
 using content::WebContents;
 using views::View;
 
@@ -368,7 +378,7 @@ bool LocationBarView::ShowPageInfoDialog(WebContents* contents) {
   // We are currently gating this behavior on the Query in Omnibox flag, since
   // it's still under active experimentation.
   if (base::FeatureList::IsEnabled(omnibox::kQueryInOmnibox))
-    omnibox_view()->model()->SetUserTextToURLForEditing();
+    omnibox_view()->model()->Unelide(true /* exit_query_in_omnibox */);
 
   return true;
 }
@@ -377,7 +387,10 @@ bool LocationBarView::ShowPageInfoDialog(WebContents* contents) {
 // LocationBarView, public LocationBar implementation:
 
 void LocationBarView::FocusLocation(bool select_all) {
-  bool already_focused = omnibox_view_->HasFocus();
+  // Only exit Query in Omnibox mode on focus command if the location bar was
+  // already focused to begin with, i.e. user presses Ctrl+L twice.
+  bool exit_query_in_omnibox = omnibox_view_->HasFocus();
+
   omnibox_view_->SetFocus();
 
   if (!select_all)
@@ -385,12 +398,8 @@ void LocationBarView::FocusLocation(bool select_all) {
 
   omnibox_view_->SelectAll(true);
 
-  // If the location bar is already focused, a second command to focus it
-  // should expose the full URL, temporarily disabling Steady State Elisions
-  // and Query in Omnibox. This behavior is currently gated on the Query in
-  // Omnibox flag, as it's still under active experimentation.
-  if (already_focused && base::FeatureList::IsEnabled(omnibox::kQueryInOmnibox))
-    omnibox_view()->model()->SetUserTextToURLForEditing();
+  if (base::FeatureList::IsEnabled(kOmniboxShowFullUrlOnKeyboardShortcut))
+    omnibox_view()->model()->Unelide(exit_query_in_omnibox);
 }
 
 void LocationBarView::Revert() {

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

@@ -318,11 +318,14 @@ void OmniboxViewViews::SetFocus() {
   // Temporarily reveal the top-of-window views (if not already revealed) so
   // that the location bar view is visible and is considered focusable. When it
   // actually receives focus, ImmersiveFocusWatcher will add another lock to
-  // keep it revealed.
-  std::unique_ptr<ImmersiveRevealedLock> focus_reveal_lock(
-      BrowserView::GetBrowserViewForBrowser(location_bar_view_->browser())
-          ->immersive_mode_controller()
-          ->GetRevealedLock(ImmersiveModeController::ANIMATE_REVEAL_YES));
+  // keep it revealed. |location_bar_view_| can be nullptr in unit tests.
+  std::unique_ptr<ImmersiveRevealedLock> focus_reveal_lock;
+  if (location_bar_view_) {
+    focus_reveal_lock.reset(
+        BrowserView::GetBrowserViewForBrowser(location_bar_view_->browser())
+            ->immersive_mode_controller()
+            ->GetRevealedLock(ImmersiveModeController::ANIMATE_REVEAL_YES));
+  }
 
   RequestFocus();
   // Restore caret visibility if focus is explicitly requested. This is
@@ -650,7 +653,8 @@ bool OmniboxViewViews::UnapplySteadyStateElisions(UnelisionGesture gesture) {
   if (model()->user_input_in_progress())
     return false;
 
-  // Don't unelide if we are currently displaying Query in Omnibox search terms.
+  // Don't unelide if we are currently displaying Query in Omnibox search terms,
+  // as otherwise, it would be impossible to refine query terms.
   if (model()->GetQueryInOmniboxSearchTerms(nullptr /* search_terms */))
     return false;
 
@@ -692,7 +696,8 @@ bool OmniboxViewViews::UnapplySteadyStateElisions(UnelisionGesture gesture) {
     OffsetDoubleClickWord(offset);
   }
 
-  model()->SetUserTextToURLForEditing();
+  // We have already early-exited if Query in Omnibox is active.
+  model()->Unelide(false /* exit_query_in_omnibox */);
   SelectRange(gfx::Range(start, end));
   return true;
 }

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

@@ -479,17 +479,11 @@ TEST_F(OmniboxViewViewsTest, Emphasis) {
 }
 
 TEST_F(OmniboxViewViewsTest, RevertOnBlur) {
-  // Since this test is not focused on steady state elisions, set the full
-  // formatted URL and the url for display to the same value.
-  toolbar_model()->set_formatted_full_url(
-      base::ASCIIToUTF16("https://permanent-text.com"));
-  toolbar_model()->set_url_for_display(
-      base::ASCIIToUTF16("https://permanent-text.com"));
-
+  toolbar_model()->set_url(GURL("https://permanent-text.com/"));
   omnibox_view()->model()->ResetDisplayTexts();
   omnibox_view()->RevertAll();
 
-  EXPECT_EQ(base::ASCIIToUTF16("https://permanent-text.com"),
+  EXPECT_EQ(base::ASCIIToUTF16("https://permanent-text.com/"),
             omnibox_view()->text());
   EXPECT_FALSE(omnibox_view()->model()->user_input_in_progress());
 
@@ -505,10 +499,11 @@ TEST_F(OmniboxViewViewsTest, RevertOnBlur) {
 
   // Expect that on blur, if the text is the same as the
   // https://permanent-text.com, exit user input mode.
-  omnibox_view()->SetUserText(base::ASCIIToUTF16("https://permanent-text.com"));
+  omnibox_view()->SetUserText(
+      base::ASCIIToUTF16("https://permanent-text.com/"));
   EXPECT_TRUE(omnibox_view()->model()->user_input_in_progress());
   omnibox_textfield()->OnBlur();
-  EXPECT_EQ(base::ASCIIToUTF16("https://permanent-text.com"),
+  EXPECT_EQ(base::ASCIIToUTF16("https://permanent-text.com/"),
             omnibox_view()->text());
   EXPECT_FALSE(omnibox_view()->model()->user_input_in_progress());
 }
@@ -548,7 +543,7 @@ class OmniboxViewViewsSteadyStateElisionsTest : public OmniboxViewViewsTest {
       : OmniboxViewViewsTest(enabled_features) {}
 
   const int kCharacterWidth = 10;
-  const base::string16 kFullUrl = base::ASCIIToUTF16("https://www.example.com");
+  const GURL kFullUrl = GURL("https://www.example.com/");
 
   void SetUp() override {
     OmniboxViewViewsTest::SetUp();
@@ -557,7 +552,7 @@ class OmniboxViewViewsSteadyStateElisionsTest : public OmniboxViewViewsTest {
     clock_.Advance(base::TimeDelta::FromSeconds(5));
     ui::SetEventTickClockForTesting(&clock_);
 
-    toolbar_model()->set_formatted_full_url(kFullUrl);
+    toolbar_model()->set_url(kFullUrl);
     toolbar_model()->set_url_for_display(base::ASCIIToUTF16("example.com"));
 
     gfx::test::RenderTextTestApi render_text_test_api(
@@ -580,13 +575,14 @@ class OmniboxViewViewsSteadyStateElisionsTest : public OmniboxViewViewsTest {
   }
 
   void ExpectFullUrlDisplayed() {
-    EXPECT_EQ(kFullUrl, omnibox_view()->text());
+    EXPECT_EQ(base::UTF8ToUTF16(kFullUrl.spec()), omnibox_view()->text());
     EXPECT_TRUE(omnibox_view()->model()->user_input_in_progress());
 
     // We test the user text stored in the model has been updated as well. The
     // model user text is used to populate the text in the Omnibox after some
     // state transitions, such as the ZeroSuggest popup opening.
-    EXPECT_EQ(kFullUrl, omnibox_view()->model()->GetUserTextForTesting());
+    EXPECT_EQ(base::UTF8ToUTF16(kFullUrl.spec()),
+              omnibox_view()->model()->GetUserTextForTesting());
   }
 
   bool IsElidedUrlDisplayed() {
@@ -768,7 +764,7 @@ TEST_F(OmniboxViewViewsSteadyStateElisionsTest, MouseTripleClick) {
   size_t start, end;
   omnibox_view()->GetSelectionBounds(&start, &end);
   EXPECT_EQ(0U, start);
-  EXPECT_EQ(23U, end);
+  EXPECT_EQ(24U, end);
 }
 
 TEST_F(OmniboxViewViewsSteadyStateElisionsTest, MouseClickDrag) {
@@ -892,12 +888,12 @@ TEST_F(OmniboxViewViewsSteadyStateElisionsTest, DontReelideOnBlurIfEdited) {
                           ui::DomKey::FromCharacter('a'),
                           ui::EventTimeForNow());
   omnibox_textfield()->InsertChar(char_event);
-  EXPECT_EQ(base::ASCIIToUTF16("https://www.a.com"), omnibox_view()->text());
+  EXPECT_EQ(base::ASCIIToUTF16("https://www.a.com/"), omnibox_view()->text());
   EXPECT_TRUE(omnibox_view()->model()->user_input_in_progress());
 
   // Now that we've edited the text, blurring should not re-elide the URL.
   BlurOmnibox();
-  EXPECT_EQ(base::ASCIIToUTF16("https://www.a.com"), omnibox_view()->text());
+  EXPECT_EQ(base::ASCIIToUTF16("https://www.a.com/"), omnibox_view()->text());
   EXPECT_TRUE(omnibox_view()->model()->user_input_in_progress());
 }
 
@@ -935,6 +931,18 @@ TEST_F(OmniboxViewViewsSteadyStateElisionsTest, SaveSelectAllOnBlurAndRefocus) {
   EXPECT_TRUE(omnibox_view()->IsSelectAll());
 }
 
+TEST_F(OmniboxViewViewsSteadyStateElisionsTest, UnelideFromModel) {
+  EXPECT_TRUE(IsElidedUrlDisplayed());
+
+  omnibox_view()->model()->Unelide(false /* exit_query_in_omnibox */);
+  EXPECT_TRUE(omnibox_view()->IsSelectAll());
+  size_t start, end;
+  omnibox_view()->GetSelectionBounds(&start, &end);
+  EXPECT_EQ(24U, start);
+  EXPECT_EQ(0U, end);
+  ExpectFullUrlDisplayed();
+}
+
 class OmniboxViewViewsSteadyStateElisionsAndQueryInOmniboxTest
     : public OmniboxViewViewsSteadyStateElisionsTest {
  public:
@@ -944,24 +952,30 @@ class OmniboxViewViewsSteadyStateElisionsAndQueryInOmniboxTest
             toolbar::features::kHideSteadyStateUrlTrivialSubdomains,
             omnibox::kQueryInOmnibox,
         }) {}
-};
 
-TEST_F(OmniboxViewViewsSteadyStateElisionsAndQueryInOmniboxTest,
-       DontUnelideQueryInOmniboxSearchTerms) {
+ protected:
   const GURL kValidSearchResultsPage =
       GURL("https://www.google.com/search?q=foo+query");
-  toolbar_model()->set_url(kValidSearchResultsPage);
-  toolbar_model()->set_security_level(security_state::SecurityLevel::SECURE);
 
-  omnibox_view()->model()->ResetDisplayTexts();
-  omnibox_view()->RevertAll();
+  void SetUp() override {
+    OmniboxViewViewsSteadyStateElisionsTest::SetUp();
 
-  // Sanity check that Query in Omnibox is working with Steady State Elisions.
-  EXPECT_EQ(base::ASCIIToUTF16("foo query"), omnibox_view()->text());
+    toolbar_model()->set_url(kValidSearchResultsPage);
+    toolbar_model()->set_security_level(security_state::SecurityLevel::SECURE);
 
-  // Focus the Omnibox.
-  SendMouseClick(0);
+    omnibox_view()->model()->ResetDisplayTexts();
+    omnibox_view()->RevertAll();
+
+    // Sanity check that Query in Omnibox is working with Steady State Elisions.
+    EXPECT_EQ(base::ASCIIToUTF16("foo query"), omnibox_view()->text());
 
+    // Focus the Omnibox.
+    SendMouseClick(0);
+  }
+};
+
+TEST_F(OmniboxViewViewsSteadyStateElisionsAndQueryInOmniboxTest,
+       DontUnelideQueryInOmniboxSearchTerms) {
   // Right key should NOT unelide, and should correctly place the cursor at the
   // end of the search query.
   omnibox_textfield_view()->OnKeyPressed(
@@ -974,3 +988,27 @@ TEST_F(OmniboxViewViewsSteadyStateElisionsAndQueryInOmniboxTest,
   EXPECT_EQ(9U, start);
   EXPECT_EQ(9U, end);
 }
+
+TEST_F(OmniboxViewViewsSteadyStateElisionsAndQueryInOmniboxTest,
+       UnelideFromModel) {
+  // Uneliding without exiting Query in Omnibox should do nothing.
+  omnibox_view()->model()->Unelide(false /* exit_query_in_omnibox */);
+  EXPECT_EQ(base::ASCIIToUTF16("foo query"), omnibox_view()->text());
+  {
+    size_t start, end;
+    omnibox_view()->GetSelectionBounds(&start, &end);
+    EXPECT_EQ(9U, start);
+    EXPECT_EQ(0U, end);
+  }
+
+  // Uneliding and exiting Query in Omnibox should reveal the full URL.
+  omnibox_view()->model()->Unelide(true /* exit_query_in_omnibox */);
+  EXPECT_EQ(base::ASCIIToUTF16(kValidSearchResultsPage.spec()),
+            omnibox_view()->text());
+  {
+    size_t start, end;
+    omnibox_view()->GetSelectionBounds(&start, &end);
+    EXPECT_EQ(41U, start);
+    EXPECT_EQ(0U, end);
+  }
+}

components/omnibox/browser/omnibox_edit_model.cc

@@ -283,7 +283,16 @@ void OmniboxEditModel::SetUserText(const base::string16& text) {
   has_temporary_text_ = false;
 }
 
-void OmniboxEditModel::SetUserTextToURLForEditing() {
+void OmniboxEditModel::Unelide(bool exit_query_in_omnibox) {
+  // Unelision should not occur if the user has already inputted text.
+  if (user_input_in_progress())
+    return;
+
+  // Early exit if we don't want to exit Query in Omnibox mode, and the omnibox
+  // is displaying a query.
+  if (!exit_query_in_omnibox && GetQueryInOmniboxSearchTerms(nullptr))
+    return;
+
   SetUserText(url_for_editing_);
   view_->SetWindowTextAndCaretPos(url_for_editing_, 0, false, false);
 

components/omnibox/browser/omnibox_edit_model.h

@@ -176,10 +176,11 @@ class OmniboxEditModel {
   // Sets the user_text_ to |text|.
   void SetUserText(const base::string16& text);
 
-  // Sets the user text to be url_for_editing_. This also selects all and
-  // enters user-input-in-progress mode. This method is used to exit both
-  // Steady State Elisions and Query in Omnibox mode.
-  void SetUserTextToURLForEditing();
+  // Unapplies any Steady State Elisions by setting the user text to be
+  // url_for_editing_. This also selects all and enters user-input-in-progress
+  // mode. If |exit_query_in_omnibox| is set to true, this will alse exit
+  // Query in Omnibox mode if the omnibox is showing a query.
+  void Unelide(bool exit_query_in_omnibox);
 
   // Invoked any time the text may have changed in the edit. Notifies the
   // controller.

components/omnibox/browser/omnibox_edit_model_unittest.cc

@@ -158,8 +158,7 @@ TEST_F(OmniboxEditModelTest, AdjustTextForCopy) {
 // Tests that AdjustTextForCopy behaves properly with Query in Omnibox enabled.
 // For more general tests of copy adjustment, see the AdjustTextForCopy test.
 TEST_F(OmniboxEditModelTest, AdjustTextForCopyQueryInOmnibox) {
-  toolbar_model()->set_formatted_full_url(
-      base::ASCIIToUTF16("https://www.example.com/"));
+  toolbar_model()->set_url(GURL("https://www.example.com/"));
   toolbar_model()->set_url_for_display(base::ASCIIToUTF16("example.com"));
 
   TestOmniboxClient* client =
@@ -255,8 +254,7 @@ TEST_F(OmniboxEditModelTest, AlternateNavHasHTTP) {
 }
 
 TEST_F(OmniboxEditModelTest, CurrentMatch) {
-  toolbar_model()->set_formatted_full_url(
-      base::ASCIIToUTF16("http://localhost/"));
+  toolbar_model()->set_url(GURL("http://localhost/"));
   toolbar_model()->set_url_for_display(base::ASCIIToUTF16("localhost"));
   model()->ResetDisplayTexts();
 
@@ -283,29 +281,38 @@ TEST_F(OmniboxEditModelTest, CurrentMatch) {
 }
 
 TEST_F(OmniboxEditModelTest, DisplayText) {
-  toolbar_model()->set_formatted_full_url(
-      base::ASCIIToUTF16("https://www.example.com/"));
+  toolbar_model()->set_url(GURL("https://www.example.com/"));
   toolbar_model()->set_url_for_display(base::ASCIIToUTF16("example.com"));
 
   // Verify we show the display text when there is no Query in Omnibox match.
-  {
-    model()->ResetDisplayTexts();
+  model()->ResetDisplayTexts();
 #if defined(OS_IOS)
-    // iOS OmniboxEditModel always provides the full URL as the OmniboxView
-    // permanent display text.
-    EXPECT_EQ(base::ASCIIToUTF16("https://www.example.com/"),
-              model()->GetPermanentDisplayText());
+  // iOS OmniboxEditModel always provides the full URL as the OmniboxView
+  // permanent display text.
+  EXPECT_EQ(base::ASCIIToUTF16("https://www.example.com/"),
+            model()->GetPermanentDisplayText());
 #else
-    EXPECT_EQ(base::ASCIIToUTF16("example.com"),
-              model()->GetPermanentDisplayText());
+  EXPECT_EQ(base::ASCIIToUTF16("example.com"),
+            model()->GetPermanentDisplayText());
 #endif
 
-    base::string16 search_terms;
-    EXPECT_FALSE(model()->GetQueryInOmniboxSearchTerms(&search_terms));
-    EXPECT_TRUE(search_terms.empty());
+  base::string16 search_terms;
+  EXPECT_FALSE(model()->GetQueryInOmniboxSearchTerms(&search_terms));
+  EXPECT_TRUE(search_terms.empty());
 
-    EXPECT_TRUE(model()->CurrentTextIsURL());
-  }
+  EXPECT_TRUE(model()->CurrentTextIsURL());
+
+  // Verify we can unelide and show the full URL properly.
+  model()->Unelide(false /* exit_query_in_omnibox */);
+  EXPECT_EQ(base::ASCIIToUTF16("https://www.example.com/"), view()->GetText());
+  EXPECT_TRUE(model()->user_input_in_progress());
+  EXPECT_TRUE(view()->IsSelectAll());
+  EXPECT_TRUE(model()->CurrentTextIsURL());
+}
+
+TEST_F(OmniboxEditModelTest, DisplayAndExitQueryInOmnibox) {
+  toolbar_model()->set_url(GURL("https://www.example.com/"));
+  toolbar_model()->set_url_for_display(base::ASCIIToUTF16("example.com"));
 
   // Verify the displayed text when there is a Query in Omnibox match.
   TestOmniboxClient* client =
@@ -321,10 +328,11 @@ TEST_F(OmniboxEditModelTest, DisplayText) {
   EXPECT_FALSE(model()->CurrentTextIsURL());
 
   // Verify we can exit Query in Omnibox mode properly.
-  model()->SetUserTextToURLForEditing();
+  model()->Unelide(true /* exit_query_in_omnibox */);
   EXPECT_EQ(base::ASCIIToUTF16("https://www.example.com/"), view()->GetText());
   EXPECT_TRUE(model()->user_input_in_progress());
   EXPECT_TRUE(view()->IsSelectAll());
+  EXPECT_TRUE(model()->CurrentTextIsURL());
 }
 
 TEST_F(OmniboxEditModelTest, DisablePasteAndGoForLongTexts) {

components/omnibox/browser/omnibox_view.h

@@ -143,7 +143,7 @@ class OmniboxView {
   // defines a method with that name.
   virtual void CloseOmniboxPopup();
 
-  // Sets the focus to the autocomplete view.
+  // Sets the focus to the omnibox.
   virtual void SetFocus() = 0;
 
   // Shows or hides the caret based on whether the model's is_caret_visible() is

components/omnibox/browser/test_toolbar_model.cc

@@ -4,6 +4,8 @@
 
 #include "components/omnibox/browser/test_toolbar_model.h"
 
+#include "base/strings/utf_string_conversions.h"
+
 #if defined(TOOLKIT_VIEWS)
 #include "components/omnibox/browser/vector_icons.h"  // nogncheck
 #endif
@@ -19,11 +21,17 @@ TestToolbarModel::TestToolbarModel()
 TestToolbarModel::~TestToolbarModel() {}
 
 base::string16 TestToolbarModel::GetFormattedFullURL() const {
-  return formatted_full_url_;
+  if (!formatted_full_url_)
+    return base::UTF8ToUTF16(url_.spec());
+
+  return *formatted_full_url_;
 }
 
 base::string16 TestToolbarModel::GetURLForDisplay() const {
-  return url_for_display_;
+  if (!url_for_display_)
+    return base::UTF8ToUTF16(url_.spec());
+
+  return *url_for_display_;
 }
 
 GURL TestToolbarModel::GetURL() const {

components/omnibox/browser/test_toolbar_model.h

@@ -6,6 +6,7 @@
 #define COMPONENTS_OMNIBOX_BROWSER_TEST_TOOLBAR_MODEL_H_
 
 #include <stddef.h>
+#include <memory>
 
 #include "base/compiler_specific.h"
 #include "base/macros.h"
@@ -36,10 +37,10 @@ class TestToolbarModel : public ToolbarModel {
   bool IsOfflinePage() const override;
 
   void set_formatted_full_url(const base::string16& url) {
-    formatted_full_url_ = url;
+    formatted_full_url_ = std::make_unique<base::string16>(url);
   }
   void set_url_for_display(const base::string16& url) {
-    url_for_display_ = url;
+    url_for_display_ = std::make_unique<base::string16>(url);
   }
   void set_url(const GURL& url) { url_ = url; }
   void set_security_level(security_state::SecurityLevel security_level) {
@@ -55,8 +56,11 @@ class TestToolbarModel : public ToolbarModel {
   void set_offline_page(bool offline_page) { offline_page_ = offline_page; }
 
  private:
-  base::string16 formatted_full_url_;
-  base::string16 url_for_display_;
+  // If either of these is not explicitly set, the test class will return
+  // |url_.spec()| for the URL for display or fully formatted URL.
+  std::unique_ptr<base::string16> formatted_full_url_;
+  std::unique_ptr<base::string16> url_for_display_;
+
   GURL url_;
   security_state::SecurityLevel security_level_ = security_state::NONE;
   const gfx::VectorIcon* icon_ = nullptr;