autocomplete: Make ContactProvider rank contacts by affinity

autocomplete: Make ContactProvider rank contacts by affinity

This adds an 'affinity' field to the contacts::Contact
protocol message and makes the ContactProvider class use it
to order matches.

BUG=141877
TEST=none


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

[Daniel Erat, 2012-09-13 23:10:08]

https://codereview.chromium.org/10916304/diff/1/chrome/browser/autocomplete/c...
File chrome/browser/autocomplete/contact_provider_chromeos.cc (right):

https://codereview.chromium.org/10916304/diff/1/chrome/browser/autocomplete/c...
chrome/browser/autocomplete/contact_provider_chromeos.cc:65: const int
ContactProvider::kBaseRelevance = 1300;
I just pulled numbers out of the air for this.  Since this is off by default and
we have no plans to expose these results anywhere outside the app list (which
itself will soon only return apps rather than giving the full omnibox
experience), I'm not sure how much these numbers matter.

[Peter Kasting, 2012-09-13 23:32:37]

LGTM

https://codereview.chromium.org/10916304/diff/1/chrome/browser/autocomplete/c...
File chrome/browser/autocomplete/contact_provider_chromeos.cc (right):

https://codereview.chromium.org/10916304/diff/1/chrome/browser/autocomplete/c...
chrome/browser/autocomplete/contact_provider_chromeos.cc:23: float
kDefaultAffinity = 1.0;
Isn't this kind of high?  I guess I don't totally understand how affinity is
calculated.

https://codereview.chromium.org/10916304/diff/1/chrome/browser/autocomplete/c...
chrome/browser/autocomplete/contact_provider_chromeos.cc:56: float affinity_a =
a->has_affinity() ? a->affinity() : kDefaultAffinity;
Super tiny nit: Seems like |a_affinity| would be just a teensy bit better than
|affinity_a|

https://codereview.chromium.org/10916304/diff/1/chrome/browser/autocomplete/c...
chrome/browser/autocomplete/contact_provider_chromeos.cc:164:
std::sort(contacts->begin(), contacts->end(), CompareContactAffinity);
Nit: If we move this down to the bottom of the function, we can get rid of the
need to check for missing affinity from CompareContactAffinity(), because the
code below will have taken care of that

https://codereview.chromium.org/10916304/diff/1/chrome/browser/autocomplete/c...
File chrome/browser/autocomplete/contact_provider_chromeos_unittest.cc (right):

https://codereview.chromium.org/10916304/diff/1/chrome/browser/autocomplete/c...
chrome/browser/autocomplete/contact_provider_chromeos_unittest.cc:81:
std::map<std::string, std::string>::const_iterator it =
Nit: Use AutocompleteMatch::AdditionalInfo::const_iterator

https://codereview.chromium.org/10916304/diff/1/chrome/browser/autocomplete/c...
chrome/browser/autocomplete/contact_provider_chromeos_unittest.cc:94:
CHECK(contact) << "Unable to find contact " << i;
Nit: Avoid [D]CHECK in tests, use ASSERT (2 places)

https://codereview.chromium.org/10916304/diff/1/chrome/browser/autocomplete/c...
chrome/browser/autocomplete/contact_provider_chromeos_unittest.cc:273:
ContactProvider::kAffinityRelevanceBoost * exp_contact.affinity())),
Nit: I try to get people to avoid checking the exact relevance in the unit test,
as it means you have to update the test every time you tweak the ranking
function; it's better to just check that the relevance is lower than the
previous match's relevance.

[Daniel Erat, 2012-09-14 15:31:50]

http://codereview.chromium.org/10916304/diff/1/chrome/browser/autocomplete/co...
File chrome/browser/autocomplete/contact_provider_chromeos.cc (right):

http://codereview.chromium.org/10916304/diff/1/chrome/browser/autocomplete/co...
chrome/browser/autocomplete/contact_provider_chromeos.cc:23: float
kDefaultAffinity = 1.0;
On 2012/09/13 23:32:37, Peter Kasting wrote:
> Isn't this kind of high?  I guess I don't totally understand how affinity is
> calculated.

Dirty secret: We're not getting affinity data yet, so I set this high so we'll
see contacts in the app list results when the flag is set.  Added a TODO.

http://codereview.chromium.org/10916304/diff/1/chrome/browser/autocomplete/co...
chrome/browser/autocomplete/contact_provider_chromeos.cc:56: float affinity_a =
a->has_affinity() ? a->affinity() : kDefaultAffinity;
On 2012/09/13 23:32:37, Peter Kasting wrote:
> Super tiny nit: Seems like |a_affinity| would be just a teensy bit better than
> |affinity_a|

Done.

