|
|
Created:
8 years, 6 months ago by horo Modified:
8 years, 6 months ago CC:
chromium-reviews, davemoore+watch_chromium.org, stevenjb+watch_chromium.org, yusukes+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionTemporary fix for Infolist font issue.
BUG=129507
TEST=gmerge passes,manually done
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141171
Patch Set 1 #
Total comments: 3
Patch Set 2 : Support Chromium OS #
Total comments: 11
Patch Set 3 : #
Messages
Total messages: 10 (0 generated)
https://chromiumcodereview.appspot.com/10537022/diff/1/chrome/browser/chromeo... File chrome/browser/chromeos/input_method/candidate_window.cc (right): https://chromiumcodereview.appspot.com/10537022/diff/1/chrome/browser/chromeo... chrome/browser/chromeos/input_method/candidate_window.cc:1370: description_font_(new gfx::Font("MotoyaG04Gothic", kFontSizeDelta + 11)) { IIRC Motoya font is only available official Chrome OS device. What is happened if there is no motoya font in device?
drive-by: https://chromiumcodereview.appspot.com/10537022/diff/1/chrome/browser/chromeo... File chrome/browser/chromeos/input_method/candidate_window.cc (right): https://chromiumcodereview.appspot.com/10537022/diff/1/chrome/browser/chromeo... chrome/browser/chromeos/input_method/candidate_window.cc:1370: description_font_(new gfx::Font("MotoyaG04Gothic", kFontSizeDelta + 11)) { On 2012/06/06 06:20:16, Seigo Nonaka wrote: > IIRC Motoya font is only available official Chrome OS device. What is happened > if there is no motoya font in device? Or you can just do something like: namespace { #if defined(GOOGLE_CHROME_BUILD) const char kJapaneseFontName[] = "Motoya..."; #else const char kJapaneseFontName[] = "IPA..."; #endif } // namespace
https://chromiumcodereview.appspot.com/10537022/diff/1/chrome/browser/chromeo... File chrome/browser/chromeos/input_method/candidate_window.cc (right): https://chromiumcodereview.appspot.com/10537022/diff/1/chrome/browser/chromeo... chrome/browser/chromeos/input_method/candidate_window.cc:1370: description_font_(new gfx::Font("MotoyaG04Gothic", kFontSizeDelta + 11)) { On 2012/06/07 05:16:19, Yusuke Sato wrote: > On 2012/06/06 06:20:16, Seigo Nonaka wrote: > > IIRC Motoya font is only available official Chrome OS device. What is happened > > if there is no motoya font in device? > > Or you can just do something like: > > namespace { > > #if defined(GOOGLE_CHROME_BUILD) > const char kJapaneseFontName[] = "Motoya..."; > #else > const char kJapaneseFontName[] = "IPA..."; > #endif > > } // namespace Done. Thank you for the suggestion.
lgtm LGTM with nits. https://chromiumcodereview.appspot.com/10537022/diff/6001/chrome/browser/chro... File chrome/browser/chromeos/input_method/candidate_window.cc (right): https://chromiumcodereview.appspot.com/10537022/diff/6001/chrome/browser/chro... chrome/browser/chromeos/input_method/candidate_window.cc:69: // TODO: Currently the infolist window only supports Japanese font. nit: Please leave user name for TODO or just remove TODO: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=TODO_C...
drive-by: http://codereview.chromium.org/10537022/diff/6001/chrome/browser/chromeos/inp... File chrome/browser/chromeos/input_method/candidate_window.cc (right): http://codereview.chromium.org/10537022/diff/6001/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/candidate_window.cc:69: // TODO: Currently the infolist window only supports Japanese font. TODO requires an assignee. http://codereview.chromium.org/10537022/diff/6001/chrome/browser/chromeos/inp... File chrome/browser/chromeos/input_method/candidate_window_view.h (right): http://codereview.chromium.org/10537022/diff/6001/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/candidate_window_view.h:13: namespace gfx { class Font; } http://codereview.chromium.org/10537022/diff/6001/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/candidate_window_view.h:234: gfx::Font GetTitleFont(); const; ? http://codereview.chromium.org/10537022/diff/6001/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/candidate_window_view.h:234: gfx::Font GetTitleFont(); I briefly checked ui/gfx/platform_font_pango.h. Looks like copying gfx::Font by value is not that cheap. How about returning a const reference to the object? http://codereview.chromium.org/10537022/diff/6001/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/candidate_window_view.h:235: gfx::Font GetDescriptionFont(); ditto.
http://codereview.chromium.org/10537022/diff/6001/chrome/browser/chromeos/inp... File chrome/browser/chromeos/input_method/candidate_window.cc (right): http://codereview.chromium.org/10537022/diff/6001/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/candidate_window.cc:69: // TODO: Currently the infolist window only supports Japanese font. On 2012/06/08 02:17:38, Seigo Nonaka wrote: > nit: Please leave user name for TODO or just remove TODO: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=TODO_C... Done. http://codereview.chromium.org/10537022/diff/6001/chrome/browser/chromeos/inp... File chrome/browser/chromeos/input_method/candidate_window_view.h (right): http://codereview.chromium.org/10537022/diff/6001/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/candidate_window_view.h:13: On 2012/06/08 02:22:42, Yusuke Sato wrote: > namespace gfx { class Font; } Done. http://codereview.chromium.org/10537022/diff/6001/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/candidate_window_view.h:234: gfx::Font GetTitleFont(); On 2012/06/08 02:22:42, Yusuke Sato wrote: > const; ? Done. http://codereview.chromium.org/10537022/diff/6001/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/candidate_window_view.h:234: gfx::Font GetTitleFont(); On 2012/06/08 02:22:42, Yusuke Sato wrote: > I briefly checked ui/gfx/platform_font_pango.h. Looks like copying gfx::Font by > value is not that cheap. How about returning a const reference to the object? Copying gfx::Font by value does not causes copying PlatformFont by value. It causes only AddRef(). ui/gfx/font.h // Font provides a wrapper around an underlying font. Copy and assignment // operators are explicitly allowed, and cheap. coped_refptr<PlatformFont> platform_font_; http://codereview.chromium.org/10537022/diff/6001/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/candidate_window_view.h:235: gfx::Font GetDescriptionFont(); On 2012/06/08 02:22:42, Yusuke Sato wrote: > ditto. Done.
lgtm On 2012/06/08 03:01:12, horo wrote: > http://codereview.chromium.org/10537022/diff/6001/chrome/browser/chromeos/inp... > File chrome/browser/chromeos/input_method/candidate_window.cc (right): > > http://codereview.chromium.org/10537022/diff/6001/chrome/browser/chromeos/inp... > chrome/browser/chromeos/input_method/candidate_window.cc:69: // TODO: Currently > the infolist window only supports Japanese font. > On 2012/06/08 02:17:38, Seigo Nonaka wrote: > > nit: Please leave user name for TODO or just remove TODO: > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=TODO_C... > > Done. > > http://codereview.chromium.org/10537022/diff/6001/chrome/browser/chromeos/inp... > File chrome/browser/chromeos/input_method/candidate_window_view.h (right): > > http://codereview.chromium.org/10537022/diff/6001/chrome/browser/chromeos/inp... > chrome/browser/chromeos/input_method/candidate_window_view.h:13: > On 2012/06/08 02:22:42, Yusuke Sato wrote: > > namespace gfx { class Font; } > > Done. > > http://codereview.chromium.org/10537022/diff/6001/chrome/browser/chromeos/inp... > chrome/browser/chromeos/input_method/candidate_window_view.h:234: gfx::Font > GetTitleFont(); > On 2012/06/08 02:22:42, Yusuke Sato wrote: > > const; ? > > Done. > > http://codereview.chromium.org/10537022/diff/6001/chrome/browser/chromeos/inp... > chrome/browser/chromeos/input_method/candidate_window_view.h:234: gfx::Font > GetTitleFont(); > On 2012/06/08 02:22:42, Yusuke Sato wrote: > > I briefly checked ui/gfx/platform_font_pango.h. Looks like copying gfx::Font > by > > value is not that cheap. How about returning a const reference to the object? > > Copying gfx::Font by value does not causes copying PlatformFont by value. > It causes only AddRef(). > > ui/gfx/font.h > // Font provides a wrapper around an underlying font. Copy and assignment > // operators are explicitly allowed, and cheap. > > coped_refptr<PlatformFont> platform_font_; > > http://codereview.chromium.org/10537022/diff/6001/chrome/browser/chromeos/inp... > chrome/browser/chromeos/input_method/candidate_window_view.h:235: gfx::Font > GetDescriptionFont(); > On 2012/06/08 02:22:42, Yusuke Sato wrote: > > ditto. > > Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/horo@chromium.org/10537022/11001
Change committed as 141171 |