Do not assign launch ordinals to ephemeral apps

Ephemeral apps are not shown in the app launcher or NTP, but they are
assigned ordinals, which results in unfilled NTP pages. This patch
does not give ephemeral apps launch ordinals until they are promoted
to fully installed apps.

AppSorting::MarkExtensionAsHidden() was refactored to
AppSorting::SetExtensionVisible() to allow the visibility of the
extension in the NTP to change.

BUG=394192
TEST=browser_tests

Review URL: https://codereview.chromium.org/397903002

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

apps/shell/browser/shell_app_sorting.cc

@@ -100,8 +100,8 @@ syncer::StringOrdinal ShellAppSorting::PageIntegerAsStringOrdinal(
   return syncer::StringOrdinal(kFirstPage);
 }
 
-void ShellAppSorting::MarkExtensionAsHidden(
-    const std::string& extension_id) {
+void ShellAppSorting::SetExtensionVisible(const std::string& extension_id,
+                                          bool visible) {
 }
 
 }  // namespace apps

apps/shell/browser/shell_app_sorting.h

@@ -53,7 +53,8 @@ class ShellAppSorting : public extensions::AppSorting {
       const syncer::StringOrdinal& page_ordinal) const OVERRIDE;
   virtual syncer::StringOrdinal PageIntegerAsStringOrdinal(
       size_t page_index) OVERRIDE;
-  virtual void MarkExtensionAsHidden(const std::string& extension_id) OVERRIDE;
+  virtual void SetExtensionVisible(const std::string& extension_id,
+                                   bool visible) OVERRIDE;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ShellAppSorting);

chrome/browser/apps/ephemeral_app_browsertest.cc

@@ -277,6 +277,22 @@ class EphemeralAppBrowserTest : public EphemeralAppTestBase {
         process_manager()->GetBackgroundHostForExtension(app_id));
   }
 
+  // Verify properties of ephemeral apps.
+  void VerifyEphemeralApp(const std::string& app_id) {
+    EXPECT_TRUE(extensions::util::IsEphemeralApp(app_id, profile()));
+
+    // Ephemeral apps should not be synced.
+    scoped_ptr<AppSyncData> sync_change = GetLastSyncChangeForApp(app_id);
+    EXPECT_FALSE(sync_change.get());
+
+    // Ephemeral apps should not be assigned ordinals.
+    extensions::AppSorting* app_sorting =
+        ExtensionPrefs::Get(profile())->app_sorting();
+    EXPECT_FALSE(app_sorting->GetAppLaunchOrdinal(app_id).IsValid());
+    EXPECT_FALSE(app_sorting->GetPageOrdinal(app_id).IsValid());
+  }
+
+  // Dispatch a fake alarm event to the app.
   void DispatchAlarmEvent(EventRouter* event_router,
                           const std::string& app_id) {
     alarms::Alarm dummy_alarm;
@@ -324,17 +340,17 @@ class EphemeralAppBrowserTest : public EphemeralAppTestBase {
             new syncer::SyncErrorFactoryMock()));
   }
 
