contacts: Add GDataContactsService.

contacts: Add GDataContactsService.

This adds a class for fetching a user's Google contacts via
the Contacts API.

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

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

[Daniel Erat, 2012-07-24 17:47:07]

Sorry, this is probably the largest of the changes.  I can split the
gdata_operations and fake class into separate changes if you'd like, although
I'm not sure whether that'd help or just make it harder to review.

Once it's reviewed, I'll check in the test data first so that the rest of this
can go in via the CQ.

[satorux1, 2012-07-24 18:25:51]

http://codereview.chromium.org/10818017/diff/4001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/fake_gdata_contacts_service.h (right):

http://codereview.chromium.org/10818017/diff/4001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/fake_gdata_contacts_service.h:21: class
FakeGDataContactsService : public GDataContactsServiceInterface {
class comment is missing. is this only used for testing?

I guess we can use gmock for this, but setting expectations for functions that
take callbacks is not easy with gmock, so doing this without gmock may be
easier, though gmock is heavily used in this directory.

BTW, I'm always confused about the difference between fake and mock
implementations. Why is this Fake not Mock?

http://codereview.chromium.org/10818017/diff/4001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/fake_gdata_contacts_service.h:30: void
SetContacts(const contacts::ContactPointers& contacts,
function comment?

http://codereview.chromium.org/10818017/diff/4001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_contacts_service.cc (right):

http://codereview.chromium.org/10818017/diff/4001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_contacts_service.cc:174: bool
GetPhotoUrl(const DictionaryValue& dict, std::string* url) {
it may be slightly simpler to return an empty string on error, than a bool +
string*?

http://codereview.chromium.org/10818017/diff/4001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_contacts_service.cc:204: bool
FillContactFromDictionary(const base::DictionaryValue& dict,
this function is lengthy. Would it make sense to use
base/json/json_value_converter.h, with which you can parse JSON values in a
declarative manner?

For Drive stuff, json_value_converter.h reduced a lot of code from a
hand-written parser.

http://codereview.chromium.org/10818017/diff/4001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_contacts_service.h (right):

http://codereview.chromium.org/10818017/diff/4001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_contacts_service.h:47: //
min_update_time.is_null() is true, all contacts will be returned.
I'm curious. Why is |min_update_time| parameter is needed?

[Daniel Erat, 2012-07-24 20:37:21]

https://chromiumcodereview.appspot.com/10818017/diff/4001/chrome/browser/chro...
File chrome/browser/chromeos/gdata/fake_gdata_contacts_service.h (right):

https://chromiumcodereview.appspot.com/10818017/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/gdata/fake_gdata_contacts_service.h:21: class
FakeGDataContactsService : public GDataContactsServiceInterface {
On 2012/07/24 18:25:51, satorux1 wrote:
> class comment is missing. is this only used for testing?
> 
> I guess we can use gmock for this, but setting expectations for functions that
> take callbacks is not easy with gmock, so doing this without gmock may be
> easier, though gmock is heavily used in this directory.

I'm really not a Gmock fan. :-)

> BTW, I'm always confused about the difference between fake and mock
> implementations. Why is this Fake not Mock?

I'm always confused about this too.  The internal writing that I looked at was
similar to http://martinfowler.com/articles/mocksArentStubs.html:

"Dummy objects are passed around but never actually used. Usually they are just
used to fill parameter lists.

Fake objects actually have working implementations, but usually take some
shortcut which makes them not suitable for production (an in memory database is
a good example).

Stubs provide canned answers to calls made during the test, usually not
responding at all to anything outside what's programmed in for the test. Stubs
may also record information about calls, such as an email gateway stub that
remembers the messages it 'sent', or maybe only how many messages it 'sent'.

Mocks are what we are talking about here: objects pre-programmed with
expectations which form a specification of the calls they are expected to
receive."

So "mock" applies to Gmock, since Gmock lets/makes you supply expectations for
how your class will be invoked.  This class doesn't do that.  "Stub" is probably
more accurate than "Fake" here.  I'll rename this to GDataContactsServiceStub
and add a comment, if that sounds okay to you.

https://chromiumcodereview.appspot.com/10818017/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/gdata/fake_gdata_contacts_service.h:30: void
SetContacts(const contacts::ContactPointers& contacts,
On 2012/07/24 18:25:51, satorux1 wrote:
> function comment?

Done.

https://chromiumcodereview.appspot.com/10818017/diff/4001/chrome/browser/chro...
File chrome/browser/chromeos/gdata/gdata_contacts_service.cc (right):

https://chromiumcodereview.appspot.com/10818017/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_contacts_service.cc:174: bool
GetPhotoUrl(const DictionaryValue& dict, std::string* url) {
On 2012/07/24 18:25:51, satorux1 wrote:
> it may be slightly simpler to return an empty string on error, than a bool +
> string*?

Done.

https://chromiumcodereview.appspot.com/10818017/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_contacts_service.cc:204: bool
FillContactFromDictionary(const base::DictionaryValue& dict,
On 2012/07/24 18:25:51, satorux1 wrote:
> this function is lengthy. Would it make sense to use
> base/json/json_value_converter.h, with which you can parse JSON values in a
> declarative manner?
> 
> For Drive stuff, json_value_converter.h reduced a lot of code from a
> hand-written parser.

Heh, see my very old, abandoned change at
https://chromiumcodereview.appspot.com/10440101/.  I'll also forward you the
email discussion that Jun and I had about this before I ultimately decided that
JSONValueConverter wouldn't save much work.

I could probably move the similar email/phone/address/im code into a templated
function that also takes a callback to handle the additional protocol field for
IM addresses, if you think it'd improve things.  Templates often make things
worse instead of better, though. :-)

https://chromiumcodereview.appspot.com/10818017/diff/4001/chrome/browser/chro...
File chrome/browser/chromeos/gdata/gdata_contacts_service.h (right):

https://chromiumcodereview.appspot.com/10818017/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_contacts_service.h:47: //
min_update_time.is_null() is true, all contacts will be returned.
On 2012/07/24 18:25:51, satorux1 wrote:
> I'm curious. Why is |min_update_time| parameter is needed?

This lets us do incremental updates.  Jumping ahead a bit, if you look at
GoogleContactSource::UpdateContacts() in
http://codereview.chromium.org/10542076/diff/36001/chrome/browser/chromeos/co...,
I'm keeping track of the max update time and using that to set a lower bound on
the contacts that we need to load in future updates.

[satorux1, 2012-07-24 21:34:29]

http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_contacts_service.cc (right):

http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_contacts_service.cc:128: // Initializes a
AddressType struct given a dictionary representing a single
nit: an AddressType

http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_contacts_service.cc:380: contact->photo =
decoded_image;
just curious but how big is the decoded image in bytes? If the photo is
150x150xRGBA, then it'll be about 90KB. If I have 1000 friends, which I don't
have but you might have, it'll be 90MB in memory. This will also consume a
considerable amount of space on the disk, if cached locally.

Would it make sense to keep the image in the compressed form (JPEG?), and decode
them only when we show them?

http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_contacts_service.cc:489: if
(!service_->feed_url_for_testing_.is_empty()) {
did you mean rewrite_photo_url_callback_for_testing_.is_empty()?

http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_contacts_service.cc:491:
service_->rewrite_photo_url_callback_for_testing_.Run(photo_url);
This is tricky. Might want to comment why we need to rewrite the URL for
testing.

http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_contacts_service.cc:536:
base::Unretained(this),
It it safe to use base::Unretained() here? I guess you can easily use WeakPtr
here.

http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_contacts_service.cc:537:
base::Unretained(contact))));
hmm, using base::Unretained() for the 2nd parameter of base::Bind()? It's new to
me. Is it necessary?

http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_contacts_service_stub.h (right):

http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_contacts_service_stub.h:23: class
GDataContactsServiceStub : public GDataContactsServiceInterface {
Is this class used in this patch? Otherwise, you might want to submit this in a
separate patch to make the gigantic patch less gigantic.

[Daniel Erat, 2012-07-27 16:54:19]

I've applied your feedback here, but I've also applied your IM suggestion of
using protocol buffers and leveldb.  I'll send you another change first that
replaces the currently-checked-in Contact class and related test utils with a
protobuf, and then upload an updated version of this change, if that sounds
reasonable.

http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_contacts_service.cc (right):

http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_contacts_service.cc:128: // Initializes a
AddressType struct given a dictionary representing a single
On 2012/07/24 21:34:29, satorux1 wrote:
> nit: an AddressType

Done.

http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_contacts_service.cc:380: contact->photo =
decoded_image;
On 2012/07/24 21:34:29, satorux1 wrote:
> just curious but how big is the decoded image in bytes? If the photo is
> 150x150xRGBA, then it'll be about 90KB. If I have 1000 friends, which I don't
> have but you might have, it'll be 90MB in memory. This will also consume a
> considerable amount of space on the disk, if cached locally.
> 
> Would it make sense to keep the image in the compressed form (JPEG?), and
decode
> them only when we show them?

I've switched this to re-encode the image as a PNG and store that instead. 
Storing it in the original compressed form seems like a non-option; we don't
necessarily trust this data, so I think that we'd always need to decode it
within a sandbox before displaying it.

http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_contacts_service.cc:489: if
(!service_->feed_url_for_testing_.is_empty()) {
On 2012/07/24 21:34:29, satorux1 wrote:
> did you mean rewrite_photo_url_callback_for_testing_.is_empty()?

No.  Callbacks can't be empty, so I was using this as a proxy.  I agree that
this is ugly, so I've added another has_rewrite_photo_url_callback_for_testing_
member.

http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_contacts_service.cc:491:
service_->rewrite_photo_url_callback_for_testing_.Run(photo_url);
On 2012/07/24 21:34:29, satorux1 wrote:
> This is tricky. Might want to comment why we need to rewrite the URL for
> testing.

Added a comment to the header.

http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_contacts_service.cc:536:
base::Unretained(this),
On 2012/07/24 21:34:29, satorux1 wrote:
> It it safe to use base::Unretained() here? I guess you can easily use WeakPtr
> here.

Is your concern that the GetContactPhotoOperation can outlive the
DownloadContactsRequest?  I thought that this wasn't a risk, since
GDataContactsService's d'tor cancels all operations along with deleting all
requests.  Additionally using WeakPtr seems fine to me, though, so added.

http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_contacts_service.cc:537:
base::Unretained(contact))));
On 2012/07/24 21:34:29, satorux1 wrote:
> hmm, using base::Unretained() for the 2nd parameter of base::Bind()? It's new
to
> me. Is it necessary?

I don't think so; deleted.

http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_contacts_service_stub.h (right):

http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_contacts_service_stub.h:23: class
GDataContactsServiceStub : public GDataContactsServiceInterface {
On 2012/07/24 21:34:29, satorux1 wrote:
> Is this class used in this patch? Otherwise, you might want to submit this in
a
> separate patch to make the gigantic patch less gigantic.

Sure, I'll defer it for a later change.

[satorux1, 2012-07-27 16:57:04]

On 2012/07/27 16:54:19, Daniel Erat wrote:
> I've applied your feedback here, but I've also applied your IM suggestion of
> using protocol buffers and leveldb.  I'll send you another change first that
> replaces the currently-checked-in Contact class and related test utils with a
> protobuf, and then upload an updated version of this change, if that sounds
> reasonable.

sounds good. please send it to me.

> 
>
http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
> File chrome/browser/chromeos/gdata/gdata_contacts_service.cc (right):
> 
>
http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
> chrome/browser/chromeos/gdata/gdata_contacts_service.cc:128: // Initializes a
> AddressType struct given a dictionary representing a single
> On 2012/07/24 21:34:29, satorux1 wrote:
> > nit: an AddressType
> 
> Done.
> 
>
http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
> chrome/browser/chromeos/gdata/gdata_contacts_service.cc:380: contact->photo =
> decoded_image;
> On 2012/07/24 21:34:29, satorux1 wrote:
> > just curious but how big is the decoded image in bytes? If the photo is
> > 150x150xRGBA, then it'll be about 90KB. If I have 1000 friends, which I
don't
> > have but you might have, it'll be 90MB in memory. This will also consume a
> > considerable amount of space on the disk, if cached locally.
> > 
> > Would it make sense to keep the image in the compressed form (JPEG?), and
> decode
> > them only when we show them?
> 
> I've switched this to re-encode the image as a PNG and store that instead. 
> Storing it in the original compressed form seems like a non-option; we don't
> necessarily trust this data, so I think that we'd always need to decode it
> within a sandbox before displaying it.
> 
>
http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
> chrome/browser/chromeos/gdata/gdata_contacts_service.cc:489: if
> (!service_->feed_url_for_testing_.is_empty()) {
> On 2012/07/24 21:34:29, satorux1 wrote:
> > did you mean rewrite_photo_url_callback_for_testing_.is_empty()?
> 
> No.  Callbacks can't be empty, so I was using this as a proxy.  I agree that
> this is ugly, so I've added another
has_rewrite_photo_url_callback_for_testing_
> member.
> 
>
http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
> chrome/browser/chromeos/gdata/gdata_contacts_service.cc:491:
> service_->rewrite_photo_url_callback_for_testing_.Run(photo_url);
> On 2012/07/24 21:34:29, satorux1 wrote:
> > This is tricky. Might want to comment why we need to rewrite the URL for
> > testing.
> 
> Added a comment to the header.
> 
>
http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
> chrome/browser/chromeos/gdata/gdata_contacts_service.cc:536:
> base::Unretained(this),
> On 2012/07/24 21:34:29, satorux1 wrote:
> > It it safe to use base::Unretained() here? I guess you can easily use
WeakPtr
> > here.
> 
> Is your concern that the GetContactPhotoOperation can outlive the
> DownloadContactsRequest?  I thought that this wasn't a risk, since
> GDataContactsService's d'tor cancels all operations along with deleting all
> requests.  Additionally using WeakPtr seems fine to me, though, so added.
> 
>
http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
> chrome/browser/chromeos/gdata/gdata_contacts_service.cc:537:
> base::Unretained(contact))));
> On 2012/07/24 21:34:29, satorux1 wrote:
> > hmm, using base::Unretained() for the 2nd parameter of base::Bind()? It's
new
> to
> > me. Is it necessary?
> 
> I don't think so; deleted.
> 
>
http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
> File chrome/browser/chromeos/gdata/gdata_contacts_service_stub.h (right):
> 
>
http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
> chrome/browser/chromeos/gdata/gdata_contacts_service_stub.h:23: class
> GDataContactsServiceStub : public GDataContactsServiceInterface {
> On 2012/07/24 21:34:29, satorux1 wrote:
> > Is this class used in this patch? Otherwise, you might want to submit this
in
> a
> > separate patch to make the gigantic patch less gigantic.
> 
> Sure, I'll defer it for a later change.

[Daniel Erat, 2012-07-27 23:55:25]

Another look?  The changes from patch set 5 are pretty minimal (and mostly
involve deleting code, since I'm deferring the image decoding -- Thanks for the
suggestion!  It makes the GData code much simpler and doesn't seem to cause
performance issues in the UI).

[satorux1, 2012-07-28 00:38:23]

LGTM with a nit:

http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_contacts_service.cc (right):

http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_contacts_service.cc:489: if
(!service_->feed_url_for_testing_.is_empty()) {
On 2012/07/27 16:54:19, Daniel Erat wrote:
> On 2012/07/24 21:34:29, satorux1 wrote:
> > did you mean rewrite_photo_url_callback_for_testing_.is_empty()?
> 
> No.  Callbacks can't be empty, so I was using this as a proxy.  I agree that
> this is ugly, so I've added another
has_rewrite_photo_url_callback_for_testing_
> member.

I meant rewrite_photo_url_callback_for_testing_.is_null(). you probably don't
need has_rewrite_photo_url_callback_for_testing_

[Daniel Erat, 2012-07-29 19:54:25]

http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_contacts_service.cc (right):

http://codereview.chromium.org/10818017/diff/10001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_contacts_service.cc:489: if
(!service_->feed_url_for_testing_.is_empty()) {
On 2012/07/28 00:38:23, satorux1 wrote:
> On 2012/07/27 16:54:19, Daniel Erat wrote:
> > On 2012/07/24 21:34:29, satorux1 wrote:
> > > did you mean rewrite_photo_url_callback_for_testing_.is_empty()?
> > 
> > No.  Callbacks can't be empty, so I was using this as a proxy.  I agree that
> > this is ugly, so I've added another
> has_rewrite_photo_url_callback_for_testing_
> > member.
> 
> I meant rewrite_photo_url_callback_for_testing_.is_null(). you probably don't
> need has_rewrite_photo_url_callback_for_testing_

Ah, thanks.  I'd looked for is_null() in base/callback.h earlier but didn't find
it.  Wasn't aware of callback_internal.h. :-P

[commit-bot: I haz the power, 2012-07-30 15:23:07]

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

[commit-bot: I haz the power, 2012-07-30 15:23:12]

Presubmit check for 10818017-18009 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  chrome/browser/chromeos/gdata/gdata_contacts_service.cc:357
    operation->set_feed_url_for_testing(service_->feed_url_for_testing_);

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

Presubmit checks took 1.2s to calculate.

[Daniel Erat, 2012-07-30 15:25:26]

TBR Ben for chrome/*.gypi

[commit-bot: I haz the power, 2012-07-30 15:25:39]

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

[commit-bot: I haz the power, 2012-07-30 18:15:50]

Try job failure for 10818017-18009 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-07-30 18:44:57]

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

[commit-bot: I haz the power, 2012-07-30 20:00:50]

Change committed as 149005