Migrate WebKit "global script" font prefs.

Migrate WebKit "global script" font prefs.

The effect is to migrate a pref like
  webkit.webprefs.global.standard_font_family to webkit.webprefs.fonts.standard.Zyyy

This has two motivations:

1) Undo the migration of font prefs to the "global" (as opposed to per-tab)
namespace, as planned here: <http://codereview.chromium.org/9838050>;. There are still
more prefs to move out of "global" after this patch.

2) Move the "global script" (as opposed to per-script) font prefs into the
per-script font maps, under the script code "Zyyy" (the ISO 15924 code for
the "Common" script). This is consistent with WebKit-side settings and should
simplify the code overall as we no longer have to special case the global
script vs the per-script fonts.

TBR=gene for chrome/browser/printing
BUG=123812
TEST=browser_tests --gtest_filter=PrefsTab* and ExtensionApiTest.Font*

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

[falken, 2012-04-17 12:44:00]

tony: Can you review please?
jshin, bauerb, mnaganov: FYI

[falken, 2012-04-18 08:08:01]

darin: Could you take a look, particularly the changes under webkit/? I think
tony@ is away.

[darin (slow to review), 2012-04-23 17:00:34]

I think Tony should review this.  I don't have enough context to be helpful.

http://codereview.chromium.org/10107014/diff/11028/webkit/glue/webpreferences.cc
File webkit/glue/webpreferences.cc (right):

http://codereview.chromium.org/10107014/diff/11028/webkit/glue/webpreferences...
webkit/glue/webpreferences.cc:29: const char kWebKitCommonScript[] = "Zyyy";
How was this string literal chosen?  What is the significance of this name?

http://codereview.chromium.org/10107014/diff/11028/webkit/tools/test_shell/te...
File webkit/tools/test_shell/test_shell.cc (right):

http://codereview.chromium.org/10107014/diff/11028/webkit/tools/test_shell/te...
webkit/tools/test_shell/test_shell.cc:81: const char kWebKitCommonScript[] =
"Zyyy";
it seems unfortunate for this magic string to be repeated in more than one .cc
file.

[falken, 2012-04-24 01:27:10]

Darin, thanks for taking a look. I'll wait for Tony's review.

http://codereview.chromium.org/10107014/diff/11028/webkit/glue/webpreferences.cc
File webkit/glue/webpreferences.cc (right):

http://codereview.chromium.org/10107014/diff/11028/webkit/glue/webpreferences...
webkit/glue/webpreferences.cc:29: const char kWebKitCommonScript[] = "Zyyy";
On 2012/04/23 17:00:35, darin wrote:
> How was this string literal chosen?  What is the significance of this name?

The font maps are from ISO 15924 script code to font. Zyyy is the code for
undetermined script aka Common, and is the default used on WebKit's side to
get/set a font setting when no script is specified.

http://codereview.chromium.org/10107014/diff/11028/webkit/tools/test_shell/te...
File webkit/tools/test_shell/test_shell.cc (right):

http://codereview.chromium.org/10107014/diff/11028/webkit/tools/test_shell/te...
webkit/tools/test_shell/test_shell.cc:81: const char kWebKitCommonScript[] =
"Zyyy";
On 2012/04/23 17:00:35, darin wrote:
> it seems unfortunate for this magic string to be repeated in more than one .cc
> file.

This patch adds kWebKitCommonScript in pref_names.h, but I'm not sure whether
it's OK for ppapi/shared_impl and test_shell to depend on chrome/common.

[darin (slow to review), 2012-04-24 03:14:46]

On Mon, Apr 23, 2012 at 6:27 PM, <falken@chromium.org> wrote:

> Darin, thanks for taking a look. I'll wait for Tony's review.
>
>
>
> http://codereview.chromium.**org/10107014/diff/11028/**
>
webkit/glue/webpreferences.cc<http://codereview.chromium.org/10107014/diff/11028/webkit/glue/webpreferences.cc>
> File webkit/glue/webpreferences.cc (right):
>
> http://codereview.chromium.**org/10107014/diff/11028/**
>
webkit/glue/webpreferences.cc#**newcode29<http://codereview.chromium.org/10107014/diff/11028/webkit/glue/webpreferences.cc#newcode29>
> webkit/glue/webpreferences.cc:**29: const char kWebKitCommonScript[] =
> "Zyyy";
> On 2012/04/23 17:00:35, darin wrote:
>
>> How was this string literal chosen?  What is the significance of this
>>
> name?
>
> The font maps are from ISO 15924 script code to font. Zyyy is the code
> for undetermined script aka Common, and is the default used on WebKit's
> side to get/set a font setting when no script is specified.
>
>
> http://codereview.chromium.**org/10107014/diff/11028/**
>
webkit/tools/test_shell/test_**shell.cc<http://codereview.chromium.org/10107014/diff/11028/webkit/tools/test_shell/test_shell.cc>
> File webkit/tools/test_shell/test_**shell.cc (right):
>
> http://codereview.chromium.**org/10107014/diff/11028/**
>
webkit/tools/test_shell/test_**shell.cc#newcode81<http://codereview.chromium.org/10107014/diff/11028/webkit/tools/test_shell/test_shell.cc#newcode81>
> webkit/tools/test_shell/test_**shell.cc:81: const char
> kWebKitCommonScript[] = "Zyyy";
> On 2012/04/23 17:00:35, darin wrote:
>
>> it seems unfortunate for this magic string to be repeated in more than
>>
> one .cc
>
>> file.
>>
>
> This patch adds kWebKitCommonScript in pref_names.h, but I'm not sure
> whether it's OK for ppapi/shared_impl and test_shell to depend on
> chrome/common.
>

