Print Preview: Remove use-cloud-print event

This event can cause a race condition in release build tests, since the
real event can fire after the original instance of print-preview-app is
removed from the DOM and the new one has been attached. This can result
in multiple use-cloud-print events (one real, one test) in tests of
cloud print behavior. This race condition appears to be exposed by
waiting for the HTML Imports polyfill and element definition, which are
necessary changes for migrating Print Preview to use the polyfill.

There is also no need for a separate event now that Print Preview is
using cr.sendWithPromise to handle initial settings instead of relying
on chrome.send and multiple calls to CallJavascriptFunctionUnsafe, which
needed to be ordered on the C++ side. Instead, we can send the cloud
print URL in the initial settings if cloud print is enabled, and always
set the cloud print interface before initializing the DestinationStore
in JS to guarantee that autoselection of cloud printers works.

Bug: 925517
Change-Id: Ifa84ef832ee8d129ab25ab5bfdb0f33882d1b48a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1576120
Commit-Queue: Rebekah Potter <rbpotter@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#652743}

chrome/browser/resources/print_preview/native_layer.js

@@ -39,10 +39,11 @@ print_preview.LocalDestinationInfo;
  *   documentHasSelection: boolean,
  *   shouldPrintSelectionOnly: boolean,
  *   printerName: string,
- *   headerFooter: ?boolean,
+ *   headerFooter: (boolean | undefined),
  *   isHeaderFooterManaged: boolean,
  *   serializedAppStateStr: ?string,
  *   serializedDefaultDestinationSelectionRulesStr: ?string,
+ *   cloudPrintURL: (string | undefined)
  * }}
  * @see corresponding field name definitions in print_preview_handler.cc
  */

chrome/browser/resources/print_preview/new/app.js

