Make application shortcuts point to app_host.exe, install App Host during app installation.

chrome/browser/extensions/crx_installer.cc

Index: chrome/browser/extensions/crx_installer.cc
diff --git a/chrome/browser/extensions/crx_installer.cc b/chrome/browser/extensions/crx_installer.cc
index 040e353d098d4a95f43a556f0f4e96a215968a18..c047cb6b39749eeb62df2f2350026fa23942b5a8 100644
--- a/chrome/browser/extensions/crx_installer.cc
+++ b/chrome/browser/extensions/crx_installer.cc
@@ -20,6 +20,7 @@
 #include "base/utf_string_conversions.h"
 #include "base/version.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/extensions/app_host_installer.h"
 #include "chrome/browser/extensions/convert_user_script.h"
 #include "chrome/browser/extensions/convert_web_app.h"
 #include "chrome/browser/extensions/extension_error_reporter.h"
@@ -463,7 +464,7 @@ void CrxInstaller::ConfirmInstall() {
   } else {
     if (!BrowserThread::PostTask(
             BrowserThread::FILE, FROM_HERE,
-            base::Bind(&CrxInstaller::CompleteInstall, this)))
+            base::Bind(&CrxInstaller::BeginInstall, this)))
       NOTREACHED();
   }
   return;
@@ -472,7 +473,7 @@ void CrxInstaller::ConfirmInstall() {
 void CrxInstaller::InstallUIProceed() {
   if (!BrowserThread::PostTask(
           BrowserThread::FILE, FROM_HERE,
-          base::Bind(&CrxInstaller::CompleteInstall, this)))
+          base::Bind(&CrxInstaller::BeginInstall, this)))
     NOTREACHED();
 
   Release();  // balanced in ConfirmInstall().
@@ -500,9 +501,8 @@ void CrxInstaller::InstallUIAbort(bool user_initiated) {
   // should go to zero and we die. The destructor will clean up the temp dir.
 }
 
-void CrxInstaller::CompleteInstall() {
+void CrxInstaller::BeginInstall() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-
   if (!current_version_.empty()) {
     Version current_version(current_version_);
     if (current_version.CompareTo(*(extension_->version())) > 0) {
@@ -515,6 +515,24 @@ void CrxInstaller::CompleteInstall() {
     }
   }
 
+  app_host_installer::InstallAppHostIfNecessary(

[Aaron Boodman, 2012/10/25 04:39:41]

This seems out of place and like a layering violation.

Conceptually, apps should depend on extensions, but not the reverse. Eventually,
we will make this real with DEPS, but for now we should not introduce more
issues.

CRXInstaller should be about installing packages, that's it. It shouldn't know
about the details of what's in those packages. Clients of CRXInstaller should
use an abstract interface to begin installation and to register for
notifications of its progress.

So I think that you should abstract out some kind of
OnInstallCompleteOnFileThread() API that other clients could also use.

[erikwright (departed), 2012/10/26 20:32:30]

OK. It would be an API similar to the following:

class ExtensionInstallDelegate {
 public:
  // Called after permissions have been granted and the install is ready to
proceed. The delegate should call |completion_callback| with true to complete
the installation or false to abort.
  virtual void OnReadyToInstall(Extension *, base::Callback<void(bool)>
completion_callback) = 0;
};

And every CrxInstaller that could theoretically be used with a platform app
needs to have our implementation of the delegate. So it would presumably make
sense to centralize the instantiation of CrxInstallers (and UnpackedInstallers)
in ExtensionService.

Also, it seems that you mean you don't even want this functionality in
chrome/browser/extensions. Is that right? If so, it would imply having some way
to register the listeners on the ExtensionService, and some other part of Chrome
would need to implement the logic to install the launcher (which part?).

And if I'm right that you don't want this in c/b/extensions, presumably that
also applies to AppShortcutManager?

[Aaron Boodman, 2012/10/29 23:37:29]

Imagine a world after:

mv /chrome/browser/extensions/api /chrome/browser/extension_apis
mv /chrome/browser/extensions/*app* /apps/*
mv /chrome/browser/extensions /extensions

/apps depends on /extensions
/chrome/browser depends on both extensions and apps

That is the world we want to move toward. It doesn't make sense for
ExtensionService to create the ExtensionInstallDelegateForApps class in such a
world, because it doesn't know anything about apps.

It sounds like this centralization is needed somewhere in chrome/browser.
Conceptually, something like "Browser::InitiateExtensionInstall()".
I don't feel as strongly about the directories right now, but if there's an
obvious home for this (/c/b/apps/shortcut_manager?) then I think it probably
makes sense to move it there so that people see it and have a better chance of
doing the right thing in the future.

+      *extension_,
+      base::Bind(&CrxInstaller::OnAppHostInstallationComplete, this));

[Aaron Boodman, 2012/10/25 04:39:41]

I didn't have time to read all the code behind InstallAppHostIfNecessary, but it
wasn't immediately clear why it is asynchronous. We are already on the FILE
thread here. Can InstallAppHostIfNecessary just block? If so that would make the
client API to CRXInstaller somewhat simpler.

[erikwright (departed), 2012/10/26 20:32:30]

We launch an external process, which can take an arbitrary length of time. I
think it's best to be asynchronous.

[Aaron Boodman, 2012/10/29 23:37:29]

You could move CrxInstaller to use the BlockingTaskPool, so that it's not a big
deal if this takes a long time. That way we don't need another resume API on
CrxInstaller.

OTOH, if you're going to keep this async, consider putting the callback to the
interface on the UI thread, since that would be more generally useful.

[huangs, 2012/11/01 22:55:48]

Per our discussion, moving installation to AppShortcutManager.

+}
+
+void CrxInstaller::OnAppHostInstallationComplete(bool success) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  if (!success) {
+    ReportFailureFromFileThread(CrxInstallerError(
+        l10n_util::GetStringUTF16(IDS_EXTENSION_APP_HOST_INSTALL_ERROR)));
+    return;
+  }
+
+  CompleteInstall();
+}
+
+void CrxInstaller::CompleteInstall() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   // See how long extension install paths are.  This is important on
   // windows, because file operations may fail if the path to a file
   // exceeds a small constant.  See crbug.com/69693 .