Move SMB share error toast to dialog

This change moves the error toast to the dialog when mounting an SMB share. Previously the flow was: - Click add mount - Dialog closes to SMB Shares parent page - Error/success string is displayed on parent page Now the flow will be: - Click add mount - Dialog stays open but add button becomes disabled - Error/success string is displayed on dialog itself An added advantage of this change is that it improves user experience as it is easier for users to debug error cases when the dialog is still open and they can still see the information they entered. Bug: chromium:887135 Test: browser_tests --gtest_filter=CrSettingsSmb* Change-Id: I6f68cd71a0002372a45975b3d8602809b36a6754 Reviewed-on: https://chromium-review.googlesource.com/c/1316664 Commit-Queue: Bailey Berro <[email protected]> Reviewed-by: Scott Chen <[email protected]> Reviewed-by: Kyle Horimoto <[email protected]> Cr-Commit-Position: refs/heads/master@{#611336}
feiyue1206 · Nov 27, 2018 · 9c9c1d2 · 9c9c1d2
1 parent 0707735
commit 9c9c1d2
Show file tree

Hide file tree

Showing 9 changed files with 140 additions and 83 deletions.
diff --git a/chrome/browser/resources/settings/downloads_page/smb_shares_page.html b/chrome/browser/resources/settings/downloads_page/smb_shares_page.html
@@ -1,11 +1,9 @@
 <link rel="import" href="chrome://resources/html/polymer.html">
 
 <link rel="import" href="chrome://resources/cr_components/chromeos/smb_shares/add_smb_share_dialog.html">
-<link rel="import" href="chrome://resources/cr_elements/cr_toast/cr_toast.html">
 <link rel="import" href="chrome://resources/html/action_link.html">
 <link rel="import" href="chrome://resources/html/action_link_css.html">
 <link rel="import" href="chrome://resources/html/assert.html">
-<link rel="import" href="chrome://resources/html/web_ui_listener_behavior.html">
 <link rel="import" href="chrome://resources/polymer/v1_0/paper-button/paper-button.html">
 <link rel="import" href="chrome://resources/html/md_select_css.html">
 <link rel="import" href="../route.html">
@@ -43,12 +41,6 @@
           last-url="[[prefs.network_file_shares.most_recently_used_url.value]]"
       </add-smb-share-dialog>
     </template>
-
-    <cr-toast id="errorToast" duration="3000">
-      <div class="error-message" id="addShareDoneMessage">
-        [[addShareResultText_]]
-      </div>
-    </cr-toast>
   </template>
   <script src="smb_shares_page.js"></script>
 </dom-module>
