Restoring notifications on startup

Restoring notifications on startup without making a server poll.

BUG=164227
TEST=Receive cards. Close Chrome process. Disconnect from network. Start Chrome. Cards should be shown on startup.
Alternatively: Receive cards. In less than 5 minutes: Close Chrome process, start it, make sure cards are shown and the devtools log doesn't show a server request.

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

[robliao, 2013-10-14 21:05:31]

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

https://codereview.chromium.org/27030012/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:524:
mergeAndShowNotificationCards();
Seems like overkill to set notificationGroups and then immediately read it again
in this function.

A better way would be to abstract the get out of mergeAndShowNotificationCards
and have it accept a parameter that is simply the notificationGroups.

https://codereview.chromium.org/27030012/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:1096:
tasks.add(SHOW_ON_START_TASK_NAME, mergeAndShowNotificationCards);
This should occur after initialize.

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

https://codereview.chromium.org/27030012/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/cards.js:222: tasks.add(SHOW_CARD_TASK_NAME,
function() {
Why is this going to a task?

[vadimt, 2013-10-14 23:51:57]

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

https://codereview.chromium.org/27030012/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:524:
mergeAndShowNotificationCards();
On 2013/10/14 21:05:31, Robert Liao wrote:
> Seems like overkill to set notificationGroups and then immediately read it
again
> in this function.
> 
> A better way would be to abstract the get out of mergeAndShowNotificationCards
> and have it accept a parameter that is simply the notificationGroups.

I'd not do serious code refactorings, assuming that persistent notifications are
coming. And the perf benefit would be not noticeable, given that we do this only
once per Chrome's process life (per session).

https://codereview.chromium.org/27030012/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:1096:
tasks.add(SHOW_ON_START_TASK_NAME, mergeAndShowNotificationCards);
On 2013/10/14 21:05:31, Robert Liao wrote:
> This should occur after initialize.

Why? Imagine that persistent notifications are implemented. Then, cards would be
restored very early, may be, before any JS code ran at all.

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

https://codereview.chromium.org/27030012/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/cards.js:222: tasks.add(SHOW_CARD_TASK_NAME,
function() {
On 2013/10/14 21:05:31, Robert Liao wrote:
> Why is this going to a task?

It manipulates with storage and can create notifications. Polling for cards and
restoring cards compete for same resources. So, task is a good idea.

[robliao, 2013-10-14 23:56:08]

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

https://codereview.chromium.org/27030012/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:524:
mergeAndShowNotificationCards();
This rearrangement only affects new code, namely mergeAndShowNotificationCards.

mergeAndShowNotificationCards can simply remove the get call and receive the
cards as an argument. 

On startup, you can add GetMergeAndShowNotificaitonCards that does the get step
and calls mergeAndShowNotificationCards.

This makes the startup path obvious that we're obtaining state and avoids the
whole set re-get code.

On 2013/10/14 23:51:57, vadimt wrote:
> On 2013/10/14 21:05:31, Robert Liao wrote:
> > Seems like overkill to set notificationGroups and then immediately read it
> again
> > in this function.
> > 
> > A better way would be to abstract the get out of
mergeAndShowNotificationCards
> > and have it accept a parameter that is simply the notificationGroups.
> 
> I'd not do serious code refactorings, assuming that persistent notifications
are
> coming. And the perf benefit would be not noticeable, given that we do this
only
> once per Chrome's process life (per session).

https://codereview.chromium.org/27030012/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:1096:
tasks.add(SHOW_ON_START_TASK_NAME, mergeAndShowNotificationCards);
It is non-obvious to an outside reader why anything should occur before an
initialize step.
On 2013/10/14 23:51:57, vadimt wrote:
> On 2013/10/14 21:05:31, Robert Liao wrote:
> > This should occur after initialize.
> 
> Why? Imagine that persistent notifications are implemented. Then, cards would
be
> restored very early, may be, before any JS code ran at all.

[vadimt, 2013-10-15 01:09:43]

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

https://codereview.chromium.org/27030012/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:524:
mergeAndShowNotificationCards();
On 2013/10/14 23:56:08, Robert Liao wrote:
> This rearrangement only affects new code, namely
mergeAndShowNotificationCards.
> 
> mergeAndShowNotificationCards can simply remove the get call and receive the
> cards as an argument. 
> 
> On startup, you can add GetMergeAndShowNotificaitonCards that does the get
step
> and calls mergeAndShowNotificationCards.
> 
> This makes the startup path obvious that we're obtaining state and avoids the
> whole set re-get code.
> 
> On 2013/10/14 23:51:57, vadimt wrote:
> > On 2013/10/14 21:05:31, Robert Liao wrote:
> > > Seems like overkill to set notificationGroups and then immediately read it
> > again
> > > in this function.
> > > 
> > > A better way would be to abstract the get out of
> mergeAndShowNotificationCards
> > > and have it accept a parameter that is simply the notificationGroups.
> > 
> > I'd not do serious code refactorings, assuming that persistent notifications
> are
> > coming. And the perf benefit would be not noticeable, given that we do this
> only
> > once per Chrome's process life (per session).
> 

Done.

https://codereview.chromium.org/27030012/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:1096:
tasks.add(SHOW_ON_START_TASK_NAME, mergeAndShowNotificationCards);
On 2013/10/14 23:56:08, Robert Liao wrote:
> It is non-obvious to an outside reader why anything should occur before an
> initialize step.
> On 2013/10/14 23:51:57, vadimt wrote:
> > On 2013/10/14 21:05:31, Robert Liao wrote:
> > > This should occur after initialize.
> > 
> > Why? Imagine that persistent notifications are implemented. Then, cards
would
> be
> > restored very early, may be, before any JS code ran at all.
> 

Done.

[robliao, 2013-10-15 01:12:59]

lgtm

[rgustafson, 2013-10-15 21:14:33]

https://codereview.chromium.org/27030012/diff/9001/chrome/browser/resources/g...
File chrome/browser/resources/google_now/background.js (left):

https://codereview.chromium.org/27030012/diff/9001/chrome/browser/resources/g...
chrome/browser/resources/google_now/background.js:478: var updatedGroups = {};
For updatedGroups -> items.notificationGroups

What if a group disappears?

https://codereview.chromium.org/27030012/diff/9001/chrome/browser/resources/g...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/27030012/diff/9001/chrome/browser/resources/g...
chrome/browser/resources/google_now/background.js:453: // Merge cards from all
groups into one set.
nit: comment not really necessary. If you want more detail, put it in the
function comment since this is describing the whole function minus
showNotificationCards which is self explanatory.

[vadimt, 2013-10-15 21:28:02]

https://codereview.chromium.org/27030012/diff/9001/chrome/browser/resources/g...
File chrome/browser/resources/google_now/background.js (left):

https://codereview.chromium.org/27030012/diff/9001/chrome/browser/resources/g...
chrome/browser/resources/google_now/background.js:478: var updatedGroups = {};
On 2013/10/15 21:14:33, rgustafson wrote:
> For updatedGroups -> items.notificationGroups
> 
> What if a group disappears? 

Ah, looks like I forgot the protocol...
So, the server will send all existing groups, always, correct?

https://codereview.chromium.org/27030012/diff/9001/chrome/browser/resources/g...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/27030012/diff/9001/chrome/browser/resources/g...
chrome/browser/resources/google_now/background.js:453: // Merge cards from all
groups into one set.
On 2013/10/15 21:14:33, rgustafson wrote:
> nit: comment not really necessary. If you want more detail, put it in the
> function comment since this is describing the whole function minus
> showNotificationCards which is self explanatory.

Done.

[rgustafson, 2013-10-15 21:43:41]

https://codereview.chromium.org/27030012/diff/9001/chrome/browser/resources/g...
File chrome/browser/resources/google_now/background.js (left):

https://codereview.chromium.org/27030012/diff/9001/chrome/browser/resources/g...
chrome/browser/resources/google_now/background.js:478: var updatedGroups = {};
On 2013/10/15 21:28:03, vadimt wrote:
> On 2013/10/15 21:14:33, rgustafson wrote:
> > For updatedGroups -> items.notificationGroups
> > 
> > What if a group disappears? 
> 
> Ah, looks like I forgot the protocol...
> So, the server will send all existing groups, always, correct?

This is the joy of having extensibility which there are no plans to use. :) With
the existing server implementation, all groups will always be sent so this would
be fine. If we were to add more groups with more specific requests, those would
likely only exist for the lifetime of one notification or less. There is a less
than one percent chance imo of another group being made which behaves in the
exact same way as the existing groups; new groups would be transient. It's up to
you how far you want to go with making this extensible since I really do not see
this being used any time soon.

[vadimt, 2013-10-15 22:05:36]

https://codereview.chromium.org/27030012/diff/9001/chrome/browser/resources/g...
File chrome/browser/resources/google_now/background.js (left):

https://codereview.chromium.org/27030012/diff/9001/chrome/browser/resources/g...
chrome/browser/resources/google_now/background.js:478: var updatedGroups = {};
On 2013/10/15 21:43:41, rgustafson wrote:
> On 2013/10/15 21:28:03, vadimt wrote:
> > On 2013/10/15 21:14:33, rgustafson wrote:
> > > For updatedGroups -> items.notificationGroups
> > > 
> > > What if a group disappears? 
> > 
> > Ah, looks like I forgot the protocol...
> > So, the server will send all existing groups, always, correct?
> 
> This is the joy of having extensibility which there are no plans to use. :)
With
> the existing server implementation, all groups will always be sent so this
would
> be fine. If we were to add more groups with more specific requests, those
would
> likely only exist for the lifetime of one notification or less. There is a
less
> than one percent chance imo of another group being made which behaves in the
> exact same way as the existing groups; new groups would be transient. It's up
to
> you how far you want to go with making this extensible since I really do not
see
> this being used any time soon.

Thanks; you've found a regression; I'm fixing it.

[rgustafson, 2013-10-15 23:43:34]

lgtm

[commit-bot: I haz the power, 2013-10-16 00:11:25]

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

[commit-bot: I haz the power, 2013-10-16 03:45:05]

Change committed as 228856