[MD settings] set pref to valid slider value

[MD settings] set pref to valid slider value

This CL will move the minimum font size to the start of the range if the pref has not been changed (instead of using a font size of zero).

BUG=692985
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2738333004
Cr-Commit-Position: refs/heads/master@{#456919}
Committed: https://chromium.googlesource.com/chromium/src/+/fd0523d1fb0087eb86205242d06a8044a5107ceb

[dschuyler, 2017-03-10 23:20:46]

Description was changed from

==========
[MD settings] set pref to valid slider value

This CL will move a preference value to a valid slider value.

BUG=692985
==========

to

==========
[MD settings] set pref to valid slider value

This CL will move a preference value to a valid slider value.

BUG=692985
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dschuyler, 2017-03-10 23:20:48]

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

[commit-bot: I haz the power, 2017-03-10 23:21:15]

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, 2017-03-10 23:33:12]

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

[commit-bot: I haz the power, 2017-03-10 23:33:13]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[dschuyler, 2017-03-13 23:36:33]

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

[commit-bot: I haz the power, 2017-03-13 23:37:02]

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

[dschuyler, 2017-03-14 00:04:30]

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

[commit-bot: I haz the power, 2017-03-14 00:05:06]

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

[dschuyler, 2017-03-14 00:05:06]

Patchset #1 (id:1) has been deleted

[dschuyler, 2017-03-14 00:05:14]

Patchset #1 (id:20001) has been deleted

[dschuyler, 2017-03-14 00:05:27]

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

[dschuyler, 2017-03-14 00:06:53]

Description was changed from

==========
[MD settings] set pref to valid slider value

This CL will move a preference value to a valid slider value.

BUG=692985
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings] set pref to valid slider value

This CL will move the minimum font size to the start of the range if the pref
has not been changed (instead of using a font size of zero).

BUG=692985
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2017-03-14 00:19:28]

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

[commit-bot: I haz the power, 2017-03-14 00:19:28]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Dan Beam, 2017-03-14 03:27:19]

https://codereview.chromium.org/2738333004/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html
(right):

https://codereview.chromium.org/2738333004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html:25:
style$="[[computeMinimumStyle_(
btw, you can now do

style$="font-family: [[computeFontFamily_(...)]]; font-size:
[[computeFontSize_(...)]]"

if that's useful to you

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

https://codereview.chromium.org/2738333004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_fonts_page.js:165:
return (value ? value : MINIMUM_FONT_SIZE_RANGE_[0]).toString();
(value || MINIMUM_FONT_SIZE_RANGE_[0])

why toString?

[dschuyler, 2017-03-14 18:54:39]

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

[commit-bot: I haz the power, 2017-03-14 18:55:15]

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

[dschuyler, 2017-03-14 18:55:31]

https://codereview.chromium.org/2738333004/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html
(right):

https://codereview.chromium.org/2738333004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html:25:
style$="[[computeMinimumStyle_(
On 2017/03/14 03:27:19, Dan Beam wrote:
> btw, you can now do
> 
> style$="font-family: [[computeFontFamily_(...)]]; font-size:
> [[computeFontSize_(...)]]"
> 
> if that's useful to you

Done.

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

https://codereview.chromium.org/2738333004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_fonts_page.js:165:
return (value ? value : MINIMUM_FONT_SIZE_RANGE_[0]).toString();
On 2017/03/14 03:27:19, Dan Beam wrote:
> (value || MINIMUM_FONT_SIZE_RANGE_[0])
> 
> why toString?

Done.

[Dan Beam, 2017-03-14 18:59:35]

https://codereview.chromium.org/2738333004/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html
(right):

https://codereview.chromium.org/2738333004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html:29:
'[[prefs.webkit.webprefs.fonts.standard.Zyyy.value]];"
only 1 single quote, does this handle font names with ' in them?

[dschuyler, 2017-03-14 19:05:16]

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

[commit-bot: I haz the power, 2017-03-14 19:05:45]

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

[dschuyler, 2017-03-14 19:13:13]

Thanks!

https://codereview.chromium.org/2738333004/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html
(right):

https://codereview.chromium.org/2738333004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html:29:
'[[prefs.webkit.webprefs.fonts.standard.Zyyy.value]];"
On 2017/03/14 18:59:35, Dan Beam wrote:
> only 1 single quote, does this handle font names with ' in them?

This is a typo, here and each font-family below. I added the closing quote. Iirc
the quotes address an issue for Windows fonts which may have a space in the name
(err, and the CSS spec calls for either quoting or escaping the font name;
quoting is simpler -- I meant that where we've seen an issue in the wild is with
Windows fonts).

Trivia: with just the leading quote, it doesn't recognize the font name and uses
the default font.

Done.

[Dan Beam, 2017-03-14 19:19:03]

https://codereview.chromium.org/2738333004/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html
(right):

