UI refresh of Advanced Font Settings Extension

UI refresh of Advanced Font Settings Extension

The UI team designed a new UI for the Advanced Font Settings Extension.
The plan is to link to this extension from the built-in Font Settings
dialog, so it now better matches the built-in Chrome settings design.

See the bug for a screenshot.

BUG=280028
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220823

[falken, 2013-08-27 12:08:22]

Hi Matt,

Could you please review?

Sorry for this bulky CL. I wanted to first get approval from the UI team before
committing to the tree, so I iterated on it locally. Now it's approved.

[Matt Perry, 2013-08-27 20:23:46]

Not sure I'm the best person to review JS. But LGTM

[falken, 2013-08-28 02:29:40]

Thanks Matt.

Yoshiki-san and/or Hirono-san, could you review, especially the JS?

[yoshiki, 2013-08-28 02:53:35]

https://chromiumcodereview.appspot.com/23434003/diff/28001/chrome/common/exte...
File chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js
(right):

https://chromiumcodereview.appspot.com/23434003/diff/28001/chrome/common/exte...
chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js:9:
PendingChanges = function() {
var PendingChanges = function() {
  ...

or 

function PendingChanges() {
  ...

https://chromiumcodereview.appspot.com/23434003/diff/28001/chrome/common/exte...
chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js:9:
PendingChanges = function() {
Add a blank line between the end of @fileoverview annotation and the function
definition.

https://chromiumcodereview.appspot.com/23434003/diff/28001/chrome/common/exte...
chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js:9:
PendingChanges = function() {
Write the JSdoc of the this function.

https://chromiumcodereview.appspot.com/23434003/diff/28001/chrome/common/exte...
chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js:22: *
@param {string} genericFamily The generic family, like "sansserif".
Add "@return" annotation.

https://chromiumcodereview.appspot.com/23434003/diff/28001/chrome/common/exte...
chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js:34: *
    'defaultFontSize', 'defaultFixedFontSize', or 'minFontSize'.
ditto

https://chromiumcodereview.appspot.com/23434003/diff/28001/chrome/common/exte...
chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js:45: *
@param {string} font The font to set the setting to, or null to clear it.
If this parameter is a null-able string, use "?string" instead of just "string"
in type annotation.

https://chromiumcodereview.appspot.com/23434003/diff/28001/chrome/common/exte...
chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js:85:
chrome.fontSettings.setDefaultFontSize( { pixelSize: size });
chrome.fontSettings.setDefaultFontSize({pixelSize: size});

http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...

https://chromiumcodereview.appspot.com/23434003/diff/28001/chrome/common/exte...
chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js:89:
chrome.fontSettings.setDefaultFixedFontSize( { pixelSize: size });
ditto

https://chromiumcodereview.appspot.com/23434003/diff/28001/chrome/common/exte...
chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js:93:
chrome.fontSettings.setMinimumFontSize( { pixelSize: size });
ditto

https://chromiumcodereview.appspot.com/23434003/diff/28001/chrome/common/exte...
File chrome/common/extensions/docs/examples/api/fontSettings/slider.js (right):

https://chromiumcodereview.appspot.com/23434003/diff/28001/chrome/common/exte...
chrome/common/extensions/docs/examples/api/fontSettings/slider.js:19: * @param
{function(boolean)} onDrag Drag begin/end handler.
Is 'onDrag' and/or 'onChange' are optional parameter? If so, rename them to
'opt_onDrag' or 'opt_onChange'

http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...

https://chromiumcodereview.appspot.com/23434003/diff/28001/chrome/common/exte...
chrome/common/extensions/docs/examples/api/fontSettings/slider.js:65:
Slider.prototype.getValue = function() {
Please write JSdoc.

https://chromiumcodereview.appspot.com/23434003/diff/28001/chrome/common/exte...
chrome/common/extensions/docs/examples/api/fontSettings/slider.js:79: var
proportion =(this.input_.value - this.input_.min) /
Add a space after "=".

https://chromiumcodereview.appspot.com/23434003/diff/28001/chrome/common/exte...
chrome/common/extensions/docs/examples/api/fontSettings/slider.js:86: if
(this.onChange_) {
Curly brackets are unnecessary, since there is only a line in the block.

[yoshiki, 2013-08-28 02:59:32]

FYI: I wrote some comments about annotation (JSdoc). Please see the following
links if you have any trouble in writing JSdoc (or ask me or hirono). Thanks!
https://developers.google.com/closure/compiler/docs/js-for-compiler
https://code.google.com/p/jsdoc-toolkit/wiki/TagReference

[falken, 2013-08-28 08:36:16]

Yoshiki-san,

Thanks for the review. Can you take another look?

I also updated options.js to be more in line with the style guidelines.

https://codereview.chromium.org/23434003/diff/28001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js
(right):

https://codereview.chromium.org/23434003/diff/28001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js:9:
PendingChanges = function() {
On 2013/08/28 02:53:35, yoshiki wrote:
> var PendingChanges = function() {
>   ...
> 
> or 
> 
> function PendingChanges() {
>   ...

Done.

https://codereview.chromium.org/23434003/diff/28001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js:9:
PendingChanges = function() {
On 2013/08/28 02:53:35, yoshiki wrote:
> Add a blank line between the end of @fileoverview annotation and the function
> definition.

Done.

https://codereview.chromium.org/23434003/diff/28001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js:9:
PendingChanges = function() {
On 2013/08/28 02:53:35, yoshiki wrote:
> Write the JSdoc of the this function.

Done.

https://codereview.chromium.org/23434003/diff/28001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js:22: *
@param {string} genericFamily The generic family, like "sansserif".
On 2013/08/28 02:53:35, yoshiki wrote:
> Add "@return" annotation.

Done.

https://codereview.chromium.org/23434003/diff/28001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js:34: *
    'defaultFontSize', 'defaultFixedFontSize', or 'minFontSize'.
On 2013/08/28 02:53:35, yoshiki wrote:
> ditto

Done.

https://codereview.chromium.org/23434003/diff/28001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js:45: *
@param {string} font The font to set the setting to, or null to clear it.
On 2013/08/28 02:53:35, yoshiki wrote:
> If this parameter is a null-able string, use "?string" instead of just
"string"
> in type annotation.

Done.

https://codereview.chromium.org/23434003/diff/28001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js:85:
chrome.fontSettings.setDefaultFontSize( { pixelSize: size });
On 2013/08/28 02:53:35, yoshiki wrote:
> chrome.fontSettings.setDefaultFontSize({pixelSize: size});
> 
>
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...

Done.

https://codereview.chromium.org/23434003/diff/28001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js:89:
chrome.fontSettings.setDefaultFixedFontSize( { pixelSize: size });
On 2013/08/28 02:53:35, yoshiki wrote:
> ditto

Done.

https://codereview.chromium.org/23434003/diff/28001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js:93:
chrome.fontSettings.setMinimumFontSize( { pixelSize: size });
On 2013/08/28 02:53:35, yoshiki wrote:
> ditto

Done.

https://codereview.chromium.org/23434003/diff/28001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/api/fontSettings/slider.js (right):

https://codereview.chromium.org/23434003/diff/28001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/slider.js:19: * @param
{function(boolean)} onDrag Drag begin/end handler.
On 2013/08/28 02:53:35, yoshiki wrote:
> Is 'onDrag' and/or 'onChange' are optional parameter? If so, rename them to
> 'opt_onDrag' or 'opt_onChange'
> 
>
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...

Done. Actually I don't need on drag anywhere, so I just deleted it.

https://codereview.chromium.org/23434003/diff/28001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/slider.js:65:
Slider.prototype.getValue = function() {
On 2013/08/28 02:53:35, yoshiki wrote:
> Please write JSdoc.

Done.

https://codereview.chromium.org/23434003/diff/28001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/slider.js:79: var
proportion =(this.input_.value - this.input_.min) /
On 2013/08/28 02:53:35, yoshiki wrote:
> Add a space after "=".

Done.

https://codereview.chromium.org/23434003/diff/28001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/slider.js:86: if
(this.onChange_) {
On 2013/08/28 02:53:35, yoshiki wrote:
> Curly brackets are unnecessary, since there is only a line in the block.

Done.

[hirono, 2013-08-28 08:47:30]

Thank you!

https://codereview.chromium.org/23434003/diff/28001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/api/fontSettings/options.js (right):

https://codereview.chromium.org/23434003/diff/28001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:4: 
Could you add a line 'use strict'; ?
It lets the JavaScript engine warn for unsafe codes.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions_a...

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/api/fontSettings/options.js (right):

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:201: 'Zyyy':
'The quick brown fox jumps over the lazy dog.'
Could you remove the quotations for keys?
The style guide shows we don't need to put quotations except for strange keys.

http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Array_a...

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:225:
advancedFonts.getBoundCallback = function(callback, font, controllable) {
It seems that function.bind can be used instead of defining the function.

[yoshiki, 2013-08-28 08:58:44]

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/api/fontSettings/options.js (right):

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:13: var
advancedFonts = {};
Please add "@namespace" annotation.

/**
 * @namespace
 */
var advancedFonts = {};

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js
(right):

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js:21: }
Add a semicolon after the bracket, since "var = function() {}" is an expression.
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...


And same below.

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js:29: *
@return {string} The pending font setting, like "My Cyrillic SansSerif Font",
s/string/?string/

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js:43: *
@return {number} The pending font size setting in pixels, or null if it
s/number/?number/

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/api/fontSettings/slider.css (right):

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/slider.css:38:
url('../images/slider/slide_bar_center.png') 1x);
(1) -webkit-image-set is not necessary if we don't have hi-dpi-version of image.

(2) quotation mark is not necessary in url(). 
http://google-styleguide.googlecode.com/svn/trunk/htmlcssguide.xml?showone=CS...

You can write just:
  background-image: url(../images/slider/slide_bar_center.png);

And same below.

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/slider.css:100: }
Add a blank line.

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/api/fontSettings/slider.js (right):

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/slider.js:25: var
document = this.container_.ownerDocument;
Could you change the name of 'document' to some other name (like
"containerDocument")? JavaScript has "document" in the global space, so using
same name makes readability wrose.

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/slider.js:82: * value.
Add "@private" anotation.

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/slider.js:91: * Called
when the slider's value changes.
ditto

[falken, 2013-08-29 09:35:43]

Thanks for the thorough reviews! How is this?

https://codereview.chromium.org/23434003/diff/28001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/api/fontSettings/options.js (right):

https://codereview.chromium.org/23434003/diff/28001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:4: 
On 2013/08/28 08:47:30, hirono wrote:
> Could you add a line 'use strict'; ?
> It lets the JavaScript engine warn for unsafe codes.
> 
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions_a...

Done.

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/api/fontSettings/options.js (right):

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:13: var
advancedFonts = {};
On 2013/08/28 08:58:44, yoshiki wrote:
> Please add "@namespace" annotation.
> 
> /**
>  * @namespace
>  */
> var advancedFonts = {};

Done.

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:201: 'Zyyy':
'The quick brown fox jumps over the lazy dog.'
On 2013/08/28 08:47:30, hirono wrote:
> Could you remove the quotations for keys?
> The style guide shows we don't need to put quotations except for strange keys.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Array_a...

Done.

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:225:
advancedFonts.getBoundCallback = function(callback, font, controllable) {
On 2013/08/28 08:47:30, hirono wrote:
> It seems that function.bind can be used instead of defining the function.

I think I can't use that since the second and third parameters are bound, rather
than first and second.

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js
(right):

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js:21: }
On 2013/08/28 08:58:44, yoshiki wrote:
> Add a semicolon after the bracket, since "var = function() {}" is an
expression.
>
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...
> 
> 
> And same below.

Done.

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js:29: *
@return {string} The pending font setting, like "My Cyrillic SansSerif Font",
On 2013/08/28 08:58:44, yoshiki wrote:
> s/string/?string/

Done.

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/pending_changes.js:43: *
@return {number} The pending font size setting in pixels, or null if it
On 2013/08/28 08:58:44, yoshiki wrote:
> s/number/?number/

Done.

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/api/fontSettings/slider.css (right):

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/slider.css:38:
url('../images/slider/slide_bar_center.png') 1x);
On 2013/08/28 08:58:44, yoshiki wrote:
> (1) -webkit-image-set is not necessary if we don't have hi-dpi-version of
image.
> 
> (2) quotation mark is not necessary in url(). 
>
http://google-styleguide.googlecode.com/svn/trunk/htmlcssguide.xml?showone=CS...
> 
> You can write just:
>   background-image: url(../images/slider/slide_bar_center.png);
> 
> And same below.

Done.

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/slider.css:100: }
On 2013/08/28 08:58:44, yoshiki wrote:
> Add a blank line.

Done.

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/api/fontSettings/slider.js (right):

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/slider.js:25: var
document = this.container_.ownerDocument;
On 2013/08/28 08:58:44, yoshiki wrote:
> Could you change the name of 'document' to some other name (like
> "containerDocument")? JavaScript has "document" in the global space, so using
> same name makes readability wrose.

Done.

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/slider.js:82: * value.
On 2013/08/28 08:58:44, yoshiki wrote:
> Add "@private" anotation.

Done.

https://codereview.chromium.org/23434003/diff/47001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/slider.js:91: * Called
when the slider's value changes.
On 2013/08/28 08:58:44, yoshiki wrote:
> ditto

Done.

[yoshiki, 2013-08-29 10:02:46]

https://codereview.chromium.org/23434003/diff/65001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/api/fontSettings/options.js (right):

https://codereview.chromium.org/23434003/diff/65001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:226: *
@return {function} A function of form function(x) which calls
function(string)

https://codereview.chromium.org/23434003/diff/65001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:316:
advancedFonts.getBoundCallback(
advancedFonts.getBoundCallback returns one-argument function, but
advancedFonts.getEffectiveFont received 3-argument function as third argument.
Which is correct?

https://codereview.chromium.org/23434003/diff/65001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:325: * @param
{array} samples An array of HTMLElement objects with sample text that
s/array/Array.<HTMLElement>/

https://codereview.chromium.org/23434003/diff/65001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:414: * @param
{array} fonts The list of fonts on the system.
"array" -> "Array.<{fontId: number, displayName: string}>"

https://codereview.chromium.org/23434003/diff/65001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:441: *
@return {function} A function to be called when the user changes the selected
s/function/function()/

https://codereview.chromium.org/23434003/diff/65001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:508: * @param
{array} sampleTexts An array of elements containing sample text
s/array/Array.{HTMLElement}/

cf. https://developers.google.com/closure/compiler/docs/js-for-compiler#types

https://codereview.chromium.org/23434003/diff/65001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:510: *
@return {function(object) A function that can listen for font size change
"}" is missing after the type.

[falken, 2013-08-30 07:33:17]

Thanks again. Please take another look.

https://codereview.chromium.org/23434003/diff/65001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/api/fontSettings/options.js (right):

https://codereview.chromium.org/23434003/diff/65001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:316:
advancedFonts.getBoundCallback(
On 2013/08/29 10:02:46, yoshiki wrote:
> advancedFonts.getBoundCallback returns one-argument function, but
> advancedFonts.getEffectiveFont received 3-argument function as third argument.
> Which is correct?

I've updated getBoundCallback to clarify. How does it look?

https://codereview.chromium.org/23434003/diff/65001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:325: * @param
{array} samples An array of HTMLElement objects with sample text that
On 2013/08/29 10:02:46, yoshiki wrote:
> s/array/Array.<HTMLElement>/

Done.

https://codereview.chromium.org/23434003/diff/65001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:414: * @param
{array} fonts The list of fonts on the system.
On 2013/08/29 10:02:46, yoshiki wrote:
> "array" -> "Array.<{fontId: number, displayName: string}>"

Done, although fontId is a string.

https://codereview.chromium.org/23434003/diff/65001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:508: * @param
{array} sampleTexts An array of elements containing sample text
On 2013/08/29 10:02:46, yoshiki wrote:
> s/array/Array.{HTMLElement}/
> 
> cf. https://developers.google.com/closure/compiler/docs/js-for-compiler#types

Done. Thanks, I'd missed that.

https://codereview.chromium.org/23434003/diff/65001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:510: *
@return {function(object) A function that can listen for font size change
On 2013/08/29 10:02:46, yoshiki wrote:
> "}" is missing after the type.

Done.

[hirono, 2013-08-30 08:57:22]

https://codereview.chromium.org/23434003/diff/79001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/api/fontSettings/options.js (right):

https://codereview.chromium.org/23434003/diff/79001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:341: return
function(effectiveFont, font, controllable) {
Instead of using closures just for arguments binding, could you use
Function.bind?
It makes codes more simpler. And by using the specific utility, readers can
easily understand intention of the codes.

https://codereview.chromium.org/23434003/diff/79001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:732:
'standard': {
Remove the quotations.

https://codereview.chromium.org/23434003/diff/79001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:764:
'defaultFontSize': {
Remove the quotations.

[yoshiki, 2013-08-30 09:01:36]

Thanks for many handling. LGTM after you address hirono's comments.

[falken, 2013-08-30 10:44:40]

Thanks again. PTAL.

https://codereview.chromium.org/23434003/diff/79001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/api/fontSettings/options.js (right):

https://codereview.chromium.org/23434003/diff/79001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:341: return
function(effectiveFont, font, controllable) {
On 2013/08/30 08:57:22, hirono wrote:
> Instead of using closures just for arguments binding, could you use
> Function.bind?
> It makes codes more simpler. And by using the specific utility, readers can
> easily understand intention of the codes.

That's a good idea. Done.

https://codereview.chromium.org/23434003/diff/79001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:732:
'standard': {
On 2013/08/30 08:57:22, hirono wrote:
> Remove the quotations.

Done.

https://codereview.chromium.org/23434003/diff/79001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:764:
'defaultFontSize': {
On 2013/08/30 08:57:22, hirono wrote:
> Remove the quotations.

Done.

[hirono, 2013-08-30 11:01:35]

Thanks!

https://codereview.chromium.org/23434003/diff/73001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/api/fontSettings/options.js (right):

https://codereview.chromium.org/23434003/diff/73001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:339:
fontSetting, effectiveFont, font, controllable) {
Can we just use Function.bind instead of getBoundCallback by changing the order
of the arguments?

https://codereview.chromium.org/23434003/diff/73001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:443:
advancedFonts.getFontChangeHandler = function(fontList, genericFamily) {
Can we also use Function.bind for the function?

https://codereview.chromium.org/23434003/diff/73001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:481:
advancedFonts.getFontSizeChangedFunc = function(fontSizeKey) {
ditto.

https://codereview.chromium.org/23434003/diff/73001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:514:
function(slider, label, sampleTexts) {
ditto.

[falken, 2013-08-30 13:22:29]

Thanks for these reviews. The code looks a lot better, and I learned a lot about
JavaScript. PTAL.

I also uncovered some bugs this round. I'll have to work out a testing plan for
this extension.

https://codereview.chromium.org/23434003/diff/73001/chrome/common/extensions/...
File chrome/common/extensions/docs/examples/api/fontSettings/options.js (right):

https://codereview.chromium.org/23434003/diff/73001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:339:
fontSetting, effectiveFont, font, controllable) {
On 2013/08/30 11:01:35, hirono wrote:
> Can we just use Function.bind instead of getBoundCallback by changing the
order
> of the arguments?

Done.

https://codereview.chromium.org/23434003/diff/73001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:443:
advancedFonts.getFontChangeHandler = function(fontList, genericFamily) {
On 2013/08/30 11:01:35, hirono wrote:
> Can we also use Function.bind for the function?

Done.

https://codereview.chromium.org/23434003/diff/73001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:481:
advancedFonts.getFontSizeChangedFunc = function(fontSizeKey) {
On 2013/08/30 11:01:35, hirono wrote:
> ditto.

Done.

https://codereview.chromium.org/23434003/diff/73001/chrome/common/extensions/...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:514:
function(slider, label, sampleTexts) {
On 2013/08/30 11:01:35, hirono wrote:
> ditto.

I just got rid of this. It was supposed to smartly refresh only the components
that could be dirtied by the change, but it's good enough to just use refresh().
The font size will rarely change on the browser.

[hirono, 2013-09-02 02:09:52]

On 2013/08/30 13:22:29, falken wrote:
> Thanks for these reviews. The code looks a lot better, and I learned a lot
about
> JavaScript. PTAL.
> 
> I also uncovered some bugs this round. I'll have to work out a testing plan
for
> this extension.
> 
>
https://codereview.chromium.org/23434003/diff/73001/chrome/common/extensions/...
> File chrome/common/extensions/docs/examples/api/fontSettings/options.js
(right):
> 
>
https://codereview.chromium.org/23434003/diff/73001/chrome/common/extensions/...
> chrome/common/extensions/docs/examples/api/fontSettings/options.js:339:
> fontSetting, effectiveFont, font, controllable) {
> On 2013/08/30 11:01:35, hirono wrote:
> > Can we just use Function.bind instead of getBoundCallback by changing the
> order
> > of the arguments?
> 
> Done.
> 
>
https://codereview.chromium.org/23434003/diff/73001/chrome/common/extensions/...
> chrome/common/extensions/docs/examples/api/fontSettings/options.js:443:
> advancedFonts.getFontChangeHandler = function(fontList, genericFamily) {
> On 2013/08/30 11:01:35, hirono wrote:
> > Can we also use Function.bind for the function?
> 
> Done.
> 
>
https://codereview.chromium.org/23434003/diff/73001/chrome/common/extensions/...
> chrome/common/extensions/docs/examples/api/fontSettings/options.js:481:
> advancedFonts.getFontSizeChangedFunc = function(fontSizeKey) {
> On 2013/08/30 11:01:35, hirono wrote:
> > ditto.
> 
> Done.
> 
>
https://codereview.chromium.org/23434003/diff/73001/chrome/common/extensions/...
> chrome/common/extensions/docs/examples/api/fontSettings/options.js:514:
> function(slider, label, sampleTexts) {
> On 2013/08/30 11:01:35, hirono wrote:
> > ditto.
> 
> I just got rid of this. It was supposed to smartly refresh only the components
> that could be dirtied by the change, but it's good enough to just use
refresh().
> The font size will rarely change on the browser.

lgtm! Thank you very much!

[yoshiki, 2013-09-02 02:14:50]

still lgtm with some nits.

https://codereview.chromium.org/23434003/diff/123001/chrome/common/extensions...
File chrome/common/extensions/docs/examples/api/fontSettings/options.html
(right):

https://codereview.chromium.org/23434003/diff/123001/chrome/common/extensions...
chrome/common/extensions/docs/examples/api/fontSettings/options.html:208: <div
style="width: 100%; -webkit-margin-after: 0px">
An unit (px) is not necessary if the value is 0.

You can write:  <div style="width: 100%; -webkit-margin-after: 0;">

https://codereview.chromium.org/23434003/diff/123001/chrome/common/extensions...
chrome/common/extensions/docs/examples/api/fontSettings/options.html:250: <div
style="width: 100%; -webkit-margin-after: 0px">
ditto

https://codereview.chromium.org/23434003/diff/123001/chrome/common/extensions...
File chrome/common/extensions/docs/examples/api/fontSettings/options.js (right):

https://codereview.chromium.org/23434003/diff/123001/chrome/common/extensions...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:230: *   
PendingChanges.getFontSize().
nit: 4 space indent

https://codereview.chromium.org/23434003/diff/123001/chrome/common/extensions...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:232: *   
function(size, controllable). |size| is the effective setting,
ditto

https://codereview.chromium.org/23434003/diff/123001/chrome/common/extensions...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:264: *   
function(font, controllable, effectiveFont). |font| is the setting
ditto

[falken, 2013-09-02 05:15:55]

Thanks!

https://codereview.chromium.org/23434003/diff/123001/chrome/common/extensions...
File chrome/common/extensions/docs/examples/api/fontSettings/options.html
(right):

https://codereview.chromium.org/23434003/diff/123001/chrome/common/extensions...
chrome/common/extensions/docs/examples/api/fontSettings/options.html:208: <div
style="width: 100%; -webkit-margin-after: 0px">
On 2013/09/02 02:14:50, yoshiki wrote:
> An unit (px) is not necessary if the value is 0.
> 
> You can write:  <div style="width: 100%; -webkit-margin-after: 0;">

Done.

https://codereview.chromium.org/23434003/diff/123001/chrome/common/extensions...
chrome/common/extensions/docs/examples/api/fontSettings/options.html:250: <div
style="width: 100%; -webkit-margin-after: 0px">
On 2013/09/02 02:14:50, yoshiki wrote:
> ditto

Done.

https://codereview.chromium.org/23434003/diff/123001/chrome/common/extensions...
File chrome/common/extensions/docs/examples/api/fontSettings/options.js (right):

https://codereview.chromium.org/23434003/diff/123001/chrome/common/extensions...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:230: *   
PendingChanges.getFontSize().
On 2013/09/02 02:14:50, yoshiki wrote:
> nit: 4 space indent

Done.

https://codereview.chromium.org/23434003/diff/123001/chrome/common/extensions...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:232: *   
function(size, controllable). |size| is the effective setting,
On 2013/09/02 02:14:50, yoshiki wrote:
> ditto

Done.

https://codereview.chromium.org/23434003/diff/123001/chrome/common/extensions...
chrome/common/extensions/docs/examples/api/fontSettings/options.js:264: *   
function(font, controllable, effectiveFont). |font| is the setting
On 2013/09/02 02:14:50, yoshiki wrote:
> ditto

Done.

[commit-bot: I haz the power, 2013-09-02 05:16:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/falken@chromium.org/23434003/135001

[commit-bot: I haz the power, 2013-09-02 09:57:30]

Change committed as 220823