[MD settings] theme name shown in sub-label, wallpaper chrome os only

[MD settings] theme name shown in sub-label, wallpaper chrome os only

This CL causes the sub-label for the theme setting to
show the current theme name and the wall paper app
row only appears on chrome os. There is also a change
to expand the font settings sub-page properly.

BUG=598462, 531786, 559457
Committed: https://crrev.com/5302e55e8bdefbc8364c889e79d185c06e6a3dda
Cr-Commit-Position: refs/heads/master@{#384424}

[dschuyler, 2016-03-31 00:18:08]

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

[dschuyler, 2016-03-31 00:18:09]

This CL is separated out of CL 1833783002.
(so you've already done a first pass on this).

[dschuyler, 2016-03-31 00:18:58]

Description was changed from

==========
[MD settings] theme name shown in sub-label, wallpaper chrome os only

This CL causes the sub-label for the theme setting to
show the current theme name and the wall paper app
row only appears on chrome os. There is also a change
to expand the font settings sub-page properly.

BUG=598462, 531786
==========

to

==========
[MD settings] theme name shown in sub-label, wallpaper chrome os only

This CL causes the sub-label for the theme setting to
show the current theme name and the wall paper app
row only appears on chrome os. There is also a change
to expand the font settings sub-page properly.

BUG=598462, 531786, 559457
==========

[michaelpg, 2016-03-31 15:48:22]

https://codereview.chromium.org/1841973004/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):

https://codereview.chromium.org/1841973004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.js:108: value:
function() { return this.i18n('chooseFromWebStore'); },
does this duplicate what happens in themeChanged_, or does that not get called
initially when the value is empty?

https://codereview.chromium.org/1841973004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.js:162: * */
nit: extra ' *'

https://codereview.chromium.org/1841973004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.js:164:
chrome.send('openWallpaperManager');
Dan just said to always use a browser proxy instead of chrome.send. I guess that
would also apply to cr.sendWithPromise (and maybe cr.addWebUIListener?) so
that's a larger scoped CL than you should do here.

https://codereview.chromium.org/1841973004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.js:164:
chrome.send('openWallpaperManager');
does this exist in a settings handler?

https://codereview.chromium.org/1841973004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.js:175:
setResetThemeEnabled: function(enabled) {
I assume you have a reason for moving this from one location between two private
functions to another location between two private functions.

https://codereview.chromium.org/1841973004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.js:190: var
self = this;
nit: .bind(this) instead, as that's more common?

https://codereview.chromium.org/1841973004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.js:193:
self.themeSublabel_ = info.name;
2-space indent

[Dan Beam, 2016-03-31 17:53:21]

https://codereview.chromium.org/1841973004/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):

https://codereview.chromium.org/1841973004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.js:164:
chrome.send('openWallpaperManager');
On 2016/03/31 15:48:21, michaelpg wrote:
> Dan just said to always use a browser proxy instead of chrome.send. I guess
that
> would also apply to cr.sendWithPromise (and maybe cr.addWebUIListener?) so
> that's a larger scoped CL than you should do here.

i didn't necesarilly say "always", but I do agree with michaelpg@ that this
class should use a browser proxy when possible.  whether that's before, during,
or after that CL is up to y'all.

Dave: you know my vote as that's what I pre-did:
https://codereview.chromium.org/1845473002/

before hooking up validation:
https://codereview.chromium.org/1849543006/

[michaelpg, 2016-03-31 18:08:43]

On 2016/03/31 17:53:21, Dan Beam wrote:
>
https://codereview.chromium.org/1841973004/diff/1/chrome/browser/resources/se...
> File chrome/browser/resources/settings/appearance_page/appearance_page.js
> (right):
> 
>
https://codereview.chromium.org/1841973004/diff/1/chrome/browser/resources/se...
> chrome/browser/resources/settings/appearance_page/appearance_page.js:164:
> chrome.send('openWallpaperManager');
> On 2016/03/31 15:48:21, michaelpg wrote:
> > Dan just said to always use a browser proxy instead of chrome.send. I guess
> that
> > would also apply to cr.sendWithPromise (and maybe cr.addWebUIListener?) so
> > that's a larger scoped CL than you should do here.
> 
> i didn't necesarilly say "always",

That's what I got from the stand-up, and that we should plan to convert all
usage of chrome.send in pages before launch. It wouldn't be my priority (feel
free to revert that decision), but if we've standardized on that I'll stick to
it.

> but I do agree with michaelpg@ that this
> class should use a browser proxy when possible.  whether that's before,
during,
> or after that CL is up to y'all.
> 
> Dave: you know my vote as that's what I pre-did:
> https://codereview.chromium.org/1845473002/
> 
> before hooking up validation:
> https://codereview.chromium.org/1849543006/

[dschuyler, 2016-03-31 19:13:36]

https://codereview.chromium.org/1841973004/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/appearance_page/appearance_page.js
(right):

https://codereview.chromium.org/1841973004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.js:108: value:
function() { return this.i18n('chooseFromWebStore'); },
On 2016/03/31 15:48:21, michaelpg wrote:
> does this duplicate what happens in themeChanged_, or does that not get called
> initially when the value is empty?

Done.

https://codereview.chromium.org/1841973004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.js:162: * */
On 2016/03/31 15:48:21, michaelpg wrote:
> nit: extra ' *'

Done.

https://codereview.chromium.org/1841973004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.js:164:
chrome.send('openWallpaperManager');
On 2016/03/31 17:53:21, Dan Beam wrote:
> On 2016/03/31 15:48:21, michaelpg wrote:
> > Dan just said to always use a browser proxy instead of chrome.send. I guess
> that
> > would also apply to cr.sendWithPromise (and maybe cr.addWebUIListener?) so
> > that's a larger scoped CL than you should do here.
> 
> i didn't necesarilly say "always", but I do agree with michaelpg@ that this
> class should use a browser proxy when possible.  whether that's before,
during,
> or after that CL is up to y'all.
> 
> Dave: you know my vote as that's what I pre-did:
> https://codereview.chromium.org/1845473002/
> 
> before hooking up validation:
> https://codereview.chromium.org/1849543006/

I'm on board with the conversion.  I'm just doing
it it a different order.

https://codereview.chromium.org/1841973004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.js:164:
chrome.send('openWallpaperManager');
On 2016/03/31 15:48:21, michaelpg wrote:
> does this exist in a settings handler?

Done.

https://codereview.chromium.org/1841973004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.js:164:
chrome.send('openWallpaperManager');
On 2016/03/31 15:48:21, michaelpg wrote:
> Dan just said to always use a browser proxy instead of chrome.send. I guess
that
> would also apply to cr.sendWithPromise (and maybe cr.addWebUIListener?) so
> that's a larger scoped CL than you should do here.

Acknowledged.

https://codereview.chromium.org/1841973004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.js:175:
setResetThemeEnabled: function(enabled) {
On 2016/03/31 15:48:21, michaelpg wrote:
> I assume you have a reason for moving this from one location between two
private
> functions to another location between two private functions.

Done.

https://codereview.chromium.org/1841973004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.js:190: var
self = this;
On 2016/03/31 15:48:21, michaelpg wrote:
> nit: .bind(this) instead, as that's more common?

Done.

https://codereview.chromium.org/1841973004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_page.js:193:
self.themeSublabel_ = info.name;
On 2016/03/31 15:48:21, michaelpg wrote:
> 2-space indent

Done.

[michaelpg, 2016-03-31 20:48:14]

https://codereview.chromium.org/1841973004/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/settings_private/prefs_util.cc (right):

https://codereview.chromium.org/1841973004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/settings_private/prefs_util.cc:73: // Appearance
settings.
i don't exactly mind, but why are you moving these?

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

https://codereview.chromium.org/1841973004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.html:33:
<div>[[i18n('setWallpaper')]]</div>
this and below: make these OS_CHROMEOS only in
md_settings_localized_strings_provider.cc

[dschuyler, 2016-03-31 21:08:22]

https://codereview.chromium.org/1841973004/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/settings_private/prefs_util.cc (right):

https://codereview.chromium.org/1841973004/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/settings_private/prefs_util.cc:73: // Appearance
settings.
On 2016/03/31 20:48:14, michaelpg wrote:
> i don't exactly mind, but why are you moving these?

"Web content" is no longer a settings section,
the new section for them is Appearance.  I wanted to
add extentions.theme.id and it seemed wrong to have
that under Web content rather than Appearance.

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

https://codereview.chromium.org/1841973004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.html:33:
<div>[[i18n('setWallpaper')]]</div>
On 2016/03/31 20:48:14, michaelpg wrote:
> this and below: make these OS_CHROMEOS only in
> md_settings_localized_strings_provider.cc

Done.

[michaelpg, 2016-03-31 21:23:46]

lgtm

[dschuyler, 2016-03-31 22:47:26]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2016-03-31 22:48:07]

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

[commit-bot: I haz the power, 2016-03-31 22:54:02]

Description was changed from

==========
[MD settings] theme name shown in sub-label, wallpaper chrome os only

This CL causes the sub-label for the theme setting to
show the current theme name and the wall paper app
row only appears on chrome os. There is also a change
to expand the font settings sub-page properly.

BUG=598462, 531786, 559457
==========

to

==========
[MD settings] theme name shown in sub-label, wallpaper chrome os only

This CL causes the sub-label for the theme setting to
show the current theme name and the wall paper app
row only appears on chrome os. There is also a change
to expand the font settings sub-page properly.

BUG=598462, 531786, 559457
==========

[commit-bot: I haz the power, 2016-03-31 22:54:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-31 22:55:28]

Description was changed from

==========
[MD settings] theme name shown in sub-label, wallpaper chrome os only

This CL causes the sub-label for the theme setting to
show the current theme name and the wall paper app
row only appears on chrome os. There is also a change
to expand the font settings sub-page properly.

BUG=598462, 531786, 559457
==========

to

==========
[MD settings] theme name shown in sub-label, wallpaper chrome os only

This CL causes the sub-label for the theme setting to
show the current theme name and the wall paper app
row only appears on chrome os. There is also a change
to expand the font settings sub-page properly.

BUG=598462, 531786, 559457

Committed: https://crrev.com/5302e55e8bdefbc8364c889e79d185c06e6a3dda
Cr-Commit-Position: refs/heads/master@{#384424}
==========

[commit-bot: I haz the power, 2016-03-31 22:55:29]

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