Auto-generates the font family scripts at build time.

Auto-generates the font family scripts at build time.

On android, this saves heap allocation and startup runtime cost by moving
the strings to RO section mmapped in the library.

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

[bulach, 2013-03-19 14:00:35]

the savings here are not that big, it's more of a small tradeoff between heap
and mmap'ed RO memory (plus some CPU cycles on startup) on all platforms, not
just android...
please let me know if you think it's worth it! :)

[Bernhard Bauer, 2013-03-19 14:44:51]

I don't have an opinion on whether this is worth it in terms of resource
consumption, but it seems reasonable in terms of DRY. I have some reservations
about creating the list though:

https://codereview.chromium.org/12902021/diff/6/chrome/common/font_family_scr...
File chrome/common/font_family_scripts_generator.cc (right):

https://codereview.chromium.org/12902021/diff/6/chrome/common/font_family_scr...
chrome/common/font_family_scripts_generator.cc:13: // at startup time by using
RO data section.
Is there any reason you're doing this in C++ instead of a scripting language,
i.e. Python?

Alternatively, how about using some preprocessing trickery?

[bulach, 2013-03-19 15:35:16]

thanks Bernhard! inline:

https://codereview.chromium.org/12902021/diff/6/chrome/common/font_family_scr...
File chrome/common/font_family_scripts_generator.cc (right):

https://codereview.chromium.org/12902021/diff/6/chrome/common/font_family_scr...
chrome/common/font_family_scripts_generator.cc:13: // at startup time by using
RO data section.
On 2013/03/19 14:44:51, Bernhard Bauer wrote:
> Is there any reason you're doing this in C++ instead of a scripting language,
> i.e. Python?
> 
> Alternatively, how about using some preprocessing trickery?

yeah, I wished I could do a script.. :)
the reason for using c++ is that the values are actually defined in
pref_names.cc, so anything but c++ would end up pretty convoluted.. :-/

for the same reason, there's no easy way to use pre-processing tricks, unless of
course we move the data out of pref_names.cc.. I can certainly try that if you
thing it's worth it? it'd probably require a header file without guards in order
to allow multiple pass, etc.., so it may end up less readable and harder to
maintain than this.. wdyt?

[Bernhard Bauer, 2013-03-19 16:06:38]

https://codereview.chromium.org/12902021/diff/6/chrome/common/font_family_scr...
File chrome/common/font_family_scripts_generator.cc (right):

https://codereview.chromium.org/12902021/diff/6/chrome/common/font_family_scr...
chrome/common/font_family_scripts_generator.cc:13: // at startup time by using
RO data section.
On 2013/03/19 15:35:16, bulach wrote:
> On 2013/03/19 14:44:51, Bernhard Bauer wrote:
> > Is there any reason you're doing this in C++ instead of a scripting
language,
> > i.e. Python?
> > 
> > Alternatively, how about using some preprocessing trickery?
> 
> yeah, I wished I could do a script.. :)
> the reason for using c++ is that the values are actually defined in
> pref_names.cc, so anything but c++ would end up pretty convoluted.. :-/
> 
> for the same reason, there's no easy way to use pre-processing tricks, unless
of
> course we move the data out of pref_names.cc.. I can certainly try that if you
> thing it's worth it? it'd probably require a header file without guards in
order
> to allow multiple pass, etc.., so it may end up less readable and harder to
> maintain than this.. wdyt?

Hm... how about the following:

#define DEFINE_FONT_PREF_NAMES(script_name)       \
  "webkit.webprefs.fonts.standard." script_name,  \
  "webkit.webprefs.fonts.fixed." script_name,     \
  // ...
  "webkit.webprefs.fonts.pictograph." script_name

const char* const kWebKitScriptsForFontFamilyMaps[] = {
  DEFINE_FONT_PREF_NAMES("Afak"),
  DEFINE_FONT_PREF_NAMES("Arab"),
  // ...
  DEFINE_FONT_PREF_NAMES("Xyzzy"),
};

We could also pull the prefixes out into #define's to avoid repeating them (they
would need to be #define's instead of constants so we can use them in the macro,
but I don't think that would be too bad).

[bulach, 2013-03-19 16:18:17]

https://codereview.chromium.org/12902021/diff/6/chrome/common/font_family_scr...
File chrome/common/font_family_scripts_generator.cc (right):

https://codereview.chromium.org/12902021/diff/6/chrome/common/font_family_scr...
chrome/common/font_family_scripts_generator.cc:13: // at startup time by using
RO data section.
On 2013/03/19 16:06:38, Bernhard Bauer wrote:
> On 2013/03/19 15:35:16, bulach wrote:
> > On 2013/03/19 14:44:51, Bernhard Bauer wrote:
> > > Is there any reason you're doing this in C++ instead of a scripting
> language,
> > > i.e. Python?
> > > 
> > > Alternatively, how about using some preprocessing trickery?
> > 
> > yeah, I wished I could do a script.. :)
> > the reason for using c++ is that the values are actually defined in
> > pref_names.cc, so anything but c++ would end up pretty convoluted.. :-/
> > 
> > for the same reason, there's no easy way to use pre-processing tricks,
unless
> of
> > course we move the data out of pref_names.cc.. I can certainly try that if
you
> > thing it's worth it? it'd probably require a header file without guards in
> order
> > to allow multiple pass, etc.., so it may end up less readable and harder to
> > maintain than this.. wdyt?
> 
> Hm... how about the following:
> 
> #define DEFINE_FONT_PREF_NAMES(script_name)       \
>   "webkit.webprefs.fonts.standard." script_name,  \
>   "webkit.webprefs.fonts.fixed." script_name,     \
>   // ...
>   "webkit.webprefs.fonts.pictograph." script_name
> 
> const char* const kWebKitScriptsForFontFamilyMaps[] = {
>   DEFINE_FONT_PREF_NAMES("Afak"),
>   DEFINE_FONT_PREF_NAMES("Arab"),
>   // ...
>   DEFINE_FONT_PREF_NAMES("Xyzzy"),
> };
> 
> We could also pull the prefixes out into #define's to avoid repeating them
(they
> would need to be #define's instead of constants so we can use them in the
macro,
> but I don't think that would be too bad).

the problem is that the "Afak", "Arab", ... "Xyzzy" is 152 long, so we
definitely want to avoid repeating them as well.. 
but pulling out all of those is a good idea, thanks!

let me try that and see how it goes, will update this patch in a bit..

[bulach, 2013-03-19 18:08:48]

alright, so following your suggestion, using pre-processor magic :) another look
please?