diff --git a/chrome/browser/resources/settings/downloads_page/smb_shares_page.js b/chrome/browser/resources/settings/downloads_page/smb_shares_page.js
@@ -6,7 +6,6 @@ Polymer({
   is: 'settings-smb-shares-page',
 
   behaviors: [
-    WebUIListenerBehavior,
     settings.RouteObserverBehavior,
   ],
 
@@ -21,9 +20,6 @@ Polymer({
 
     /** @private */
     showAddSmbDialog_: Boolean,
-
-    /** @private */
-    addShareResultText_: String,
   },
 
   /**
@@ -38,11 +34,6 @@ Polymer({
     }
   },
 
-  /** @override */
-  attached: function() {
-    this.addWebUIListener('on-add-smb-share', this.onAddShare_.bind(this));
-  },
-
   /** @private */
   onAddShareTap_: function() {
     this.showAddSmbDialog_ = true;
@@ -52,42 +43,4 @@ Polymer({
   onAddSmbDialogClosed_: function() {
     this.showAddSmbDialog_ = false;
   },
-
-  /**
-   * @param {SmbMountResult} result
-   * @private
-   */
-  onAddShare_: function(result) {
-    switch (result) {
-      case SmbMountResult.SUCCESS:
-        this.addShareResultText_ =
-            loadTimeData.getString('smbShareAddedSuccessfulMessage');
-        break;
-      case SmbMountResult.AUTHENTICATION_FAILED:
-        this.addShareResultText_ =
-            loadTimeData.getString('smbShareAddedAuthFailedMessage');
-        break;
-      case SmbMountResult.NOT_FOUND:
-        this.addShareResultText_ =
-            loadTimeData.getString('smbShareAddedNotFoundMessage');
-        break;
-      case SmbMountResult.UNSUPPORTED_DEVICE:
-        this.addShareResultText_ =
-            loadTimeData.getString('smbShareAddedUnsupportedDeviceMessage');
-        break;
-      case SmbMountResult.MOUNT_EXISTS:
-        this.addShareResultText_ =
-            loadTimeData.getString('smbShareAddedMountExistsMessage');
-        break;
-      case SmbMountResult.INVALID_URL:
-        this.addShareResultText_ =
-            loadTimeData.getString('smbShareAddedInvalidURLMessage');
-        break;
-      default:
-        this.addShareResultText_ =
-            loadTimeData.getString('smbShareAddedErrorMessage');
-    }
-    this.$.errorToast.show();
-  },
-
 });
diff --git a/chrome/browser/ui/webui/chromeos/smb_shares/smb_handler.cc b/chrome/browser/ui/webui/chromeos/smb_shares/smb_handler.cc
@@ -48,17 +48,20 @@ void SmbHandler::RegisterMessages() {
 }
 
 void SmbHandler::HandleSmbMount(const base::ListValue* args) {
-  CHECK_EQ(5U, args->GetSize());
+  CHECK_EQ(6U, args->GetSize());
+  std::string callback_id;
+  CHECK(args->GetString(0, &callback_id));
+
   std::string mount_url;
   std::string mount_name;
   std::string username;
   std::string password;
   bool use_kerberos;
-  CHECK(args->GetString(0, &mount_url));
-  CHECK(args->GetString(1, &mount_name));
-  CHECK(args->GetString(2, &username));
-  CHECK(args->GetString(3, &password));
-  CHECK(args->GetBoolean(4, &use_kerberos));
+  CHECK(args->GetString(1, &mount_url));
+  CHECK(args->GetString(2, &mount_name));
+  CHECK(args->GetString(3, &username));
+  CHECK(args->GetString(4, &password));
+  CHECK(args->GetBoolean(5, &use_kerberos));
 
   smb_client::SmbService* const service = GetSmbService(profile_);
   if (!service) {
@@ -69,8 +72,9 @@ void SmbHandler::HandleSmbMount(const base::ListValue* args) {
   mo.display_name = mount_name.empty() ? mount_url : mount_name;
   mo.writable = true;
 
-  auto mount_response = base::BindOnce(&SmbHandler::HandleSmbMountResponse,
-                                       weak_ptr_factory_.GetWeakPtr());
+  auto mount_response =
+      base::BindOnce(&SmbHandler::HandleSmbMountResponse,
+                     weak_ptr_factory_.GetWeakPtr(), callback_id);
   auto mount_call =
       base::BindOnce(&smb_client::SmbService::Mount, base::Unretained(service),
                      mo, base::FilePath(mount_url), username, password,
@@ -83,9 +87,11 @@ void SmbHandler::HandleSmbMount(const base::ListValue* args) {
   }
 }
 
-void SmbHandler::HandleSmbMountResponse(SmbMountResult result) {
+void SmbHandler::HandleSmbMountResponse(const std::string& callback_id,
+                                        SmbMountResult result) {
   AllowJavascript();
-  FireWebUIListener("on-add-smb-share", base::Value(static_cast<int>(result)));
+  ResolveJavascriptCallback(base::Value(callback_id),
+                            base::Value(static_cast<int>(result)));
 }
 
 void SmbHandler::HandleStartDiscovery(const base::ListValue* args) {

diff --git a/chrome/browser/ui/webui/chromeos/smb_shares/smb_handler.h b/chrome/browser/ui/webui/chromeos/smb_shares/smb_handler.h
@@ -35,7 +35,8 @@ class SmbHandler : public content::WebUIMessageHandler {
   void HandleStartDiscovery(const base::ListValue* args);
 
   // Callback handler for SmbMount.
-  void HandleSmbMountResponse(SmbMountResult result);
+  void HandleSmbMountResponse(const std::string& callback_id,
+                              SmbMountResult result);
 
   // Callback handler for StartDiscovery.
   void HandleGatherSharesResponse(

diff --git a/chrome/browser/ui/webui/chromeos/smb_shares/smb_shares_localized_strings_provider.cc b/chrome/browser/ui/webui/chromeos/smb_shares/smb_shares_localized_strings_provider.cc
@@ -27,6 +27,20 @@ void AddLocalizedStrings(content::WebUIDataSource* html_source) {
        IDS_SETTINGS_DOWNLOADS_ADD_SHARE_STANDARD_AUTHENTICATION},
       {"smbShareKerberosAuthentication",
        IDS_SETTINGS_DOWNLOADS_ADD_SHARE_KERBEROS_AUTHENTICATION},
+      {"smbShareAddedSuccessfulMessage",
+       IDS_SETTINGS_DOWNLOADS_SHARE_ADDED_SUCCESS_MESSAGE},
+      {"smbShareAddedErrorMessage",
+       IDS_SETTINGS_DOWNLOADS_SHARE_ADDED_ERROR_MESSAGE},
+      {"smbShareAddedAuthFailedMessage",
+       IDS_SETTINGS_DOWNLOADS_SHARE_ADDED_AUTH_FAILED_MESSAGE},
+      {"smbShareAddedNotFoundMessage",
+       IDS_SETTINGS_DOWNLOADS_SHARE_ADDED_NOT_FOUND_MESSAGE},
+      {"smbShareAddedUnsupportedDeviceMessage",
+       IDS_SETTINGS_DOWNLOADS_SHARE_ADDED_UNSUPPORTED_DEVICE_MESSAGE},
+      {"smbShareAddedMountExistsMessage",
+       IDS_SETTINGS_DOWNLOADS_SHARE_ADDED_MOUNT_EXISTS_MESSAGE},
+      {"smbShareAddedInvalidURLMessage",
+       IDS_SETTINGS_DOWNLOADS_SHARE_ADDED_MOUNT_INVALID_URL_MESSAGE},
   };
   for (const auto& entry : localized_strings)
     html_source->AddLocalizedString(entry.name, entry.id);

diff --git a/chrome/test/data/webui/settings/smb_shares_page_tests.js b/chrome/test/data/webui/settings/smb_shares_page_tests.js
@@ -15,6 +15,7 @@ class TestSmbBrowserProxy extends TestBrowserProxy {
   smbMount(smbUrl, smbName, username, password, authMethod) {
     this.methodCalled(
         'smbMount', [smbUrl, smbName, username, password, authMethod]);
+    return Promise.resolve(SmbMountResult.SUCCESS);
   }
 
   /** @override */
@@ -62,7 +63,7 @@ suite('AddSmbShareDialogTests', function() {
     expectTrue(!!url);
     url.value = 'smb://192.168.1.1/testshare';
 
-    const addButton = addDialog.$$('#actionButton');
+    const addButton = addDialog.$$('.action-button');
     expectTrue(!!addButton);
     expectFalse(addButton.disabled);
   });

diff --git a/ui/webui/resources/cr_components/chromeos/smb_shares/add_smb_share_dialog.html b/ui/webui/resources/cr_components/chromeos/smb_shares/add_smb_share_dialog.html
@@ -3,6 +3,7 @@
 <link rel="import" href="chrome://resources/cr_elements/cr_dialog/cr_dialog.html">
 <link rel="import" href="chrome://resources/cr_elements/cr_input/cr_input.html">
 <link rel="import" href="chrome://resources/cr_elements/cr_searchable_drop_down/cr_searchable_drop_down.html">
+<link rel="import" href="chrome://resources/cr_elements/cr_toast/cr_toast.html">
 <link rel="import" href="chrome://resources/cr_elements/paper_button_style_css.html">
 <link rel="import" href="chrome://resources/cr_elements/shared_vars_css.html">
 <link rel="import" href="chrome://resources/html/i18n_behavior.html">
@@ -40,6 +41,33 @@
       cr-searchable-drop-down {
         margin-bottom: var(--cr-form-field-bottom-spacing);
       }
+
+      cr-toast {
+        left: 0;
+        max-height: 78px;
+        max-width: 300px;
+        position: absolute;
+        right: 0;
+      }
+
+      paper-button {
+        position: relative;
+        top: 38px;
+      }
+
+      .error-message {
+        color: white;
+        font: 13px;
+        padding-bottom: 15px;
+        padding-top: 15px;
+        text-align: center;
+        white-space: normal;
+      }
+
+      [slot='button-container'] {
+        height: 78px;
+        justify-content: space-between;
+      }
     </style>
 
     <cr-dialog id="dialog">
@@ -78,13 +106,24 @@
         </div>
       </div>
       <div slot="button-container">
-        <paper-button class="cancel-button" on-click="cancel_" id="cancel">
-          [[i18n('cancel')]]
-        </paper-button>
-        <paper-button id="actionButton" class="action-button"
-            on-click="onAddButtonTap_" disabled="[[!canAddShare_(mountUrl_)]]">
-          [[i18n('add')]]
-        </paper-button>
+        <!-- Error toast -->
+        <div>
+          <cr-toast id="errorToast" duration="3000">
+            <div class="error-message">
+              [[addShareResultText_]]
+            </div>
+          </cr-toast>
+        </div>
+        <!-- Buttons -->
+        <div>
+          <paper-button class="cancel-button" on-click="cancel_" id="cancel">
+            [[i18n('cancel')]]
+          </paper-button>
+          <paper-button class="action-button" on-click="onAddButtonTap_"
+              disabled="[[!canAddShare_(mountUrl_, inProgress_)]]">
+            [[i18n('add')]]
+          </paper-button>
+        </div>
       </div>
     </cr-dialog>
   </template>

diff --git a/ui/webui/resources/cr_components/chromeos/smb_shares/add_smb_share_dialog.js b/ui/webui/resources/cr_components/chromeos/smb_shares/add_smb_share_dialog.js
@@ -65,6 +65,15 @@ Polymer({
             SmbAuthMethod.CREDENTIALS;
       },
     },
+
+    /** @private */
+    addShareResultText_: String,
+
+    /** @private */
+    inProgress_: {
+      type: Boolean,
+      value: false,
+    }
   },
 
   /** @private {?smb_shares.SmbBrowserProxy} */
@@ -91,10 +100,14 @@ Polymer({
 
   /** @private */
   onAddButtonTap_: function() {
-    this.browserProxy_.smbMount(
-        this.mountUrl_, this.mountName_.trim(), this.username_, this.password_,
-        this.authenticationMethod_);
-    this.$.dialog.close();
+    this.inProgress_ = true;
+    this.browserProxy_
+        .smbMount(
+            this.mountUrl_, this.mountName_.trim(), this.username_,
+            this.password_, this.authenticationMethod_)
+        .then(result => {
+          this.onAddShare_(result);
+        });
   },
 
   /** @private */
@@ -108,7 +121,7 @@ Polymer({
    * @private
    */
   canAddShare_: function() {
-    return !!this.mountUrl_;
+    return !!this.mountUrl_ && !this.inProgress_;
   },
 
   /**
@@ -126,4 +139,42 @@ Polymer({
   shouldShowCredentialUI_: function() {
     return this.authenticationMethod_ == SmbAuthMethod.CREDENTIALS;
   },
+
+  /**
+   * @param {SmbMountResult} result
+   * @private
+   */
+  onAddShare_: function(result) {
+    this.inProgress_ = false;
+    switch (result) {
+      case SmbMountResult.SUCCESS:
+        this.$.dialog.close();
+        break;
+      case SmbMountResult.AUTHENTICATION_FAILED:
+        this.addShareResultText_ =
+            loadTimeData.getString('smbShareAddedAuthFailedMessage');
+        break;
+      case SmbMountResult.NOT_FOUND:
+        this.addShareResultText_ =
+            loadTimeData.getString('smbShareAddedNotFoundMessage');
+        break;
+      case SmbMountResult.UNSUPPORTED_DEVICE:
+        this.addShareResultText_ =
+            loadTimeData.getString('smbShareAddedUnsupportedDeviceMessage');
+        break;
+      case SmbMountResult.MOUNT_EXISTS:
+        this.addShareResultText_ =
+            loadTimeData.getString('smbShareAddedMountExistsMessage');
+        break;
+      case SmbMountResult.INVALID_URL:
+        this.addShareResultText_ =
+            loadTimeData.getString('smbShareAddedInvalidURLMessage');
+        break;
+      default:
+        this.addShareResultText_ =
+            loadTimeData.getString('smbShareAddedErrorMessage');
+    }
+    this.$.errorToast.show();
+  },
+
 });
diff --git a/ui/webui/resources/cr_components/chromeos/smb_shares/smb_browser_proxy.js b/ui/webui/resources/cr_components/chromeos/smb_shares/smb_browser_proxy.js
@@ -38,6 +38,7 @@ cr.define('smb_shares', function() {
      * @param {string} username
      * @param {string} password
      * @param {string} authMethod
+     * @return {!Promise<SmbMountResult>}
      */
     smbMount(smbUrl, smbName, username, password, authMethod) {}
 
@@ -51,10 +52,9 @@ cr.define('smb_shares', function() {
   class SmbBrowserProxyImpl {
     /** @override */
     smbMount(smbUrl, smbName, username, password, authMethod) {
-      chrome.send('smbMount', [
-        smbUrl, smbName, username, password,
-        authMethod == SmbAuthMethod.KERBEROS
-      ]);
+      return cr.sendWithPromise(
+          'smbMount', smbUrl, smbName, username, password,
+          authMethod == SmbAuthMethod.KERBEROS);
     }
 
     /** @override */