Hide the "downloads" page policy disabled UI elements from supervised users

chrome/browser/ui/webui/downloads_ui_browsertest.js

Index: chrome/browser/ui/webui/downloads_ui_browsertest.js
diff --git a/chrome/browser/ui/webui/downloads_ui_browsertest.js b/chrome/browser/ui/webui/downloads_ui_browsertest.js
index bf0c464f1d906ea79a8da2743d96f3b3ed2039b4..954952f6480c091a969d1fa5b7255eb0ff2c683a 100644
--- a/chrome/browser/ui/webui/downloads_ui_browsertest.js
+++ b/chrome/browser/ui/webui/downloads_ui_browsertest.js
@@ -87,21 +87,21 @@ BaseDownloadsWebUITest.prototype = {
 TEST_F('BaseDownloadsWebUITest', 'deleteAllowed', function() {
   // "Clear all" should be a link.
   var clearAllHolder = document.querySelectorAll('#clear-all-holder > a');
-  expectEquals(clearAllHolder.length, 1);
+  expectEquals(1, clearAllHolder.length);
 
   // All the "Remove from list" items should be links.
   var removeLinks = document.querySelectorAll(
       '.controls > a.control-remove-link');
-  expectEquals(removeLinks.length, TOTAL_RESULT_COUNT);
+  expectEquals(TOTAL_RESULT_COUNT, removeLinks.length);
 
   // There should be no disabled text "links".
   var disabledLinks = document.querySelectorAll('.disabled-link');
-  expectEquals(disabledLinks.length, 0);
+  expectEquals(0, disabledLinks.length);
 
   // The model is updated synchronously, even though the actual back-end removal
   // (tested elsewhere) is asynchronous.
   clearAll();
-  expectEquals(downloads.size(), 0);
+  expectEquals(0, downloads.size());
 
   // TODO(pamg): Mock out the back-end calls, so we can also test removing a
   // single item.
@@ -122,37 +122,69 @@ DownloadsWebUIDeleteProhibitedTest.prototype = {
   testGenPreamble: function() {
     GEN('  SetDeleteAllowed(false);');
   },
+
+  /**
+   * Asserts the correctness of the state of the UI elements
+   * that delete the download history when deletion is prohibited.
+   * @param expectControlsHidden True if the controls to delete download history

[Bernhard Bauer, 2013/09/06 11:23:26]

Add the type?

[ibra, 2013/09/06 16:56:42]

Done.

+   *     are expected to be hidden and false otherwise.
+   */
+  deleteProhibitedTestHelper: function(expectControlsHidden) {

[Bernhard Bauer, 2013/09/06 11:23:26]

This is not the best name for the function; does it delete a prohibited test
helper? :)

+    // "Clear all" should not be a link.
+    var clearAllText = document.querySelectorAll(
+        '#clear-all-holder.disabled-link');
+    expectEquals(1, clearAllText.length);
+    expectEquals('SPAN', clearAllText[0].nodeName);
+    expectEquals(expectControlsHidden, clearAllText[0].hidden);
+
+    // There should be no "Clear all" link.
+    var clearAllLinks = document.querySelectorAll('clear-all-link');

[Bernhard Bauer, 2013/09/06 11:23:26]

This selects all <clear-all-link> elements, which is probably not what you want.

You should 
* use getElementsByClassName() instead of relying on the CSS selector syntax
* make the base test case above go through the same code paths, but test the
opposite condition. Maybe you could even reuse this method for the base case
too?

[ibra, 2013/09/06 14:18:46]

As far as I understand the above test goes through the opposite code paths.
 
I can use a single helper method but that may be somewhat messy since basically
we query different names for the CSS classes each case and assertions are not
perfectly the opposite, are you ok with that?

[Bernhard Bauer, 2013/09/06 15:12:36]

No, it doesn't. This test selects all <clear-all-link> elements (of which there
are none, because what it should do is select all elements with the
clear-all-link class), and the above one doesn't. If it would, it would not find
any in the case where deletion is allowed either, which would cause the test to
fail and alert the author that something is wrong.
My point is, we should not do different queries (see above for why).

+    expectEquals(0, clearAllLinks.length);
+
+    // All the "Remove from list" items should be text. Check only one, to avoid
+    // spam in case of failure.
+    var removeTexts = document.querySelectorAll('.controls > .disabled-link');
+    expectEquals(TOTAL_RESULT_COUNT, removeTexts.length);
+    expectEquals('SPAN', removeTexts[0].nodeName);
+    expectEquals(expectControlsHidden, removeTexts[0].hidden);
+
+    // There should be no "Remove from list" links.
+    var removeLinks = document.querySelectorAll('control-remove-link');
+    expectEquals(0, removeLinks.length);
+
+    // Attempting to remove items anyway should fail.
+    // The model would have been cleared synchronously, even though the actual
+    // back-end removal (also disabled, but tested elsewhere) is asynchronous.
+    clearAll();
+    expectEquals(TOTAL_RESULT_COUNT, downloads.size());
+  }
 };
 
 // Test UI when removing entries is prohibited.
 TEST_F('DownloadsWebUIDeleteProhibitedTest', 'deleteProhibited', function() {
-  // "Clear all" should not be a link.
-  var clearAllText = document.querySelectorAll(
-      '#clear-all-holder.disabled-link');
-  expectEquals(clearAllText.length, 1);
-  expectEquals(clearAllText[0].nodeName, 'SPAN');
-
-  // There should be no "Clear all" link.
-  var clearAllLinks = document.querySelectorAll('clear-all-link');
-  expectEquals(clearAllLinks.length, 0);
-
-  // All the "Remove from list" items should be text. Check only one, to avoid
-  // spam in case of failure.
-  var removeTexts = document.querySelectorAll('.controls > .disabled-link');
-  expectEquals(removeTexts.length, TOTAL_RESULT_COUNT);
-  expectEquals(removeTexts[0].nodeName, 'SPAN');
-
-  // There should be no "Remove from list" links.
-  var removeLinks = document.querySelectorAll('control-remove-link');
-  expectEquals(removeLinks.length, 0);
-
-  // Attempting to remove items anyway should fail.
-  // The model would have been cleared synchronously, even though the actual
-  // back-end removal (also disabled, but tested elsewhere) is asynchronous.
-  clearAll();
-  expectEquals(downloads.size(), TOTAL_RESULT_COUNT);
-
+  this.deleteProhibitedTestHelper(false);
   // TODO(pamg): Mock out the back-end calls, so we can also test removing a
   // single item.
   testDone();
 });
+
+/**
+ * Fixture for Downloads WebUI testing for a supervised user.
+ * @extends {DownloadsWebUIDeleteProhibitedTest}
+ * @constructor
+ */
+function DownloadsWebUIForSupervisedUsersTest() {}
+
+DownloadsWebUIForSupervisedUsersTest.prototype = {
+  __proto__: DownloadsWebUIDeleteProhibitedTest.prototype,
+
+  /** @override */
+  testGenPreamble: function() {
+    GEN('  ChangeProfileToSupervised();');
+  },
+};
+
+TEST_F('DownloadsWebUIForSupervisedUsersTest', 'supervisedUsers', function() {

[Bernhard Bauer, 2013/09/06 11:23:26]

Test cases should start with an uppercase letter (SupervisedUsers); also above.

[ibra, 2013/09/06 16:56:42]

Done.

+  this.deleteProhibitedTestHelper(true);
+  testDone();
+});