Reading list downloader retries on recoverable error

Reading list downloader retries on recoverable error

This CL make sure the downloads of the entries start with the delay associated
with the entry and retry to download them if the failure is recoverable.

BUG=629771
Committed: https://crrev.com/412ca5d4a096dbed65fda0d789ae3d6d0a1da305
Cr-Commit-Position: refs/heads/master@{#420585}

[gambard, 2016-09-20 15:47:10]

Patchset #2 (id:20001) has been deleted

[gambard, 2016-09-20 15:54:43]

Description was changed from

==========
Reading list downloader retry if the error is recoverable

This CL make sure the downloads of the entries start with the delay associated
with the entry and retry to download them if the failure is recoverable.

BUG=629771
==========

to

==========
Reading list downloader retries on recoverable error

This CL make sure the downloads of the entries start with the delay associated
with the entry and retry to download them if the failure is recoverable.

BUG=629771
==========

[gambard, 2016-09-20 15:55:05]

gambard@chromium.org changed reviewers:
+ noyau@chromium.org

[gambard, 2016-09-20 15:55:05]

PTAL, I will write tests before landing this.

https://codereview.chromium.org/2351113003/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_download_service.cc (right):

https://codereview.chromium.org/2351113003/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_download_service.cc:113:
base::Unretained(this), entry.URL()),
I am not really sure about this, in particular of what happens if the class is
destroyed before the callback fires.

[noyau (Ping after 24h), 2016-09-21 11:08:06]

https://codereview.chromium.org/2351113003/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_download_service.cc (right):

https://codereview.chromium.org/2351113003/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_download_service.cc:113:
base::Unretained(this), entry.URL()),
On 2016/09/20 15:55:05, gambard wrote:
> I am not really sure about this, in particular of what happens if the class is
> destroyed before the callback fires.

You're right not to be sure :)

See the use of base::CancelableTaskTracker in URLDownloader for an example of
use.

[gambard, 2016-09-22 11:37:31]

On 2016/09/21 11:08:06, noyau wrote:
>
https://codereview.chromium.org/2351113003/diff/40001/ios/chrome/browser/read...
> File ios/chrome/browser/reading_list/reading_list_download_service.cc (right):
> 
>
https://codereview.chromium.org/2351113003/diff/40001/ios/chrome/browser/read...
> ios/chrome/browser/reading_list/reading_list_download_service.cc:113:
> base::Unretained(this), entry.URL()),
> On 2016/09/20 15:55:05, gambard wrote:
> > I am not really sure about this, in particular of what happens if the class
is
> > destroyed before the callback fires.
> 
> You're right not to be sure :)
> 
> See the use of base::CancelableTaskTracker in URLDownloader for an example of
> use.

base::CancelableTaskTracker doesn't have a way to post delayed tasks.
I could use CancelableCallback, but the class is not use anywhere.

[gambard, 2016-09-22 13:52:46]

PTAL.

https://codereview.chromium.org/2351113003/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_download_service.cc (right):

https://codereview.chromium.org/2351113003/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_download_service.cc:113:
base::Unretained(this), entry.URL()),
On 2016/09/21 11:08:06, noyau wrote:
> On 2016/09/20 15:55:05, gambard wrote:
> > I am not really sure about this, in particular of what happens if the class
is
> > destroyed before the callback fires.
> 
> You're right not to be sure :)
> 
> See the use of base::CancelableTaskTracker in URLDownloader for an example of
> use.

Using GetWeakPtr() should be OK according to
https://cs.chromium.org/chromium/src/docs/callback.md

[noyau (Ping after 24h), 2016-09-22 14:28:57]

lgtm

[gambard, 2016-09-22 16:46:55]

Fix it before committing.

https://codereview.chromium.org/2351113003/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_download_service.cc (right):

https://codereview.chromium.org/2351113003/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_download_service.cc:113:
base::Bind(&ReadingListDownloadService::DownloadEntryFromURL,
This is a loop

[gambard, 2016-09-23 09:45:52]

The CQ bit was checked by gambard@chromium.org

[gambard, 2016-09-23 09:45:52]

The patchset sent to the CQ was uploaded after l-g-t-m from noyau@chromium.org
Link to the patchset: https://codereview.chromium.org/2351113003/#ps80001
(title: "Fix looping issue")

[commit-bot: I haz the power, 2016-09-23 09:46:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 11:20:26]

Description was changed from

==========
Reading list downloader retries on recoverable error

This CL make sure the downloads of the entries start with the delay associated
with the entry and retry to download them if the failure is recoverable.

BUG=629771
==========

to

==========
Reading list downloader retries on recoverable error

This CL make sure the downloads of the entries start with the delay associated
with the entry and retry to download them if the failure is recoverable.

BUG=629771
==========

[commit-bot: I haz the power, 2016-09-23 11:20:27]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-09-23 11:22:26]

Description was changed from

==========
Reading list downloader retries on recoverable error

This CL make sure the downloads of the entries start with the delay associated
with the entry and retry to download them if the failure is recoverable.

BUG=629771
==========

to

==========
Reading list downloader retries on recoverable error

This CL make sure the downloads of the entries start with the delay associated
with the entry and retry to download them if the failure is recoverable.

BUG=629771

Committed: https://crrev.com/412ca5d4a096dbed65fda0d789ae3d6d0a1da305
Cr-Commit-Position: refs/heads/master@{#420585}
==========

[commit-bot: I haz the power, 2016-09-23 11:22:27]

Patchset 4 (id:??) landed as
https://crrev.com/412ca5d4a096dbed65fda0d789ae3d6d0a1da305
Cr-Commit-Position: refs/heads/master@{#420585}