app_list: Drive app integration.

chrome/browser/apps/drive/drive_app_provider.cc

+// Copyright 2014 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/apps/drive/drive_app_provider.h"
+
+#include <vector>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/logging.h"
+#include "base/message_loop/message_loop.h"
+#include "base/stl_util.h"
+#include "chrome/browser/apps/drive/drive_app_converter.h"
+#include "chrome/browser/apps/drive/drive_app_mapping.h"
+#include "chrome/browser/apps/drive/drive_service_bridge.h"
+#include "chrome/browser/drive/drive_app_registry.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/install_tracker.h"
+#include "chrome/browser/extensions/install_tracker_factory.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/extensions/manifest_handlers/app_launch_info.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/browser/extension_system.h"
+#include "extensions/common/extension.h"
+
+using extensions::Extension;
+using extensions::ExtensionRegistry;
+
+namespace {
+
+void IgnoreUninstallResult(google_apis::GDataErrorCode) {
+}
+
+}  // namespace
+
+DriveAppProvider::DriveAppProvider(Profile* profile)
+    : profile_(profile),
+      service_bridge_(DriveServiceBridge::Create(profile).Pass()),
+      mapping_(new DriveAppMapping(profile->GetPrefs())),
+      weak_ptr_factory_(this) {
+  service_bridge_->GetAppRegistry()->AddObserver(this);
+  extensions::InstallTracker::Get(profile_)->AddObserver(this);
+}
+
+DriveAppProvider::~DriveAppProvider() {
+  extensions::InstallTracker::Get(profile_)->RemoveObserver(this);
+  service_bridge_->GetAppRegistry()->RemoveObserver(this);
+}
+
+// static
+void DriveAppProvider::AppendDependsOnFactories(
+    std::set<BrowserContextKeyedServiceFactory*>* factories) {
+  factories->insert(extensions::InstallTrackerFactory::GetInstance());
+  DriveServiceBridge::AppendDependsOnFactories(factories);
+}
+
+void DriveAppProvider::SetDriveServiceBridgeForTest(
+    scoped_ptr<DriveServiceBridge> test_bridge) {
+  service_bridge_->GetAppRegistry()->RemoveObserver(this);
+  service_bridge_ = test_bridge.Pass();
+  service_bridge_->GetAppRegistry()->AddObserver(this);
+}
+
+void DriveAppProvider::UpdateMappingAndExtensionSystem(
+    const std::string& drive_app_id,
+    const Extension* new_app) {
+  const std::string& new_chrome_app_id = new_app->id();
+
+  const std::string existing_chrome_app_id =
+      mapping_->GetChromeApp(drive_app_id);
+  if (existing_chrome_app_id == new_chrome_app_id)
+    return;
+
+  mapping_->Add(drive_app_id, new_chrome_app_id);
+
+  const Extension* existing_app =
+      ExtensionRegistry::Get(profile_)->GetExtensionById(
+          existing_chrome_app_id, ExtensionRegistry::EVERYTHING);
+  if (existing_app && existing_app->from_bookmark()) {

[benwells, 2014/06/11 00:15:20]

This from_bookmark doesn't differentiate between user created bookmark apps and
generated drive bookmark apps. I think that might be OK as only generated ones
will be mapped to drive apps. Is that right? It feels a bit fragile if correct.

If that is correct, can you add a comment explaining? Longer term I think we
need to rethink from_bookmark.

[xiyuan, 2014/06/11 04:52:34]

You are correct that the code could uninstall user created URL app. The case
should be rare but it's possible. If we want to be 100% sure, we could add a
boolean flag in the mappings to indicate whether the URL app is generated or not
and only uninstall the generated ones.

[benwells, 2014/06/12 00:16:02]

Actually I didn't think it was possible, and I don't understand what
circumstances it could happen in.

From what I can see things can get in the drive mapping in two ways:
- they are manually installed by the user and have an id that matches the drive
app's product id
- they were auto generated by a pending_converter_ thing

Is there some way that a user created bookmark app can get into the drive
mapping? Can you explain how?

[xiyuan, 2014/06/12 00:53:19]

The id of the URL apps are not random and derived from the URL. If user has
manually created an URL app that matches a Drive app's create url, the URL app
will then be in the mapping via the pending_converter_ way. I have added
UserInstallBefore/AfterDriveApp test to cover the cases.

[benwells, 2014/06/12 06:28:59]

Sorry I still don't get it (I'm probably being dumb). The only way something can
get into the drive map from a pending_converter_ is if it is scheduled via
OnDriveAppRegistryUpdated. Something the user creates won't go through
pending_conveters_ (will it??) so it won't ever get in the drive app -> chrome
app map, so it can't get deleted.

So I think your tests UserInstallBefore/AfterDriveApp should have passed before
the last change.

Maybe the problem is that the user could create one, then if this system tries
to create another that matches exactly, it won't actually succeed but the drive
mapping will be added anyway?

The reason I am asking is that I think patch set 9 was easier to understand. I
did think the logic was a bit fragile, but I think that problem is more with the
from_bookmark flag itself which is a bit of a hack. If you can go back to patch
set 9 and not have problems I think you should.

Sorry for slowing you down on this. I tried to patch in your change to
understand better but it wouldn't compile for me (or patch, but I forced it in).

[xiyuan, 2014/06/12 19:30:20]

This is where we differ. Suppose user installs an URL app that has exactly the
same URL as a Drive app's create URL. User installed app does not go through
converters but the Drive app meta would cause a converter be created to create
the URL app (with the same URL thus same id). This would cause the URL app's id
to get into the mapping. from_bookmark(), as you pointed out earlier would not
tell the difference. Thus, when the Drive app is removed (or switched to a
different URL), the user installed URL app will be auto uninstalled too. So
UserInstallBefore/AfterDriveApp would fail in PS 9.

This is an edge case that PS 9 does not handle and it is good that you pointed
out. PS 10 tracks the "generated" fact explicitly and we no longer uninstalls a
user installed URL app.
The tests would fail with PS 9.
Yes, the mapping will be added as long as the Drive app meta exists. Creating an
app with exactly matched URL succeeds and CrxInstaller treats it as an update.
To try the CL, you also need to patch https://codereview.chromium.org/323843003/
that I split out.

[benwells, 2014/06/13 02:03:01]

Thanks for the detailed explanation, I get it now :)

