Translate: language list smart updater

Translate: language list smart updater

Omit language list update until EULA is accepted,
and try update again if previous update fails in cases;
1) Network becomes online
2) Infobar goes to be shown
3) EULA is accepted

BUG=244202, 126701
TEST=browser_tests, and unit_tests
TBR=sky@chromium.org, rohitrao@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207482

[Takashi Toyoshima, 2013-06-06 07:38:57]

Hi mad,

Can you take a look this?
Once this change is landed, I'll introduce CrOS specific change using
ResourceRequestAllowedNotifier.

Thanks.

[Takashi Toyoshima, 2013-06-07 03:40:01]

+Hajime

[hajimehoshi, 2013-06-07 04:15:44]

https://chromiumcodereview.appspot.com/15949022/diff/4003/chrome/browser/tran...
File chrome/browser/translate/translate_language_list.cc (right):

https://chromiumcodereview.appspot.com/15949022/diff/4003/chrome/browser/tran...
chrome/browser/translate/translate_language_list.cc:206: scoped_ptr<const
net::URLFetcher> delete_ptr(fetcher_.release());
Remove this line (as we discussed).

[Takashi Toyoshima, 2013-06-07 05:17:17]

https://chromiumcodereview.appspot.com/15949022/diff/4003/chrome/browser/tran...
File chrome/browser/translate/translate_language_list.cc (right):

https://chromiumcodereview.appspot.com/15949022/diff/4003/chrome/browser/tran...
chrome/browser/translate/translate_language_list.cc:189: VLOG(9) << "Fetch
supporting language list from: " << url.spec().c_str();
FYI, VLOG(9) will be replace with translate-internals Event Logs by another
prior CL. Rebase will merge them.

https://chromiumcodereview.appspot.com/15949022/diff/4003/chrome/browser/tran...
chrome/browser/translate/translate_language_list.cc:206: scoped_ptr<const
net::URLFetcher> delete_ptr(fetcher_.release());
Thanks.
And after another discussion, I notice that this is not so bad to remove
URLFetcher safely.
I reorganized delete_ptr and callback invocation to improve readability.

[hajimehoshi, 2013-06-07 05:43:13]

Much clearer! lgtm

On 2013/06/07 05:17:17, Takashi Toyoshima (chromium) wrote:
>
https://chromiumcodereview.appspot.com/15949022/diff/4003/chrome/browser/tran...
> File chrome/browser/translate/translate_language_list.cc (right):
> 
>
https://chromiumcodereview.appspot.com/15949022/diff/4003/chrome/browser/tran...
> chrome/browser/translate/translate_language_list.cc:189: VLOG(9) << "Fetch
> supporting language list from: " << url.spec().c_str();
> FYI, VLOG(9) will be replace with translate-internals Event Logs by another
> prior CL. Rebase will merge them.
> 
>
https://chromiumcodereview.appspot.com/15949022/diff/4003/chrome/browser/tran...
> chrome/browser/translate/translate_language_list.cc:206: scoped_ptr<const
> net::URLFetcher> delete_ptr(fetcher_.release());
> Thanks.
> And after another discussion, I notice that this is not so bad to remove
> URLFetcher safely.
> I reorganized delete_ptr and callback invocation to improve readability.

[MAD, 2013-06-07 13:29:24]

Sorry about the delay, I'm Sheriff today as I was yesterday... I have a few
comments, but admit not having read other comments yet, just wanted to send
those before you commit, just in case...

https://codereview.chromium.org/15949022/diff/4003/chrome/browser/translate/t...
File chrome/browser/translate/translate_language_list.cc (right):

https://codereview.chromium.org/15949022/diff/4003/chrome/browser/translate/t...
chrome/browser/translate/translate_language_list.cc:173:
callback.Run(include_alpha_languages_, false, std::string());
If callbacks are comparable, shouldn't we simply DCHECK that callback ==
callback_?

