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;
   }
 
+  // Add a trailing slash if the path does not have one.

[Peter Kasting, 2015/06/11 23:05:37]

Nit: I might add "It's OK if this adds a slash even where one would be
inappropriate if we were really going to navigate to this URL, e.g.
"foo.com/index.html/"."

[Mark P, 2015/06/12 17:25:57]

Done.  (I thought it was clear from preexisting comments that we never use the
stripped_destination_url as a destination as it may often be non-navigable. 
Nonetheless, added this comment here because it cannot hurt.)

[Peter Kasting, 2015/06/12 17:49:04]

It's clear we're not going to navigate there -- but the other changes at least
look like plausible sorts of transforms to the URL, whereas appending a slash
sometimes makes no sense at all (except in terms of dupe elimination).

For this reason I'd actually prefer to always strip the slash than always add
one.  That's a more plausible-looking transform and I don't think would need an
extra comment.  The only reason I didn't suggest this is that you can't strip
the slash if it's the only thing in the path, and I was worried that the
inconsistency there might be confusing.

But if you think you could feasibly remove slashes (except for lone ones) as
easily as add them, I'd go with that.  It has the added bonus that you won't
have to allocate space for a new string and copy the existing path, too (yay
efficiency).

[Mark P, 2015/06/12 18:04:59]

I think the problem with lone slashes cannot be easily fixed.  I too wanted to
strip
the slash rather than add one, but thought of the lone slash issue.  I could fix
that issue by not stripping lone slashes, but I think the "path non-empty"
restriction only applies to certain types of URLs (http and the like), not all
kinds.
Indeed, that's why I asked on the bug whether you wanted to do this for all
protocols.  Given that you said yes, I thought adding the "/" was the safer,
better
thing to do.

[Peter Kasting, 2015/06/12 18:25:37]

Well, just because another protocol may allow an empty path doesn't mean that
having a slash in that case would be unsafe or break anything.

The logic could be something like this:

if (!path.empty() && path.back() == '/')
  strip slash;
if (path.empty())
  path = '/';

This ensures no paths have trailing slashes except the empty path, and also
ensures that "" and "/" both get transformed to "/".  (You could easily write a
single-conditional form of the logic that guaranteed the first condition, but
not the second.)

I think that would be fine for all protocols.

+  const std::string& path = stripped_destination_url.path();
+  // |path_with_extra_slash| lives at this scope rather than inside the if
+  // statement because it has to live until the ReplaceComponents() call below.
+  std::string path_with_extra_slash(path + '/');
+  if (path.empty() || (path.back() != '/')) {
+    replacements.SetPathStr(base::StringPiece(path_with_extra_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