More fine grained understanding of Omnibox navigation counts.

More fine grained understanding of Omnibox navigation counts.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=117318

[Peter Kasting, 2012-01-09 20:47:37]

http://codereview.chromium.org/9153004/diff/1/chrome/browser/autocomplete/net...
File chrome/browser/autocomplete/network_action_predictor.cc (right):

http://codereview.chromium.org/9153004/diff/1/chrome/browser/autocomplete/net...
chrome/browser/autocomplete/network_action_predictor.cc:60: bool
IsSearchType(const AutocompleteMatch::Type& type) {
Nit: I wonder if this is consistent with other places we make this
determination.  For example, when we decide whether to show "paste and search"
versus "paste and go", we look at the match transition type and check whether
it's TYPED.

http://codereview.chromium.org/9153004/diff/1/chrome/browser/autocomplete/net...
chrome/browser/autocomplete/network_action_predictor.cc:66: // A match that uses
a non-default search engine (e.g. for tab-to-search).
Nit: Unindent 2.

[dominich, 2012-01-09 22:43:43]

http://codereview.chromium.org/9153004/diff/1/chrome/browser/autocomplete/net...
File chrome/browser/autocomplete/network_action_predictor.cc (right):

http://codereview.chromium.org/9153004/diff/1/chrome/browser/autocomplete/net...
chrome/browser/autocomplete/network_action_predictor.cc:60: bool
IsSearchType(const AutocompleteMatch::Type& type) {
On 2012/01/09 20:47:37, Peter Kasting wrote:
> Nit: I wonder if this is consistent with other places we make this
> determination.  For example, when we decide whether to show "paste and search"
> versus "paste and go", we look at the match transition type and check whether
> it's TYPED.

Hmm. On the other side of this, when we swap in the prerendered page, we set the
transition type to TYPED, so that might make sense. I do want to be explicit
about not prerendering search recommendations though, and this check definitely
does that. The check for transition type would be harder to explain and my sense
is that it's a more dangerous check to make. Ie, if there's a search suggestion
that's being set to typed, that would lead to a subtle bug here.

http://codereview.chromium.org/9153004/diff/1/chrome/browser/autocomplete/net...
chrome/browser/autocomplete/network_action_predictor.cc:66: // A match that uses
a non-default search engine (e.g. for tab-to-search).
On 2012/01/09 20:47:37, Peter Kasting wrote:
> Nit: Unindent 2.

Done.

[Peter Kasting, 2012-01-09 22:51:30]

http://codereview.chromium.org/9153004/diff/1/chrome/browser/autocomplete/net...
File chrome/browser/autocomplete/network_action_predictor.cc (right):

http://codereview.chromium.org/9153004/diff/1/chrome/browser/autocomplete/net...
chrome/browser/autocomplete/network_action_predictor.cc:60: bool
IsSearchType(const AutocompleteMatch::Type& type) {
On 2012/01/09 22:43:43, dominich wrote:
> The check for transition type would be harder to explain

In the abstract, yes; the fact that we have many other places that do this
already, though, makes it more sane in context.  (Well, kinda.)

> my sense
> is that it's a more dangerous check to make. Ie, if there's a search
suggestion
> that's being set to typed, that would lead to a subtle bug here.

Sure, but if there's a search recommendation being set to HISTORY_WHAT_YOU_TYPED
that'd be wrong too.

The critical bit here is that TYPED is only supposed to be used for when the
user navigates to an actual URL they typed in directly.  If their typed string
didn't look like their navigation string, we're to use GENERATED.  The
HistoryURLProvider relies on this in order to autocomplete correctly so we have
strong incentive to get it right.

The search cases are the ones in which the destination URL doesn't look like the
fill_into_edit, so they match up neatly with "search" versus "navigate".

On interesting case to examine is NavSuggest results.  These actually have their
own type, NAVSUGGEST, but one could have argued they should be SEARCH_SUGGEST. 
However, because they look like the actual URL, their transition would be TYPED
and not GENERATED.  So in that world, the transition type would be more reliable
than the match type.

That said, I think the practical effect of these two routes, today, should
always be the same.  My argument is mainly one of consistency with the rest of
the omnibox code.  Perhaps we should achieve this by adding a helper function on
AutocompleteMatch that basically returns "search" or "navigation" and then
picking a method to implement it.

[dominich, 2012-01-09 23:10:16]

On 2012/01/09 22:51:30, Peter Kasting wrote:
>
http://codereview.chromium.org/9153004/diff/1/chrome/browser/autocomplete/net...
> File chrome/browser/autocomplete/network_action_predictor.cc (right):
> 
>
http://codereview.chromium.org/9153004/diff/1/chrome/browser/autocomplete/net...
> chrome/browser/autocomplete/network_action_predictor.cc:60: bool
> IsSearchType(const AutocompleteMatch::Type& type) {
> On 2012/01/09 22:43:43, dominich wrote:
> > The check for transition type would be harder to explain
> 
> In the abstract, yes; the fact that we have many other places that do this
> already, though, makes it more sane in context.  (Well, kinda.)
> 
> > my sense
> > is that it's a more dangerous check to make. Ie, if there's a search
> suggestion
> > that's being set to typed, that would lead to a subtle bug here.
> 
> Sure, but if there's a search recommendation being set to
HISTORY_WHAT_YOU_TYPED
> that'd be wrong too.
> 
> The critical bit here is that TYPED is only supposed to be used for when the
> user navigates to an actual URL they typed in directly.  If their typed string
> didn't look like their navigation string, we're to use GENERATED.  The
> HistoryURLProvider relies on this in order to autocomplete correctly so we
have
> strong incentive to get it right.

I think that for prerendering I actually want to know if the returned match is
from a search engine or not, more than I need it to match what the user typed.
If the suggestion was generated by some hypothetical system but they user
follows that suggestion after typing in the same text a few times, I still want
to prerender it. If, however, the suggestion leads to a search results page I
never want to prerender it.

So for this use case, checking for Search vs Other Things is the correct thing
to do.

> 
> The search cases are the ones in which the destination URL doesn't look like
the
> fill_into_edit, so they match up neatly with "search" versus "navigate".
> 
> On interesting case to examine is NavSuggest results.  These actually have
their
> own type, NAVSUGGEST, but one could have argued they should be SEARCH_SUGGEST.

> However, because they look like the actual URL, their transition would be
TYPED
> and not GENERATED.  So in that world, the transition type would be more
reliable
> than the match type.
> 
> That said, I think the practical effect of these two routes, today, should
> always be the same.  My argument is mainly one of consistency with the rest of
> the omnibox code.  Perhaps we should achieve this by adding a helper function
on
> AutocompleteMatch that basically returns "search" or "navigation" and then
> picking a method to implement it.

I agree with any argument from consistency, and if the effect of the two routes
is identical and should remain identical, then I think finding a way to simplify
the API is a worthy step to take. However, I think there is a difference between
wanting to know if the suggestion matches what was typed vs its source.

[Peter Kasting, 2012-01-11 18:39:09]

Did you mean to ping on this rather than close it?  I thought this got
obsoleted...

[dominich, 2012-01-11 19:32:58]

Not completely deprecated. There are still new histograms that I want to track,
the search stuff is deprecated.

[Peter Kasting, 2012-01-11 19:34:01]

LGTM

[commit-bot: I haz the power, 2012-01-11 20:16:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominich@chromium.org/9153004/12002

[commit-bot: I haz the power, 2012-01-11 23:10:33]

Change committed as 117318