Renames gfx::FontList::Derive* family.

Renames gfx::FontList::Derive* family.

1. Removes obsolete methods.
- DeriveFontList(font_style)
  # The method name and argument are not clear compared to other versions.  Use DeriveWithStyle(style) below instead.
- DeriveFontListWithSize(absolute_size)
  # Don't specify the absolute size.

2. Renames very long named methods.
- DeriveFontListWithSizeDeltaAndStyle => Derive
- DeriveFontListWithSizeDelta => DeriveWithSizeDelta
- new method => DeriveWithStyle

Since DeriveFontListWithSizeDeltaAndStyle is used more often than DeriveFontListWithSizeDelta, so give the former a shorter name.

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

[Yuki, 2014-01-20 09:26:25]

As Mike suggested before, I'm now planning to rename
FontList::DeriveFontListWithSizeDeltaAndStyle() to Derive().

Could you guys have a quick look only at font_list.h?

If it looks okay, I'll complete this CL.  If you have a preference on naming,
please let me know.

[Alexei Svitkine (slow), 2014-01-20 15:38:10]

I think it would be cleaner to switch the GetStringWidth() changes to their own
CL and then have a separate CL that just refactors the Derive* functions.

https://codereview.chromium.org/142523003/diff/30001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (left):

https://codereview.chromium.org/142523003/diff/30001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:268: font_list =
font_list.DeriveFontListWithSize(desired_font_size);
Hmm, the left side looks cleaner. Is there a good reason for killing it, rather
than still keeping it as a convenience method?

https://codereview.chromium.org/142523003/diff/30001/ui/app_list/views/speech...
File ui/app_list/views/speech_view.cc (right):

https://codereview.chromium.org/142523003/diff/30001/ui/app_list/views/speech...
ui/app_list/views/speech_view.cc:118: ui::ResourceBundle& bundle =
ui::ResourceBundle::GetSharedInstance();
Nit: Non-const refs are discouraged (even though I know the RB is used that way
often). I suggest either making it const if possible, or if not making this a
pointer (by putting & on the right side).

https://codereview.chromium.org/142523003/diff/30001/ui/gfx/font_list.cc
File ui/gfx/font_list.cc (right):

