Omnibox - Mark As Duplicates URLs that only differ by a trailing slash

components/omnibox/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/autocomplete_match.h"
 
 #include "base/i18n/time_formatting.h"
 #include "base/logging.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
@@ skipping 397 matching lines @@
 
   // Remove the www. prefix from the host.
   static const char prefix[] = "www.";
   static const size_t prefix_len = arraysize(prefix) - 1;
   std::string host = stripped_destination_url.host();
   if (host.compare(0, prefix_len, prefix) == 0) {
     replacements.SetHostStr(base::StringPiece(host).substr(prefix_len));
     needs_replacement = true;
   }
 
+  // Remove the trailing slash if any (if it's not a lone slash), and

[Peter Kasting, 2015/06/12 19:30:36]

Nit: "Remove any trailing slash ..."; and -> or

[Mark P, 2015/06/12 19:57:57]

Done.  (removed "if any", replaced "the" with "any")
Done.

+  // add a slash (to make a lone slash) if the path is empty.

[Peter Kasting, 2015/06/12 19:30:36]

Nit: Maybe add: "(We can't unconditionally remove even lone slashes because for
some schemes the path must consist of at least a slash.)"

[Mark P, 2015/06/12 19:57:57]

Okay. Done.

+  const std::string& path = stripped_destination_url.path();
+  // |slash| lives at this scope rather than inside the if
+  // statement because it has to live until the ReplaceComponents() call below.
+  std::string slash("/");

[Peter Kasting, 2015/06/12 19:30:36]

Can we do:

  static const char slash[] = "/";

?  Then you could put it inside the conditional if you wanted and I think it
would be more efficient to boot (maybe)

[Mark P, 2015/06/12 19:57:57]

Apparently we can.  Done.

+  if ((path.length() > 1) && (path[path.length() - 1] == '/')) {
+    replacements.SetPathStr(
+        base::StringPiece(path).substr(0, path.length() - 1));
+    needs_replacement = true;
+  } else if (path.empty()) {
+    replacements.SetPathStr(base::StringPiece(slash));
+    needs_replacement = true;
+  }
+
   // Replace https protocol with http protocol.
   if (stripped_destination_url.SchemeIs(url::kHttpsScheme)) {
     replacements.SetScheme(url::kHttpScheme,
                            url::Component(0, strlen(url::kHttpScheme)));
     needs_replacement = true;
   }
 
   if (needs_replacement)
     stripped_destination_url = stripped_destination_url.ReplaceComponents(
         replacements);
@@ skipping 129 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