kiosk: Retry failed app data fetch.

kiosk: Retry failed app data fetch.

- Add KioskAppManager::RetryFailedAppDataFetch that starts fetch for failed
  app data fetch (status == STATUS_ERROR);
- RetryFailedAppDataFetch when kiosk app is added/removed (include policy refrech);
- RetryFailedAppDataFetch on login screen when portal detection is passed;
- Add max auto retry times for underlying URLFetcher's 5xx retry and
  ERR_NETWORK_CHANGED retry in WebstoreDataFetcher;
- KioskAppData sets max retry times to 3 for its webstore data fetch;

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

[xiyuan, 2014-07-10 17:26:25]

"Apps" menu on login screen only shows up when webstore data is successfully
fetched. Currently, there is no retry and transient network errors cause the
menu to be missing. This CL adds a few ways to re-fetch the data (when portal
detection is done on login screen, refresh policy and use auto retry of the
underlying URLFetcher).

asargent:
  chrome/browser/extensions/*

zel:
  everything else

[zel, 2014-07-10 17:29:27]

lgtm

[asargent_no_longer_on_chrome, 2014-07-10 23:08:34]

lgtm w/ one question

https://codereview.chromium.org/386633003/diff/1/chrome/browser/extensions/we...
File chrome/browser/extensions/webstore_data_fetcher.cc (right):

https://codereview.chromium.org/386633003/diff/1/chrome/browser/extensions/we...
chrome/browser/extensions/webstore_data_fetcher.cc:47:
webstore_data_url_fetcher_->SetMaxRetriesOn5xx(max_auto_retries_);
Should you only call this if max_auto_retries_ is > 0?

[xiyuan, 2014-07-10 23:23:13]

https://codereview.chromium.org/386633003/diff/1/chrome/browser/extensions/we...
File chrome/browser/extensions/webstore_data_fetcher.cc (right):

https://codereview.chromium.org/386633003/diff/1/chrome/browser/extensions/we...
chrome/browser/extensions/webstore_data_fetcher.cc:47:
webstore_data_url_fetcher_->SetMaxRetriesOn5xx(max_auto_retries_);
On 2014/07/10 23:08:34, Antony Sargent wrote:
> Should you only call this if max_auto_retries_ is > 0?

The code assumes that the default retry numbers in URLFetcher are 0 (which is
currently the case) and this is a no-op when |max_auto_retries_| is 0.

Do you feel this assumes too much implementation details and we should wrap it
with the suggested "if"?

[asargent_no_longer_on_chrome, 2014-07-10 23:49:07]

https://codereview.chromium.org/386633003/diff/1/chrome/browser/extensions/we...
File chrome/browser/extensions/webstore_data_fetcher.cc (right):

https://codereview.chromium.org/386633003/diff/1/chrome/browser/extensions/we...
chrome/browser/extensions/webstore_data_fetcher.cc:47:
webstore_data_url_fetcher_->SetMaxRetriesOn5xx(max_auto_retries_);
On 2014/07/10 23:23:13, xiyuan wrote:
> On 2014/07/10 23:08:34, Antony Sargent wrote:
> > Should you only call this if max_auto_retries_ is > 0?
> 
> The code assumes that the default retry numbers in URLFetcher are 0 (which is
> currently the case) and this is a no-op when |max_auto_retries_| is 0.
> 
> Do you feel this assumes too much implementation details and we should wrap it
> with the suggested "if"?

Yeah, I'm not sure. The URLFetcher header comments don't really make this clear,
so I was thinking that we might want to just take whatever the default behavior
is (which is currently 0, but could conceivably change some day?) unless our own
consumer wanted to change it to something else. I don't think it's a really a
big deal either way though, so use your best judgement.

[xiyuan, 2014-07-11 04:05:44]

https://codereview.chromium.org/386633003/diff/1/chrome/browser/extensions/we...
File chrome/browser/extensions/webstore_data_fetcher.cc (right):

https://codereview.chromium.org/386633003/diff/1/chrome/browser/extensions/we...
chrome/browser/extensions/webstore_data_fetcher.cc:47:
webstore_data_url_fetcher_->SetMaxRetriesOn5xx(max_auto_retries_);
On 2014/07/10 23:49:06, Antony Sargent wrote:
> On 2014/07/10 23:23:13, xiyuan wrote:
> > On 2014/07/10 23:08:34, Antony Sargent wrote:
> > > Should you only call this if max_auto_retries_ is > 0?
> > 
> > The code assumes that the default retry numbers in URLFetcher are 0 (which
is
> > currently the case) and this is a no-op when |max_auto_retries_| is 0.
> > 
> > Do you feel this assumes too much implementation details and we should wrap
it
> > with the suggested "if"?
> 
> Yeah, I'm not sure. The URLFetcher header comments don't really make this
clear,
> so I was thinking that we might want to just take whatever the default
behavior
> is (which is currently 0, but could conceivably change some day?) unless our
own
> consumer wanted to change it to something else. I don't think it's a really a
> big deal either way though, so use your best judgement. 

Okay. Think it makes more sense than assuming the default is no retry.

I have added the "if" so that we use the URLFetcher default behavior on 0 and
updated the comment in header file.

[xiyuan, 2014-07-11 04:07:05]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2014-07-11 04:08:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xiyuan@chromium.org/386633003/20001

[commit-bot: I haz the power, 2014-07-11 05:42:16]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-11 07:20:49]

Change committed as 282577