Make sideloaded (externally installed) extensions display webstore info

chrome/browser/extensions/external_install_ui.cc

Index: chrome/browser/extensions/external_install_ui.cc
diff --git a/chrome/browser/extensions/external_install_ui.cc b/chrome/browser/extensions/external_install_ui.cc
index 7d4936b3d21229d3f784ce9c8769633b9ac4947d..06e19524902da525b2abc21437a31edf3d8c1222 100644
--- a/chrome/browser/extensions/external_install_ui.cc
+++ b/chrome/browser/extensions/external_install_ui.cc
@@ -19,6 +19,8 @@
 #include "chrome/browser/extensions/extension_install_ui.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_uninstall_dialog.h"
+#include "chrome/browser/extensions/webstore_data_fetcher.h"
+#include "chrome/browser/extensions/webstore_data_fetcher_delegate.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
@@ -56,11 +58,10 @@ static const int kMenuCommandId = IDC_EXTERNAL_EXTENSION_ALERT;
 
 class ExternalInstallGlobalError;
 
-// TODO(mpcomplete): Get rid of the refcounting on this class, or document
-// why it's necessary. Will do after refactoring to merge back with
-// ExtensionDisabledDialogDelegate.
+// This class is refcounted to stay alive while we try and pull webstore data.
 class ExternalInstallDialogDelegate
     : public ExtensionInstallPrompt::Delegate,
+      public WebstoreDataFetcherDelegate,
       public base::RefCountedThreadSafe<ExternalInstallDialogDelegate> {
  public:
   ExternalInstallDialogDelegate(Browser* browser,
@@ -80,12 +81,27 @@ class ExternalInstallDialogDelegate
   virtual void InstallUIProceed() OVERRIDE;
   virtual void InstallUIAbort(bool user_initiated) OVERRIDE;
 
+  // WebstoreDataFetcherDelegate:
+  virtual void OnWebstoreRequestFailure() OVERRIDE;
+  virtual void OnWebstoreResponseParseSuccess(
+      scoped_ptr<base::DictionaryValue> webstore_data) OVERRIDE;
+  virtual void OnWebstoreResponseParseFailure(
+      const std::string& error) OVERRIDE;
+
+  // Show the install dialog to the user.
+  void ShowInstallUI();
+
   // The UI for showing the install dialog when enabling.
   scoped_ptr<ExtensionInstallPrompt> install_ui_;
+  scoped_ptr<ExtensionInstallPrompt::Prompt> prompt_;
 
   Browser* browser_;
   base::WeakPtr<ExtensionService> service_weak_;
-  const std::string extension_id_;
+  scoped_ptr<WebstoreDataFetcher> webstore_data_fetcher_;
+  const Extension* extension_;
+  bool use_global_error_;
+
+  DISALLOW_COPY_AND_ASSIGN(ExternalInstallDialogDelegate);
 };
 
 // Only shows a menu item, no bubble. Clicking the menu item shows
@@ -123,6 +139,9 @@ class ExternalInstallMenuAlert : public GlobalErrorWithStandardBubble,
   ExtensionService* service_;
   const Extension* extension_;
   content::NotificationRegistrar registrar_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ExternalInstallMenuAlert);
 };
 
 // Shows a menu item and a global error bubble, replacing the install dialog.
