Omnibox - Add About Flag to Reverse Title and URLs in the Dropdown

Omnibox - Add About Flag to Reverse Title and URLs in the Dropdown

(under some conditions)

Precisely, if the input is detected to be of type QUERY OR FORCED_QUERY,
then it's likely that any (or at least most) URL suggestions we have
have matches in the title.  In this case, it's better to display the
titles more prominently than the URLs in the omnibox dropdown.

I've found this policy of flipping based entirely on the input type (not
where the matches are) makes the omnibox look more consistent than a
per-suggestion decision.  Also, because inputs that are detected to
be of type QUERY or FORCED_QUERY tend to keep their type classification
as the user types, this also helps maintain a consistent presentation
look in the omnibox.

This is submitted as an about flag so other people can turn it on and
experience this behavior.  It's for getting feedback and iterating.

BUG=338066
Committed: https://crrev.com/2a18bc7516b082cdd5d8a59b4f38d93cf76c27ac
Cr-Commit-Position: refs/heads/master@{#331260}

[Mark P, 2015-05-21 21:34:20]

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

[Mark P, 2015-05-21 21:34:20]

Hi Peter,

Take a look at your leisure.

thanks,
mark

[Peter Kasting, 2015-05-21 21:55:34]

LGTM with one substantive refactor suggestion

https://codereview.chromium.org/1155673002/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1155673002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7470: +        Emphasize the titles of URLs
in the omnibox dropdown
Nit: "the titles of URLs" -> "titles"

https://codereview.chromium.org/1155673002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7473: +        Causes the omnibox to sometime
display the titles of URL suggestions that appear in the dropdown more
prominently than the URL itself.  Does not change the order of any suggestions.
Nit: How about: "Causes the omnibox dropdown to sometimes display suggestion
titles ahead of URLs."  (I don't think the second sentence is needed.)

https://codereview.chromium.org/1155673002/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/bookmark_provider.cc (right):

https://codereview.chromium.org/1155673002/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/bookmark_provider.cc:227:
match.PossiblySwapContentsAndDescriptionForURLSuggestion(input);
Rather than adding these calls to each provider, can't we swap in one place (the
controller?) based on the match type?

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

https://codereview.chromium.org/1155673002/diff/20001/components/omnibox/auto...
components/omnibox/autocomplete_match.cc:535: // we're supposed to (due to
about:flags).
Nit: I think this comment just restates the (fairly clear) code, so I'd omit it.

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

https://codereview.chromium.org/1155673002/diff/20001/components/omnibox/auto...
components/omnibox/autocomplete_match.h:266: // (i.e., the URL itself) under
some conditions.
Nit: How about:

Swaps the contents and description fields, and their associated classifications,
if this is a match for which we should emphasize the title (in the description
field) over the URL (in the contents field).  See the implementation for the
conditions under which this is true.

https://codereview.chromium.org/1155673002/diff/20001/components/omnibox/omni...
File components/omnibox/omnibox_switches.cc (right):

https://codereview.chromium.org/1155673002/diff/20001/components/omnibox/omni...
components/omnibox/omnibox_switches.cc:15: // Causes the omnibox's dropdown to
emphasize the titles of URL suggestions
Nit: omnibox's -> omnibox

https://codereview.chromium.org/1155673002/diff/20001/components/omnibox/omni...
components/omnibox/omnibox_switches.cc:16: // rather than the URL.
Nit: "rather than the URL" -> "for query-like inputs"

[Mark P, 2015-05-21 23:47:36]

https://codereview.chromium.org/1155673002/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/bookmark_provider.cc (right):

https://codereview.chromium.org/1155673002/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/bookmark_provider.cc:227:
match.PossiblySwapContentsAndDescriptionForURLSuggestion(input);
On 2015/05/21 21:55:33, Peter Kasting wrote:
> Rather than adding these calls to each provider, can't we swap in one place
(the
> controller?) based on the match type?

I thought about it quite a bit.  If we wanted to swap them in one place, the
best place to do the swap is in AutocompleteResult.  There's only one code path
there that incorporates matches from the providers (AppendMatches()).  However,
I found such a solution confusing.  This meant that in my mental model, I'd have
to remember that certain matches look different when in the combined flow
(AutocompleteController, AutocompleteResult) than returned by the providers. 
Having this strange situation--where the same data type should get interpreted
somewhat differently depending on where in the code it's been examined--I think
is too much of a design mess.  I prefer this solution, where each provider is
responsible for swapping (if appropriate) or not.

