ntp4: app install improvements

ntp4: app install improvements

1. when you install a new app, only one NTP will highlight (i.e. switch to the correct page for that app), rather than any open NTP
2. when you do an app install via dragging a bookmark or most visited tile to a navigation dot, work correctly instead of asserting
3. correct drop effect for dragging onto navigation dots

BUG=93159
TEST=see above

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

[Rick Byers, 2011-08-23 20:52:12]

Just a couple minor suggestions

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

http://codereview.chromium.org/7685049/diff/2001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.js:327:
cardSlider.currentCardValue.navigationDot.classList.add('selected');
Looks like you're not using the 'dots' list anymore, remove it.

http://codereview.chromium.org/7685049/diff/2001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.js:340: if (hash &&
hash.indexOf('#app-id=') == 0 && app.id == hash.split('=')[1]) {
the logic to parse the hash is duplicated in initialize.  I suggest adding a
little helper function to reduce/eliminate the duplication (eg.
getAndClearHashAppId - but it'll be a little more complicated than that if you
want to avoid unconditionally clearing it here).

http://codereview.chromium.org/7685049/diff/2001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.js:350: page = new ntp4.AppsPage();
is it not possible for page_index to be more than 1 greater than the number of
current pages?  Eg., what if all apps are removed from some middle page?  I'd
suggest instead looping here until you know you have enough pages...

http://codereview.chromium.org/7685049/diff/2001/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/app_launcher_handler.h (right):

http://codereview.chromium.org/7685049/diff/2001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.h:194: // waiting to here about
success or failure from the extensions system.
s/here/hear

[Evan Stade, 2011-08-25 03:08:56]

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

http://codereview.chromium.org/7685049/diff/2001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.js:327:
cardSlider.currentCardValue.navigationDot.classList.add('selected');
On 2011/08/23 20:52:13, Rick Byers wrote:
> Looks like you're not using the 'dots' list anymore, remove it.

Done.

http://codereview.chromium.org/7685049/diff/2001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.js:340: if (hash &&
hash.indexOf('#app-id=') == 0 && app.id == hash.split('=')[1]) {
On 2011/08/23 20:52:13, Rick Byers wrote:
> the logic to parse the hash is duplicated in initialize.  I suggest adding a
> little helper function to reduce/eliminate the duplication (eg.
> getAndClearHashAppId - but it'll be a little more complicated than that if you
> want to avoid unconditionally clearing it here).

Done.

http://codereview.chromium.org/7685049/diff/2001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.js:350: page = new ntp4.AppsPage();
On 2011/08/23 20:52:13, Rick Byers wrote:
> is it not possible for page_index to be more than 1 greater than the number of
> current pages?  Eg., what if all apps are removed from some middle page?  I'd
> suggest instead looping here until you know you have enough pages...

Done.

http://codereview.chromium.org/7685049/diff/2001/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/app_launcher_handler.h (right):

http://codereview.chromium.org/7685049/diff/2001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/app_launcher_handler.h:194: // waiting to here about
success or failure from the extensions system.
On 2011/08/23 20:52:13, Rick Byers wrote:
> s/here/hear

Done.

[Rick Byers, 2011-08-25 21:17:14]

LGTM