autocomplete: Make ContactProvider rank contacts by affinity

chrome/browser/autocomplete/contact_provider_chromeos_unittest.cc

 // Copyright (c) 2012 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 "chrome/browser/autocomplete/contact_provider_chromeos.h"
 
+#include <cmath>
 #include <map>
 #include <string>
 #include <vector>
 
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
+#include "base/string_number_conversions.h"
 #include "base/string16.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/autocomplete/autocomplete_input.h"
 #include "chrome/browser/autocomplete/autocomplete_match.h"
 #include "chrome/browser/autocomplete/autocomplete_provider.h"
 #include "chrome/browser/chromeos/contacts/contact.pb.h"
 #include "chrome/browser/chromeos/contacts/contact_manager_stub.h"
 #include "chrome/browser/chromeos/contacts/contact_test_util.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile.h"
@@ skipping 43 matching lines @@
     contact_provider_->Start(
         AutocompleteInput(UTF8ToUTF16(utf8_text),
                           string16(),  // desired_tld
                           false,       // prevent_inline_autocomplete
                           false,       // prefer_keyword
                           false,       // allow_exact_keyword_match
                           AutocompleteInput::ALL_MATCHES),
         false);  // minimal_changes
   }
 
+  // Returns the contact ID in |match|'s additional info, or an empty string if
+  // no ID is present.
+  std::string GetContactIdFromMatch(const AutocompleteMatch& match) {
+    std::map<std::string, std::string>::const_iterator it =

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

Nit: Use AutocompleteMatch::AdditionalInfo::const_iterator

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

Done.

+        match.additional_info.find(ContactProvider::kMatchContactIdKey);
+    return it != match.additional_info.end() ? it->second : std::string();
+  }
+
   // Returns pointers to all of the Contact objects referenced in
   // |contact_provider_|'s current results.
   contacts::ContactPointers GetMatchedContacts() {
     contacts::ContactPointers contacts;
     const ACMatches& matches = contact_provider_->matches();
     for (size_t i = 0; i < matches.size(); ++i) {
-      std::map<std::string, std::string>::const_iterator id_it =
-          matches[i].additional_info.find(ContactProvider::kMatchContactIdKey);
-      CHECK(id_it != matches[i].additional_info.end());
-      const contacts::Contact* contact =
-          contact_manager_->GetContactById(profile_, id_it->second);
-      CHECK(contact) << "Unable to find contact with ID " << id_it->second;
+      const contacts::Contact* contact = contact_manager_->GetContactById(
+          profile_, GetContactIdFromMatch(matches[i]));
+      CHECK(contact) << "Unable to find contact " << i;

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

Nit: Avoid [D]CHECK in tests, use ASSERT (2 places)

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

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*>}'

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

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().

       contacts.push_back(contact);
     }
     return contacts;
   }
 
   // Returns a semicolon-separated string containing string representations (as
   // provided by AutocompleteMatch::ClassificationsToString()) of the
   // |contents_class| fields of all current matches.  Results are sorted by
   // contact ID.
   std::string GetMatchClassifications() {
     typedef std::map<std::string, std::string> StringMap;
     StringMap contact_id_classifications;
     const ACMatches& matches = contact_provider_->matches();
     for (size_t i = 0; i < matches.size(); ++i) {
-      std::map<std::string, std::string>::const_iterator id_it =
-          matches[i].additional_info.find(ContactProvider::kMatchContactIdKey);
-      CHECK(id_it != matches[i].additional_info.end());
-      contact_id_classifications[id_it->second] =
+      std::string id = GetContactIdFromMatch(matches[i]);
+      CHECK(!id.empty()) << "Unable to find contact " << i;
+      contact_id_classifications[id] =
           AutocompleteMatch::ClassificationsToString(matches[i].contents_class);
     }
 
     std::string result;
     for (StringMap::const_iterator it = contact_id_classifications.begin();
          it != contact_id_classifications.end(); ++it) {
       if (!result.empty())
         result += ";";
       result += it->second;
     }
@@ skipping 104 matching lines @@
       contacts::test::VarContactsToString(1, contact.get()),
       contacts::test::ContactsToString(GetMatchedContacts()));
   EXPECT_EQ("0,2,5,0", GetMatchClassifications());
 
   StartQuery("adelsvard");
   EXPECT_EQ(
       contacts::test::VarContactsToString(1, contact.get()),
       contacts::test::ContactsToString(GetMatchedContacts()));
   EXPECT_EQ("0,0,6,2", GetMatchClassifications());
 }
+
+TEST_F(ContactProviderTest, Relevance) {
+  // Create more contacts than the maximum number of results that an
+  // AutocompleteProvider should return.  Give them all the same family name and
+  // ascending affinities from 0.0 to 1.0.
+  const size_t kNumContacts = AutocompleteProvider::kMaxMatches + 1;
+  const std::string kFamilyName = "Jones";
+
+  ScopedVector<contacts::Contact> contacts;
+  contacts::ContactPointers contact_pointers;
+  for (size_t i = 0; i < kNumContacts; ++i) {
+    contacts::Contact* contact = new contacts::Contact;
+    std::string id_string = base::IntToString(static_cast<int>(i));
+    InitContact(id_string, id_string, kFamilyName,
+                id_string + " " + kFamilyName, contact);
+    contact->set_affinity(static_cast<float>(i) / kNumContacts);
+    contacts.push_back(contact);
+    contact_pointers.push_back(contact);
+  }
+
+  contact_manager_->SetContacts(contact_pointers);
+  contact_manager_->NotifyObserversAboutUpdatedContacts();
+
+  // Do a search for the family name and check that the total number of results
+  // is limited as expected and that the results are ordered by descending
+  // affinity.
+  StartQuery(kFamilyName);
+  const ACMatches& matches = contact_provider_->matches();
+  ASSERT_EQ(AutocompleteProvider::kMaxMatches, matches.size());
+  for (size_t i = 0; i < matches.size(); ++i) {
+    const contacts::Contact& exp_contact =
+        *(contacts[kNumContacts - 1 - i]);
+    std::string match_id = GetContactIdFromMatch(matches[i]);
+    EXPECT_EQ(exp_contact.contact_id(), match_id)
+        << "Expected contact ID " << exp_contact.contact_id()
+        << " for match " << i << " but got " << match_id << " instead";
+    EXPECT_EQ(static_cast<int>(roundf(ContactProvider::kBaseRelevance +
+        ContactProvider::kAffinityRelevanceBoost * exp_contact.affinity())),

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

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]

Done.

+        matches[i].relevance);
+  }
+}