Retries with exponential backoff for dismissals

Retries with exponential backoff for dismissals.
Unification of retry mechanisms for updating cards and dismissals.

BUG=164227
TEST=For updating cards: all previously existing tests apply. Make sure that existing tests include checking for exponentially growing (with random jitter) retry period starting with a random value between 5-6 min till we get to 1 hour. Make sure that the tests check that after successfully receiving cards, next update will be exactly after the time suggested by the server.
For dismissals: modify existing tests to expect exponentially growing retry period, starting from 1-1.2 minutes, capped by 1 hour. After dismissal success, dismissal retries should stop.

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

[skare_, 2013-05-11 00:56:23]

https://codereview.chromium.org/14743007/diff/2001/chrome/browser/resources/g...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/14743007/diff/2001/chrome/browser/resources/g...
chrome/browser/resources/google_now/utility.js:270: storage.set(items);
this is just syntax but either

var items = {cDSK: nRDS};
storage.set(items);

or 
storage.set({currentDelayStorageKey: newRetryDelaySeconds})
?

https://codereview.chromium.org/14743007/diff/2001/chrome/browser/resources/g...
chrome/browser/resources/google_now/utility.js:279: function
start(optionalFirstDelaySeconds) {
opt_FirstDelaySeconds
(s/optional/opt_ unless something supersedes the general js style)

[skare_, 2013-05-11 01:03:05]

https://codereview.chromium.org/14743007/diff/2001/chrome/browser/resources/g...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/14743007/diff/2001/chrome/browser/resources/g...
chrome/browser/resources/google_now/utility.js:279: function
start(optionalFirstDelaySeconds) {
On 2013/05/11 00:56:23, Travis Skare wrote:
> opt_FirstDelaySeconds
> (s/optional/opt_ unless something supersedes the general js style)

typo -- opt_firstDelaySeconds actually.

[vadimt, 2013-05-11 01:55:15]

https://codereview.chromium.org/14743007/diff/2001/chrome/browser/resources/g...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/14743007/diff/2001/chrome/browser/resources/g...
chrome/browser/resources/google_now/utility.js:270: storage.set(items);
On 2013/05/11 00:56:23, Travis Skare wrote:
> this is just syntax but either
> 
> var items = {cDSK: nRDS};
> storage.set(items);
> 
> or 
> storage.set({currentDelayStorageKey: newRetryDelaySeconds})
> ?

This would create a field 'currentDelayStorageKey' not 'XXX-current-delay'.

https://codereview.chromium.org/14743007/diff/2001/chrome/browser/resources/g...
chrome/browser/resources/google_now/utility.js:279: function
start(optionalFirstDelaySeconds) {
On 2013/05/11 00:56:23, Travis Skare wrote:
> opt_FirstDelaySeconds
> (s/optional/opt_ unless something supersedes the general js style)

Done.

[skare_, 2013-05-11 02:07:19]

https://codereview.chromium.org/14743007/diff/2001/chrome/browser/resources/g...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/14743007/diff/2001/chrome/browser/resources/g...
chrome/browser/resources/google_now/utility.js:270: storage.set(items);
On 2013/05/11 01:55:15, vadimt wrote:
> On 2013/05/11 00:56:23, Travis Skare wrote:
> > this is just syntax but either
> > 
> > var items = {cDSK: nRDS};
> > storage.set(items);
> > 
> > or 
> > storage.set({currentDelayStorageKey: newRetryDelaySeconds})
> > ?
> 
> This would create a field 'currentDelayStorageKey' not 'XXX-current-delay'.

[ok]
oops, thought this was a literal. 
Might want an underscore on currentDelayStorageKey_ -- normally it would suggest
a private property on an object but I'm not totally sure if that applies in a
closure. Will check.

[rgustafson, 2013-05-14 01:25:17]

https://codereview.chromium.org/14743007/diff/12001/chrome/browser/resources/...
File chrome/browser/resources/google_now/manifest.json (right):

https://codereview.chromium.org/14743007/diff/12001/chrome/browser/resources/...
chrome/browser/resources/google_now/manifest.json:4: "version": "1.3",
I suggest the lower parts of version 1.2.0.X or 1.2.X so this number doesn't get
huge.

https://codereview.chromium.org/14743007/diff/12001/chrome/browser/resources/...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/14743007/diff/12001/chrome/browser/resources/...
chrome/browser/resources/google_now/utility.js:257: *     retry attemps, if
specified. Not specified for scheduling first retry
attempts

https://codereview.chromium.org/14743007/diff/12001/chrome/browser/resources/...
chrome/browser/resources/google_now/utility.js:300: function
planForNext(callback) {
The wording of this confused me too. At the least, include the fact that the
callback is called after scheduling the next attempt in the function comment.
Just passing a function in looks like that's what you're going to do next time
rather than something to be called now, after the next one is scheduled.

https://codereview.chromium.org/14743007/diff/12001/chrome/browser/resources/...
chrome/browser/resources/google_now/utility.js:304:
scheduleNextAttempt(items[currentDelayStorageKey]);
This value isn't going to exist if you call this after start with
opt_firstDelaySeconds, right? It only gets set in scheduleNextAttempt.

[skare_, 2013-05-14 02:03:52]

in general, functionality looks good.

high-level comment:
could this patch become easier to read by having the functionality closer to the
network requests? Can chat.

https://codereview.chromium.org/14743007/diff/12001/chrome/browser/resources/...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/14743007/diff/12001/chrome/browser/resources/...
chrome/browser/resources/google_now/background.js:306:
updateCardsAttempts.start(parsedResponse.expiration_timestamp_seconds);
just as an opinion as a first-time reader, name for these is slightly hard to
read but I don't have a great suggestion --
s/updateCardsAttempts/updateCardsRequest?

https://codereview.chromium.org/14743007/diff/12001/chrome/browser/resources/...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/14743007/diff/12001/chrome/browser/resources/...
chrome/browser/resources/google_now/utility.js:230: * @param {function()}
attempt Activity that the manager retries till it
nit: expand till->until

https://codereview.chromium.org/14743007/diff/12001/chrome/browser/resources/...
chrome/browser/resources/google_now/utility.js:261: var base =
opt_previousDelaySeconds ? opt_previousDelaySeconds * 2 :
it's likely this isn't a concern since this isn't exposed publicly and this
class will be the only caller, but just in case -- do you need to differentiate
undefined from 0?

[vadimt, 2013-05-14 18:47:50]

https://codereview.chromium.org/14743007/diff/12001/chrome/browser/resources/...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/14743007/diff/12001/chrome/browser/resources/...
chrome/browser/resources/google_now/background.js:306:
updateCardsAttempts.start(parsedResponse.expiration_timestamp_seconds);
On 2013/05/14 02:03:52, Travis Skare wrote:
> just as an opinion as a first-time reader, name for these is slightly hard to
> read but I don't have a great suggestion --
> s/updateCardsAttempts/updateCardsRequest?

'Request' has connotations with a single network request. 'Attempts' stresses
the repeating nature of this beast.

https://codereview.chromium.org/14743007/diff/12001/chrome/browser/resources/...
File chrome/browser/resources/google_now/manifest.json (right):

https://codereview.chromium.org/14743007/diff/12001/chrome/browser/resources/...
chrome/browser/resources/google_now/manifest.json:4: "version": "1.3",
On 2013/05/14 01:25:17, rgustafson wrote:
> I suggest the lower parts of version 1.2.0.X or 1.2.X so this number doesn't
get
> huge.

Done.

https://codereview.chromium.org/14743007/diff/12001/chrome/browser/resources/...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/14743007/diff/12001/chrome/browser/resources/...
chrome/browser/resources/google_now/utility.js:230: * @param {function()}
attempt Activity that the manager retries till it
On 2013/05/14 02:03:52, Travis Skare wrote:
> nit: expand till->until

Done.

https://codereview.chromium.org/14743007/diff/12001/chrome/browser/resources/...
chrome/browser/resources/google_now/utility.js:257: *     retry attemps, if
specified. Not specified for scheduling first retry
On 2013/05/14 01:25:17, rgustafson wrote:
> attempts

Done.

https://codereview.chromium.org/14743007/diff/12001/chrome/browser/resources/...
chrome/browser/resources/google_now/utility.js:261: var base =
opt_previousDelaySeconds ? opt_previousDelaySeconds * 2 :
On 2013/05/14 02:03:52, Travis Skare wrote:
> it's likely this isn't a concern since this isn't exposed publicly and this
> class will be the only caller, but just in case -- do you need to
differentiate
> undefined from 0?

No. Anyways, we'd need to special-case 0, otherwise the calculations would
result in 0 again.

https://codereview.chromium.org/14743007/diff/12001/chrome/browser/resources/...
chrome/browser/resources/google_now/utility.js:300: function
planForNext(callback) {
On 2013/05/14 01:25:17, rgustafson wrote:
> The wording of this confused me too. At the least, include the fact that the
> callback is called after scheduling the next attempt in the function comment.
> Just passing a function in looks like that's what you're going to do next time
> rather than something to be called now, after the next one is scheduled.

Done.

https://codereview.chromium.org/14743007/diff/12001/chrome/browser/resources/...
chrome/browser/resources/google_now/utility.js:304:
scheduleNextAttempt(items[currentDelayStorageKey]);
On 2013/05/14 01:25:17, rgustafson wrote:
> This value isn't going to exist if you call this after start with
> opt_firstDelaySeconds, right? It only gets set in scheduleNextAttempt.

Yes, in this case, we start from the default value here.

[rgustafson, 2013-05-14 21:42:11]

lgtm

[skare_, 2013-05-14 22:55:29]

lgtm

Reads clearer second time through but just as an fyi it seemed a little heavy
the first pass -- perhaps I needed coffee.

[vadimt, 2013-05-14 23:00:20]

arv@, please provide OWNER's approval

[arv (Not doing code reviews), 2013-05-15 17:25:36]

LGTM

https://codereview.chromium.org/14743007/diff/23001/chrome/browser/resources/...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/14743007/diff/23001/chrome/browser/resources/...
chrome/browser/resources/google_now/utility.js:236: function
buildAttemptManager(
In general build and create functions should be replaced with constructors. Then
you can do `new AttemptManager` instead. This way it easier to reuse code. In
this case you only have a few instances so performance is not an issue, but if
you need a lot of instances using the "class pattern" gives you a lot better
memory and runtime performance.

[commit-bot: I haz the power, 2013-05-15 18:15:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vadimt@chromium.org/14743007/23001

[vadimt, 2013-05-15 19:05:17]

https://codereview.chromium.org/14743007/diff/23001/chrome/browser/resources/...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/14743007/diff/23001/chrome/browser/resources/...
chrome/browser/resources/google_now/utility.js:236: function
buildAttemptManager(
On 2013/05/15 17:25:36, arv wrote:
> In general build and create functions should be replaced with constructors.
Then
> you can do `new AttemptManager` instead. This way it easier to reuse code. In
> this case you only have a few instances so performance is not an issue, but if
> you need a lot of instances using the "class pattern" gives you a lot better
> memory and runtime performance.

Ack

[commit-bot: I haz the power, 2013-05-15 22:17:11]

Change committed as 200368