Print Preview: Simplify signin

Print Preview Handler was only observing the identity manager if
mirror was enabled. However, mirror is now generally disabled, so Print
Preview never actually received any notifications about cookie jar
changes. This CL changes PrintPreviewHandler to always add this observer
unless cloud print is disabled (in which case Print Preview does not
need login information).

With the observer, the callback after sign in completion is no longer
necessary. If the user signs in to cloud print successfully,
IdentityManager::Observer::OnAccountToCookieCompleted() will be called.
The reload-printer-list event fired from this function triggers a call
to onDestinationsReload(), which is the same call that happens after
the sign in promise is resolved.

The only expected behavior change from this CL is that if the
destinations dialog is open when a user signs in or out in a different
tab, the destinations should refresh.

Bug: 932692, 958767
Change-Id: Ibdabf382f4eb8f6c438376b2f2415c27f4e9c586
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1626933Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Rebekah Potter <rbpotter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#663859}

chrome/browser/printing/print_dialog_cloud.cc

@@ -4,7 +4,6 @@
 
 #include "chrome/browser/printing/print_dialog_cloud.h"
 
-#include "base/bind.h"
 #include "base/macros.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/browser_process.h"
@@ -28,12 +27,8 @@ namespace {
 
 class SignInObserver : public content::WebContentsObserver {
  public:
-  SignInObserver(content::WebContents* web_contents,
-                 const base::Closure& callback)
-      : WebContentsObserver(web_contents),
-        callback_(callback),
-        weak_ptr_factory_(this) {
-  }
+  explicit SignInObserver(content::WebContents* web_contents)
+      : WebContentsObserver(web_contents), weak_ptr_factory_(this) {}
 
  private:
   // Overridden from content::WebContentsObserver:
@@ -54,13 +49,10 @@ class SignInObserver : public content::WebContentsObserver {
   void WebContentsDestroyed() override { delete this; }
 
   void OnSignIn() {
-    callback_.Run();
     if (web_contents())
       web_contents()->Close();
   }
 
-  GURL cloud_print_url_;
-  base::Closure callback_;
   base::WeakPtrFactory<SignInObserver> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(SignInObserver);
@@ -68,9 +60,7 @@ class SignInObserver : public content::WebContentsObserver {
 
 }  // namespace
 
-void CreateCloudPrintSigninTab(Browser* browser,
-                               bool add_account,
-                               const base::Closure& callback) {
+void CreateCloudPrintSigninTab(Browser* browser, bool add_account) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (AccountConsistencyModeManager::IsMirrorEnabledForProfile(
           browser->profile())) {
@@ -88,7 +78,8 @@ void CreateCloudPrintSigninTab(Browser* browser,
                 url, g_browser_process->GetApplicationLocale()),
             content::Referrer(), WindowOpenDisposition::NEW_FOREGROUND_TAB,
             ui::PAGE_TRANSITION_AUTO_BOOKMARK, false));
-    new SignInObserver(web_contents, callback);
+    // This observer will delete itself after destroying the WebContents.
+    new SignInObserver(web_contents);
   }
 }
 

chrome/browser/printing/print_dialog_cloud.h

@@ -7,8 +7,6 @@
 
 #include <string>
 
-#include "base/callback.h"
-
 class Browser;
 class Profile;
 
@@ -20,10 +18,7 @@ namespace print_dialog_cloud {
 
 // Creates a tab with Google 'sign in' or 'add account' page, based on
 // passed |add_account| value.
-// Calls |callback| when complete.
-void CreateCloudPrintSigninTab(Browser* browser,
-                               bool add_account,
-                               const base::Closure& callback);
+void CreateCloudPrintSigninTab(Browser* browser, bool add_account);
 
 // Parse switches from command_line and display the print dialog as appropriate.
 bool CreatePrintDialogFromCommandLine(Profile* profile,

chrome/browser/resources/print_preview/native_layer.js

@@ -279,15 +279,13 @@ cr.define('print_preview', function() {
     }
 
     /**
-     * Opens the Google Cloud Print sign-in tab. The DESTINATIONS_RELOAD event
-     *     will be dispatched in response.
+     * Opens the Google Cloud Print sign-in tab. If the user signs in
+     * successfully, the user-accounts-updated event will be sent in response.
      * @param {boolean} addAccount Whether to open an 'add a new account' or
      *     default sign in page.
-     * @return {!Promise} Promise that resolves when the sign in tab has been
-     *     closed and the destinations should be reloaded.
      */
     signIn(addAccount) {
-      return cr.sendWithPromise('signIn', addAccount);
+      chrome.send('signIn', [addAccount]);
     }
 
     /**

chrome/browser/resources/print_preview/ui/destination_dialog.js

@@ -288,9 +288,7 @@ Polymer({
   onSignInClick_: function() {
     this.metrics_.record(
         print_preview.Metrics.DestinationSearchBucket.SIGNIN_TRIGGERED);
-    print_preview.NativeLayer.getInstance().signIn(false).then(() => {
-      this.destinationStore.onDestinationsReload();
-    });
+    print_preview.NativeLayer.getInstance().signIn(false);
   },
 
   /** @private */
@@ -380,9 +378,7 @@ Polymer({
       this.metrics_.record(
           print_preview.Metrics.DestinationSearchBucket.ACCOUNT_CHANGED);
     } else {
-      print_preview.NativeLayer.getInstance().signIn(true).then(
-          this.destinationStore.onDestinationsReload.bind(
-              this.destinationStore));
+      print_preview.NativeLayer.getInstance().signIn(true);
       const options = select.querySelectorAll('option');
       for (let i = 0; i < options.length; i++) {
         if (options[i].value == this.activeUser) {

chrome/browser/ui/webui/print_preview/print_preview_handler.cc

@@ -10,6 +10,7 @@
 #include <memory>
 #include <string>
 #include <utility>
+#include <vector>
 
 #include "base/base64.h"
 #include "base/bind.h"
@@ -855,22 +856,13 @@ void PrintPreviewHandler::HandlePrinterSetup(const base::ListValue* args) {
                                        printer_name));
 }
 
-void PrintPreviewHandler::OnSigninComplete(const std::string& callback_id) {
-  ResolveJavascriptCallback(base::Value(callback_id), base::Value());
-}
-
 void PrintPreviewHandler::HandleSignin(const base::ListValue* args) {
-  std::string callback_id;
   bool add_account = false;
-  CHECK(args->GetString(0, &callback_id));
-  CHECK(!callback_id.empty());
-  CHECK(args->GetBoolean(1, &add_account));
+  CHECK(args->GetBoolean(0, &add_account));
 
   chrome::ScopedTabbedBrowserDisplayer displayer(Profile::FromWebUI(web_ui()));
-  print_dialog_cloud::CreateCloudPrintSigninTab(
-      displayer.browser(), add_account,
-      base::Bind(&PrintPreviewHandler::OnSigninComplete,
-                 weak_factory_.GetWeakPtr(), callback_id));
+  print_dialog_cloud::CreateCloudPrintSigninTab(displayer.browser(),
+                                                add_account);
 }
 
 #if defined(OS_CHROMEOS)
@@ -1001,7 +993,7 @@ void PrintPreviewHandler::SendInitialSettings(
     initial_settings.SetBoolKey(kHeaderFooter,
                                 prefs->GetBoolean(prefs::kPrintHeaderFooter));
   }
-  if (prefs->GetBoolean(prefs::kCloudPrintSubmitEnabled) &&
+  if (IsCloudPrintEnabled() &&
       !base::FeatureList::IsEnabled(features::kCloudPrinterHandler)) {
     initial_settings.SetStringKey(
         kCloudPrintURL, GURL(cloud_devices::GetCloudPrintURL()).spec());
@@ -1024,7 +1016,7 @@ void PrintPreviewHandler::SendInitialSettings(
   }
 
   GetNumberFormatAndMeasurementSystem(&initial_settings);
-  if (prefs->GetBoolean(prefs::kCloudPrintSubmitEnabled)) {
+  if (IsCloudPrintEnabled()) {
     GetUserAccountList(&initial_settings);
   }
 
@@ -1113,8 +1105,6 @@ WebContents* PrintPreviewHandler::GetInitiator() const {
   return dialog_controller->GetInitiator(preview_web_contents());
 }
 
-// TODO(crbug.com/932692): Investigate the replacement or removal of
-// this override altogether.
 void PrintPreviewHandler::OnAccountsInCookieUpdated(
     const identity::AccountsInCookieJarInfo& accounts_in_cookie_jar_info,
     const GoogleServiceAuthError& error) {
@@ -1330,22 +1320,28 @@ void PrintPreviewHandler::OnPrintResult(const std::string& callback_id,
 
 void PrintPreviewHandler::RegisterForGaiaCookieChanges() {
   DCHECK(!identity_manager_);
+  cloud_print_enabled_ =
+      GetPrefs()->GetBoolean(prefs::kCloudPrintSubmitEnabled);
+
+  if (!cloud_print_enabled_)
+    return;
+
   Profile* profile = Profile::FromWebUI(web_ui());
   identity_manager_ = IdentityManagerFactory::GetForProfile(profile);
-  if (AccountConsistencyModeManager::IsMirrorEnabledForProfile(profile)) {
-    identity_manager_->AddObserver(this);
-  }
+  identity_manager_->AddObserver(this);
 }
 
 void PrintPreviewHandler::UnregisterForGaiaCookieChanges() {
   if (!identity_manager_)
     return;
 
-  Profile* profile = Profile::FromWebUI(web_ui());
-  if (AccountConsistencyModeManager::IsMirrorEnabledForProfile(profile)) {
-    identity_manager_->RemoveObserver(this);
-  }
+  identity_manager_->RemoveObserver(this);
   identity_manager_ = nullptr;
+  cloud_print_enabled_ = false;
+}
+
+bool PrintPreviewHandler::IsCloudPrintEnabled() {
+  return cloud_print_enabled_;
 }
 
 void PrintPreviewHandler::BadMessageReceived() {

chrome/browser/ui/webui/print_preview/print_preview_handler.h

@@ -124,6 +124,7 @@ class PrintPreviewHandler : public content::WebUIMessageHandler,
  protected:
   // Protected so unit tests can override.
   virtual PrinterHandler* GetPrinterHandler(PrinterType printer_type);
+  virtual bool IsCloudPrintEnabled();
 
   // Shuts down the initiator renderer. Called when a bad IPC message is
   // received.
@@ -220,11 +221,8 @@ class PrintPreviewHandler : public content::WebUIMessageHandler,
   void HandleShowSystemDialog(const base::ListValue* args);
 #endif
 
-  // Callback for the signin dialog to call once signin is complete.
-  void OnSigninComplete(const std::string& callback_id);
-
-  // Brings up a dialog to allow the user to sign into cloud print.
-  // |args| is unused.
+  // Opens a new tab to allow the user to sign into cloud print. |args| holds
+  // a boolean indicating whether the user is adding an account.
   void HandleSignin(const base::ListValue* args);
 
 #if defined(OS_CHROMEOS)
@@ -322,6 +320,9 @@ class PrintPreviewHandler : public content::WebUIMessageHandler,
   // Whether we have already logged the number of printers this session.
   bool has_logged_printers_count_;
 
+  // Whether Google Cloud Print is enabled for the active profile.
+  bool cloud_print_enabled_ = false;
+
   // The settings used for the most recent preview request.
   base::Value last_preview_settings_;
 

chrome/browser/ui/webui/print_preview/print_preview_handler_unittest.cc

@@ -209,6 +209,8 @@ class TestPrintPreviewHandler : public PrintPreviewHandler {
     return test_printer_handler_.get();
   }
 
+  bool IsCloudPrintEnabled() override { return true; }
+
   void RegisterForGaiaCookieChanges() override {}
   void UnregisterForGaiaCookieChanges() override {}
 

chrome/test/data/webui/print_preview/native_layer_stub.js

@@ -198,7 +198,7 @@ cr.define('print_preview', function() {
     /** @override */
     signIn(addAccount) {
       this.methodCalled('signIn', addAccount);
-      return Promise.resolve();
+      cr.webUIListenerCallback('reload-printer-list');
     }
 
     /**