[Bernhard Bauer, 2013-03-19 18:22:17]

LGTM with a nit:

https://codereview.chromium.org/12902021/diff/11001/chrome/common/pref_font_s...
File chrome/common/pref_font_script_names-inl.h (right):

https://codereview.chromium.org/12902021/diff/11001/chrome/common/pref_font_s...
chrome/common/pref_font_script_names-inl.h:12: EXPAND_SCRIPT_FONT(x, "Afak") \
Nit: AFAICT, we indent multi-line #defines with two spaces and align the
backslashes.

[falken, 2013-03-19 18:26:04]

https://codereview.chromium.org/12902021/diff/11001/chrome/browser/ui/prefs/p...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/12902021/diff/11001/chrome/browser/ui/prefs/p...
chrome/browser/ui/prefs/prefs_tab_helper.cc:126: void
RegisterFontFamilyMap(PrefRegistrySyncable* registry,
Can you update this comment and change the function name to reflect what it does
now? Like "RegisterFontFamilyPrefs". Maybe also add a comment about what
|fonts_with_defaults| does (there should have been one before).

[bulach, 2013-03-20 11:01:27]

thanks both! all comments addressed, ptal.

https://codereview.chromium.org/12902021/diff/11001/chrome/browser/ui/prefs/p...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/12902021/diff/11001/chrome/browser/ui/prefs/p...
chrome/browser/ui/prefs/prefs_tab_helper.cc:126: void
RegisterFontFamilyMap(PrefRegistrySyncable* registry,
On 2013/03/19 18:26:04, falken wrote:
> Can you update this comment and change the function name to reflect what it
does
> now? Like "RegisterFontFamilyPrefs". Maybe also add a comment about what
> |fonts_with_defaults| does (there should have been one before).

Done.

https://codereview.chromium.org/12902021/diff/11001/chrome/common/pref_font_s...
File chrome/common/pref_font_script_names-inl.h (right):

https://codereview.chromium.org/12902021/diff/11001/chrome/common/pref_font_s...
chrome/common/pref_font_script_names-inl.h:12: EXPAND_SCRIPT_FONT(x, "Afak") \
On 2013/03/19 18:22:17, Bernhard Bauer wrote:
> Nit: AFAICT, we indent multi-line #defines with two spaces and align the
> backslashes.

Done.

[falken, 2013-03-20 16:21:17]

lgtm with a comment

https://codereview.chromium.org/12902021/diff/14001/chrome/browser/ui/prefs/p...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/12902021/diff/14001/chrome/browser/ui/prefs/p...
chrome/browser/ui/prefs/prefs_tab_helper.cc:128: // as the primary script of the
browser's UI locale.
Actually |fonts_with_defaults| contains pref names that have already been
registered since they have a specified default value, not just prefs for the UI
locale. RegisterFontFamilyPrefs just goes through and registers the remaining
prefs that don't have defaults (although this is currently the vast majority of
them).

[bulach, 2013-03-20 16:32:00]

thanks! I updated the comment, would you mind double checking if it's any
clearer please?

also, +jochen for OWNERS review

[falken, 2013-03-20 17:48:22]

lgtm 

On 2013/03/20 16:32:00, bulach wrote:
> thanks! I updated the comment, would you mind double checking if it's any
> clearer please?
> 
> also, +jochen for OWNERS review

[jochen (gone - plz use gerrit), 2013-03-20 17:49:40]

lgtm

https://codereview.chromium.org/12902021/diff/22002/chrome/common_constants.gyp
File chrome/common_constants.gyp (right):

https://codereview.chromium.org/12902021/diff/22002/chrome/common_constants.g...
chrome/common_constants.gyp:33: 'common/pref_font_script_names-inl.h',
nit alphabetical ordering

[bulach, 2013-03-21 12:30:45]

thanks jochen! nit addressed, CQing.

https://codereview.chromium.org/12902021/diff/22002/chrome/common_constants.gyp
File chrome/common_constants.gyp (right):

https://codereview.chromium.org/12902021/diff/22002/chrome/common_constants.g...
chrome/common_constants.gyp:33: 'common/pref_font_script_names-inl.h',
On 2013/03/20 17:49:40, jochen wrote:
> nit alphabetical ordering

Done.

[commit-bot: I haz the power, 2013-03-21 12:31:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/12902021/8007

[commit-bot: I haz the power, 2013-03-21 14:04:19]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-03-21 16:08:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/12902021/8007

[commit-bot: I haz the power, 2013-03-22 10:01:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/12902021/8007

[commit-bot: I haz the power, 2013-03-22 16:32:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/12902021/8007

[commit-bot: I haz the power, 2013-03-22 19:04:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/12902021/8007

[commit-bot: I haz the power, 2013-03-23 15:48:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bulach@chromium.org/12902021/8007

[commit-bot: I haz the power, 2013-03-24 07:07:25]

Change committed as 190229