contacts: Add Contact struct and test functions.

contacts: Add Contact struct and test functions.

This adds a (currently-Chrome-OS-only) Contact struct,
loosely patterned on Google's GData Contact kind v2.0.

It also adds test functions related to this struct.

BUG=128805
TEST=tested in conjunction with later code


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

[Daniel Erat, 2012-07-19 21:44:00]

https://chromiumcodereview.appspot.com/10803045/diff/1001/chrome/browser/chro...
File chrome/browser/chromeos/contacts/contact.cc (right):

https://chromiumcodereview.appspot.com/10803045/diff/1001/chrome/browser/chro...
chrome/browser/chromeos/contacts/contact.cc:5: #include
"chrome/browser/chromeos/contacts/contact.h"
Ugh, I'm wondering if this incorrect move detection means that I need to use svn
to upload this change. :-(

[Daniel Erat, 2012-07-19 21:49:00]

+kochi

[satorux1, 2012-07-20 17:42:57]

http://codereview.chromium.org/10803045/diff/1001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/contact.cc (right):

http://codereview.chromium.org/10803045/diff/1001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact.cc:5: #include
"chrome/browser/chromeos/contacts/contact.h"
On 2012/07/19 21:44:00, Daniel Erat wrote:
> Ugh, I'm wondering if this incorrect move detection means that I need to use
svn
> to upload this change. :-(

haha, this is nice. :)

http://codereview.chromium.org/10803045/diff/1001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/contact.h (right):

http://codereview.chromium.org/10803045/diff/1001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact.h:32: };
you might want to add a constructor for this. otherwise, relation can be
referenced before initialized.

http://codereview.chromium.org/10803045/diff/1001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact.h:37: bool primary;
ditto. I'm wary of a struct with a primitive type member without a constructor.

http://codereview.chromium.org/10803045/diff/1001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/contact_test_lib.cc (right):

http://codereview.chromium.org/10803045/diff/1001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_test_lib.cc:28:
base::Callback<std::string(const T&)> stringify_callback,
oh this is a cool use of callback!

http://codereview.chromium.org/10803045/diff/1001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/contact_test_lib.h (right):

http://codereview.chromium.org/10803045/diff/1001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_test_lib.h:5: #ifndef
CHROME_BROWSER_CHROMEOS_CONTACTS_CONTACT_TEST_LIB_H_
maybe contact_test_util.h would sound slightly nicer?

http://codereview.chromium.org/10803045/diff/1001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_test_lib.h:37: void CopyContacts(const
ScopedVector<Contact>& source,
give them different names?

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

http://codereview.chromium.org/10803045/diff/1001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_test_lib.h:44: bool deleted,
could you document the three parameters?

[Daniel Erat, 2012-07-23 16:55:47]

http://codereview.chromium.org/10803045/diff/1001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/contact.cc (right):

http://codereview.chromium.org/10803045/diff/1001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact.cc:5: #include
"chrome/browser/chromeos/contacts/contact.h"
On 2012/07/20 17:42:57, satorux1 wrote:
> On 2012/07/19 21:44:00, Daniel Erat wrote:
> > Ugh, I'm wondering if this incorrect move detection means that I need to use
> svn
> > to upload this change. :-(
> 
> haha, this is nice. :)

I think that adding more constructors will fix this. :-)

http://codereview.chromium.org/10803045/diff/1001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/contact.h (right):

http://codereview.chromium.org/10803045/diff/1001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact.h:32: };
On 2012/07/20 17:42:57, satorux1 wrote:
> you might want to add a constructor for this. otherwise, relation can be
> referenced before initialized.

Done.

http://codereview.chromium.org/10803045/diff/1001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact.h:37: bool primary;
On 2012/07/20 17:42:57, satorux1 wrote:
> ditto. I'm wary of a struct with a primitive type member without a
constructor.

Done.

http://codereview.chromium.org/10803045/diff/1001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/contact_test_lib.h (right):

http://codereview.chromium.org/10803045/diff/1001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_test_lib.h:5: #ifndef
CHROME_BROWSER_CHROMEOS_CONTACTS_CONTACT_TEST_LIB_H_
On 2012/07/20 17:42:57, satorux1 wrote:
> maybe contact_test_util.h would sound slightly nicer?

Done.

http://codereview.chromium.org/10803045/diff/1001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_test_lib.h:37: void CopyContacts(const
ScopedVector<Contact>& source,
On 2012/07/20 17:42:57, satorux1 wrote:
> give them different names?
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Are you sure that this doesn't fall under the "a call site can get a good idea
of what is happening without having to first figure out exactly which overload
is being called" case?  It feels pretty close to the string vs. const char*
example, in that both versions do the same thing.

http://codereview.chromium.org/10803045/diff/1001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_test_lib.h:44: bool deleted,
On 2012/07/20 17:42:57, satorux1 wrote:
> could you document the three parameters?

Done.

[satorux1, 2012-07-23 19:39:50]

LGTM

http://codereview.chromium.org/10803045/diff/1001/chrome/browser/chromeos/con...
File chrome/browser/chromeos/contacts/contact_test_lib.h (right):

http://codereview.chromium.org/10803045/diff/1001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_test_lib.h:37: void CopyContacts(const
ScopedVector<Contact>& source,
On 2012/07/23 16:55:47, Daniel Erat wrote:
> On 2012/07/20 17:42:57, satorux1 wrote:
> > give them different names?
> > 
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> 
> Are you sure that this doesn't fall under the "a call site can get a good idea
> of what is happening without having to first figure out exactly which overload
> is being called" case?  It feels pretty close to the string vs. const char*
> example, in that both versions do the same thing.

Oh I see. then it's fine.

http://codereview.chromium.org/10803045/diff/11001/chrome/browser/chromeos/co...
File chrome/browser/chromeos/contacts/contact_test_util.h (right):

http://codereview.chromium.org/10803045/diff/11001/chrome/browser/chromeos/co...
chrome/browser/chromeos/contacts/contact_test_util.h:80: // Initializes
|contact|'s photo to a bitmap of the given size.
the size may be a bit confusing. is it a height/width of a square image?
gspencer@ named it something like |side_length|

[Daniel Erat, 2012-07-23 19:52:55]

http://codereview.chromium.org/10803045/diff/11001/chrome/browser/chromeos/co...
File chrome/browser/chromeos/contacts/contact_test_util.h (right):

http://codereview.chromium.org/10803045/diff/11001/chrome/browser/chromeos/co...
chrome/browser/chromeos/contacts/contact_test_util.h:80: // Initializes
|contact|'s photo to a bitmap of the given size.
On 2012/07/23 19:39:51, satorux1 wrote:
> the size may be a bit confusing. is it a height/width of a square image?
> gspencer@ named it something like |side_length|

I think that the images we get back from Google are usually square, although
they're not guaranteed to be so.  This function is just for testing, though; I'm
using it to make sure that when we load a contact, we load the photo correctly
as well -- see how ContactToString() includes the photo dimensions.  I suppose I
could take a hash of the image data instead and include that in the string, but
the dimensions seem a bit more human-friendly when diagnosing test failures. 
I'll elaborate on this in the comment.

[commit-bot: I haz the power, 2012-07-23 20:32:37]

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

[commit-bot: I haz the power, 2012-07-23 22:08:49]

Change committed as 147960