It is not OK for them to have that dependency.  Hmm...

This "Zyyy" string is specific to WebKit?  We probably don't want
ppapi/shared_impl
to include a header from src/webkit either.


>
>
http://codereview.chromium.**org/10107014/<http://codereview.chromium.org/101...
>

[falken, 2012-04-24 04:57:35]

On 2012/04/24 03:14:46, darin wrote:
> On Mon, Apr 23, 2012 at 6:27 PM, <mailto:falken@chromium.org> wrote:
> 
> > Darin, thanks for taking a look. I'll wait for Tony's review.
> >
> >
> >
> > http://codereview.chromium.**org/10107014/diff/11028/**
> >
>
webkit/glue/webpreferences.cc<http://codereview.chromium.org/10107014/diff/11028/webkit/glue/webpreferences.cc>
> > File webkit/glue/webpreferences.cc (right):
> >
> > http://codereview.chromium.**org/10107014/diff/11028/**
> >
>
webkit/glue/webpreferences.cc#**newcode29<http://codereview.chromium.org/10107014/diff/11028/webkit/glue/webpreferences.cc#newcode29>
> > webkit/glue/webpreferences.cc:**29: const char kWebKitCommonScript[] =
> > "Zyyy";
> > On 2012/04/23 17:00:35, darin wrote:
> >
> >> How was this string literal chosen?  What is the significance of this
> >>
> > name?
> >
> > The font maps are from ISO 15924 script code to font. Zyyy is the code
> > for undetermined script aka Common, and is the default used on WebKit's
> > side to get/set a font setting when no script is specified.
> >
> >
> > http://codereview.chromium.**org/10107014/diff/11028/**
> >
>
webkit/tools/test_shell/test_**shell.cc<http://codereview.chromium.org/10107014/diff/11028/webkit/tools/test_shell/test_shell.cc>
> > File webkit/tools/test_shell/test_**shell.cc (right):
> >
> > http://codereview.chromium.**org/10107014/diff/11028/**
> >
>
webkit/tools/test_shell/test_**shell.cc#newcode81<http://codereview.chromium.org/10107014/diff/11028/webkit/tools/test_shell/test_shell.cc#newcode81>
> > webkit/tools/test_shell/test_**shell.cc:81: const char
> > kWebKitCommonScript[] = "Zyyy";
> > On 2012/04/23 17:00:35, darin wrote:
> >
> >> it seems unfortunate for this magic string to be repeated in more than
> >>
> > one .cc
> >
> >> file.
> >>
> >
> > This patch adds kWebKitCommonScript in pref_names.h, but I'm not sure
> > whether it's OK for ppapi/shared_impl and test_shell to depend on
> > chrome/common.
> >
> 
> It is not OK for them to have that dependency.  Hmm...
> 
> This "Zyyy" string is specific to WebKit? 

Kind of. This patch would also rename Chrome-side prefs like
"webkit.webprefs.global.standard_font_family" to
"webkit.webprefs.fonts.standard.Zyyy" so "Zyyy" would also be part of Chrome.

Perhaps a better approach would be to keep webkit/ as is and special case the
Chrome code to access WebPreferences.*_font_family instead of
WebPreferences.*_font_map for the "*.Zyyy" prefs. Then we don't need to spread
"Zyyy" around, but don't get the symmetry with WebKit settings.

> We probably don't want
> ppapi/shared_impl
> to include a header from src/webkit either.

Here the patch is just moving an existing webkit/glue/webpreferences.h include
from the .cc to the .h file.

[tony, 2012-04-27 17:22:18]

Can we get the Zyyy constant from ICU?

http://codereview.chromium.org/10107014/diff/11028/chrome/browser/ui/prefs/pr...
File chrome/browser/ui/prefs/prefs_tab_helper_browsertest2.cc (right):

