[MD settings] appearance theme and wallpaper

[MD settings] appearance browser proxy and wallpaper

This CL adds the appearance browser proxy and wallpaper
settings in the appearance settings.  Wallpaper settings
only appear on Chrome OS.

BUG=531786
Committed: https://crrev.com/0614eafcb01516c1ea51a7cbb7790af1243f8f3f
Cr-Commit-Position: refs/heads/master@{#388572}

[dschuyler, 2016-04-19 22:30:59]

Patchset #1 (id:1) has been deleted

[dschuyler, 2016-04-19 22:46:33]

Description was changed from

==========
[MD settings] appearnce theme and wallpaper

This CL adds theme and wallpaper settings in the appearance
settings.  Wallpaper settings only appear on Chrome OS.

BUG=531786
==========

to

==========
[MD settings] appearance theme and wallpaper

This CL adds theme and wallpaper settings in the appearance
settings.  Wallpaper settings only appear on Chrome OS.

BUG=531786
==========

[dschuyler, 2016-04-19 22:46:46]

Patchset #1 (id:10010) has been deleted

[dschuyler, 2016-04-19 22:46:58]

dschuyler@chromium.org changed reviewers:
+ dpapad@chromium.org

[Dan Beam, 2016-04-19 22:54:48]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2016-04-19 22:54:48]

can you add screenshots?

[Dan Beam, 2016-04-19 22:54:55]

dbeam@chromium.org changed reviewers:
- dbeam@chromium.org

[dschuyler, 2016-04-19 23:22:31]

Description was changed from

==========
[MD settings] appearance theme and wallpaper

This CL adds theme and wallpaper settings in the appearance
settings.  Wallpaper settings only appear on Chrome OS.

BUG=531786
==========

to

==========
[MD settings] appearance browser proxy and wallpaper

This CL adds the appearance browser proxy and wallpaper
settings in the appearance settings.  Wallpaper settings
only appear on Chrome OS.

BUG=531786
==========

[dschuyler, 2016-04-19 23:23:27]

On 2016/04/19 22:54:48, Dan Beam wrote:
> can you add screenshots?

Screen shots are at: http://imgur.com/a/PKadf

[dschuyler, 2016-04-19 23:29:14]

On 2016/04/19 23:23:27, dschuyler wrote:
> On 2016/04/19 22:54:48, Dan Beam wrote:
> > can you add screenshots?
> 

Before screen shots are at: http://imgur.com/a/PKadf
After screen shots are at:http://imgur.com/a/F9n6B

Shots with the Wallpaper row are Chrome OS.

[dpapad, 2016-04-19 23:32:40]

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

https://codereview.chromium.org/1896283003/diff/30001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js:11:
* @return {!Promise} Whether the theme may be reset.
!Promise<boolean>

https://codereview.chromium.org/1896283003/diff/30001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js:18:
openWallpaperManager: assertNotReached,
Can you wrap this (and the implementation below) with "if expr" as done at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

https://codereview.chromium.org/1896283003/diff/30001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):

https://codereview.chromium.org/1896283003/diff/30001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:170: /**
Same here, let's wrap with "if expr". I think you also need to mark this file as
"flattenhtml=true" in settings_resources.grd for this to work.

