Do not assign launch ordinals to ephemeral apps

chrome/browser/extensions/extension_service.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/extension_service.h"
 
 #include <algorithm>
 #include <iterator>
 #include <set>
 
@@ skipping 1415 matching lines @@
               extension->id(), Extension::DISABLE_REMOTE_INSTALL));
     }
   } else if (reloading) {
     // Replace the old extension with the new version.
     CHECK(!registry_->AddDisabled(extension));
     EnableExtension(extension->id());
   } else {
     // 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()));

[tapted, 2014/07/17 04:29:46]

are ephemeral apps never displayed in the new tab page? (i.e. should
extension->ShouldDisplayInNewTabPage() return false for ephemeral apps?). 

Although.. the same might also be true for
Extension::ShouldDisplayInAppLauncher, and if they're both true they disappear
from chrome;//extensions as well unless you update
Extension::ShouldDisplayInExtensionSettings() to return true for ephemeral apps
early on.

But I think Extension::ShouldDisplayInXXX() should be consistent with what's
done for ephemeral apps.

[tmdiep, 2014/07/17 05:48:57]

I wish it could be just fully encapsulated in the Extension object. The problem
is that IsEphemeral is a runtime flag can change even while the app is running
and the Extension object can't access prefs.

[tapted, 2014/07/17 05:59:53]

Ah - drat.

+      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);
     if (extension_sync_service_)
       extension_sync_service_->SyncExtensionChangeIfNeeded(*extension);
     NotifyExtensionLoaded(extension);
   }
   system_->runtime_data()->SetBeingUpgraded(extension, false);
 }
 
@@ skipping 387 matching lines @@
   // was not available.
   if (SharedModuleInfo::IsSharedModule(extension))
     MaybeFinishDelayedInstallations();
 }
 
 void ExtensionService::PromoteEphemeralApp(
     const extensions::Extension* extension, bool is_from_sync) {
   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) {

[tapted, 2014/07/17 04:29:46]

After tracing down ordinal CHECK()s in the past I'm not 100% sure an ordinal is
guaranteed to come in from sync. Although the odds might be increasingly
unlikely and obscure - e.g. ~2years ago before there was "launch ordinal" there
was a different kind of ordinal. ntp/app_launcher_handler.cc has code to upgrade
them. But if someone ran Chrome 2 years ago with sync enabled does a new
install. Runs an ephemeral app, then signs in and gets their old sync data for
the same app, it might have the wrong kind of ordinal.

But I think the odds of this are too remote to bother with adding logic for :/.
You could perhaps do

if (is_from_sync) {
  bool has_ordinal = extension_prefs_->app_sorting()
      ->GetAppLaunchOrdinal(extension->id()).IsValid();
  CHECK(has_ordinal);
} else {
 ...
}

but I don't think it's necessary unless there's other more likely scenarios you
can think of.

[tmdiep, 2014/07/17 05:48:57]

I've refactored to always call EnsureValidOrdinals, as there is no harm in doing
so, but maybe more harm in not doing so.

It relies on sync having updated the launch ordinals in the extension prefs
before installing the extension. Can we rely on this order of events? Probably,
otherwise if the install came first, the call to EnsureValidOrdinals in
ExtensionService::AddExtension would clobber the ordinals.

       // 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());
     }
   }
 
   // Remove the ephemeral flags from the preferences.
   extension_prefs_->OnEphemeralAppPromoted(extension->id());
@@ skipping 515 matching lines @@
 }
 
 void ExtensionService::OnProfileDestructionStarted() {
   ExtensionIdSet ids_to_unload = registry_->enabled_extensions().GetIDs();
   for (ExtensionIdSet::iterator it = ids_to_unload.begin();
        it != ids_to_unload.end();
        ++it) {
     UnloadExtension(*it, UnloadedExtensionInfo::REASON_PROFILE_SHUTDOWN);
   }
 }