contacts: Add UpdateMetadata protocol buffer.

contacts: Add UpdateMetadata protocol buffer.

This adds a new message containing update-related
information and updates the ContactDatabase class to read
and write it.

BUG=128805
TEST=none


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

[satorux1, 2012-08-01 20:12:32]

LGTM with optional suggestions

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

http://codereview.chromium.org/10836043/diff/4001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_database.cc:97: contacts_ptr,
were you bitten by doing  contacts.get() here? This is subtle...

for objects used for PostTaskAndReply, raw pointers with base::Owned() may be a
better approach. just my 2c.

http://codereview.chromium.org/10836043/diff/4001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_database.cc:199: if (key !=
kUpdateMetadataKey)
do you plan to add more special keys? If so, checking by a prefix may be better
like StartsWith(key, kSpecialKeyPrefix)

Another approach would be to use two separate prefixes: 1) for provider ids, 2)
for metadata

However, the 2nd approach breaks the compatibility with the exsting database
file. not sure if it matters, though.

http://codereview.chromium.org/10836043/diff/4001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_database.cc:214: if
(contact.provider_id() == kUpdateMetadataKey) {
this may be a bit dirty? if we are to use two separate prefixes, we don't need
this.

[Daniel Erat, 2012-08-01 20:22:01]

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

http://codereview.chromium.org/10836043/diff/4001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_database.cc:97: contacts_ptr,
On 2012/08/01 20:12:32, satorux1 wrote:
> were you bitten by doing  contacts.get() here? This is subtle...
> 
> for objects used for PostTaskAndReply, raw pointers with base::Owned() may be
a
> better approach. just my 2c.

Yeah, I was. :-(

I think I still prefer scoped_ptr, since it makes it clear to the caller that
they're taking ownership of the contacts.

I'll add a comment, if that's okay.

http://codereview.chromium.org/10836043/diff/4001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_database.cc:199: if (key !=
kUpdateMetadataKey)
On 2012/08/01 20:12:32, satorux1 wrote:
> do you plan to add more special keys? If so, checking by a prefix may be
better
> like StartsWith(key, kSpecialKeyPrefix)
> 
> Another approach would be to use two separate prefixes: 1) for provider ids,
2)
> for metadata
> 
> However, the 2nd approach breaks the compatibility with the exsting database
> file. not sure if it matters, though.

I don't have any plans to add more, but if it happens, using "__chrome_" as a
prefix seems okay to me.

http://codereview.chromium.org/10836043/diff/4001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_database.cc:214: if
(contact.provider_id() == kUpdateMetadataKey) {
On 2012/08/01 20:12:32, satorux1 wrote:
> this may be a bit dirty? if we are to use two separate prefixes, we don't need
> this.

I think it might be overkill, but I'll change it if you feel strongly. :-)

[satorux1, 2012-08-01 20:37:05]

LGTM

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

http://codereview.chromium.org/10836043/diff/4001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_database.cc:203: }
BTW, is there any better way to clear the table than removing every entry? would
be nice if we have something like db_->DeleteAll()

http://codereview.chromium.org/10836043/diff/4001/chrome/browser/chromeos/con...
chrome/browser/chromeos/contacts/contact_database.cc:214: if
(contact.provider_id() == kUpdateMetadataKey) {
On 2012/08/01 20:22:02, Daniel Erat wrote:
> On 2012/08/01 20:12:32, satorux1 wrote:
> > this may be a bit dirty? if we are to use two separate prefixes, we don't
need
> > this.
> 
> I think it might be overkill, but I'll change it if you feel strongly. :-)

No, I don't feel strongly

[commit-bot: I haz the power, 2012-08-01 21:10:04]

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

[commit-bot: I haz the power, 2012-08-01 22:39:48]

Change committed as 149521