@@ -116,8 +116,6 @@ Polymer({
   attached: function() {
     document.documentElement.classList.remove('loading');
     this.nativeLayer_ = print_preview.NativeLayer.getInstance();
-    this.addWebUIListener(
-        'use-cloud-print', this.onCloudPrintEnable_.bind(this));
     this.addWebUIListener('print-failed', this.onPrintFailed_.bind(this));
     this.addWebUIListener(
         'print-preset-options', this.onPrintPresetOptions_.bind(this));
@@ -249,6 +247,12 @@ Polymer({
       return;
     }
     this.whenReady_.then(() => {
+      // The cloud print interface should be initialized before initializing the
+      // sidebar, so that cloud printers can be selected automatically.
+      if (settings.cloudPrintURL) {
+        this.initializeCloudPrint_(
+            settings.cloudPrintURL, settings.isInAppKioskMode);
+      }
       this.$.documentInfo.init(
           settings.previewModifiable, settings.documentTitle,
           settings.documentHasSelection);
@@ -275,33 +279,23 @@ Polymer({
   },
 
   /**
-   * Called when Google Cloud Print integration is enabled by the
-   * PrintPreviewHandler.
-   * Fetches the user's cloud printers.
+   * Called when Google Cloud Print integration is enabled.
    * @param {string} cloudPrintUrl The URL to use for cloud print servers.
-   * @param {boolean} appKioskMode Whether to print automatically for kiosk
-   *     mode.
+   * @param {boolean} appKioskMode Whether the browser is in app kiosk mode.
    * @private
    */
-  onCloudPrintEnable_: function(cloudPrintUrl, appKioskMode) {
-    if (!this.whenReady_) {
-      // This element and its corresponding model were detached while waiting
-      // for the callback. This can happen in tests; return early.
-      return;
-    }
-    this.whenReady_.then(() => {
-      assert(!this.cloudPrintInterface_);
-      this.cloudPrintInterface_ = cloudprint.getCloudPrintInterface(
-          cloudPrintUrl, assert(this.nativeLayer_), appKioskMode);
-      this.tracker_.add(
-          assert(this.cloudPrintInterface_).getEventTarget(),
-          cloudprint.CloudPrintInterfaceEventType.SUBMIT_DONE,
-          this.close_.bind(this));
-      this.tracker_.add(
-          assert(this.cloudPrintInterface_).getEventTarget(),
-          cloudprint.CloudPrintInterfaceEventType.SUBMIT_FAILED,
-          this.onCloudPrintError_.bind(this, appKioskMode));
-    });
+  initializeCloudPrint_: function(cloudPrintUrl, appKioskMode) {
+    assert(!this.cloudPrintInterface_);
+    this.cloudPrintInterface_ = cloudprint.getCloudPrintInterface(
+        cloudPrintUrl, assert(this.nativeLayer_), appKioskMode);
+    this.tracker_.add(
+        assert(this.cloudPrintInterface_).getEventTarget(),
+        cloudprint.CloudPrintInterfaceEventType.SUBMIT_DONE,
+        this.close_.bind(this));
+    this.tracker_.add(
+        assert(this.cloudPrintInterface_).getEventTarget(),
+        cloudprint.CloudPrintInterfaceEventType.SUBMIT_FAILED,
+        this.onCloudPrintError_.bind(this, appKioskMode));
   },
 
   /** @private */

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

@@ -279,6 +279,8 @@ const char kHeaderFooter[] = "headerFooter";
 // Name of a dictionary field telling us whether the kPrintHeaderFooter pref is
 // managed by an enterprise policy.
 const char kIsHeaderFooterManaged[] = "isHeaderFooterManaged";
+// Name of a dictionary field holding the cloud print URL.
+const char kCloudPrintURL[] = "cloudPrintURL";
 
 // Get the print job settings dictionary from |json_str|.
 // Returns |base::Value()| on failure.
@@ -952,8 +954,6 @@ void PrintPreviewHandler::HandleGetInitialSettings(
 
   AllowJavascript();
 
-  // Send before SendInitialSettings() to allow cloud printer auto select.
-  SendCloudPrintEnabled();
   GetPrinterHandler(PrinterType::kLocalPrinter)
       ->GetDefaultPrinter(base::Bind(&PrintPreviewHandler::SendInitialSettings,
                                      weak_factory_.GetWeakPtr(), callback_id));
@@ -988,6 +988,11 @@ void PrintPreviewHandler::SendInitialSettings(
     initial_settings.SetBoolean(kHeaderFooter,
                                 prefs->GetBoolean(prefs::kPrintHeaderFooter));
   }
+  if (prefs->GetBoolean(prefs::kCloudPrintSubmitEnabled) &&
+      !base::FeatureList::IsEnabled(features::kCloudPrinterHandler)) {
+    initial_settings.SetString(kCloudPrintURL,
+                               GURL(cloud_devices::GetCloudPrintURL()).spec());
+  }
   initial_settings.SetBoolean(
       kIsHeaderFooterManaged,
       prefs->IsManagedPreference(prefs::kPrintHeaderFooter));
@@ -1067,17 +1072,6 @@ void PrintPreviewHandler::SendPrinterSetup(const std::string& callback_id,
   ResolveJavascriptCallback(base::Value(callback_id), response);
 }
 
-void PrintPreviewHandler::SendCloudPrintEnabled() {
-  PrefService* prefs = GetPrefs();
-  if (prefs->GetBoolean(prefs::kCloudPrintSubmitEnabled) &&
-      !base::FeatureList::IsEnabled(features::kCloudPrinterHandler)) {
-    FireWebUIListener(
-        "use-cloud-print",
-        base::Value(GURL(cloud_devices::GetCloudPrintURL()).spec()),
-        base::Value(chrome::IsRunningInForcedAppMode()));
-  }
-}
-
 void PrintPreviewHandler::SendCloudPrintJob(
     const std::string& callback_id,
     const base::RefCountedMemory* data) {

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

@@ -266,9 +266,6 @@ class PrintPreviewHandler : public content::WebUIMessageHandler,
                         const std::string& printer_name,
                         base::Value settings_info);
 
-  // Send whether cloud print integration should be enabled.
-  void SendCloudPrintEnabled();
-
   // Send the PDF data to the cloud to print.
   void SendCloudPrintJob(const std::string& callback_id,
                          const base::RefCountedMemory* data);

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

@@ -295,9 +295,8 @@ class PrintPreviewHandlerTest : public testing::Test {
         base::ListValue::From(base::Value::ToUniquePtrValue(std::move(args)));
     handler()->HandleGetInitialSettings(list_args.get());
 
-    // In response to get initial settings, the initial settings are sent back
-    // and a use-cloud-print event is dispatched.
-    ASSERT_EQ(2u, web_ui()->call_data().size());
+    // In response to get initial settings, the initial settings are sent back.
+    ASSERT_EQ(1u, web_ui()->call_data().size());
   }
 
   void AssertWebUIEventFired(const content::TestWebUI::CallData& data,
@@ -370,6 +369,9 @@ class PrintPreviewHandlerTest : public testing::Test {
     ASSERT_EQ(expected_header_footer.has_value(), !!header_footer);
     if (expected_header_footer.has_value())
       EXPECT_EQ(expected_header_footer.value(), header_footer->GetBool());
+
+    ASSERT_TRUE(
+        settings->FindKeyOfType("cloudPrintURL", base::Value::Type::STRING));
   }
 
   IPC::TestSink& initiator_sink() {
@@ -423,9 +425,6 @@ TEST_F(PrintPreviewHandlerTest, InitialSettingsSimple) {
   // Verify initial settings were sent.
   ValidateInitialSettings(*web_ui()->call_data().back(), kDummyPrinterName,
                           kDummyInitiatorName, {});
-
-  // Check that the use-cloud-print event got sent
-  AssertWebUIEventFired(*web_ui()->call_data().front(), "use-cloud-print");
 }
 
 TEST_F(PrintPreviewHandlerTest, InitialSettingsEnableHeaderFooter) {
@@ -464,9 +463,9 @@ TEST_F(PrintPreviewHandlerTest, GetPrinters) {
     handler()->HandleGetPrinters(list_args.get());
     EXPECT_TRUE(handler()->CalledOnlyForType(type));
 
-    // Start with 2 calls from initial settings, then add 2 more for each loop
+    // Start with 1 call from initial settings, then add 2 more for each loop
     // iteration (one for printers-added, and one for the response).
-    ASSERT_EQ(2u + 2 * (i + 1), web_ui()->call_data().size());
+    ASSERT_EQ(1u + 2 * (i + 1), web_ui()->call_data().size());
 
     // Validate printers-added
     const content::TestWebUI::CallData& add_data =
@@ -511,9 +510,9 @@ TEST_F(PrintPreviewHandlerTest, GetPrinterCapabilities) {
     handler()->HandleGetPrinterCapabilities(list_args.get());
     EXPECT_TRUE(handler()->CalledOnlyForType(type));
 
-    // Start with 2 calls from initial settings, then add 1 more for each loop
+    // Start with 1 call from initial settings, then add 1 more for each loop
     // iteration.
-    ASSERT_EQ(2u + (i + 1), web_ui()->call_data().size());
+    ASSERT_EQ(1u + (i + 1), web_ui()->call_data().size());
 
     // Verify that the printer capabilities promise was resolved correctly.
     const content::TestWebUI::CallData& data = *web_ui()->call_data().back();
@@ -541,9 +540,9 @@ TEST_F(PrintPreviewHandlerTest, GetPrinterCapabilities) {
     handler()->HandleGetPrinterCapabilities(list_args.get());
     EXPECT_TRUE(handler()->CalledOnlyForType(type));
 
-    // Start with 2 calls from initial settings plus base::size(kAllTypes) from
+    // Start with 1 call from initial settings plus base::size(kAllTypes) from
     // first loop, then add 1 more for each loop iteration.
-    ASSERT_EQ(2u + base::size(kAllTypes) + (i + 1),
+    ASSERT_EQ(1u + base::size(kAllTypes) + (i + 1),
               web_ui()->call_data().size());
 
     // Verify printer capabilities promise was rejected.
@@ -747,9 +746,9 @@ TEST_F(PrintPreviewHandlerFailingTest, GetPrinterCapabilities) {
     handler()->HandleGetPrinterCapabilities(list_args.get());
     EXPECT_TRUE(handler()->CalledOnlyForType(type));
 
-    // Start with 2 calls from initial settings, then add 1 more for each loop
+    // Start with 1 call from initial settings, then add 1 more for each loop
     // iteration.
-    ASSERT_EQ(2u + (i + 1), web_ui()->call_data().size());
+    ASSERT_EQ(1u + (i + 1), web_ui()->call_data().size());
 
     // Verify printer capabilities promise was rejected.
     const content::TestWebUI::CallData& data = *web_ui()->call_data().back();

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

@@ -96,12 +96,12 @@ cr.define('invalid_settings_browsertest', function() {
           print_preview.makeRecentDestination(printers[1]),
         ],
       });
+      initialSettings.cloudPrintURL = 'cloudprint URL';
       localDestinationInfos = [];
 
       loadTimeData.overrideValues({isEnterpriseManaged: false});
       createPage(true);
 
-      cr.webUIListenerCallback('use-cloud-print', 'cloudprint url', false);
       printers.forEach(printer => cloudPrintInterface.setPrinter(printer));
     }
 

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

@@ -36,7 +36,8 @@ cr.define('print_preview_app_test', function() {
       shouldPrintSelectionOnly: false,
       printerName: 'FooDevice',
       serializedAppStateStr: null,
-      serializedDefaultDestinationSelectionRulesStr: null
+      serializedDefaultDestinationSelectionRulesStr: null,
+      cloudPrintURL: 'cloudprint URL'
     };
 
     /** @override */
@@ -57,10 +58,7 @@ cr.define('print_preview_app_test', function() {
       document.body.appendChild(page);
       const previewArea = page.$.previewArea;
       pluginProxy.setLoadCallback(previewArea.onPluginLoad_.bind(previewArea));
-      return print_preview.Model.whenReady().then(() => {
-        cr.webUIListenerCallback('use-cloud-print', 'cloudprint url', false);
-        return nativeLayer.whenCalled('getPrinterCapabilities');
-      });
+      return nativeLayer.whenCalled('getPreview');
     });
 
     // Regression test for https://crbug.com/936029