https://codereview.chromium.org/2738333004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html:29:
'[[prefs.webkit.webprefs.fonts.standard.Zyyy.value]];"
On 2017/03/14 19:13:12, dschuyler wrote:
> On 2017/03/14 18:59:35, Dan Beam wrote:
> > only 1 single quote, does this handle font names with ' in them?
> 
> This is a typo, here and each font-family below. I added the closing quote.
Iirc
> the quotes address an issue for Windows fonts which may have a space in the
name
> (err, and the CSS spec calls for either quoting or escaping the font name;
> quoting is simpler -- I meant that where we've seen an issue in the wild is
with
> Windows fonts).
> 
> Trivia: with just the leading quote, it doesn't recognize the font name and
uses
> the default font.
> 
> Done.

i'm asking, what if the font name is

  Doesn't Work

it results in

  style="font-family: 'Doesn't Work';"

which... doesn't work ;) (I believe)

[dschuyler, 2017-03-14 19:43:17]

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

[commit-bot: I haz the power, 2017-03-14 19:44:18]

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

[dschuyler, 2017-03-14 19:48:40]

Description was changed from

==========
[MD settings] set pref to valid slider value

This CL will move the minimum font size to the start of the range if the pref
has not been changed (instead of using a font size of zero).

BUG=692985
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings] set pref to valid slider value

This CL will move the minimum font size to the start of the range if the pref
has not been changed (instead of using a font size of zero).
Changed font-family to not quote font names.

BUG=692985
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dschuyler, 2017-03-14 20:35:21]

Patchset #4 (id:100001) has been deleted

[dschuyler, 2017-03-14 20:35:31]

Description was changed from

==========
[MD settings] set pref to valid slider value

This CL will move the minimum font size to the start of the range if the pref
has not been changed (instead of using a font size of zero).
Changed font-family to not quote font names.

BUG=692985
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings] set pref to valid slider value

This CL will move the minimum font size to the start of the range if the pref
has not been changed (instead of using a font size of zero).

BUG=692985
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dschuyler, 2017-03-14 20:39:34]

https://codereview.chromium.org/2738333004/diff/60001/chrome/browser/resource...
File
chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html
(right):

https://codereview.chromium.org/2738333004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html:29:
'[[prefs.webkit.webprefs.fonts.standard.Zyyy.value]];"
On 2017/03/14 19:19:02, Dan Beam wrote:
> On 2017/03/14 19:13:12, dschuyler wrote:
> > On 2017/03/14 18:59:35, Dan Beam wrote:
> > > only 1 single quote, does this handle font names with ' in them?
> > 
> > This is a typo, here and each font-family below. I added the closing quote.
> Iirc
> > the quotes address an issue for Windows fonts which may have a space in the
> name
> > (err, and the CSS spec calls for either quoting or escaping the font name;
> > quoting is simpler -- I meant that where we've seen an issue in the wild is
> with
> > Windows fonts).
> > 
> > Trivia: with just the leading quote, it doesn't recognize the font name and
> uses
> > the default font.
> > 
> > Done.
> 
> i'm asking, what if the font name is
> 
>   Doesn't Work
> 
> it results in
> 
>   style="font-family: 'Doesn't Work';"
> 
> which... doesn't work ;) (I believe)

Good question. Since it's a change from what we were doing previously and I'd
like to separate the topics of the minimum font size and the font-family names,
I've made a new CL about the quoting at
https://codereview.chromium.org/2749873003/

[commit-bot: I haz the power, 2017-03-14 21:12:44]

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

[commit-bot: I haz the power, 2017-03-14 21:12:45]

Dry run: This issue passed the CQ dry run.

[tommycli, 2017-03-14 23:57:30]

lgtm from my perspective. I'm okay with fixing the quoting issue in a separate
CL, since the status quo has the same breaking-quotes problem.

[Dan Beam, 2017-03-15 00:00:52]

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

[Dan Beam, 2017-03-15 00:00:53]

so the old code does have the same problem, and i can't actually find anything
online to say that font-family often has any type of quotes in it

lgtm

[dschuyler, 2017-03-15 00:33:52]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2017-03-15 00:34:32]

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, 2017-03-15 00:41:03]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489538032165350,
"parent_rev": "e6aa471664d17dc77990fc4584918c523e0fcf8a", "commit_rev":
"fd0523d1fb0087eb86205242d06a8044a5107ceb"}

[commit-bot: I haz the power, 2017-03-15 00:42:19]

Description was changed from

==========
[MD settings] set pref to valid slider value

This CL will move the minimum font size to the start of the range if the pref
has not been changed (instead of using a font size of zero).

BUG=692985
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings] set pref to valid slider value

This CL will move the minimum font size to the start of the range if the pref
has not been changed (instead of using a font size of zero).

BUG=692985
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2738333004
Cr-Commit-Position: refs/heads/master@{#456919}
Committed:
https://chromium.googlesource.com/chromium/src/+/fd0523d1fb0087eb86205242d06a...
==========

[commit-bot: I haz the power, 2017-03-15 00:42:21]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/fd0523d1fb0087eb86205242d06a...