+    extensions::ExtensionSystem::Get(profile_)
+        ->extension_service()
+        ->UninstallExtension(existing_chrome_app_id, false, NULL);
+  }
+}
+
+void DriveAppProvider::ProcessDeferredOnExtensionInstalled(
+    const std::string drive_app_id,
+    const std::string chrome_app_id) {
+  const Extension* app = ExtensionRegistry::Get(profile_)->GetExtensionById(
+      chrome_app_id, ExtensionRegistry::EVERYTHING);
+  if (!app)
+    return;
+
+  UpdateMappingAndExtensionSystem(drive_app_id, app);
+}
+
+void DriveAppProvider::SchedulePendingConverters() {
+  if (pending_converters_.empty())
+    return;
+
+  if (!pending_converters_.front()->IsStarted())
+    pending_converters_.front()->Start();
+}
+
+void DriveAppProvider::OnLocalAppConverted(const DriveAppConverter* converter,
+                                           bool success) {
+  DCHECK_EQ(pending_converters_.front(), converter);
+
+  if (success) {
+    UpdateMappingAndExtensionSystem(converter->app_info().app_id,
+                                    converter->app());
+  } else {
+    LOG(WARNING) << "Failed to convert drive app to web app, "
+                 << "drive app id= " << converter->app_info().app_id
+                 << ", name=" << converter->app_info().app_name;
+  }
+
+  pending_converters_.erase(pending_converters_.begin());
+  SchedulePendingConverters();
+}
+
+bool DriveAppProvider::IsDriveAppUpToDate(
+    const drive::DriveAppInfo& drive_app) const {
+  const std::string& url_app_id = mapping_->GetChromeApp(drive_app.app_id);
+  if (url_app_id.empty())
+    return false;
+
+  const Extension* url_app = ExtensionRegistry::Get(profile_)->GetExtensionById(
+      url_app_id, ExtensionRegistry::EVERYTHING);
+  if (!url_app)
+    return false;
+
+  return drive_app.app_name == url_app->name() &&

[benwells, 2014/06/11 00:15:20]

What if there is a matching app but it is not a URL app. I.e. it was installed
manually and has app id matching the drive app's product_id?

Edit: OK I can see now this is not called in that case. I think a better name
would be IsGeneratedDriveAppUpToDate or something similar. What do you think?

[xiyuan, 2014/06/11 04:52:34]

Sounds good. Will rename.

[xiyuan, 2014/06/11 21:36:43]

Renamed to IsMappedUrlAppUpToDate because we should do the check for user
installed url app as well.

+         drive_app.create_url ==
+             extensions::AppLaunchInfo::GetLaunchWebURL(url_app);
+}
+
+void DriveAppProvider::AddOrUpdateDriveApp(
+    const drive::DriveAppInfo& drive_app) {
+  const Extension* chrome_app =
+      ExtensionRegistry::Get(profile_)->GetExtensionById(
+          drive_app.product_id, ExtensionRegistry::EVERYTHING);
+  if (chrome_app) {
+    UpdateMappingAndExtensionSystem(drive_app.app_id, chrome_app);
+    return;
+  }
+
+  if (IsDriveAppUpToDate(drive_app))
+    return;
+
+  ScopedVector<DriveAppConverter>::iterator it = pending_converters_.begin();
+  while (it != pending_converters_.end()) {
+    if (!(*it)->IsStarted() && (*it)->app_info().app_id == drive_app.app_id) {

[benwells, 2014/06/11 00:15:20]

What will happen if there is a converter in progress?

[xiyuan, 2014/06/11 04:52:34]

The "while" loop here is just an optimization. The converter could create the
same URL app multiple times (covered in InstallTwice test case). But it could
not reliably abort running process.  So when it happens, we waste some resources
to create the URL app twice.

[benwells, 2014/06/13 02:03:01]

If two of these do happen now, the second one will be marked as not generated,
which will uninstall the first. I think. Is that a problem?

[xiyuan, 2014/06/13 21:01:15]

The current code does not change the "generated" flag if the mapped url app id
is not changed. So the flag sticks.

I gave it a second look and feels the code is tricky. So I have added
|was_generated| flag check in OnLocalAppConverted to make this explicit
(generated flag should be kept no matter how many convertions it goes through)
and not depending on UpdateMappingAndExtensionSystem impl. Also added a
KeepGeneratedFlagBetweenUpdates to ensure it would not break later.

+      it = pending_converters_.erase(it);
+    } else {
+      ++it;
+    }
+  }
+
+  pending_converters_.push_back(
+      new DriveAppConverter(profile_,
+                            drive_app,
+                            base::Bind(&DriveAppProvider::OnLocalAppConverted,
+                                       base::Unretained(this))));
+}
+
+void DriveAppProvider::RemoveDriveApp(const std::string& drive_app_id) {

[benwells, 2014/06/11 00:15:21]

Nit: I think a better name would be ProcessRemovedDriveApp, as this doesn't go
an uninstall a drive app which the current name suggests.

[xiyuan, 2014/06/11 04:52:34]

Will do.

+  const std::string chrome_app_id = mapping_->GetChromeApp(drive_app_id);
+  mapping_->Remove(drive_app_id);
+
+  if (chrome_app_id.empty())
+    return;
+
+  const Extension* existing_app =
+      ExtensionRegistry::Get(profile_)
+          ->GetExtensionById(chrome_app_id, ExtensionRegistry::EVERYTHING);
+  if (!existing_app || !existing_app->from_bookmark())
+    return;
+
+  extensions::ExtensionSystem::Get(profile_)
+      ->extension_service()
+      ->UninstallExtension(chrome_app_id, false, NULL);
+}
+
+void DriveAppProvider::OnDriveAppRegistryUpdated() {

[benwells, 2014/06/11 00:15:20]

This is the only place that drive_apps_ is initialized. Is it guaranteed to be
called at startup, after this object is created?

[xiyuan, 2014/06/11 04:52:34]

It actually ties to the ProfileOAuth2TokenService's token status. If the profile
is signed in, DriveServiceBridgeImpl::Initialize will have
drive_service_->CanSendRequest() returns true and call
drive_app_registry_->Update() to trigger this. Otherwise,
DriveServiceBridgeImpl::OnReadyToSendRequests will be called when profile is
signed in and token is available and we call drive_app_registry_->Update() then
to trigger this.

+  service_bridge_->GetAppRegistry()->GetAppList(&drive_apps_);
+
+  IdSet current_ids;
+  for (size_t i = 0; i < drive_apps_.size(); ++i)
+    current_ids.insert(drive_apps_[i].app_id);
+
+  const IdSet existing_ids = mapping_->GetDriveAppIds();
+  const IdSet ids_to_remove =
+      base::STLSetDifference<IdSet>(existing_ids, current_ids);
+  for (IdSet::const_iterator it = ids_to_remove.begin();
+       it != ids_to_remove.end();
+       ++it) {
+    RemoveDriveApp(*it);
+  }
+
+  for (size_t i = 0; i < drive_apps_.size(); ++i) {
+    AddOrUpdateDriveApp(drive_apps_[i]);
+  }
+  SchedulePendingConverters();
+}
+
+void DriveAppProvider::OnExtensionInstalled(const Extension* extension) {
+  for (size_t i = 0; i < drive_apps_.size(); ++i) {
+    if (drive_apps_[i].product_id == extension->id()) {
+      base::MessageLoop::current()->PostTask(

[benwells, 2014/06/11 00:15:21]

Why is this deferred? Could you add a comment explaining?

[xiyuan, 2014/06/11 04:52:34]

Will add a comment. This is because we touch extensions system during the
processing, doing so in an observer callback might have problem that it might
have unexpected incomplete status.   

+          FROM_HERE,
+          base::Bind(&DriveAppProvider::ProcessDeferredOnExtensionInstalled,
+                     weak_ptr_factory_.GetWeakPtr(),
+                     drive_apps_[i].app_id,
+                     extension->id()));
+      return;
+    }
+  }
+}
+
+void DriveAppProvider::OnExtensionUninstalled(const Extension* extension) {
+  std::string drive_app_id = mapping_->GetDriveApp(extension->id());
+  if (drive_app_id.empty())
+    return;
+
+  service_bridge_->GetAppRegistry()->UninstallApp(
+      drive_app_id, base::Bind(&IgnoreUninstallResult));
+}