https://codereview.chromium.org/1896283003/diff/30001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:174:
openWallpaperGallery_: function() {
Nit(optional): Gallery vs Manager, can we stick with a single term?

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

https://codereview.chromium.org/1896283003/diff/30001/chrome/test/data/webui/...
chrome/test/data/webui/settings/appearance_page_test.js:22: this.allowResetTheme
= false;
Nit: @type {boolean}

https://codereview.chromium.org/1896283003/diff/30001/chrome/test/data/webui/...
chrome/test/data/webui/settings/appearance_page_test.js:109:
PolymerTest.importHtml('chrome://md-settings/i18n_setup.html'),
Is this necessary? Can it be added as a dependency to appearance_page.html
instead?

https://codereview.chromium.org/1896283003/diff/30001/chrome/test/data/webui/...
chrome/test/data/webui/settings/appearance_page_test.js:117: var button =
appearancePage.$$('#wallpaperButton');
Nit: Does the following work?

var button = appearancePage.$.wallpaperButton;

https://codereview.chromium.org/1896283003/diff/30001/chrome/test/data/webui/...
chrome/test/data/webui/settings/appearance_page_test.js:118: assert(!!button);
Nit(optional): assertTrue(!!button).

Effectively assert() and assertTrue() both check for the same thing, but
assert() is used to ensure production code invariants hold, where as
assertTrue() is more appropriate for testing code IMO.

Similarly, I suggest assertFalse(!!someElement) instead of assert(!someElement),
for the reason mentioned above, but also because the double negation
(counter-intuitively) makes the code more readable if it is used everywhere,
consider

assertFalse(!!element2);
vs
assert(!element2);

The former produces an error message that is closer to what we are actually
checking.

[dschuyler, 2016-04-20 00:19:07]

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

https://codereview.chromium.org/1896283003/diff/30001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js:11:
* @return {!Promise} Whether the theme may be reset.
On 2016/04/19 23:32:40, dpapad wrote:
> !Promise<boolean>

Done.

https://codereview.chromium.org/1896283003/diff/30001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js:18:
openWallpaperManager: assertNotReached,
On 2016/04/19 23:32:40, dpapad wrote:
> Can you wrap this (and the implementation below) with "if expr" as done at
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

Done.

https://codereview.chromium.org/1896283003/diff/30001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):

https://codereview.chromium.org/1896283003/diff/30001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:170: /**
On 2016/04/19 23:32:40, dpapad wrote:
> Same here, let's wrap with "if expr". I think you also need to mark this file
as
> "flattenhtml=true" in settings_resources.grd for this to work.

Done.

https://codereview.chromium.org/1896283003/diff/30001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.js:174:
openWallpaperGallery_: function() {
On 2016/04/19 23:32:40, dpapad wrote:
> Nit(optional): Gallery vs Manager, can we stick with a single term?

Done.

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

https://codereview.chromium.org/1896283003/diff/30001/chrome/test/data/webui/...
chrome/test/data/webui/settings/appearance_page_test.js:22: this.allowResetTheme
= false;
On 2016/04/19 23:32:40, dpapad wrote:
> Nit: @type {boolean}

Done.

https://codereview.chromium.org/1896283003/diff/30001/chrome/test/data/webui/...
chrome/test/data/webui/settings/appearance_page_test.js:109:
PolymerTest.importHtml('chrome://md-settings/i18n_setup.html'),
On 2016/04/19 23:32:40, dpapad wrote:
> Is this necessary? Can it be added as a dependency to appearance_page.html
> instead?

Done.

https://codereview.chromium.org/1896283003/diff/30001/chrome/test/data/webui/...
chrome/test/data/webui/settings/appearance_page_test.js:117: var button =
appearancePage.$$('#wallpaperButton');
On 2016/04/19 23:32:40, dpapad wrote:
> Nit: Does the following work?
> 
> var button = appearancePage.$.wallpaperButton;

Done.

https://codereview.chromium.org/1896283003/diff/30001/chrome/test/data/webui/...
chrome/test/data/webui/settings/appearance_page_test.js:118: assert(!!button);
On 2016/04/19 23:32:40, dpapad wrote:
> Nit(optional): assertTrue(!!button).
> 
> Effectively assert() and assertTrue() both check for the same thing, but
> assert() is used to ensure production code invariants hold, where as
> assertTrue() is more appropriate for testing code IMO.
> 
> Similarly, I suggest assertFalse(!!someElement) instead of
assert(!someElement),
> for the reason mentioned above, but also because the double negation
> (counter-intuitively) makes the code more readable if it is used everywhere,
> consider
> 
> assertFalse(!!element2);
> vs
> assert(!element2);
> 
> The former produces an error message that is closer to what we are actually
> checking.
> 
> 

Done.

[dpapad, 2016-04-20 01:16:58]

lgtm

LGTM, with nits.

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

https://codereview.chromium.org/1896283003/diff/50001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js:17:
* ChromeOS only.
Nit(optional): You can remove the comment now, since it is redundant.

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

https://codereview.chromium.org/1896283003/diff/50001/chrome/test/data/webui/...
chrome/test/data/webui/settings/appearance_page_test.js:23: this.allowResetTheme
= false;
This can be private (allowResetTheme_).

https://codereview.chromium.org/1896283003/diff/50001/chrome/test/data/webui/...
chrome/test/data/webui/settings/appearance_page_test.js:45: setAllowResetTheme:
function(isEnabled) {
Nit: Add @param

https://codereview.chromium.org/1896283003/diff/50001/chrome/test/data/webui/...
chrome/test/data/webui/settings/appearance_page_test.js:129: var button =
appearancePage.$$('#resetTheme');
Nit(optional): No need to query the DOM, when an element has an ID (here and
elsewhere), you can simply grab it from the "appearancePage.$" map.

[dschuyler, 2016-04-20 19:14:03]

Patchset #3 (id:70001) has been deleted

[dschuyler, 2016-04-20 19:14:12]

Patchset #3 (id:90001) has been deleted

[dschuyler, 2016-04-20 19:15:46]

also fixed mistake in reset theme.

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

https://codereview.chromium.org/1896283003/diff/50001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_browser_proxy.js:17:
* ChromeOS only.
On 2016/04/20 01:16:58, dpapad wrote:
> Nit(optional): You can remove the comment now, since it is redundant.

Done.

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

https://codereview.chromium.org/1896283003/diff/50001/chrome/test/data/webui/...
chrome/test/data/webui/settings/appearance_page_test.js:23: this.allowResetTheme
= false;
On 2016/04/20 01:16:58, dpapad wrote:
> This can be private (allowResetTheme_).

Done.

https://codereview.chromium.org/1896283003/diff/50001/chrome/test/data/webui/...
chrome/test/data/webui/settings/appearance_page_test.js:45: setAllowResetTheme:
function(isEnabled) {
On 2016/04/20 01:16:58, dpapad wrote:
> Nit: Add @param

Done.

https://codereview.chromium.org/1896283003/diff/50001/chrome/test/data/webui/...
chrome/test/data/webui/settings/appearance_page_test.js:129: var button =
appearancePage.$$('#resetTheme');
On 2016/04/20 01:16:58, dpapad wrote:
> Nit(optional): No need to query the DOM, when an element has an ID (here and
> elsewhere), you can simply grab it from the "appearancePage.$" map.

Ah, because this is in a template, we need
the .$$ here.

[dpapad, 2016-04-20 19:23:09]

SLGTM

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

https://codereview.chromium.org/1896283003/diff/110001/chrome/test/data/webui...
chrome/test/data/webui/settings/appearance_page_test.js:22: /** @type {boolean}
*/
@private

[dschuyler, 2016-04-20 20:46:22]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2016-04-20 20:46:22]

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/1896283003/#ps130001
(title: "private")

[commit-bot: I haz the power, 2016-04-20 20:46:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1896283003/130001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1896283003/130001

[commit-bot: I haz the power, 2016-04-20 20:50:55]

Description was changed from

==========
[MD settings] appearance browser proxy and wallpaper

This CL adds the appearance browser proxy and wallpaper
settings in the appearance settings.  Wallpaper settings
only appear on Chrome OS.

BUG=531786
==========

to

==========
[MD settings] appearance browser proxy and wallpaper

This CL adds the appearance browser proxy and wallpaper
settings in the appearance settings.  Wallpaper settings
only appear on Chrome OS.

BUG=531786
==========

[commit-bot: I haz the power, 2016-04-20 20:50:56]

Committed patchset #4 (id:130001)

[commit-bot: I haz the power, 2016-04-22 19:26:19]

Description was changed from

==========
[MD settings] appearance browser proxy and wallpaper

This CL adds the appearance browser proxy and wallpaper
settings in the appearance settings.  Wallpaper settings
only appear on Chrome OS.

BUG=531786
==========

to

==========
[MD settings] appearance browser proxy and wallpaper

This CL adds the appearance browser proxy and wallpaper
settings in the appearance settings.  Wallpaper settings
only appear on Chrome OS.

BUG=531786

Committed: https://crrev.com/0614eafcb01516c1ea51a7cbb7790af1243f8f3f
Cr-Commit-Position: refs/heads/master@{#388572}
==========

[commit-bot: I haz the power, 2016-04-22 19:26:20]

Patchset 4 (id:??) landed as
https://crrev.com/0614eafcb01516c1ea51a7cbb7790af1243f8f3f
Cr-Commit-Position: refs/heads/master@{#388572}