[Extensions] Rework inline installation observation

chrome/browser/extensions/tab_helper.cc

Index: chrome/browser/extensions/tab_helper.cc
diff --git a/chrome/browser/extensions/tab_helper.cc b/chrome/browser/extensions/tab_helper.cc
index bc92af0b88af50c8d971f4b81f97573ccb8a3a08..c58143a32e21de559ce70ac40104902af6a4ff3c 100644
--- a/chrome/browser/extensions/tab_helper.cc
+++ b/chrome/browser/extensions/tab_helper.cc
@@ -12,12 +12,14 @@
 #include "chrome/browser/extensions/activity_log/activity_log.h"
 #include "chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry.h"
 #include "chrome/browser/extensions/api/extension_action/extension_action_api.h"
-#include "chrome/browser/extensions/api/webstore/webstore_api.h"
 #include "chrome/browser/extensions/bookmark_app_helper.h"
 #include "chrome/browser/extensions/chrome_extension_web_contents_observer.h"
 #include "chrome/browser/extensions/extension_action_runner.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
 #include "chrome/browser/extensions/extension_util.h"
+#include "chrome/browser/extensions/install_observer.h"
+#include "chrome/browser/extensions/install_tracker.h"
+#include "chrome/browser/extensions/install_tracker_factory.h"
 #include "chrome/browser/extensions/location_bar_controller.h"
 #include "chrome/browser/extensions/webstore_inline_installer.h"
 #include "chrome/browser/extensions/webstore_inline_installer_factory.h"
@@ -28,6 +30,7 @@
 #include "chrome/browser/ui/browser_dialogs.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/web_applications/web_app.h"
+#include "chrome/common/extensions/api/webstore/webstore_api_constants.h"
 #include "chrome/common/extensions/chrome_extension_messages.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/extensions/manifest_handlers/app_launch_info.h"
@@ -72,6 +75,77 @@ DEFINE_WEB_CONTENTS_USER_DATA_KEY(extensions::TabHelper);
 
 namespace extensions {
 
+// A helper class to watch the progress of inline installation and update the
+// renderer. Owned by the TabHelper.
+class TabHelper::InlineInstallObserver : public InstallObserver {
+ public:
+  InlineInstallObserver(TabHelper* tab_helper,
+                        content::BrowserContext* browser_context,
+                        int routing_id,
+                        const std::string& extension_id,
+                        bool observe_download_progress,
+                        bool observe_install_stage)
+      : tab_helper_(tab_helper),
+        routing_id_(routing_id),
+        extension_id_(extension_id),
+        observe_download_progress_(observe_download_progress),
+        observe_install_stage_(observe_install_stage),
+        install_observer_(this) {
+    DCHECK(observe_download_progress || observe_install_stage);

[lazyboy, 2016/06/29 21:03:03]

DCHECK(tab_helper_)

[Devlin, 2016/06/29 22:32:09]

Sure, can't hurt.

+    InstallTracker* install_tracker =
+        InstallTrackerFactory::GetForBrowserContext(browser_context);
+    if (install_tracker)
+      install_observer_.Add(install_tracker);
+  }
+  ~InlineInstallObserver() override {}
+
+ private:
+  // InstallObserver:
+  void OnBeginExtensionDownload(const std::string& extension_id) override {
+    SendInstallStageChangedMessage(extension_id,
+                                   api::webstore::INSTALL_STAGE_DOWNLOADING);
+  }
+  void OnDownloadProgress(const std::string& extension_id,
+                          int percent_downloaded) override {
+    if (observe_download_progress_ && extension_id == extension_id_) {
+      tab_helper_->Send(new ExtensionMsg_InlineInstallDownloadProgress(
+                            routing_id_, percent_downloaded));
+    }
+  }
+  void OnBeginCrxInstall(const std::string& extension_id) override {
+    SendInstallStageChangedMessage(extension_id,
+                                   api::webstore::INSTALL_STAGE_INSTALLING);
+  }
+  void OnShutdown() override {
+    install_observer_.RemoveAll();
+  }
+
+  void SendInstallStageChangedMessage(const std::string& extension_id,
+                                      api::webstore::InstallStage stage) {
+    if (observe_install_stage_ && extension_id == extension_id_) {
+      tab_helper_->Send(new ExtensionMsg_InlineInstallStageChanged(
+                            routing_id_, stage));
+    }
+  }
+
+  // The owning TabHelper (guaranteed to be valid).
+  TabHelper* tab_helper_;

[lazyboy, 2016/06/29 21:03:04]

Mark this and last two bool members const.

[Devlin, 2016/06/29 22:32:09]

Done (assuming you mean TabHelper* const tab_helper_, and not const TabHelper*
tab_helper_ - Send() is not const so the latter is impossible).

[lazyboy, 2016/06/29 23:10:27]

Yes that's what I meant.
, and not const TabHelper*

+
+  // The routing id to use in sending IPC updates.
+  int routing_id_;
+
+  // The id of the extension to observe.
+  std::string extension_id_;
+
+  // Whether or not to observe download/install progress.
+  bool observe_download_progress_;
+  bool observe_install_stage_;
+
+  ScopedObserver<InstallTracker, InstallObserver> install_observer_;
+
+  DISALLOW_COPY_AND_ASSIGN(InlineInstallObserver);
+};
+
 TabHelper::TabHelper(content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       profile_(Profile::FromBrowserContext(web_contents->GetBrowserContext())),
@@ -369,6 +443,18 @@ void TabHelper::OnInlineWebstoreInstall(content::RenderFrameHost* host,
     NOTREACHED();
     return;
   }
+
+  if (pending_inline_installations_.count(webstore_item_id) != 0) {

[lazyboy, 2016/06/29 21:03:03]

This seems like a new behavior? Test would be helpful :)

[Devlin, 2016/06/29 22:32:09]

D'oh, I knew you would say that! ;)

So, looking into this, it's actually rather painful.
- We have a check renderer side as well that prevents duplicate installs early.
- But that won't catch all cases - e.g., if two separate JS contexts call it.
- They emit separate error messages. :(

I added a test for the most common case - two calls from the same JS context -
but I'm not sure it's worth it to manufacture a test for the other.  Let me know
what you think.

[lazyboy, 2016/06/29 23:10:27]

Thanks!
Maybe file a bug (GoodFirstBug?) to add the test?
I can clearly see how the test can be painful to write :), I'd file a bug
instead then with some notes about this as this is not an uncommon use case.
Just for a note to put in the bug: I'd say window.open and window.opener might
be helpful for the test to execute 2 install calls from 2 different JS contexts.
Let me know

[Devlin, 2016/06/30 14:46:55]

Filed a bug: 624614

+    Send(new ExtensionMsg_InlineWebstoreInstallResponse(
+        return_route_id,
+        install_id,
+        false,
+        "An install is already in progress",

[lazyboy, 2016/06/29 21:03:03]

This needs to be in a constant, pull out the constant from
webstore_standalone_installer.cc/kInstallInProgressError to
webstore_constants.cc

[Devlin, 2016/06/29 22:32:09]

Done.

+        webstore_install::INSTALL_IN_PROGRESS));
+    return;
+  }
+
+  pending_inline_installations_.insert(webstore_item_id);
   // Inform the Webstore API that an inline install is happening, in case the
   // page requested status updates.
   ExtensionRegistry* registry = ExtensionRegistry::Get(profile_);
@@ -389,18 +475,31 @@ void TabHelper::OnInlineWebstoreInstall(content::RenderFrameHost* host,
           base::Bind(&TabHelper::OnReenableComplete,
                      weak_ptr_factory_.GetWeakPtr(),
                      install_id,
-                     return_route_id));
+                     return_route_id,
+                     webstore_item_id));
   } else {
     // TODO(devlin): We should adddress the case of the extension already

[lazyboy, 2016/06/29 21:03:04]

Doesn't the beginning of the function prevent this (extension already being
installed) now?

