Mac Web Intents Part 1: Show extension download progress

chrome/browser/ui/intents/web_intent_picker_controller.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/ui/intents/web_intent_picker_controller.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/md5.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
+#include "chrome/browser/download/download_item_model.h"
+#include "chrome/browser/download/download_util.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/platform_app_launcher.h"
 #include "chrome/browser/extensions/webstore_installer.h"
 #include "chrome/browser/favicon/favicon_service.h"
 #include "chrome/browser/favicon/favicon_service_factory.h"
 #include "chrome/browser/intents/cws_intents_registry_factory.h"
 #include "chrome/browser/intents/default_web_intent_service.h"
 #include "chrome/browser/intents/web_intents_registry_factory.h"
 #include "chrome/browser/intents/web_intents_reporting.h"
 #include "chrome/browser/intents/web_intents_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/tab_contents/tab_util.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_navigator.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/constrained_window_tab_helper.h"
 #include "chrome/browser/ui/intents/web_intent_picker.h"
 #include "chrome/browser/ui/intents/web_intent_picker_model.h"
 #include "chrome/browser/ui/tab_contents/tab_contents.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/webdata/web_data_service.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/url_constants.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/download_manager.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_intents_dispatcher.h"
 #include "grit/generated_resources.h"
 #include "ipc/ipc_message.h"
 #include "net/base/load_flags.h"
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_fetcher_delegate.h"
@@ skipping 410 matching lines @@
 }
 
 void WebIntentPickerController::OnInlineDispositionWebContentsCreated(
     content::WebContents* web_contents) {
   if (web_contents)
     intents_dispatcher_->DispatchIntent(web_contents);
 }
 
 void WebIntentPickerController::OnExtensionInstallRequested(
     const std::string& id) {
+  picker_model_->SetPendingInstallExtensionId(id);

[Greg Billock, 2012/09/25 07:15:46]

Needed? I don't see where this is used.

[Greg Billock, 2012/10/02 03:02:29]

Now used?

[sail, 2012/10/02 18:57:01]

Yea, used here:
http://codereview.chromium.org/11009017/diff/1015/chrome/browser/ui/cocoa/int...

+
   scoped_ptr<WebstoreInstaller::Approval> approval(
       WebstoreInstaller::Approval::CreateWithInstallPrompt(
           tab_contents_->profile()));
 
   scoped_refptr<WebstoreInstaller> installer = new WebstoreInstaller(
       tab_contents_->profile(), this,
       &tab_contents_->web_contents()->GetController(), id,
       approval.Pass(), WebstoreInstaller::FLAG_INLINE_INSTALL);
 
   pending_async_count_++;
@@ skipping 38 matching lines @@
 
 void WebIntentPickerController::OnChooseAnotherService() {
   DCHECK(intents_dispatcher_);
   web_intents::RecordChooseAnotherService(uma_bucket_);
   intents_dispatcher_->ResetDispatch();
 }
 
 void WebIntentPickerController::OnClosing() {
   SetDialogState(kPickerHidden);
   picker_ = NULL;
+  CancelDownload();
 #if defined(TOOLKIT_VIEWS)
   if (cancelled_)
     OnUserCancelledPickerDialog();
 #endif
 }
 
+void WebIntentPickerController::OnExtensionDownloadStarted(
+    const std::string& id,
+    content::DownloadItem* item) {
+  download_util::SetShouldShowInShelf(item, false);

[Nico, 2012/09/25 03:07:18]

From what I understand, this relies on this notification running before the
"download started" notification, so that this flag is set before the shelf
decides if it needs to do anything. Is that right? Is that enforced somehow?

[asanka, 2012/09/25 16:29:30]

The ordering is:
1. DownloadManager::OnDownloadCreated().
   Called when the DownloadItem is created.

2. OnStartedCallback for the download request.
   This function (OnExtensionDownloadStarted) is running as part of this step.
ShowShouldInShelf flag is set here.

3. <download filename determination>

4. WebContentsDelegate::OnStartDownload().
   Browser::OnStartDownload() checks the ShouldShowInShelf flag here.

[Steve McKay, 2012/09/25 18:56:04]

What if we flipped the perspective a bit and started all downloads as "system"
download, then promote them to "user" downloads once we know the download is
actually a user "download" and not the system loading the data for some other
purpose.

[Greg Billock, 2012/10/02 19:20:38]

When we use a separate RequestHandler for intents, we'll basically have that.
That'll likely be able to make use of this machinery, though (the show-in-shelf
flag is great).

Thanks, asanka, for the clarification.


On 2012/09/25 18:56:04, Steve McKay wrote:

+  download_id_ = item->GetGlobalId();
+  UpdateDownloadState(id, item);
+}
+
+void WebIntentPickerController::OnExtensionDownloadProgress(
+    const std::string& id,
+    content::DownloadItem* item) {
+  UpdateDownloadState(id, item);
+}
+
 void WebIntentPickerController::OnExtensionInstallSuccess(
     const std::string& extension_id) {
   // OnExtensionInstallSuccess is called via NotificationService::Notify before
   // the extension is added to the ExtensionService. Dispatch via PostTask to
   // allow ExtensionService to update.
   MessageLoop::current()->PostTask(
       FROM_HERE,
       base::Bind(
           &WebIntentPickerController::DispatchToInstalledExtension,
           base::Unretained(this),
           extension_id));
 }
 
 void WebIntentPickerController::DispatchToInstalledExtension(
     const std::string& extension_id) {
   web_intents::RecordCWSExtensionInstalled(uma_bucket_);
-  picker_->OnExtensionInstallSuccess(extension_id);
+
+  download_id_ = content::DownloadId();
+  picker_model_->ClearPendingInstall();
+  if (picker_)

[groby-ooo-7-16, 2012/09/25 22:20:56]

nit: This should never be NULL. Can we just DCHECK(picker_)?

[sail, 2012/10/02 18:57:01]

Unfortunately there's this function is called asynchronously and the picker my
be set to NULL in the meantime.

+    picker_->OnExtensionInstallSuccess(extension_id);
+
   WebIntentsRegistry::IntentServiceList services;
   GetWebIntentsRegistry(tab_contents_)->GetIntentServicesForExtensionFilter(
       picker_model_->action(), picker_model_->type(),
       extension_id,
       &services);
 
   // Extension must be registered with registry by now.
   DCHECK(services.size() > 0);
 
   // TODO(binji): We're going to need to disambiguate if there are multiple
   // services. For now, just choose the first.
   const webkit_glue::WebIntentServiceData& service_data = services[0];
 
   picker_model_->AddInstalledService(
       service_data.title, service_data.service_url,
       service_data.disposition);
   OnServiceChosen(service_data.service_url, service_data.disposition);
   AsyncOperationFinished();
 }
 
 void WebIntentPickerController::OnExtensionInstallFailure(
     const std::string& id,
-    const std::string& error) {
-  picker_->OnExtensionInstallFailure(id);
+    const std::string& error,
+    bool cancelled) {
+  CancelDownload();

[asanka, 2012/09/25 16:29:30]

Why call CancelDownload() here?

[sail, 2012/10/02 18:57:01]

For example, if we weren't able to unpack the extension this would be called but
the download would be still marked as pending. We need to explicitly cancel
here.

+  // If the user cancelled the install then don't show an error message.

[Nico, 2012/09/25 03:07:18]

nit: canceled

[sail, 2012/10/02 18:57:01]

Weird, it looks like we use both in chrome/*:
dhcp-172-19-0-37:chrome sail$ grep -ris canceled * | wc -l
     583
dhcp-172-19-0-37:chrome sail$ grep -ris cancelled * | wc -l
     896

intents/* only uses cancelled so keeping that.

+  if (cancelled)
+    picker_model_->ClearPendingInstall();
+  else
+    picker_model_->SetPendingInstallStatusString(UTF8ToUTF16(error));
+
+  if (picker_)
+    picker_->OnExtensionInstallFailure(id);
   AsyncOperationFinished();
 }
 
 void WebIntentPickerController::OnSendReturnMessage(
     webkit_glue::WebIntentReplyType reply_type) {
   ClosePicker();
   uma_reporter_->RecordServiceActiveDuration(reply_type);
 
   if (service_tab_ &&
       reply_type != webkit_glue::WEB_INTENT_SERVICE_CONTENTS_CLOSED) {
@@ skipping 468 matching lines @@
   web_intents::RecordPickerShow(
       uma_bucket_, picker_model_->GetInstalledServiceCount());
   picker_shown_ = true;
 }
 
 void WebIntentPickerController::ClosePicker() {
   SetDialogState(kPickerHidden);
   if (picker_)
     picker_->Close();
 }
+
+void WebIntentPickerController::UpdateDownloadState(

[Steve McKay, 2012/09/25 18:56:04]

Feels like this entire method should be pushed into the picker model...and named
UpdateExtensionDownloadState.

[sail, 2012/10/02 18:57:01]

Done.

+    const std::string& id,
+    content::DownloadItem* item) {
+  picker_model_->SetPendingInstallDownloadPercent(item->PercentComplete());
+  DownloadItemModel download_model(item);
+  picker_model_->SetPendingInstallStatusString(download_model.GetStatusText());
+}
+
+void WebIntentPickerController::CancelDownload() {

[Steve McKay, 2012/09/25 18:56:04]

CancelExtensionDownload

Does the picker model need to be updated too?

[sail, 2012/10/02 18:57:01]

Done.
This used to be in the caller code. Moved to here.

+  if (!download_id_.IsValid())
+    return;
+  content::DownloadManager* download_manager =
+      content::BrowserContext::GetDownloadManager(tab_contents_->profile());
+  if (!download_manager)
+    return;
+  content::DownloadItem* item =
+      download_manager->GetDownload(download_id_.local());
+  if (item)
+    item->Cancel(true);
+  download_id_ = content::DownloadId();
+}