Add onFontNameChanged event to Font Settings API

Add onFontNameChanged event to Font Settings API

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

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132408

[falken, 2012-04-12 12:42:39]

mpcomplete: Please review.
bauerb: Can you review preferences-related code in
extension_font_settings_api.cc? As you'll see, I copied some code from
ExtensionPreferenceEventRouter::OnPrefChanged

[Bernhard Bauer, 2012-04-12 13:14:01]

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

https://chromiumcodereview.appspot.com/10008076/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.cc:86: NOTREACHED();
If you're already DCHECKing the return value, you don't need to DCHECK here (and
as a bonus, you can call this method in unit tests without crashing).

https://chromiumcodereview.appspot.com/10008076/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.cc:182:
DCHECK(!incognito);
This DCHECK will always succeed, but that's because you're only observing pref
changes on |profile_->GetPrefs()|, not because incognito prefs don't exist :) In
ExtensionPreferenceEventRouter, we're observing pref changes on the regular and
the incognito profile.

https://chromiumcodereview.appspot.com/10008076/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.cc:202:
ExtensionEventRouter* router = profile_->GetExtensionEventRouter();
I would be very happy if we could pull everything from here on down into a
method that's used by both ExtensionPreferenceEventRouter and
ExtensionFontSettingsEventRouter.

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

https://chromiumcodereview.appspot.com/10008076/diff/1/chrome/common/extensio...
chrome/common/extensions/api/experimental.fontSettings.json:292: "type":
"function",
The "type" attribute is unnecessary for events, and it's not a function in this
case :) (it's also unnecessary for API methods above; you only need it for
parameter lists or attributes of classes like ChromeSetting)

https://chromiumcodereview.appspot.com/10008076/diff/1/chrome/common/extensio...
chrome/common/extensions/api/experimental.fontSettings.json:302: "enum": [
You could pull out this enum into a separate type, so you don't have to copy the
definition everywhere.

[Matt Perry, 2012-04-12 20:31:44]

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

https://chromiumcodereview.appspot.com/10008076/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.cc:224: extension_id,
kOnFontNameChanged, json_args, NULL, GURL());
indent += 2

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

https://chromiumcodereview.appspot.com/10008076/diff/1/chrome/common/extensio...
chrome/common/extensions/api/experimental.fontSettings.json:291: "name":
"onFontNameChanged",
The name of this event is confusing to me. It sounds like the name of the font
has changed. Would something like onFontSettingChanged make more sense? I
realize it wouldn't parallel get/setFontName, unfortunately...

[falken, 2012-04-13 10:33:45]

Thanks for the reviews!

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

https://chromiumcodereview.appspot.com/10008076/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.cc:86: NOTREACHED();
On 2012/04/12 13:14:01, Bernhard Bauer wrote:
> If you're already DCHECKing the return value, you don't need to DCHECK here
(and
> as a bonus, you can call this method in unit tests without crashing).

Good point, thanks. Removed the check.

https://chromiumcodereview.appspot.com/10008076/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.cc:182:
DCHECK(!incognito);
On 2012/04/12 13:14:01, Bernhard Bauer wrote:
> This DCHECK will always succeed, but that's because you're only observing pref
> changes on |profile_->GetPrefs()|, not because incognito prefs don't exist :)
In
> ExtensionPreferenceEventRouter, we're observing pref changes on the regular
and
> the incognito profile.

Ah, I see. I've updated the comment. I think it's good to have |incognito| so it
can be used below, but also want to make clear it will always be false.

https://chromiumcodereview.appspot.com/10008076/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.cc:202:
ExtensionEventRouter* router = profile_->GetExtensionEventRouter();
On 2012/04/12 13:14:01, Bernhard Bauer wrote:
> I would be very happy if we could pull everything from here on down into a
> method that's used by both ExtensionPreferenceEventRouter and
> ExtensionFontSettingsEventRouter.

Done. I moved them to extension_preference_helpers::DispatchEventToExtensions.