http://codereview.chromium.org/10916304/diff/1/chrome/browser/autocomplete/co...
chrome/browser/autocomplete/contact_provider_chromeos.cc:164:
std::sort(contacts->begin(), contacts->end(), CompareContactAffinity);
On 2012/09/13 23:32:37, Peter Kasting wrote:
> Nit: If we move this down to the bottom of the function, we can get rid of the
> need to check for missing affinity from CompareContactAffinity(), because the
> code below will have taken care of that

I went back and forth on this.  Sorting pointers seemed better than sorting
structs, but that's a pretty specious argument for me to take (if it were really
a big deal performance-wise, I shouldn't be storing the structs directly in a
vector, or I should at least be calling reserve() first).  Updated.

http://codereview.chromium.org/10916304/diff/1/chrome/browser/autocomplete/co...
File chrome/browser/autocomplete/contact_provider_chromeos_unittest.cc (right):

http://codereview.chromium.org/10916304/diff/1/chrome/browser/autocomplete/co...
chrome/browser/autocomplete/contact_provider_chromeos_unittest.cc:81:
std::map<std::string, std::string>::const_iterator it =
On 2012/09/13 23:32:37, Peter Kasting wrote:
> Nit: Use AutocompleteMatch::AdditionalInfo::const_iterator

Done.

http://codereview.chromium.org/10916304/diff/1/chrome/browser/autocomplete/co...
chrome/browser/autocomplete/contact_provider_chromeos_unittest.cc:94:
CHECK(contact) << "Unable to find contact " << i;
On 2012/09/13 23:32:37, Peter Kasting wrote:
> Nit: Avoid [D]CHECK in tests, use ASSERT (2 places)

I think that the testing macros won't work here.  Added a LOG(ERROR) and made us
just skip the contact instead (which should still result in a test failure).

../../chrome/browser/autocomplete/contact_provider_chromeos_unittest.cc: In
member function 'contacts::ContactPointers
ContactProviderTest::GetMatchedContacts()':
../../chrome/browser/autocomplete/contact_provider_chromeos_unittest.cc:94:60:
error: could not convert
'testing::internal::AssertHelper((testing::TestPartResult::Type)2u, ((const
char*)"../../chrome/browser/autocomplete/contact_provider_chromeos_unittest.cc"),
94, testing::internal::GetBoolAssertionFailureMessage(const
testing::AssertionResult&, const char*, const char*, const char*)(((const
char*)"contact"), ((const char*)"false"), ((const
char*)"true")).testing::internal::String::c_str()).testing::internal::AssertHelper::operator=((*(const
testing::Message*)(& testing::Message().testing::Message::operator<< [with T =
char [24], testing::Message = testing::Message]((*"Unable to find contact
")))->testing::Message::operator<< [with T = long unsigned int, testing::Message
= testing::Message]((*(const long unsigned int*)(& i)))))' from 'void' to
'contacts::ContactPointers {aka std::vector<const contacts::Contact*>}'

http://codereview.chromium.org/10916304/diff/1/chrome/browser/autocomplete/co...
chrome/browser/autocomplete/contact_provider_chromeos_unittest.cc:273:
ContactProvider::kAffinityRelevanceBoost * exp_contact.affinity())),
On 2012/09/13 23:32:37, Peter Kasting wrote:
> Nit: I try to get people to avoid checking the exact relevance in the unit
test,
> as it means you have to update the test every time you tweak the ranking
> function; it's better to just check that the relevance is lower than the
> previous match's relevance.

Done.

[Peter Kasting, 2012-09-14 18:05:52]

LGTM

http://codereview.chromium.org/10916304/diff/1/chrome/browser/autocomplete/co...
File chrome/browser/autocomplete/contact_provider_chromeos_unittest.cc (right):

http://codereview.chromium.org/10916304/diff/1/chrome/browser/autocomplete/co...
chrome/browser/autocomplete/contact_provider_chromeos_unittest.cc:94:
CHECK(contact) << "Unable to find contact " << i;
On 2012/09/14 15:31:50, Daniel Erat wrote:
> On 2012/09/13 23:32:37, Peter Kasting wrote:
> > Nit: Avoid [D]CHECK in tests, use ASSERT (2 places)
> 
> I think that the testing macros won't work here.

That's because this function returns non-void.  If you change the return value
to an outparam, you can use these macros, and then on the caller side, wrap
calls with ASSERT_NO_FATAL_FAILURE().

[Daniel Erat, 2012-09-14 18:27:06]

