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

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

R=dpapad@chromium.org
BUG=655948
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/a3579d084d1c7856a8ccff5d6191d6eaba9fad97
Cr-Commit-Position: refs/heads/master@{#426093}

[Dan Beam, 2016-10-18 02:22:33]

Description was changed from

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

R=dpapad@chromium.org
BUG=655948
==========

to

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

R=dpapad@chromium.org
BUG=655948
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-10-18 02:23:09]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 02:23:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 03:16:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-18 03:16:38]

Dry run: This issue passed the CQ dry run.

[dpapad, 2016-10-18 17:49:35]

https://chromiumcodereview.appspot.com/2426863002/diff/1/chrome/browser/resou...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(right):

https://chromiumcodereview.appspot.com/2426863002/diff/1/chrome/browser/resou...
chrome/browser/resources/settings/appearance_page/appearance_page.html:72: <div
class="secondary-action">
Can we just make this div hidden instead of wrapping the entire div with a new
dom-if? Performance wise, this will be an empty <div> when hidden, so not a big
deal.

https://chromiumcodereview.appspot.com/2426863002/diff/1/chrome/browser/resou...
chrome/browser/resources/settings/appearance_page/appearance_page.html:80:
<template is="dom-if" if="[[showUseSystem_(
Can we add a test regarding showing/hiding the theme buttons based on policy (or
anything else that shows/hides those buttons)?

https://chromiumcodereview.appspot.com/2426863002/diff/1/chrome/browser/resou...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):

https://chromiumcodereview.appspot.com/2426863002/diff/1/chrome/browser/resou...
chrome/browser/resources/settings/appearance_page/appearance_page.js:182: return
!!this.get('prefs.extensions.theme.id.value');
Nit:
return this.useSystemTheme_ || !!this.get('prefs.extensions.theme.id.value');

[Dan Beam, 2016-10-18 18:59:52]

https://chromiumcodereview.appspot.com/2426863002/diff/1/chrome/browser/resou...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(right):

https://chromiumcodereview.appspot.com/2426863002/diff/1/chrome/browser/resou...
chrome/browser/resources/settings/appearance_page/appearance_page.html:72: <div
class="secondary-action">
On 2016/10/18 17:49:34, dpapad wrote:
> Can we just make this div hidden instead of wrapping the entire div with a new
> dom-if? Performance wise, this will be an empty <div> when hidden, so not a
big
> deal.

Done.

https://chromiumcodereview.appspot.com/2426863002/diff/1/chrome/browser/resou...
chrome/browser/resources/settings/appearance_page/appearance_page.html:80:
<template is="dom-if" if="[[showUseSystem_(
On 2016/10/18 17:49:34, dpapad wrote:
> Can we add a test regarding showing/hiding the theme buttons based on policy
(or
> anything else that shows/hides those buttons)?

Done.

https://chromiumcodereview.appspot.com/2426863002/diff/1/chrome/browser/resou...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):

https://chromiumcodereview.appspot.com/2426863002/diff/1/chrome/browser/resou...
chrome/browser/resources/settings/appearance_page/appearance_page.js:182: return
!!this.get('prefs.extensions.theme.id.value');
On 2016/10/18 17:49:34, dpapad wrote:
> Nit:
> return this.useSystemTheme_ || !!this.get('prefs.extensions.theme.id.value');

see updated code

[Dan Beam, 2016-10-18 19:00:43]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 19:01:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2016-10-18 19:18:01]

Thanks for adding tests! Few more questions/comments.

https://codereview.chromium.org/2426863002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js
(right):

https://codereview.chromium.org/2426863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js:48:
return loadTimeData.getBoolean('isSupervised');
I like the trick of using loadTimeData from within a BrowserPorxy to send info
to the page synchronously.

https://codereview.chromium.org/2426863002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(right):

https://codereview.chromium.org/2426863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.html:70: <div
class="secondary-action" hidden="[[!showThemesSecondary_(
I thought native elements' |hidden| had to be set as an attribute, therefore
using "$=" instead. Does "=" also work?

https://codereview.chromium.org/2426863002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/appearance_page_test.js (right):

https://codereview.chromium.org/2426863002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/appearance_page_test.js:154:
assertFalse(appearancePage.useSystemTheme_);
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]

https://codereview.chromium.org/2426863002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js
(right):

https://codereview.chromium.org/2426863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js:48:
return loadTimeData.getBoolean('isSupervised');
On 2016/10/18 19:18:00, dpapad wrote:
> I like the trick of using loadTimeData from within a BrowserPorxy to send info
> to the page synchronously.

