Mac Web Intents Part 1: Show extension download progress

chrome/browser/ui/intents/web_intent_picker_controller.cc

Index: chrome/browser/ui/intents/web_intent_picker_controller.cc
diff --git a/chrome/browser/ui/intents/web_intent_picker_controller.cc b/chrome/browser/ui/intents/web_intent_picker_controller.cc
index 75dd7fd8dab27847bb2d0ca05f100183a996dca7..9fc23636316e5adb8e95194a613cfff75c53d3d9 100644
--- a/chrome/browser/ui/intents/web_intent_picker_controller.cc
+++ b/chrome/browser/ui/intents/web_intent_picker_controller.cc
@@ -12,6 +12,8 @@
 #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"
@@ -39,6 +41,7 @@
 #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"
@@ -469,6 +472,8 @@ void WebIntentPickerController::OnInlineDispositionWebContentsCreated(
 
 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()));
@@ -527,12 +532,27 @@ void WebIntentPickerController::OnChooseAnotherService() {
 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
@@ -549,7 +569,12 @@ void WebIntentPickerController::OnExtensionInstallSuccess(
 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(),
@@ -572,8 +597,17 @@ void WebIntentPickerController::DispatchToInstalledExtension(
 
 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();
 }
 
@@ -1062,3 +1096,25 @@ void WebIntentPickerController::ClosePicker() {
   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();
+}