ntp4: make app-install-via-drag less janky

ntp4: make app-install-via-drag less janky

don't reposition the tiles since we always add the app at the end anyways.

Also, respect which page it's dropped on (instead of always adding to the first page).

BUG=93159
TEST=drag a most visited tile onto an apps page.

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

[Evan Stade, 2011-08-19 01:31:55]

mpcomplete for extensions review,
arv for the rest

http://codereview.chromium.org/7677032/diff/3010/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/7677032/diff/3010/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.h:107: int page_index);
adding page_index here is admittedly a little sketch, but it makes about as much
sense as from_webstore. Perhaps we need a way to pass in a list of initial
extension prefs. WDYT?

(I haven't updated the tests yet because I expect there to be feedback on this
point)

[Matt Perry, 2011-08-19 18:23:09]

LGTM with comment fixed

http://codereview.chromium.org/7677032/diff/3010/chrome/browser/extensions/cr...
File chrome/browser/extensions/crx_installer.h (right):

http://codereview.chromium.org/7677032/diff/3010/chrome/browser/extensions/cr...
chrome/browser/extensions/crx_installer.h:246: // The index of the page
|extension_| will be installed to.
I don't understand this comment. Could you expand on it a bit? Is it
app-specific? ntp-specific?

http://codereview.chromium.org/7677032/diff/3010/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs.h (right):

http://codereview.chromium.org/7677032/diff/3010/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.h:107: int page_index);
On 2011/08/19 01:31:56, Evan Stade wrote:
> adding page_index here is admittedly a little sketch, but it makes about as
much
> sense as from_webstore. Perhaps we need a way to pass in a list of initial
> extension prefs. WDYT?
> 
> (I haven't updated the tests yet because I expect there to be feedback on this
> point)

I'm not a fan of passing in initial extension prefs, because that would require
exposing the pref names to consumers, which I'd rather not do.

The current way does seem a little ugly, but I think it's OK.

[Evan Stade, 2011-08-19 19:19:28]

fixed tests as well

http://codereview.chromium.org/7677032/diff/3010/chrome/browser/extensions/cr...
File chrome/browser/extensions/crx_installer.h (right):

http://codereview.chromium.org/7677032/diff/3010/chrome/browser/extensions/cr...
chrome/browser/extensions/crx_installer.h:246: // The index of the page
|extension_| will be installed to.
On 2011/08/19 18:23:09, Matt Perry wrote:
> I don't understand this comment. Could you expand on it a bit? Is it
> app-specific? ntp-specific?

The NTP page. You can have multiple pages of apps with the new NTP. I will try
to clarify.

[arv (Not doing code reviews), 2011-08-22 18:49:02]

LGTM

[Aaron Boodman, 2011-09-09 21:00:16]

http://codereview.chromium.org/7677032/diff/9001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/7677032/diff/9001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:193: extension_, false, 0);  //
Not from web store.
Since there are now two magic params that need explanation, you should format
it:

extension_,
false,  // not from web store
0);  // <I'm not sure what this means, but explain it>