@@ -151,6 +170,9 @@ class ExternalInstallGlobalError : public ExternalInstallMenuAlert {
   // manually).
   ExternalInstallDialogDelegate* delegate_;
   const ExtensionInstallPrompt::Prompt* prompt_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ExternalInstallGlobalError);
 };
 
 static void CreateExternalInstallGlobalError(
@@ -196,18 +218,73 @@ ExternalInstallDialogDelegate::ExternalInstallDialogDelegate(
     bool use_global_error)
     : browser_(browser),
       service_weak_(service->AsWeakPtr()),
-      extension_id_(extension->id()) {
+      extension_(extension),
+      use_global_error_(use_global_error) {
   AddRef();  // Balanced in Proceed or Abort.
 
+  prompt_.reset(new ExtensionInstallPrompt::Prompt(
+      ExtensionInstallPrompt::EXTERNAL_INSTALL_PROMPT));
+
+  // If we don't have a browser, we can't go to the webstore to fetch data.

[Finnur, 2014/01/24 11:38:31]

Does this ever happen?

[Devlin, 2014/01/24 18:10:24]

TL;DR: Yes, but usually only in tests.

It can, though rarely in practice.  We create this at line 502, and |browser|
can be NULL if we're on android (I don't know that we can ever actually get this
far on android, though - that might just be for compilation), or if
FindTabbedBrowser() returns NULL.  This happens in unittests, most notably those
for ExtensionService.  But I figured that, rather than crash, we should handle
it gracefully, on the off-chance it does happen in normal use.

[Finnur, 2014/01/27 13:41:04]

It is certainly not unheard of to have these things return NULL in tests. We
don't have extension support on Android, so we won't get as far as showing the
install dialog. I would therefore document that this only happens in tests.

[Devlin, 2014/01/27 18:15:21]

Done.

+  if (!browser) {
+    ShowInstallUI();
+    return;
+  }
+
+  webstore_data_fetcher_.reset(new WebstoreDataFetcher(
+      this,
+      browser->profile()->GetRequestContext(),
+      GURL::EmptyGURL(),
+      extension->id()));
+  webstore_data_fetcher_->Start();
+}
+
+void ExternalInstallDialogDelegate::OnWebstoreRequestFailure() {
+  ShowInstallUI();
+}
+
+void ExternalInstallDialogDelegate::OnWebstoreResponseParseSuccess(
+    scoped_ptr<base::DictionaryValue> webstore_data) {
+  std::string localized_user_count;
+  double average_rating;
+  int rating_count;
+  if (!webstore_data->GetString(kUsersKey, &localized_user_count) ||
+      !webstore_data->GetDouble(kAverageRatingKey, &average_rating) ||
+      !webstore_data->GetInteger(kRatingCountKey, &rating_count)) {
+    // If we don't get a valid webstore response, short circuit, and continue
+    // to show a prompt without webstore data.
+    ShowInstallUI();
+    return;
+  }
+
+  bool show_user_count = true;
+  webstore_data->GetBoolean(kShowUserCountKey, &show_user_count);
+
+  prompt_->SetWebstoreData(localized_user_count,
+                           show_user_count,
+                           average_rating,
+                           rating_count);
+
+  ShowInstallUI();
+}
+
+void ExternalInstallDialogDelegate::OnWebstoreResponseParseFailure(
+    const std::string& error) {
+  ShowInstallUI();
+}
+
+void ExternalInstallDialogDelegate::ShowInstallUI() {
   install_ui_.reset(
-      ExtensionInstallUI::CreateInstallPromptWithBrowser(browser));
+      ExtensionInstallUI::CreateInstallPromptWithBrowser(browser_));
 
   const ExtensionInstallPrompt::ShowDialogCallback callback =
-      use_global_error ?
-      base::Bind(&CreateExternalInstallGlobalError,
-                 service_weak_, extension_id_) :
-      ExtensionInstallPrompt::GetDefaultShowDialogCallback();
-  install_ui_->ConfirmExternalInstall(this, extension, callback);
+      use_global_error_ ?
+          base::Bind(&CreateExternalInstallGlobalError,
+                     service_weak_,
+                     extension_->id()) :
+          ExtensionInstallPrompt::GetDefaultShowDialogCallback();
+
+  install_ui_->ConfirmExternalInstall(this, extension_, callback, *prompt_);
 }
 
 ExternalInstallDialogDelegate::~ExternalInstallDialogDelegate() {
@@ -217,7 +294,7 @@ void ExternalInstallDialogDelegate::InstallUIProceed() {
   if (!service_weak_.get())
     return;
   const Extension* extension =
-      service_weak_->GetInstalledExtension(extension_id_);
+      service_weak_->GetInstalledExtension(extension_->id());
   if (!extension)
     return;
   service_weak_->GrantPermissionsAndEnableExtension(extension);
@@ -227,11 +304,7 @@ void ExternalInstallDialogDelegate::InstallUIProceed() {
 void ExternalInstallDialogDelegate::InstallUIAbort(bool user_initiated) {
   if (!service_weak_.get())
     return;
-  const Extension* extension =
-      service_weak_->GetInstalledExtension(extension_id_);
-  if (!extension)
-    return;

[Finnur, 2014/01/24 11:38:31]

Tread carefully here.
https://code.google.com/p/chromium/issues/detail?id=256640

I think there's a reason why we use the ID and not the extension pointer so I
don't think it is safe to simplify this. Yoyo can probably say for sure.

[Yoyo Zhou, 2014/01/24 23:12:57]

Yeah, I don't remember the exact scenario, but it's risky to assume the
Extension doesn't get reloaded, uninstalled, etc. particularly because there's
an arbitrary amount of time between the delegate getting created and the user
acting on it (Proceed or Abort).

[Devlin, 2014/01/27 18:15:21]

Done.

-  service_weak_->UninstallExtension(extension_id_, false, NULL);
+  service_weak_->UninstallExtension(extension_->id(), false, NULL);
   Release();
 }