-  scoped_ptr<AppSyncData> GetFirstSyncChangeForApp(const std::string& id) {
+  scoped_ptr<AppSyncData> GetLastSyncChangeForApp(const std::string& id) {
     scoped_ptr<AppSyncData> sync_data;
     for (syncer::SyncChangeList::iterator it =
              mock_sync_processor_.changes().begin();
          it != mock_sync_processor_.changes().end(); ++it) {
-      sync_data.reset(new AppSyncData(*it));
-      if (sync_data->id() == id)
-        return sync_data.Pass();
+      scoped_ptr<AppSyncData> data(new AppSyncData(*it));
+      if (data->id() == id)
+        sync_data.reset(data.release());
     }
 
-    return scoped_ptr<AppSyncData>();
+    return sync_data.Pass();
   }
 
   void VerifySyncChange(const AppSyncData* sync_change, bool expect_enabled) {
@@ -402,13 +418,18 @@ IN_PROC_BROWSER_TEST_F(EphemeralAppBrowserTest,
 // Verify that an updated ephemeral app will still have its ephemeral flag
 // enabled.
 IN_PROC_BROWSER_TEST_F(EphemeralAppBrowserTest, UpdateEphemeralApp) {
-  const Extension* app_v1 = InstallEphemeralApp(kMessagingReceiverApp);
+  InitSyncService();
+
+  const Extension* app_v1 = InstallAndLaunchEphemeralApp(kMessagingReceiverApp);
   ASSERT_TRUE(app_v1);
   std::string app_id = app_v1->id();
   base::Version app_original_version = *app_v1->version();
-  app_v1 = NULL; // The extension object will be destroyed during update.
+
+  VerifyEphemeralApp(app_id);
+  CloseApp(app_id);
 
   // Update to version 2 of the app.
+  app_v1 = NULL;  // The extension object will be destroyed during update.
   InstallObserver installed_observer(profile());
   const Extension* app_v2 = UpdateEphemeralApp(
       app_id, GetTestPath(kMessagingReceiverAppV2),
@@ -423,8 +444,8 @@ IN_PROC_BROWSER_TEST_F(EphemeralAppBrowserTest, UpdateEphemeralApp) {
 
   // The ephemeral flag should still be enabled.
   ASSERT_TRUE(app_v2);
-  EXPECT_TRUE(app_v2->version()->CompareTo(app_original_version) > 0);
-  EXPECT_TRUE(extensions::util::IsEphemeralApp(app_v2->id(), profile()));
+  EXPECT_GT(app_v2->version()->CompareTo(app_original_version), 0);
+  VerifyEphemeralApp(app_id);
 }
 
 // Verify that if notifications have been disabled for an ephemeral app, it will
@@ -533,7 +554,7 @@ IN_PROC_BROWSER_TEST_F(EphemeralAppBrowserTest, PromoteEphemeralApp) {
   ASSERT_TRUE(app);
 
   // Ephemeral apps should not be synced.
-  scoped_ptr<AppSyncData> sync_change = GetFirstSyncChangeForApp(app->id());
+  scoped_ptr<AppSyncData> sync_change = GetLastSyncChangeForApp(app->id());
   EXPECT_FALSE(sync_change.get());
 
   // Promote the app to a regular installed app.
@@ -548,7 +569,7 @@ IN_PROC_BROWSER_TEST_F(EphemeralAppBrowserTest, PromoteEphemeralApp) {
   EXPECT_TRUE(params.from_ephemeral);
 
   // The installation should now be synced.
-  sync_change = GetFirstSyncChangeForApp(app->id());
+  sync_change = GetLastSyncChangeForApp(app->id());
   VerifySyncChange(sync_change.get(), true);
 }
 
@@ -574,7 +595,7 @@ IN_PROC_BROWSER_TEST_F(EphemeralAppBrowserTest, PromoteEphemeralAppAndEnable) {
   VerifyPromotedApp(app->id(), ExtensionRegistry::ENABLED);
   EXPECT_FALSE(prefs->DidExtensionEscalatePermissions(app->id()));
 
-  scoped_ptr<AppSyncData> sync_change = GetFirstSyncChangeForApp(app->id());
+  scoped_ptr<AppSyncData> sync_change = GetLastSyncChangeForApp(app->id());
   VerifySyncChange(sync_change.get(), true);
 }
 
@@ -599,7 +620,7 @@ IN_PROC_BROWSER_TEST_F(EphemeralAppBrowserTest,
   PromoteEphemeralApp(app);
   VerifyPromotedApp(app->id(), ExtensionRegistry::DISABLED);
 
-  scoped_ptr<AppSyncData> sync_change = GetFirstSyncChangeForApp(app->id());
+  scoped_ptr<AppSyncData> sync_change = GetLastSyncChangeForApp(app->id());
   VerifySyncChange(sync_change.get(), false);
 }
 

chrome/browser/extensions/chrome_app_sorting.cc

@@ -435,8 +435,12 @@ syncer::StringOrdinal ChromeAppSorting::PageIntegerAsStringOrdinal(
   return ntp_ordinal_map_.rbegin()->first;
 }
 
-void ChromeAppSorting::MarkExtensionAsHidden(const std::string& extension_id) {
-  ntp_hidden_extensions_.insert(extension_id);
+void ChromeAppSorting::SetExtensionVisible(const std::string& extension_id,
+                                           bool visible) {
+  if (visible)
+    ntp_hidden_extensions_.erase(extension_id);
+  else
+    ntp_hidden_extensions_.insert(extension_id);
 }
 
 syncer::StringOrdinal ChromeAppSorting::GetMinOrMaxAppLaunchOrdinalsOnPage(

chrome/browser/extensions/chrome_app_sorting.h

@@ -62,7 +62,8 @@ class ChromeAppSorting : public AppSorting {
       const syncer::StringOrdinal& page_ordinal) const OVERRIDE;
   virtual syncer::StringOrdinal PageIntegerAsStringOrdinal(
       size_t page_index) OVERRIDE;
-  virtual void MarkExtensionAsHidden(const std::string& extension_id) OVERRIDE;
+  virtual void SetExtensionVisible(const std::string& extension_id,
+                                   bool visible) OVERRIDE;
 
  private:
   // The StringOrdinal is the app launch ordinal and the string is the extension
@@ -81,6 +82,7 @@ class ChromeAppSorting : public AppSorting {
   friend class ChromeAppSortingGetMinOrMaxAppLaunchOrdinalsOnPage;
   friend class ChromeAppSortingInitializeWithNoApps;
   friend class ChromeAppSortingPageOrdinalMapping;
+  friend class ChromeAppSortingSetExtensionVisible;
 
   // An enum used by GetMinOrMaxAppLaunchOrdinalsOnPage to specify which
   // value should be returned.

chrome/browser/extensions/chrome_app_sorting_unittest.cc

@@ -959,4 +959,42 @@ class ChromeAppSortingDefaultOrdinalNoCollision
 TEST_F(ChromeAppSortingDefaultOrdinalNoCollision,
        ChromeAppSortingDefaultOrdinalNoCollision) {}
 
+// Tests that SetExtensionVisible() correctly hides and unhides extensions.
+class ChromeAppSortingSetExtensionVisible : public ChromeAppSortingTest {
+ public:
+  ChromeAppSortingSetExtensionVisible() {}
+  virtual ~ChromeAppSortingSetExtensionVisible() {}
+
+  virtual void Initialize() OVERRIDE {
+    first_app_ = prefs_.AddApp("first_app");
+    second_app_ = prefs_.AddApp("second_app");
+  }
+
+  virtual void Verify() OVERRIDE {
+    ChromeAppSorting* sorting = app_sorting();
+    syncer::StringOrdinal page1 = sorting->GetPageOrdinal(first_app_->id());
+    syncer::StringOrdinal page2 = sorting->GetPageOrdinal(second_app_->id());
+    EXPECT_TRUE(sorting->GetAppLaunchOrdinal(first_app_->id()).IsValid());
+    EXPECT_TRUE(sorting->GetAppLaunchOrdinal(second_app_->id()).IsValid());
+    EXPECT_TRUE(page1.IsValid());
+    EXPECT_TRUE(page2.IsValid());
+    EXPECT_TRUE(page1.Equals(page2));
+
+    sorting->SetExtensionVisible(first_app_->id(), false);
+    EXPECT_EQ(
+        1U, sorting->CountItemsVisibleOnNtp(sorting->ntp_ordinal_map_[page1]));
+
+    sorting->SetExtensionVisible(first_app_->id(), true);
+    EXPECT_EQ(
+        2U, sorting->CountItemsVisibleOnNtp(sorting->ntp_ordinal_map_[page1]));
+  }
+
+ private:
+  scoped_refptr<Extension> first_app_;
+  scoped_refptr<Extension> second_app_;
+};
+TEST_F(ChromeAppSortingSetExtensionVisible,
+       ChromeAppSortingSetExtensionVisible) {
+}
+
 }  // namespace extensions

chrome/browser/extensions/extension_service.cc

@@ -1410,11 +1410,14 @@ void ExtensionService::AddExtension(const Extension* extension) {
     // All apps that are displayed in the launcher are ordered by their ordinals
     // so we must ensure they have valid ordinals.
     if (extension->RequiresSortOrdinal()) {
-      if (!extension->ShouldDisplayInNewTabPage()) {
-        extension_prefs_->app_sorting()->MarkExtensionAsHidden(extension->id());
+      extension_prefs_->app_sorting()->SetExtensionVisible(
+          extension->id(),
+          extension->ShouldDisplayInNewTabPage() &&
+              !extension_prefs_->IsEphemeralApp(extension->id()));
+      if (!extension_prefs_->IsEphemeralApp(extension->id())) {
+        extension_prefs_->app_sorting()->EnsureValidOrdinals(
+            extension->id(), syncer::StringOrdinal());
       }
-      extension_prefs_->app_sorting()->EnsureValidOrdinals(
-          extension->id(), syncer::StringOrdinal());
     }
 
     registry_->AddEnabled(extension);
@@ -1817,15 +1820,21 @@ void ExtensionService::PromoteEphemeralApp(
   DCHECK(GetInstalledExtension(extension->id()) &&
          extension_prefs_->IsEphemeralApp(extension->id()));
 
-  if (!is_from_sync) {
-    if (extension->RequiresSortOrdinal()) {
+  if (extension->RequiresSortOrdinal()) {
+    extension_prefs_->app_sorting()->SetExtensionVisible(
+        extension->id(), extension->ShouldDisplayInNewTabPage());
+
+    if (!is_from_sync) {
       // Reset the sort ordinals of the app to ensure it is added to the default
       // position, like newly installed apps would.
       extension_prefs_->app_sorting()->ClearOrdinals(extension->id());
-      extension_prefs_->app_sorting()->EnsureValidOrdinals(
-          extension->id(), syncer::StringOrdinal());
     }
 
+    extension_prefs_->app_sorting()->EnsureValidOrdinals(
+        extension->id(), syncer::StringOrdinal());
+  }
+
+  if (!is_from_sync) {
     // Cached ephemeral apps may be updated and disabled due to permissions
     // increase. The app can be enabled as the install was user-acknowledged.
     if (extension_prefs_->DidExtensionEscalatePermissions(extension->id()))

extensions/browser/app_sorting.h

@@ -109,8 +109,10 @@ class AppSorting {
   virtual syncer::StringOrdinal PageIntegerAsStringOrdinal(
       size_t page_index) = 0;
 
-  // Hidden extensions don't appear in the new tab page.
-  virtual void MarkExtensionAsHidden(const std::string& extension_id) = 0;
+  // Hides an extension from the new tab page, or makes a previously hidden
+  // extension visible.
+  virtual void SetExtensionVisible(const std::string& extension_id,
+                                   bool visible) = 0;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(AppSorting);

extensions/browser/extension_prefs.cc

@@ -1238,9 +1238,14 @@ void ExtensionPrefs::OnExtensionInstalled(
                              install_flags,
                              install_parameter,
                              extension_dict);
-  FinishExtensionInfoPrefs(extension->id(), install_time,
-                           extension->RequiresSortOrdinal(),
-                           page_ordinal, extension_dict);
+
+  bool requires_sort_ordinal = extension->RequiresSortOrdinal() &&
+                               (install_flags & kInstallFlagIsEphemeral) == 0;
+  FinishExtensionInfoPrefs(extension->id(),
+                           install_time,
+                           requires_sort_ordinal,
+                           page_ordinal,
+                           extension_dict);
 }
 
 void ExtensionPrefs::OnExtensionUninstalled(const std::string& extension_id,
@@ -1453,7 +1458,8 @@ void ExtensionPrefs::SetDelayedInstallInfo(
   // Add transient data that is needed by FinishDelayedInstallInfo(), but
   // should not be in the final extension prefs. All entries here should have
   // a corresponding Remove() call in FinishDelayedInstallInfo().
-  if (extension->RequiresSortOrdinal()) {
+  if (extension->RequiresSortOrdinal() &&
+      (install_flags & kInstallFlagIsEphemeral) == 0) {
     extension_dict->SetString(
         kPrefSuggestedPageOrdinal,
         page_ordinal.IsValid() ? page_ordinal.ToInternalValue()