Using “is opted in” server request

Using “is opted in” server request. Also, taking into account ‘googleNowDisabled’ value in server response.


BUG=164227
TEST=
1. Install Chrome, make sure the first request it does is not poll for cards, but a poll for opted-in state (URL + ‘settings/optin’). If the user is opted in, this should be followed by a normal request for cards. If the user is opted out, make sure the next poll is in 1 week (stop by to learn how to check this).
2. While polling for cards, opt out the user fro Now. Force a server ping; make sure that the next poll is scheduled to be in 2 week.

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

[vadimt, 2013-10-31 00:37:17]

Note that I couldn't quickly make experimental variation params work (they used
to work for the extension in the past). May be, this is a regression in Finch
(unlikely). I’ll figure out this later; in the worst case, we’ll get stuck with
1-week fixed update time till this is fixed in Stable.

[skare_, 2013-10-31 00:49:21]

https://codereview.chromium.org/53823002/diff/1/chrome/browser/resources/goog...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/53823002/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:430: *     information. If
this map is empty, the user is not opted in to Google
consider making signed-in state an explicit param

https://codereview.chromium.org/53823002/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:591: * @param {function()}
successCallback Function that will be called if
maybe add param to make it successCallback(bool isOptedIn) or rename the
function from successCallback since the request could be ok but user not signed
in.

[vadimt, 2013-10-31 14:36:00]

https://codereview.chromium.org/53823002/diff/1/chrome/browser/resources/goog...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/53823002/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:430: *     information. If
this map is empty, the user is not opted in to Google
On 2013/10/31 00:49:21, Travis Skare wrote:
> consider making signed-in state an explicit param

I thought it would make sense as is to keep the extension afloat if the server
sends empty set of groups by mistake. Instead of crashing, the client will
re-request within 1 week. Makes sense?

https://codereview.chromium.org/53823002/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:591: * @param {function()}
successCallback Function that will be called if
On 2013/10/31 00:49:21, Travis Skare wrote:
> maybe add param to make it successCallback(bool isOptedIn) or rename the
> function from successCallback since the request could be ok but user not
signed
> in.

Done.

[robliao, 2013-10-31 16:42:36]

lgtm

[rgustafson, 2013-10-31 18:37:58]

https://codereview.chromium.org/53823002/diff/1/chrome/browser/resources/goog...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/53823002/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:430: *     information. If
this map is empty, the user is not opted in to Google
On 2013/10/31 14:36:00, vadimt wrote:
> On 2013/10/31 00:49:21, Travis Skare wrote:
> > consider making signed-in state an explicit param
> 
> I thought it would make sense as is to keep the extension afloat if the server
> sends empty set of groups by mistake. Instead of crashing, the client will
> re-request within 1 week. Makes sense?

It's important to handle the possibility of no groups when the user should be
getting notifications in a way doesn't make the extension stop functioning, but
I don't think using the opted out state is the best way. Maybe another default
value? 1 day?

Btw, extremely unlikely that it would happen, but breaking with no chance of
recovery is bad.

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

https://codereview.chromium.org/53823002/diff/60001/chrome/browser/resources/...
chrome/browser/resources/google_now/background.js:433: function
scheduleNextPoll(groups) {
Why not add another optional param for boolean opted out? Or better yet, use a
different function scheduleNextOptInPoll? 

It's safe to say that if there are groups when opted out, we shouldn't pay
attention to them. Then there's no overlapping code here between the two
functions for scheduling a notifications poll and opt in.

https://codereview.chromium.org/53823002/diff/60001/chrome/browser/resources/...
chrome/browser/resources/google_now/background.js:483: if
(parsedResponse.googleNowDisabled) {
Why not call the function to schedule next poll then early return?

None of the code below is used. It's mostly just for loops with 0 items, but
once you get to showNotificationCards, there's also unnecessary storage reads,
etc. Early return prevents future readers from having to hunt for what else will
happen.

You might still want to count it as cards_parse_success though.

[vadimt, 2013-10-31 19:59:56]

https://codereview.chromium.org/53823002/diff/1/chrome/browser/resources/goog...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/53823002/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:430: *     information. If
this map is empty, the user is not opted in to Google
On 2013/10/31 18:37:59, rgustafson wrote:
> On 2013/10/31 14:36:00, vadimt wrote:
> > On 2013/10/31 00:49:21, Travis Skare wrote:
> > > consider making signed-in state an explicit param
> > 
> > I thought it would make sense as is to keep the extension afloat if the
server
> > sends empty set of groups by mistake. Instead of crashing, the client will
> > re-request within 1 week. Makes sense?
> 
> It's important to handle the possibility of no groups when the user should be
> getting notifications in a way doesn't make the extension stop functioning,
but
> I don't think using the opted out state is the best way. Maybe another default
> value? 1 day?
> 
> Btw, extremely unlikely that it would happen, but breaking with no chance of
> recovery is bad.

Added parameter. Restored old behavior for opted-in users, which is throw an
exception and retry in 1 hour (we use a recurring timer that never stops).

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

https://codereview.chromium.org/53823002/diff/60001/chrome/browser/resources/...
chrome/browser/resources/google_now/background.js:433: function
scheduleNextPoll(groups) {
On 2013/10/31 18:37:59, rgustafson wrote:
> Why not add another optional param for boolean opted out? Or better yet, use a
> different function scheduleNextOptInPoll? 
> 
> It's safe to say that if there are groups when opted out, we shouldn't pay
> attention to them. Then there's no overlapping code here between the two
> functions for scheduling a notifications poll and opt in.

Added param. Note that we happen to call this function with a non-constant
param.

https://codereview.chromium.org/53823002/diff/60001/chrome/browser/resources/...
chrome/browser/resources/google_now/background.js:483: if
(parsedResponse.googleNowDisabled) {
On 2013/10/31 18:37:59, rgustafson wrote:
> Why not call the function to schedule next poll then early return?
> 
> None of the code below is used. It's mostly just for loops with 0 items, but
> once you get to showNotificationCards, there's also unnecessary storage reads,
> etc. Early return prevents future readers from having to hunt for what else
will
> happen.
> 
> You might still want to count it as cards_parse_success though.

We want to delete cards, avoid restoring cards on startup, clean cards triggers,
store empty groups set...

[rgustafson, 2013-10-31 20:50:48]

lgtm

[commit-bot: I haz the power, 2013-10-31 21:12:45]

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

[commit-bot: I haz the power, 2013-10-31 23:48:14]

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

[commit-bot: I haz the power, 2013-11-01 13:57:10]

Change committed as 232365