contacts: Strip Unicode byte order marks from strings.

The feeds that we get from the Contacts API are UTF-8, but
they're littered with BOM characters.  This change makes
GDataContactsService strip out these characters when
initializing contacts::Contact objects.

I'm also merging a few related browser tests together so
that the new test that I'm adding won't increase the total
running time.

BUG=128805
TEST=none


Review URL: https://chromiumcodereview.appspot.com/10829435

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@152389 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chromeos/contacts/contact.proto

@@ -12,7 +12,7 @@ package contacts;
 
 // A contact, roughly based on the GData Contact kind:
 // https://developers.google.com/gdata/docs/2.0/elements#gdContactKind
-// All strings are UTF-8.
+// All strings are UTF-8 with Unicode byte order marks stripped out.
 message Contact {
   // Next ID to use: 15
 

chrome/browser/chromeos/gdata/gdata_contacts_service.cc

@@ -14,6 +14,7 @@
 #include "base/logging.h"
 #include "base/memory/weak_ptr.h"
 #include "base/stl_util.h"
+#include "base/string_util.h"
 #include "base/time.h"
 #include "base/timer.h"
 #include "base/values.h"
@@ -138,6 +139,20 @@ std::string PrettyPrintValue(const base::Value& value) {
   return out;
 }
 
+// Assigns the value at |path| within |dict| to |out|, returning false if the
+// path wasn't present.  Unicode byte order marks are removed from the string.
+bool GetCleanedString(const DictionaryValue& dict,
+                      const std::string& path,
+                      std::string* out) {
+  if (!dict.GetString(path, out))
+    return false;
+
+  // The Unicode byte order mark, U+FEFF, is useless in UTF-8 strings (which are
+  // interpreted one byte at a time).
+  ReplaceSubstringsAfterOffset(out, 0, "\xEF\xBB\xBF", "");
+  return true;
+}
+
 // Returns whether an address is primary, given a dictionary representing a
 // single address.
 bool IsAddressPrimary(const DictionaryValue& address_dict) {
@@ -164,7 +179,7 @@ void InitAddressType(const DictionaryValue& address_dict,
   else
     type->set_relation(contacts::Contact_AddressType_Relation_OTHER);
 
-  address_dict.GetString(kAddressLabelField, type->mutable_label());
+  GetCleanedString(address_dict, kAddressLabelField, type->mutable_label());
 }
 
 // Maps the protocol from a dictionary representing a contact's IM address to a
@@ -248,12 +263,13 @@ bool FillContactFromDictionary(const base::DictionaryValue& dict,
   if (contact->deleted())
     return true;
 
-  dict.GetString(kFullNameField, contact->mutable_full_name());
-  dict.GetString(kGivenNameField, contact->mutable_given_name());
-  dict.GetString(kAdditionalNameField, contact->mutable_additional_name());
-  dict.GetString(kFamilyNameField, contact->mutable_family_name());
-  dict.GetString(kNamePrefixField, contact->mutable_name_prefix());
-  dict.GetString(kNameSuffixField, contact->mutable_name_suffix());
+  GetCleanedString(dict, kFullNameField, contact->mutable_full_name());
+  GetCleanedString(dict, kGivenNameField, contact->mutable_given_name());
+  GetCleanedString(
+      dict, kAdditionalNameField, contact->mutable_additional_name());
+  GetCleanedString(dict, kFamilyNameField, contact->mutable_family_name());
+  GetCleanedString(dict, kNamePrefixField, contact->mutable_name_prefix());
+  GetCleanedString(dict, kNameSuffixField, contact->mutable_name_suffix());
 
   const ListValue* email_list = NULL;
   if (dict.GetList(kEmailField, &email_list)) {
@@ -263,8 +279,11 @@ bool FillContactFromDictionary(const base::DictionaryValue& dict,
         return false;
 
       contacts::Contact_EmailAddress* email = contact->add_email_addresses();
-      if (!email_dict->GetString(kEmailAddressField, email->mutable_address()))
+      if (!GetCleanedString(*email_dict,
+                            kEmailAddressField,
+                            email->mutable_address())) {
         return false;
+      }
       email->set_primary(IsAddressPrimary(*email_dict));
       InitAddressType(*email_dict, email->mutable_type());
     }
@@ -278,8 +297,11 @@ bool FillContactFromDictionary(const base::DictionaryValue& dict,
         return false;
 
       contacts::Contact_PhoneNumber* phone = contact->add_phone_numbers();
-      if (!phone_dict->GetString(kPhoneNumberField, phone->mutable_number()))
+      if (!GetCleanedString(*phone_dict,
+                            kPhoneNumberField,
+                            phone->mutable_number())) {
         return false;
+      }
       phone->set_primary(IsAddressPrimary(*phone_dict));
       InitAddressType(*phone_dict, phone->mutable_type());
     }
@@ -294,8 +316,9 @@ bool FillContactFromDictionary(const base::DictionaryValue& dict,
 
       contacts::Contact_PostalAddress* address =
           contact->add_postal_addresses();
-      if (!address_dict->GetString(kPostalAddressFormattedField,
-                                   address->mutable_address())) {
+      if (!GetCleanedString(*address_dict,
+                            kPostalAddressFormattedField,
+                            address->mutable_address())) {
         return false;
       }
       address->set_primary(IsAddressPrimary(*address_dict));
@@ -312,8 +335,9 @@ bool FillContactFromDictionary(const base::DictionaryValue& dict,
 
       contacts::Contact_InstantMessagingAddress* im =
           contact->add_instant_messaging_addresses();
-      if (!im_dict->GetString(kInstantMessagingAddressField,
-                              im->mutable_address())) {
+      if (!GetCleanedString(*im_dict,
+                            kInstantMessagingAddressField,
+                            im->mutable_address())) {
         return false;
       }
       im->set_primary(IsAddressPrimary(*im_dict));

chrome/browser/chromeos/gdata/gdata_contacts_service.h

@@ -71,6 +71,9 @@ class GDataContactsService : public GDataContactsServiceInterface {
   const std::string& cached_my_contacts_group_id_for_testing() const {
     return cached_my_contacts_group_id_;
   }
+  void clear_cached_my_contacts_group_id_for_testing() {
+    cached_my_contacts_group_id_.clear();
+  }
 
   void set_max_photo_downloads_per_second_for_testing(int max_downloads) {
     max_photo_downloads_per_second_ = max_downloads;

chrome/browser/chromeos/gdata/gdata_contacts_service_browsertest.cc

@@ -169,22 +169,18 @@ IN_PROC_BROWSER_TEST_F(GDataContactsServiceTest, BrokenFeeds) {
   EXPECT_TRUE(Download("feed_photo_404.json", base::Time(), &contacts));
   ASSERT_EQ(static_cast<size_t>(1), contacts->size());
   EXPECT_FALSE((*contacts)[0]->has_raw_untrusted_photo());
-}
 
-// We should report failure when we're unable to download the contact group
-// feed.
-IN_PROC_BROWSER_TEST_F(GDataContactsServiceTest, MissingGroupsFeed) {
-  scoped_ptr<ScopedVector<contacts::Contact> > contacts;
+  // We should report failure when we're unable to download the contact group
+  // feed.
+  service_->clear_cached_my_contacts_group_id_for_testing();
   service_->set_groups_feed_url_for_testing(
       test_server_.GetURL(std::string(kFeedBaseUrl) + "404"));
   EXPECT_FALSE(Download("feed.json", base::Time(), &contacts));
   EXPECT_TRUE(service_->cached_my_contacts_group_id_for_testing().empty());
-}
 
-// We should also fail when the "My Contacts" group isn't listed in the group
-// feed.
-IN_PROC_BROWSER_TEST_F(GDataContactsServiceTest, NoMyContactsGroup) {
-  scoped_ptr<ScopedVector<contacts::Contact> > contacts;
+  // We should also fail when the "My Contacts" group isn't listed in the group
+  // feed.
+  service_->clear_cached_my_contacts_group_id_for_testing();
   service_->set_groups_feed_url_for_testing(
       test_server_.GetURL(std::string(kFeedBaseUrl) +
                           "groups_no_my_contacts.json"));
@@ -292,4 +288,23 @@ IN_PROC_BROWSER_TEST_F(GDataContactsServiceTest, ParallelPhotoDownload) {
             contacts::test::ContactsToString(*contacts));
 }
 
+IN_PROC_BROWSER_TEST_F(GDataContactsServiceTest, UnicodeStrings) {
+  scoped_ptr<ScopedVector<contacts::Contact> > contacts;
+  EXPECT_TRUE(Download("feed_unicode.json", base::Time(), &contacts));
+
+  // All of these expected values are hardcoded in the feed.
+  scoped_ptr<contacts::Contact> contact1(new contacts::Contact);
+  InitContact("http://example.com/1", "2012-06-04T15:53:36.023Z",
+              false, "\xE5\xAE\x89\xE8\x97\xA4\x20\xE5\xBF\xA0\xE9\x9B\x84",
+              "\xE5\xBF\xA0\xE9\x9B\x84", "", "\xE5\xAE\x89\xE8\x97\xA4",
+              "", "", contact1.get());
+  scoped_ptr<contacts::Contact> contact2(new contacts::Contact);
+  InitContact("http://example.com/2", "2012-06-21T16:20:13.208Z",
+              false, "Bob Smith", "Bob", "", "Smith", "", "",
+              contact2.get());
+  EXPECT_EQ(contacts::test::VarContactsToString(
+                2, contact1.get(), contact2.get()),
+            contacts::test::ContactsToString(*contacts));
+}
+
 }  // namespace gdata

chrome/test/data/chromeos/gdata/contacts/feed_unicode.json

+{
+   "encoding": "UTF-8",
+   "feed": {
+      "author": [ {
+         "email": {
+            "$t": "test.user@gmail.com"
+         },
+         "name": {
+            "$t": "Test User"
+         }
+      } ],
+      "category": [ {
+         "scheme": "http://schemas.google.com/g/2005#kind",
+         "term": "http://schemas.google.com/contact/2008#contact"
+      } ],
+      "entry": [ {
+         "app$edited": {
+            "$t": "2012-06-04T15:53:36.023Z",
+            "xmlns$app": "http://www.w3.org/2007/app"
+         },
+         "category": [ {
+            "scheme": "http://schemas.google.com/g/2005#kind",
+            "term": "http://schemas.google.com/contact/2008#contact"
+         } ],
+         "gContact$groupMembershipInfo": [ {
+            "deleted": "false",
+            "href": "http://www.google.com/m8/feeds/groups/test.user%40gmail.com/base/6"
+         } ],
+         "gd$etag": "\"R345ezVSLit7I2A9WhVbGE8PRQU.\"",
+         "gd$name": {
+            "gd$familyName": {
+               "$t": "\u5B89\u85E4"
+            },
+            "gd$fullName": {
+               "$t": "\u5B89\u85E4 \u5FE0\u96C4"
+            },
+            "gd$givenName": {
+               "$t": "\u5FE0\u96C4"
+            }
+         },
+         "id": {
+            "$t":
+            "http://example.com/1"
+         },
+         "title": {
+            "$t": "\u5B89\u85E4 \u5FE0\u96C4"
+         },
+         "updated": {
+            "$t": "2012-06-04T15:53:36.023Z"
+         }
+      }, {
+         "app$edited": {
+            "$t": "2012-06-21T16:20:13.208Z",
+            "xmlns$app": "http://www.w3.org/2007/app"
+         },
+         "category": [ {
+            "scheme": "http://schemas.google.com/g/2005#kind",
+            "term": "http://schemas.google.com/contact/2008#contact"
+         } ],
+         "gContact$groupMembershipInfo": [ {
+            "deleted": "false",
+            "href": "http://www.google.com/m8/feeds/groups/test.user%40gmail.com/base/6"
+         } ],
+         "gd$etag": "\"Qnw7cDVSLCt7I2A9WhJTEkQPQQU.\"",
+         "gd$name": {
+            "gd$familyName": {
+               "$t": "Smith\uFEFF"
+            },
+            "gd$fullName": {
+               "$t": "\uFEFFBob\uFEFF\uFEFF Smith\uFEFF"
+            },
+            "gd$givenName": {
+               "$t": "\uFEFFBob\uFEFF\uFEFF"
+            }
+         },
+         "id": {
+            "$t": "http://example.com/2"
+         },
+         "title": {
+            "$t": "\uFEFFBob\uFEFF\uFEFF Smith\uFEFF"
+         },
+         "updated": {
+            "$t": "2012-06-21T16:20:13.208Z"
+         }
+      } ],
+      "gd$etag": "W/\"CEUHSXg8fSt7I2A9WhJSEEw.\"",
+      "generator": {
+         "$t": "Contacts",
+         "uri": "http://www.google.com/m8/feeds",
+         "version": "1.0"
+      },
+      "id": {
+         "$t": "test.user@gmail.com"
+      },
+      "link": [ {
+         "href": "http://www.google.com/",
+         "rel": "alternate",
+         "type": "text/html"
+      }, {
+         "href": "https://www.google.com/m8/feeds/contacts/test.user%40gmail.com/full",
+         "rel": "http://schemas.google.com/g/2005#feed",
+         "type": "application/atom+xml"
+      }, {
+         "href": "https://www.google.com/m8/feeds/contacts/test.user%40gmail.com/full",
+         "rel": "http://schemas.google.com/g/2005#post",
+         "type": "application/atom+xml"
+      }, {
+         "href": "https://www.google.com/m8/feeds/contacts/test.user%40gmail.com/full/batch",
+         "rel": "http://schemas.google.com/g/2005#batch",
+         "type": "application/atom+xml"
+      }, {
+         "href": "https://www.google.com/m8/feeds/contacts/test.user%40gmail.com/full?alt=json&max-results=1000&showdeleted=true",
+         "rel": "self",
+         "type": "application/atom+xml"
+      } ],
+      "openSearch$itemsPerPage": {
+         "$t": "1000"
+      },
+      "openSearch$startIndex": {
+         "$t": "1"
+      },
+      "openSearch$totalResults": {
+         "$t": "2"
+      },
+      "title": {
+         "$t": "Test User's Contacts"
+      },
+      "updated": {
+         "$t": "2012-06-29T23:23:58.675Z"
+      },
+      "xmlns": "http://www.w3.org/2005/Atom",
+      "xmlns$batch": "http://schemas.google.com/gdata/batch",
+      "xmlns$gContact": "http://schemas.google.com/contact/2008",
+      "xmlns$gd": "http://schemas.google.com/g/2005",
+      "xmlns$openSearch": "http://a9.com/-/spec/opensearch/1.1/"
+   },
+   "version": "1.0"
+}