Omnibox UI Experiments: Implement scheme-trimming for suggested URLs.

components/omnibox/browser/autocomplete_match.cc

 // Copyright 2014 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.
 
 #include "components/omnibox/browser/autocomplete_match.h"
 
 #include <algorithm>
 #include <utility>
 
+#include "base/feature_list.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "components/omnibox/browser/autocomplete_provider.h"
+#include "components/omnibox/browser/omnibox_field_trial.h"
 #include "components/omnibox/browser/suggestion_answer.h"
 #include "components/search_engines/template_url.h"
 #include "components/search_engines/template_url_service.h"
 #include "components/url_formatter/url_formatter.h"
 #include "ui/gfx/vector_icon_types.h"
 
 #if !defined(OS_ANDROID) && !defined(OS_IOS)
 #include "components/omnibox/browser/vector_icons.h"  // nogncheck
 #include "ui/vector_icons/vector_icons.h"             // nogncheck
 #endif
@@ skipping 447 matching lines @@
                            url::Component(0, strlen(url::kHttpScheme)));
     needs_replacement = true;
   }
 
   if (needs_replacement)
     stripped_destination_url = stripped_destination_url.ReplaceComponents(
         replacements);
   return stripped_destination_url;
 }
 
+// static
+base::string16 AutocompleteMatch::FormatUrlForSuggestionDisplay(
+    const GURL& url,
+    bool trim_scheme,
+    size_t* offset_for_adjustment) {
+  std::vector<size_t> offsets;
+  if (offset_for_adjustment)
+    offsets.push_back(*offset_for_adjustment);
+  base::string16 result =
+      FormatUrlForSuggestionDisplayWithOffsets(url, trim_scheme, &offsets);
+  if (offset_for_adjustment)
+    *offset_for_adjustment = offsets[0];
+  return result;
+}
+
+// static
+base::string16 AutocompleteMatch::FormatUrlForSuggestionDisplayWithOffsets(
+    const GURL& url,
+    bool trim_scheme,
+    std::vector<size_t>* offsets_for_adjustment) {
+  base::OffsetAdjuster::Adjustments adjustments;
+  const base::string16& format_url_return_value =
+      FormatUrlForSuggestionDisplayWithAdjustments(url, trim_scheme,
+                                                   &adjustments);
+  base::OffsetAdjuster::AdjustOffsets(adjustments, offsets_for_adjustment);
+  if (offsets_for_adjustment) {
+    std::for_each(
+        offsets_for_adjustment->begin(), offsets_for_adjustment->end(),
+        base::LimitOffset<std::string>(format_url_return_value.length()));
+  }
+  return format_url_return_value;
+}
+
+// static
+base::string16 AutocompleteMatch::FormatUrlForSuggestionDisplayWithAdjustments(
+    const GURL& url,
+    bool trim_scheme,
+    base::OffsetAdjuster::Adjustments* adjustments) {

[tommycli, 2017/06/21 18:53:44]

These three methods duplicate a lot of offset adjustment code found in
url_formatter.

The alternative Justin and I were discussing was to fold this logic into
url_formatter::FormatUrlWithAdjustments.

The downside of that is 1) We would have to extend the bitmask flags used in
FormatUrlWithAdjustments, and that would change the meaning of
kFormatUrlOmitAll. 2) These elisions are experimental and pretty application
specific.

But the upside is that then we wouldn't need these three methods that are kind
of duplicated.

We wanted to know your thoughts...

[Peter Kasting, 2017/06/21 23:17:27]

I'm a little leery of adding a new flag type and this functionality, which as
you mention does seem pretty specific.

However, it seems like this code really boils down to:
* We want to FormatUrlWithAdjustments(),
* And then add one more adjustment
* And some callers want to convert to offsets

And half of what you're adding here is about reimplementing the third bullet.

I wonder if you could refactor the code in url_formatter to expose some of these
helpers more.  Or possibly some refactoring should happen elsewhere, e.g.
AdjustOffsets() should take an optional third arg that's a limit to clamp
everything to.  (But maybe not, it looks like right now LimitOffsets is only
used by two main places, and one clamps before adjusting while the other clamps
after.  I don't know why someone would need to clamp before, shouldn't the
caller have done that?  But whatever.)

Basically, it seems like there's an opportunity to shrink down to "only add the
actual HTTPS-stripping here", at which point it probably makes sense to leave
this here until you know for sure this is going in for good, at which point
maybe you do pull it into url_formatter.

[tommycli, 2017/06/22 19:18:43]

Okay cool -- In that case, it seems like the best thing to do is to send this in
pretty much as is, and look for an opportunity for refactoring in a followup --
or after we find out if this is going to be enabled by default.

[tommycli, 2017/06/22 19:22:51]

FWIW, Justin actually liked that these convenience methods encapsulate the
generation of the correct FormatUrlTypes into a single boolean (omit_scheme)...

+  const url_formatter::FormatUrlTypes format_types =
+      url_formatter::kFormatUrlOmitAll &
+      ~(trim_scheme ? 0 : url_formatter::kFormatUrlOmitHTTP);
+  base::string16 result = url_formatter::FormatUrlWithAdjustments(
+      url, format_types, net::UnescapeRule::SPACES, nullptr, nullptr,
+      adjustments);
+
+  // Also trim HTTPS if experiment is enabled. Note this intentionally has
+  // no effect on view-source URLs.
+  if (trim_scheme && base::FeatureList::IsEnabled(
+                         omnibox::kUIExperimentHideSuggestionUrlScheme)) {
+    // TODO(tommycli): If this becomes enabled by default, investigate
+    // folding this logic into url_formatter::FormatUrlWithAdjustments.
+    const std::string kHTTPS =
+        std::string(url::kHttpsScheme) + url::kStandardSchemeSeparator;
+    if (base::StartsWith(result, base::ASCIIToUTF16(kHTTPS),
+                         base::CompareCase::SENSITIVE)) {

[Peter Kasting, 2017/06/21 23:17:27]

Can we just check if url.SchemeIs(url::kHttpsScheme)?  I'd think that'd be
simpler and maybe cheaper.

[tommycli, 2017/06/22 19:18:43]

Done.

+      const size_t kHTTPSSize = kHTTPS.size();
+      result = result.substr(kHTTPSSize);
+      adjustments->insert(adjustments->begin(),
+                          base::OffsetAdjuster::Adjustment(0, kHTTPSSize, 0));
+    }
+  }
+
+  return result;
+}
+
 void AutocompleteMatch::ComputeStrippedDestinationURL(
     const AutocompleteInput& input,
     TemplateURLService* template_url_service) {
   stripped_destination_url = GURLToStrippedGURL(
       destination_url, input, template_url_service, keyword);
 }
 
 void AutocompleteMatch::EnsureUWYTIsAllowedToBeDefault(
     const AutocompleteInput& input,
     TemplateURLService* template_url_service) {
@@ skipping 125 matching lines @@
         << " is unsorted in relation to last offset of " << last_offset
         << ". Provider: " << provider_name << ".";
     DCHECK_LT(i->offset, text.length())
         << " Classification of [" << i->offset << "," << text.length()
         << "] is out of bounds for \"" << text << "\". Provider: "
         << provider_name << ".";
     last_offset = i->offset;
   }
 }
 #endif