ntp4: improved app install, try 2

ntp4: improved app install, try 2

1. install to the correct page (0 -> -1 in crx_installer)
2. scroll to show the app (scrollTop change in apps_page.js)
3. reset tiles after trashing an app (trash.js change)
4. fix install-display for already-open NTPs. The old technique of always navigating stacked up history entries when the NTP had already been open. It was also flaky (seemed to be vulnerable to some race). Now we use an install notification which the NTP in question will be listening for.

BUG=92995, 91583
TEST=manual

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

[Rick Byers, 2011-08-29 16:01:00]

http://codereview.chromium.org/7776001/diff/3001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_install_ui.cc (right):

http://codereview.chromium.org/7776001/diff/3001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_install_ui.cc:235: "%s#app-id=%s",
chrome::kChromeUINewTabURL, app_id.c_str());
Why do you still need the code for setting and reading the hash?  I would have
thought that supporting the "already open NTP" case with the notification could
also easily handle the case where no NTP is open.  It's confusing having two
different channels for the information about which app to highlight....

http://codereview.chromium.org/7776001/diff/3001/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/7776001/diff/3001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:419: console.log('selecting card');
We don't generally want to leave diagnostic logging in the code, right?

http://codereview.chromium.org/7776001/diff/3001/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/7776001/diff/3001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.js:373: highlightAppId = appId;
Should you perhaps assert that highlightAppId is null?  Or is it possible that
we'd get two app installs in a row without an intervening getAppsCallback?  In
which case is it OK to just highlight the last?  Probably at least worth a
comment.  I assume it's not worth the extra complexity to keep a list and
highlight all in the list.

http://codereview.chromium.org/7776001/diff/3001/chrome/common/chrome_notific...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/7776001/diff/3001/chrome/common/chrome_notific...
chrome/common/chrome_notification_types.h:845: // TabContents that was shown,
and Details is the Extension which was
s/Extension/extension ID

[Evan Stade, 2011-08-29 18:15:16]

http://codereview.chromium.org/7776001/diff/3001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_install_ui.cc (right):

http://codereview.chromium.org/7776001/diff/3001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_install_ui.cc:235: "%s#app-id=%s",
chrome::kChromeUINewTabURL, app_id.c_str());
On 2011/08/29 16:01:00, Rick Byers wrote:
> Why do you still need the code for setting and reading the hash?  I would have
> thought that supporting the "already open NTP" case with the notification
could
> also easily handle the case where no NTP is open.  It's confusing having two
> different channels for the information about which app to highlight....

hmm. I tried to do what you're suggesting and it didn't work, so I assumed that
the new AppLauncherHandler wasn't being constructed till after this notification
(i.e. that Navigate wasn't creating it synchronously), although now that I test
construction order explicitly that doesn't seem to be the case. I'll see if I
can fix it because I agree the #app-id hack is annoying.

http://codereview.chromium.org/7776001/diff/3001/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/apps_page.js (right):

http://codereview.chromium.org/7776001/diff/3001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/apps_page.js:419: console.log('selecting card');
On 2011/08/29 16:01:00, Rick Byers wrote:
> We don't generally want to leave diagnostic logging in the code, right?

correct, left here by mistake

http://codereview.chromium.org/7776001/diff/3001/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/7776001/diff/3001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.js:373: highlightAppId = appId;
On 2011/08/29 16:01:00, Rick Byers wrote:
> Should you perhaps assert that highlightAppId is null?  Or is it possible that
> we'd get two app installs in a row without an intervening getAppsCallback?  In
> which case is it OK to just highlight the last?  Probably at least worth a
> comment.  I assume it's not worth the extra complexity to keep a list and
> highlight all in the list.

yea I assume it's possible if unlikely to have a race condition with installing
multiple apps. And it's not just an intervening call to getAppsCallback, usually
after setAppToBeHighlighted there will be an appAdded call. I'm not sure what
would go into a comment, something like

// This should be null but it's harmless if we are overwriting something.

is in my mind adds verbosity but doesn't really explain anything subtle.

[Evan Stade, 2011-08-29 19:07:20]

updated; #app-id removed. Had to move the new notification registration to
Attach() (so we would get notifications before GetApps was called).

http://codereview.chromium.org/7776001/diff/3001/chrome/common/chrome_notific...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/7776001/diff/3001/chrome/common/chrome_notific...
chrome/common/chrome_notification_types.h:845: // TabContents that was shown,
and Details is the Extension which was
On 2011/08/29 16:01:00, Rick Byers wrote:
> s/Extension/extension ID

Done.

[Rick Byers, 2011-08-29 19:51:12]

LGTM

http://codereview.chromium.org/7776001/diff/3001/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/7776001/diff/3001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.js:373: highlightAppId = appId;
On 2011/08/29 18:15:16, Evan Stade wrote:
> On 2011/08/29 16:01:00, Rick Byers wrote:
> > Should you perhaps assert that highlightAppId is null?  Or is it possible
that
> > we'd get two app installs in a row without an intervening getAppsCallback? 
In
> > which case is it OK to just highlight the last?  Probably at least worth a
> > comment.  I assume it's not worth the extra complexity to keep a list and
> > highlight all in the list.
> 
> yea I assume it's possible if unlikely to have a race condition with
installing
> multiple apps. And it's not just an intervening call to getAppsCallback,
usually
> after setAppToBeHighlighted there will be an appAdded call. I'm not sure what
> would go into a comment, something like
> 
> // This should be null but it's harmless if we are overwriting something.
> 
> is in my mind adds verbosity but doesn't really explain anything subtle.

Yeah, I guess the race potential and highlight-last behavior is fairly obvious
and OK not to dwell on.

http://codereview.chromium.org/7776001/diff/8002/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

http://codereview.chromium.org/7776001/diff/8002/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:281:
SetAppToBeHighlighted();
I assume we can't do this unconditionally because it may be too early for the
setAppToBeHighlighted function to even run, right?  It seems a shame to have an
"app to highlight" here, which at some later point gets pushed to JS and cached
again, which at some later point actually causes a behavior change.  But if we
really can't reliably call into the JS this early, then I don't see an obviously
simpler option.

[Evan Stade, 2011-08-29 20:30:51]

http://codereview.chromium.org/7776001/diff/8002/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

http://codereview.chromium.org/7776001/diff/8002/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:281:
SetAppToBeHighlighted();
On 2011/08/29 19:51:12, Rick Byers wrote:
> I assume we can't do this unconditionally because it may be too early for the
> setAppToBeHighlighted function to even run, right?  It seems a shame to have
an
> "app to highlight" here, which at some later point gets pushed to JS and
cached
> again, which at some later point actually causes a behavior change.  But if we
> really can't reliably call into the JS this early, then I don't see an
obviously
> simpler option.

yes, the js isn't ready yet.