app_list: Drive app integration.

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 b77769ff455590584b3e6232c284f5dacda26f6b..21767b884349d2cdeefb5f10e4fc3c1afc5ad7da 100644
--- a/chrome/browser/extensions/crx_installer.cc
+++ b/chrome/browser/extensions/crx_installer.cc
@@ -229,21 +229,23 @@ void CrxInstaller::ConvertUserScriptOnFileThread() {
 void CrxInstaller::InstallWebApp(const WebApplicationInfo& web_app) {
   NotifyCrxInstallBegin();
 
+  DCHECK(creation_flags_ & Extension::FROM_BOOKMARK);

[not at google - send to devlin, 2014/06/05 17:50:45]

could you briefly explain why this check is made? the function name
"InstallWebApp" does not adequately imply the validity of this DCHECK without
some sort of explanation.

[xiyuan, 2014/06/05 18:06:53]

This is inherited from the current ConvertWebAppToExtension behavior.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

I put a DCHECK so that I don't break any expectations. This may not be
necessary.

Added a comment "The app created should always be marked as a bookmark app." to
explain the DCHECK. But I am open to removing it all together if you feel that
is the right thing.

[not at google - send to devlin, 2014/06/05 18:11:54]

Yeah, I think that a comment like "Web apps can only ever be converted to
bookmark apps".

[xiyuan, 2014/06/05 18:27:35]

Done.

   if (!installer_task_runner_->PostTask(
           FROM_HERE,
           base::Bind(&CrxInstaller::ConvertWebAppOnFileThread,
                      this,
                      web_app,
+                     creation_flags_,
                      install_directory_)))
     NOTREACHED();
 }
 
 void CrxInstaller::ConvertWebAppOnFileThread(
     const WebApplicationInfo& web_app,
+    int creation_flags,

[not at google - send to devlin, 2014/06/05 17:50:45]

this shouldn't be necessary, it can be read from creation_flags_.

[xiyuan, 2014/06/05 18:06:53]

I am slightly prefer to have an arg here because it avoid potential read/write
races since this function runs on a different thread (even though it is very
unlikely to happen). If we can access members, then |install_directory| is also
not necessarily to be an argument.

[not at google - send to devlin, 2014/06/05 18:11:54]

I see your point. It looks like a bunch of this accessing local variables
happens anyway, like in OnUnpackSuccess. Perhaps mark it (and install
directory?) as const for some peace of mind.

[xiyuan, 2014/06/05 18:27:35]

Right. I might be over-thinking it. :p

- Removed creation_flags and install_direction from arguments use members;
- |creation_flags_| could not be const because it has a setter;
- |install_directory_| is changed to be const;

[not at google - send to devlin, 2014/06/05 18:31:45]

ah.

     const base::FilePath& install_directory) {
-  base::string16 error;
-  scoped_refptr<Extension> extension(
-      ConvertWebAppToExtension(web_app, base::Time::Now(), install_directory));
+  scoped_refptr<Extension> extension(ConvertWebAppToExtension(
+      web_app, base::Time::Now(), creation_flags, install_directory));
   if (!extension.get()) {
     // Validation should have stopped any potential errors before getting here.
     NOTREACHED() << "Could not convert web app to extension.";