Acknowledged.

https://codereview.chromium.org/2426863002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(right):

https://codereview.chromium.org/2426863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.html:70: <div
class="secondary-action" hidden="[[!showThemesSecondary_(
On 2016/10/18 19:18:00, dpapad wrote:
> I thought native elements' |hidden| had to be set as an attribute, therefore
> using "$=" instead. Does "=" also work?

$= means element.setAttribute(prop, val)
= means element[prop] = val

either would work in this case

https://codereview.chromium.org/2426863002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/appearance_page_test.js (right):

https://codereview.chromium.org/2426863002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/appearance_page_test.js:154:
assertFalse(appearancePage.useSystemTheme_);
On 2016/10/18 19:18:00, dpapad wrote:
> 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)?

the pref only exists on linux

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

LGTM, with follow up testing related question.

https://codereview.chromium.org/2426863002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/appearance_page_test.js (right):

https://codereview.chromium.org/2426863002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/appearance_page_test.js:154:
assertFalse(appearancePage.useSystemTheme_);
On 2016/10/18 at 19:27:44, Dan Beam wrote:
> On 2016/10/18 19:18:00, dpapad wrote:
> > 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)?
> 
> the pref only exists on linux

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.

https://codereview.chromium.org/2426863002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/appearance_page_test.js:181:
appearancePage.useSystemTheme_ = true;
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?

[commit-bot: I haz the power, 2016-10-18 20:03:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-18 20:03:36]

Dry run: Exceeded global retry quota

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

https://codereview.chromium.org/2426863002/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/appearance_page_test.js (right):

https://codereview.chromium.org/2426863002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/appearance_page_test.js:154:
assertFalse(appearancePage.useSystemTheme_);
On 2016/10/18 19:37:27, dpapad wrote:
> On 2016/10/18 at 19:27:44, Dan Beam wrote:
> > On 2016/10/18 19:18:00, dpapad wrote:
> > > 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)?
> > 
> > the pref only exists on linux
> 
> 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. 

Done.

https://codereview.chromium.org/2426863002/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/appearance_page_test.js:181:
appearancePage.useSystemTheme_ = true;
On 2016/10/18 19:37:27, dpapad wrote:
> 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?

Done.

[Dan Beam, 2016-10-18 21:54:25]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[dpapad, 2016-10-18 21:54:40]

On 2016/10/18 at 21:50:25, dbeam wrote:
>
https://codereview.chromium.org/2426863002/diff/20001/chrome/test/data/webui/...
> File chrome/test/data/webui/settings/appearance_page_test.js (right):
> 
>
https://codereview.chromium.org/2426863002/diff/20001/chrome/test/data/webui/...
> chrome/test/data/webui/settings/appearance_page_test.js:154:
assertFalse(appearancePage.useSystemTheme_);
> On 2016/10/18 19:37:27, dpapad wrote:
> > On 2016/10/18 at 19:27:44, Dan Beam wrote:
> > > On 2016/10/18 19:18:00, dpapad wrote:
> > > > 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)?
> > > 
> > > the pref only exists on linux
> > 
> > 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. 
> 
> Done.
> 
>
https://codereview.chromium.org/2426863002/diff/20001/chrome/test/data/webui/...
> chrome/test/data/webui/settings/appearance_page_test.js:181:
appearancePage.useSystemTheme_ = true;
> On 2016/10/18 19:37:27, dpapad wrote:
> > 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?
> 
> Done.

Thanks! Tests look great now.

[commit-bot: I haz the power, 2016-10-18 21:55:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 23:49:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-18 23:49:30]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2016-10-18 23:49:58]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2016-10-18 23:49:59]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2426863002/#ps40001
(title: "split tests")

[commit-bot: I haz the power, 2016-10-18 23:50:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-19 00:01:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-21 13:04:53]

Description was changed from

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

R=dpapad@chromium.org
BUG=655948
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

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

R=dpapad@chromium.org
BUG=655948
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/a3579d084d1c7856a8ccff5d6191d6eaba9fad97
Cr-Commit-Position: refs/heads/master@{#426093}
==========

[commit-bot: I haz the power, 2016-10-21 13:04:54]

Patchset 3 (id:??) landed as
https://crrev.com/a3579d084d1c7856a8ccff5d6191d6eaba9fad97
Cr-Commit-Position: refs/heads/master@{#426093}