ntp4: new app animation

ntp4: new app animation

The new app animation already worked, but only if you have the new tab page already open (installing the app will switch to already-existent NTP and show the app being added).

Now it also works if the app install triggers a new NTP.

BUG=91583
TEST=try installing new app with and without NTP already open.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95433

[Rick Byers, 2011-08-04 13:21:58]

http://codereview.chromium.org/7577002/diff/1/chrome/browser/resources/ntp4/n...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/7577002/diff/1/chrome/browser/resources/ntp4/n...
chrome/browser/resources/ntp4/new_tab.js:307: appAdded(highlightApp);
Is there some reason you can't just do this call to appAdded directly above,
instead of waiting until the end?

If (eg. the user navigates to the hash URL directly or has it bookmarked, etc.),
this app isn't actually the last app on the page, this will put it in the wrong
place.  Then if they do an unrelated rearrange, the new location will be
persisted.  A pretty minor bug, but I could see it annoying some users.

If you can't just move this up for some reason, then maybe you should only check
for (what should be the common case) of the app being the last one on the page.

[Evan Stade, 2011-08-04 16:35:02]

http://codereview.chromium.org/7577002/diff/1/chrome/browser/resources/ntp4/n...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/7577002/diff/1/chrome/browser/resources/ntp4/n...
chrome/browser/resources/ntp4/new_tab.js:307: appAdded(highlightApp);
On 2011/08/04 13:21:58, Rick Byers wrote:
> Is there some reason you can't just do this call to appAdded directly above,
> instead of waiting until the end?

All the cards have to be present and correct.

> 
> If (eg. the user navigates to the hash URL directly or has it bookmarked,
etc.),

The first example is officially not supported (and almost impossible for it to
happen by accident). The second example I suppose might possibly happen if you
bookmark the ntp at the right time. I think the best way to fix this is simply
clearing the hash value after reading it.

> this app isn't actually the last app on the page, this will put it in the
wrong
> place.  Then if they do an unrelated rearrange, the new location will be
> persisted.  A pretty minor bug, but I could see it annoying some users.

even if it is the last app on the page, you wouldn't want to animate it every
time (in the bookmark example)

> 
> If you can't just move this up for some reason, then maybe you should only
check
> for (what should be the common case) of the app being the last one on the
page.

[Rick Byers, 2011-08-04 16:54:19]

LGTM

http://codereview.chromium.org/7577002/diff/1/chrome/browser/resources/ntp4/n...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/7577002/diff/1/chrome/browser/resources/ntp4/n...
chrome/browser/resources/ntp4/new_tab.js:307: appAdded(highlightApp);
On 2011/08/04 16:35:02, Evan Stade wrote:
> On 2011/08/04 13:21:58, Rick Byers wrote:
> > Is there some reason you can't just do this call to appAdded directly above,
> > instead of waiting until the end?
> 
> All the cards have to be present and correct.
> 
> > 
> > If (eg. the user navigates to the hash URL directly or has it bookmarked,
> etc.),
> 
> The first example is officially not supported (and almost impossible for it to
> happen by accident). The second example I suppose might possibly happen if you
> bookmark the ntp at the right time. I think the best way to fix this is simply
> clearing the hash value after reading it.
> 
> > this app isn't actually the last app on the page, this will put it in the
> wrong
> > place.  Then if they do an unrelated rearrange, the new location will be
> > persisted.  A pretty minor bug, but I could see it annoying some users.
> 
> even if it is the last app on the page, you wouldn't want to animate it every
> time (in the bookmark example)
> 
> > 
> > If you can't just move this up for some reason, then maybe you should only
> check
> > for (what should be the common case) of the app being the last one on the
> page.
> 

Yeah, I think that's good enough.  Thanks.