When a font family pref changes to the empty string, pass it to WebKit.

When a font family pref changes to the empty string, pass it to WebKit.

The empty string means to fall back to the pref for the Common script ("Zyyy").
For example, if chrome.fonts.serif.Cyrl is the empty string, it means to use
chrome.fonts.serif.Zyyy for Cyrillic script. Prefs that are the empty string are
normally not passed to WebKit since there are so many of them that doing so
would cause a perf regression (see r164050 and its revert r164294). Not passing
the pref is normally okay since WebKit does the desired fallback behavior
regardless of whether the empty string is passed or the pref is not passed at
all. But if the pref has changed from non-empty to the empty string, we must
let WebKit know.

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

[falken, 2012-10-29 11:54:28]

Hi Tony,

This is a different approach than http://codereview.chromium.org/11264006/ which
caused a perf regression.

I did a simplistic measuring of running time just using base::Time::Now(), and
it looks like the perf hit comes from ApplyFontsFromMap in WebPreferences. Each
font pref added to WebPreferences results in a WebCore::Settings::set*FontFamily
call which does a hashmap lookup. With 152 scripts * 7 generic families, that's
1064 prefs.

Instead of passing all prefs with the empty string, this change passes a pref
that changed to the empty string.

It's a bit hacky but I couldn't see a much better solution. 
Another thing I considered is to add a clearFontSettings() to WebCore::Settings
and call it before applying the font maps. But Settings is careful to not do a
style recalc unless a font actually changed. This would always cause something
to change and thus always do a style recalc when any web pref changes, which
looks costly.

What do you think?

[tony, 2012-10-29 18:13:52]

LGTM.

This doesn't seem too hacky to me.  In the long run, we may want to make a
specific IPC for setting a single font (or an array of fonts) and just call it
as needed rather than having this as part of the webprefs IPC.

https://codereview.chromium.org/11336008/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/api/font_settings/font_settings_api.cc (right):

