Add getDefaultFontSize and setDefaultFontSize to Font Settings Extension API.

Add getDefaultFontSize and setDefaultFontSize to Font Settings Extension API.

BUG=114148
TEST=manually and browser_tests --gtest_filter=ExtensionApiTest.FontSettings

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

[falken, 2012-03-23 14:08:04]

Hi Matt, PTAL.

[Matt Perry, 2012-03-23 18:32:53]

lgtm

https://chromiumcodereview.appspot.com/9836036/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/extension_font_settings_api.cc (right):

https://chromiumcodereview.appspot.com/9836036/diff/1/chrome/browser/extensio...
chrome/browser/extensions/extension_font_settings_api.cc:149: DictionaryValue*
result = new DictionaryValue();
why not just result_.reset(new DictionaryValue())

https://chromiumcodereview.appspot.com/9836036/diff/1/chrome/common/extension...
File chrome/common/extensions/api/experimental.fontSettings.json (right):

https://chromiumcodereview.appspot.com/9836036/diff/1/chrome/common/extension...
chrome/common/extensions/api/experimental.fontSettings.json:137: "description":
"This parameter is currently unused."
Just remove this param if its not used. Do you have plans to make use of it?

[falken, 2012-03-24 04:27:13]

Thanks for the review.

https://chromiumcodereview.appspot.com/9836036/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/extension_font_settings_api.cc (right):

https://chromiumcodereview.appspot.com/9836036/diff/1/chrome/browser/extensio...
chrome/browser/extensions/extension_font_settings_api.cc:149: DictionaryValue*
result = new DictionaryValue();
On 2012/03/23 18:32:53, Matt Perry wrote:
> why not just result_.reset(new DictionaryValue())

That would require the next line to be rather unwieldy, no?
  static_cast<DictionaryValue*>(result_.get())->SetInteger(kPixelSizeKey, size);

https://chromiumcodereview.appspot.com/9836036/diff/1/chrome/common/extension...
File chrome/common/extensions/api/experimental.fontSettings.json (right):

https://chromiumcodereview.appspot.com/9836036/diff/1/chrome/common/extension...
chrome/common/extensions/api/experimental.fontSettings.json:137: "description":
"This parameter is currently unused."
On 2012/03/23 18:32:53, Matt Perry wrote:
> Just remove this param if its not used. Do you have plans to make use of it?

Yes, I think eventually we want the font size prefs to also be per-script, at
least for minimum font size (http://crbug.com/17078). I figure reserving this
parameter would make it less disruptive to extend the API if that happens.

[Matt Perry, 2012-03-26 18:31:18]

https://chromiumcodereview.appspot.com/9836036/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/extension_font_settings_api.cc (right):

https://chromiumcodereview.appspot.com/9836036/diff/1/chrome/browser/extensio...
chrome/browser/extensions/extension_font_settings_api.cc:149: DictionaryValue*
result = new DictionaryValue();
On 2012/03/24 04:27:13, falken wrote:
> On 2012/03/23 18:32:53, Matt Perry wrote:
> > why not just result_.reset(new DictionaryValue())
> 
> That would require the next line to be rather unwieldy, no?
>   static_cast<DictionaryValue*>(result_.get())->SetInteger(kPixelSizeKey,
size);

Ah, I forgot result_ was not a DictionaryValue. Nevermind!

https://chromiumcodereview.appspot.com/9836036/diff/1/chrome/common/extension...
File chrome/common/extensions/api/experimental.fontSettings.json (right):

https://chromiumcodereview.appspot.com/9836036/diff/1/chrome/common/extension...
chrome/common/extensions/api/experimental.fontSettings.json:137: "description":
"This parameter is currently unused."
On 2012/03/24 04:27:13, falken wrote:
> On 2012/03/23 18:32:53, Matt Perry wrote:
> > Just remove this param if its not used. Do you have plans to make use of it?
> 
> Yes, I think eventually we want the font size prefs to also be per-script, at
> least for minimum font size (http://crbug.com/17078). I figure reserving this
> parameter would make it less disruptive to extend the API if that happens.

OK, makes sense. Do you foresee it being optional? (i.e. omitting it means "get
font size for all fonts" ?) If so, I'd make it optional now. Otherwise, this is
good.

[falken, 2012-03-27 02:04:11]

http://codereview.chromium.org/9836036/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/experimental.fontSettings.json (right):

http://codereview.chromium.org/9836036/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/experimental.fontSettings.json:137: "description":
"This parameter is currently unused."
On 2012/03/26 18:31:18, Matt Perry wrote:
> On 2012/03/24 04:27:13, falken wrote:
> > On 2012/03/23 18:32:53, Matt Perry wrote:
> > > Just remove this param if its not used. Do you have plans to make use of
it?
> > 
> > Yes, I think eventually we want the font size prefs to also be per-script,
at
> > least for minimum font size (http://crbug.com/17078). I figure reserving
this
> > parameter would make it less disruptive to extend the API if that happens.
> 
> OK, makes sense. Do you foresee it being optional? (i.e. omitting it means
"get
> font size for all fonts" ?) If so, I'd make it optional now. Otherwise, this
is
> good.

OK, I'll make it optional in the next patch
(http://codereview.chromium.org/9853013/). I thought it would have to be
specified whenever the callback is specified (due to the order of parameters),
but it seems to work.