contacts: Add GoogleContactStore.

contacts: Add GoogleContactStore.

This class uses GDataContactsService to download contacts
from Google and uses ContactDatabase to save them to disk.
It also contains logic for periodically refreshing contacts.

BUG=128805
TEST=none
TBR=ben@chromium.org

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

[Daniel Erat, 2012-08-02 16:55:48]

This is the last of the big changes!

[satorux1, 2012-08-02 22:21:25]

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/contact_store.h (right):

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_store.h:17: class ContactStoreObserver;
I thought we usually defined Observer as a nested class, like
ContactStore::Observer. However inside a class, "Observer" becomes ambiguous, if
the class observes another class's Observer...

That said, I probably like this approach better. :)

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/google_contact_store.cc (right):

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/google_contact_store.cc:30:
FILE_PATH_LITERAL("Google Contacts");
I'm old fashioned enough to worry about file names with a space. :) Maybe rename
to GoogleContacts or just Contacts? 

FWIW, I recently had an issue with "Google Drive" directory, which caused some
unexpected behavior with rsync over ssh...

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/google_contact_store.cc:217:
updated_contacts->weak_clear();
I was a bit confused. Here, weak_clear() is used, as the all elements of
updated_contents are already moved to |contacts_|, correct?

Maybe it's less confusing to use a loop like below, instead of a for loop:

  while (!updated_contents->empty()) {
     Contact* contact = updated_contents->last();
     updated_contents->pop_back();
     ...
  }

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/google_contact_store.cc:307:
scoped_ptr<UpdateMetadata> metadata) {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); is missing?

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/google_contact_store.cc:325: void
GoogleContactStore::OnDatabaseContactsSaved(bool success) {
ditto

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/google_contact_store.h (right):

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/google_contact_store.h:38: public
base::SupportsWeakPtr<GoogleContactStore> {
Per the chromium-dev thread on WeakPtr, SupportsWeakPtr<> was considered a bad
idea for a subtle reason. I't be probably better to use WeakPtrFactory instead

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/google_contact_store_unittest.cc (right):

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/google_contact_store_unittest.cc:77:
gdata::GDataContactsServiceStub* gdata_service() { return gdata_service_; }
In tests, you might want to make all members protected: so you could access them
from TEST_F bodies.

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/google_contact_store_unittest.cc:122:
ASSERT_TRUE(loaded_contact1 != NULL);
you could do ASSERT_TRUE(loaded_contact1);

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/google_contact_store_unittest.cc:126:
EXPECT_TRUE(store()->GetContactByProviderId("bogus_id") == NULL);
you could do EXPECT_FALSE(store()->GetContactByProviderId("bogus_id"))

[Daniel Erat, 2012-08-02 23:23:18]

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/contact_store.h (right):

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_store.h:17: class ContactStoreObserver;
On 2012/08/02 22:21:25, satorux1 wrote:
> I thought we usually defined Observer as a nested class, like
> ContactStore::Observer. However inside a class, "Observer" becomes ambiguous,
if
> the class observes another class's Observer...
> 
> That said, I probably like this approach better. :)

I've seen people go both ways on this.  This way usually seems safer to me, in
case I'm sending my code to a reviewer who will complain about header bloat
(i.e. all of the observer implementations' headers would need to include
contact_store.h instead of contact_store_observer.h). :-)

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/google_contact_store.cc (right):

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/google_contact_store.cc:30:
FILE_PATH_LITERAL("Google Contacts");
On 2012/08/02 22:21:25, satorux1 wrote:
> I'm old fashioned enough to worry about file names with a space. :) Maybe
rename
> to GoogleContacts or just Contacts? 
> 
> FWIW, I recently had an issue with "Google Drive" directory, which caused some
> unexpected behavior with rsync over ssh...

Hmm.  It looks like using spaces is more common than not in
~/.config/google-chrome/Default, though ("Application Cache", "Archived
History", "Cached Theme Images", "Current Session", etc.).

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/google_contact_store.cc:217:
updated_contacts->weak_clear();
On 2012/08/02 22:21:25, satorux1 wrote:
> I was a bit confused. Here, weak_clear() is used, as the all elements of
> updated_contents are already moved to |contacts_|, correct?
> 
> Maybe it's less confusing to use a loop like below, instead of a for loop:
> 
>   while (!updated_contents->empty()) {
>      Contact* contact = updated_contents->last();
>      updated_contents->pop_back();
>      ...
>   }

ScopedVector doesn't offer pop_back() (and if it did, it'd probably also delete
the element -- we'd need something like weak_pop_back() instead).  I could
repeatedly call weak_erase() on the last element, but I don't know if it'd be
any clearer than using weak_clear().

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/google_contact_store.cc:307:
scoped_ptr<UpdateMetadata> metadata) {
On 2012/08/02 22:21:25, satorux1 wrote:
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); is missing?