https://codereview.chromium.org/11336008/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/font_settings/font_settings_api.cc:169: &script))
{
Nit: I would put &generic_family on the previous line (looks like it'll fit and
that's what is done in the line below).

[falken, 2012-10-30 00:12:38]

Tony: thanks for the review.

Adding OWNERS reviewers:
thakis: chrome/
bauerb: prefs
mpcomplete: extensions

https://codereview.chromium.org/11336008/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/api/font_settings/font_settings_api.cc (right):

https://codereview.chromium.org/11336008/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/font_settings/font_settings_api.cc:169: &script))
{
On 2012/10/29 18:13:52, tony wrote:
> Nit: I would put &generic_family on the previous line (looks like it'll fit
and
> that's what is done in the line below).

Done.

[tfarina, 2012-10-30 01:22:19]

http://codereview.chromium.org/11336008/diff/3007/chrome/common/pref_names_ut...
File chrome/common/pref_names_util.h (right):

http://codereview.chromium.org/11336008/diff/3007/chrome/common/pref_names_ut...
chrome/common/pref_names_util.h:1: // Copyright (c) 2010 The Chromium Authors.
All rights reserved.
2010 -> 2012

http://codereview.chromium.org/11336008/diff/3007/chrome/common/pref_names_ut...
chrome/common/pref_names_util.h:10: namespace pref_names_util {
I'm not sure what is the policy for chrome/common/ namespace.

May be that is fine, may be not :/

[Nico, 2012-10-30 04:24:28]

lgtm with comments

https://codereview.chromium.org/11336008/diff/3007/chrome/browser/ui/prefs/pr...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/11336008/diff/3007/chrome/browser/ui/prefs/pr...
chrome/browser/ui/prefs/prefs_tab_helper.cc:356:
WebPreferences::ScriptFontFamilyMap* map;
= NULL

Else it won't be uninitialized in the NOTREACHED() case and will potentially
lead to tricky bugs. (NOTREACHED is only DCHECK)

https://codereview.chromium.org/11336008/diff/3007/chrome/common/pref_names_u...
File chrome/common/pref_names_util.cc (right):

https://codereview.chromium.org/11336008/diff/3007/chrome/common/pref_names_u...
chrome/common/pref_names_util.cc:9: namespace {
const has implicit internal linkage, no need for an unnamed namespace

https://codereview.chromium.org/11336008/diff/3007/chrome/common/pref_names_u...
chrome/common/pref_names_util.cc:11: const char kWebKitFontPrefPrefix[] =
"webkit.webprefs.fonts.";
Add a comment to pref_names.cc where the webkit.webprefs.fonts are defined that
says "if you change this you also need to change the prefix in
prref_names_util.cc"

https://codereview.chromium.org/11336008/diff/3007/chrome/common/pref_names_u...
chrome/common/pref_names_util.cc:32: }
no test for this?

[falken, 2012-10-30 07:01:34]

Thanks for the reviews.

http://codereview.chromium.org/11336008/diff/3007/chrome/browser/ui/prefs/pre...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

http://codereview.chromium.org/11336008/diff/3007/chrome/browser/ui/prefs/pre...
chrome/browser/ui/prefs/prefs_tab_helper.cc:356:
WebPreferences::ScriptFontFamilyMap* map;
On 2012/10/30 04:24:29, Nico wrote:
> = NULL
> 
> Else it won't be uninitialized in the NOTREACHED() case and will potentially
> lead to tricky bugs. (NOTREACHED is only DCHECK)

Done.

http://codereview.chromium.org/11336008/diff/3007/chrome/common/pref_names_ut...
File chrome/common/pref_names_util.cc (right):

http://codereview.chromium.org/11336008/diff/3007/chrome/common/pref_names_ut...
chrome/common/pref_names_util.cc:9: namespace {
On 2012/10/30 04:24:29, Nico wrote:
> const has implicit internal linkage, no need for an unnamed namespace

Done.

http://codereview.chromium.org/11336008/diff/3007/chrome/common/pref_names_ut...
chrome/common/pref_names_util.cc:11: const char kWebKitFontPrefPrefix[] =
"webkit.webprefs.fonts.";
On 2012/10/30 04:24:29, Nico wrote:
> Add a comment to pref_names.cc where the webkit.webprefs.fonts are defined
that
> says "if you change this you also need to change the prefix in
> prref_names_util.cc"

Done. I also rearranged the pref orders to make a bit more sense.

http://codereview.chromium.org/11336008/diff/3007/chrome/common/pref_names_ut...
chrome/common/pref_names_util.cc:32: }
On 2012/10/30 04:24:29, Nico wrote:
> no test for this?

I added a unit test.

This was refactored out of the font settings API code, so it's also used within
the ExtensionApiTest.FontSetting* browser tests.

http://codereview.chromium.org/11336008/diff/3007/chrome/common/pref_names_ut...
File chrome/common/pref_names_util.h (right):

http://codereview.chromium.org/11336008/diff/3007/chrome/common/pref_names_ut...
chrome/common/pref_names_util.h:1: // Copyright (c) 2010 The Chromium Authors.
All rights reserved.
On 2012/10/30 01:22:19, tfarina wrote:
> 2010 -> 2012

Done.

[tony, 2012-10-30 17:13:15]

https://codereview.chromium.org/11336008/diff/1008/chrome/common/pref_names_u...
File chrome/common/pref_names_util.h (right):

https://codereview.chromium.org/11336008/diff/1008/chrome/common/pref_names_u...
chrome/common/pref_names_util.h:10: namespace pref_names_util {
Oh yeah, this should be in the chrome or chrome_common namespace.

[Nico, 2012-10-30 17:16:11]

https://codereview.chromium.org/11336008/diff/1008/chrome/common/pref_names_u...
File chrome/common/pref_names_util.h (right):

https://codereview.chromium.org/11336008/diff/1008/chrome/common/pref_names_u...
chrome/common/pref_names_util.h:10: namespace pref_names_util {
On 2012/10/30 17:13:16, tony wrote:
> Oh yeah, this should be in the chrome or chrome_common namespace.

Really? From jam's content namespace mail: "per the namespace convention we have
for Chromium (namely, that all modules should be in a namespace except for
chrome)"

[Matt Perry, 2012-10-30 20:28:16]

extensions lgtm

[tony, 2012-10-30 20:53:12]

https://codereview.chromium.org/11336008/diff/1008/chrome/common/pref_names_u...
File chrome/common/pref_names_util.h (right):

https://codereview.chromium.org/11336008/diff/1008/chrome/common/pref_names_u...
chrome/common/pref_names_util.h:10: namespace pref_names_util {
On 2012/10/30 17:16:12, Nico wrote:
> On 2012/10/30 17:13:16, tony wrote:
> > Oh yeah, this should be in the chrome or chrome_common namespace.
> 
> Really? From jam's content namespace mail: "per the namespace convention we
have
> for Chromium (namely, that all modules should be in a namespace except for
> chrome)"

That seems like an argument to not have a namespace at all? I guess it would be
fine to not have a namespace at all, but that seems worse to me.

If we want this to be in a namespace, then it should be named based on the path
like we do everywhere else.

[Bernhard Bauer, 2012-10-30 20:57:51]

On 2012/10/30 20:53:12, tony wrote:
>
https://codereview.chromium.org/11336008/diff/1008/chrome/common/pref_names_u...
> File chrome/common/pref_names_util.h (right):
> 
>
https://codereview.chromium.org/11336008/diff/1008/chrome/common/pref_names_u...
> chrome/common/pref_names_util.h:10: namespace pref_names_util {
> On 2012/10/30 17:16:12, Nico wrote:
> > On 2012/10/30 17:13:16, tony wrote:
> > > Oh yeah, this should be in the chrome or chrome_common namespace.
> > 
> > Really? From jam's content namespace mail: "per the namespace convention we
> have
> > for Chromium (namely, that all modules should be in a namespace except for
> > chrome)"
> 
> That seems like an argument to not have a namespace at all? I guess it would
be
> fine to not have a namespace at all, but that seems worse to me.
> 
> If we want this to be in a namespace, then it should be named based on the
path
> like we do everywhere else.

Isn't putting it in a namespace based on the filename equivalent to putting it
into a class based on the filename (which we usually do), just with a different
naming scheme?

https://codereview.chromium.org/11336008/diff/1008/chrome/browser/ui/prefs/pr...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/11336008/diff/1008/chrome/browser/ui/prefs/pr...
chrome/browser/ui/prefs/prefs_tab_helper.cc:373: (*map)[script] =
UTF8ToUTF16(pref_value);
If somebody passes in an invalid value for |generic_family|, you will crash here
anyway, so I'm not really sure the NOTREACHED is needed.

https://codereview.chromium.org/11336008/diff/1008/chrome/browser/ui/prefs/pr...
chrome/browser/ui/prefs/prefs_tab_helper.cc:611: // means to use
chrome.fonts.serif.Zyyy for Cyrllic script. Prefs that are the
Nit: "Cyrillic script"

https://codereview.chromium.org/11336008/diff/1008/chrome/common/pref_names_u...
File chrome/common/pref_names_util_unittest.cc (right):

https://codereview.chromium.org/11336008/diff/1008/chrome/common/pref_names_u...
chrome/common/pref_names_util_unittest.cc:12: {
Nit: brace on previous line

https://codereview.chromium.org/11336008/diff/1008/chrome/common/pref_names_u...
chrome/common/pref_names_util_unittest.cc:23:
EXPECT_TRUE(pref_names_util::ParseFontNamePrefPath(path, &generic_family,
It doesn't really make a difference, but I think you want to switch EXPECT and
ASSERT here. If parsing the pref path fails, you know that the strings are going
to be empty, whereas you don't need to immediately return if |generic_family|
has the wrong value.

[Nico, 2012-10-30 20:57:59]

On Tue, Oct 30, 2012 at 1:53 PM,  <tony@chromium.org> wrote:
>
>
https://codereview.chromium.org/11336008/diff/1008/chrome/common/pref_names_u...
> File chrome/common/pref_names_util.h (right):
>
>
https://codereview.chromium.org/11336008/diff/1008/chrome/common/pref_names_u...
> chrome/common/pref_names_util.h:10: namespace pref_names_util {
> On 2012/10/30 17:16:12, Nico wrote:
>>
>> On 2012/10/30 17:13:16, tony wrote:
>> > Oh yeah, this should be in the chrome or chrome_common namespace.
>
>
>> Really? From jam's content namespace mail: "per the namespace
>
> convention we have
>>
>> for Chromium (namely, that all modules should be in a namespace except
>
> for
>>
>> chrome)"
>
>
> That seems like an argument to not have a namespace at all? I guess it
> would be fine to not have a namespace at all, but that seems worse to
> me.
>
> If we want this to be in a namespace, then it should be named based on
> the path like we do everywhere else.

Current state of the code base:
http://code.google.com/p/chromium/source/search?q=file%3Abrowser+%5Enamespace...

Some stuff is feature-based (win_util, about_flags, platform_util,
extensions, gpu_util, protector, disposition_utils, browser_util,
chrome_page_zoom, browser_shutdown), one path-based
(chrome_browser_net). Precedent at the moment seems to be "not path
based".

If you feel that this is important enough to warrant a rule, feel free
to reach consensus for something on chromium-dev. *shrug*

[tony, 2012-10-30 21:16:56]

On 2012/10/30 20:57:59, Nico wrote:
> Some stuff is feature-based (win_util, about_flags, platform_util,
> extensions, gpu_util, protector, disposition_utils, browser_util,
> chrome_page_zoom, browser_shutdown), one path-based
> (chrome_browser_net). Precedent at the moment seems to be "not path
> based".

In chrome/browser:
git gs "namespace [a-z]" | grep \{ | cut -d: -f3 | sort | uniq -c | sort -n

    134 namespace history {
    148 namespace gfx {
    149 namespace views {
    170 namespace browser_sync {
    201 namespace policy {
    287 namespace base {
    297 namespace chrome {
    413 namespace content {
    635 namespace chromeos {
    671 namespace extensions {

Using the path seems to be the most common.

> If you feel that this is important enough to warrant a rule, feel free
> to reach consensus for something on chromium-dev. *shrug*

The style guide says, "With named namespaces, choose the name based on the
project, and possibly its path."  I guess pref_names_util would fall under
"possibly its path".

Anyway, I'm not opposed to it being named pref_names_util, it just seems less
consistent with the rest of src/chrome/.

[falken, 2012-10-31 06:27:59]

Hi Bernhard, please take another look.

As for the namespace, looking at other util and helper files in chrome/common,
most seem to use the file name as the namespace. So, I think I'll keep it as
pref_names_util.

https://codereview.chromium.org/11336008/diff/1008/chrome/browser/ui/prefs/pr...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/11336008/diff/1008/chrome/browser/ui/prefs/pr...
chrome/browser/ui/prefs/prefs_tab_helper.cc:373: (*map)[script] =
UTF8ToUTF16(pref_value);
On 2012/10/30 20:57:51, Bernhard Bauer wrote:
> If somebody passes in an invalid value for |generic_family|, you will crash
here
> anyway, so I'm not really sure the NOTREACHED is needed.

Maybe it's useful as documentation? I've added a simple error message to the
NOTREACHED.

https://codereview.chromium.org/11336008/diff/1008/chrome/browser/ui/prefs/pr...
chrome/browser/ui/prefs/prefs_tab_helper.cc:611: // means to use
chrome.fonts.serif.Zyyy for Cyrllic script. Prefs that are the
On 2012/10/30 20:57:51, Bernhard Bauer wrote:
> Nit: "Cyrillic script"

Done.

https://codereview.chromium.org/11336008/diff/1008/chrome/common/pref_names_u...
File chrome/common/pref_names_util_unittest.cc (right):

https://codereview.chromium.org/11336008/diff/1008/chrome/common/pref_names_u...
chrome/common/pref_names_util_unittest.cc:12: {
On 2012/10/30 20:57:51, Bernhard Bauer wrote:
> Nit: brace on previous line

Done.

https://codereview.chromium.org/11336008/diff/1008/chrome/common/pref_names_u...
chrome/common/pref_names_util_unittest.cc:23:
EXPECT_TRUE(pref_names_util::ParseFontNamePrefPath(path, &generic_family,
On 2012/10/30 20:57:51, Bernhard Bauer wrote:
> It doesn't really make a difference, but I think you want to switch EXPECT and
> ASSERT here. If parsing the pref path fails, you know that the strings are
going
> to be empty, whereas you don't need to immediately return if |generic_family|
> has the wrong value.

Done.

[Bernhard Bauer, 2012-10-31 06:33:01]

lgtm

[commit-bot: I haz the power, 2012-10-31 06:37:05]

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

[commit-bot: I haz the power, 2012-10-31 08:34:22]

Change committed as 165125