contacts: Rate-limit GData photo requests and handle 404s.

contacts: Rate-limit GData photo requests and handle 404s.

The API returns 503 errors if we try to download photos too
quickly.  This change adds cheesy rate-limiting so we don't
request more than 10 photos per second (which appears to be
the undocumented limit) and also makes us retry when we get
a 503.

I'm also making GDataContactsService report success even
when we got 404 errors for some photos, as I see this happen
with my personal account. :-/

BUG=128805
TEST=none


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

[Daniel Erat, 2012-08-06 16:25:33]

https://chromiumcodereview.appspot.com/10823182/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/gdata/gdata_contacts_service_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10823182/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/gdata/gdata_contacts_service_browsertest.cc:245:
service()->set_max_photo_downloads_per_second_for_testing(6);
This means that the test will take at least a second to run. :-(

[satorux1, 2012-08-06 20:57:08]

LGTM with suggestions.

http://codereview.chromium.org/10823182/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_contacts_service.cc (right):

http://codereview.chromium.org/10823182/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_contacts_service.cc:544: << "for " <<
contact->provider_id() << "; giving up";
don't have to do it in the patch, but you might want to add some UMA for
failures like this.

http://codereview.chromium.org/10823182/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_contacts_service_browsertest.cc
(right):

http://codereview.chromium.org/10823182/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_contacts_service_browsertest.cc:245:
service()->set_max_photo_downloads_per_second_for_testing(6);
On 2012/08/06 16:25:34, Daniel Erat wrote:
> This means that the test will take at least a second to run. :-(

Might want to add some comment?

You might want to add some "for_testing" function that can tweak the interval of
RepeatingTimer

[Daniel Erat, 2012-08-06 23:12:21]

http://codereview.chromium.org/10823182/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_contacts_service.cc (right):

http://codereview.chromium.org/10823182/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_contacts_service.cc:544: << "for " <<
contact->provider_id() << "; giving up";
On 2012/08/06 20:57:08, satorux1 wrote:
> don't have to do it in the patch, but you might want to add some UMA for
> failures like this.

Yep, I plan to add a lot of metrics in a later patch for temporary and permanent
failures (along with histograms for things like total number of contacts and
time taken to download them).

http://codereview.chromium.org/10823182/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_contacts_service_browsertest.cc
(right):

http://codereview.chromium.org/10823182/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_contacts_service_browsertest.cc:245:
service()->set_max_photo_downloads_per_second_for_testing(6);
On 2012/08/06 20:57:08, satorux1 wrote:
> On 2012/08/06 16:25:34, Daniel Erat wrote:
> > This means that the test will take at least a second to run. :-(
> 
> Might want to add some comment?
> 
> You might want to add some "for_testing" function that can tweak the interval
of
> RepeatingTimer

Done.

[satorux1, 2012-08-06 23:49:55]

LGTM!

[commit-bot: I haz the power, 2012-08-07 00:25:49]

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

[commit-bot: I haz the power, 2012-08-07 02:20:21]

Failed to apply patch for
chrome/browser/chromeos/gdata/gdata_contacts_service.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/chromeos/gdata/gdata_contacts_service.cc
Hunk #2 succeeded at 30 (offset -1 lines).
Hunk #3 succeeded at 322 with fuzz 2 (offset -2 lines).
Hunk #4 succeeded at 329 (offset -3 lines).
Hunk #5 succeeded at 375 (offset -6 lines).
Hunk #6 succeeded at 472 (offset -6 lines).
Hunk #7 succeeded at 480 (offset -6 lines).
Hunk #8 succeeded at 515 (offset -7 lines).
Hunk #9 succeeded at 561 (offset -8 lines).
Hunk #10 FAILED at 588.
Hunk #11 succeeded at 608 (offset -11 lines).
1 out of 11 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/gdata/gdata_contacts_service.cc.rej

[commit-bot: I haz the power, 2012-08-07 13:13:14]

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

[commit-bot: I haz the power, 2012-08-07 14:57:19]

Change committed as 150344