ntp4: default to 18 apps per page for new installs

BUG=none
TEST=manual

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

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

chrome/browser/extensions/extension_prefs.cc

@@ -21,6 +21,10 @@ using base::Time;
 
 namespace {
 
+// The number of apps per page. This isn't a hard limit, but new apps installed
+// from the webstore will overflow onto a new page if this limit is reached.
+const int kNaturalAppPageSize = 18;
+
 // Additional preferences keys
 
 // Where an extension was installed from. (see Extension::Location)
@@ -980,10 +984,15 @@ 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(page_index)));
+
+  if (extension->is_app()) {
+    if (page_index == -1)
+      page_index = GetNaturalAppPageIndex();
+    extension_dict->Set(kPrefPageIndex,
+                        Value::CreateIntegerValue(page_index));
+    extension_dict->Set(kPrefAppLaunchIndex,
+        Value::CreateIntegerValue(GetNextAppLaunchIndex(page_index)));
+  }
   extension_pref_value_map_->RegisterExtension(
       id, install_time, initial_state == Extension::ENABLED);
   content_settings_store_->RegisterExtension(
@@ -1342,6 +1351,25 @@ int ExtensionPrefs::GetNextAppLaunchIndex(int on_page) {
   return max_value + 1;
 }
 
+int ExtensionPrefs::GetNaturalAppPageIndex() {
+  const DictionaryValue* extensions = prefs_->GetDictionary(kExtensionsPref);
+  if (!extensions)
+    return 0;
+
+  std::map<int, int> page_counts;
+  for (DictionaryValue::key_iterator extension_id = extensions->begin_keys();
+       extension_id != extensions->end_keys(); ++extension_id) {
+    int page_index = GetPageIndex(*extension_id);
+    if (page_index >= 0)
+      page_counts[page_index] = page_counts[page_index] + 1;
+  }
+  for (int i = 0; ; i++) {
+    std::map<int, int>::const_iterator it = page_counts.find(i);
+    if (it == page_counts.end() || it->second < kNaturalAppPageSize)
+      return i;
+  }
+}
+
 void ExtensionPrefs::SetAppLauncherOrder(
     const std::vector<std::string>& extension_ids) {
   for (size_t i = 0; i < extension_ids.size(); ++i)

chrome/browser/extensions/extension_prefs.h

@@ -101,6 +101,7 @@ class ExtensionPrefs : public ExtensionContentSettingsStore::Observer {
   void SetToolbarOrder(const std::vector<std::string>& extension_ids);
 
   // Called when an extension is installed, so that prefs get created.
+  // If |page_index| is -1, and the then a page will be found for the App.
   void OnExtensionInstalled(const Extension* extension,
                             Extension::State initial_state,
                             bool from_webstore,
@@ -277,6 +278,10 @@ class ExtensionPrefs : public ExtensionContentSettingsStore::Observer {
   // highest current application launch index found for the page |on_page|.
   int GetNextAppLaunchIndex(int on_page);
 
+  // Gets the page a new app should install to. Starts on page 0, and if there
+  // are N or more apps on it, tries to install on the next page.
+  int GetNaturalAppPageIndex();
+
   // Sets the order the apps should be displayed in the app launcher.
   void SetAppLauncherOrder(const std::vector<std::string>& extension_ids);
 

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, 0);
+        extension_.get(), Extension::DISABLED, false, -1);
   }
 
   virtual void Verify() {
@@ -597,10 +597,10 @@ class ExtensionPrefsAppLaunchIndex : public ExtensionPrefsTest {
     // No extensions yet.
     EXPECT_EQ(0, prefs()->GetNextAppLaunchIndex(0));
 
-    extension_ = prefs_.AddExtension("on_extension_installed");
+    extension_ = prefs_.AddApp("on_extension_installed");
     EXPECT_FALSE(prefs()->IsExtensionDisabled(extension_->id()));
     prefs()->OnExtensionInstalled(extension_.get(), Extension::ENABLED,
-                                  false, 0);
+                                  false, -1);
   }
 
   virtual void Verify() {
@@ -630,11 +630,17 @@ TEST_F(ExtensionPrefsAppLaunchIndex, ExtensionPrefsAppLaunchIndex) {}
 class ExtensionPrefsPageIndex : public ExtensionPrefsTest {
  public:
   virtual void Initialize() {
-    extension_ = prefs_.AddExtension("page_index");
+    extension_ = prefs_.AddApp("page_index");
     // Install to page 3 (index 2).
     prefs()->OnExtensionInstalled(extension_.get(), Extension::ENABLED,
                                   false, 2);
     EXPECT_EQ(2, prefs()->GetPageIndex(extension_->id()));
+
+    scoped_refptr<Extension> extension2 = prefs_.AddApp("page_index_2");
+    // Install without any page preference.
+    prefs()->OnExtensionInstalled(extension_.get(), Extension::ENABLED,
+                                  false, -1);
+    EXPECT_EQ(0, prefs()->GetPageIndex(extension_->id()));
   }
 
   virtual void Verify() {
@@ -652,13 +658,32 @@ class ExtensionPrefsPageIndex : public ExtensionPrefsTest {
 };
 TEST_F(ExtensionPrefsPageIndex, ExtensionPrefsPageIndex) {}
 
+class ExtensionPrefsAppLocation : public ExtensionPrefsTest {
+ public:
+  virtual void Initialize() {
+    extension_ = prefs_.AddExtension("not_an_app");
+    // Non-apps should not have any app launch index or page index.
+    prefs()->OnExtensionInstalled(extension_.get(), Extension::ENABLED,
+                                  false, 0);
+  }
+
+  virtual void Verify() {
+    EXPECT_EQ(-1, prefs()->GetAppLaunchIndex(extension_->id()));
+    EXPECT_EQ(-1, prefs()->GetPageIndex(extension_->id()));
+  }
+
+ private:
+  scoped_refptr<Extension> extension_;
+};
+TEST_F(ExtensionPrefsAppLocation, ExtensionPrefsAppLocation) {}
+
 class ExtensionPrefsAppDraggedByUser : public ExtensionPrefsTest {
  public:
   virtual void Initialize() {
     extension_ = prefs_.AddExtension("on_extension_installed");
     EXPECT_FALSE(prefs()->WasAppDraggedByUser(extension_->id()));
     prefs()->OnExtensionInstalled(extension_.get(), Extension::ENABLED,
-                                  false, 0);
+                                  false, -1);
   }
 
   virtual void Verify() {
@@ -808,7 +833,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, 0);
+        prefs()->OnExtensionInstalled(ext, Extension::ENABLED, false, -1);
         installed[i] = true;
         break;
       }

chrome/browser/extensions/extension_service.cc

@@ -199,7 +199,7 @@ void SimpleExtensionLoadPrompt::ShowPrompt() {
 void SimpleExtensionLoadPrompt::InstallUIProceed() {
   if (extension_service_.get())
     extension_service_->OnExtensionInstalled(
-        extension_, false, 0);  // Not from web store.
+        extension_, false, -1);  // Not from web store.
   delete this;
 }
 
@@ -2249,6 +2249,15 @@ void ExtensionService::AddExtension(const Extension* extension) {
     return;
   }
 
+  // Unfortunately, we used to set app launcher indices for non-apps. If this
+  // extension has an index (page or in-page), set it to -1.
+  if (!extension->is_app()) {
+    if (extension_prefs_->GetAppLaunchIndex(extension->id()) != -1)
+      extension_prefs_->SetAppLaunchIndex(extension->id(), -1);
+    if (extension_prefs_->GetPageIndex(extension->id()) != -1)
+      extension_prefs_->SetPageIndex(extension->id(), -1);
+  }
+
   extensions_.push_back(scoped_extension);
   SyncExtensionChangeIfNeeded(*extension);
   NotifyExtensionLoaded(extension);
@@ -2382,7 +2391,7 @@ void ExtensionService::OnLoadSingleExtension(const Extension* extension,
     prompt->ShowPrompt();
     return;  // continues in SimpleExtensionLoadPrompt::InstallUI*
   }
-  OnExtensionInstalled(extension, false, 0);  // Not from web store.
+  OnExtensionInstalled(extension, false, -1);  // Not from web store.
 }
 
 void ExtensionService::OnExtensionInstalled(

chrome/browser/extensions/test_extension_prefs.cc

@@ -101,6 +101,17 @@ scoped_refptr<Extension> TestExtensionPrefs::AddExtension(std::string name) {
   return AddExtensionWithManifest(dictionary, Extension::INTERNAL);
 }
 
+scoped_refptr<Extension> TestExtensionPrefs::AddApp(std::string name) {
+  DictionaryValue dictionary;
+  dictionary.SetString(extension_manifest_keys::kName, name);
+  dictionary.SetString(extension_manifest_keys::kVersion, "0.1");
+  dictionary.SetString(extension_manifest_keys::kApp, "true");
+  dictionary.SetString(extension_manifest_keys::kLaunchWebURL,
+                       "http://example.com");
+  return AddExtensionWithManifest(dictionary, Extension::INTERNAL);
+
+}
+
 scoped_refptr<Extension> TestExtensionPrefs::AddExtensionWithManifest(
     const DictionaryValue& manifest, Extension::Location location) {
   return AddExtensionWithManifestAndFlags(manifest, location,
@@ -117,7 +128,7 @@ scoped_refptr<Extension> TestExtensionPrefs::AddExtensionWithManifestAndFlags(
   std::string errors;
   scoped_refptr<Extension> extension = Extension::Create(
       path, location, manifest, extra_flags, &errors);
-  EXPECT_TRUE(extension);
+  EXPECT_TRUE(extension) << errors;
   if (!extension)
     return NULL;
 

chrome/browser/extensions/test_extension_prefs.h

@@ -40,6 +40,9 @@ class TestExtensionPrefs {
   // our ExtensionPrefs, and returns it.
   scoped_refptr<Extension> AddExtension(std::string name);
 
+  // As above, but the extension is an app.
+  scoped_refptr<Extension> AddApp(std::string name);
+
   // Similar to AddExtension, but takes a dictionary with manifest values.
   scoped_refptr<Extension> AddExtensionWithManifest(
       const base::DictionaryValue& manifest, Extension::Location location);

chrome/browser/ui/webui/ntp/app_launcher_handler.cc

@@ -167,10 +167,14 @@ void AppLauncherHandler::CreateAppInfo(const Extension* extension,
   value->SetInteger("app_launch_index", app_launch_index);
 
   int page_index = prefs->GetPageIndex(extension->id());
-  if (page_index >= 0) {
-    // Only provide a value if one is stored
-    value->SetInteger("page_index", page_index);
-  }
+  if (page_index < 0) {
+    // Make sure every app has a page index (some predate the page index).
+    // The webstore app should be on the first page.
+    page_index = extension->id() == extension_misc::kWebStoreAppId ?
+        0 : prefs->GetNaturalAppPageIndex();
+    prefs->SetPageIndex(extension->id(), page_index);
+  }
+  value->SetInteger("page_index", page_index);
 }
 
 // TODO(estade): remove this. We record app launches via js calls rather than