https://codereview.chromium.org/142523003/diff/30001/ui/gfx/font_list.cc#newc...
ui/gfx/font_list.cc:199: FontList FontList::DeriveFontListWithSizeDelta(int
size_delta) const {
Remove this one since it's equivalent to DeriveWithSizeDelta().

[Yuki, 2014-01-20 16:43:24]

I'll separate this CL into two as suggested.
Will work on non-const ref issue in those CLs.

https://codereview.chromium.org/142523003/diff/30001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (left):

https://codereview.chromium.org/142523003/diff/30001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:268: font_list =
font_list.DeriveFontListWithSize(desired_font_size);
On 2014/01/20 15:38:10, Alexei Svitkine wrote:
> Hmm, the left side looks cleaner. Is there a good reason for killing it,
rather
> than still keeping it as a convenience method?

IMHO, all the fonts should ideally have relative size to the BaseFont in
ui::ResourceBundle.  It's not recommended to specify the absolute size assuming
the default font size would be 14px or so.

In order to discourage specifying the absolute size, I'd like to remove this
method from FontList and Font.

https://codereview.chromium.org/142523003/diff/30001/ui/gfx/font_list.cc
File ui/gfx/font_list.cc (right):

https://codereview.chromium.org/142523003/diff/30001/ui/gfx/font_list.cc#newc...
ui/gfx/font_list.cc:199: FontList FontList::DeriveFontListWithSizeDelta(int
size_delta) const {
On 2014/01/20 15:38:10, Alexei Svitkine wrote:
> Remove this one since it's equivalent to DeriveWithSizeDelta().

Will do after I rewrite all calls to this method.

[msw, 2014-01-21 19:31:11]

Just one additional nit beyond what Alexei said.

https://codereview.chromium.org/142523003/diff/30001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/142523003/diff/30001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:267: if
(current_font_size < desired_font_size) {
nit: this should be !=, to also decrease the font size if needed.

[Yuki, 2014-01-23 15:24:19]

https://codereview.chromium.org/142523003/diff/30001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/142523003/diff/30001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:267: if
(current_font_size < desired_font_size) {
On 2014/01/21 19:31:12, msw wrote:
> nit: this should be !=, to also decrease the font size if needed.

Done.

https://codereview.chromium.org/142523003/diff/30001/ui/app_list/views/speech...
File ui/app_list/views/speech_view.cc (right):

https://codereview.chromium.org/142523003/diff/30001/ui/app_list/views/speech...
ui/app_list/views/speech_view.cc:118: ui::ResourceBundle& bundle =
ui::ResourceBundle::GetSharedInstance();
On 2014/01/20 15:38:10, Alexei Svitkine wrote:
> Nit: Non-const refs are discouraged (even though I know the RB is used that
way
> often). I suggest either making it const if possible, or if not making this a
> pointer (by putting & on the right side).

Done.

[msw, 2014-01-23 17:56:44]

LGTM with a couple nits

https://codereview.chromium.org/142523003/diff/160001/ui/gfx/font_list_unitte...
File ui/gfx/font_list_unittest.cc (right):

https://codereview.chromium.org/142523003/diff/160001/ui/gfx/font_list_unitte...
ui/gfx/font_list_unittest.cc:211: FontList font_list = FontList("Arial,Sans
serif,Bold Italic  8px");
nit: should there be two spaces after "Bold Italic"?

https://codereview.chromium.org/142523003/diff/160001/ui/gfx/font_list_unitte...
ui/gfx/font_list_unittest.cc:217: // FontList has a special care for
Font::UNDERLINE.  See if the handling of
nit: should this be "special case"?

[Alexei Svitkine (slow), 2014-01-23 18:47:54]

Please update the CL description.

Also, it seems a lot of callsites seem to call Derive with the first param of 0.
I think it would be cleaner to have a DeriveWithStyle(style) for that use cases,
rather than seeing cryptic 0's.

https://codereview.chromium.org/142523003/diff/160001/ash/ime/infolist_window.cc
File ash/ime/infolist_window.cc (right):

https://codereview.chromium.org/142523003/diff/160001/ash/ime/infolist_window...
ash/ime/infolist_window.cc:173: title_font_list_(gfx::Font(kJapaneseFontName,
kFontSizeDelta + 15)),
I know your code isn't changing this, but I find these defaults pretty bizarre.
Any idea why Japanese font is used explicitly here (rather than getting the
current UI font)?

https://codereview.chromium.org/142523003/diff/160001/ash/sticky_keys/sticky_...
File ash/sticky_keys/sticky_keys_overlay.cc (right):

https://codereview.chromium.org/142523003/diff/160001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:75:
SetFontList(font_list().DeriveWithSizeDelta(4));
Can you comment where the 4 comes from? Are you sure this doesn't result in a
change of behaviour? (i.e. was font list size always 14 on all platforms
before)?

[Yuki, 2014-01-24 10:23:19]

https://codereview.chromium.org/142523003/diff/160001/ash/ime/infolist_window.cc
File ash/ime/infolist_window.cc (right):

https://codereview.chromium.org/142523003/diff/160001/ash/ime/infolist_window...
ash/ime/infolist_window.cc:173: title_font_list_(gfx::Font(kJapaneseFontName,
kFontSizeDelta + 15)),
On 2014/01/23 18:47:54, Alexei Svitkine wrote:
> I know your code isn't changing this, but I find these defaults pretty
bizarre.
> Any idea why Japanese font is used explicitly here (rather than getting the
> current UI font)?

Some people type Japanese text while using English UI.  In the case that the
Chrome's UI language is English and a user wants to type Japanese, the current
UI font list doesn't include any Japanese font.  I think this is the reason.  I
agree that this shouldn't be this way.

https://codereview.chromium.org/142523003/diff/160001/ash/sticky_keys/sticky_...
File ash/sticky_keys/sticky_keys_overlay.cc (right):

https://codereview.chromium.org/142523003/diff/160001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:75:
SetFontList(font_list().DeriveWithSizeDelta(4));
On 2014/01/23 18:47:54, Alexei Svitkine wrote:
> Can you comment where the 4 comes from? Are you sure this doesn't result in a
> change of behaviour? (i.e. was font list size always 14 on all platforms
> before)?

The default font size depends on platforms and user's settings.  But it's about
14px in most cases, I think.  I wasn't sure if we should use MediumFont (+3px)
or not.  I'm now inclined to MediumFont.

For the reason why +4px, I cannot tell you the reason because I don't know why
it was 18px.  Probably there's no special reason.

If you're okay, can I specify MediumFont here?

My point is that we shouldn't specify the absolute font size as much as
possible.
For this case, the original author surely meant this label to be larger than the
default text, but it's possible that this label could be smaller than the
default text if the default font is enough large.

https://codereview.chromium.org/142523003/diff/160001/ui/gfx/font_list_unitte...
File ui/gfx/font_list_unittest.cc (right):

https://codereview.chromium.org/142523003/diff/160001/ui/gfx/font_list_unitte...
ui/gfx/font_list_unittest.cc:211: FontList font_list = FontList("Arial,Sans
serif,Bold Italic  8px");
On 2014/01/23 17:56:45, msw wrote:
> nit: should there be two spaces after "Bold Italic"?

Removed an extra space.

https://codereview.chromium.org/142523003/diff/160001/ui/gfx/font_list_unitte...
ui/gfx/font_list_unittest.cc:217: // FontList has a special care for
Font::UNDERLINE.  See if the handling of
On 2014/01/23 17:56:45, msw wrote:
> nit: should this be "special case"?

Done.

[Alexei Svitkine (slow), 2014-01-24 15:53:41]

https://codereview.chromium.org/142523003/diff/160001/ash/sticky_keys/sticky_...
File ash/sticky_keys/sticky_keys_overlay.cc (right):

https://codereview.chromium.org/142523003/diff/160001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:75:
SetFontList(font_list().DeriveWithSizeDelta(4));
On 2014/01/24 10:23:20, Yuki wrote:
> On 2014/01/23 18:47:54, Alexei Svitkine wrote:
> > Can you comment where the 4 comes from? Are you sure this doesn't result in
a
> > change of behaviour? (i.e. was font list size always 14 on all platforms
> > before)?
> 
> The default font size depends on platforms and user's settings.  But it's
about
> 14px in most cases, I think.  I wasn't sure if we should use MediumFont (+3px)
> or not.  I'm now inclined to MediumFont.
> 
> For the reason why +4px, I cannot tell you the reason because I don't know why
> it was 18px.  Probably there's no special reason.
> 
> If you're okay, can I specify MediumFont here?
> 
> My point is that we shouldn't specify the absolute font size as much as
> possible.
> For this case, the original author surely meant this label to be larger than
the
> default text, but it's possible that this label could be smaller than the
> default text if the default font is enough large.

I agree with the motivation and also agree that if we could use MediumFont, it
would be better. However, I'm worried that making these changes blindly will
regress some UI. So I'd rather you double check this (i.e. post screenshots of
before and after for this view) to make sure it still looks ok after the change.

[Yuki, 2014-01-27 07:03:49]

https://codereview.chromium.org/142523003/diff/160001/ash/sticky_keys/sticky_...
File ash/sticky_keys/sticky_keys_overlay.cc (right):

https://codereview.chromium.org/142523003/diff/160001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:75:
SetFontList(font_list().DeriveWithSizeDelta(4));
On 2014/01/24 15:53:41, Alexei Svitkine wrote:
> On 2014/01/24 10:23:20, Yuki wrote:
> > On 2014/01/23 18:47:54, Alexei Svitkine wrote:
> > > Can you comment where the 4 comes from? Are you sure this doesn't result
in
> a
> > > change of behaviour? (i.e. was font list size always 14 on all platforms
> > > before)?
> > 
> > The default font size depends on platforms and user's settings.  But it's
> about
> > 14px in most cases, I think.  I wasn't sure if we should use MediumFont
(+3px)
> > or not.  I'm now inclined to MediumFont.
> > 
> > For the reason why +4px, I cannot tell you the reason because I don't know
why
> > it was 18px.  Probably there's no special reason.
> > 
> > If you're okay, can I specify MediumFont here?
> > 
> > My point is that we shouldn't specify the absolute font size as much as
> > possible.
> > For this case, the original author surely meant this label to be larger than
> the
> > default text, but it's possible that this label could be smaller than the
> > default text if the default font is enough large.
> 
> I agree with the motivation and also agree that if we could use MediumFont, it
> would be better. However, I'm worried that making these changes blindly will
> regress some UI. So I'd rather you double check this (i.e. post screenshots of
> before and after for this view) to make sure it still looks ok after the
change.

Here are screenshots (on Pixel).  I took screenshots of the full screen so you
can get a feeling compared to other text.  You can see the sticky keys'
indicator at top-left corner.

Original 18px font:
https://docs.google.com/a/chromium.org/file/d/0B3Ozb9XcozcMekt2djE0bnlnMkU/view

MediumFont:
https://docs.google.com/a/chromium.org/file/d/0B3Ozb9XcozcMRXcxZ2RhSlIwOUE/view

LargeFont:
https://docs.google.com/a/chromium.org/file/d/0B3Ozb9XcozcMcDUtYlFSVHZPUXM/view

After taking screenshots, I think the LargeFont is best.  Let me use LargeFont
here, if you guys are okay.

[Alexei Svitkine (slow), 2014-01-27 16:00:38]

LGTM

https://codereview.chromium.org/142523003/diff/160001/ash/sticky_keys/sticky_...
File ash/sticky_keys/sticky_keys_overlay.cc (right):

https://codereview.chromium.org/142523003/diff/160001/ash/sticky_keys/sticky_...
ash/sticky_keys/sticky_keys_overlay.cc:75:
SetFontList(font_list().DeriveWithSizeDelta(4));
On 2014/01/27 07:03:49, Yuki wrote:
> On 2014/01/24 15:53:41, Alexei Svitkine wrote:
> > On 2014/01/24 10:23:20, Yuki wrote:
> > > On 2014/01/23 18:47:54, Alexei Svitkine wrote:
> > > > Can you comment where the 4 comes from? Are you sure this doesn't result
> in
> > a
> > > > change of behaviour? (i.e. was font list size always 14 on all platforms
> > > > before)?
> > > 
> > > The default font size depends on platforms and user's settings.  But it's
> > about
> > > 14px in most cases, I think.  I wasn't sure if we should use MediumFont
> (+3px)
> > > or not.  I'm now inclined to MediumFont.
> > > 
> > > For the reason why +4px, I cannot tell you the reason because I don't know
> why
> > > it was 18px.  Probably there's no special reason.
> > > 
> > > If you're okay, can I specify MediumFont here?
> > > 
> > > My point is that we shouldn't specify the absolute font size as much as
> > > possible.
> > > For this case, the original author surely meant this label to be larger
than
> > the
> > > default text, but it's possible that this label could be smaller than the
> > > default text if the default font is enough large.
> > 
> > I agree with the motivation and also agree that if we could use MediumFont,
it
> > would be better. However, I'm worried that making these changes blindly will
> > regress some UI. So I'd rather you double check this (i.e. post screenshots
of
> > before and after for this view) to make sure it still looks ok after the
> change.
> 
> Here are screenshots (on Pixel).  I took screenshots of the full screen so you
> can get a feeling compared to other text.  You can see the sticky keys'
> indicator at top-left corner.
> 
> Original 18px font:
>
https://docs.google.com/a/chromium.org/file/d/0B3Ozb9XcozcMekt2djE0bnlnMkU/view
> 
> MediumFont:
>
https://docs.google.com/a/chromium.org/file/d/0B3Ozb9XcozcMRXcxZ2RhSlIwOUE/view
> 
> LargeFont:
>
https://docs.google.com/a/chromium.org/file/d/0B3Ozb9XcozcMcDUtYlFSVHZPUXM/view
> 
> After taking screenshots, I think the LargeFont is best.  Let me use LargeFont
> here, if you guys are okay.

Thanks for checking!

Switching to LargeFont indeed seems fine to me here. (Though to be certain, you
might want to double check with the feature author.)

[Yuki, 2014-01-27 16:32:07]

Hi Tim,

Could you take a look at a discussion at
https://codereview.chromium.org/142523003/diff2/160001:480001/ash/sticky_keys...
?

I'm going to change the font size of sticky key indicators from the absolute
size of 18px to ui::ResourceBundle::LargeFont which is relative to the default
base font.

If you have any concerns, please let me know.

Alexei, thank you for your review.

[Tim Song, 2014-01-27 18:56:14]

I got the 18px font size from the mock designs. The screenshots aren't working
for me. Can you reupload them? Also, why is it discouraged to use absolute font
sizes?

[Yuki, 2014-01-28 03:44:00]

On 2014/01/27 18:56:14, Tim Song wrote:
> I got the 18px font size from the mock designs. The screenshots aren't working
> for me. Can you reupload them? Also, why is it discouraged to use absolute
font
> sizes?

I shared them for everyone who knows a link, but somehow it seems that only
those who signed in with an account can see them.  I can see them with any of my
accounts (chromium, personal gmail or corp), but cannot see in a incognito
window.  Just in case, I shared them with your chromium account.

There are several reasons to discourage to use absolute font sizes.
Not all users use the same font size as the default font size.  The default font
size depends on platforms and user's settings.  If you specify the absolute
size, there's no guarantee that it's larger than the default size.
Another reason is various screen resolutions.  For those who use very high
resolution display, 18px is just too small and hard to read.
Yet another reason is that some of people (vision-impaired people for example)
need much more large font size.

In a word, we cannot assume a certain absolute size is the best size.  We need
to be capable to adapt to a variety of environments and users.

[Tim Song, 2014-01-28 19:26:43]

Let me check with the designer if this is okay.

[Tim Song, 2014-01-29 18:23:05]

Okay, I talked to a designer, and it looks like I will need to update the
margins based on the new font size. I looked through the enum of font sizes, and
does MediumBoldFont look closer to the original?

If this patch is blocking you, then I'll lgtm it, and update the margins when I
get the new mocks.

[Yuki, 2014-01-30 02:58:34]

On 2014/01/29 18:23:05, Tim Song wrote:
> Okay, I talked to a designer, and it looks like I will need to update the
> margins based on the new font size. I looked through the enum of font sizes,
and
> does MediumBoldFont look closer to the original?
> 
> If this patch is blocking you, then I'll lgtm it, and update the margins when
I
> get the new mocks.

Thanks for checking this.  I'll commit this CL as is, and leave the details for
you.

Maybe MediumBoldFont would be best, maybe LargeFont (or LargeBoldFont) would be
best.
Since this feature is a sort of accessibility assistance, we may want to avoid
small font.  Too large would be better than too small, I think.  But, it's up to
you (and the designer).

[commit-bot: I haz the power, 2014-01-30 02:58:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yukishiino@chromium.org/142523003/480001

[Yuki, 2014-01-30 03:31:29]

Add more owner reviewers.
Could you guys review this CL?

komatsu:
  ash/ime/

xiyuan:
  ui/app_list/

tony:
  ui/base/resource/

sadrul:
  ui/views/

[Hiro Komatsu, 2014-01-30 03:35:55]

On 2014/01/30 03:31:29, Yuki wrote:
> Add more owner reviewers.
> Could you guys review this CL?
> 
> komatsu:
>   ash/ime/
> 
> xiyuan:
>   ui/app_list/
> 
> tony:
>   ui/base/resource/
> 
> sadrul:
>   ui/views/

ash/ime/: lgtm

[commit-bot: I haz the power, 2014-01-30 03:40:46]

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

[xiyuan, 2014-01-30 03:49:27]

+mukai for the speech view label font size change.

Mukai, the CL changes from absolute size 20 font to large font (which is
probably close to size 20 on chrmeos). I think it is okay but I did not talk to
the UX people.  Please double check.

https://codereview.chromium.org/142523003/diff/480001/ui/app_list/views/speec...
File ui/app_list/views/speech_view.cc (right):

https://codereview.chromium.org/142523003/diff/480001/ui/app_list/views/speec...
ui/app_list/views/speech_view.cc:118: ui::ResourceBundle* bundle =
&ui::ResourceBundle::GetSharedInstance();
Thought "ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();" is
more popular in current code base.

[Jun Mukai, 2014-01-30 04:02:37]

https://codereview.chromium.org/142523003/diff/480001/ui/app_list/views/speec...
File ui/app_list/views/speech_view.cc (right):

https://codereview.chromium.org/142523003/diff/480001/ui/app_list/views/speec...
ui/app_list/views/speech_view.cc:118: ui::ResourceBundle* bundle =
&ui::ResourceBundle::GetSharedInstance();
On 2014/01/30 03:49:27, xiyuan wrote:
> Thought "ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();" is
> more popular in current code base.

+1 to xiyuan

https://codereview.chromium.org/142523003/diff/480001/ui/app_list/views/speec...
ui/app_list/views/speech_view.cc:120: base::string16(),
bundle->GetFontList(ui::ResourceBundle::LargeFont));
Not sure how large is LargeFont. Could you give me the data?

https://codereview.chromium.org/142523003/diff/480001/ui/app_list/views/speec...
ui/app_list/views/speech_view.cc:141: ui::ResourceBundle* bundle =
&ui::ResourceBundle::GetSharedInstance();
ditto

[Jun Mukai, 2014-01-30 04:10:10]

https://codereview.chromium.org/142523003/diff/480001/ui/app_list/views/speec...
File ui/app_list/views/speech_view.cc (right):

https://codereview.chromium.org/142523003/diff/480001/ui/app_list/views/speec...
ui/app_list/views/speech_view.cc:120: base::string16(),
bundle->GetFontList(ui::ResourceBundle::LargeFont));
On 2014/01/30 04:02:37, Jun Mukai wrote:
> Not sure how large is LargeFont. Could you give me the data?

note that the spec says that this text is 20sp. I don't think we have to follow
this exactly but want to choose the closest one.

[xiyuan, 2014-01-30 05:18:35]

https://codereview.chromium.org/142523003/diff/480001/ui/app_list/views/speec...
File ui/app_list/views/speech_view.cc (right):

https://codereview.chromium.org/142523003/diff/480001/ui/app_list/views/speec...
ui/app_list/views/speech_view.cc:120: base::string16(),
bundle->GetFontList(ui::ResourceBundle::LargeFont));
On 2014/01/30 04:10:10, Jun Mukai wrote:
> On 2014/01/30 04:02:37, Jun Mukai wrote:
> > Not sure how large is LargeFont. Could you give me the data?
> 
> note that the spec says that this text is 20sp. I don't think we have to
follow
> this exactly but want to choose the closest one.

My impression is that ChromeOS base font is defined in IDS_UI_FONT_FAMILY_CROS,
which is "Noto Sans UI,ui-sans, 12px" for en. LargeFont is base font size +
kLargeFontSizeDelta (which is 8). It should be 20px for en.

But be warned that I might be totally wrong. :) Let's see what Yuki responds.

