Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7606028:
Pass per-script fonts to WebKit settings. (Closed)
|
Created:
9 years, 4 months ago by falken Modified:
9 years ago CC:
chromium-reviews, Avi (use Gerrit), finnur+watch_chromium.org, jam, joi+watch-content_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org, jshin+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPass per-script fonts to WebKit settings.
WebKit now has per-script font settings (https://bugs.webkit.org/show_bug.cgi?id=20797).
This change adds per-script font preferences to Chromium and passes them to the WebKit settings. For this
first patch, default per-script fonts are defined only on Chrome OS, and only for Arabic and CJK scripts.
It specifies the default fonts in locale_settings_cros.grd. The preferences are passed to WebPreferences in render_view_host_delegate_helper.cc (in the same manner as current font preferences).
BUG=92302
TEST=manually
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98024
Patch Set 1 #
Total comments: 12
Patch Set 2 : pref strings instead of dictionary #
Total comments: 10
Patch Set 3 : updated patch #
Total comments: 4
Patch Set 4 : address Mattias's nits #
Total comments: 2
Patch Set 5 : revised patch to address sky's comment #
Total comments: 10
Patch Set 6 : address brettw's comments #
Total comments: 2
Patch Set 7 : patch to commit #
Total comments: 1
Messages
Total messages: 23 (0 generated)
Can you please review my patch? I chose reviewers from OWNERS files: brettw: chrome/browser/tab_contents, content/common sky: chrome/browser/ui tony: webkit/glue Thanks, Matt http://codereview.chromium.org/7606028/diff/1/chrome/browser/ui/tab_contents/... File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right): http://codereview.chromium.org/7606028/diff/1/chrome/browser/ui/tab_contents/... chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:306: l10n_util::GetStringUTF16(IDS_STANDARD_FONT_FAMILY_JAPANESE)); This seems flawed because it overrides the user's preferences, when I just want to set a default value. Is it possible to set a default value within a Dictionary?
Jungshik would be a good reviewer of this too. http://codereview.chromium.org/7606028/diff/1/chrome/browser/tab_contents/ren... File chrome/browser/tab_contents/render_view_host_delegate_helper.cc (right): http://codereview.chromium.org/7606028/diff/1/chrome/browser/tab_contents/ren... chrome/browser/tab_contents/render_view_host_delegate_helper.cc:305: void RenderViewHostDelegateHelper::FillFontFamilyMap( Does this need to be part of RenderViewHostDelegateHelper or can we make it local to the cc file in an anonymous namespace? http://codereview.chromium.org/7606028/diff/1/chrome/browser/tab_contents/ren... File chrome/browser/tab_contents/render_view_host_delegate_helper.h (right): http://codereview.chromium.org/7606028/diff/1/chrome/browser/tab_contents/ren... chrome/browser/tab_contents/render_view_host_delegate_helper.h:140: WebPreferences::ScriptFontFamilyMap& to); Chromium style is to have out parameters be pointers rather than using pass by reference. http://codereview.chromium.org/7606028/diff/1/chrome/browser/ui/tab_contents/... File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right): http://codereview.chromium.org/7606028/diff/1/chrome/browser/ui/tab_contents/... chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:306: l10n_util::GetStringUTF16(IDS_STANDARD_FONT_FAMILY_JAPANESE)); On 2011/08/10 13:06:53, falken wrote: > This seems flawed because it overrides the user's preferences, when I just want > to set a default value. Is it possible to set a default value within a > Dictionary? Can we just make pref values for each string in the dictionaries? E.g., webkit.webprefs.standard_font_family_map.katakana_or_hiragana Also, it might be good to hardcode the script names rather than using uscript_getShortName. If that method hits disk, it might slow down startup. http://codereview.chromium.org/7606028/diff/1/webkit/glue/webpreferences.cc File webkit/glue/webpreferences.cc (right): http://codereview.chromium.org/7606028/diff/1/webkit/glue/webpreferences.cc#n... webkit/glue/webpreferences.cc:131: void WebPreferences::ApplyFontsFromMap(const ScriptFontFamilyMap& map, It looks like this could also be a local function rather than a member function. It would be nice to keep WebPreferences more like a struct.
Thanks for the review! I just have a comment about the pref dictionary. http://codereview.chromium.org/7606028/diff/1/chrome/browser/ui/tab_contents/... File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right): http://codereview.chromium.org/7606028/diff/1/chrome/browser/ui/tab_contents/... chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:306: l10n_util::GetStringUTF16(IDS_STANDARD_FONT_FAMILY_JAPANESE)); On 2011/08/10 19:23:40, tony wrote: > On 2011/08/10 13:06:53, falken wrote: > > This seems flawed because it overrides the user's preferences, when I just > want > > to set a default value. Is it possible to set a default value within a > > Dictionary? > > Can we just make pref values for each string in the dictionaries? E.g., > webkit.webprefs.standard_font_family_map.katakana_or_hiragana This would be good, but if I register "webkit.webprefs.standard_font_family_map.katakana_or_hiragana" and later call GetDictionary("webkit.webprefs.standard_font_family_map"), the pref doesn't appear in the returned value. It seems like DictionaryPrefUpdate is the proper way to update values within a dictionary, but I don't see how to set defaults for them. Am I missing something about the pref service? Otherwise, perhaps a solution is to register prefs for the things we have a default for, and when getting the dictionary, check if each registered pref is in the dictionary, and if not, retrieve the pref itself. Perhaps another option is to not use a dictionary and just have pref values for each possible entry. But it seems ugly since there are over 100 scripts (though we can perhaps limit them to commonly used ones) and 6 generic font families. > > Also, it might be good to hardcode the script names rather than using > uscript_getShortName. If that method hits disk, it might slow down startup.
On 2011/08/11 08:54:19, falken wrote:
>
> This would be good, but if I register
> "webkit.webprefs.standard_font_family_map.katakana_or_hiragana" and later call
> GetDictionary("webkit.webprefs.standard_font_family_map"), the pref doesn't
> appear in the returned value.
You mean if the value is unset, right? Yeah, you would only get the default
value if you called PrefService::GetString on the full path.
> It seems like DictionaryPrefUpdate is the proper way to update values within a
> dictionary, but I don't see how to set defaults for them.
>
> Am I missing something about the pref service?
mnissler or battre might be able to tell you how to accomplish this.
> Otherwise, perhaps a solution is to register prefs for the things we have a
> default for, and when getting the dictionary, check if each registered pref is
> in the dictionary, and if not, retrieve the pref itself.
That seems OK to me.
> Perhaps another option is to not use a dictionary and just have pref values
for
> each possible entry. But it seems ugly since there are over 100 scripts
(though
> we can perhaps limit them to commonly used ones) and 6 generic font families.
How many possible entries would we end up with? I think this is OK too if we
can just make a list of values and loop over them.
http://codereview.chromium.org/7606028/diff/1/chrome/browser/ui/tab_contents/... File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right): http://codereview.chromium.org/7606028/diff/1/chrome/browser/ui/tab_contents/... chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:306: l10n_util::GetStringUTF16(IDS_STANDARD_FONT_FAMILY_JAPANESE)); On 2011/08/11 08:54:20, falken wrote: > On 2011/08/10 19:23:40, tony wrote: > > On 2011/08/10 13:06:53, falken wrote: > > > This seems flawed because it overrides the user's preferences, when I just > > want > > > to set a default value. Is it possible to set a default value within a > > > Dictionary? > > > > Can we just make pref values for each string in the dictionaries? E.g., > > webkit.webprefs.standard_font_family_map.katakana_or_hiragana > > This would be good, but if I register > "webkit.webprefs.standard_font_family_map.katakana_or_hiragana" and later call > GetDictionary("webkit.webprefs.standard_font_family_map"), the pref doesn't > appear in the returned value. That's by design. The PrefService is a key-value store and there's no way to register a pref that accesses a path in a different dictionary pref. > > It seems like DictionaryPrefUpdate is the proper way to update values within a > dictionary, but I don't see how to set defaults for them. What you can do is just call the 3-parameter version of RegisterDictionaryPref() and pass a DictionaryValue* as the default you want to set. > > Am I missing something about the pref service? > > Otherwise, perhaps a solution is to register prefs for the things we have a > default for, and when getting the dictionary, check if each registered pref is > in the dictionary, and if not, retrieve the pref itself. I strongly advise against this. The PrefService nowadays has a stack of sources (aka PrefStores) for prefs, and the default values should be handled by the least-priority PrefStore. The RegisterXYZPref() calls take care of putting the default in place. > > Perhaps another option is to not use a dictionary and just have pref values for > each possible entry. But it seems ugly since there are over 100 scripts (though > we can perhaps limit them to commonly used ones) and 6 generic font families. > > > > > Also, it might be good to hardcode the script names rather than using > > uscript_getShortName. If that method hits disk, it might slow down startup. > http://codereview.chromium.org/7606028/diff/1/webkit/glue/webpreferences.h File webkit/glue/webpreferences.h (right): http://codereview.chromium.org/7606028/diff/1/webkit/glue/webpreferences.h#ne... webkit/glue/webpreferences.h:32: typedef std::vector<std::pair<std::string, string16> > ScriptFontFamilyMap; style guide says type declarations should go first. http://codereview.chromium.org/7606028/diff/1/webkit/glue/webpreferences.h#ne... webkit/glue/webpreferences.h:112: typedef void (*SetFontFamilyWrapper)( same here
Thanks for the comments! http://codereview.chromium.org/7606028/diff/1/chrome/browser/ui/tab_contents/... File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right): http://codereview.chromium.org/7606028/diff/1/chrome/browser/ui/tab_contents/... chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:306: l10n_util::GetStringUTF16(IDS_STANDARD_FONT_FAMILY_JAPANESE)); > What you can do is just call the 3-parameter version of RegisterDictionaryPref() > and pass a DictionaryValue* as the default you want to set. I see. What happens if the user preferences have a dictionary like: { "a": "1" } and the registered default is { "a": "2", "b": "3" } Is only the "a" key overridden and the "b" key is as set in the default? Or is the entire default overridden so "b" does not exist? I could test myself but I wonder if there is an intended behavior.
http://codereview.chromium.org/7606028/diff/1/chrome/browser/ui/tab_contents/... File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right): http://codereview.chromium.org/7606028/diff/1/chrome/browser/ui/tab_contents/... chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:306: l10n_util::GetStringUTF16(IDS_STANDARD_FONT_FAMILY_JAPANESE)); On 2011/08/12 09:37:00, falken wrote: > > What you can do is just call the 3-parameter version of > RegisterDictionaryPref() > > and pass a DictionaryValue* as the default you want to set. > > I see. What happens if the user preferences have a dictionary like: { "a": "1" > } > and the registered default is { "a": "2", "b": "3" } > > Is only the "a" key overridden and the "b" key is as set in the default? Or is > the entire default overridden so "b" does not exist? > > I could test myself but I wonder if there is an intended behavior. Values from lower-priority sources are hidden by higher-priority sources, there is no merging implemented at the moment (although in some other contexts we have thought about doing it already). So if you write a user setting through PrefService::Set() or DictionaryPrefUpdate, it will hide the whole defaults dictionary and you'll not see the default values any longer in Get() calls. I take it you'd like to have merging behavior? The reason why we haven't implemented it yet is mainly that there's no generic way to do it, i.e. different consumers will probably require different merging behavior.
http://codereview.chromium.org/7606028/diff/1/chrome/browser/ui/tab_contents/... File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right): http://codereview.chromium.org/7606028/diff/1/chrome/browser/ui/tab_contents/... chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:306: l10n_util::GetStringUTF16(IDS_STANDARD_FONT_FAMILY_JAPANESE)); On 2011/08/12 09:51:05, Mattias Nissler wrote: > On 2011/08/12 09:37:00, falken wrote: > > > What you can do is just call the 3-parameter version of > > RegisterDictionaryPref() > > > and pass a DictionaryValue* as the default you want to set. > > > > I see. What happens if the user preferences have a dictionary like: { "a": > "1" > > } > > and the registered default is { "a": "2", "b": "3" } > > > > Is only the "a" key overridden and the "b" key is as set in the default? Or > is > > the entire default overridden so "b" does not exist? > > > > I could test myself but I wonder if there is an intended behavior. > > Values from lower-priority sources are hidden by higher-priority sources, there > is no merging implemented at the moment (although in some other contexts we have > thought about doing it already). So if you write a user setting through > PrefService::Set() or DictionaryPrefUpdate, it will hide the whole defaults > dictionary and you'll not see the default values any longer in Get() calls. > > I take it you'd like to have merging behavior? The reason why we haven't > implemented it yet is mainly that there's no generic way to do it, i.e. > different consumers will probably require different merging behavior. Thanks, I see. I thought merging would be useful here in case the user sets the font for one script, so: user dictionary is: { "Hang" : "MyKoreanFont" } default dictionary is: { "Hang" : "DefaultKoreanFont", "Hrkt" : "DefaultJapaneseFont", ... } Then later we might change the default dictionary, e.g., to add new scripts or change default fonts. But without merging behavior it seems like the new defaults would not have effect. But I see what you mean about strongly advising against handling defaults manually. Perhaps it is better to do the other idea of using fixed strings instead of a dictionary. The number of possible entries is (num of scripts) * (num of font families). ICU's uscript.h has about 150 scripts and there are currently 6 font families, so 900 entries. But probably supporting about 30 scripts is enough (I notice the Firefox Preferences UI exposes about 30 "language groups"), which would make it 180 entries.
I've revised the patch per comments from Tony and Mattias. Please have another look.
http://codereview.chromium.org/7606028/diff/14002/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): http://codereview.chromium.org/7606028/diff/14002/chrome/common/pref_names.cc... chrome/common/pref_names.cc:124: "webkit.webprefs.fantasy_font_family_map"; I wonder if we can use the following names/hierarchy: webkit.webprefs.fonts.<css generic> instead of the current webkit.webprefs.{cursive,serif,sans-serif,...}_font_family webkit.webprefs.fonts.{sans-serif,serif, ....}.<script> : for per-script font prefs... Or, can we use the following instead? Perhaps, not given the way these values are read/written? webkit.webprefs.fonts.<script>.<css generic> ? Well, either of these does not change any functionality, but it seems tidier (on the other hand, we lose the 'direct' name mapping between our preference and webkit settings). http://codereview.chromium.org/7606028/diff/14002/chrome/common/pref_names.cc... chrome/common/pref_names.cc:126: // TODO(falken): Add all the scripts we should support. When we add 30+ scripts later, we may have to use a code generating script :-) http://codereview.chromium.org/7606028/diff/14002/chrome/common/pref_names.cc... chrome/common/pref_names.cc:128: { "Cyrl", "Hang", "Hans", "Hant", "Hrkt" }; As a pilot implementation, can you use 'Arab' rather than 'Cyrl'? Using 'Arab' will fix a real bug on CrOS right away. sans-serif: Droid Arabic Naskh serif : Droid Arabic Kufic standard: Droid Arabic Naskh (sans-serif and serif are reveresed because that's what Arabic speakers want).
http://codereview.chromium.org/7606028/diff/14002/chrome/browser/ui/tab_conte... File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right): http://codereview.chromium.org/7606028/diff/14002/chrome/browser/ui/tab_conte... chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:302: IDS_STANDARD_FONT_FAMILY_JAPANESE, PrefService::UNSYNCABLE_PREF); Nit: We may want to move all the font related prefs into a struct and register prefs in a loop. This could be a followup change. http://codereview.chromium.org/7606028/diff/14002/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): http://codereview.chromium.org/7606028/diff/14002/chrome/common/pref_names.cc... chrome/common/pref_names.cc:131: sizeof(kWebKitScriptsForFontFamilyMaps[0]); Can we use the arraysize() macro in base/basictypes.h here?
http://codereview.chromium.org/7606028/diff/14002/webkit/glue/webpreferences.cc File webkit/glue/webpreferences.cc (right): http://codereview.chromium.org/7606028/diff/14002/webkit/glue/webpreferences.... webkit/glue/webpreferences.cc:138: int32 script = u_getPropertyValueEnum(UCHAR_SCRIPT, (it->first).c_str()); Does it work with Hans and Hant? It looks like Hans and Hant are not listed in PropertyValueAliases.txt at http://unicode.org/Public/UNIDATA/PropertyValueAliases.txt (I'm not sure whether it's intentional or a bug. I'll check with the Unicode folk). In the meantime, you can use uscript_getCode(). See http://icu-project.org/apiref/icu4c/uscript_8h.html#ad7c87340add879032324be15... It's a bit awkward to use but not very much.
Thanks for the review comments. I've uploaded a updated patch. http://codereview.chromium.org/7606028/diff/14002/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): http://codereview.chromium.org/7606028/diff/14002/chrome/common/pref_names.cc... chrome/common/pref_names.cc:124: "webkit.webprefs.fantasy_font_family_map"; On 2011/08/15 22:50:40, Jungshik Shin wrote: > I wonder if we can use the following names/hierarchy: > > webkit.webprefs.fonts.<css generic> instead of the current > webkit.webprefs.{cursive,serif,sans-serif,...}_font_family > > webkit.webprefs.fonts.{sans-serif,serif, ....}.<script> : for per-script font > prefs... I don't know if we can change the current webkit.webprefs.{cursive,serif,sans-serif,...}_font_family without losing the existing user prefs. In the latest patch, I've put the new per-script settings in: webkit.webprefs.fonts.<css generic>.<script> It might be ugly to have these under "fonts" without the original settings under fonts as well. On the other hand, it is really only noticeable if you look at the Preferences file. Furthermore, if we add USCRIPT_COMMON ("Zyyy") to the list of supported scripts, the current webkit.webprefs.{cursive,serif,sans-serif,...}_font_family preferences could be superseded by the new webkit.webprefs.fonts.<css generic>.Zyyy preferences. > > Or, can we use the following instead? Perhaps, not given the way these values > are read/written? > > webkit.webprefs.fonts.<script>.<css generic> ? This is also possible. Is one more preferable than the other? I decided to do <css generic>.<script> since it is more consistent with WebPreferences and webkit settings, as you say. > Well, either of these does not change any functionality, but it seems tidier (on > the other hand, we lose the 'direct' name mapping between our preference and > webkit settings). http://codereview.chromium.org/7606028/diff/14002/chrome/common/pref_names.cc... chrome/common/pref_names.cc:128: { "Cyrl", "Hang", "Hans", "Hant", "Hrkt" }; On 2011/08/15 22:50:40, Jungshik Shin wrote: > As a pilot implementation, can you use 'Arab' rather than 'Cyrl'? Using 'Arab' > will fix a real bug on CrOS right away. > > sans-serif: Droid Arabic Naskh > serif : Droid Arabic Kufic > standard: Droid Arabic Naskh > > (sans-serif and serif are reveresed because that's what Arabic speakers want). Done. I used "Droid Arabic Kufi" instead of Kufic since i don't see Kufic on Chrome OS. http://codereview.chromium.org/7606028/diff/14002/chrome/common/pref_names.cc... chrome/common/pref_names.cc:131: sizeof(kWebKitScriptsForFontFamilyMaps[0]); On 2011/08/15 23:08:28, tony wrote: > Can we use the arraysize() macro in base/basictypes.h here? Done. http://codereview.chromium.org/7606028/diff/14002/webkit/glue/webpreferences.cc File webkit/glue/webpreferences.cc (right): http://codereview.chromium.org/7606028/diff/14002/webkit/glue/webpreferences.... webkit/glue/webpreferences.cc:138: int32 script = u_getPropertyValueEnum(UCHAR_SCRIPT, (it->first).c_str()); On 2011/08/15 23:12:42, Jungshik Shin wrote: > Does it work with Hans and Hant? It looks like Hans and Hant are not listed in > PropertyValueAliases.txt at > http://unicode.org/Public/UNIDATA/PropertyValueAliases.txt (I'm not sure > whether it's intentional or a bug. I'll check with the Unicode folk). Strange, it seems like "Hans" and "Hant" are working with u_getPropertyValueEnum. > > In the meantime, you can use uscript_getCode(). See > > > http://icu-project.org/apiref/icu4c/uscript_8h.html#ad7c87340add879032324be15... > > It's a bit awkward to use but not very much.
LGTM for the prefs part, some nits. http://codereview.chromium.org/7606028/diff/20001/webkit/glue/webpreferences.cc File webkit/glue/webpreferences.cc (right): http://codereview.chromium.org/7606028/diff/20001/webkit/glue/webpreferences.... webkit/glue/webpreferences.cc:95: void setStandardFontFamilyWrapper(WebSettings* settings, declare these helpers static or wrap them in an anonymous namespace. http://codereview.chromium.org/7606028/diff/20001/webkit/glue/webpreferences.... webkit/glue/webpreferences.cc:154: ApplyFontsFromMap(fixed_font_family_map, setFixedFontFamilyWrapper,settings); missing space after ,
Thanks! http://codereview.chromium.org/7606028/diff/20001/webkit/glue/webpreferences.cc File webkit/glue/webpreferences.cc (right): http://codereview.chromium.org/7606028/diff/20001/webkit/glue/webpreferences.... webkit/glue/webpreferences.cc:95: void setStandardFontFamilyWrapper(WebSettings* settings, On 2011/08/16 09:22:45, Mattias Nissler wrote: > declare these helpers static or wrap them in an anonymous namespace. Done. http://codereview.chromium.org/7606028/diff/20001/webkit/glue/webpreferences.... webkit/glue/webpreferences.cc:154: ApplyFontsFromMap(fixed_font_family_map, setFixedFontFamilyWrapper,settings); On 2011/08/16 09:22:45, Mattias Nissler wrote: > missing space after , Done.
LGTM2.
LGTM http://codereview.chromium.org/7606028/diff/21011/chrome/browser/ui/tab_conte... File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right): http://codereview.chromium.org/7606028/diff/21011/chrome/browser/ui/tab_conte... chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:215: static void RegisterFontFamilyMap(PrefService* prefs, Move this into the the anonymous namespace above and add a description.
brettw, can you please review? I need OWNER approval for chrome/browser/tab_contents and content/common. Thanks! http://codereview.chromium.org/7606028/diff/21011/chrome/browser/ui/tab_conte... File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right): http://codereview.chromium.org/7606028/diff/21011/chrome/browser/ui/tab_conte... chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:215: static void RegisterFontFamilyMap(PrefService* prefs, On 2011/08/17 17:06:32, sky wrote: > Move this into the the anonymous namespace above and add a description. Done.
http://codereview.chromium.org/7606028/diff/28001/chrome/browser/tab_contents... File chrome/browser/tab_contents/render_view_host_delegate_helper.cc (right): http://codereview.chromium.org/7606028/diff/28001/chrome/browser/tab_contents... chrome/browser/tab_contents/render_view_host_delegate_helper.cc:303: static void FillFontFamilyMap(const PrefService* prefs, Don't put static functions in the middle of the file, they should go at the top, and I'd use an anonymous namespace instead (encouraged by the style guide). http://codereview.chromium.org/7606028/diff/28001/chrome/browser/tab_contents... chrome/browser/tab_contents/render_view_host_delegate_helper.cc:309: base::StringPrintf("%s.%s", map_name, script).c_str(); This is wrong, the temporary string returned by stringprintf will be destroyed when this line is done, and pref_name will point to garbage. pref_name should be a std::string and you should do the c_str() inside the GetString invocation. http://codereview.chromium.org/7606028/diff/28001/chrome/browser/ui/tab_conte... File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right): http://codereview.chromium.org/7606028/diff/28001/chrome/browser/ui/tab_conte... chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:118: const char* pref_name = This has the same memory bug I mentioned above. http://codereview.chromium.org/7606028/diff/28001/chrome/browser/ui/tab_conte... chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:304: // Register per-script font prefs that have defaults. It's not clear from reading this why this should be ChromeOS-specific. Can you add some justification why this is the case in the comment? http://codereview.chromium.org/7606028/diff/28001/webkit/glue/webpreferences.h File webkit/glue/webpreferences.h (right): http://codereview.chromium.org/7606028/diff/28001/webkit/glue/webpreferences.... webkit/glue/webpreferences.h:25: typedef std::vector<std::pair<std::string, string16> > ScriptFontFamilyMap; Can you provide a comment here what this is mapping to/from? With maybe an example, too. I'm not sure all developers know what a "script" is.
Thanks for the review. http://codereview.chromium.org/7606028/diff/28001/chrome/browser/tab_contents... File chrome/browser/tab_contents/render_view_host_delegate_helper.cc (right): http://codereview.chromium.org/7606028/diff/28001/chrome/browser/tab_contents... chrome/browser/tab_contents/render_view_host_delegate_helper.cc:303: static void FillFontFamilyMap(const PrefService* prefs, On 2011/08/22 23:23:28, brettw wrote: > Don't put static functions in the middle of the file, they should go at the top, > and I'd use an anonymous namespace instead (encouraged by the style guide). Done. http://codereview.chromium.org/7606028/diff/28001/chrome/browser/tab_contents... chrome/browser/tab_contents/render_view_host_delegate_helper.cc:309: base::StringPrintf("%s.%s", map_name, script).c_str(); On 2011/08/22 23:23:28, brettw wrote: > This is wrong, the temporary string returned by stringprintf will be destroyed > when this line is done, and pref_name will point to garbage. pref_name should be > a std::string and you should do the c_str() inside the GetString invocation. Oops. Done. http://codereview.chromium.org/7606028/diff/28001/chrome/browser/ui/tab_conte... File chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (right): http://codereview.chromium.org/7606028/diff/28001/chrome/browser/ui/tab_conte... chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:118: const char* pref_name = On 2011/08/22 23:23:28, brettw wrote: > This has the same memory bug I mentioned above. Done. http://codereview.chromium.org/7606028/diff/28001/chrome/browser/ui/tab_conte... chrome/browser/ui/tab_contents/tab_contents_wrapper.cc:304: // Register per-script font prefs that have defaults. On 2011/08/22 23:23:28, brettw wrote: > It's not clear from reading this why this should be ChromeOS-specific. Can you > add some justification why this is the case in the comment? Done. http://codereview.chromium.org/7606028/diff/28001/webkit/glue/webpreferences.h File webkit/glue/webpreferences.h (right): http://codereview.chromium.org/7606028/diff/28001/webkit/glue/webpreferences.... webkit/glue/webpreferences.h:25: typedef std::vector<std::pair<std::string, string16> > ScriptFontFamilyMap; On 2011/08/22 23:23:28, brettw wrote: > Can you provide a comment here what this is mapping to/from? With maybe an > example, too. I'm not sure all developers know what a "script" is. Done.
LGTM http://codereview.chromium.org/7606028/diff/35001/chrome/browser/tab_contents... File chrome/browser/tab_contents/render_view_host_delegate_helper.cc (right): http://codereview.chromium.org/7606028/diff/35001/chrome/browser/tab_contents... chrome/browser/tab_contents/render_view_host_delegate_helper.cc:56: } // namespace Two spaces before // http://codereview.chromium.org/7606028/diff/35001/webkit/glue/webpreferences.cc File webkit/glue/webpreferences.cc (right): http://codereview.chromium.org/7606028/diff/35001/webkit/glue/webpreferences.... webkit/glue/webpreferences.cc:149: } // namespace Twos spaces before //
http://codereview.chromium.org/7606028/diff/41001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): http://codereview.chromium.org/7606028/diff/41001/chrome/common/pref_names.h#... chrome/common/pref_names.h:58: extern const char* kWebKitScriptsForFontFamilyMaps[]; !!! Never name a non-const variable with kSomething. You understand that const char foo[] = { "Arab" }; kWebKitScriptsForFontFamilyMaps = foo; will compile, right? Please make it const with: extern const char* const kWebKitScriptsForFontFamilyMaps[]; Thanks.
> Please make it const with: > extern const char* const kWebKitScriptsForFontFamilyMaps[]; > > Thanks. Thanks for noticing. I'll fix it. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