I'm a little concerned the EventArgsTransformerInterface mechanism is overkill
for this refactoring, but it also felt bizarre for DispatchEventToExtensions to
reach in and inject the level of control value to the args by itself.

https://chromiumcodereview.appspot.com/10008076/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.cc:224: extension_id,
kOnFontNameChanged, json_args, NULL, GURL());
On 2012/04/12 20:31:44, Matt Perry wrote:
> indent += 2

Done.

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

https://chromiumcodereview.appspot.com/10008076/diff/1/chrome/common/extensio...
chrome/common/extensions/api/experimental.fontSettings.json:291: "name":
"onFontNameChanged",
On 2012/04/12 20:31:44, Matt Perry wrote:
> The name of this event is confusing to me. It sounds like the name of the font
> has changed. Would something like onFontSettingChanged make more sense? I
> realize it wouldn't parallel get/setFontName, unfortunately...

What do you think of changing get/setFontName to just get/setFont? Then the
event can be onFontChanged. I wanted them to be parallel since I plan to add
onDefaultFontSizeChanged, etc, as well.

If that's OK, I'd like to make that change in a second patch, since this one is
getting rather big and it would require changing the example extension as well.

https://chromiumcodereview.appspot.com/10008076/diff/1/chrome/common/extensio...
chrome/common/extensions/api/experimental.fontSettings.json:292: "type":
"function",
On 2012/04/12 13:14:01, Bernhard Bauer wrote:
> The "type" attribute is unnecessary for events, and it's not a function in
this
> case :) (it's also unnecessary for API methods above; you only need it for
> parameter lists or attributes of classes like ChromeSetting)

Done.

