Enable sending Feedback from the Login screen.

This CL allows users to use the Feedback keyboard shortcut to launch the
Feedback UI and send Feedback. This also fixes an issue with the current
implementation of login feedback, where if we try to send feedback after
we have launched the app before and restarted Chrome, the UI would not
launch.

This is because on the first run, the background host is always created
just to get the types of events the app can handle. Once this is done,
we save this information in the extension registry and only load the
app once the event is actually dispatched. Since the existing code
was instead waiting for the host to load, it would never send the
event and wait forever.

Instead we're not checking if the extension has been added to the
registry. If it has, we can safely dispatch the event. If not, we instead
wait for the extension to load and then send feedback.

I've tested that this works now both for the first run and subsequent runs
of the app. The existing browser test should still be applicable since the
CL just changed the entry point for login feedback; did not add any
additional functionality.

R=rdevlin.cronin@chromium.org, xiyuan@chromium.org

Bug: 809715
Change-Id: Ib26cb879b15334c010bfed539bc83384c09d2a01
Reviewed-on: https://chromium-review.googlesource.com/953372
Commit-Queue: Rahul Chaturvedi <rkc@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541958}

chrome/browser/chromeos/login/ui/login_feedback.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/chromeos/login/ui/login_feedback.h"
 
+#include <utility>
 #include <vector>
 
 #include "base/bind.h"
@@ -22,9 +23,9 @@
 #include "extensions/browser/app_window/app_window.h"
 #include "extensions/browser/app_window/app_window_registry.h"
 #include "extensions/browser/extension_host.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/browser/extension_registry_observer.h"
 #include "extensions/browser/extension_system.h"
-#include "extensions/browser/process_manager.h"
-#include "extensions/browser/process_manager_observer.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/extension.h"
 #include "ui/aura/window.h"
@@ -43,83 +44,58 @@ extensions::ComponentLoader* GetComponentLoader(
   return extension_service->component_loader();
 }
 
-extensions::ProcessManager* GetProcessManager(
-    content::BrowserContext* context) {
-  return extensions::ProcessManager::Get(context);
-}
+}  // namespace
 
 // Ensures that the feedback extension is loaded on the signin profile and
 // invokes the callback when the extension is ready to use. Unload the
 // extension and delete itself when the extension's background page shuts down.