On Fri, Sep 14, 2012 at 2:05 PM,  <pkasting@chromium.org> wrote:
> LGTM
>
>
>
>
http://codereview.chromium.org/10916304/diff/1/chrome/browser/autocomplete/co...
> File chrome/browser/autocomplete/contact_provider_chromeos_unittest.cc
> (right):
>
>
http://codereview.chromium.org/10916304/diff/1/chrome/browser/autocomplete/co...
> chrome/browser/autocomplete/contact_provider_chromeos_unittest.cc:94:
> CHECK(contact) << "Unable to find contact " << i;
> On 2012/09/14 15:31:50, Daniel Erat wrote:
>>
>> On 2012/09/13 23:32:37, Peter Kasting wrote:
>> > Nit: Avoid [D]CHECK in tests, use ASSERT (2 places)
>
>
>> I think that the testing macros won't work here.
>
>
> That's because this function returns non-void.  If you change the return
> value to an outparam, you can use these macros, and then on the caller
> side, wrap calls with ASSERT_NO_FATAL_FAILURE().

I'll stick with the LOG(ERROR)s, if you don't object.  If these
methods are switched to use out-params, I believe I'll need to move
all of the invocations to standalone statements instead of the current
inline calls within comparisons, which'll make everything about twice
as long and harder to read.

> http://codereview.chromium.org/10916304/

[commit-bot: I haz the power, 2012-09-14 18:28:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/derat@chromium.org/10916304/2002

[commit-bot: I haz the power, 2012-09-14 20:56:27]

Change committed as 156883

[Peter Kasting, 2012-09-14 21:03:21]

(Sorry, was afk 2 hours)

I see what you mean.  In that case I'd either return to using DCHECK or rip out
the statements entirely.  Since we really expect these not to ever fail, DCHECK
seems more correct than LOG(ERROR).

[Daniel Erat, 2012-09-16 13:08:35]

On Fri, Sep 14, 2012 at 5:03 PM,  <pkasting@chromium.org> wrote:
> (Sorry, was afk 2 hours)
>
> I see what you mean.  In that case I'd either return to using DCHECK or rip
> out
> the statements entirely.  Since we really expect these not to ever fail,
> DCHECK
> seems more correct than LOG(ERROR).

I'll delete the the LOG(ERROR)s if you feel strongly about this, but
they seem useful to me in their present form: the messages should only
get printed when the test is going to fail soon afterwards, and they
give additional information that's useful in debugging the failure.

> http://codereview.chromium.org/10916304/

[Peter Kasting, 2012-09-17 02:34:34]

On 2012/09/16 13:08:35, Daniel Erat wrote:
> On Fri, Sep 14, 2012 at 5:03 PM,  <mailto:pkasting@chromium.org> wrote:
> > (Sorry, was afk 2 hours)
> >
> > I see what you mean.  In that case I'd either return to using DCHECK or rip
> > out
> > the statements entirely.  Since we really expect these not to ever fail,
> > DCHECK
> > seems more correct than LOG(ERROR).
> 
> I'll delete the the LOG(ERROR)s if you feel strongly about this, but
> they seem useful to me in their present form: the messages should only
> get printed when the test is going to fail soon afterwards, and they
> give additional information that's useful in debugging the failure.

LOG messages are often lost or mangled for various reasons.  Plus, they don't
imply that the condition is invariably false the way [D]CHECK does (see end of
http://dev.chromium.org/developers/coding-style ). For both these reasons, a
DCHECK seems better.

[Daniel Erat, 2012-09-18 20:19:46]

On Sun, Sep 16, 2012 at 7:34 PM,  <pkasting@chromium.org> wrote:
> On 2012/09/16 13:08:35, Daniel Erat wrote:
>
>> On Fri, Sep 14, 2012 at 5:03 PM,  <mailto:pkasting@chromium.org> wrote:
>> > (Sorry, was afk 2 hours)
>> >
>> > I see what you mean.  In that case I'd either return to using DCHECK or
>> > rip
>> > out
>> > the statements entirely.  Since we really expect these not to ever fail,
>> > DCHECK
>> > seems more correct than LOG(ERROR).
>
>
>> I'll delete the the LOG(ERROR)s if you feel strongly about this, but
>> they seem useful to me in their present form: the messages should only
>> get printed when the test is going to fail soon afterwards, and they
>> give additional information that's useful in debugging the failure.
>
>
> LOG messages are often lost or mangled for various reasons.  Plus, they
> don't
> imply that the condition is invariably false the way [D]CHECK does (see end
> of
> http://dev.chromium.org/developers/coding-style ). For both these reasons, a
> DCHECK seems better.

Following up on this: what are the reasons to prefer DCHECK over CHECK
here?  Are tests ever built in non-debug mode?

> http://codereview.chromium.org/10916304/

[Peter Kasting, 2012-09-18 20:26:56]

On 2012/09/18 20:19:46, Daniel Erat wrote:
> Following up on this: what are the reasons to prefer DCHECK over CHECK
> here?  Are tests ever built in non-debug mode?

We always use DCHECK except where (a) we're temporarily using CHECK in order to
track down what assertions are being violated on the trail of some bug, or (b)
violating this assertion would cause a security hole (e.g. we start sending
private data to an attacker-controlled renderer) and thus we must crash Chrome
instead.