https://chromiumcodereview.appspot.com/10008076/diff/1/chrome/common/extensio...
chrome/common/extensions/api/experimental.fontSettings.json:302: "enum": [
On 2012/04/12 13:14:01, Bernhard Bauer wrote:
> You could pull out this enum into a separate type, so you don't have to copy
the
> definition everywhere.

Done, and also moved genericFamily to a new type.

[Bernhard Bauer, 2012-04-13 12:19:31]

http://codereview.chromium.org/10008076/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_font_settings_api.cc (right):

http://codereview.chromium.org/10008076/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_font_settings_api.cc:202:
ExtensionEventRouter* router = profile_->GetExtensionEventRouter();
On 2012/04/13 10:33:46, falken wrote:
> On 2012/04/12 13:14:01, Bernhard Bauer wrote:
> > I would be very happy if we could pull everything from here on down into a
> > method that's used by both ExtensionPreferenceEventRouter and
> > ExtensionFontSettingsEventRouter.
> 
> Done. I moved them to extension_preference_helpers::DispatchEventToExtensions.
> 
> I'm a little concerned the EventArgsTransformerInterface mechanism is overkill
> for this refactoring, but it also felt bizarre for DispatchEventToExtensions
to
> reach in and inject the level of control value to the args by itself.

I don't think that would be a huge problem. This is all part of the preference
system, and preferences have the concept of "level of control", so it wouldn't
be too much of a stretch IMO for DispatchEventToExtensions to inject the level
of control itself.

[Matt Perry, 2012-04-13 18:15:45]

lgtm

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

https://chromiumcodereview.appspot.com/10008076/diff/1/chrome/common/extensio...
chrome/common/extensions/api/experimental.fontSettings.json:291: "name":
"onFontNameChanged",
On 2012/04/13 10:33:46, falken wrote:
> On 2012/04/12 20:31:44, Matt Perry wrote:
> > The name of this event is confusing to me. It sounds like the name of the
font
> > has changed. Would something like onFontSettingChanged make more sense? I
> > realize it wouldn't parallel get/setFontName, unfortunately...
> 
> What do you think of changing get/setFontName to just get/setFont? Then the
> event can be onFontChanged. I wanted them to be parallel since I plan to add
> onDefaultFontSizeChanged, etc, as well.
> 
> If that's OK, I'd like to make that change in a second patch, since this one
is
> getting rather big and it would require changing the example extension as
well.

Perfect, SGTM

[falken, 2012-04-16 03:47:06]

Matt: Thanks!
Bernhard: Please take another look.

http://codereview.chromium.org/10008076/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_font_settings_api.cc (right):

http://codereview.chromium.org/10008076/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_font_settings_api.cc:202:
ExtensionEventRouter* router = profile_->GetExtensionEventRouter();
On 2012/04/13 12:19:31, Bernhard Bauer wrote:
> On 2012/04/13 10:33:46, falken wrote:
> > On 2012/04/12 13:14:01, Bernhard Bauer wrote:
> > > I would be very happy if we could pull everything from here on down into a
> > > method that's used by both ExtensionPreferenceEventRouter and
> > > ExtensionFontSettingsEventRouter.
> > 
> > Done. I moved them to
extension_preference_helpers::DispatchEventToExtensions.
> > 
> > I'm a little concerned the EventArgsTransformerInterface mechanism is
overkill
> > for this refactoring, but it also felt bizarre for DispatchEventToExtensions
> to
> > reach in and inject the level of control value to the args by itself.
> 
> I don't think that would be a huge problem. This is all part of the preference
> system, and preferences have the concept of "level of control", so it wouldn't
> be too much of a stretch IMO for DispatchEventToExtensions to inject the level
> of control itself.

OK, sounds right. I've removed the EventArgsTransformerInterface.

[Bernhard Bauer, 2012-04-16 08:00:40]

http://codereview.chromium.org/10008076/diff/13001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_preference_helpers.cc (right):

http://codereview.chromium.org/10008076/diff/13001/chrome/browser/extensions/...
chrome/browser/extensions/extension_preference_helpers.cc:73: const char*
browser_pref) {
I think I'd prefer passing around std::string's for as long as possible.

http://codereview.chromium.org/10008076/diff/13001/chrome/browser/extensions/...
chrome/browser/extensions/extension_preference_helpers.cc:89: if (browser_pref)
{
If we don't have a caller that passes in NULL, I think we can get rid of the
NULL check here (which also allows us to pass in a std::string then).

[falken, 2012-04-16 09:25:31]

http://codereview.chromium.org/10008076/diff/13001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_preference_helpers.cc (right):

http://codereview.chromium.org/10008076/diff/13001/chrome/browser/extensions/...
chrome/browser/extensions/extension_preference_helpers.cc:89: if (browser_pref)
{
On 2012/04/16 08:00:40, Bernhard Bauer wrote:
> If we don't have a caller that passes in NULL, I think we can get rid of the
> NULL check here (which also allows us to pass in a std::string then).

Done. My idea was to check in case someone wants to use
DispatchEventToExtensions without the injection behavior, but I see we probably
won't need that.

[Bernhard Bauer, 2012-04-16 09:51:43]

Thanks!

LGTM with a nit:

http://codereview.chromium.org/10008076/diff/15004/chrome/browser/extensions/...
File chrome/browser/extensions/extension_font_settings_api.cc (right):

http://codereview.chromium.org/10008076/diff/15004/chrome/browser/extensions/...
chrome/browser/extensions/extension_font_settings_api.cc:203:
pref_key->c_str());
You can pass in the string directly now (same in extension_preference_api.cc).

[falken, 2012-04-16 10:08:08]

Thanks for the review!

http://codereview.chromium.org/10008076/diff/15004/chrome/browser/extensions/...
File chrome/browser/extensions/extension_font_settings_api.cc (right):

http://codereview.chromium.org/10008076/diff/15004/chrome/browser/extensions/...
chrome/browser/extensions/extension_font_settings_api.cc:203:
pref_key->c_str());
On 2012/04/16 09:51:43, Bernhard Bauer wrote:
> You can pass in the string directly now (same in extension_preference_api.cc).

Done.

[commit-bot: I haz the power, 2012-04-16 10:11:25]

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

[commit-bot: I haz the power, 2012-04-16 13:41:38]

Try job failure for 10008076-20004 (retry) (retry) on win_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-04-16 14:10:54]

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

[commit-bot: I haz the power, 2012-04-16 15:58:42]

Change committed as 132408