[Yuki, 2014-01-30 05:30:34]

https://codereview.chromium.org/142523003/diff/480001/ui/app_list/views/speec...
File ui/app_list/views/speech_view.cc (right):

https://codereview.chromium.org/142523003/diff/480001/ui/app_list/views/speec...
ui/app_list/views/speech_view.cc:118: ui::ResourceBundle* bundle =
&ui::ResourceBundle::GetSharedInstance();
On 2014/01/30 04:02:37, Jun Mukai wrote:
> On 2014/01/30 03:49:27, xiyuan wrote:
> > Thought "ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();"
is
> > more popular in current code base.
> 
> +1 to xiyuan

Our policy is to use a const reference or a pointer (to a mutable object). 
ui::ResourceBundle is violating this guideline.  GetSharedInstance() should
return ui::ResourceBundle* instead of ui::ResourceBundle&.

This is the reason why I used ui::ResourceBundle* here.
I personally don't insist this point, but there are not few reviewers who highly
recommend a pointer to a mutable object.

https://codereview.chromium.org/142523003/diff/480001/ui/app_list/views/speec...
ui/app_list/views/speech_view.cc:120: base::string16(),
bundle->GetFontList(ui::ResourceBundle::LargeFont));
On 2014/01/30 04:10:10, Jun Mukai wrote:
> On 2014/01/30 04:02:37, Jun Mukai wrote:
> > Not sure how large is LargeFont. Could you give me the data?
> 
> note that the spec says that this text is 20sp. I don't think we have to
follow
> this exactly but want to choose the closest one.

