contacts: Add ContactStoreFactory and FakeContactStore.

contacts: Add ContactStoreFactory and FakeContactStore.

This adds a factory interface for ContactStores and a fake,
in-memory implementation of ContactStore.  Both will be
needed for testing the upcoming ContactManager class.

I'm also fixing a bug in GoogleContactStore (all contacts
were being passed to ContactDatabase in partial updates
instead of just the updated ones) and another bug in
FakeContactDatabase::SaveContacts() that was hidden by the
GoogleContactStore bug.

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

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

[satorux1, 2012-08-03 18:16:52]

LGTM with a nit

https://chromiumcodereview.appspot.com/10850033/diff/1007/chrome/browser/chro...
File chrome/browser/chromeos/contacts/fake_contact_store.h (right):

https://chromiumcodereview.appspot.com/10850033/diff/1007/chrome/browser/chro...
chrome/browser/chromeos/contacts/fake_contact_store.h:76: void
OnFakeContactStoreDestroyed(FakeContactStore* store);
I think the function name comes from the fact that we should call this when fake
constact store is destroyed, which is a commonly used pattern for callback
functions.

However, for a public function, simply RemoveStore() may be easier to read.

[Daniel Erat, 2012-08-03 20:00:23]

I snuck in a few bug fixes.  Do you mind reviewing the differences between PS3
and PS4?

https://chromiumcodereview.appspot.com/10850033/diff/1007/chrome/browser/chro...
File chrome/browser/chromeos/contacts/fake_contact_store.h (right):

https://chromiumcodereview.appspot.com/10850033/diff/1007/chrome/browser/chro...
chrome/browser/chromeos/contacts/fake_contact_store.h:76: void
OnFakeContactStoreDestroyed(FakeContactStore* store);
On 2012/08/03 18:16:52, satorux1 wrote:
> I think the function name comes from the fact that we should call this when
fake
> constact store is destroyed, which is a commonly used pattern for callback
> functions.
> 
> However, for a public function, simply RemoveStore() may be easier to read.

Done.

https://chromiumcodereview.appspot.com/10850033/diff/1007/chrome/browser/chro...
File chrome/browser/chromeos/contacts/google_contact_store.cc (right):

