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.

Review URL: http://codereview.chromium.org/7677032

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@97733 0039d316-1c4b-4281-b951-d872f2087c98

14 files changed, 94 insertions, 52 deletions

diff --git a/chrome/browser/extensions/crx_installer.cc b/chrome/browser/extensions/crx_installer.cc
index e5a008f..126a7ce 100644
--- a/chrome/browser/extensions/crx_installer.cc
+++ b/chrome/browser/extensions/crx_installer.cc
@@ -120,6 +120,7 @@ CrxInstaller::CrxInstaller(base::WeakPtr<ExtensionService> frontend_weak,
       delete_source_(false),
       is_gallery_install_(false),
       create_app_shortcut_(false),
+      page_index_(0),
       frontend_weak_(frontend_weak),
       client_(client),
       apps_require_extension_mime_type_(false),
@@ -563,7 +564,8 @@ void CrxInstaller::ReportSuccessFromUIThread() {
 
   // Tell the frontend about the installation and hand off ownership of
   // extension_ to it.
-  frontend_weak_->OnExtensionInstalled(extension_, is_gallery_install());
+  frontend_weak_->OnExtensionInstalled(extension_, is_gallery_install(),
+                                       page_index_);
   extension_ = NULL;
 
   NotifyCrxInstallComplete();
diff --git a/chrome/browser/extensions/crx_installer.h b/chrome/browser/extensions/crx_installer.h
index 1db8104..52d6248 100644
--- a/chrome/browser/extensions/crx_installer.h
+++ b/chrome/browser/extensions/crx_installer.h
@@ -153,6 +153,10 @@ class CrxInstaller
     install_cause_ = install_cause;
   }
 
+  void set_page_index(int page_index) {
+    page_index_ = page_index;
+  }
+
  private:
   friend class ExtensionUpdaterTest;
 
@@ -239,6 +243,9 @@ class CrxInstaller
   // ExtensionService on success, or delete it on failure.
   scoped_refptr<const Extension> extension_;
 
+  // The index of the NTP apps page |extension_| will be shown on.
+  int page_index_;
+
   // A parsed copy of the unmodified original manifest, before any
   // transformations like localization have taken place.
   scoped_ptr<base::DictionaryValue> original_manifest_;
diff --git a/chrome/browser/extensions/extension_browsertest.cc b/chrome/browser/extensions/extension_browsertest.cc
index b6ca886..8fa5035 100644
--- a/chrome/browser/extensions/extension_browsertest.cc
+++ b/chrome/browser/extensions/extension_browsertest.cc
@@ -90,7 +90,7 @@ const Extension* ExtensionBrowserTest::LoadExtensionWithOptions(
   // The call to OnExtensionInstalled ensures the other extension prefs
   // are set up with the defaults.
   service->extension_prefs()->OnExtensionInstalled(
-      extension, Extension::ENABLED, false);
+      extension, Extension::ENABLED, false, 0);
 
   // Toggling incognito or file access will reload the extension, so wait for
   // the reload and grab the new extension instance. The default state is
diff --git a/chrome/browser/extensions/extension_prefs.cc b/chrome/browser/extensions/extension_prefs.cc
index 776273e..95c10b1 100644
--- a/chrome/browser/extensions/extension_prefs.cc
+++ b/chrome/browser/extensions/extension_prefs.cc
@@ -949,7 +949,8 @@ void ExtensionPrefs::SetToolbarOrder(
 void ExtensionPrefs::OnExtensionInstalled(
     const Extension* extension,
     Extension::State initial_state,
-    bool from_webstore) {
+    bool from_webstore,
+    int page_index) {
   const std::string& id = extension->id();
   CHECK(Extension::IdIsValid(id));
   ScopedExtensionPrefUpdate update(prefs_, id);
@@ -979,8 +980,10 @@ void ExtensionPrefs::OnExtensionInstalled(
     extension_dict->Set(kPrefManifest,
                         extension->manifest_value()->DeepCopy());
   }
+  extension_dict->Set(kPrefPageIndex,
+                      Value::CreateIntegerValue(page_index));
   extension_dict->Set(kPrefAppLaunchIndex,
-                      Value::CreateIntegerValue(GetNextAppLaunchIndex()));
+      Value::CreateIntegerValue(GetNextAppLaunchIndex(page_index)));
   extension_pref_value_map_->RegisterExtension(
       id, install_time, initial_state == Extension::ENABLED);
   content_settings_store_->RegisterExtension(
@@ -1324,7 +1327,7 @@ void ExtensionPrefs::SetAppLaunchIndex(const std::string& extension_id,
                       Value::CreateIntegerValue(index));
 }
 
-int ExtensionPrefs::GetNextAppLaunchIndex() {
+int ExtensionPrefs::GetNextAppLaunchIndex(int on_page) {
   const DictionaryValue* extensions = prefs_->GetDictionary(kExtensionsPref);
   if (!extensions)
     return 0;
@@ -1333,7 +1336,8 @@ int ExtensionPrefs::GetNextAppLaunchIndex() {
   for (DictionaryValue::key_iterator extension_id = extensions->begin_keys();
        extension_id != extensions->end_keys(); ++extension_id) {
     int value = GetAppLaunchIndex(*extension_id);
-    if (value > max_value)
+    int page = GetPageIndex(*extension_id);
+    if (page == on_page && value > max_value)
       max_value = value;
   }
   return max_value + 1;
diff --git a/chrome/browser/extensions/extension_prefs.h b/chrome/browser/extensions/extension_prefs.h
index 25c2c9a..cd64463 100644
--- a/chrome/browser/extensions/extension_prefs.h
+++ b/chrome/browser/extensions/extension_prefs.h
@@ -103,7 +103,8 @@ class ExtensionPrefs : public ExtensionContentSettingsStore::Observer {
   // Called when an extension is installed, so that prefs get created.
   void OnExtensionInstalled(const Extension* extension,
                             Extension::State initial_state,
-                            bool from_webstore);
+                            bool from_webstore,
+                            int page_index);
 
   // Called when an extension is uninstalled, so that prefs get cleaned up.
   void OnExtensionUninstalled(const std::string& extension_id,
@@ -273,8 +274,8 @@ class ExtensionPrefs : public ExtensionContentSettingsStore::Observer {
   void SetAppLaunchIndex(const std::string& extension_id, int index);
 
   // Gets the next available application launch index. This is 1 higher than the
-  // highest current application launch index found.
-  int GetNextAppLaunchIndex();
+  // highest current application launch index found for the page |on_page|.
+  int GetNextAppLaunchIndex(int on_page);
 
   // Sets the order the apps should be displayed in the app launcher.
   void SetAppLauncherOrder(const std::vector<std::string>& extension_ids);
diff --git a/chrome/browser/extensions/extension_prefs_unittest.cc b/chrome/browser/extensions/extension_prefs_unittest.cc
index 10a4ae8..433c9a1 100644
--- a/chrome/browser/extensions/extension_prefs_unittest.cc
+++ b/chrome/browser/extensions/extension_prefs_unittest.cc
@@ -578,7 +578,7 @@ class ExtensionPrefsOnExtensionInstalled : public ExtensionPrefsTest {
     extension_ = prefs_.AddExtension("on_extension_installed");
     EXPECT_FALSE(prefs()->IsExtensionDisabled(extension_->id()));
     prefs()->OnExtensionInstalled(
-        extension_.get(), Extension::DISABLED, false);
+        extension_.get(), Extension::DISABLED, false, 0);
   }
 
   virtual void Verify() {
@@ -595,26 +595,31 @@ class ExtensionPrefsAppLaunchIndex : public ExtensionPrefsTest {
  public:
   virtual void Initialize() {
     // No extensions yet.
-    EXPECT_EQ(0, prefs()->GetNextAppLaunchIndex());
+    EXPECT_EQ(0, prefs()->GetNextAppLaunchIndex(0));
 
     extension_ = prefs_.AddExtension("on_extension_installed");
     EXPECT_FALSE(prefs()->IsExtensionDisabled(extension_->id()));
-    prefs()->OnExtensionInstalled(extension_.get(), Extension::ENABLED, false);
+    prefs()->OnExtensionInstalled(extension_.get(), Extension::ENABLED,
+                                  false, 0);
   }
 
   virtual void Verify() {
     int launch_index = prefs()->GetAppLaunchIndex(extension_->id());
     // Extension should have been assigned a launch index > 0.
     EXPECT_GT(launch_index, 0);
-    EXPECT_EQ(launch_index + 1, prefs()->GetNextAppLaunchIndex());
+    EXPECT_EQ(launch_index + 1, prefs()->GetNextAppLaunchIndex(0));
     // Set a new launch index of one higher and verify.
     prefs()->SetAppLaunchIndex(extension_->id(),
-        prefs()->GetNextAppLaunchIndex());
+                               prefs()->GetNextAppLaunchIndex(0));
     int new_launch_index = prefs()->GetAppLaunchIndex(extension_->id());
     EXPECT_EQ(launch_index + 1, new_launch_index);
 
     // This extension doesn't exist, so it should return -1.
     EXPECT_EQ(-1, prefs()->GetAppLaunchIndex("foo"));
+
+    // The second page doesn't have any apps so its next launch index should
+    // still be 0.
+    EXPECT_EQ(prefs()->GetNextAppLaunchIndex(1), 0);
   }
 
  private:
@@ -625,27 +630,25 @@ TEST_F(ExtensionPrefsAppLaunchIndex, ExtensionPrefsAppLaunchIndex) {}
 class ExtensionPrefsPageIndex : public ExtensionPrefsTest {
  public:
   virtual void Initialize() {
-    extension_id_ = prefs_.AddExtensionAndReturnId("page_index");
-
-    int page_index = prefs()->GetPageIndex(extension_id_);
-    // Extension should not have been assigned a page
-    EXPECT_EQ(page_index, -1);
-
-    // Set the page index
-    prefs()->SetPageIndex(extension_id_, 2);
+    extension_ = prefs_.AddExtension("page_index");
+    // Install to page 3 (index 2).
+    prefs()->OnExtensionInstalled(extension_.get(), Extension::ENABLED,
+                                  false, 2);
+    EXPECT_EQ(2, prefs()->GetPageIndex(extension_->id()));
   }
 
   virtual void Verify() {
+    // Set the page index.
+    prefs()->SetPageIndex(extension_->id(), 1);
     // Verify the page index.
-    int page_index = prefs()->GetPageIndex(extension_id_);
-    EXPECT_EQ(page_index, 2);
+    EXPECT_EQ(1, prefs()->GetPageIndex(extension_->id()));
 
     // This extension doesn't exist, so it should return -1.
     EXPECT_EQ(-1, prefs()->GetPageIndex("foo"));
   }
 
  private:
-  std::string extension_id_;
+  scoped_refptr<Extension> extension_;
 };
 TEST_F(ExtensionPrefsPageIndex, ExtensionPrefsPageIndex) {}
 
@@ -654,7 +657,8 @@ class ExtensionPrefsAppDraggedByUser : public ExtensionPrefsTest {
   virtual void Initialize() {
     extension_ = prefs_.AddExtension("on_extension_installed");
     EXPECT_FALSE(prefs()->WasAppDraggedByUser(extension_->id()));
-    prefs()->OnExtensionInstalled(extension_.get(), Extension::ENABLED, false);
+    prefs()->OnExtensionInstalled(extension_.get(), Extension::ENABLED,
+                                  false, 0);
   }
 
   virtual void Verify() {
@@ -804,7 +808,7 @@ class ExtensionPrefsPreferencesBase : public ExtensionPrefsTest {
     Extension* extensions[] = {ext1_, ext2_, ext3_};
     for (int i = 0; i < 3; ++i) {
       if (ext == extensions[i] && !installed[i]) {
-        prefs()->OnExtensionInstalled(ext, Extension::ENABLED, false);
+        prefs()->OnExtensionInstalled(ext, Extension::ENABLED, false, 0);
         installed[i] = true;
         break;
       }
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index c2f6431..e271cab 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -190,7 +190,7 @@ void SimpleExtensionLoadPrompt::ShowPrompt() {
 void SimpleExtensionLoadPrompt::InstallUIProceed() {
   if (extension_service_.get())
     extension_service_->OnExtensionInstalled(
-        extension_, false);  // Not from web store.
+        extension_, false, 0);  // Not from web store.
   delete this;
 }
 
@@ -2161,11 +2161,11 @@ void ExtensionService::OnLoadSingleExtension(const Extension* extension,
     prompt->ShowPrompt();
     return;  // continues in SimpleExtensionLoadPrompt::InstallUI*
   }
-  OnExtensionInstalled(extension, false);  // Not from web store.
+  OnExtensionInstalled(extension, false, 0);  // Not from web store.
 }
 
 void ExtensionService::OnExtensionInstalled(
-    const Extension* extension, bool from_webstore) {
+    const Extension* extension, bool from_webstore, int page_index) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   // Ensure extension is deleted unless we transfer ownership.
@@ -2222,7 +2222,8 @@ void ExtensionService::OnExtensionInstalled(
   extension_prefs_->OnExtensionInstalled(
       extension,
       initial_enable ? Extension::ENABLED : Extension::DISABLED,
-      from_webstore);
+      from_webstore,
+      page_index);
 
   // Unpacked extensions default to allowing file access, but if that has been
   // overridden, don't reset the value.
diff --git a/chrome/browser/extensions/extension_service.h b/chrome/browser/extensions/extension_service.h
index f3ce586..03b3a7a 100644
--- a/chrome/browser/extensions/extension_service.h
+++ b/chrome/browser/extensions/extension_service.h
@@ -411,7 +411,7 @@ class ExtensionService
 
   // Called by the backend when an extension has been installed.
   void OnExtensionInstalled(
-      const Extension* extension, bool from_webstore);
+      const Extension* extension, bool from_webstore, int page_index);
 
   // Initializes the |extension|'s active permission set and disables the
   // extension if the privilege level has increased (e.g., due to an upgrade).
diff --git a/chrome/browser/extensions/test_extension_prefs.cc b/chrome/browser/extensions/test_extension_prefs.cc
index aee917f..60b06b4 100644
--- a/chrome/browser/extensions/test_extension_prefs.cc
+++ b/chrome/browser/extensions/test_extension_prefs.cc
@@ -123,7 +123,7 @@ scoped_refptr<Extension> TestExtensionPrefs::AddExtensionWithManifestAndFlags(
 
   EXPECT_TRUE(Extension::IdIsValid(extension->id()));
   prefs_->OnExtensionInstalled(extension, Extension::ENABLED,
-                               extra_flags & Extension::FROM_WEBSTORE);
+                               extra_flags & Extension::FROM_WEBSTORE, 0);
   return extension;
 }
 
diff --git a/chrome/browser/resources/ntp4/apps_page.js b/chrome/browser/resources/ntp4/apps_page.js
index b6ef1e6..0b5a567 100644
--- a/chrome/browser/resources/ntp4/apps_page.js
+++ b/chrome/browser/resources/ntp4/apps_page.js
@@ -415,6 +415,17 @@ cr.define('ntp4', function() {
       this.appendTile(new App(appData), animate);
     },
 
+    /** @inheritdoc */
+    doDragOver: function(e) {
+      var tile = ntp4.getCurrentlyDraggingTile();
+      if (!tile.querySelector('.app')) {
+        e.preventDefault();
+        e.dataTransfer.dropEffect = 'copy';
+      } else {
+        TilePage.prototype.doDragOver.call(this, e);
+      }
+    },
+
     /** @inheritDoc */
     shouldAcceptDrag: function(e) {
       return ntp4.getCurrentlyDraggingTile() ||
@@ -431,7 +442,7 @@ cr.define('ntp4', function() {
               currentlyDraggingTile,
               this.tileElements_[index]);
           this.tileMoved(currentlyDraggingTile);
-        } else if (tileContents.classList.contains('most-visited')) {
+        } else if (currentlyDraggingTile.querySelector('.most-visited')) {
           this.generateAppForLink(tileContents.data);
         }
       } else {
@@ -485,7 +496,8 @@ cr.define('ntp4', function() {
     generateAppForLink: function(data) {
       assert(data.url != undefined);
       assert(data.title != undefined);
-      chrome.send('generateAppForLink', [data.url, data.title]);
+      var pageIndex = ntp4.getAppsPageIndex(this);
+      chrome.send('generateAppForLink', [data.url, data.title, pageIndex]);
     },
 
     /** @inheritDoc */
diff --git a/chrome/browser/resources/ntp4/new_tab.js b/chrome/browser/resources/ntp4/new_tab.js
index c7414bf4..09f2438 100644
--- a/chrome/browser/resources/ntp4/new_tab.js
+++ b/chrome/browser/resources/ntp4/new_tab.js
@@ -335,8 +335,6 @@ cr.define('ntp4', function() {
    */
   function appAdded(app) {
     var pageIndex = app.page_index || 0;
-    assert(pageIndex == 0, 'pageIndex != 0 not implemented');
-
     var page = appsPages[pageIndex];
     cardSlider.selectCardByValue(page);
     page.appendApp(app, true);
diff --git a/chrome/browser/ui/webui/ntp/app_launcher_handler.cc b/chrome/browser/ui/webui/ntp/app_launcher_handler.cc
index 8bc9b69..53c68cf 100644
--- a/chrome/browser/ui/webui/ntp/app_launcher_handler.cc
+++ b/chrome/browser/ui/webui/ntp/app_launcher_handler.cc
@@ -156,7 +156,7 @@ void AppLauncherHandler::CreateAppInfo(const Extension* extension,
   int app_launch_index = prefs->GetAppLaunchIndex(extension->id());
   if (app_launch_index == -1) {
     // Make sure every app has a launch index (some predate the launch index).
-    app_launch_index = prefs->GetNextAppLaunchIndex();
+    app_launch_index = prefs->GetNextAppLaunchIndex(0);
     prefs->SetAppLaunchIndex(extension->id(), app_launch_index);
   }
   value->SetInteger("app_launch_index", app_launch_index);
@@ -690,32 +690,32 @@ void AppLauncherHandler::HandleSaveAppPageName(const ListValue* args) {
 void AppLauncherHandler::HandleGenerateAppForLink(const ListValue* args) {
   std::string url;
   CHECK(args->GetString(0, &url));
+  GURL launch_url(url);
 
   string16 title;
   CHECK(args->GetString(1, &title));
 
-  GURL launch_url(url);
-
-  scoped_ptr<WebApplicationInfo> web_app(new WebApplicationInfo);
-  web_app->is_bookmark_app = true;
-  web_app->title = title;
-  web_app->app_url = launch_url;
+  double page_index;
+  CHECK(args->GetDouble(2, &page_index));
 
   Profile* profile = Profile::FromWebUI(web_ui_);
   FaviconService* favicon_service =
       profile->GetFaviconService(Profile::EXPLICIT_ACCESS);
   if (!favicon_service) {
     LOG(ERROR) << "No favicon service";
-    scoped_refptr<CrxInstaller> installer(
-        extension_service_->MakeCrxInstaller(NULL));
-    installer->InstallWebApp(*web_app);
     return;
   }
 
+  scoped_ptr<AppInstallInfo> install_info(new AppInstallInfo());
+  install_info->is_bookmark_app = true;
+  install_info->title = title;
+  install_info->app_url = launch_url;
+  install_info->page_index = static_cast<int>(page_index);
+
   FaviconService::Handle h = favicon_service->GetFaviconForURL(
       launch_url, history::FAVICON, &favicon_consumer_,
       NewCallback(this, &AppLauncherHandler::OnFaviconForApp));
-  favicon_consumer_.SetClientData(favicon_service, h, web_app.release());
+  favicon_consumer_.SetClientData(favicon_service, h, install_info.release());
 }
 
 void AppLauncherHandler::HandleRecordAppLaunchByURL(
@@ -734,8 +734,13 @@ void AppLauncherHandler::HandleRecordAppLaunchByURL(
 
 void AppLauncherHandler::OnFaviconForApp(FaviconService::Handle handle,
                                          history::FaviconData data) {
-  scoped_ptr<WebApplicationInfo> web_app(
+  scoped_ptr<AppInstallInfo> install_info(
       favicon_consumer_.GetClientDataForCurrentRequest());
+  scoped_ptr<WebApplicationInfo> web_app(new WebApplicationInfo());
+  web_app->is_bookmark_app = install_info->is_bookmark_app;
+  web_app->title = install_info->title;
+  web_app->app_url = install_info->app_url;
+
   WebApplicationInfo::IconInfo icon;
   web_app->icons.push_back(icon);
   if (data.is_valid() && gfx::PNGCodec::Decode(data.image_data->front(),
@@ -750,6 +755,7 @@ void AppLauncherHandler::OnFaviconForApp(FaviconService::Handle handle,
 
   scoped_refptr<CrxInstaller> installer(
       extension_service_->MakeCrxInstaller(NULL));
+  installer->set_page_index(install_info->page_index);
   installer->InstallWebApp(*web_app);
 }
 
@@ -760,7 +766,7 @@ void AppLauncherHandler::RegisterUserPrefs(PrefService* pref_service) {
                                  PrefService::UNSYNCABLE_PREF);
 }
 
-// static
+// statiic
 void AppLauncherHandler::RecordWebStoreLaunch(bool promo_active) {
   UMA_HISTOGRAM_ENUMERATION(extension_misc::kAppLaunchHistogram,
                             extension_misc::APP_LAUNCH_NTP_WEBSTORE,
diff --git a/chrome/browser/ui/webui/ntp/app_launcher_handler.h b/chrome/browser/ui/webui/ntp/app_launcher_handler.h
index 0b2c395..1559d24 100644
--- a/chrome/browser/ui/webui/ntp/app_launcher_handler.h
+++ b/chrome/browser/ui/webui/ntp/app_launcher_handler.h
@@ -115,6 +115,13 @@ class AppLauncherHandler : public WebUIMessageHandler,
   static void RegisterUserPrefs(PrefService* pref_service);
 
  private:
+  struct AppInstallInfo {
+    bool is_bookmark_app;
+    string16 title;
+    GURL app_url;
+    int page_index;
+  };
+
   // Records a web store launch in the appropriate histograms. |promo_active|
   // specifies if the web store promotion was active.
   static void RecordWebStoreLaunch(bool promo_active);
@@ -184,7 +191,7 @@ class AppLauncherHandler : public WebUIMessageHandler,
   bool ignore_changes_;
 
   // Hold state for favicon requests.
-  CancelableRequestConsumerTSimple<WebApplicationInfo*> favicon_consumer_;
+  CancelableRequestConsumerTSimple<AppInstallInfo*> favicon_consumer_;
 
   DISALLOW_COPY_AND_ASSIGN(AppLauncherHandler);
 };
diff --git a/chrome/test/live_sync/sync_extension_helper.cc b/chrome/test/live_sync/sync_extension_helper.cc
index c8ea765..e8dbae8 100644
--- a/chrome/test/live_sync/sync_extension_helper.cc
+++ b/chrome/test/live_sync/sync_extension_helper.cc
@@ -64,7 +64,7 @@ void SyncExtensionHelper::InstallExtension(
   ASSERT_TRUE(extension.get()) << "Could not get extension " << name
                                << " (profile = " << profile << ")";
   profile->GetExtensionService()->OnExtensionInstalled(
-      extension, extension->UpdatesFromGallery());
+      extension, extension->UpdatesFromGallery(), 0);
 }
 
 void SyncExtensionHelper::UninstallExtension(