[MD settings] adding font settings files

[MD settings] adding font settings files

This CL adds the new files needed for font
settings (web content) in the appearance
settings.

BUG=531786
Committed: https://crrev.com/8eb14f22d158e59ff8794b533f72783f6faa175f
Cr-Commit-Position: refs/heads/master@{#357916}

[dschuyler, 2015-10-27 23:52:47]

Description was changed from

==========
[MD settings] adding font settings files

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

to

==========
[MD settings] adding font settings files

This CL adds the new files needed for font
settings (web content) in the appearance
settings.

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

[dschuyler, 2015-10-27 23:52:47]

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

[stevenjb, 2015-10-28 00:17:16]

https://codereview.chromium.org/1415463013/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/appearance_page/appearance_font_menu.html
(right):

https://codereview.chromium.org/1415463013/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_font_menu.html:13:
<div>
needed?

https://codereview.chromium.org/1415463013/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_font_menu.html:17:
<template is="dom-repeat" items="{{menuOptions}}">
[[menuOptions]] ?

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

https://codereview.chromium.org/1415463013/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_font_menu.js:32:
menuLabel_: {
nit: private properties after public ones

https://codereview.chromium.org/1415463013/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_font_menu.js:59: *
Preferences state.
This is just a single pref right? Also, can we typedef it?

https://codereview.chromium.org/1415463013/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_font_menu.js:103: *
Check to see if we have all the pieces needed to enable the control.
@param for menuOptions, selectedValue_

https://codereview.chromium.org/1415463013/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_font_menu.js:121:
if (this.selectedValue_ !== undefined) {
This should not be possible, checkSetup_ will only be called once menuOptions
and selectedValue_ are defined.

https://codereview.chromium.org/1415463013/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html
(right):

https://codereview.chromium.org/1415463013/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html:44:
</div>
There is a lot of copy/paste here, maybe a good candidate for a small helper
element?

[dschuyler, 2015-10-28 22:22:22]

https://codereview.chromium.org/1415463013/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/appearance_page/appearance_font_menu.html
(right):

https://codereview.chromium.org/1415463013/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_font_menu.html:13:
<div>
On 2015/10/28 00:17:15, stevenjb wrote:
> needed?

Done.

https://codereview.chromium.org/1415463013/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_font_menu.html:17:
<template is="dom-repeat" items="{{menuOptions}}">
On 2015/10/28 00:17:15, stevenjb wrote:
> [[menuOptions]] ?

Done.

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

https://codereview.chromium.org/1415463013/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_font_menu.js:32:
menuLabel_: {
On 2015/10/28 00:17:16, stevenjb wrote:
> nit: private properties after public ones

Done.

https://codereview.chromium.org/1415463013/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_font_menu.js:59: *
Preferences state.
On 2015/10/28 00:17:16, stevenjb wrote:
> This is just a single pref right? Also, can we typedef it?

I can roughly @type it.  Michael had a good idea to generate
a typedef from the white-listed prefs.

https://codereview.chromium.org/1415463013/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_font_menu.js:103: *
Check to see if we have all the pieces needed to enable the control.
On 2015/10/28 00:17:16, stevenjb wrote:
> @param for menuOptions, selectedValue_

Done.

https://codereview.chromium.org/1415463013/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_font_menu.js:121:
if (this.selectedValue_ !== undefined) {
On 2015/10/28 00:17:16, stevenjb wrote:
> This should not be possible, checkSetup_ will only be called once menuOptions
> and selectedValue_ are defined.

Done.

https://codereview.chromium.org/1415463013/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html
(right):

https://codereview.chromium.org/1415463013/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html:44:
</div>
On 2015/10/28 00:17:16, stevenjb wrote:
> There is a lot of copy/paste here, maybe a good candidate for a small helper
> element?

Like a settings-row class with <left> and <right> elements?  :)
Or is it something in the font-menu?

[stevenjb, 2015-10-29 19:56:02]

lgtm with @type nits

https://codereview.chromium.org/1415463013/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html
(right):

https://codereview.chromium.org/1415463013/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html:44:
</div>
On 2015/10/28 22:22:22, dschuyler wrote:
> On 2015/10/28 00:17:16, stevenjb wrote:
> > There is a lot of copy/paste here, maybe a good candidate for a small helper
> > element?
> 
> Like a settings-row class with <left> and <right> elements?  :)
> Or is it something in the font-menu?

I would suggest that there is a little more copy/paste here than that, but it's
mostly just a personal preference.

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

https://codereview.chromium.org/1415463013/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_font_menu.js:40: *
Preference state.
This should be more descriptive.

https://codereview.chromium.org/1415463013/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_font_menu.js:41: *
@type {!Object}
'Object' doesn't add any type info. Isn't this always:
{value: (number|string)} ?

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

https://codereview.chromium.org/1415463013/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_fonts_page.js:131:
* @type {!Object}
Not needed. In this case the entire pref store is just a dictionary (Object).

[dschuyler, 2015-11-04 22:10:56]

https://codereview.chromium.org/1415463013/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html
(right):

https://codereview.chromium.org/1415463013/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html:44:
</div>
On 2015/10/29 19:56:02, stevenjb wrote:
> On 2015/10/28 22:22:22, dschuyler wrote:
> > On 2015/10/28 00:17:16, stevenjb wrote:
> > > There is a lot of copy/paste here, maybe a good candidate for a small
helper
> > > element?
> > 
> > Like a settings-row class with <left> and <right> elements?  :)
> > Or is it something in the font-menu?
> 
> I would suggest that there is a little more copy/paste here than that, but
it's
> mostly just a personal preference.

I'd like to give this a little time to see if the UI settles down a bit more.

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

https://codereview.chromium.org/1415463013/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_font_menu.js:40: *
Preference state.
On 2015/10/29 19:56:02, stevenjb wrote:
> This should be more descriptive.

Done.

https://codereview.chromium.org/1415463013/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_font_menu.js:41: *
@type {!Object}
On 2015/10/29 19:56:02, stevenjb wrote:
> 'Object' doesn't add any type info. Isn't this always:
> {value: (number|string)} ?

I've tested it with number, string and dictionary.
How about {?PrefObject}

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

https://codereview.chromium.org/1415463013/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_fonts_page.js:131:
* @type {!Object}
On 2015/10/29 19:56:02, stevenjb wrote:
> Not needed. In this case the entire pref store is just a dictionary (Object).

Done.

[dschuyler, 2015-11-04 22:25:37]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2015-11-04 22:25:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/1415463013/#ps60001
(title: "prefs fix")

[commit-bot: I haz the power, 2015-11-04 22:26:09]

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

[commit-bot: I haz the power, 2015-11-04 22:42:11]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-04 22:42:59]

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