The current definition of font sizes are as follows.  They may change in future
though.

SmallFont = -1px relative to the default font size
MediumFont = +3px relative to the default font size
LargeFont = +8px relative to the default font size

Typical default font size would be 12px or 13px, but could be a different size
depending on platforms and user's settings.

The current definition on CrOS is 12px in English UI and 13px in most other
languages.  On Windows, it depends on user's font settings.
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/strings/ap...

[Jun Mukai, 2014-01-30 06:00:55]

https://codereview.chromium.org/142523003/diff/480001/ui/app_list/views/speec...
File ui/app_list/views/speech_view.cc (right):

https://codereview.chromium.org/142523003/diff/480001/ui/app_list/views/speec...
ui/app_list/views/speech_view.cc:118: ui::ResourceBundle* bundle =
&ui::ResourceBundle::GetSharedInstance();
On 2014/01/30 05:30:35, Yuki wrote:
> On 2014/01/30 04:02:37, Jun Mukai wrote:
> > On 2014/01/30 03:49:27, xiyuan wrote:
> > > Thought "ui::ResourceBundle& rb =
ui::ResourceBundle::GetSharedInstance();"
> is
> > > more popular in current code base.
> > 
> > +1 to xiyuan
> 
> Our policy is to use a const reference or a pointer (to a mutable object). 
> ui::ResourceBundle is violating this guideline.  GetSharedInstance() should
> return ui::ResourceBundle* instead of ui::ResourceBundle&.
> 
> This is the reason why I used ui::ResourceBundle* here.
> I personally don't insist this point, but there are not few reviewers who
highly
> recommend a pointer to a mutable object.

