Fix initial focus, and the way elements are disabled on the 'Clear browsing data' dialog.

chrome/browser/resources/options/clear_browser_data_overlay.js

Index: chrome/browser/resources/options/clear_browser_data_overlay.js
diff --git a/chrome/browser/resources/options/clear_browser_data_overlay.js b/chrome/browser/resources/options/clear_browser_data_overlay.js
index 75c712a7f4d3d02321f66d4068a21318ca1ce755..56a9cc035d7e47859da8d0fb59ef3708f5d4930c 100644
--- a/chrome/browser/resources/options/clear_browser_data_overlay.js
+++ b/chrome/browser/resources/options/clear_browser_data_overlay.js
@@ -34,7 +34,18 @@ cr.define('options', function() {
      * @type {boolean}
      * @private
      */
-    isClearingInProgress_: true,
+    isClearingInProgress_: false,
+
+    /**
+     * Whether or not the WebUI handler have completed initialization.
+     *
+     * Unless this becomes true, it must be assumed that the above flags might
+     * not contain the authoritative values.
+     *
+     * @type {boolean}
+     * @private
+     */
+    isInitializationComplete_: false,
 
     /**
      * Initialize the page.
@@ -43,7 +54,7 @@ cr.define('options', function() {
       // Call base class implementation to starts preference initialization.
       OptionsPage.prototype.initializePage.call(this);
 
-      var f = this.updateCommitButtonState_.bind(this);
+      var f = this.updateStateOfControls_.bind(this);
       var types = ['browser.clear_data.browsing_history',
                    'browser.clear_data.download_history',
                    'browser.clear_data.cache',
@@ -62,12 +73,6 @@ cr.define('options', function() {
         checkboxes[i].onclick = f;
       }
 
-      // At this point, assume that we are currently in the process of clearing
-      // data, so as to prevent the controls from being hazardously enabled for
-      // a very short time before ClearBrowserDataOverlay.setClearing() is
-      // called by the native side with the authoritative state.
-      this.setClearing(true);
-
       this.createStuffRemainsFooter_();
 
       $('clear-browser-data-dismiss').onclick = function(event) {
@@ -78,8 +83,15 @@ cr.define('options', function() {
         chrome.send('performClearBrowserData');
       };
 
-      var show = loadTimeData.getBoolean('showDeleteBrowsingHistoryCheckboxes');
-      this.showDeleteHistoryCheckboxes_(show);
+      // For managed profiles, hide the checkboxes controlling whether or not
+      // browsing and download history should be cleared. Note that this is
+      // different than just disabling them as a result of enterprise policies.
+      if (!loadTimeData.getBoolean('showDeleteBrowsingHistoryCheckboxes')) {
+        $('delete-browsing-history-container').hidden = true;
+        $('delete-download-history-container').hidden = true;
+      }
+
+      this.updateStateOfControls_();
     },
 
     /**
@@ -133,85 +145,102 @@ cr.define('options', function() {
     },
 
     /**
-     * Sets the enabled state of the checkboxes and buttons based on whether or
-     * not we are in the process of clearing data.
-     * @param {boolean} clearing Whether the browsing history is currently
-     *     being cleared.
+     * Sets whether or not we are in the process of clearing data.
+     * @param {boolean} clearing Whether the browsing data is currently being
+     *     cleared.
+     * @private
      */
-    setClearing: function(clearing) {
-      $('delete-browsing-history-checkbox').disabled = clearing;
-      $('delete-download-history-checkbox').disabled = clearing;
-      $('delete-cache-checkbox').disabled = clearing;
-      $('delete-cookies-checkbox').disabled = clearing;
-      $('delete-passwords-checkbox').disabled = clearing;
-      $('delete-form-data-checkbox').disabled = clearing;
-      $('delete-hosted-apps-data-checkbox').disabled = clearing;
-      $('deauthorize-content-licenses-checkbox').disabled = clearing;
-      $('clear-browser-data-time-period').disabled = clearing;
-      $('cbd-throbber').style.visibility = clearing ? 'visible' : 'hidden';
-      $('clear-browser-data-dismiss').disabled = clearing;
-
-      // The enabled state of the commit button is further based on whether or
-      // not any of the check boxes are checked.
+    setClearing_: function(clearing) {
       this.isClearingInProgress_ = clearing;
-      this.updateCommitButtonState_();
+      this.updateStateOfControls_();
     },
 
     /**
-     * Sets the enabled state of the commit button.
+     * Sets whether deleting history and downloads is disallowed by enterprise
+     * policies. This is called on initialization and in response to a change in
+     * the corresponding preference.
+     * @param {boolean} allowed Whether to allow deleting history and downloads.
+     * @private
      */
-    updateCommitButtonState_: function() {
-      var checkboxes = document.querySelectorAll(
-          '#cbd-content-area input[type=checkbox]');
-      var isChecked = false;
-      for (var i = 0; i < checkboxes.length; i++) {
-        if (checkboxes[i].checked) {
-          isChecked = true;
-          break;
-        }
-      }
-      $('clear-browser-data-commit').disabled =
-          !isChecked || this.isClearingInProgress_;
+    setAllowDeletingHistory_: function(allowed) {
+      this.allowDeletingHistory_ = allowed;
+      this.updateStateOfControls_();
     },
 
-    setAllowDeletingHistory: function(allowed) {
-      this.allowDeletingHistory_ = allowed;
+    /**
+     * Called by the WebUI handler to signal that it has finished calling all
+     * initialization methods.
+     * @private
+     */
+    markInitializationComplete_: function() {
+      this.isInitializationComplete_ = true;
+      this.updateStateOfControls_();
     },
 
-    showDeleteHistoryCheckboxes_: function(show) {
-      if (!show) {
-        $('delete-browsing-history-container').hidden = true;
-        $('delete-download-history-container').hidden = true;
+    /**
+     * Updates the enabled/disabled/hidden status of all controls on the dialog.
+     * @private
+     */
+    updateStateOfControls_: function() {
+      // The commit button is enabled if at least one data type selected to be
+      // cleared, and if we are not already in the process of clearing.
+      // To prevent the commit button from being hazardously enabled for a very
+      // short time before setClearing() is called the first time by the native
+      // side, also disable the button if |isInitializationComplete_| is false.
+      if (this.isClearingInProgress_ || !this.isInitializationComplete) {

[Dan Beam, 2014/05/19 21:41:16]

isInitializationComplete -> isInitializationComplete_

[engedy, 2014/05/19 23:19:22]

Done. Thanks, sorry, working through SSH...

+        $('clear-browser-data-commit').disabled = true;
+      } else {
+        var checkboxes = document.querySelectorAll(
+            '#cbd-content-area input[type=checkbox]');
+        var isChecked = false;
+        for (var i = 0; i < checkboxes.length; i++) {
+          if (checkboxes[i].checked) {
+            isChecked = true;

[Dan Beam, 2014/05/19 21:41:16]

just remove isCheck and inline

  $('clear-browser-data-commit').disabled = true;

here, IMO

[Dan Beam, 2014/05/19 21:41:34]

isChecked**

[engedy, 2014/05/19 23:19:22]

In that case we would need a "disabled = false" call in the beginning. Are
visual styles possibly multiple times *during* running the JS, or only once
afterwards? (I do not want to introduce flickering by this.)

[engedy, 2014/05/19 23:20:20]

... Are visual styles *applied* possibly...

On 2014/05/19 23:19:22, engedy wrote:

[Dan Beam, 2014/05/20 00:07:49]

that seems highly unlikely but here's an alternative:

var enabled = this.isInitializationComplete_ && !this.isClearingInProgress_;
if (enabled) {
  var checkboxes = document.querySelectorAll(
      '#cbd-content-area input[type=checkbox]');
  for (var i = 0; i < checkboxes.length; i++) {
    if (checkboxes[i].checked)
      break;
  }
  // Disable the checkbox if there's nothing to clear.
  enabled = i == checkboxes.length;
}
$('clear-browser-data-commit').disabled = !enabled;

[engedy, 2014/05/20 01:02:33]

Done something along these lines.

+            break;
+          }
+        }
+        $('clear-browser-data-commit').disabled = !isChecked;
       }
-    },
 
-    /** @override */
-    didShowPage: function() {
-      var allowed = ClearBrowserDataOverlay.getInstance().allowDeletingHistory_;
-      ClearBrowserDataOverlay.updateHistoryCheckboxes(allowed);
-    },
+      // The checkboxes for clearing history/downloads are enabled unless they
+      // are disallowed by policies, or we are in the process of clearing data.
+      // To prevent flickering, these, and the rest of the controls can safely
+      // be enabled for a short time before the first call to setClearing().
+      var enabled = this.allowDeletingHistory_ && !this.isClearingInProgress_;

[Dan Beam, 2014/05/19 21:41:16]

don't we need to check isInitializationComplete_ here?

[engedy, 2014/05/19 23:19:22]

To avoid all the controls becoming disabled then enabled when the dialog is
first shown (looks really ugly), I would just leave them enabled in the primary
use case. There is no harm even if the user can toggle their state.

Should I add more detail to the comment above?

[Dan Beam, 2014/05/20 00:07:49]

can we make them readonly instead of disabled in this case?

[engedy, 2014/05/20 01:02:33]

As discussed off-line, there is no risk in leaving them enabled for the initial
transient period.

+      $('delete-browsing-history-checkbox').disabled = !enabled;
+      $('delete-download-history-checkbox').disabled = !enabled;
+      if (!this.allowDeletingHistory_) {
+        $('delete-browsing-history-checkbox').checked = false;
+        $('delete-download-history-checkbox').checked = false;
+      }
+
+      // Enable everything else unless we are in the process of clearing.
+      var clearing = this.isClearingInProgress_;
+      $('delete-cache-checkbox').disabled = clearing;
+      $('delete-cookies-checkbox').disabled = clearing;
+      $('delete-passwords-checkbox').disabled = clearing;
+      $('delete-form-data-checkbox').disabled = clearing;
+      $('delete-hosted-apps-data-checkbox').disabled = clearing;
+      $('deauthorize-content-licenses-checkbox').disabled = clearing;
+      $('clear-browser-data-time-period').disabled = clearing;
+      $('cbd-throbber').style.visibility = clearing ? 'visible' : 'hidden';
+      $('clear-browser-data-dismiss').disabled = clearing;
+    }
   };
 
   //
   // Chrome callbacks
   //
-  /**
-   * Updates the disabled status of the browsing-history and downloads
-   * checkboxes, also unchecking them if they are disabled. This is called in
-   * response to a change in the corresponding preference.
-   */
-  ClearBrowserDataOverlay.updateHistoryCheckboxes = function(allowed) {
-    $('delete-browsing-history-checkbox').disabled = !allowed;
-    $('delete-download-history-checkbox').disabled = !allowed;
-    if (!allowed) {
-      $('delete-browsing-history-checkbox').checked = false;
-      $('delete-download-history-checkbox').checked = false;
-    }
-    ClearBrowserDataOverlay.getInstance().setAllowDeletingHistory(allowed);
+  ClearBrowserDataOverlay.setAllowDeletingHistory = function(allowed) {
+    ClearBrowserDataOverlay.getInstance().setAllowDeletingHistory_(allowed);
   };
 
   ClearBrowserDataOverlay.setClearing = function(clearing) {
-    ClearBrowserDataOverlay.getInstance().setClearing(clearing);
+    ClearBrowserDataOverlay.getInstance().setClearing_(clearing);
+  };
+
+  ClearBrowserDataOverlay.markInitializationComplete = function() {
+    ClearBrowserDataOverlay.getInstance().markInitializationComplete_();
   };
 
   ClearBrowserDataOverlay.setBannerVisibility = function(args) {
@@ -224,6 +253,7 @@ cr.define('options', function() {
     // actually worked. Otherwise the dialog just vanishes instantly in most
     // cases.
     window.setTimeout(function() {
+      ClearBrowserDataOverlay.setClearing(false);
       ClearBrowserDataOverlay.dismiss();
     }, 200);
   };
@@ -232,7 +262,6 @@ cr.define('options', function() {
     var topmostVisiblePage = OptionsPage.getTopmostVisiblePage();
     if (topmostVisiblePage && topmostVisiblePage.name == 'clearBrowserData')
       OptionsPage.closeOverlay();
-    this.setClearing(false);
   };
 
   // Export