https://chromiumcodereview.appspot.com/10850033/diff/1007/chrome/browser/chro...
chrome/browser/chromeos/contacts/google_contact_store.cc:276: for
(ContactMap::const_iterator it = contacts_.begin();
This is an embarrassing bug -- I was writing all contacts to disk instead of
just the updated ones. :-(

https://chromiumcodereview.appspot.com/10850033/diff/11001/chrome/browser/chr...
File chrome/browser/chromeos/contacts/contact_store.h (right):

https://chromiumcodereview.appspot.com/10850033/diff/11001/chrome/browser/chr...
chrome/browser/chromeos/contacts/contact_store.h:53: virtual bool
CanCreateContactStoreForProfile(Profile* profile) = 0;
I snuck this in so ContactManager can use it.

https://chromiumcodereview.appspot.com/10850033/diff/11001/chrome/browser/chr...
File chrome/browser/chromeos/contacts/fake_contact_database.cc (left):

https://chromiumcodereview.appspot.com/10850033/diff/11001/chrome/browser/chr...
chrome/browser/chromeos/contacts/fake_contact_database.cc:45:
test::CopyContacts(*contacts, &contacts_);
This bug was hidden by the bug in GoogleContactStore.  I was replacing all the
existing contacts when !is_full_update, but GoogleContactStore was passing all
contacts for partial updates. :-(

https://chromiumcodereview.appspot.com/10850033/diff/11001/chrome/browser/chr...
File chrome/browser/chromeos/contacts/fake_contact_database.cc (right):

https://chromiumcodereview.appspot.com/10850033/diff/11001/chrome/browser/chr...
chrome/browser/chromeos/contacts/fake_contact_database.cc:76: for
(ScopedVector<Contact>::const_iterator existing_it = contacts_.begin();
This is ugly and inefficient, but I'm not sure how much it matters.  I could use
map<string, Contact*> instead, but then I'll need to update all of the tests
that call contacts().

https://chromiumcodereview.appspot.com/10850033/diff/11001/chrome/browser/chr...
File chrome/browser/chromeos/contacts/google_contact_store.cc (right):

https://chromiumcodereview.appspot.com/10850033/diff/11001/chrome/browser/chr...
chrome/browser/chromeos/contacts/google_contact_store.cc:89: // message loop is
typically already getting torn down at this point.
[9076:9076:952949823645:FATAL:google_contact_store.cc(88)] Check failed:
BrowserThread::CurrentlyOn(BrowserThread::UI). 
Backtrace:
        base::debug::StackTrace::StackTrace() [0x7f16a33714f2]
        logging::LogMessage::~LogMessage() [0x7f16a33a5097]
        contacts::GoogleContactStore::~GoogleContactStore() [0x7f16a5c7002f]
        STLDeleteValues<>() [0x7f16a5c6bccd]
        STLValueDeleter<>::~STLValueDeleter() [0x7f16a5c6b07b]
        contacts::ContactManager::~ContactManager() [0x7f16a5c69560]
        scoped_ptr<>::~scoped_ptr() [0x7f16a5c67d89]
        ChromeBrowserMainPartsChromeos::~ChromeBrowserMainPartsChromeos()
[0x7f16a5c6559b]
        scoped_ptr<>::~scoped_ptr() [0x7f16992a0e07]
        content::BrowserMainLoop::~BrowserMainLoop() [0x7f169929f0dc]
        scoped_ptr<>::reset() [0x7f16992a2902]
        (anonymous namespace)::BrowserMainRunnerImpl::Shutdown()
[0x7f16992a2698]
        BrowserMain() [0x7f169929dbae]
        content::RunNamedProcessTypeMain() [0x7f169927a682]
        content::ContentMainRunnerImpl::Run() [0x7f169927b4c8]
        content::ContentMain() [0x7f1699279ce9]
        ChromeMain [0x7f16a5ab3cc1]
        main [0x7f16a5ab3c8c]
        0x7f16936efc4d
        0x7f16a5ab3b99

[satorux1, 2012-08-03 20:08:14]

LGTM

http://codereview.chromium.org/10850033/diff/11001/chrome/browser/chromeos/co...
File chrome/browser/chromeos/contacts/fake_contact_database.cc (right):

http://codereview.chromium.org/10850033/diff/11001/chrome/browser/chromeos/co...
chrome/browser/chromeos/contacts/fake_contact_database.cc:76: for
(ScopedVector<Contact>::const_iterator existing_it = contacts_.begin();
On 2012/08/03 20:00:23, Daniel Erat wrote:
> This is ugly and inefficient, but I'm not sure how much it matters.  I could
use
> map<string, Contact*> instead, but then I'll need to update all of the tests
> that call contacts().

Shouldn't matter for tests

http://codereview.chromium.org/10850033/diff/11001/chrome/browser/chromeos/co...
File chrome/browser/chromeos/contacts/google_contact_store.cc (right):

http://codereview.chromium.org/10850033/diff/11001/chrome/browser/chromeos/co...
chrome/browser/chromeos/contacts/google_contact_store.cc:89: // message loop is
typically already getting torn down at this point.
wow this is subtle...

On 2012/08/03 20:00:23, Daniel Erat wrote:
> [9076:9076:952949823645:FATAL:google_contact_store.cc(88)] Check failed:
> BrowserThread::CurrentlyOn(BrowserThread::UI). 
> Backtrace:
>         base::debug::StackTrace::StackTrace() [0x7f16a33714f2]
>         logging::LogMessage::~LogMessage() [0x7f16a33a5097]
>         contacts::GoogleContactStore::~GoogleContactStore() [0x7f16a5c7002f]
>         STLDeleteValues<>() [0x7f16a5c6bccd]
>         STLValueDeleter<>::~STLValueDeleter() [0x7f16a5c6b07b]
>         contacts::ContactManager::~ContactManager() [0x7f16a5c69560]
>         scoped_ptr<>::~scoped_ptr() [0x7f16a5c67d89]
>         ChromeBrowserMainPartsChromeos::~ChromeBrowserMainPartsChromeos()
> [0x7f16a5c6559b]
>         scoped_ptr<>::~scoped_ptr() [0x7f16992a0e07]
>         content::BrowserMainLoop::~BrowserMainLoop() [0x7f169929f0dc]
>         scoped_ptr<>::reset() [0x7f16992a2902]
>         (anonymous namespace)::BrowserMainRunnerImpl::Shutdown()
> [0x7f16992a2698]
>         BrowserMain() [0x7f169929dbae]
>         content::RunNamedProcessTypeMain() [0x7f169927a682]
>         content::ContentMainRunnerImpl::Run() [0x7f169927b4c8]
>         content::ContentMain() [0x7f1699279ce9]
>         ChromeMain [0x7f16a5ab3cc1]
>         main [0x7f16a5ab3c8c]
>         0x7f16936efc4d
>         0x7f16a5ab3b99

[Daniel Erat, 2012-08-03 20:16:54]

TBR ben for chrome_tests.gypi

[commit-bot: I haz the power, 2012-08-03 20:17:34]

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

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

Change committed as 149924