I don't want to argue here and I'm really happy if you want to refactor
ResourceBundle.
However, for some reasons I don't know, ui::ResourceBundle is widely used as
non-const reference right now, all among our code base.
see;
https://code.google.com/p/chromium/codesearch#search/&q=ui::ResourceBundle&am...

This situation isn't ideal, but that doesn't justify using a pointer here by
itself.

Anyways, I think there are two ways. Stop this CL and do your refactoring of
ResourceBundle first, or move this with const reference and revisit the issue of
ResourceBundle::SharedInstance() later.
Either is fine.

https://codereview.chromium.org/142523003/diff/480001/ui/app_list/views/speec...
ui/app_list/views/speech_view.cc:120: base::string16(),
bundle->GetFontList(ui::ResourceBundle::LargeFont));
On 2014/01/30 05:30:35, Yuki wrote:
> On 2014/01/30 04:10:10, Jun Mukai wrote:
> > On 2014/01/30 04:02:37, Jun Mukai wrote:
> > > Not sure how large is LargeFont. Could you give me the data?
> > 
> > note that the spec says that this text is 20sp. I don't think we have to
> follow
> > this exactly but want to choose the closest one.
> 
> The current definition of font sizes are as follows.  They may change in
future
> though.
> 
> SmallFont = -1px relative to the default font size
> MediumFont = +3px relative to the default font size
> LargeFont = +8px relative to the default font size
> 
> Typical default font size would be 12px or 13px, but could be a different size
> depending on platforms and user's settings.
> 
> The current definition on CrOS is 12px in English UI and 13px in most other
> languages.  On Windows, it depends on user's font settings.
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/strings/ap...