https://codereview.chromium.org/15949022/diff/4003/chrome/browser/translate/t...
chrome/browser/translate/translate_language_list.cc:191:
fetcher_.reset(net::URLFetcher::Create(include_alpha_languages_ ? 2 : 1,
I didn't notice in previous version that we were using IDs, please add a comment
to explain why.

https://codereview.chromium.org/15949022/diff/4003/chrome/browser/translate/t...
chrome/browser/translate/translate_language_list.cc:313: if (!success)
If we would only call here when there's a real error (and note when we call
twice as commented on above), then we should be able to handle the exponential
backoff'd retries here.

https://codereview.chromium.org/15949022/diff/4003/chrome/browser/translate/t...
File chrome/browser/translate/translate_language_list.h (right):

https://codereview.chromium.org/15949022/diff/4003/chrome/browser/translate/t...
chrome/browser/translate/translate_language_list.h:69: // Represents internal
state if the fetch is completed successfully.
This comment is a bit misleading... The state is not JUST for completed fetches,
right?

[Takashi Toyoshima, 2013-06-07 14:42:55]

Thanks.
Yes, this change is not simple, so I though I should wait for your LGTM.
I'll revise it again and wait for your grant.

For now, I just write directions.

https://codereview.chromium.org/15949022/diff/4003/chrome/browser/translate/t...
File chrome/browser/translate/translate_language_list.cc (right):

https://codereview.chromium.org/15949022/diff/4003/chrome/browser/translate/t...
chrome/browser/translate/translate_language_list.cc:173:
callback.Run(include_alpha_languages_, false, std::string());
(state_ == REQUESTING) means another Request() is called before previous
Request() is not finished.
This case, callback_ holds a callback for the first Request(), so callback !=
callback_ may happen.

Here, I just abort the second request by invoking callback, and allow the first
Request() to continue to perform.

https://codereview.chromium.org/15949022/diff/4003/chrome/browser/translate/t...
chrome/browser/translate/translate_language_list.cc:191:
fetcher_.reset(net::URLFetcher::Create(include_alpha_languages_ ? 2 : 1,
In another change, I defined const int for these 1 and 2.
URLFetcher class also explains that this ID is aimed for testing.
These IDs are used in testing to distinguish each Fetch for simulating results.
I'll add comment around const int definitions.

https://codereview.chromium.org/15949022/diff/4003/chrome/browser/translate/t...
chrome/browser/translate/translate_language_list.cc:313: if (!success)
Retry is issued when network status is changed to online, or when EULA is
accepted (EULA handling is implemented in the next CL).
Also, when infobar appears, also it retries to fetch the language list if list
is not updated yet.

But, as you suggested, it must be better to reduce retry opportunity
exponentially.

https://codereview.chromium.org/15949022/diff/4003/chrome/browser/translate/t...
File chrome/browser/translate/translate_language_list.h (right):

https://codereview.chromium.org/15949022/diff/4003/chrome/browser/translate/t...
chrome/browser/translate/translate_language_list.h:69: // Represents internal
state if the fetch is completed successfully.
I see.
Is it enough to say just 'Represents internal fetch state.'?

[MAD, 2013-06-07 15:02:14]

Some answers to your comments...

Thanks!

BYE
MAD

https://codereview.chromium.org/15949022/diff/4003/chrome/browser/translate/t...
File chrome/browser/translate/translate_language_list.cc (right):

https://codereview.chromium.org/15949022/diff/4003/chrome/browser/translate/t...
chrome/browser/translate/translate_language_list.cc:173:
callback.Run(include_alpha_languages_, false, std::string());
On 2013/06/07 14:42:55, Takashi Toyoshima (chromium) wrote:
> (state_ == REQUESTING) means another Request() is called before previous
> Request() is not finished.
> This case, callback_ holds a callback for the first Request(), so callback !=
> callback_ may happen.
> 
> Here, I just abort the second request by invoking callback, and allow the
first
> Request() to continue to perform.

As I mentioned below, if callback == callback_ when they have the same
method/this pointers, then I think it's better not to call with false in that
case. Only callback with false success when callback are not the same.

https://codereview.chromium.org/15949022/diff/4003/chrome/browser/translate/t...
chrome/browser/translate/translate_language_list.cc:189: VLOG(9) << "Fetch
supporting language list from: " << url.spec().c_str();
On 2013/06/07 05:17:17, Takashi Toyoshima (chromium) wrote:
> FYI, VLOG(9) will be replace with translate-internals Event Logs by another
> prior CL. Rebase will merge them.

OK.

https://codereview.chromium.org/15949022/diff/4003/chrome/browser/translate/t...
chrome/browser/translate/translate_language_list.cc:191:
fetcher_.reset(net::URLFetcher::Create(include_alpha_languages_ ? 2 : 1,
On 2013/06/07 14:42:55, Takashi Toyoshima (chromium) wrote:
> In another change, I defined const int for these 1 and 2.
> URLFetcher class also explains that this ID is aimed for testing.
> These IDs are used in testing to distinguish each Fetch for simulating
results.
> I'll add comment around const int definitions.
OK thanks.

https://codereview.chromium.org/15949022/diff/4003/chrome/browser/translate/t...
chrome/browser/translate/translate_language_list.cc:313: if (!success)
And what I meant is that, since you return an error when there's already a
pending request, this code here can not tell the difference between a real error
and an overlap...

This is why I think the code above should compare the callbacks (I think they
are the same when the Bind objects are created with the same method and this
pointer, but I'm not sure) and only return an error when it's not the same,
because the second callback will never be called afterward, so it's OK to call
it with an error, as opposed to when the callback is the same, then it will
eventually be called with proper results... Is this clear enough? Maybe I don't
explain it correctly...

[Takashi Toyoshima, 2013-06-10 13:36:57]

Hi mad,

I rebase this change, and fix some points you suggested.
Also, I introduce exponential backoff and limitation against retry.

https://codereview.chromium.org/15949022/diff/4003/chrome/browser/translate/t...
File chrome/browser/translate/translate_language_list.cc (right):

https://codereview.chromium.org/15949022/diff/4003/chrome/browser/translate/t...
chrome/browser/translate/translate_language_list.cc:173:
callback.Run(include_alpha_languages_, false, std::string());
Ah, now I understand what you concerned about.

I can add a check as DCHECK(!callback_.Equals(callback)), but now I feel it must
return false synchronously here. As you mentioned below, asynchronous errors
look a little difficult to understand what happens.

https://codereview.chromium.org/15949022/diff/4003/chrome/browser/translate/t...
chrome/browser/translate/translate_language_list.cc:191:
fetcher_.reset(net::URLFetcher::Create(include_alpha_languages_ ? 2 : 1,
On 2013/06/07 15:02:15, MAD wrote:
> On 2013/06/07 14:42:55, Takashi Toyoshima (chromium) wrote:
> > In another change, I defined const int for these 1 and 2.
> > URLFetcher class also explains that this ID is aimed for testing.
> > These IDs are used in testing to distinguish each Fetch for simulating
> results.
> > I'll add comment around const int definitions.
> OK thanks.

Done.

https://codereview.chromium.org/15949022/diff/4003/chrome/browser/translate/t...
chrome/browser/translate/translate_language_list.cc:313: if (!success)
Thank you for clarification.
After understanding your concerns, I think the simplest solution is that
Request() returns false if another Request() is not finished.

[MAD, 2013-06-10 18:28:10]

More comments... I think you misunderstood my comment about exponential
backoff... More details below...

BYE
MAD

https://codereview.chromium.org/15949022/diff/65001/chrome/browser/translate/...
File chrome/browser/translate/translate_language_list.cc (right):

https://codereview.chromium.org/15949022/diff/65001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:186: bool IsOneHot(int n) {
I would prefer to name this: IsPowerOfTwo.

And here's a trick to do it more easily:
http://bits.stephan-brumme.com/isPowerOfTwo.html
http://stackoverflow.com/questions/600293/how-to-check-if-a-number-is-a-power...

(n != 0) && ((n & (n - 1)) == 0)

https://codereview.chromium.org/15949022/diff/65001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:200: retry_count_(0) {
You never reset it to 0, so it's not just retries, it's for ALL calls, which I
don't think is what you intend this for, right?

https://codereview.chromium.org/15949022/diff/65001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:208: // This function is not
supporsed to be called before previous operaion is not
supporsed -> supposed

https://codereview.chromium.org/15949022/diff/65001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:222: if
(!IsOneHot(++retry_count_)) {
Actually, the URLFetcher already supports backoff delays:
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...

https://codereview.chromium.org/15949022/diff/65001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:377: if (!success)
You don't actually retry on failures, and you don't look at the return value of
LanguageListFetcher::Request(), so your backoff system doesn't really work, and
is not needed, it just prevents the fetch from happening more and more as we get
the languages more and more...

And actually, since you don't check if the network is available (you rely on the
URLFetcher to fail when there's no network?) and you don't have the wait for
EULA yet (it's in your other patch), I don't think you need more than what the
URLFetch already offer as a retry/backoff service.

[Takashi Toyoshima, 2013-06-11 01:33:51]

Thanks.
By the way, this patch can be splited to two part. One is for refactoring, and
the other is for retry.
Refactoring patch will be based on this patch set 2.
I'll land it firstly, then merge EULA CL to the latter one.
I guess it may make more sense for reviews now.

[MAD, 2013-06-11 16:49:52]

On 2013/06/11 01:33:51, Takashi Toyoshima (chromium) wrote:
> Thanks.
> By the way, this patch can be splited to two part. One is for refactoring, and
> the other is for retry.
> Refactoring patch will be based on this patch set 2.
> I'll land it firstly, then merge EULA CL to the latter one.
> I guess it may make more sense for reviews now.

OK, I'll wait for the next iteration then...

[Takashi Toyoshima, 2013-06-12 05:20:43]

Hi mad,
Since I already land a split CL to reconstruct URLFetcher handling, this change
looks simpler and focusing on changes for retry.
Can you take a look this again?

Alexei,
As I said in another review, I merged it to this CL.
Can you take a look this from the viewpoint of ResourceRequestAllowedNotifier?
I found a problem, and fix it in this CL. Without this change, it's never
notified on the second and later cases when network comes online.

https://codereview.chromium.org/15949022/diff/65001/chrome/browser/translate/...
File chrome/browser/translate/translate_language_list.cc (right):

https://codereview.chromium.org/15949022/diff/65001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:186: bool IsOneHot(int n) {
I'll stop to use exponential backoff here.
Because retry request is not timer-based, but user action or monitoring based.
The retry cycle can be reasonable.
It looks safe just to have a maximum limit.

https://codereview.chromium.org/15949022/diff/65001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:200: retry_count_(0) {
If I reset retry_count whenever a request is issued, interval will be 1, 2, 4,
8, ....
But, also it works without reseting this because (2-1), (4-2), (8-4), (16-8),
... is also 1, 2, 4, 8, ....

https://codereview.chromium.org/15949022/diff/65001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:208: // This function is not
supporsed to be called before previous operaion is not
On 2013/06/10 18:28:10, MAD wrote:
> supporsed -> supposed

Done.

https://codereview.chromium.org/15949022/diff/65001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:222: if
(!IsOneHot(++retry_count_)) {
Yes. But it works only against HTTP status 5xx.
This retry covers network offline, flaky network, and so on.

https://codereview.chromium.org/15949022/diff/65001/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:377: if (!success)
No. It was invoked in GetSupportedLanguages() if the list is not updated.
This function is called whenever infobar appears.

Intentionally, I prevent to invoke retry here because we fails fetch just now
even if URLFetcher performed internal retry, and there rarely  internal retry,
and there is rarely a chance to success.

Also I invoked retry when network becomes online. See, OnNetworkChanged() of
this change set.
Or OnResourceRequestsAllowed() of the change set 12

[Alexei Svitkine (slow), 2013-06-12 14:50:14]

Thanks for the resource_request_allowed_notifier.cc bug fix! Could you move the
changes to it in a separate CL, though? (In case we may need to merge them back
separately.)

https://codereview.chromium.org/15949022/diff/92002/chrome/browser/web_resour...
File chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc
(right):

https://codereview.chromium.org/15949022/diff/92002/chrome/browser/web_resour...
chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc:194:
TEST_F(ResourceRequestAllowedNotifierTest, NoNotifyOnFlakyConnection) {
Why did you remove lines from this test?

[Alexei Svitkine (slow), 2013-06-13 14:46:02]

https://codereview.chromium.org/15949022/diff/92002/chrome/browser/translate/...
File chrome/browser/translate/translate_language_list.cc (right):

https://codereview.chromium.org/15949022/diff/92002/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:206: // Limits
LanguageListFetcher level retry to kMaxRetryLanguageListFetcher
Nit: I don't think the comment is needed, the code below is clear enough.

https://codereview.chromium.org/15949022/diff/92002/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:208: NotifyEvent(__LINE__,
"Request is omitted due to retry limitation");
Nit: limitation -> limit

(The two have slightly different meanings.)

https://codereview.chromium.org/15949022/diff/92002/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:274: // Transfer
URLFetcher's ownership before invoking a callback.
This comment seems out of place...

https://codereview.chromium.org/15949022/diff/92002/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:364: // will be invoked
later to reuest resources again.
Can you expand this comment to mention that fetcher is responsible for aborting
re-tries if it's asked to re-try too many times?

Else, just from the block of code it looks like you could get into an infinite
retry/fail cycle.

[Takashi Toyoshima, 2013-06-14 08:13:55]

Thanks Alexei,

As you suggested, I split ResourceRequestAllowedNotifier changes to
https://codereview.chromium.org/16841020/.

After the previous change, I found another problematic scenarios. Please see the
filed bug and the CL.

https://codereview.chromium.org/15949022/diff/92002/chrome/browser/web_resour...
File chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc
(right):

https://codereview.chromium.org/15949022/diff/92002/chrome/browser/web_resour...
chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc:194:
TEST_F(ResourceRequestAllowedNotifierTest, NoNotifyOnFlakyConnection) {
Because this test looks wrong.

1) network goes online
2) network goes offline
3) network goes online

This is the case I want to get notified, but this test expects not notified.

I changed this test again in a different way.
Please continue to review https://codereview.chromium.org/16841020/

[Takashi Toyoshima, 2013-06-14 09:10:48]

Thanks Alexei,

I revised translate side change, too.

https://codereview.chromium.org/15949022/diff/92002/chrome/browser/translate/...
File chrome/browser/translate/translate_language_list.cc (right):

https://codereview.chromium.org/15949022/diff/92002/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:206: // Limits
LanguageListFetcher level retry to kMaxRetryLanguageListFetcher
On 2013/06/13 14:46:02, Alexei Svitkine wrote:
> Nit: I don't think the comment is needed, the code below is clear enough.

Done.

https://codereview.chromium.org/15949022/diff/92002/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:208: NotifyEvent(__LINE__,
"Request is omitted due to retry limitation");
Wow, thank you:)
I didn't know.

Done!

https://codereview.chromium.org/15949022/diff/92002/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:274: // Transfer
URLFetcher's ownership before invoking a callback.
Oops. Sorry, it looks like that I made wrong rebase to move this line here.
I fix to move it to proper place.

https://codereview.chromium.org/15949022/diff/92002/chrome/browser/translate/...
chrome/browser/translate/translate_language_list.cc:364: // will be invoked
later to reuest resources again.
On 2013/06/13 14:46:02, Alexei Svitkine wrote:
> Can you expand this comment to mention that fetcher is responsible for
aborting
> re-tries if it's asked to re-try too many times?
> 
> Else, just from the block of code it looks like you could get into an infinite
> retry/fail cycle.

Done.

[Alexei Svitkine (slow), 2013-06-14 22:06:05]

lgtm

https://codereview.chromium.org/15949022/diff/103001/chrome/browser/translate...
File chrome/browser/translate/translate_language_list.cc (right):

https://codereview.chromium.org/15949022/diff/103001/chrome/browser/translate...
chrome/browser/translate/translate_language_list.cc:367: to invoke
OnLanguageListFetchComplete.
Nit: remove this line?

[Takashi Toyoshima, 2013-06-17 07:20:04]

Thanks.
Now, I'll wait for mad's another lgtm.

https://codereview.chromium.org/15949022/diff/103001/chrome/browser/translate...
File chrome/browser/translate/translate_language_list.cc (right):

https://codereview.chromium.org/15949022/diff/103001/chrome/browser/translate...
chrome/browser/translate/translate_language_list.cc:367: to invoke
OnLanguageListFetchComplete.
Sorry for poor nit...
Removed.

[MAD, 2013-06-17 14:48:41]

LGTM with one small typo nit request... :-)

Thanks!

BYE
MAD

https://codereview.chromium.org/15949022/diff/119001/chrome/browser/translate...
File chrome/browser/translate/translate_language_list.cc (right):

https://codereview.chromium.org/15949022/diff/119001/chrome/browser/translate...
chrome/browser/translate/translate_language_list.cc:372: // will be invoked
later to reuest resources again.
reuest -> request?

[Takashi Toyoshima, 2013-06-19 10:06:15]

Dependent CL was committed.
I'll push it to CQ.

https://codereview.chromium.org/15949022/diff/119001/chrome/browser/translate...
File chrome/browser/translate/translate_language_list.cc (right):

https://codereview.chromium.org/15949022/diff/119001/chrome/browser/translate...
chrome/browser/translate/translate_language_list.cc:372: // will be invoked
later to reuest resources again.
On 2013/06/17 14:48:41, MAD wrote:
> reuest -> request?

Done.

[commit-bot: I haz the power, 2013-06-19 10:21:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/15949022/137001

[commit-bot: I haz the power, 2013-06-19 11:32:47]

Retried try job too often on mac_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-06-19 14:28:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/15949022/137001

[commit-bot: I haz the power, 2013-06-19 14:31:06]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-06-19 16:54:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/15949022/137001

[commit-bot: I haz the power, 2013-06-19 17:01:31]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-06-20 02:42:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/15949022/137001

[commit-bot: I haz the power, 2013-06-20 02:42:18]

Failed to apply patch for chrome/browser/translate/translate_language_list.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/translate/translate_language_list.cc
  Hunk #1 succeeded at 104 (offset -3 lines).
  Hunk #2 FAILED at 195.
  Hunk #3 FAILED at 203.
  Hunk #4 FAILED at 237.
  Hunk #5 FAILED at 264.
  Hunk #6 FAILED at 282.
  Hunk #7 succeeded at 231 (offset -62 lines).
  Hunk #8 succeeded at 262 with fuzz 2 (offset -62 lines).
  Hunk #9 FAILED at 351.
  6 out of 9 hunks FAILED -- saving rejects to file
chrome/browser/translate/translate_language_list.cc.rej

Patch:       chrome/browser/translate/translate_language_list.cc
Index: chrome/browser/translate/translate_language_list.cc
diff --git a/chrome/browser/translate/translate_language_list.cc
b/chrome/browser/translate/translate_language_list.cc
index
af0f77f502ea8ca6a067711852b49064e8dc7666..88f1edb7efa407d6b7863a6ee89e405d6b45452d
100644
--- a/chrome/browser/translate/translate_language_list.cc
+++ b/chrome/browser/translate/translate_language_list.cc
@@ -107,7 +107,10 @@ const char kAlphaLanguageQueryName[] = "alpha";
 const char kAlphaLanguageQueryValue[] = "1";
 
 // Retry parameter for fetching supporting language list.
-const int kMaxRetryLanguageListFetch = 5;
+const int kMaxRetryLanguageListFetchOn5xx = 5;
+
+// Retry parameter for LanguageListFetcher.
+const int kMaxRetryLanguageListFetcher = 16;
 
 // Assign following IDs to URLFetchers so that tests can distinguish each
 // request in order to simiulate respectively.
@@ -192,7 +195,8 @@ void SetSupportedLanguages(const std::string& language_list,
 TranslateLanguageList::LanguageListFetcher::LanguageListFetcher(
     bool include_alpha_languages)
   : include_alpha_languages_(include_alpha_languages),
-    state_(IDLE) {
+    state_(IDLE),
+    retry_count_(0) {
 }
 
 TranslateLanguageList::LanguageListFetcher::~LanguageListFetcher() {
@@ -200,13 +204,19 @@
TranslateLanguageList::LanguageListFetcher::~LanguageListFetcher() {
 
 bool TranslateLanguageList::LanguageListFetcher::Request(
     const TranslateLanguageList::LanguageListFetcher::Callback& callback) {
-  // This function is not supporsed to be called before previous operaion is
not
+  // This function is not supposed to be called before previous operaion is not
   // finished.
   if (state_ == REQUESTING) {
     NOTREACHED();
     return false;
   }
 
+  if (retry_count_ >= kMaxRetryLanguageListFetcher) {
+    NotifyEvent(__LINE__, "Request is omitted due to retry limit");
+    return false;
+  }
+  retry_count_++;
+
   state_ = REQUESTING;
   callback_ = callback;
 
@@ -234,7 +244,10 @@ bool TranslateLanguageList::LanguageListFetcher::Request(
   fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
                          net::LOAD_DO_NOT_SAVE_COOKIES);
   fetcher_->SetRequestContext(g_browser_process->system_request_context());
-  fetcher_->SetMaxRetriesOn5xx(kMaxRetryLanguageListFetch);
+  // Set retry parameter for HTTP status code 5xx. This doesn't work against
+  // 106 (net::ERR_INTERNET_DISCONNECTED) and so on.
+  // TranslateLanguageList handles network status, and implements retry.
+  fetcher_->SetMaxRetriesOn5xx(kMaxRetryLanguageListFetchOn5xx);
   fetcher_->Start();
 
   return true;
@@ -261,6 +274,7 @@ void
TranslateLanguageList::LanguageListFetcher::OnURLFetchComplete(
     NotifyEvent(__LINE__, message);
   }
 
+  // Transfer URLFetcher's ownership before invoking a callback.
   scoped_ptr<const net::URLFetcher> delete_ptr(fetcher_.release());
   callback_.Run(include_alpha_languages_, state_ == COMPLETED, data);
 }
@@ -279,6 +293,8 @@ TranslateLanguageList::TranslateLanguageList() {
 
   language_list_fetcher_.reset(new LanguageListFetcher(false));
   alpha_language_list_fetcher_.reset(new LanguageListFetcher(true));
+
+  resource_request_allowed_notifier_.Init(this);
 }
 
 TranslateLanguageList::~TranslateLanguageList() {
@@ -290,6 +306,11 @@ void TranslateLanguageList::GetSupportedLanguages(
   std::set<std::string>::const_iterator iter =
all_supported_languages_.begin();
   for (; iter != all_supported_languages_.end(); ++iter)
     languages->push_back(*iter);
+
+  // Update language lists if they are not updated after Chrome was launched
+  // for later requests.
+  if (language_list_fetcher_.get() || alpha_language_list_fetcher_.get())
+    RequestLanguageList();
 }
 
 std::string TranslateLanguageList::GetLanguageCode(
@@ -316,6 +337,12 @@ bool TranslateLanguageList::IsAlphaLanguage(const
std::string& language) {
 }
 
 void TranslateLanguageList::RequestLanguageList() {
+  // If resource requests are not allowed, we'll get a callback when they are.
+  if (resource_request_allowed_notifier_.ResourceRequestsAllowed())
+    OnResourceRequestsAllowed();
+}
+
+void TranslateLanguageList::OnResourceRequestsAllowed() {
   if (language_list_fetcher_.get() &&
       (language_list_fetcher_->state() == LanguageListFetcher::IDLE ||
        language_list_fetcher_->state() == LanguageListFetcher::FAILED)) {
@@ -337,8 +364,16 @@ void TranslateLanguageList::OnLanguageListFetchComplete(
     bool include_alpha_languages,
     bool success,
     const std::string& data) {
-  if (!success)
+  if (!success) {
+    resource_request_allowed_notifier_.ResourceRequestsAllowed();
+    // Since it fails just now, omit to schedule resource requests if
+    // ResourceRequestAllowedNotifier think it's ready. Otherwise, a callback
+    // will be invoked later to request resources again.
+    // The LanguageListFetcher has a limit |kMaxRetryLanguageListFetcher| and
+    // aborts re-try not to invoke OnLanguageListFetchComplete anymore if it's
+    // asked to re-try too many times.
     return;
+  }
 
   if (!include_alpha_languages) {
     SetSupportedLanguages(data, &supported_languages_);

[Takashi Toyoshima, 2013-06-20 06:03:54]

Hi Hajime,
This change was conflicted with your previous change.
Now that this rebase is not trivial, can you take a look this again before
landing?

[hajimehoshi, 2013-06-20 06:05:12]

Sure. Sorry for the chaos.

[hajimehoshi, 2013-06-20 08:15:50]

lgtm

On 2013/06/20 06:05:12, hajimehoshi wrote:
> Sure. Sorry for the chaos.

[hajimehoshi, 2013-06-20 08:16:16]

Oops. I mean lgtm with nits.

On 2013/06/20 08:15:50, hajimehoshi wrote:
> lgtm
> 
> On 2013/06/20 06:05:12, hajimehoshi wrote:
> > Sure. Sorry for the chaos.

[hajimehoshi, 2013-06-20 08:16:31]

https://codereview.chromium.org/15949022/diff/173002/chrome/browser/translate...
File chrome/browser/translate/translate_language_list.cc (right):

https://codereview.chromium.org/15949022/diff/173002/chrome/browser/translate...
chrome/browser/translate/translate_language_list.cc:310: // The
TranslateURLFetcher has a limit for retried requests and aborts re-try
Please break this line.

[Takashi Toyoshima, 2013-06-20 08:26:19]

https://codereview.chromium.org/15949022/diff/173002/chrome/browser/translate...
File chrome/browser/translate/translate_language_list.cc (right):

https://codereview.chromium.org/15949022/diff/173002/chrome/browser/translate...
chrome/browser/translate/translate_language_list.cc:310: // The
TranslateURLFetcher has a limit for retried requests and aborts re-try
On 2013/06/20 08:16:31, hajimehoshi wrote:
> Please break this line.

Done.

[commit-bot: I haz the power, 2013-06-20 08:27:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/15949022/124003

[commit-bot: I haz the power, 2013-06-20 08:30:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/15949022/124003

[Takashi Toyoshima, 2013-06-20 14:06:57]

With this change, URLFetcher runs to update language list in unit_tests for
mac's infobar.
I added a flag to disable language list update for testing.

[commit-bot: I haz the power, 2013-06-20 14:07:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/15949022/130006

[commit-bot: I haz the power, 2013-06-20 18:14:25]

Change committed as 207482