-class FeedbackExtensionLoader : public extensions::ProcessManagerObserver,
-                                public content::WebContentsObserver {
+class FeedbackExtensionLoader : public extensions::ExtensionRegistryObserver {
  public:
+  explicit FeedbackExtensionLoader(Profile* profile);
+  ~FeedbackExtensionLoader() override;
+
   // Loads the feedback extension on the given profile and invokes
   // |on_ready_callback| when it is ready.
-  static void Load(Profile* profile, const base::Closure& on_ready_callback);
+  void Load(base::OnceClosure on_ready_callback);
 
  private:
-  explicit FeedbackExtensionLoader(Profile* profile);
-  ~FeedbackExtensionLoader() override;
+  // extensions::ExtensionRegistryObserver overrides:
+  void OnExtensionLoaded(content::BrowserContext* browser_context,
+                         const extensions::Extension* extension) override;
 
-  void Initialize();
+  void RunOnReadyCallback();
 
-  void AddOnReadyCallback(const base::Closure& on_ready_callback);
-  void RunOnReadyCallbacks();
-
-  // extensions::ProcessManagerObserver
-  void OnBackgroundHostCreated(extensions::ExtensionHost* host) override;
-  void OnBackgroundHostClose(const std::string& extension_id) override;
+  Profile* const profile_;
 
-  // content::WebContentsObserver
-  void DocumentOnLoadCompletedInMainFrame() override;
+  // Extension registry for the login profile lives till the login profile is
+  // destructed. This will always happen after the signin screen web UI has
+  // been destructed (which destructs us in the process). Hence this pointer
+  // will outlive us.
+  extensions::ExtensionRegistry* extension_registry_;
 
-  Profile* const profile_;
-  std::vector<base::Closure> on_ready_callbacks_;
-  bool ready_ = false;
+  base::OnceClosure on_ready_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(FeedbackExtensionLoader);
 };
 
-// Current live instance of FeedbackExtensionLoader.
-FeedbackExtensionLoader* instance = nullptr;
-
-// static
-void FeedbackExtensionLoader::Load(Profile* profile,
-                                   const base::Closure& on_ready_callback) {
-  if (instance == nullptr) {
-    instance = new FeedbackExtensionLoader(profile);
-    instance->Initialize();
-  }
-
-  DCHECK_EQ(instance->profile_, profile);
-  DCHECK(!on_ready_callback.is_null());
-  instance->AddOnReadyCallback(on_ready_callback);
-}
-
 FeedbackExtensionLoader::FeedbackExtensionLoader(Profile* profile)
-    : profile_(profile) {}
+    : profile_(profile),
+      extension_registry_(extensions::ExtensionRegistry::Get(profile)) {}
 
 FeedbackExtensionLoader::~FeedbackExtensionLoader() {
-  DCHECK_EQ(instance, this);
-  instance = nullptr;
-
-  GetProcessManager(profile_)->RemoveObserver(this);
+  extension_registry_->RemoveObserver(this);
   GetComponentLoader(profile_)->Remove(extension_misc::kFeedbackExtensionId);
 }
 
-void FeedbackExtensionLoader::Initialize() {
-  extensions::ProcessManager* pm = GetProcessManager(profile_);
-  pm->AddObserver(this);
-  extensions::ExtensionHost* const host =
-      pm->GetBackgroundHostForExtension(extension_misc::kFeedbackExtensionId);
-  if (host) {
-    OnBackgroundHostCreated(host);
-    if (!host->host_contents()->IsLoading())
-      DocumentOnLoadCompletedInMainFrame();
+void FeedbackExtensionLoader::Load(base::OnceClosure on_ready_callback) {
+  DCHECK(!on_ready_callback.is_null());
+  on_ready_callback_ = std::move(on_ready_callback);
+  if (extension_registry_->enabled_extensions().Contains(
+          extension_misc::kFeedbackExtensionId)) {
+    RunOnReadyCallback();
     return;
   }
-
+  extension_registry_->AddObserver(this);
   extensions::ComponentLoader* component_loader = GetComponentLoader(profile_);
   if (!component_loader->Exists(extension_misc::kFeedbackExtensionId)) {
     component_loader->Add(IDR_FEEDBACK_MANIFEST,
@@ -127,40 +103,20 @@ void FeedbackExtensionLoader::Initialize() {
   }
 }
 
-void FeedbackExtensionLoader::AddOnReadyCallback(
-    const base::Closure& on_ready_callback) {
-  on_ready_callbacks_.push_back(on_ready_callback);
-  if (ready_)
-    RunOnReadyCallbacks();
-}
-
-void FeedbackExtensionLoader::RunOnReadyCallbacks() {
-  std::vector<base::Closure> callbacks;
-  callbacks.swap(on_ready_callbacks_);
-
-  for (const auto& callback : callbacks)
-    callback.Run();
+void FeedbackExtensionLoader::RunOnReadyCallback() {
+  DCHECK(!on_ready_callback_.is_null());
+  std::move(on_ready_callback_).Run();
+  extension_registry_->RemoveObserver(this);
 }
 
-void FeedbackExtensionLoader::OnBackgroundHostCreated(
-    extensions::ExtensionHost* host) {
-  if (host->extension_id() == extension_misc::kFeedbackExtensionId)
-    Observe(host->host_contents());
-}
-
-void FeedbackExtensionLoader::OnBackgroundHostClose(
-    const std::string& extension_id) {
-  if (extension_id == extension_misc::kFeedbackExtensionId)
-    delete this;
-}
-
-void FeedbackExtensionLoader::DocumentOnLoadCompletedInMainFrame() {
-  ready_ = true;
-  RunOnReadyCallbacks();
+void FeedbackExtensionLoader::OnExtensionLoaded(
+    content::BrowserContext* browser_context,
+    const extensions::Extension* extension) {
+  if (extension->id() == extension_misc::kFeedbackExtensionId) {
+    RunOnReadyCallback();
+  }
 }
 
-}  // namespace
-
 ////////////////////////////////////////////////////////////////////////////////
 // LoginFeedback::FeedbackWindowHandler
 
@@ -232,9 +188,9 @@ LoginFeedback::LoginFeedback(Profile* signin_profile)
 LoginFeedback::~LoginFeedback() {}
 
 void LoginFeedback::Request(const std::string& description,
-                            const base::Closure& finished_callback) {
+                            base::OnceClosure finished_callback) {
   description_ = description;
-  finished_callback_ = finished_callback;
+  finished_callback_ = std::move(finished_callback);
   feedback_window_handler_.reset(new FeedbackWindowHandler(this));
 
   // Do not call EnsureFeedbackUI() immediately. Otherwise, event listener is
@@ -242,9 +198,10 @@ void LoginFeedback::Request(const std::string& description,
   // EventRouter::DispatchEventWithLazyListener() which possibly causes a race
   // condition.
 
-  FeedbackExtensionLoader::Load(
-      profile_,
-      base::Bind(&LoginFeedback::EnsureFeedbackUI, weak_factory_.GetWeakPtr()));
+  feedback_extension_loader_ =
+      std::make_unique<FeedbackExtensionLoader>(profile_);
+  feedback_extension_loader_->Load(base::BindOnce(
+      &LoginFeedback::EnsureFeedbackUI, weak_factory_.GetWeakPtr()));
 }
 
 void LoginFeedback::EnsureFeedbackUI() {
@@ -268,7 +225,7 @@ void LoginFeedback::EnsureFeedbackUI() {
 
 void LoginFeedback::OnFeedbackFinished() {
   if (!finished_callback_.is_null())
-    finished_callback_.Run();
+    std::move(finished_callback_).Run();
 }
 
 }  // namespace chromeos

chrome/browser/chromeos/login/ui/login_feedback.h

@@ -16,6 +16,8 @@ class Profile;
 
 namespace chromeos {
 
+class FeedbackExtensionLoader;
+
 // Show the feedback UI to collect a feedback on the login screen. Note that
 // it dynamically loads/unloads the feedback extension on the signin profile.
 class LoginFeedback {
@@ -27,7 +29,7 @@ class LoginFeedback {
   // will be invoked when the feedback UI is closed, either cancel or send the
   // feedback.
   void Request(const std::string& description,
-               const base::Closure& finished_callback);
+               base::OnceClosure finished_callback);
 
  private:
   // Makes the feedback UI windows on top of login screen and watches when
@@ -42,10 +44,12 @@ class LoginFeedback {
 
   Profile* const profile_;
   std::string description_;
-  base::Closure finished_callback_;
+  base::OnceClosure finished_callback_;
 
   std::unique_ptr<FeedbackWindowHandler> feedback_window_handler_;
 
+  std::unique_ptr<FeedbackExtensionLoader> feedback_extension_loader_;
+
   base::WeakPtrFactory<LoginFeedback> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(LoginFeedback);

chrome/browser/chromeos/login/ui/webui_login_view.cc

@@ -90,6 +90,7 @@ const char kAccelNameAppLaunchBailout[] = "app_launch_bailout";
 const char kAccelNameAppLaunchNetworkConfig[] = "app_launch_network_config";
 const char kAccelNameBootstrappingSlave[] = "bootstrapping_slave";
 const char kAccelNameDemoMode[] = "demo_mode";
+const char kAccelSendFeedback[] = "send_feedback";
 
 // A class to change arrow key traversal behavior when it's alive.
 class ScopedArrowKeyTraversal {
@@ -179,6 +180,9 @@ WebUILoginView::WebUILoginView(const WebViewSettings& settings)
   accel_map_[ui::Accelerator(
       ui::VKEY_D, ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN)] = kAccelNameDemoMode;
 
+  accel_map_[ui::Accelerator(ui::VKEY_I, ui::EF_SHIFT_DOWN | ui::EF_ALT_DOWN)] =
+      kAccelSendFeedback;
+
   for (AccelMap::iterator i(accel_map_.begin()); i != accel_map_.end(); ++i) {
     if (!ash_util::IsRunningInMash()) {
       // To make reset accelerator work while system tray is open, register it

chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc

@@ -532,6 +532,7 @@ void SigninScreenHandler::RegisterMessages() {
               &SigninScreenHandler::HandleFirstIncorrectPasswordAttempt);
   AddCallback("maxIncorrectPasswordAttempts",
               &SigninScreenHandler::HandleMaxIncorrectPasswordAttempts);
+  AddCallback("sendFeedback", &SigninScreenHandler::HandleSendFeedback);
   AddCallback("sendFeedbackAndResyncUserData",
               &SigninScreenHandler::HandleSendFeedbackAndResyncUserData);
   AddCallback("setupDemoMode", &SigninScreenHandler::HandleSetupDemoMode);
@@ -1551,16 +1552,27 @@ void SigninScreenHandler::HandleMaxIncorrectPasswordAttempts(
   RecordReauthReason(account_id, ReauthReason::INCORRECT_PASSWORD_ENTERED);
 }
 
+void SigninScreenHandler::HandleSendFeedback() {
+  login_feedback_ =
+      std::make_unique<LoginFeedback>(Profile::FromWebUI(web_ui()));
+  login_feedback_->Request(
+      std::string(), base::BindOnce(&SigninScreenHandler::OnFeedbackFinished,
+                                    weak_factory_.GetWeakPtr()));
+}
+
 void SigninScreenHandler::HandleSendFeedbackAndResyncUserData() {
   const std::string description = base::StringPrintf(
       "Auto generated feedback for http://crbug.com/547857.\n"
       "(uniquifier:%s)",
       base::Int64ToString(base::Time::Now().ToInternalValue()).c_str());
 
-  login_feedback_.reset(new LoginFeedback(Profile::FromWebUI(web_ui())));
-  login_feedback_->Request(description,
-                           base::Bind(&SigninScreenHandler::OnFeedbackFinished,
-                                      weak_factory_.GetWeakPtr()));
+  login_feedback_ =
+      std::make_unique<LoginFeedback>(Profile::FromWebUI(web_ui()));
+  login_feedback_->Request(
+      description,
+      base::BindOnce(
+          &SigninScreenHandler::OnUnrecoverableCryptohomeFeedbackFinished,
+          weak_factory_.GetWeakPtr()));
 }
 
 void SigninScreenHandler::HandleRequestNewNoteAction(
@@ -1659,10 +1671,16 @@ void SigninScreenHandler::OnCapsLockChanged(bool enabled) {
 }
 
 void SigninScreenHandler::OnFeedbackFinished() {
+  login_feedback_.reset();
+}
+
+void SigninScreenHandler::OnUnrecoverableCryptohomeFeedbackFinished() {
   CallJS("login.UnrecoverableCryptohomeErrorScreen.resumeAfterFeedbackUI");
 
   // Recreate user's cryptohome after the feedback is attempted.
   HandleResyncUserData();
+
+  login_feedback_.reset();
 }
 
 void SigninScreenHandler::OnAllowedInputMethodsChanged() {

chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h

@@ -407,6 +407,7 @@ class SigninScreenHandler
   void HandleLogRemoveUserWarningShown();
   void HandleFirstIncorrectPasswordAttempt(const AccountId& account_id);
   void HandleMaxIncorrectPasswordAttempts(const AccountId& account_id);
+  void HandleSendFeedback();
   void HandleSendFeedbackAndResyncUserData();
   void HandleRequestNewNoteAction(const std::string& request_type);
   void HandleNewNoteLaunchAnimationDone();
@@ -453,6 +454,10 @@ class SigninScreenHandler
   // Callback invoked after the feedback is finished.
   void OnFeedbackFinished();
 
+  // Callback invoked after the feedback sent from the unrecoverable cryptohome
+  // page is finished.
+  void OnUnrecoverableCryptohomeFeedbackFinished();
+
   // Called when the cros property controlling allowed input methods changes.
   void OnAllowedInputMethodsChanged();
 

ui/login/display_manager.js

@@ -58,6 +58,7 @@
     'app_launch_network_config';
 /** @const */ var ACCELERATOR_BOOTSTRAPPING_SLAVE = "bootstrapping_slave";
 /** @const */ var ACCELERATOR_DEMO_MODE = "demo_mode";
+/** @const */ var ACCELERATOR_SEND_FEEDBACK = "send_feedback";
 
 /* Signin UI state constants. Used to control header bar UI. */
 /** @const */ var SIGNIN_UI_STATE = {
@@ -447,6 +448,8 @@ cr.define('cr.ui.login', function() {
             -1) {
           chrome.send('setupDemoMode');
         }
+      } else if (name == ACCELERATOR_SEND_FEEDBACK) {
+        chrome.send('sendFeedback');
       }
     },