http://codereview.chromium.org/10107014/diff/11028/chrome/browser/ui/prefs/pr...
chrome/browser/ui/prefs/prefs_tab_helper_browsertest2.cc:55:
IN_PROC_BROWSER_TEST_F(PrefsTabHelperBrowserTest2, GlobalPrefsAreMigrated) {
Rather than making a separate test file, it seems like you could make this a
separate test case in prefs_tab_helper_browsertest.cc to increase code sharing.

http://codereview.chromium.org/10107014/diff/11028/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/10107014/diff/11028/chrome/common/pref_names.h...
chrome/common/pref_names.h:39: // Obselete keys, kept only for migration code to
the new keys.
Can you file a bug about removing the migration code at some point in the future
and include a link here?

http://codereview.chromium.org/10107014/diff/11028/webkit/glue/webpreferences.cc
File webkit/glue/webpreferences.cc (right):

http://codereview.chromium.org/10107014/diff/11028/webkit/glue/webpreferences...
webkit/glue/webpreferences.cc:117: ASCIIToUTF16("");  // Not sure what to use on
Windows
Impact is the default on Windows.

[falken, 2012-05-09 06:48:08]

Thanks for the review!

Good idea to use ICU to get Zyyy, I've changed it to do so.

http://codereview.chromium.org/10107014/diff/11028/chrome/browser/ui/prefs/pr...
File chrome/browser/ui/prefs/prefs_tab_helper_browsertest2.cc (right):

http://codereview.chromium.org/10107014/diff/11028/chrome/browser/ui/prefs/pr...
chrome/browser/ui/prefs/prefs_tab_helper_browsertest2.cc:55:
IN_PROC_BROWSER_TEST_F(PrefsTabHelperBrowserTest2, GlobalPrefsAreMigrated) {
On 2012/04/27 17:22:18, tony wrote:
> Rather than making a separate test file, it seems like you could make this a
> separate test case in prefs_tab_helper_browsertest.cc to increase code
sharing.

I think I need the separate file since it uses a different Preferences file in
SetupDataDirectory.

http://codereview.chromium.org/10107014/diff/11028/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/10107014/diff/11028/chrome/common/pref_names.h...
chrome/common/pref_names.h:39: // Obselete keys, kept only for migration code to
the new keys.
On 2012/04/27 17:22:18, tony wrote:
> Can you file a bug about removing the migration code at some point in the
future
> and include a link here?

Done.

http://codereview.chromium.org/10107014/diff/11028/webkit/glue/webpreferences.cc
File webkit/glue/webpreferences.cc (right):

http://codereview.chromium.org/10107014/diff/11028/webkit/glue/webpreferences...
webkit/glue/webpreferences.cc:117: ASCIIToUTF16("");  // Not sure what to use on
Windows
On 2012/04/27 17:22:18, tony wrote:
> Impact is the default on Windows.

Done.

[tony, 2012-05-09 17:11:15]

LGTM

http://codereview.chromium.org/10107014/diff/31031/chrome/browser/ui/prefs/pr...
File chrome/browser/ui/prefs/prefs_tab_helper_browsertest2.cc (right):

http://codereview.chromium.org/10107014/diff/31031/chrome/browser/ui/prefs/pr...
chrome/browser/ui/prefs/prefs_tab_helper_browsertest2.cc:30:
.AppendASCII("webkit_global_reverse_migration")
E.g., you could have PrefsTabHelperBrowserTest2 inherit from
PrefsTabHelperBrowserTest and have the path to the preferences file be a virtual
function that you override.  The rest of SetUpUserDataDirectory() would then be
shared.

http://codereview.chromium.org/10107014/diff/31031/chrome/common/extensions/d...
File chrome/common/extensions/docs/samples.json (right):

http://codereview.chromium.org/10107014/diff/31031/chrome/common/extensions/d...
chrome/common/extensions/docs/samples.json:178:
"chrome.experimental.serial.getPorts":
"experimental.serial.html#method-getPorts",
Is this change intentional?

http://codereview.chromium.org/10107014/diff/31031/ppapi/shared_impl/private/...
File ppapi/shared_impl/private/ppb_browser_font_trusted_shared.cc (right):

http://codereview.chromium.org/10107014/diff/31031/ppapi/shared_impl/private/...
ppapi/shared_impl/private/ppb_browser_font_trusted_shared.cc:42: const char
*kCommonScript = uscript_getShortName(USCRIPT_COMMON);
Nit: char*, not char *.

http://codereview.chromium.org/10107014/diff/31031/ppapi/shared_impl/private/...
File ppapi/shared_impl/private/ppb_font_shared.cc (right):

http://codereview.chromium.org/10107014/diff/31031/ppapi/shared_impl/private/...
ppapi/shared_impl/private/ppb_font_shared.cc:45: const char *kCommonScript =
uscript_getShortName(USCRIPT_COMMON);
Nit: char*

http://codereview.chromium.org/10107014/diff/31031/webkit/glue/webpreferences.cc
File webkit/glue/webpreferences.cc (right):

http://codereview.chromium.org/10107014/diff/31031/webkit/glue/webpreferences...
webkit/glue/webpreferences.cc:30: const char* kCommonScript =
uscript_getShortName(USCRIPT_COMMON);
This one is correct :)

[falken, 2012-05-11 11:04:14]

Tony, thanks for the review!

Can the following OWNERS please review?

viettrungluu: ppapi
gene: cloud_print
pkasting: browser/ui
mpcomplete: extensions
jschuh: content/common
estade: options2

http://codereview.chromium.org/10107014/diff/31031/chrome/browser/ui/prefs/pr...
File chrome/browser/ui/prefs/prefs_tab_helper_browsertest2.cc (right):

http://codereview.chromium.org/10107014/diff/31031/chrome/browser/ui/prefs/pr...
chrome/browser/ui/prefs/prefs_tab_helper_browsertest2.cc:30:
.AppendASCII("webkit_global_reverse_migration")
On 2012/05/09 17:11:15, tony wrote:
> E.g., you could have PrefsTabHelperBrowserTest2 inherit from
> PrefsTabHelperBrowserTest and have the path to the preferences file be a
virtual
> function that you override.  The rest of SetUpUserDataDirectory() would then
be
> shared.

OK I see. Done.

http://codereview.chromium.org/10107014/diff/31031/chrome/common/extensions/d...
File chrome/common/extensions/docs/samples.json (right):

http://codereview.chromium.org/10107014/diff/31031/chrome/common/extensions/d...
chrome/common/extensions/docs/samples.json:178:
"chrome.experimental.serial.getPorts":
"experimental.serial.html#method-getPorts",
On 2012/05/09 17:11:15, tony wrote:
> Is this change intentional?

Yes, samples.json is autogenerated by build.py. This just means someone probably
forgot to run build.py.

http://codereview.chromium.org/10107014/diff/31031/ppapi/shared_impl/private/...
File ppapi/shared_impl/private/ppb_browser_font_trusted_shared.cc (right):

http://codereview.chromium.org/10107014/diff/31031/ppapi/shared_impl/private/...
ppapi/shared_impl/private/ppb_browser_font_trusted_shared.cc:42: const char
*kCommonScript = uscript_getShortName(USCRIPT_COMMON);
On 2012/05/09 17:11:15, tony wrote:
> Nit: char*, not char *.

Done.

http://codereview.chromium.org/10107014/diff/31031/ppapi/shared_impl/private/...
File ppapi/shared_impl/private/ppb_font_shared.cc (right):

http://codereview.chromium.org/10107014/diff/31031/ppapi/shared_impl/private/...
ppapi/shared_impl/private/ppb_font_shared.cc:45: const char *kCommonScript =
uscript_getShortName(USCRIPT_COMMON);
On 2012/05/09 17:11:15, tony wrote:
> Nit: char*

Done.

[jschuh, 2012-05-11 13:24:09]

Sorry, you'll need a different owner for content/common. I'm on that list only
for changes to the sandbox policy (there's a comment to that effect in the
owners file). Though, given that it's a simple mechanical change, you can
probably just TBR it to an owner.

[Matt Perry, 2012-05-11 20:47:14]

extensions lgtm

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

http://codereview.chromium.org/10107014/diff/33004/chrome/browser/extensions/...
chrome/browser/extensions/extension_font_settings_api.cc:61: if
(!details->HasKey(kScriptKey))
use braces when an else is present.

http://codereview.chromium.org/10107014/diff/33004/chrome/browser/extensions/...
chrome/browser/extensions/extension_font_settings_api.cc:63: else if 
(!details->GetString(kScriptKey, &script))
extra space before (

[Evan Stade, 2012-05-11 21:40:14]

options2 lgtm

[Peter Kasting, 2012-05-11 22:01:06]

browser/ui rubber-stamp LGTM; questions below because I am somewhat clueless

http://codereview.chromium.org/10107014/diff/33004/chrome/browser/ui/prefs/pr...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (left):

http://codereview.chromium.org/10107014/diff/33004/chrome/browser/ui/prefs/pr...
chrome/browser/ui/prefs/prefs_tab_helper.cc:416: // per-script fonts are added
to Preferences UI.
So, this can never be removed now?

http://codereview.chromium.org/10107014/diff/33004/chrome/browser/ui/prefs/pr...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

http://codereview.chromium.org/10107014/diff/33004/chrome/browser/ui/prefs/pr...
chrome/browser/ui/prefs/prefs_tab_helper.cc:269: // "fonts.standard.Zyyy". These
prefs will exist if the migration to global
Nit: Should you explain "Zyyy" here?  Maybe "to 'fonts.standard.<blah>', where
<blah> is the such-and-such constant from ICU, chosen because ___"?  (My
suggestion is really vague here because I have absolutely no idea what Zyyy
means)

http://codereview.chromium.org/10107014/diff/33004/chrome/browser/ui/prefs/pr...
File chrome/browser/ui/prefs/prefs_tab_helper_browsertest.cc (right):

http://codereview.chromium.org/10107014/diff/33004/chrome/browser/ui/prefs/pr...
chrome/browser/ui/prefs/prefs_tab_helper_browsertest.cc:81: //
webkit.webprefs.standard_font_family -> webkit.webprefs.fonts.standard.Zyyy
Nit: Might also want to comment about this migration or the "Zyyy" constant here
and below

[falken, 2012-05-14 09:08:22]

Thanks for the reviews!

viettrungluu: ping for ppapi changes

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

http://codereview.chromium.org/10107014/diff/33004/chrome/browser/extensions/...
chrome/browser/extensions/extension_font_settings_api.cc:61: if
(!details->HasKey(kScriptKey))
On 2012/05/11 20:47:15, Matt Perry wrote:
> use braces when an else is present.

Always? The style guide says "if one part of an if-else statement uses curly
braces, the other part must too".

http://codereview.chromium.org/10107014/diff/33004/chrome/browser/extensions/...
chrome/browser/extensions/extension_font_settings_api.cc:63: else if 
(!details->GetString(kScriptKey, &script))
On 2012/05/11 20:47:15, Matt Perry wrote:
> extra space before (

Done.

http://codereview.chromium.org/10107014/diff/33004/chrome/browser/ui/prefs/pr...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (left):

http://codereview.chromium.org/10107014/diff/33004/chrome/browser/ui/prefs/pr...
chrome/browser/ui/prefs/prefs_tab_helper.cc:416: // per-script fonts are added
to Preferences UI.
On 2012/05/11 22:01:06, Peter Kasting wrote:
> So, this can never be removed now?

The original idea was for per-script fonts to be part of the built-in Settings
UI. That way, if Chrome upgrades and the user finds his fonts have been
overwritten, he would be able to go into Settings and fix it.

But that won't happen now; instead the Font Settings Extension API must be used.
Maybe if it turns out access to per-script settings in the extension model is
easy enough, or it turns out not to really be a problem, we can remove these
lines then.

http://codereview.chromium.org/10107014/diff/33004/chrome/browser/ui/prefs/pr...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

http://codereview.chromium.org/10107014/diff/33004/chrome/browser/ui/prefs/pr...
chrome/browser/ui/prefs/prefs_tab_helper.cc:269: // "fonts.standard.Zyyy". These
prefs will exist if the migration to global
On 2012/05/11 22:01:06, Peter Kasting wrote:
> Nit: Should you explain "Zyyy" here?  Maybe "to 'fonts.standard.<blah>', where
> <blah> is the such-and-such constant from ICU, chosen because ___"?  (My
> suggestion is really vague here because I have absolutely no idea what Zyyy
> means)

I added some more explanation.

http://codereview.chromium.org/10107014/diff/33004/chrome/browser/ui/prefs/pr...
File chrome/browser/ui/prefs/prefs_tab_helper_browsertest.cc (right):

http://codereview.chromium.org/10107014/diff/33004/chrome/browser/ui/prefs/pr...
chrome/browser/ui/prefs/prefs_tab_helper_browsertest.cc:81: //
webkit.webprefs.standard_font_family -> webkit.webprefs.fonts.standard.Zyyy
On 2012/05/11 22:01:06, Peter Kasting wrote:
> Nit: Might also want to comment about this migration or the "Zyyy" constant
here
> and below

Done.

[Peter Kasting, 2012-05-14 18:18:32]

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

http://codereview.chromium.org/10107014/diff/33004/chrome/browser/extensions/...
chrome/browser/extensions/extension_font_settings_api.cc:61: if
(!details->HasKey(kScriptKey))
On 2012/05/14 09:08:23, falken wrote:
> On 2012/05/11 20:47:15, Matt Perry wrote:
> > use braces when an else is present.
> 
> Always? The style guide says "if one part of an if-else statement uses curly
> braces, the other part must too".

Braces aren't needed here.

http://codereview.chromium.org/10107014/diff/33004/chrome/browser/ui/prefs/pr...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (left):

http://codereview.chromium.org/10107014/diff/33004/chrome/browser/ui/prefs/pr...
chrome/browser/ui/prefs/prefs_tab_helper.cc:416: // per-script fonts are added
to Preferences UI.
On 2012/05/14 09:08:23, falken wrote:
> On 2012/05/11 22:01:06, Peter Kasting wrote:
> > So, this can never be removed now?
> 
> The original idea was for per-script fonts to be part of the built-in Settings
> UI. That way, if Chrome upgrades and the user finds his fonts have been
> overwritten, he would be able to go into Settings and fix it.
> 
> But that won't happen now; instead the Font Settings Extension API must be
used.
> Maybe if it turns out access to per-script settings in the extension model is
> easy enough, or it turns out not to really be a problem, we can remove these
> lines then.

Perhaps you could write some comments about this?

http://codereview.chromium.org/10107014/diff/34011/chrome/browser/ui/prefs/pr...
File chrome/browser/ui/prefs/prefs_tab_helper_browsertest.cc (right):

http://codereview.chromium.org/10107014/diff/34011/chrome/browser/ui/prefs/pr...
chrome/browser/ui/prefs/prefs_tab_helper_browsertest.cc:124: // This undos the
migration to "global" names (originally done for the per-tab
Nit: undos -> undoes

[falken, 2012-05-15 03:38:29]

On 2012/05/14 18:18:32, Peter Kasting wrote:
>
http://codereview.chromium.org/10107014/diff/33004/chrome/browser/extensions/...
> File chrome/browser/extensions/extension_font_settings_api.cc (right):
> 
>
http://codereview.chromium.org/10107014/diff/33004/chrome/browser/extensions/...
> chrome/browser/extensions/extension_font_settings_api.cc:61: if
> (!details->HasKey(kScriptKey))
> On 2012/05/14 09:08:23, falken wrote:
> > On 2012/05/11 20:47:15, Matt Perry wrote:
> > > use braces when an else is present.
> > 
> > Always? The style guide says "if one part of an if-else statement uses curly
> > braces, the other part must too".
> 
> Braces aren't needed here.
> 
>
http://codereview.chromium.org/10107014/diff/33004/chrome/browser/ui/prefs/pr...
> File chrome/browser/ui/prefs/prefs_tab_helper.cc (left):
> 
>
http://codereview.chromium.org/10107014/diff/33004/chrome/browser/ui/prefs/pr...
> chrome/browser/ui/prefs/prefs_tab_helper.cc:416: // per-script fonts are added
> to Preferences UI.
> On 2012/05/14 09:08:23, falken wrote:
> > On 2012/05/11 22:01:06, Peter Kasting wrote:
> > > So, this can never be removed now?
> > 
> > The original idea was for per-script fonts to be part of the built-in
Settings
> > UI. That way, if Chrome upgrades and the user finds his fonts have been
> > overwritten, he would be able to go into Settings and fix it.
> > 
> > But that won't happen now; instead the Font Settings Extension API must be
> used.
> > Maybe if it turns out access to per-script settings in the extension model
is
> > easy enough, or it turns out not to really be a problem, we can remove these
> > lines then.
> 
> Perhaps you could write some comments about this?

Done.

> 
>
http://codereview.chromium.org/10107014/diff/34011/chrome/browser/ui/prefs/pr...
> File chrome/browser/ui/prefs/prefs_tab_helper_browsertest.cc (right):
> 
>
http://codereview.chromium.org/10107014/diff/34011/chrome/browser/ui/prefs/pr...
> chrome/browser/ui/prefs/prefs_tab_helper_browsertest.cc:124: // This undos the
> migration to "global" names (originally done for the per-tab
> Nit: undos -> undoes

Done.

[falken, 2012-05-15 03:42:40]

viettrungluu: friendly ping

[viettrungluu, 2012-05-15 17:21:17]

+brettw

http://codereview.chromium.org/10107014/diff/33010/ppapi/shared_impl/private/...
File ppapi/shared_impl/private/ppb_browser_font_trusted_shared.cc (right):

http://codereview.chromium.org/10107014/diff/33010/ppapi/shared_impl/private/...
ppapi/shared_impl/private/ppb_browser_font_trusted_shared.cc:7: #include
<unicode/uscript.h>
This is a dependency on ICU, and should be #include
"third_party/icu/public/common/unicode/uscript.h". Of course, that would
probably fail the deps check.

I don't know if we want to allow ppapi/shared_impl to depend on ICU. I'm pretty
sure we don't.

http://codereview.chromium.org/10107014/diff/33010/ppapi/shared_impl/private/...
ppapi/shared_impl/private/ppb_browser_font_trusted_shared.cc:42: const char*
kCommonScript = uscript_getShortName(USCRIPT_COMMON);
I don't think we want to add this static initializer.

http://codereview.chromium.org/10107014/diff/33010/ppapi/shared_impl/private/...
File ppapi/shared_impl/private/ppb_font_shared.cc (right):

http://codereview.chromium.org/10107014/diff/33010/ppapi/shared_impl/private/...
ppapi/shared_impl/private/ppb_font_shared.cc:7: #include <unicode/uscript.h>
What I said previously.

[brettw, 2012-05-15 22:53:10]

What is "Zyyy"?

http://codereview.chromium.org/10107014/diff/33010/ppapi/shared_impl/private/...
File ppapi/shared_impl/private/ppb_browser_font_trusted_shared.cc (right):

http://codereview.chromium.org/10107014/diff/33010/ppapi/shared_impl/private/...
ppapi/shared_impl/private/ppb_browser_font_trusted_shared.cc:42: const char*
kCommonScript = uscript_getShortName(USCRIPT_COMMON);
Right.

[falken, 2012-05-16 02:08:07]

Thanks for the reviews!

On 2012/05/15 22:53:10, brettw wrote:
> What is "Zyyy"?

The font pref maps are from ISO 15924 script code to font. "Zyyy" is the ISO
15924 script code for undetermined script aka Common. It's the default used on
WebKit's side to get/set a font setting when no script is specified.

How about using kCommonScript = "Zyyy" with a comment about what Zyyy is?

> 
>
http://codereview.chromium.org/10107014/diff/33010/ppapi/shared_impl/private/...
> File ppapi/shared_impl/private/ppb_browser_font_trusted_shared.cc (right):
> 
>
http://codereview.chromium.org/10107014/diff/33010/ppapi/shared_impl/private/...
> ppapi/shared_impl/private/ppb_browser_font_trusted_shared.cc:42: const char*
> kCommonScript = uscript_getShortName(USCRIPT_COMMON);
> Right.

[falken, 2012-05-16 04:37:37]

On 2012/05/16 02:08:07, falken wrote:
> Thanks for the reviews!
> 
> On 2012/05/15 22:53:10, brettw wrote:
> > What is "Zyyy"?
> 
> The font pref maps are from ISO 15924 script code to font. "Zyyy" is the ISO
> 15924 script code for undetermined script aka Common. It's the default used on
> WebKit's side to get/set a font setting when no script is specified.
> 
> How about using kCommonScript = "Zyyy" with a comment about what Zyyy is?

Uploaded a revised patch to do it that way.

[viettrungluu, 2012-05-16 13:27:57]

http://codereview.chromium.org/10107014/diff/37020/webkit/glue/webpreferences.cc
File webkit/glue/webpreferences.cc (right):

http://codereview.chromium.org/10107014/diff/37020/webkit/glue/webpreferences...
webkit/glue/webpreferences.cc:8: #include <unicode/uscript.h>
As I said elsewhere, these are really ICU includes -- is it really our practice
to use include them as if they're system headers (the preexisting inclusion of
uchar.h notwithstanding)?

http://codereview.chromium.org/10107014/diff/37020/webkit/glue/webpreferences...
webkit/glue/webpreferences.cc:30: const char* kCommonScript =
uscript_getShortName(USCRIPT_COMMON);
Not wanting static initializers applies to all of Chromium, not just ppapi.

Maybe there should be a common constant somewhere for this? (Maybe a |const
static| member of |WebPreferences| called |kCommonScriptName| or some such?)

http://codereview.chromium.org/10107014/diff/37020/webkit/tools/test_shell/te...
File webkit/tools/test_shell/test_shell.cc (right):

http://codereview.chromium.org/10107014/diff/37020/webkit/tools/test_shell/te...
webkit/tools/test_shell/test_shell.cc:9: #include <unicode/uscript.h>
"

http://codereview.chromium.org/10107014/diff/37020/webkit/tools/test_shell/te...
webkit/tools/test_shell/test_shell.cc:84: const char* kCommonScript =
uscript_getShortName(USCRIPT_COMMON);
" (even if no one cares about test_shell)

[falken, 2012-05-17 07:08:41]

Thanks for the review.

https://chromiumcodereview.appspot.com/10107014/diff/37020/webkit/glue/webpre...
File webkit/glue/webpreferences.cc (right):

https://chromiumcodereview.appspot.com/10107014/diff/37020/webkit/glue/webpre...
webkit/glue/webpreferences.cc:8: #include <unicode/uscript.h>
On 2012/05/16 13:27:58, viettrungluu wrote:
> As I said elsewhere, these are really ICU includes -- is it really our
practice
> to use include them as if they're system headers (the preexisting inclusion of
> uchar.h notwithstanding)?

I've changed the existing include to "unicode/uchar.h".
It seems most files that include ICU do it that way. I found only one file that
uses a path like "third_party/icu/public/common/unicode/uchar.h" for ICU
(system_settings_provider2.h). I'm not sure whether that's a deliberate
preference, though.

https://chromiumcodereview.appspot.com/10107014/diff/37020/webkit/glue/webpre...
webkit/glue/webpreferences.cc:30: const char* kCommonScript =
uscript_getShortName(USCRIPT_COMMON);
On 2012/05/16 13:27:58, viettrungluu wrote:
> Not wanting static initializers applies to all of Chromium, not just ppapi.

I've moved it back to the "Zyyy" constant. I think we wanted to get it from ICU
for readability, but I think just a comment explaining what it is is sufficient.

> Maybe there should be a common constant somewhere for this? (Maybe a |const
> static| member of |WebPreferences| called |kCommonScriptName| or some such?)

I agree it'd be good to have that, but can't get ppapi to access it. It compiles
but then a linker error occurs:

out/Release/../../third_party/gold/gold64:
out/Release/obj.target/ppapi/libppapi_shared.a(out/Release/obj.target/ppapi/../ppapi_shared/ppapi/shared_impl/private/ppb_browser_font_trusted_shared.o):
in function ppapi::(anonymous
namespace)::PPFontDescToWebFontDesc(PP_BrowserFont_Trusted_Description const&,
ppapi::Preferences
const&):ppb_browser_font_trusted_shared.cc(.text._ZN5ppapi12_GLOBAL__N_123PPFontDescToWebFontDescERK34PP_BrowserFont_Trusted_DescriptionRKNS_11PreferencesE+0x42b):
error: undefined reference to 'webkit_glue::WebPreferences::kCommonScript'

Maybe it has something to do with DEPS or the WEBKIT_GLUE_EXPORT macro? It
occurs even for a extern const char[] outside of the struct and for a static
member function.

https://chromiumcodereview.appspot.com/10107014/diff/37020/webkit/tools/test_...
File webkit/tools/test_shell/test_shell.cc (right):

https://chromiumcodereview.appspot.com/10107014/diff/37020/webkit/tools/test_...
webkit/tools/test_shell/test_shell.cc:9: #include <unicode/uscript.h>
On 2012/05/16 13:27:58, viettrungluu wrote:
> "

Removed this as now it uses WebPreferences::kCommonScript.

https://chromiumcodereview.appspot.com/10107014/diff/37020/webkit/tools/test_...
webkit/tools/test_shell/test_shell.cc:84: const char* kCommonScript =
uscript_getShortName(USCRIPT_COMMON);
On 2012/05/16 13:27:58, viettrungluu wrote:
> " (even if no one cares about test_shell)

Done.

[viettrungluu, 2012-05-18 02:44:52]

LGTM otherwise.

https://chromiumcodereview.appspot.com/10107014/diff/47003/webkit/glue/webpre...
File webkit/glue/webpreferences.h (right):

https://chromiumcodereview.appspot.com/10107014/diff/47003/webkit/glue/webpre...
webkit/glue/webpreferences.h:36: static const char kCommonScript[];
To get this to work elsewhere, I think you might need to do |WEBKIT_GLUE_EXPORT
static const char kCommonScript[];|.

[falken, 2012-05-18 05:58:26]

Thanks all for the reviews. I'll go ahead and land this and hopefully get the
below comment resolved in a later patch.

Tony: you may want to look over the changes to webkit/ in the newest patch, in
case you didn't see them already.

https://chromiumcodereview.appspot.com/10107014/diff/47003/webkit/glue/webpre...
File webkit/glue/webpreferences.h (right):

https://chromiumcodereview.appspot.com/10107014/diff/47003/webkit/glue/webpre...
webkit/glue/webpreferences.h:36: static const char kCommonScript[];
On 2012/05/18 02:44:53, viettrungluu wrote:
> To get this to work elsewhere, I think you might need to do
|WEBKIT_GLUE_EXPORT
> static const char kCommonScript[];|.

I'd tried that but no luck (including trying WEBKIT_GLUE_EXPORT in each possible
position). Tony, do you have an idea?

[brettw, 2012-05-18 05:58:34]

ppapi/webkit lgtm

[commit-bot: I haz the power, 2012-05-18 06:05:26]

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

[commit-bot: I haz the power, 2012-05-18 07:20:35]

Change committed as 137835

[tony, 2012-05-18 17:03:42]

Using the literal + a comment is fine.  It looks like there's some other code
that puts the EXPORT before "static const", but maybe those symbols aren't
actually used?
content/browser/accessibility/browser_accessibility_win.h:86:  CONTENT_EXPORT
static const char16 kEmbeddedCharacter[];
content/test/net/url_request_slow_download_job.h:24:  CONTENT_EXPORT static
const char kUnknownSizeUrl[];

Maybe the magic is to use array notation rather than *?