Add a TimeUntilNextTry to reading list

Add a TimeUntilNextTry to reading list

This CL adds a method to get the time delta until the next try for a reading
list entry. This method is increased exponentially and automatically when the
state changes from non-error to error.

BUG=none

Committed: https://crrev.com/cb3322bfbf31dac33a17266afca8c57a10c2a2e4
Cr-Commit-Position: refs/heads/master@{#420337}

[gambard, 2016-09-16 13:39:59]

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

[gambard, 2016-09-16 13:40:00]

PTAL.

[gambard, 2016-09-16 14:23:07]

Closing this.

[noyau (Ping after 24h), 2016-09-16 14:25:13]

https://codereview.chromium.org/2338133010/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_entry.cc (right):

https://codereview.chromium.org/2338133010/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.cc:71: distilled_state_ !=
WILL_RETRY && distilled_state_ != ERROR)
This condition is quite hard to read. Can you add a comment to clarify the
intent?

https://codereview.chromium.org/2338133010/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_entry_unittest.cc (right):

https://codereview.chromium.org/2338133010/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry_unittest.cc:63:
e.SetDistilledState(ReadingListEntry::WILL_RETRY);
Is there any case where a transition from ERROR to WILL_RETRY is possible? Or
desirable? Should it be DCHECKed instead?

[gambard, 2016-09-16 14:51:13]

Description was changed from

==========
Add a failed download counter to reading list

This CL adds a failed downloads counter to reading list, automatically
incremented when we update the entry to an error state.

BUG=none
==========

to

==========
Add a failed download counter to reading list

This CL adds a failed downloads counter to reading list, automatically
incremented when we update the entry to an error state.

BUG=none
==========

[gambard, 2016-09-19 08:42:33]

Description was changed from

==========
Add a failed download counter to reading list

This CL adds a failed downloads counter to reading list, automatically
incremented when we update the entry to an error state.

BUG=none
==========

to

==========
Add a TimeUntilNextTry to reading list

This CL adds a method to get the time delta until the next try for a reading
list entry. This method is exponentially increased automatically we the state
changes from non-error to error.

BUG=none
==========

[gambard, 2016-09-19 08:43:24]

Description was changed from

==========
Add a TimeUntilNextTry to reading list

This CL adds a method to get the time delta until the next try for a reading
list entry. This method is exponentially increased automatically we the state
changes from non-error to error.

BUG=none
==========

to

==========
Add a TimeUntilNextTry to reading list

This CL adds a method to get the time delta until the next try for a reading
list entry. This method is increased exponentially and automatically when the
state changes from non-error to error.

BUG=none
==========

[gambard, 2016-09-19 09:08:10]

PTAL.
Re-opening this.

https://codereview.chromium.org/2338133010/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_entry.cc (right):

https://codereview.chromium.org/2338133010/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.cc:71: distilled_state_ !=
WILL_RETRY && distilled_state_ != ERROR)
On 2016/09/16 14:25:13, noyau wrote:
> This condition is quite hard to read. Can you add a comment to clarify the
> intent?

Done.

https://codereview.chromium.org/2338133010/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_entry_unittest.cc (right):

https://codereview.chromium.org/2338133010/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry_unittest.cc:63:
e.SetDistilledState(ReadingListEntry::WILL_RETRY);
On 2016/09/16 14:25:13, noyau wrote:
> Is there any case where a transition from ERROR to WILL_RETRY is possible? Or
> desirable? Should it be DCHECKed instead?

Potentially, if you launch two download before the first of the two has ended,
one finishing in ERROR and the other in WILL_RETRY. I don't know if it is
possible, but I don't want to prevent it.

https://codereview.chromium.org/2338133010/diff/50001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_entry.cc (right):

https://codereview.chromium.org/2338133010/diff/50001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.cc:57: clock_ = other.clock_;
I am not sure about this pointer copy and the one in the copy constructor, as
the entry does not retain the pointer, the object making the copy should also
copy the clock. As I intend to see this primarily used in tests I think it is
not a problem.
Is a comment for the class enough? Or is there a way to expose the clock
constructor for tests only?

[noyau (Ping after 24h), 2016-09-19 09:19:54]

https://codereview.chromium.org/2338133010/diff/50001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_entry.cc (right):

https://codereview.chromium.org/2338133010/diff/50001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.cc:57: clock_ = other.clock_;
On 2016/09/19 09:08:09, gambard wrote:
> I am not sure about this pointer copy and the one in the copy constructor, as
> the entry does not retain the pointer, the object making the copy should also
> copy the clock. As I intend to see this primarily used in tests I think it is
> not a problem.
> Is a comment for the class enough? Or is there a way to expose the clock
> constructor for tests only?

The ownership of clock is very unclear and needs fixing.  Short of passing a
lambda that returns a clock instead of the clock itself in the constructor, I
don't see a good way out.

[gambard, 2016-09-19 15:33:03]

PTAL.

https://codereview.chromium.org/2338133010/diff/50001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/reading_list_entry.cc (right):

https://codereview.chromium.org/2338133010/diff/50001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_entry.cc:57: clock_ = other.clock_;
On 2016/09/19 09:19:54, noyau wrote:
> On 2016/09/19 09:08:09, gambard wrote:
> > I am not sure about this pointer copy and the one in the copy constructor,
as
> > the entry does not retain the pointer, the object making the copy should
also
> > copy the clock. As I intend to see this primarily used in tests I think it
is
> > not a problem.
> > Is a comment for the class enough? Or is there a way to expose the clock
> > constructor for tests only?
> 
> The ownership of clock is very unclear and needs fixing.  Short of passing a
> lambda that returns a clock instead of the clock itself in the constructor, I
> don't see a good way out.

I moved to a different model.

[noyau (Ping after 24h), 2016-09-19 15:40:15]

lgtm

[gambard, 2016-09-22 14:49:02]

The CQ bit was checked by gambard@chromium.org

[commit-bot: I haz the power, 2016-09-22 14:49:25]

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-22 14:59:44]

Description was changed from

==========
Add a TimeUntilNextTry to reading list

This CL adds a method to get the time delta until the next try for a reading
list entry. This method is increased exponentially and automatically when the
state changes from non-error to error.

BUG=none
==========

to

==========
Add a TimeUntilNextTry to reading list

This CL adds a method to get the time delta until the next try for a reading
list entry. This method is increased exponentially and automatically when the
state changes from non-error to error.

BUG=none
==========

[commit-bot: I haz the power, 2016-09-22 14:59:49]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2016-09-22 15:01:19]

Description was changed from

==========
Add a TimeUntilNextTry to reading list

This CL adds a method to get the time delta until the next try for a reading
list entry. This method is increased exponentially and automatically when the
state changes from non-error to error.

BUG=none
==========

to

==========
Add a TimeUntilNextTry to reading list

This CL adds a method to get the time delta until the next try for a reading
list entry. This method is increased exponentially and automatically when the
state changes from non-error to error.

BUG=none

Committed: https://crrev.com/cb3322bfbf31dac33a17266afca8c57a10c2a2e4
Cr-Commit-Position: refs/heads/master@{#420337}
==========

[commit-bot: I haz the power, 2016-09-22 15:01:20]

Patchset 6 (id:??) landed as
https://crrev.com/cb3322bfbf31dac33a17266afca8c57a10c2a2e4
Cr-Commit-Position: refs/heads/master@{#420337}