app_list: Drive app integration.

chrome/browser/apps/drive/drive_app_converter.cc

Index: chrome/browser/apps/drive/drive_app_converter.cc
diff --git a/chrome/browser/ui/app_list/drive/drive_app_converter.cc b/chrome/browser/apps/drive/drive_app_converter.cc
similarity index 89%
rename from chrome/browser/ui/app_list/drive/drive_app_converter.cc
rename to chrome/browser/apps/drive/drive_app_converter.cc
index 096a6f37cebfd151157eb6936374b86810ff86b8..d0e2a965abb25465491a24764e8fa3cde7f111f6 100644
--- a/chrome/browser/ui/app_list/drive/drive_app_converter.cc
+++ b/chrome/browser/apps/drive/drive_app_converter.cc
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/ui/app_list/drive/drive_app_converter.h"
+#include "chrome/browser/apps/drive/drive_app_converter.h"
 
 #include <algorithm>
 #include <set>
@@ -115,8 +115,9 @@ DriveAppConverter::~DriveAppConverter() {
 }
 
 void DriveAppConverter::Start() {
-  if (app_info_.app_name.empty() ||
-      !app_info_.create_url.is_valid()) {
+  DCHECK(!IsStarted());
+
+  if (app_info_.app_name.empty() || !app_info_.create_url.is_valid()) {
     finished_callback_.Run(this, false);
     return;
   }
@@ -146,6 +147,10 @@ void DriveAppConverter::Start() {
     StartInstall();
 }
 
+bool DriveAppConverter::IsStarted() const {
+  return !fetchers_.empty() || crx_installer_;
+}
+
 void DriveAppConverter::OnIconFetchComplete(const IconFetcher* fetcher) {
   const SkBitmap& icon = fetcher->icon();
   if (!icon.empty() && icon.width() != 0) {
@@ -167,6 +172,15 @@ void DriveAppConverter::StartInstall() {
   DCHECK(!crx_installer_);
   crx_installer_ = extensions::CrxInstaller::CreateSilent(
       extensions::ExtensionSystem::Get(profile_)->extension_service());
+
+  // Sets WAS_INSTALLED_BY_DEFAULT to make the converted url app not syncable.
+  // Drive apps go with the user's account and url apps will be created when
+  // needed. Syncing those apps could hit an edge case where the synced url
+  // apps become orphans when the user has corresponding chrome apps.
+  crx_installer_->set_creation_flags(
+      extensions::Extension::FROM_BOOKMARK |
+      extensions::Extension::WAS_INSTALLED_BY_DEFAULT);

[stevenjb, 2014/06/05 20:15:16]

WAS_INSTALLED_BY_DEFAULT might trigger other behaviors, maybe we should add an
explicit IS_SYNCABLE flag?

[xiyuan, 2014/06/05 20:36:14]

The converted app is sort of a default (in a sense that it comes as part of the
user's account). I checked where WAS_INSTALLED_BY_DEFAULT is used (not many of
places) and seems to be safe to use.

But I'll leave this to the judgement of extensions OWNERs (benwells@, yoz@,
kalman@) and suggestions of what to use.

[Yoyo Zhou, 2014/06/05 20:45:33]

This isn't really what we mean by default apps; those come with the chrome
installation, rather than with a drive login. One of these would be better:

- if we could determine from the extension that it was a drive app, and
therefore not to sync it (see several existing cases in GetSyncType in
sync_helper.cc)
- if there was a new creation flag like SYNC_NOT_ALLOWED

[not at google - send to devlin, 2014/06/05 20:47:47]

I agree with Steven, even if this works at the moment it's absolutely not
"installed by default" and it's things like that have made sync bugs creep in in
the past.

Attaching this stuff to "creation flags" isn't really right anyway. Things like
it being a bookmark is a property of the environment not a property of the
extension itself, if that makes sense (basically: it should be stored in Prefs
not on the Extension object, if it needs to be).

If you don't want to sync it, how about a set_do_not_sync(..) function on the
CrxInstaller.

[xiyuan, 2014/06/05 21:17:06]

Okay.

- Add a ExtensionPrefs backed boolean property do_not_sync() to Extension;
- The property is initiated in CrxInstaller when creating the URL app;
- GetSyncType checks this property and returns SYNC_TYPE_NONE to disable sync
for this item. 

How does this sound?

[Yoyo Zhou, 2014/06/05 21:18:20]

Minor detail: you can check this in IsSyncable instead of GetSyncType. But
sounds fine to me.

+
   extensions::InstallTracker::Get(profile_)->AddObserver(this);
   crx_installer_->InstallWebApp(web_app_);
 }
@@ -187,7 +201,8 @@ void DriveAppConverter::OnFinishCrxInstall(const std::string& extension_id,
   }
 
   app_ = crx_installer_->extension();
-  finished_callback_.Run(this, success);
-
   PostInstallCleanUp();
+
+  finished_callback_.Run(this, success);
+  // |finished_callback_| could delete this.
 }