(This follows the same principle that you had me do when marking matches as
allowed to be the default match.  I wanted to do this in a central place but you
argued that it's best done within each provider.)

[Peter Kasting, 2015-05-22 00:52:42]

https://codereview.chromium.org/1155673002/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/bookmark_provider.cc (right):

https://codereview.chromium.org/1155673002/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/bookmark_provider.cc:227:
match.PossiblySwapContentsAndDescriptionForURLSuggestion(input);
On 2015/05/21 23:47:36, Mark P wrote:
> On 2015/05/21 21:55:33, Peter Kasting wrote:
> > Rather than adding these calls to each provider, can't we swap in one place
> (the
> > controller?) based on the match type?
> 
> I thought about it quite a bit.  If we wanted to swap them in one place, the
> best place to do the swap is in AutocompleteResult.  There's only one code
path
> there that incorporates matches from the providers (AppendMatches()). 
However,
> I found such a solution confusing.  This meant that in my mental model, I'd
have
> to remember that certain matches look different when in the combined flow
> (AutocompleteController, AutocompleteResult) than returned by the providers. 
> Having this strange situation--where the same data type should get interpreted
> somewhat differently depending on where in the code it's been examined--I
think
> is too much of a design mess.  I prefer this solution, where each provider is
> responsible for swapping (if appropriate) or not.

Huh.  Seems a lot simpler and clearer to me the other way.  But if you hate it,
then what about replacing the "contents" and "description" fields with some kind
of explicit "URL" field and a catchall "description/other text" sort of field? 
That might make the struct contents more understandable, and then we could do
the switch in the display code instead.  (Of course, that would mean we'd need a
per-display-implementation version of the solution.)

> (This follows the same principle that you had me do when marking matches as
> allowed to be the default match.  I wanted to do this in a central place but
you
> argued that it's best done within each provider.)

If we can mark matches as "allowed to be default" in a centralized way that's
just as readable and accurate as today's code, we should do that.  My original
argument was based on the idea that we couldn't really know this perfectly in a
central way, or at least not without writing something ugly and
incomprehensible.  If I was wrong then or now, let's change it.  Simpler code >
*

[Mark P, 2015-05-22 22:51:29]

Refactored per your suggestion.  Let me know what you think.  If you're good
with it, I'll have revise a few unit tests to reflect the API change.

--mark

https://codereview.chromium.org/1155673002/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1155673002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7470: +        Emphasize the titles of URLs
in the omnibox dropdown
On 2015/05/21 21:55:33, Peter Kasting wrote:
> Nit: "the titles of URLs" -> "titles"

Done.

https://codereview.chromium.org/1155673002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7473: +        Causes the omnibox to sometime
display the titles of URL suggestions that appear in the dropdown more
prominently than the URL itself.  Does not change the order of any suggestions.
On 2015/05/21 21:55:33, Peter Kasting wrote:
> Nit: How about: "Causes the omnibox dropdown to sometimes display suggestion
> titles ahead of URLs."  (I don't think the second sentence is needed.)

Removed second sentence.

Used your sentence but modified it slightly because "ahead" sounds like an
ordering change.  Used the word prominently.

https://codereview.chromium.org/1155673002/diff/20001/chrome/browser/autocomp...
File chrome/browser/autocomplete/bookmark_provider.cc (right):

https://codereview.chromium.org/1155673002/diff/20001/chrome/browser/autocomp...
chrome/browser/autocomplete/bookmark_provider.cc:227:
match.PossiblySwapContentsAndDescriptionForURLSuggestion(input);
On 2015/05/22 00:52:42, Peter Kasting wrote:
> On 2015/05/21 23:47:36, Mark P wrote:
> > On 2015/05/21 21:55:33, Peter Kasting wrote:
> > > Rather than adding these calls to each provider, can't we swap in one
place
> > (the
> > > controller?) based on the match type?
> > 
> > I thought about it quite a bit.  If we wanted to swap them in one place, the
> > best place to do the swap is in AutocompleteResult.  There's only one code
> path
> > there that incorporates matches from the providers (AppendMatches()). 
> However,
> > I found such a solution confusing.  This meant that in my mental model, I'd
> have
> > to remember that certain matches look different when in the combined flow
> > (AutocompleteController, AutocompleteResult) than returned by the providers.

> > Having this strange situation--where the same data type should get
interpreted
> > somewhat differently depending on where in the code it's been examined--I
> think
> > is too much of a design mess.  I prefer this solution, where each provider
is
> > responsible for swapping (if appropriate) or not.
> 
> Huh.  Seems a lot simpler and clearer to me the other way.

I tried the other way.  It looks less messy than I thought.  It helps that I can
put the code in one place and write a clean, rational comment for it.  Let me
know what you think.

> But if you hate it,
> then what about replacing the "contents" and "description" fields with some
kind
> of explicit "URL" field and a catchall "description/other text" sort of field?

> That might make the struct contents more understandable, and then we could do
> the switch in the display code instead.  (Of course, that would mean we'd need
a
> per-display-implementation version of the solution.)
> 
> > (This follows the same principle that you had me do when marking matches as
> > allowed to be the default match.  I wanted to do this in a central place but
> you
> > argued that it's best done within each provider.)
> 
> If we can mark matches as "allowed to be default" in a centralized way that's
> just as readable and accurate as today's code, we should do that.  My original
> argument was based on the idea that we couldn't really know this perfectly in
a
> central way, or at least not without writing something ugly and
> incomprehensible.  If I was wrong then or now, let's change it.  Simpler code
>
> *

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

https://codereview.chromium.org/1155673002/diff/20001/components/omnibox/auto...
components/omnibox/autocomplete_match.cc:535: // we're supposed to (due to
about:flags).
On 2015/05/21 21:55:33, Peter Kasting wrote:
> Nit: I think this comment just restates the (fairly clear) code, so I'd omit
it.

Okay, removed.  I only added the comment because I thought one item in this if
clause was not obvious: the fact that we need to check that description is not
empty.

If you think need is obvious (it wasn't to me; I originally wrote the code
without that test), then we're good.  If not, can you suggest a comment?

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

https://codereview.chromium.org/1155673002/diff/20001/components/omnibox/auto...
components/omnibox/autocomplete_match.h:266: // (i.e., the URL itself) under
some conditions.
On 2015/05/21 21:55:33, Peter Kasting wrote:
> Nit: How about:
> 
> Swaps the contents and description fields, and their associated
classifications,
> if this is a match for which we should emphasize the title (in the description
> field) over the URL (in the contents field).  See the implementation for the
> conditions under which this is true.

Sure, that sounds cleaner.

https://codereview.chromium.org/1155673002/diff/20001/components/omnibox/omni...
File components/omnibox/omnibox_switches.cc (right):

https://codereview.chromium.org/1155673002/diff/20001/components/omnibox/omni...
components/omnibox/omnibox_switches.cc:15: // Causes the omnibox's dropdown to
emphasize the titles of URL suggestions
On 2015/05/21 21:55:33, Peter Kasting wrote:
> Nit: omnibox's -> omnibox

Done.

https://codereview.chromium.org/1155673002/diff/20001/components/omnibox/omni...
components/omnibox/omnibox_switches.cc:16: // rather than the URL.
On 2015/05/21 21:55:33, Peter Kasting wrote:
> Nit: "rather than the URL" -> "for query-like inputs"

Okay.  Done.

[Peter Kasting, 2015-05-22 23:39:37]

LGTM

https://codereview.chromium.org/1155673002/diff/40001/components/omnibox/auto...
File components/omnibox/autocomplete_result.cc (right):

https://codereview.chromium.org/1155673002/diff/40001/components/omnibox/auto...
components/omnibox/autocomplete_result.cc:166: for (ACMatches::const_iterator
i(matches.begin()); i != matches.end(); ++i) {
Nit: Range-based for?

https://codereview.chromium.org/1155673002/diff/40001/components/omnibox/auto...
File components/omnibox/autocomplete_result.h (right):

https://codereview.chromium.org/1155673002/diff/40001/components/omnibox/auto...
components/omnibox/autocomplete_result.h:75: // change depending on |input|.
Nit: These last two sentences are vague.  I'd just say something like "Calls
PossiblySwapContentsAndDescriptionForURLSuggestion(input)" on all added matches;
see comments there for more information."

[Mark P, 2015-05-23 20:32:13]

I also fixed the tests.  (The test changes were purely mechanical.)

--mark

https://codereview.chromium.org/1155673002/diff/40001/components/omnibox/auto...
File components/omnibox/autocomplete_result.cc (right):

https://codereview.chromium.org/1155673002/diff/40001/components/omnibox/auto...
components/omnibox/autocomplete_result.cc:166: for (ACMatches::const_iterator
i(matches.begin()); i != matches.end(); ++i) {
On 2015/05/22 23:39:36, Peter Kasting wrote:
> Nit: Range-based for?

Okay.  Done.

https://codereview.chromium.org/1155673002/diff/40001/components/omnibox/auto...
File components/omnibox/autocomplete_result.h (right):

https://codereview.chromium.org/1155673002/diff/40001/components/omnibox/auto...
components/omnibox/autocomplete_result.h:75: // change depending on |input|.
On 2015/05/22 23:39:36, Peter Kasting wrote:
> Nit: These last two sentences are vague.  I'd just say something like "Calls
> PossiblySwapContentsAndDescriptionForURLSuggestion(input)" on all added
matches;
> see comments there for more information."

I slightly prefer the vagueness because I want to make the larger point that
matches in AutocompleteResult can be transformed in arbitrary ways.  Yet, I see
your point that it's not necessary to make the larger point when the smaller,
more-exact point will do for now.  Revised as you suggested.

[Mark P, 2015-05-25 04:00:20]

The CQ bit was checked by mpearson@chromium.org

[Mark P, 2015-05-25 04:00:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1155673002/#ps80001
(title: "fix mac and add command line flag to histograms.xml")

[commit-bot: I haz the power, 2015-05-25 04:00:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1155673002/80001

[commit-bot: I haz the power, 2015-05-25 05:37:19]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-25 05:38:50]

Patchset 5 (id:??) landed as
https://crrev.com/2a18bc7516b082cdd5d8a59b4f38d93cf76c27ac
Cr-Commit-Position: refs/heads/master@{#331260}