contacts: Add ContactMap class.

contacts: Add ContactMap class.

This is a simple map from contact ID to Contact object.  I'm
moving the merging code from GoogleContactStore into it, and
making both GoogleContactStore and FakeContactDatabase
(which formerly had ugly code that used a vector of
contacts) use it.

A later change will make GoogleContactStore drop deleted
contacts (hanging on to them indefinitely seems ugly).
While working on that, I noticed that FakeContactDatabase's
vector is lacking for supporting that and figured that it'd
be best to make it share GoogleContactStore's existing
merging code.

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

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

[satorux1, 2012-09-06 17:16:48]

LGTM with nits. sorry for the belated response. I'm back to work

http://codereview.chromium.org/10905033/diff/4001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/contact_map.cc (right):

http://codereview.chromium.org/10905033/diff/4001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_map.cc:25: void
ContactMap::Merge(scoped_ptr<ScopedVector<Contact> > updated_contacts,
scoped_ptr of ScopedVector looks a bit tricky, but this makes it clear that the
ownership of updated_contacts is transferred to this function, so this may be a
good idea.

I thought about using ScopedVector::release() in client code like:

  std::vector<Contact*> released;
  updated_contacts.release(&released);
  map->Merge(&released, true);

but not sure if this is any better.

http://codereview.chromium.org/10905033/diff/4001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/contact_map.h (right):

http://codereview.chromium.org/10905033/diff/4001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_map.h:47: bool drop_deleted);
boolean parameter like this can make callers cryptic (ex. Merge(foo, true) --
what does 'true' mean?) you might want to define a enum like
SHOULD_DROP_DELETED, KEEP_DELETED

http://codereview.chromium.org/10905033/diff/4001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/contact_map_unittest.cc (right):

http://codereview.chromium.org/10905033/diff/4001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_map_unittest.cc:22:
EXPECT_EQ(static_cast<size_t>(0), map.size());
Maybe just 0U

http://codereview.chromium.org/10905033/diff/4001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_map_unittest.cc:38:
EXPECT_EQ(static_cast<size_t>(1), map.size());
1U

[Daniel Erat, 2012-09-09 14:51:30]

Thanks!

http://codereview.chromium.org/10905033/diff/4001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/contact_map.cc (right):

http://codereview.chromium.org/10905033/diff/4001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_map.cc:25: void
ContactMap::Merge(scoped_ptr<ScopedVector<Contact> > updated_contacts,
On 2012/09/06 17:16:48, satorux1 wrote:
> scoped_ptr of ScopedVector looks a bit tricky, but this makes it clear that
the
> ownership of updated_contacts is transferred to this function, so this may be
a
> good idea.
> 
> I thought about using ScopedVector::release() in client code like:
> 
>   std::vector<Contact*> released;
>   updated_contacts.release(&released);
>   map->Merge(&released, true);
> 
> but not sure if this is any better.

Yeah, I think that scoped_ptr makes the ownership clearer.

http://codereview.chromium.org/10905033/diff/4001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/contact_map.h (right):

http://codereview.chromium.org/10905033/diff/4001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_map.h:47: bool drop_deleted);
On 2012/09/06 17:16:48, satorux1 wrote:
> boolean parameter like this can make callers cryptic (ex. Merge(foo, true) --
> what does 'true' mean?) you might want to define a enum like
> SHOULD_DROP_DELETED, KEEP_DELETED

Good suggestion; done.

http://codereview.chromium.org/10905033/diff/4001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/contact_map_unittest.cc (right):

http://codereview.chromium.org/10905033/diff/4001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_map_unittest.cc:22:
EXPECT_EQ(static_cast<size_t>(0), map.size());
On 2012/09/06 17:16:48, satorux1 wrote:
> Maybe just 0U

Done.

I wasn't sure that this is portable, since I don't think that %u works when
passing a size_t to printf() (presumably because int is still 32 bits on a
64-bit machine).  I've seen it done elsewhere, though, and it seems to work here
too.  I guess it makes sense that the compiler should be okay with the two terms
being different sizes in a comparison like this.

http://codereview.chromium.org/10905033/diff/4001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_map_unittest.cc:38:
EXPECT_EQ(static_cast<size_t>(1), map.size());
On 2012/09/06 17:16:48, satorux1 wrote:
> 1U

Done.

[commit-bot: I haz the power, 2012-09-09 14:52:13]

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

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

Presubmit check for 10905033-4003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.3s to calculate.

[Daniel Erat, 2012-09-09 14:53:35]

TBR sky for *.gypi

[commit-bot: I haz the power, 2012-09-09 14:53:51]

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

[commit-bot: I haz the power, 2012-09-09 18:23:25]

Change committed as 155634

[tfarina, 2012-09-09 19:01:10]

https://chromiumcodereview.appspot.com/10905033/diff/4003/chrome/browser/chro...
File chrome/browser/chromeos/contacts/contact_map.h (right):

https://chromiumcodereview.appspot.com/10905033/diff/4003/chrome/browser/chro...
chrome/browser/chromeos/contacts/contact_map.h:48: const Contact* Find(const
std::string& contact_id) const;
nit: may be GetContactById?

https://chromiumcodereview.appspot.com/10905033/diff/4003/chrome/browser/chro...
chrome/browser/chromeos/contacts/contact_map.h:51: void Clear();
nit: may be DeleteAll()?

[tfarina, 2012-09-09 19:04:29]

https://chromiumcodereview.appspot.com/10905033/diff/4003/chrome/browser/chro...
File chrome/browser/chromeos/contacts/contact_map_unittest.cc (right):

https://chromiumcodereview.appspot.com/10905033/diff/4003/chrome/browser/chro...
chrome/browser/chromeos/contacts/contact_map_unittest.cc:17: typedef
testing::Test ContactMapTest;
this is not necessary if you use TEST() macro!

[tfarina, 2012-09-09 19:20:49]

https://chromiumcodereview.appspot.com/10905033/diff/4003/chrome/browser/chro...
File chrome/browser/chromeos/contacts/contact_map.cc (right):

https://chromiumcodereview.appspot.com/10905033/diff/4003/chrome/browser/chro...
chrome/browser/chromeos/contacts/contact_map.cc:22: contacts_.clear();
STLDeleteValues already does this for you, no?

[Daniel Erat, 2012-09-10 15:01:43]

https://chromiumcodereview.appspot.com/10905033/diff/4003/chrome/browser/chro...
File chrome/browser/chromeos/contacts/contact_map.cc (right):

https://chromiumcodereview.appspot.com/10905033/diff/4003/chrome/browser/chro...
chrome/browser/chromeos/contacts/contact_map.cc:22: contacts_.clear();
On 2012/09/09 19:20:49, tfarina wrote:
> STLDeleteValues already does this for you, no?

Thanks, this was unexpected to me.  Done.

https://chromiumcodereview.appspot.com/10905033/diff/4003/chrome/browser/chro...
File chrome/browser/chromeos/contacts/contact_map.h (right):

https://chromiumcodereview.appspot.com/10905033/diff/4003/chrome/browser/chro...
chrome/browser/chromeos/contacts/contact_map.h:48: const Contact* Find(const
std::string& contact_id) const;
On 2012/09/09 19:01:10, tfarina wrote:
> nit: may be GetContactById?

Since this is really just a glorified wrapper around STL map, I prefer using the
same method names.

https://chromiumcodereview.appspot.com/10905033/diff/4003/chrome/browser/chro...
chrome/browser/chromeos/contacts/contact_map.h:51: void Clear();
On 2012/09/09 19:01:10, tfarina wrote:
> nit: may be DeleteAll()?

Same here.

https://chromiumcodereview.appspot.com/10905033/diff/4003/chrome/browser/chro...
File chrome/browser/chromeos/contacts/contact_map_unittest.cc (right):

https://chromiumcodereview.appspot.com/10905033/diff/4003/chrome/browser/chro...
chrome/browser/chromeos/contacts/contact_map_unittest.cc:17: typedef
testing::Test ContactMapTest;
On 2012/09/09 19:04:29, tfarina wrote:
> this is not necessary if you use TEST() macro!

Thanks, done.