Omnibox UI Experiments: Steady State Elisions - Fix Cut to clipboard

This CL improves the after-Cut text processing to reliably handle the
Cut case when Steady State Elisions is on.

Previously, the logic relied on IsSelectAll, which doesn't work
correctly in the Cut case. Now it just compares the contents of the
paste buffer vs. the current page's display URL.

This also has the side benefit of simplifying the API.

It also updates a test to explicitly check a Steady State Elisions
case.

Bug: 838159, 797354
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Iae3ee431051e2f7bbbf63a0b87e82b7f363213aa
Reviewed-on: https://chromium-review.googlesource.com/1040912Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556113}

chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm

@@ -901,8 +901,7 @@ base::scoped_nsobject<NSPasteboardItem> OmniboxViewMac::CreatePasteboardItem() {
   // Copy the URL.
   GURL url;
   bool write_url = false;
-  model()->AdjustTextForCopy(selection.location, IsSelectAll(), &text, &url,
-                             &write_url);
+  model()->AdjustTextForCopy(selection.location, &text, &url, &write_url);
 
   if (IsSelectAll())
     UMA_HISTOGRAM_COUNTS(OmniboxEditModel::kCutOrCopyAllTextHistogram, 1);

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

@@ -1257,8 +1257,8 @@ void OmniboxViewViews::OnAfterCutOrCopy(ui::ClipboardType clipboard_type) {
   cb->ReadText(clipboard_type, &selected_text);
   GURL url;
   bool write_url;
-  model()->AdjustTextForCopy(GetSelectedRange().GetMin(), IsSelectAll(),
-                             &selected_text, &url, &write_url);
+  model()->AdjustTextForCopy(GetSelectedRange().GetMin(), &selected_text, &url,
+                             &write_url);
   if (IsSelectAll())
     UMA_HISTOGRAM_COUNTS(OmniboxEditModel::kCutOrCopyAllTextHistogram, 1);
 
@@ -1269,15 +1269,14 @@ void OmniboxViewViews::OnAfterCutOrCopy(ui::ClipboardType clipboard_type) {
 void OmniboxViewViews::OnWriteDragData(ui::OSExchangeData* data) {
   GURL url;
   bool write_url;
-  bool is_all_selected = IsSelectAll();
   base::string16 selected_text = GetSelectedText();
-  model()->AdjustTextForCopy(GetSelectedRange().GetMin(), is_all_selected,
-                             &selected_text, &url, &write_url);
+  model()->AdjustTextForCopy(GetSelectedRange().GetMin(), &selected_text, &url,
+                             &write_url);
   data->SetString(selected_text);
   if (write_url) {
     gfx::Image favicon;
     base::string16 title = selected_text;
-    if (is_all_selected)
+    if (IsSelectAll())
       model()->GetDataForURLExport(&url, &title, &favicon);
     button_drag_utils::SetURLAndDragImage(url, title, favicon.AsImageSkia(),
                                           nullptr, *GetWidget(), data);
@@ -1289,8 +1288,8 @@ void OmniboxViewViews::OnGetDragOperationsForTextfield(int* drag_operations) {
   base::string16 selected_text = GetSelectedText();
   GURL url;
   bool write_url;
-  model()->AdjustTextForCopy(GetSelectedRange().GetMin(), IsSelectAll(),
-                             &selected_text, &url, &write_url);
+  model()->AdjustTextForCopy(GetSelectedRange().GetMin(), &selected_text, &url,
+                             &write_url);
   if (write_url)
     *drag_operations |= ui::DragDropTypes::DRAG_LINK;
 }

components/omnibox/browser/omnibox_edit_model.cc

@@ -288,7 +288,6 @@ AutocompleteMatch::Type OmniboxEditModel::CurrentTextType() const {
 }
 
 void OmniboxEditModel::AdjustTextForCopy(int sel_min,
-                                         bool is_all_selected,
                                          base::string16* text,
                                          GURL* url_from_text,
                                          bool* write_url) {
@@ -298,25 +297,9 @@ void OmniboxEditModel::AdjustTextForCopy(int sel_min,
   if (sel_min != 0)
     return;
 
-  // Check whether the user is trying to copy the current page's URL by
-  // selecting the whole thing without editing it.
-  //
-  // This is complicated by ZeroSuggest.  When ZeroSuggest is active, the user
-  // may be selecting different items and thus changing the address bar text,
-  // even though !user_input_in_progress_; and the permanent URL may change
-  // without updating the visible text, just like when user input is in
-  // progress.  In these cases, we don't want to copy the underlying URL, we
-  // want to copy what the user actually sees.  However, if we simply never do
-  // this block when !PopupIsOpen(), then just clicking into the address bar and
-  // trying to copy will always bypass this block on pages that trigger
-  // ZeroSuggest, which is too conservative.  Instead, in the ZeroSuggest case,
-  // we check that (a) the user hasn't selected one of the other suggestions,
-  // and (b) the selected text is still the same as the permanent text.  ((b)
-  // probably implies (a), but it doesn't hurt to be sure.)  If these hold, then
-  // it's safe to copy the underlying URL.
-  if (!user_input_in_progress_ && is_all_selected &&
-      (!PopupIsOpen() || ((popup_model()->selected_line() == 0) &&
-                          (*text == url_for_editing_)))) {
+  // Check whether the user is trying to copy the current page's URL by checking
+  // if |text| is the whole permanent URL.
+  if (!user_input_in_progress_ && *text == GetCurrentPermanentUrlText()) {
     // It's safe to copy the underlying URL.  These lines ensure that if the
     // scheme was stripped it's added back, and the URL is unescaped (we escape
     // parts of it for display).

components/omnibox/browser/omnibox_edit_model.h

@@ -137,11 +137,9 @@ class OmniboxEditModel {
   // Invoked to adjust the text before writting to the clipboard for a copy
   // (e.g. by adding 'http' to the front). |sel_min| gives the minimum position
   // of the selection e.g. min(selection_start, selection_end). |text| is the
-  // currently selected text. If |is_all_selected| is true all the text in the
-  // edit is selected. If the url should be copied to the clipboard |write_url|
-  // is set to true and |url_from_text| set to the url to write.
+  // currently selected text. If the url should be copied to the clipboard
+  // |write_url| is set to true and |url_from_text| set to the url to write.
   void AdjustTextForCopy(int sel_min,
-                         bool is_all_selected,
                          base::string16* text,
                          GURL* url_from_text,
                          bool* write_url);

components/omnibox/browser/omnibox_edit_model_unittest.cc

@@ -8,8 +8,10 @@
 
 #include <memory>
 
+#include "base/test/scoped_feature_list.h"
 #include "base/test/scoped_task_environment.h"
 #include "components/omnibox/browser/autocomplete_match.h"
+#include "components/omnibox/browser/omnibox_field_trial.h"
 #include "components/omnibox/browser/omnibox_view.h"
 #include "components/omnibox/browser/search_provider.h"
 #include "components/omnibox/browser/test_omnibox_client.h"
@@ -74,7 +76,6 @@ TEST_F(OmniboxEditModelTest, AdjustTextForCopy) {
   struct Data {
     const char* url_for_editing;
     const int sel_start;
-    const bool is_all_selected;
 
     const char* match_destination_url;
     const bool is_match_selected_in_popup;
@@ -83,71 +84,82 @@ TEST_F(OmniboxEditModelTest, AdjustTextForCopy) {
     const char* expected_output;
     const bool write_url;
     const char* expected_url;
+
+    bool steady_state_elisions_on = false;
+    const char* url_for_display = "";
   } input[] = {
-      // Test that http:// and https:// are inserted if all text is selected.
-      {"a.de/b", 0, true, "", false, "a.de/b", "http://a.de/b", true,
+      // Test that http:// is inserted if all text is selected.
+      {"a.de/b", 0, "", false, "a.de/b", "http://a.de/b", true,
        "http://a.de/b"},
-      {"https://a.de/b", 0, true, "", false, "a.de/b", "https://a.de/b", true,
-       "https://a.de/b"},
 
       // Test that http:// and https:// are inserted if the host is selected.
-      {"a.de/b", 0, false, "", false, "a.de/", "http://a.de/", true,
-       "http://a.de/"},
-      {"https://a.de/b", 0, false, "", false, "https://a.de/", "https://a.de/",
-       true, "https://a.de/"},
+      {"a.de/b", 0, "", false, "a.de/", "http://a.de/", true, "http://a.de/"},
+      {"https://a.de/b", 0, "", false, "https://a.de/", "https://a.de/", true,
+       "https://a.de/"},
 
       // Tests that http:// is inserted if the path is modified.
-      {"a.de/b", 0, false, "", false, "a.de/c", "http://a.de/c", true,
+      {"a.de/b", 0, "", false, "a.de/c", "http://a.de/c", true,
        "http://a.de/c"},
 
       // Tests that http:// isn't inserted if the host is modified.
-      {"a.de/b", 0, false, "", false, "a.com/b", "a.com/b", false, ""},
+      {"a.de/b", 0, "", false, "a.com/b", "a.com/b", false, ""},
 
       // Tests that http:// isn't inserted if the start of the selection is 1.
-      {"a.de/b", 1, false, "", false, "a.de/b", "a.de/b", false, ""},
+      {"a.de/b", 1, "", false, "a.de/b", "a.de/b", false, ""},
 
       // Tests that http:// isn't inserted if a portion of the host is selected.
-      {"a.de/", 0, false, "", false, "a.d", "a.d", false, ""},
+      {"a.de/", 0, "", false, "a.d", "a.d", false, ""},
 
       // Tests that http:// isn't inserted if the user adds to the host.
-      {"a.de/", 0, false, "", false, "a.de.com/", "a.de.com/", false, ""},
+      {"a.de/", 0, "", false, "a.de.com/", "a.de.com/", false, ""},
 
       // Tests that we don't get double schemes if the user manually inserts
       // a scheme.
-      {"a.de/", 0, false, "", false, "http://a.de/", "http://a.de/", true,
+      {"a.de/", 0, "", false, "http://a.de/", "http://a.de/", true,
        "http://a.de/"},
-      {"a.de/", 0, false, "", false, "HTtp://a.de/", "HTtp://a.de/", true,
+      {"a.de/", 0, "", false, "HTtp://a.de/", "HTtp://a.de/", true,
        "http://a.de/"},
-      {"https://a.de/", 0, false, "", false, "https://a.de/", "https://a.de/",
-       true, "https://a.de/"},
+      {"https://a.de/", 0, "", false, "https://a.de/", "https://a.de/", true,
+       "https://a.de/"},
 
       // Test that we don't get double schemes or revert the change if the user
       // manually changes the scheme from 'http://' to 'https://' or vice versa.
-      {"a.de/", 0, false, "", false, "https://a.de/", "https://a.de/", true,
+      {"a.de/", 0, "", false, "https://a.de/", "https://a.de/", true,
        "https://a.de/"},
-      {"https://a.de/", 0, false, "", false, "http://a.de/", "http://a.de/",
-       true, "http://a.de/"},
+      {"https://a.de/", 0, "", false, "http://a.de/", "http://a.de/", true,
+       "http://a.de/"},
 
       // Makes sure intranet urls get 'http://' prefixed to them.
-      {"b/foo", 0, true, "", false, "b/foo", "http://b/foo", true,
-       "http://b/foo"},
+      {"b/foo", 0, "", false, "b/foo", "http://b/foo", true, "http://b/foo"},
 
       // Verifies a search term 'foo' doesn't end up with http.
-      {"www.google.com/search?", 0, false, "", false, "foo", "foo", false, ""},
+      {"www.google.com/search?", 0, "", false, "foo", "foo", false, ""},
 
       // Verifies that http:// and https:// are inserted for a match in a popup.
-      {"a.com", 0, true, "http://b.com/foo", true, "b.com/foo",
-       "http://b.com/foo", true, "http://b.com/foo"},
-      {"a.com", 0, true, "https://b.com/foo", true, "b.com/foo",
-       "https://b.com/foo", true, "https://b.com/foo"},
+      {"a.com", 0, "http://b.com/foo", true, "b.com/foo", "http://b.com/foo",
+       true, "http://b.com/foo"},
+      {"a.com", 0, "https://b.com/foo", true, "b.com/foo", "https://b.com/foo",
+       true, "https://b.com/foo"},
 
       // Verifies that no scheme is inserted if there is no valid match.
-      {"a.com", 0, true, "", true, "b.com/foo", "b.com/foo", false, ""},
+      {"a.com", 0, "", true, "b.com/foo", "b.com/foo", false, ""},
+
+      // Steady State Elisions test for re-adding an elided 'https://'.
+      {"https://a.de/b", 0, "", false, "a.de/b", "https://a.de/b", true,
+       "https://a.de/b", true, "a.de/b"},
   };
 
   for (size_t i = 0; i < arraysize(input); ++i) {
+    base::test::ScopedFeatureList feature_list;
+    if (input[i].steady_state_elisions_on) {
+      feature_list.InitAndEnableFeature(
+          omnibox::kUIExperimentHideSteadyStateUrlSchemeAndSubdomains);
+    }
+
     toolbar_model()->set_formatted_full_url(
         base::ASCIIToUTF16(input[i].url_for_editing));
+    toolbar_model()->set_url_for_display(
+        base::ASCIIToUTF16(input[i].url_for_display));
     model()->ResetDisplayUrls();
 
     model()->SetInputInProgress(input[i].is_match_selected_in_popup);
@@ -160,8 +172,7 @@ TEST_F(OmniboxEditModelTest, AdjustTextForCopy) {
     base::string16 result = base::ASCIIToUTF16(input[i].input);
     GURL url;
     bool write_url;
-    model()->AdjustTextForCopy(input[i].sel_start, input[i].is_all_selected,
-                               &result, &url, &write_url);
+    model()->AdjustTextForCopy(input[i].sel_start, &result, &url, &write_url);
     EXPECT_EQ(base::ASCIIToUTF16(input[i].expected_output), result)
         << "@: " << i;
     EXPECT_EQ(input[i].write_url, write_url) << " @" << i;

ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm

@@ -613,21 +613,14 @@ void OmniboxViewIOS::OnClear() {
 bool OmniboxViewIOS::OnCopy() {
   UIPasteboard* board = [UIPasteboard generalPasteboard];
   NSString* selectedText = nil;
-  BOOL is_select_all = NO;
   NSInteger start_location = 0;
   if ([field_ isPreEditing]) {
     selectedText = [field_ preEditText];
-    is_select_all = YES;
     start_location = 0;
   } else {
     UITextRange* selected_range = [field_ selectedTextRange];
     selectedText = [field_ textInRange:selected_range];
     UITextPosition* start = [field_ beginningOfDocument];
-    UITextPosition* end = [field_ endOfDocument];
-    is_select_all = ([field_ comparePosition:[selected_range start]
-                                  toPosition:start] == NSOrderedSame) &&
-                    ([field_ comparePosition:[selected_range end]
-                                  toPosition:end] == NSOrderedSame);
     // The following call to |-offsetFromPosition:toPosition:| gives the offset
     // in terms of the number of "visible characters."  The documentation does
     // not specify whether this means glyphs or UTF16 chars.  This does not
@@ -640,8 +633,7 @@ bool OmniboxViewIOS::OnCopy() {
 
   GURL url;
   bool write_url = false;
-  model()->AdjustTextForCopy(start_location, is_select_all, &text, &url,
-                             &write_url);
+  model()->AdjustTextForCopy(start_location, &text, &url, &write_url);
 
   // Create the pasteboard item manually because the pasteboard expects a single
   // item with multiple representations.  This is expressed as a single