Omnibox UI Experiments: Strip trivial subdomains

components/url_formatter/url_formatter.cc

 // Copyright 2015 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/url_formatter/url_formatter.h"
 
 #include <algorithm>
 #include <utility>
 #include <vector>
 
 #include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/strings/string_piece.h"
+#include "base/strings/string_tokenizer.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_offset_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_local_storage.h"
 #include "components/url_formatter/idn_spoof_checker.h"
+#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "third_party/icu/source/common/unicode/uidna.h"
 #include "third_party/icu/source/common/unicode/utypes.h"
 #include "url/gurl.h"
 #include "url/third_party/mozilla/url_parse.h"
 
 namespace url_formatter {
 
 namespace {
 
 base::string16 IDNToUnicodeWithAdjustments(
     base::StringPiece host,
     base::OffsetAdjuster::Adjustments* adjustments);
 bool IDNToUnicodeOneComponent(const base::char16* comp,
                               size_t comp_len,
                               bool is_tld_ascii,
                               base::string16* out);
 
 class AppendComponentTransform {
  public:
   AppendComponentTransform() {}
   virtual ~AppendComponentTransform() {}
 
   virtual base::string16 Execute(
       const std::string& component_text,
       base::OffsetAdjuster::Adjustments* adjustments) const = 0;
 
   // NOTE: No DISALLOW_COPY_AND_ASSIGN here, since gcc < 4.3.0 requires an
   // accessible copy constructor in order to call AppendFormattedComponent()
   // with an inline temporary (see http://gcc.gnu.org/bugs/#cxx%5Frvalbind ).

[Peter Kasting, 2017/07/06 06:01:21]

FWIW, I think we could probably replace this comment with
DISALLOW_COPY_AND_ASSIGN since I think we no longer compile with gcc anywhere? 
But probably best done in a separate patch :)

[tommycli, 2017/07/06 16:25:46]

Acknowledged.

 };
 
 class HostComponentTransform : public AppendComponentTransform {
  public:
-  HostComponentTransform() {}
+  HostComponentTransform(bool trim_trivial_subdomains)
+      : trim_trivial_subdomains_(trim_trivial_subdomains) {}
 
  private:
   base::string16 Execute(
       const std::string& component_text,
       base::OffsetAdjuster::Adjustments* adjustments) const override {
-    return IDNToUnicodeWithAdjustments(component_text, adjustments);
+    if (!trim_trivial_subdomains_)
+      return IDNToUnicodeWithAdjustments(component_text, adjustments);
+
+    // Exclude the registry and domain from trivial subdomain stripping.
+    // To get the adjustment offset calculations correct, we need to transform
+    // the registry and domain portion of the host as well.
+    std::string domain_and_registry =
+        net::registry_controlled_domains::GetDomainAndRegistry(
+            component_text,
+            net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
+
+    base::OffsetAdjuster::Adjustments trivial_subdomains_adjustments;
+    base::StringTokenizer t(component_text.begin(),

[Peter Kasting, 2017/07/06 06:01:21]

Nit: t -> tokenizer (avoid abbreviation in general; see
http://google.github.io/styleguide/cppguide.html#General_Naming_Rules )

[tommycli, 2017/07/06 16:25:46]

Done.

+                            component_text.end() - domain_and_registry.length(),
+                            ".");
+    t.set_options(base::StringTokenizer::RETURN_DELIMS);
+
+    std::string new_subdomain_string;

[Peter Kasting, 2017/07/06 06:01:21]

Nit: |transformed_subdomain|?

[tommycli, 2017/07/06 16:25:46]

Done.

+    size_t offset = 0;
+    while (t.GetNext()) {
+      // Append delimiters and non-trivial subdomains to the new subdomain part.
+      if (t.token_is_delim() || (t.token() != "m" && t.token() != "www")) {
+        new_subdomain_string.append(t.token());

[Peter Kasting, 2017/07/06 06:01:21]

Nit: append() is fine, I find += a little more idiomatic myself

[tommycli, 2017/07/06 16:25:46]

Done. Ah i did not realize that string had operator+=. I thought it was the
equivalent of s = s + t;

+        offset += t.token().length();
+        continue;
+      }
+
+      // When we find a trivial subdomain, we simply do not append to
+      // |new_subdomain_string|. We also consume the next token, which must be
+      // a delimiter.
+      size_t trivial_subdomain_length = t.token().length();
+      bool next_delimiter_found = t.GetNext();
+      DCHECK(next_delimiter_found);
+      DCHECK(t.token_is_delim());
+
+      // Add an adjustment accounting for the consumed subdomain and delimiter.
+      trivial_subdomains_adjustments.push_back(base::OffsetAdjuster::Adjustment(
+          offset, trivial_subdomain_length + 1, 0));
+      offset += t.token().length() + 1;

[Peter Kasting, 2017/07/06 06:01:21]

Wait, is this right?  I think t.token() is pointing at the delimiter here, so
this always adds 2.  I think you wanted trivial_subdomain_length + 1 here,
didn't you?  But in that case I don't know how your adjustment unittest passes.

It feels like this is symptomatic of a design problem: |offset| itself seems
like it's just trying to track where in the original string you are.  But I
think you can read that off the tokenizer (via e.g. t.token_begin() -
component_text.begin(), or maybe using token_end(), depending on when you're
reading it and what you want).  So rather than iterating an offset and also
iterating through the string, you can just do one.

Or you could eliminate the tokenizer, keep the read offset, and use a simple
loop that calls find() starting from that offset.  Either way would probably
work.

[tommycli, 2017/07/06 16:25:46]

Done. Good suggestion on re-using the tokenizer iterator rather than keeping
track of "offset" separately. Much cleaner now.

The unit tests worked because the adjustment verification unit tests for trivial
subdomains only trimmed a single subdomain (which was correct). only subsequent
trimmings would be incorrect.

I beefed up the test. It now fails on the old code and passes on the new.

[Peter Kasting, 2017/07/06 16:31:47]

Yeah, the new version feels a lot cleaner to me now.  High five!

+    }
+
+    std::string new_component_text = new_subdomain_string + domain_and_registry;
+    base::string16 unicode_result =
+        IDNToUnicodeWithAdjustments(new_component_text, adjustments);
+
+    base::OffsetAdjuster::MergeSequentialAdjustments(
+        trivial_subdomains_adjustments, adjustments);
+    return unicode_result;
   }
+
+  bool trim_trivial_subdomains_;
 };
 
 class NonHostComponentTransform : public AppendComponentTransform {
  public:
   explicit NonHostComponentTransform(net::UnescapeRule::Type unescape_rules)
       : unescape_rules_(unescape_rules) {}
 
  private:
   base::string16 Execute(
       const std::string& component_text,
@@ skipping 284 matching lines @@
 
 const FormatUrlType kFormatUrlOmitNothing = 0;
 const FormatUrlType kFormatUrlOmitUsernamePassword = 1 << 0;
 const FormatUrlType kFormatUrlOmitHTTP = 1 << 1;
 const FormatUrlType kFormatUrlOmitTrailingSlashOnBareHostname = 1 << 2;
 const FormatUrlType kFormatUrlOmitAll =
     kFormatUrlOmitUsernamePassword | kFormatUrlOmitHTTP |
     kFormatUrlOmitTrailingSlashOnBareHostname;
 const FormatUrlType kFormatUrlExperimentalElideAfterHost = 1 << 3;
 const FormatUrlType kFormatUrlExperimentalOmitHTTPS = 1 << 4;
+const FormatUrlType kFormatUrlExperimentalOmitTrivialSubdomains = 1 << 5;
 
 base::string16 FormatUrl(const GURL& url,
                          FormatUrlTypes format_types,
                          net::UnescapeRule::Type unescape_rules,
                          url::Parsed* new_parsed,
                          size_t* prefix_end,
                          size_t* offset_for_adjustment) {
   std::vector<size_t> offsets;
   if (offset_for_adjustment)
     offsets.push_back(*offset_for_adjustment);
@@ skipping 99 matching lines @@
     AppendFormattedComponent(spec, parsed.password,
                              NonHostComponentTransform(unescape_rules),
                              &url_string, &new_parsed->password, adjustments);
     if (parsed.username.is_valid() || parsed.password.is_valid())
       url_string.push_back('@');
   }
   if (prefix_end)
     *prefix_end = static_cast<size_t>(url_string.length());
 
   // Host.
-  AppendFormattedComponent(spec, parsed.host, HostComponentTransform(),
+  bool trim_trivial_subdomains =
+      (format_types & kFormatUrlExperimentalOmitTrivialSubdomains) != 0;
+  AppendFormattedComponent(spec, parsed.host,
+                           HostComponentTransform(trim_trivial_subdomains),
                            &url_string, &new_parsed->host, adjustments);
 
   // Port.
   if (parsed.port.is_nonempty()) {
     url_string.push_back(':');
     new_parsed->port.begin = url_string.length();
     url_string.insert(url_string.end(), spec.begin() + parsed.port.begin,
                       spec.begin() + parsed.port.end());
     new_parsed->port.len = url_string.length() - new_parsed->port.begin;
   } else {
@@ skipping 92 matching lines @@
 bool CanStripTrailingSlash(const GURL& url) {
   // Omit the path only for standard, non-file URLs with nothing but "/" after
   // the hostname.
   return url.IsStandard() && !url.SchemeIsFile() && !url.SchemeIsFileSystem() &&
          !url.has_query() && !url.has_ref() && url.path_piece() == "/";
 }
 
 void AppendFormattedHost(const GURL& url, base::string16* output) {
   AppendFormattedComponent(
       url.possibly_invalid_spec(), url.parsed_for_possibly_invalid_spec().host,
-      HostComponentTransform(), output, NULL, NULL);
+      HostComponentTransform(false /* trim_trivial_subdomains */), output,

[Peter Kasting, 2017/07/06 06:01:21]

(I still dislike these kinda comments but whatever)

[tommycli, 2017/07/06 16:25:46]

Done.

+      nullptr, nullptr);
 }
 
 base::string16 IDNToUnicode(base::StringPiece host) {
   return IDNToUnicodeWithAdjustments(host, nullptr);
 }
 
 base::string16 StripWWW(const base::string16& text) {
   const base::string16 www(base::ASCIIToUTF16("www."));
   return base::StartsWith(text, www, base::CompareCase::SENSITIVE)
       ? text.substr(www.length()) : text;
 }
 
 base::string16 StripWWWFromHost(const GURL& url) {
   DCHECK(url.is_valid());
   return StripWWW(base::ASCIIToUTF16(url.host_piece()));
 }
 
 }  // namespace url_formatter