contacts: Add ContactManager.

contacts: Add ContactManager.

This adds a singleton class that holds ContactStore objects
and exposes Contact objects to the rest of the browser.

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

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

[Daniel Erat, 2012-08-03 20:58:12]

This is the last part. :-)

[satorux1, 2012-08-03 21:16:22]

http://codereview.chromium.org/10831162/diff/5001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/contact_manager.cc (right):

http://codereview.chromium.org/10831162/diff/5001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_manager.cc:178:
store->AddObserver(this);
I don't see store->RemoveObserver(this); Usually they should be paired.

http://codereview.chromium.org/10831162/diff/5001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/contact_manager.h (right):

http://codereview.chromium.org/10831162/diff/5001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_manager.h:42: void
set_contact_store_factory_for_testing(
maybe SetContactStore... because the function is not inlined?

http://codereview.chromium.org/10831162/diff/5001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_manager.h:83: static ContactManager*
instance_;
 matter of taste of the day: you might want to put it in an anonymous space in
.cc file in favor of less things in .h.

http://codereview.chromium.org/10831162/diff/5001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/contact_manager_observer.h (right):

http://codereview.chromium.org/10831162/diff/5001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_manager_observer.h:21: virtual void
OnContactsUpdated(ContactManager* manager, Profile* profile) = 0;
Do you need to pass |manager|? Usually observers have a pointer to the object.

[Daniel Erat, 2012-08-03 22:12:07]

Please ignore PS4; it was uploaded pre-merge (there's no way to delete
patchsets, is there?).

http://codereview.chromium.org/10831162/diff/5001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/contact_manager.cc (right):

http://codereview.chromium.org/10831162/diff/5001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_manager.cc:43: // message loop is
already getting torn down at this point.
I think that I can avoid this by just destroying the ContactManager earlier in
ChromeBrowserMainPartsChromeos.

http://codereview.chromium.org/10831162/diff/5001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_manager.cc:178:
store->AddObserver(this);
On 2012/08/03 21:16:22, satorux1 wrote:
> I don't see store->RemoveObserver(this); Usually they should be paired. 

Heh, went back and forth on this.  Updated.

http://codereview.chromium.org/10831162/diff/5001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/contact_manager.h (right):

http://codereview.chromium.org/10831162/diff/5001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_manager.h:42: void
set_contact_store_factory_for_testing(
On 2012/08/03 21:16:22, satorux1 wrote:
> maybe SetContactStore... because the function is not inlined?

Done.

http://codereview.chromium.org/10831162/diff/5001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_manager.h:83: static ContactManager*
instance_;
On 2012/08/03 21:16:22, satorux1 wrote:
>  matter of taste of the day: you might want to put it in an anonymous space in
> .cc file in favor of less things in .h.

Done.

http://codereview.chromium.org/10831162/diff/5001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/contact_manager_observer.h (right):

http://codereview.chromium.org/10831162/diff/5001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_manager_observer.h:21: virtual void
OnContactsUpdated(ContactManager* manager, Profile* profile) = 0;
On 2012/08/03 21:16:22, satorux1 wrote:
> Do you need to pass |manager|? Usually observers have a pointer to the object.

You're right; since ContactManager is a singleton there's no reason to tell the
observer where the notification came from.  Done.

[satorux1, 2012-08-03 22:18:27]

LGTM with a nit

http://codereview.chromium.org/10831162/diff/9001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/contact_manager.cc (right):

http://codereview.chromium.org/10831162/diff/9001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_manager.cc:26: ContactManager* instance
= NULL;
please add g_ or global_ prefix per the style guide.

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

http://codereview.chromium.org/10831162/diff/9001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/contact_manager.cc (right):

http://codereview.chromium.org/10831162/diff/9001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_manager.cc:26: ContactManager* instance
= NULL;
On 2012/08/03 22:18:28, satorux1 wrote:
> please add g_ or global_ prefix per the style guide.

Sure (wasn't sure if this applies to variables in an anonymous namespace or just
exported symbols).

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

TBR ben for *.gypi

[commit-bot: I haz the power, 2012-08-03 22:33:28]

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

[commit-bot: I haz the power, 2012-08-03 22:47:58]

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

[commit-bot: I haz the power, 2012-08-04 00:09:20]

Change committed as 149980