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

chrome/browser/resources/options/clear_browser_data_overlay.js

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 cr.define('options', function() {
   var OptionsPage = options.OptionsPage;
 
   /**
    * ClearBrowserDataOverlay class
    * Encapsulated handling of the 'Clear Browser Data' overlay page.
@@ skipping 13 matching lines @@
 
     /**
      * Whether deleting history and downloads is allowed.
      * @type {boolean}
      * @private
      */
     allowDeletingHistory_: true,
 
     /**
      * 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.
      */
     initializePage: 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',
                    'browser.clear_data.cookies',
                    'browser.clear_data.passwords',
                    'browser.clear_data.form_data',
                    'browser.clear_data.hosted_apps_data',
                    'browser.clear_data.content_licenses'];
       types.forEach(function(type) {
           Preferences.getInstance().addEventListener(type, f);
       });
 
       var checkboxes = document.querySelectorAll(
           '#cbd-content-area input[type=checkbox]');
       for (var i = 0; i < checkboxes.length; i++) {
         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) {
         ClearBrowserDataOverlay.dismiss();
       };
       $('clear-browser-data-commit').onclick = function(event) {
         ClearBrowserDataOverlay.setClearing(true);
         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_();
     },
 
     /**
      * Create a footer that explains that some content is not cleared by the
      * clear browsing history dialog.
      */
     createStuffRemainsFooter_: function() {
       // The localized string is of the form "Saved [content settings] and
       // {search engines} will not be cleared and may reflect your browsing
       // habits.". The following parses out the parts in brackts and braces and
@@ skipping 33 matching lines @@
           function(event) {
         OptionsPage.navigateToPage('content');
       }
       $('open-search-engines-from-clear-browsing-data').onclick =
           function(event) {
         OptionsPage.navigateToPage('searchEngines');
       }
     },
 
     /**
-     * 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;
       for (var i = 0; i < checkboxes.length; i++) {
         if (checkboxes[i].checked) {
           isChecked = true;
           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_;
-    },
+          !isChecked || (this.isClearingInProgress_ !== false);
 
-    setAllowDeletingHistory: function(allowed) {
-      this.allowDeletingHistory_ = 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_;
+      $('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;
+      }
 
-    showDeleteHistoryCheckboxes_: function(show) {
-      if (!show) {
-        $('delete-browsing-history-container').hidden = true;
-        $('delete-download-history-container').hidden = true;
-      }
-    },
-
-    /** @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);
   };
 
   ClearBrowserDataOverlay.setClearing = function(clearing) {
     ClearBrowserDataOverlay.getInstance().setClearing(clearing);
   };
 
   ClearBrowserDataOverlay.setBannerVisibility = function(args) {
     var visible = args[0];
     $('clear-browser-data-info-banner').hidden = !visible;
   };
 
   ClearBrowserDataOverlay.doneClearing = function() {
     // The delay gives the user some feedback that the clearing
     // 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);
   };
 
   ClearBrowserDataOverlay.dismiss = function() {
     var topmostVisiblePage = OptionsPage.getTopmostVisiblePage();
     if (topmostVisiblePage && topmostVisiblePage.name == 'clearBrowserData')
       OptionsPage.closeOverlay();
-    this.setClearing(false);
   };
 
   // Export
   return {
     ClearBrowserDataOverlay: ClearBrowserDataOverlay
   };
 });