Do not assign launch ordinals to ephemeral apps

chrome/browser/extensions/extension_service.cc

Index: chrome/browser/extensions/extension_service.cc
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index 6a0b4b6e4410f62ba0964d252c1a163df7999f7b..4a9fecd17c439e4f78c67fd6b20f282f80620bfd 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -1433,11 +1433,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()));

[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);
@@ -1845,8 +1848,11 @@ 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) {

[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());