Yep, also noticed this and added it locally. :-)

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/google_contact_store.cc:325: void
GoogleContactStore::OnDatabaseContactsSaved(bool success) {
On 2012/08/02 22:21:25, satorux1 wrote:
> ditto

Done.

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/google_contact_store.h (right):

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/google_contact_store.h:38: public
base::SupportsWeakPtr<GoogleContactStore> {
On 2012/08/02 22:21:25, satorux1 wrote:
> Per the chromium-dev thread on WeakPtr, SupportsWeakPtr<> was considered a bad
> idea for a subtle reason. I't be probably better to use WeakPtrFactory instead

Done.

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/google_contact_store_unittest.cc (right):

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/google_contact_store_unittest.cc:77:
gdata::GDataContactsServiceStub* gdata_service() { return gdata_service_; }
On 2012/08/02 22:21:25, satorux1 wrote:
> In tests, you might want to make all members protected: so you could access
them
> from TEST_F bodies.

Done.  Thinking back, I believe that I just did it this way so I could get the
destruction order that I wanted without needing multiple protected and private
blocks.

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/google_contact_store_unittest.cc:122:
ASSERT_TRUE(loaded_contact1 != NULL);
On 2012/08/02 22:21:25, satorux1 wrote:
> you could do ASSERT_TRUE(loaded_contact1);

Done.

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/google_contact_store_unittest.cc:126:
EXPECT_TRUE(store()->GetContactByProviderId("bogus_id") == NULL);
On 2012/08/02 22:21:25, satorux1 wrote:
> you could do EXPECT_FALSE(store()->GetContactByProviderId("bogus_id"))

Done.

[Daniel Erat, 2012-08-02 23:34:22]

http://codereview.chromium.org/10829144/diff/10001/chrome/browser/chromeos/co...
File chrome/browser/chromeos/contacts/google_contact_store_unittest.cc (right):

http://codereview.chromium.org/10829144/diff/10001/chrome/browser/chromeos/co...
chrome/browser/chromeos/contacts/google_contact_store_unittest.cc:74:
MessageLoopForUI message_loop_;
There's no real reason to make these private.  I'll just make the whole section
protected instead.

[satorux1, 2012-08-03 00:11:10]

LGTM

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/google_contact_store.cc (right):

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/google_contact_store.cc:30:
FILE_PATH_LITERAL("Google Contacts");
On 2012/08/02 23:23:18, Daniel Erat wrote:
> On 2012/08/02 22:21:25, satorux1 wrote:
> > I'm old fashioned enough to worry about file names with a space. :) Maybe
> rename
> > to GoogleContacts or just Contacts? 
> > 
> > FWIW, I recently had an issue with "Google Drive" directory, which caused
some
> > unexpected behavior with rsync over ssh...
> 
> Hmm.  It looks like using spaces is more common than not in
> ~/.config/google-chrome/Default, though ("Application Cache", "Archived
> History", "Cached Theme Images", "Current Session", etc.).

Ah then, spaces are fine.

http://codereview.chromium.org/10829144/diff/2001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/google_contact_store.cc:217:
updated_contacts->weak_clear();
On 2012/08/02 23:23:18, Daniel Erat wrote:
> On 2012/08/02 22:21:25, satorux1 wrote:
> > I was a bit confused. Here, weak_clear() is used, as the all elements of
> > updated_contents are already moved to |contacts_|, correct?
> > 
> > Maybe it's less confusing to use a loop like below, instead of a for loop:
> > 
> >   while (!updated_contents->empty()) {
> >      Contact* contact = updated_contents->last();
> >      updated_contents->pop_back();
> >      ...
> >   }
> 
> ScopedVector doesn't offer pop_back() (and if it did, it'd probably also
delete
> the element -- we'd need something like weak_pop_back() instead).  I could
> repeatedly call weak_erase() on the last element, but I don't know if it'd be
> any clearer than using weak_clear().

I see. Then, we should keep this way.

http://codereview.chromium.org/10829144/diff/3008/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/google_contact_store_unittest.cc (right):

http://codereview.chromium.org/10829144/diff/3008/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/google_contact_store_unittest.cc:273:
EXPECT_TRUE(store_->GetContactByProviderId(kProviderId) == NULL);
maybe expect_false()

[satorux1, 2012-08-03 00:12:47]

http://codereview.chromium.org/10829144/diff/3008/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/google_contact_store_unittest.cc (right):

http://codereview.chromium.org/10829144/diff/3008/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/google_contact_store_unittest.cc:273:
EXPECT_TRUE(store_->GetContactByProviderId(kProviderId) == NULL);
On 2012/08/03 00:11:10, satorux1 wrote:
> maybe expect_false()

I meant  EXPECT_FALSE(store_->GetContactByProviderId(kProviderId))

[Daniel Erat, 2012-08-03 00:22:04]

http://codereview.chromium.org/10829144/diff/3008/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/google_contact_store_unittest.cc (right):

http://codereview.chromium.org/10829144/diff/3008/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/google_contact_store_unittest.cc:273:
EXPECT_TRUE(store_->GetContactByProviderId(kProviderId) == NULL);
On 2012/08/03 00:12:47, satorux1 wrote:
> On 2012/08/03 00:11:10, satorux1 wrote:
> > maybe expect_false()
> 
> I meant  EXPECT_FALSE(store_->GetContactByProviderId(kProviderId))

Sure, done.

[Daniel Erat, 2012-08-03 00:22:32]

TBR ben for *.gypi

[commit-bot: I haz the power, 2012-08-03 00:22:49]

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

[Daniel Erat, 2012-08-03 03:23:37]

Holding off while I try to figure out the source of some memory leaks reported
by Valgrind.  I added logging to GoogleContactSource and don't think that any
Contact objects are being leaked there.

Leak_DefinitelyLost
1,278 (640 direct, 638 indirect) bytes in 2 blocks are definitely lost in loss
record 735 of 787
  operator new(unsigned long) (m_replacemalloc/vg_replace_malloc.c:1074)
  contacts::test::CopyContacts(std::vector<contacts::Contact const*,
std::allocator<contacts::Contact const*> > const&,
ScopedVector<contacts::Contact>*)
(/out/Debug/../../chrome/browser/chromeos/contacts/contact_test_util.cc:142)
  contacts::test::CopyContacts(ScopedVector<contacts::Contact> const&,
ScopedVector<contacts::Contact>*)
(/out/Debug/../../chrome/browser/chromeos/contacts/contact_test_util.cc:153)
  contacts::FakeContactDatabase::LoadContacts(base::Callback<void ()(bool,
scoped_ptr<ScopedVector<contacts::Contact> >,
scoped_ptr<contacts::UpdateMetadata>)>)
(/out/Debug/../../chrome/browser/chromeos/contacts/fake_contact_database.cc:56)
  contacts::GoogleContactStore::OnDatabaseInitialized(bool)
(/out/Debug/../../chrome/browser/chromeos/contacts/google_contact_store.cc:324)

Leak_DefinitelyLost
752 (640 direct, 112 indirect) bytes in 2 blocks are definitely lost in loss
record 720 of 787
  operator new(unsigned long) (m_replacemalloc/vg_replace_malloc.c:1074)
  contacts::test::CopyContacts(std::vector<contacts::Contact const*,
std::allocator<contacts::Contact const*> > const&,
ScopedVector<contacts::Contact>*)
(/out/Debug/../../chrome/browser/chromeos/contacts/contact_test_util.cc:142)
  contacts::test::CopyContacts(ScopedVector<contacts::Contact> const&,
ScopedVector<contacts::Contact>*)
(/out/Debug/../../chrome/browser/chromeos/contacts/contact_test_util.cc:153)
  gdata::GDataContactsServiceStub::DownloadContacts(base::Callback<void
()(scoped_ptr<ScopedVector<contacts::Contact> >)>, base::Callback<void ()()>,
base::Time const&)
(/out/Debug/../../chrome/browser/chromeos/gdata/gdata_contacts_service_stub.cc:56)
  contacts::GoogleContactStore::UpdateContacts()
(/out/Debug/../../chrome/browser/chromeos/contacts/google_contact_store.cc:192)
  contacts::GoogleContactStore::TestAPI::DoUpdate()
(/out/Debug/../../chrome/browser/chromeos/contacts/google_contact_store.cc:74)
 
contacts::test::GoogleContactStoreTest_FullRefreshAfterThirtyDays_Test::TestBody()
(/out/Debug/../../chrome/browser/chromeos/contacts/google_contact_store_unittest.cc:325)

[Daniel Erat, 2012-08-03 04:53:31]

http://codereview.chromium.org/10829144/diff/6002/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/google_contact_store.cc (right):

http://codereview.chromium.org/10829144/diff/6002/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/google_contact_store.cc:213: contacts_.clear();
Ugh, I looked right past this without seeing the problem.  I think I missed it
because most of this code is using ScopedVector, whose clear() method deletes
its elements.  Too bad there's no ScopedMap (I know that people were discussing
this on the chromium-dev thread about linked_ptr).

[commit-bot: I haz the power, 2012-08-03 04:54:17]

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

[commit-bot: I haz the power, 2012-08-03 07:27:23]

Change committed as 149805