app_list: Drive app integration.

chrome/browser/extensions/crx_installer.cc

 // Copyright (c) 2012 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/crx_installer.h"
 
 #include <map>
 #include <set>
 
 #include "base/bind.h"
@@ skipping 211 matching lines @@
     return;
   }
 
   OnUnpackSuccess(extension->path(), extension->path(), NULL, extension.get(),
                   SkBitmap());
 }
 
 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.";
     return;
   }
 
   // TODO(aa): conversion data gets lost here :(
 
   OnUnpackSuccess(extension->path(), extension->path(), NULL, extension.get(),
                   SkBitmap());
@@ skipping 650 matching lines @@
   if (!prefs->DidExtensionEscalatePermissions(extension()->id()))
     return;
 
   if (client_) {
     AddRef();  // Balanced in InstallUIProceed() and InstallUIAbort().
     client_->ConfirmReEnable(this, extension());
   }
 }
 
 }  // namespace extensions