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

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

Make the stripped URL always end the path component in a trailing slash.

previous attempt:
https://chromium.googlesource.com/chromium/src.git/+/86d6ea9832f60721adc36a3372b51016947dfcb2

Plus (this is what will make this attempt work),
have each provider strip "/" in inline autocompletion aggressively

BUG=490084, 501048

[Mark P, 2015-06-30 23:39:48]

mpearson@chromium.org changed reviewers:
+ pkasting@chromium.org

[Mark P, 2015-06-30 23:47:30]

Hey,

Please take a look at this proposed solution and let me know what you think.

thanks,
mark

https://codereview.chromium.org/1215233003/diff/20001/components/omnibox/auto...
File components/omnibox/autocomplete_match.cc (right):

https://codereview.chromium.org/1215233003/diff/20001/components/omnibox/auto...
components/omnibox/autocomplete_match.cc:570: if
((inline_autocompletion.length() == 1) &&
I know you may argue that we should simply remove a trailing slash, not merely
lone slashes.  However, I sometimes find the trailing slash suggestions useful;
with them, I can type "www", see www.host.com", press the right arrow to accept
the text, type "/f", see the completion to www.host.com/foo/, press the right
arrow to accept the text, type "b", see the completion to www.host.com/foo/bar,
and press enter.  If we stripped trailing slashes, I'd have to type "/b" instead
of "b".  You may argue for consistency that we should remove the trailing
slashes everywhere, but I think I tend to believe this efficiency argument is
slightly stronger, and believe no one will complain that as they type they see
the trailing slash until they type the last character before the slash.

https://codereview.chromium.org/1215233003/diff/20001/components/omnibox/auto...
File components/omnibox/autocomplete_match.h (right):

https://codereview.chromium.org/1215233003/diff/20001/components/omnibox/auto...
components/omnibox/autocomplete_match.h:287: void
StripLoneSlashOnInlineAutocompletion();
I know this function is trivial and could be inlined in the places that call it.
 However, I want a common place for a comment arguing why this is the right
thing to do.  This is a decent place for it.

https://codereview.chromium.org/1215233003/diff/20001/components/omnibox/auto...
File components/omnibox/autocomplete_match_unittest.cc (right):

https://codereview.chromium.org/1215233003/diff/20001/components/omnibox/auto...
components/omnibox/autocomplete_match_unittest.cc:131:
TEST(AutocompleteMatchTest, Duplicates) {
I will have to revise this after rebasing.

[Peter Kasting, 2015-07-01 22:01:36]

https://codereview.chromium.org/1215233003/diff/20001/components/omnibox/auto...
File components/omnibox/autocomplete_match.cc (right):

https://codereview.chromium.org/1215233003/diff/20001/components/omnibox/auto...
components/omnibox/autocomplete_match.cc:569: static const base::char16
slash_utf8 = 0x002F;
Why declare a variable for this?  And use 0x002f instead of '/', which is
equivalent?

It seems more readable to just use '/' directly in the conditional below, and I
don't think there should be a problem with char16 vs. char type mismatches,
we'll just auto-promote the char.

https://codereview.chromium.org/1215233003/diff/20001/components/omnibox/auto...
components/omnibox/autocomplete_match.cc:571: (inline_autocompletion[0] ==
slash_utf8)) {
Nit: No {}

https://codereview.chromium.org/1215233003/diff/20001/components/omnibox/auto...
File components/omnibox/autocomplete_match.h (right):

https://codereview.chromium.org/1215233003/diff/20001/components/omnibox/auto...
components/omnibox/autocomplete_match.h:281: // of a slash isn't very useful in
this context and can cause issues if the
I would stop after "isn't very useful in this context".  I think that's a
sufficiently good argument for this, and the further arguments you make apply
equally to any inline autocompletion, not just to trailing slashes, so they just
raise the question for the reader of what's different about this case.

https://codereview.chromium.org/1215233003/diff/20001/components/omnibox/book...
File components/omnibox/bookmark_provider.cc (right):

https://codereview.chromium.org/1215233003/diff/20001/components/omnibox/book...
components/omnibox/bookmark_provider.cc:211: if (inline_autocomplete_offset <
match.fill_into_edit.length()) {
Nit: Other than the builtin provider, every other place you touch does something
equivalent to this (sometimes stricter by DCHECKing that non-npos offsets are
always < fill_into_edit.length()).

Perhaps it makes sense to create some sort of helper on AutocompleteMatch like
SetURLAutocompletion(offset) that is basically this conditional block, then just
call that everywhere.  We'd lose the strictness of those DCHECKs, which seems
OK.

https://codereview.chromium.org/1215233003/diff/20001/components/omnibox/hist...
File components/omnibox/history_url_provider.cc (left):

https://codereview.chromium.org/1215233003/diff/20001/components/omnibox/hist...
components/omnibox/history_url_provider.cc:1179: if
(!params.prevent_inline_autocomplete &&
What makes this safe to remove?

[Peter Kasting, 2016-04-29 02:10:12]

Mark, what's the story here?

[Mark P, 2016-04-29 04:05:07]

On 2016/04/29 02:10:12, Peter Kasting wrote:
> Mark, what's the story here?

I do not plan to revive this.  I don't think it'll do much; other things on my
plate are more important.

--mark

[Peter Kasting, 2016-04-29 05:01:57]

On 2016/04/29 04:05:07, Mark P wrote:
> On 2016/04/29 02:10:12, Peter Kasting wrote:
> > Mark, what's the story here?
> 
> I do not plan to revive this.  I don't think it'll do much; other things on my
> plate are more important.

Closing.