Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1405453002:
[MD settings] adding c++ side of font settings (Closed)
|
Created:
5 years, 2 months ago by dschuyler Modified:
5 years, 1 month ago Reviewers:
Dan Beam CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MD settings] adding c++ side of font settings
This CL handles the requests for OS font lists
and character encoding lists. It will be used
by the WebContent settings.
BUG=538372
Committed: https://crrev.com/a02f4993a13bc0d82c406aa136f1b8fd1fd6114a
Cr-Commit-Position: refs/heads/master@{#356139}
Patch Set 1 : touch ups #
Total comments: 36
Patch Set 2 : review changes #
Total comments: 20
Patch Set 3 : 'review changes' #
Messages
Total messages: 14 (3 generated)
dschuyler@chromium.org changed reviewers: + dbeam@chromium.org
Patchset #1 (id:1) has been deleted
https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/settings_font_handler.cc (right): https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:29: std::string MaybeGetLocalizedFontName(const std::string& font_name) { I'm taking it on face value that this is still needed. The older options code performed this step, and I'm mimicking it here. https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:54: fixed_font_.Init(prefs::kWebKitFixedFontFamily, pref_service); We could pull out these values and strictly use the entries from the prefs_util code to determine the current selection. My concern is that simplifying this code would complicate JavaScript code. https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:80: font->Append(new base::StringValue(has_rtl_chars ? "rtl" : "ltr")); Identifying the rtl and ltr fonts is something I'm pulling forward from the older options code without yet confirming if it's required. https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:106: // Add empty name/value to indicate a separator item. Using a blank entry is what the old options code did, there may be other ways to handle adding a separator item, but this seems reasonable, so I didn't change it.
https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/settings_font_handler.cc (right): https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:29: std::string MaybeGetLocalizedFontName(const std::string& font_name) { On 2015/10/12 23:24:51, dschuyler wrote: > I'm taking it on face value that this is still needed. The older options code > performed this step, and I'm mimicking it here. probably is https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:54: fixed_font_.Init(prefs::kWebKitFixedFontFamily, pref_service); On 2015/10/12 23:24:51, dschuyler wrote: > We could pull out these values and strictly use the entries from the prefs_util > code to determine the current selection. My concern is that simplifying this > code would complicate JavaScript code. i think it'd be pretty simple to bind prefs={{prefs}} and listen for changes to: prefs.webkit.webprefs.fonts.* probably as simple if not simpler than this C++. the real question is whether we'd like to port the logic (i.e. getting the localized font, determining if there are strong RTL chars). fwiw: there seems to be a fontSettings extension API -- would it be worth adding to that? https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:67: base::Unretained(this))); this class should use a WeakPtrFactory<> and instead of base::Unretained(this) we should be doing weak_ptr_factory_.GetWeakPtr(); i have no idea how this code isn't causing crashes like this annnnnnnnnnd I was right and here's the fix: https://codereview.chromium.org/1397903004/ https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:75: DCHECK(has_font); \n https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:78: DCHECK(has_value); \n https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:80: font->Append(new base::StringValue(has_rtl_chars ? "rtl" : "ltr")); On 2015/10/12 23:24:51, dschuyler wrote: > Identifying the rtl and ltr fonts is something I'm pulling forward from the > older options code without yet confirming if it's required. this is mildly weird to me. this is checking for an RTL character in the title of the font. but it doesn't check the font's character set. whatevs https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:84: base::ListValue encoding_list; declare lower https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:91: DCHECK(encodings); nit: DCHECK(ptr); followed by DCHECK(ptr->DoSomething()); seems silly to me as ptr->DoSomething() would crash anyways... https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:92: DCHECK(!encodings->empty()); \n https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:94: for (it = encodings->begin(); it != encodings->end(); ++it) { you could probably use for ( : ) now https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:95: base::ListValue* option = new base::ListValue(); nit: use scoped_ptr to make the ownership more explicit https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:108: option->Append(new base::StringValue(std::string())); why do we need 2 empty strings? https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:113: // Current selections. The order of items in this tuple are important. would be nice to make this a dictionary instead so the order of items in this tuple AREN'T important https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:125: web_ui()->CallJavascriptFunction("Settings.setFontsData", *list.get(), why is this doing *.get()? scoped_ptr implements operator* https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped... https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/settings_font_handler.h (right): https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.h:7: #include "base/macros.h" i'm gonna write a presubmit about this, tired of saying it for everybody
https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/settings_font_handler.cc (right): https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:29: std::string MaybeGetLocalizedFontName(const std::string& font_name) { On 2015/10/15 02:34:41, Dan Beam wrote: > On 2015/10/12 23:24:51, dschuyler wrote: > > I'm taking it on face value that this is still needed. The older options code > > performed this step, and I'm mimicking it here. > > probably is Acknowledged. https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:54: fixed_font_.Init(prefs::kWebKitFixedFontFamily, pref_service); On 2015/10/15 02:34:41, Dan Beam wrote: > On 2015/10/12 23:24:51, dschuyler wrote: > > We could pull out these values and strictly use the entries from the > prefs_util > > code to determine the current selection. My concern is that simplifying this > > code would complicate JavaScript code. > > i think it'd be pretty simple to bind prefs={{prefs}} and listen for changes to: > > prefs.webkit.webprefs.fonts.* > > probably as simple if not simpler than this C++. > > the real question is whether we'd like to port the logic (i.e. getting the > localized font, determining if there are strong RTL chars). > > fwiw: there seems to be a fontSettings extension API -- would it be worth adding > to that? > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... These have been removed with the change to use the prefs_util entries. The font_settings_api sounds interesting. Done. https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:67: base::Unretained(this))); On 2015/10/15 02:34:41, Dan Beam wrote: > this class should use a WeakPtrFactory<> and instead of base::Unretained(this) > we should be doing weak_ptr_factory_.GetWeakPtr(); > > i have no idea how this code isn't causing crashes like this > > annnnnnnnnnd I was right and here's the fix: > https://codereview.chromium.org/1397903004/ Done. https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:75: DCHECK(has_font); On 2015/10/15 02:34:41, Dan Beam wrote: > \n Done. https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:78: DCHECK(has_value); On 2015/10/15 02:34:41, Dan Beam wrote: > \n Done. https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:80: font->Append(new base::StringValue(has_rtl_chars ? "rtl" : "ltr")); On 2015/10/15 02:34:41, Dan Beam wrote: > On 2015/10/12 23:24:51, dschuyler wrote: > > Identifying the rtl and ltr fonts is something I'm pulling forward from the > > older options code without yet confirming if it's required. > > this is mildly weird to me. this is checking for an RTL character in the title > of the font. but it doesn't check the font's character set. whatevs Acknowledged. https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:84: base::ListValue encoding_list; On 2015/10/15 02:34:41, Dan Beam wrote: > declare lower Done. https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:91: DCHECK(encodings); On 2015/10/15 02:34:41, Dan Beam wrote: > nit: DCHECK(ptr); followed by DCHECK(ptr->DoSomething()); seems silly to me as > ptr->DoSomething() would crash anyways... Done. https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:92: DCHECK(!encodings->empty()); On 2015/10/15 02:34:41, Dan Beam wrote: > \n Done. https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:94: for (it = encodings->begin(); it != encodings->end(); ++it) { On 2015/10/15 02:34:41, Dan Beam wrote: > you could probably use for ( : ) now Done. https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:95: base::ListValue* option = new base::ListValue(); On 2015/10/15 02:34:41, Dan Beam wrote: > nit: use scoped_ptr to make the ownership more explicit Done. https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:108: option->Append(new base::StringValue(std::string())); On 2015/10/15 02:34:41, Dan Beam wrote: > why do we need 2 empty strings? It seems to work ok with one. Done. https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:113: // Current selections. The order of items in this tuple are important. On 2015/10/15 02:34:41, Dan Beam wrote: > would be nice to make this a dictionary instead so the order of items in this > tuple AREN'T important Done. https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:125: web_ui()->CallJavascriptFunction("Settings.setFontsData", *list.get(), On 2015/10/15 02:34:41, Dan Beam wrote: > why is this doing *.get()? scoped_ptr implements operator* > > https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped... Done. https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/settings_font_handler.h (right): https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.h:7: On 2015/10/15 02:34:41, Dan Beam wrote: > #include "base/macros.h" > > i'm gonna write a presubmit about this, tired of saying it for everybody Done.
https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/settings_font_handler.h (right): https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.h:7: On 2015/10/15 21:54:20, dschuyler wrote: > On 2015/10/15 02:34:41, Dan Beam wrote: > > #include "base/macros.h" > > > > i'm gonna write a presubmit about this, tired of saying it for everybody > > Done. also Done: https://codereview.chromium.org/1411553003/
https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/settings_font_handler.h (right): https://codereview.chromium.org/1405453002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.h:7: On 2015/10/15 23:33:17, Dan Beam wrote: > On 2015/10/15 21:54:20, dschuyler wrote: > > On 2015/10/15 02:34:41, Dan Beam wrote: > > > #include "base/macros.h" > > > > > > i'm gonna write a presubmit about this, tired of saying it for everybody > > > > Done. > > also Done: https://codereview.chromium.org/1411553003/ Acknowledged.
https://codereview.chromium.org/1405453002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/settings_font_handler.cc (right): https://codereview.chromium.org/1405453002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:7: #include "base/basictypes.h" ^ no longer required for size_t https://codereview.chromium.org/1405453002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:8: #include "base/bind.h" base/bind_helpers.h for base::Unretained https://codereview.chromium.org/1405453002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:17: #include "content/public/browser/web_ui.h" ^ why do you need web_ui.h? https://codereview.chromium.org/1405453002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:33: weak_ptr_factory_.GetWeakPtr())); in this case, use base::Unretained(this) instead of weak_ptr_factory_.GetWeakPtr() https://codereview.chromium.org/1405453002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:39: weak_ptr_factory_.GetWeakPtr())); keep weak_ptr_factory_.GetWeakPtr() for this https://codereview.chromium.org/1405453002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:43: // Font list. Selects the directionality for the fonts in the given list. \s\s -> \s https://codereview.chromium.org/1405453002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:72: CharacterEncoding::GetCanonicalEncodingNameByCommandId(cmd_id); option->ApendString( CharacterEncoding::GetCanonicalEncodingNameByCommandId(it.encoding_id)); option->AppendString(it.encoding_display_name); option->AppendString(has_rtl_chars ? "rtl" : "ltr"); https://codereview.chromium.org/1405453002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:80: option->Append(new base::StringValue(std::string())); option->AppendString(std::string()); https://codereview.chromium.org/1405453002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/settings_font_handler.h (right): https://codereview.chromium.org/1405453002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.h:24: class SettingsFontHandler : public SettingsPageUIHandler { out of curiosity, why SettingsFontHandler instead of FontHandler? it's already in settings:: https://codereview.chromium.org/1405453002/diff/40001/chrome/chrome_browser_u... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/1405453002/diff/40001/chrome/chrome_browser_u... chrome/chrome_browser_ui.gypi:1949: 'browser/ui/webui/settings/settings_startup_pages_handler.h', ugh, this is why, I suppose... such settings, many text, wow
https://codereview.chromium.org/1405453002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/settings_font_handler.cc (right): https://codereview.chromium.org/1405453002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:7: #include "base/basictypes.h" On 2015/10/24 01:19:47, Dan Beam wrote: > ^ no longer required for size_t Done. https://codereview.chromium.org/1405453002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:8: #include "base/bind.h" On 2015/10/24 01:19:47, Dan Beam wrote: > base/bind_helpers.h for base::Unretained Done. https://codereview.chromium.org/1405453002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:17: #include "content/public/browser/web_ui.h" On 2015/10/24 01:19:47, Dan Beam wrote: > ^ why do you need web_ui.h? It's for web_ui(). Is there a better way to do this? web_ui() is actually in web_ui_message_handler.h, but I'd need to include more files than web_ui_message_handler.h to make it compile. https://codereview.chromium.org/1405453002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:33: weak_ptr_factory_.GetWeakPtr())); On 2015/10/24 01:19:47, Dan Beam wrote: > in this case, use base::Unretained(this) instead of > weak_ptr_factory_.GetWeakPtr() Done. https://codereview.chromium.org/1405453002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:39: weak_ptr_factory_.GetWeakPtr())); On 2015/10/24 01:19:47, Dan Beam wrote: > keep weak_ptr_factory_.GetWeakPtr() for this Acknowledged. https://codereview.chromium.org/1405453002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:43: // Font list. Selects the directionality for the fonts in the given list. On 2015/10/24 01:19:47, Dan Beam wrote: > \s\s -> \s Done. https://codereview.chromium.org/1405453002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:72: CharacterEncoding::GetCanonicalEncodingNameByCommandId(cmd_id); On 2015/10/24 01:19:47, Dan Beam wrote: > option->ApendString( > CharacterEncoding::GetCanonicalEncodingNameByCommandId(it.encoding_id)); > option->AppendString(it.encoding_display_name); > option->AppendString(has_rtl_chars ? "rtl" : "ltr"); Done. https://codereview.chromium.org/1405453002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.cc:80: option->Append(new base::StringValue(std::string())); On 2015/10/24 01:19:47, Dan Beam wrote: > option->AppendString(std::string()); Done. https://codereview.chromium.org/1405453002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/settings_font_handler.h (right): https://codereview.chromium.org/1405453002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/settings_font_handler.h:24: class SettingsFontHandler : public SettingsPageUIHandler { On 2015/10/24 01:19:47, Dan Beam wrote: > out of curiosity, why SettingsFontHandler instead of FontHandler? it's already > in settings:: I'd like the file name to match. I can change both the file name and class name of course. Would FontHandler be preferred? https://codereview.chromium.org/1405453002/diff/40001/chrome/chrome_browser_u... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/1405453002/diff/40001/chrome/chrome_browser_u... chrome/chrome_browser_ui.gypi:1949: 'browser/ui/webui/settings/settings_startup_pages_handler.h', On 2015/10/24 01:19:47, Dan Beam wrote: > ugh, this is why, I suppose... such settings, many text, wow Acknowledged.
lgtm
The CQ bit was checked by dschuyler@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1405453002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1405453002/60001
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a02f4993a13bc0d82c406aa136f1b8fd1fd6114a Cr-Commit-Position: refs/heads/master@{#356139} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