okay, go ahead.

[Yuki, 2014-01-30 06:29:55]

https://codereview.chromium.org/142523003/diff/480001/ui/app_list/views/speec...
File ui/app_list/views/speech_view.cc (right):

https://codereview.chromium.org/142523003/diff/480001/ui/app_list/views/speec...
ui/app_list/views/speech_view.cc:118: ui::ResourceBundle* bundle =
&ui::ResourceBundle::GetSharedInstance();
On 2014/01/30 06:00:56, Jun Mukai wrote:
> On 2014/01/30 05:30:35, Yuki wrote:
> > On 2014/01/30 04:02:37, Jun Mukai wrote:
> > > On 2014/01/30 03:49:27, xiyuan wrote:
> > > > Thought "ui::ResourceBundle& rb =
> ui::ResourceBundle::GetSharedInstance();"
> > is
> > > > more popular in current code base.
> > > 
> > > +1 to xiyuan
> > 
> > Our policy is to use a const reference or a pointer (to a mutable object). 
> > ui::ResourceBundle is violating this guideline.  GetSharedInstance() should
> > return ui::ResourceBundle* instead of ui::ResourceBundle&.
> > 
> > This is the reason why I used ui::ResourceBundle* here.
> > I personally don't insist this point, but there are not few reviewers who
> highly
> > recommend a pointer to a mutable object.
> 
> I don't want to argue here and I'm really happy if you want to refactor
> ResourceBundle.
> However, for some reasons I don't know, ui::ResourceBundle is widely used as
> non-const reference right now, all among our code base.
> see;
>
https://code.google.com/p/chromium/codesearch#search/&q=ui::ResourceBundle&am...
> 
> This situation isn't ideal, but that doesn't justify using a pointer here by
> itself.
> 
> Anyways, I think there are two ways. Stop this CL and do your refactoring of
> ResourceBundle first, or move this with const reference and revisit the issue
of
> ResourceBundle::SharedInstance() later.
> Either is fine.

