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..b258148cb09b606f06d9770513f35fcd2278b364 100644
--- a/chrome/browser/resources/options/clear_browser_data_overlay.js
+++ b/chrome/browser/resources/options/clear_browser_data_overlay.js
@@ -31,10 +31,15 @@ cr.define('options', function() {
 
     /**
      * Whether or not clearing browsing data is currently in progress.
-     * @type {boolean}
+     *
+     * This is null until we get the authoritative value in the first call to
+     * setClearing(). In this intermediary state, the commit button is disabled,
+     * but everything else can still be enabled.
+     *
+     * @type {?boolean}
      * @private
      */
-    isClearingInProgress_: true,
+    isClearingInProgress_: null,

[Dan Beam, 2014/05/19 17:39:07]

overall I think it's pretty dubious to have a variable that could be null or
false...

[engedy, 2014/05/19 21:32:59]

I have introduces a third boolean that indicates whether or not we have already
finished initializing. WDYT?

 
     /**
      * Initialize the page.
@@ -43,7 +48,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 +67,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 +77,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;
+      }

[Dan Beam, 2014/05/19 17:39:07]

can this be in |updateStateOfControls_|?

[engedy, 2014/05/19 21:32:59]

I would prefer keeping this separate. This is a load time, permanent hiding of
these controls, which is quite different than how updateStateOfControls_()
operates.

+
+      this.updateStateOfControls_();
     },
 
     /**
@@ -133,34 +139,32 @@ 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.
      */
     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.
       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.
      */
-    updateCommitButtonState_: function() {
+    setAllowDeletingHistory: function(allowed) {

[Dan Beam, 2014/05/19 17:39:07]

nit: setAllowDeletingHistory_ + @private if only used in
|ClearBrowserDataOverlay.setAllowDeletingHistory|

[engedy, 2014/05/19 21:32:59]

Done.

+      this.allowDeletingHistory_ = allowed;
+      this.updateStateOfControls_();
+    },
+
+    /**
+     * Updates the enabled/disabled/hidden status of all controls on the dialog.

[Dan Beam, 2014/05/19 17:39:07]

@private

[engedy, 2014/05/19 21:32:59]

Done.

+     */
+    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.

[Dan Beam, 2014/05/19 17:39:07]

can we do the |isClearingInProgress_| check first?

[engedy, 2014/05/19 21:32:59]

Done.

       var checkboxes = document.querySelectorAll(
           '#cbd-content-area input[type=checkbox]');
       var isChecked = false;
@@ -170,43 +174,42 @@ cr.define('options', function() {
           break;
         }
       }
+      // 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 |isClearingInProgress_| is null.
       $('clear-browser-data-commit').disabled =
-          !isChecked || this.isClearingInProgress_;
-    },
-
-    setAllowDeletingHistory: function(allowed) {
-      this.allowDeletingHistory_ = allowed;
-    },
-
-    showDeleteHistoryCheckboxes_: function(show) {
-      if (!show) {
-        $('delete-browsing-history-container').hidden = true;
-        $('delete-download-history-container').hidden = true;
+          !isChecked || (this.isClearingInProgress_ !== false);
+
+      // 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_;
+      $('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;
       }
-    },
 
-    /** @override */
-    didShowPage: function() {
-      var allowed = ClearBrowserDataOverlay.getInstance().allowDeletingHistory_;
-      ClearBrowserDataOverlay.updateHistoryCheckboxes(allowed);
-    },
+      // 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.setAllowDeletingHistory = function(allowed) {
     ClearBrowserDataOverlay.getInstance().setAllowDeletingHistory(allowed);
   };
 
@@ -224,6 +227,7 @@ cr.define('options', function() {
     // actually worked. Otherwise the dialog just vanishes instantly in most
     // cases.
     window.setTimeout(function() {
+      ClearBrowserDataOverlay.getInstance().setClearing(false);

[Dan Beam, 2014/05/19 17:39:07]

nit: ClearBrowserDataOverlay.setClearing(false);

[engedy, 2014/05/19 21:32:59]

Done.

       ClearBrowserDataOverlay.dismiss();
     }, 200);
   };
@@ -232,7 +236,6 @@ cr.define('options', function() {
     var topmostVisiblePage = OptionsPage.getTopmostVisiblePage();
     if (topmostVisiblePage && topmostVisiblePage.name == 'clearBrowserData')
       OptionsPage.closeOverlay();
-    this.setClearing(false);
   };
 
   // Export