[Devlin, 2016/06/29 22:32:09]

Nope.  The beginning prevents two inline installations at the same time, but if
the item is already installed and enabled, it doesn't.

[lazyboy, 2016/06/29 23:10:27]

Acknowledged.

     // being installed and enabled.
-    WebstoreAPI::Get(profile_)->OnInlineInstallStart(
-        return_route_id, this, webstore_item_id, listeners_mask);
+    bool observe_download_progress =
+        (listeners_mask & api::webstore::DOWNLOAD_PROGRESS_LISTENER) != 0;
+    bool observe_install_stage =
+        (listeners_mask & api::webstore::INSTALL_STAGE_LISTENER) != 0;
+    if (observe_install_stage || observe_download_progress) {
+      DCHECK(install_observers_.find(webstore_item_id) ==

[lazyboy, 2016/06/29 21:03:03]

can use install_observers_.count(webstore_item_id) == 0

[Devlin, 2016/06/29 22:32:09]

Done.

+                 install_observers_.end());
+      install_observers_[webstore_item_id] =
+          base::MakeUnique<InlineInstallObserver>(
+              this, web_contents()->GetBrowserContext(), return_route_id,
+              webstore_item_id, observe_download_progress,
+              observe_install_stage);
+    }
 
     WebstoreStandaloneInstaller::Callback callback =
         base::Bind(&TabHelper::OnInlineInstallComplete,
-                   base::Unretained(this),
+                   weak_ptr_factory_.GetWeakPtr(),
                    install_id,
-                   return_route_id);
+                   return_route_id,
+                   webstore_item_id);
     scoped_refptr<WebstoreInlineInstaller> installer(
         webstore_inline_installer_factory_->CreateInstaller(
             web_contents(), host, webstore_item_id, requestor_url, callback));
@@ -494,8 +593,8 @@ WindowController* TabHelper::GetExtensionWindowController() const  {
 
 void TabHelper::OnReenableComplete(int install_id,
                                    int return_route_id,
+                                   const std::string& extension_id,
                                    ExtensionReenabler::ReenableResult result) {
-  extension_reenabler_.reset();
   // Map the re-enable results to webstore-install results.
   webstore_install::Result webstore_result = webstore_install::SUCCESS;
   std::string error;
@@ -518,16 +617,24 @@ void TabHelper::OnReenableComplete(int install_id,
 
   OnInlineInstallComplete(install_id,
                           return_route_id,
+                          extension_id,
                           result == ExtensionReenabler::REENABLE_SUCCESS,
                           error,
                           webstore_result);
+  // Note: ExtensionReenabler contained the callback with the curried-in
+  // extension id; delete it last.

[lazyboy, 2016/06/29 21:03:03]

|extension_id|?

[Devlin, 2016/06/29 22:32:09]

Done.

+  extension_reenabler_.reset();
 }
 
 void TabHelper::OnInlineInstallComplete(int install_id,
                                         int return_route_id,
+                                        const std::string& extension_id,
                                         bool success,
                                         const std::string& error,
                                         webstore_install::Result result) {
+  DCHECK_EQ(1u, pending_inline_installations_.count(extension_id));
+  pending_inline_installations_.erase(extension_id);
+  install_observers_.erase(extension_id);
   Send(new ExtensionMsg_InlineWebstoreInstallResponse(
       return_route_id,
       install_id,