MD Settings: don't allow supervised users to switch to GTK+ theme

chrome/test/data/webui/settings/appearance_page_test.js

Index: chrome/test/data/webui/settings/appearance_page_test.js
diff --git a/chrome/test/data/webui/settings/appearance_page_test.js b/chrome/test/data/webui/settings/appearance_page_test.js
index 4ade4fc2f9c366a241d3c4dff5e9d28d25dc3771..cd3e3970e274d741ca92290aeb826e4e20c6a916 100644
--- a/chrome/test/data/webui/settings/appearance_page_test.js
+++ b/chrome/test/data/webui/settings/appearance_page_test.js
@@ -15,6 +15,7 @@ cr.define('settings_appearance', function() {
   var TestAppearanceBrowserProxy = function() {
     settings.TestBrowserProxy.call(this, [
       'getThemeInfo',
+      'isSupervised',
       'openWallpaperManager',
       'useDefaultTheme',
       'useSystemTheme',
@@ -24,6 +25,9 @@ cr.define('settings_appearance', function() {
   TestAppearanceBrowserProxy.prototype = {
     __proto__: settings.TestBrowserProxy.prototype,
 
+    /** @private */
+    isSupervised_: false,
+
     /** @override */
     getThemeInfo: function(themeId) {
       this.methodCalled('getThemeInfo', themeId);
@@ -31,6 +35,12 @@ cr.define('settings_appearance', function() {
     },
 
     /** @override */
+    isSupervised: function() {
+      this.methodCalled('isSupervised');
+      return this.isSupervised_;
+    },
+
+    /** @override */
     openWallpaperManager: function() {
       this.methodCalled('openWallpaperManager');
     },
@@ -44,6 +54,11 @@ cr.define('settings_appearance', function() {
     useSystemTheme: function() {
       this.methodCalled('useSystemTheme');
     },
+
+    /** @param {boolean} Whether the user is supervised */
+    setIsSupervised: function(isSupervised) {
+      this.isSupervised_ = isSupervised;
+    },
   };
 
   /**
@@ -105,7 +120,7 @@ cr.define('settings_appearance', function() {
           extensions: {
             theme: {
               id: {
-                value: 'asdf',
+                value: '',
               },
               use_system: {
                 value: false,
@@ -135,16 +150,58 @@ cr.define('settings_appearance', function() {
       }
 
       test('useDefaultTheme', function() {
+        assertFalse(!!appearancePage.get('prefs.extensions.theme.id.value'));
+        assertFalse(appearancePage.useSystemTheme_);

[dpapad, 2016/10/18 19:18:00]

Is there any way to avoid referring to private state of the appearance page
here? Perhaps dropping this assertion, keeping only assertions that refer to the
DOM, and at line 159, instead of setting the private var directly, somehow
trigger the useSystemThemePrefChanged_ properly (maybe by actually changing the
pref)?

[Dan Beam, 2016/10/18 19:27:44]

the pref only exists on linux

[dpapad, 2016/10/18 19:37:27]

Ok. Can we split the the default and system themes test then? And wrap the
"system" theme test in an 
if (cr.isLinux && !cr.isChromeOS) {...}, similar to the next test? See also
question at line 181. 

[Dan Beam, 2016/10/18 21:50:25]

Done.

+        // No custom nor system theme in use; the "RESET TO DEFAULT" or "USE
+        // CLASSIC" should be hidden.
+        assertFalse(!!appearancePage.$$('#useDefault'));
+
+        appearancePage.useSystemTheme_ = true;
+        Polymer.dom.flush();
+        // The system theme only currently exists on Linux, but the code is
+        // cross-platform. If the system theme is in use, "USE CLASSIC" should
+        // show.
+        assertTrue(!!appearancePage.$$('#useDefault'));
+
+        appearancePage.useSystemTheme_ = false;
+        appearancePage.set('prefs.extensions.theme.id.value', 'fake theme id');
+        Polymer.dom.flush();
+
+        // With a custom theme installed, "USE CLASSIC"/"RESET" should show.
         var button = appearancePage.$$('#useDefault');
         assertTrue(!!button);
+
         MockInteractions.tap(button);
         return appearanceBrowserProxy.whenCalled('useDefaultTheme');
       });
 
       if (cr.isLinux && !cr.isChromeOS) {
         test('useSystemTheme', function() {
+          assertFalse(!!appearancePage.get('prefs.extensions.theme.id.value'));
+          appearancePage.useSystemTheme_ = true;

[dpapad, 2016/10/18 19:37:27]

This tests only runs on Linux and therefore the pref should exist. Can we avoid
referring to private member vars of the advanced page here and use the pref to
trigger the codepaths to be tested?

[Dan Beam, 2016/10/18 21:50:25]

Done.

+          Polymer.dom.flush();
+          // The "USE GTK+" button shouldn't be showing if it's already in use.
+          assertFalse(!!appearancePage.$$('#useSystem'));
+
+          appearanceBrowserProxy.setIsSupervised(true);
+          appearancePage.useSystemTheme_ = false;
+          Polymer.dom.flush();
+          // Supervised users have their own theme and can't use GTK+ theme.
+          assertFalse(!!appearancePage.$$('#useDefault'));
+          assertFalse(!!appearancePage.$$('#useSystem'));
+          // If there's no "USE" buttons, the container should be hidden.
+          assertTrue(appearancePage.$$('.secondary-action').hidden);
+
+          appearanceBrowserProxy.setIsSupervised(false);
+          appearancePage.set('prefs.extensions.theme.id.value', 'fake theme');
+          Polymer.dom.flush();
+          // If there's "USE" buttons again, the container should be visible.
+          assertTrue(!!appearancePage.$$('#useDefault'));
+          assertFalse(appearancePage.$$('.secondary-action').hidden);
+
           var button = appearancePage.$$('#useSystem');
           assertTrue(!!button);
+
           MockInteractions.tap(button);
           return appearanceBrowserProxy.whenCalled('useSystemTheme');
         });