Done.

https://codereview.chromium.org/142523003/diff/480001/ui/app_list/views/speec...
ui/app_list/views/speech_view.cc:141: ui::ResourceBundle* bundle =
&ui::ResourceBundle::GetSharedInstance();
On 2014/01/30 04:02:37, Jun Mukai wrote:
> ditto

Done.

[Yuki, 2014-01-30 06:33:06]

+sadrul for ui/views/

[sadrul, 2014-01-30 06:34:21]

ui/views/ lgtm

[Jun Mukai, 2014-01-30 06:39:27]

sorry for taking time. lgtm.

[xiyuan, 2014-01-30 07:08:21]

app_list LGTM

[tony, 2014-01-30 18:44:45]

ui/base/resource LGTM.

As for the pointer vs reference issue, it would probably be nice to switch
ResourceBundle to a LeakySingleton.  At the time ResourceBundle was added,
LeakySingleton didn't exist.

[tfarina, 2014-01-30 22:58:58]

For the ResourceBundle issue I have crbug.com/289640 filed.

On Thu, Jan 30, 2014 at 4:44 PM, <tony@chromium.org> wrote:

> ui/base/resource LGTM.
>
> As for the pointer vs reference issue, it would probably be nice to switch
> ResourceBundle to a LeakySingleton.  At the time ResourceBundle was added,
> LeakySingleton didn't exist.
>
>
I will file another one for this.

-- 
Thiago

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tfarina, 2014-01-31 01:40:57]

On Thu, Jan 30, 2014 at 4:44 PM, <tony@chromium.org> wrote:

> ui/base/resource LGTM.
>
> As for the pointer vs reference issue, it would probably be nice to switch
> ResourceBundle to a LeakySingleton.


OK, filed crbug.com/339672 for that.

-- 
Thiago

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-01-31 02:35:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yukishiino@chromium.org/142523003/590001

[commit-bot: I haz the power, 2014-01-31 05:31:55]

Change committed as 248131

[commit-bot: I haz the power, 2014-01-31 05:32:04]

CQ bit was unchecked on CL. Ignoring.