Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
21868004:
Supports FontList::DeriveFontListWithSizeDeltaAndStyle, etc. (Closed)
|
Created:
7 years, 4 months ago by Yuki Modified:
7 years, 4 months ago CC:
chromium-reviews, craigdh+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org, Peter Kasting, Alexei Svitkine (slow) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSupports FontList::DeriveFontListWithSizeDeltaAndStyle, etc.
This CL improves FontList so the class has useful methods for replacing Font class. Also improves the performance of GetFontStyle() and GetFontSize().
This CL also adds GetAverageCharacterWidth(), GetStringWidth(), and GetExpectedTextWidth(). These methods are supported by Font class, and FontList should implement them for replacing Font.
Just FYI, I'm now working on http://crrev.com/21736002 , and these new methods are used there.
BUG=265485
TEST=Included in the unittests as ui_unittests.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216563
Patch Set 1 #
Total comments: 23
Patch Set 2 : Fixes based on review comments. #
Total comments: 4
Patch Set 3 : Added a TODO comment, updated comments. #
Messages
Total messages: 14 (0 generated)
Could you review this CL?
ping? (Just as a friendly reminder.)
Sorry for the review delay. Alexei should probably take a look too. These new functions match gfx::Font and are mostly used in your followup Label upgrade CL, but this might be a good opportunity to trim some unnecessary API, like GetAverageCharacterWidth or GetExpectedTextWidth, or one of the Derive* functions; WDYT? https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list.cc File ui/gfx/font_list.cc (right): https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list.cc#newcode203 ui/gfx/font_list.cc:203: // TODO(yukishiino): implements this based on the actual font list. nit: Remove this TODO, I think the comment above is sufficient. https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list.cc#newcode261 ui/gfx/font_list.cc:261: &font_style, &font_size); nit: use the class members here. https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list.h File ui/gfx/font_list.h (right): https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list.h#newcode65 ui/gfx/font_list.h:65: // Returns a new FontList with the same font names and style but with resized. nit: s/but with resized/but resized/ (same on line 69) https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list.h#newcode87 ui/gfx/font_list.h:87: // Returns the number of horizontal pixels needed to display the specified nit: s/the specified string/|text|/ https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list.h#newcode94 ui/gfx/font_list.h:94: int GetExpectedTextWidth(int length) const; Is this necessary? It doesn't seem to be used in your followup CL: https://codereview.chromium.org/2173600 https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list_unittest.cc File ui/gfx/font_list_unittest.cc (right): https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list_unittest.cc#... ui/gfx/font_list_unittest.cc:48: const std::string& font_str = font_list.GetFontDescriptionString(); nit: Inline font_list.GetFontDescriptionString() where possible. https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list_unittest.cc#... ui/gfx/font_list_unittest.cc:266: EXPECT_EQ("Arial|13|italic", FontToString(derived_fonts[0])); nit: Check GetFontDescriptionString() instead, where possible.
On 2013/08/06 05:10:14, msw wrote: > Sorry for the review delay. Alexei should probably take a look too. > > These new functions match gfx::Font and are mostly used in your followup Label > upgrade CL, but this might be a good opportunity to trim some unnecessary API, > like GetAverageCharacterWidth or GetExpectedTextWidth, or one of the Derive* > functions; WDYT? Sounds good to me. I think we'd need DeriveFontList(size_delta) DeriveFontList(size_delta, font_style) as the same as Font class has these two methods, and do not need other Derive* family. IMHO, we shouldn't use DeriveFontListWithSize(size) which specify the absolute font size. We should always use the relative size, so the magnifier for accessibility works well. My concerns here are: - Is it okay to overload member functions? I remember Google's C++ style guide discourages overloading. - The current implementation provides DeriveFontList(int style), so it seems a little risky to replace it with DeriveFontList(int size_delta). The signatures are the same. For Get* family, we'd need int GetExpectedTextWidth(int length) int GetStringWidth(const std::string& text) and GetAverageCharacterWidth() will be replaced with GetExpectedTextWidth(1). They must be equivalent to each other. Since the return value is type of int, GetExpectedTextWidth(n) is more accurate than GetAverageCharacterWidth() * n. If you guys are okay, I'll make FontList have DeriveFontList(size_delta) DeriveFontList(size_delta, font_style) GetExpectedTextWidth(int length) GetStringWidth(const std::string& text) and remove other unnecessary methods.
On 2013/08/06 06:02:38, Yuki wrote: > On 2013/08/06 05:10:14, msw wrote: > > Sorry for the review delay. Alexei should probably take a look too. > > > > These new functions match gfx::Font and are mostly used in your followup Label > > upgrade CL, but this might be a good opportunity to trim some unnecessary API, > > like GetAverageCharacterWidth or GetExpectedTextWidth, or one of the Derive* > > functions; WDYT? > > Sounds good to me. > > I think we'd need > DeriveFontList(size_delta) > DeriveFontList(size_delta, font_style) > as the same as Font class has these two methods, and do not need other Derive* > family. > IMHO, we shouldn't use DeriveFontListWithSize(size) which specify the absolute > font size. We should always use the relative size, so the magnifier for > accessibility works well. > > My concerns here are: > - Is it okay to overload member functions? I remember Google's C++ style guide > discourages overloading. > - The current implementation provides DeriveFontList(int style), so it seems a > little risky to replace it with DeriveFontList(int size_delta). The signatures > are the same. > > For Get* family, we'd need > int GetExpectedTextWidth(int length) > int GetStringWidth(const std::string& text) > and GetAverageCharacterWidth() will be replaced with GetExpectedTextWidth(1). > They must be equivalent to each other. > Since the return value is type of int, GetExpectedTextWidth(n) is more accurate > than GetAverageCharacterWidth() * n. > > If you guys are okay, I'll make FontList have > DeriveFontList(size_delta) > DeriveFontList(size_delta, font_style) > GetExpectedTextWidth(int length) > GetStringWidth(const std::string& text) > and remove other unnecessary methods. Sounds good for the most part, but definitely avoid overloading. (DeriveFontListWithSizeDelta[AndStyle] seem verbose, but not awful) I suggest changing Font similarly in a separate CL (first if possible). Check with OWNERS/authors before spending lots of time on this.
https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list.cc File ui/gfx/font_list.cc (right): https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list.cc#newcode96 ui/gfx/font_list.cc:96: font_style, font_size)), I think font_style can go on the previous line and then you just need to align font_size. https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list.cc#newcode213 ui/gfx/font_list.cc:213: if (font_style_ < 0) { Compare explicitly to -1. https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list.cc#newcode214 ui/gfx/font_list.cc:214: if (!fonts_.empty()) This should have {}'s. https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list.cc#newcode242 ui/gfx/font_list.cc:242: if (!fonts_.empty()) This block is almost identical to the one in GetFontStyle(). Refactor it into a private function that initialized both font_size_ and font_style_ and call it from both functions.
Thanks for the comments and advice. I'll remove DeriveFontList(style) and DeriveFontListWithSize(absolute_size) after I remove use of them. By the way, I'd like to refactor views::Label first because it reduces use of obsolete methods and use of gfx::Font::DeriveFont. After that, retirement of DeriveFontList* and renaming of DeriveFont will be easier. Otherwise, I'll have to ask two reviews for the same many files: 1) rename Font::DeriveFont => Font::DeriveFontWithSizeDelta, etc. 2) replace Font::DeriveFontWithSizeDelta => FontList::DeriveFontListWithSizeDelta, etc. Cheers, Yuki Shiino https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list.cc File ui/gfx/font_list.cc (right): https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list.cc#newcode96 ui/gfx/font_list.cc:96: font_style, font_size)), On 2013/08/06 15:15:28, Alexei Svitkine wrote: > I think font_style can go on the previous line and then you just need to align > font_size. Done. https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list.cc#newcode203 ui/gfx/font_list.cc:203: // TODO(yukishiino): implements this based on the actual font list. On 2013/08/06 05:10:14, msw wrote: > nit: Remove this TODO, I think the comment above is sufficient. Done. https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list.cc#newcode213 ui/gfx/font_list.cc:213: if (font_style_ < 0) { On 2013/08/06 15:15:28, Alexei Svitkine wrote: > Compare explicitly to -1. Done. https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list.cc#newcode214 ui/gfx/font_list.cc:214: if (!fonts_.empty()) On 2013/08/06 15:15:28, Alexei Svitkine wrote: > This should have {}'s. Done. https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list.cc#newcode242 ui/gfx/font_list.cc:242: if (!fonts_.empty()) On 2013/08/06 15:15:28, Alexei Svitkine wrote: > This block is almost identical to the one in GetFontStyle(). > > Refactor it into a private function that initialized both font_size_ and > font_style_ and call it from both functions. Thanks for the comment. Applied the same thing to GetHeight() and GetBaseline(). https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list.cc#newcode261 ui/gfx/font_list.cc:261: &font_style, &font_size); On 2013/08/06 05:10:14, msw wrote: > nit: use the class members here. Done. https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list.h File ui/gfx/font_list.h (right): https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list.h#newcode65 ui/gfx/font_list.h:65: // Returns a new FontList with the same font names and style but with resized. On 2013/08/06 05:10:14, msw wrote: > nit: s/but with resized/but resized/ (same on line 69) Done. https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list.h#newcode87 ui/gfx/font_list.h:87: // Returns the number of horizontal pixels needed to display the specified On 2013/08/06 05:10:14, msw wrote: > nit: s/the specified string/|text|/ Done. https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list.h#newcode94 ui/gfx/font_list.h:94: int GetExpectedTextWidth(int length) const; On 2013/08/06 05:10:14, msw wrote: > Is this necessary? It doesn't seem to be used in your followup CL: > https://codereview.chromium.org/2173600 As we discussed, I removed GetAverageCharacterWidth() instead. https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list_unittest.cc File ui/gfx/font_list_unittest.cc (right): https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list_unittest.cc#... ui/gfx/font_list_unittest.cc:48: const std::string& font_str = font_list.GetFontDescriptionString(); On 2013/08/06 05:10:14, msw wrote: > nit: Inline font_list.GetFontDescriptionString() where possible. Done. https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list_unittest.cc#... ui/gfx/font_list_unittest.cc:266: EXPECT_EQ("Arial|13|italic", FontToString(derived_fonts[0])); On 2013/08/06 05:10:14, msw wrote: > nit: Check GetFontDescriptionString() instead, where possible. I guess that the original author intended to test if each derived_fonts[i] is in the right status or not, because: a) If the original FontList is made from vector<Font>, DeriveFontList* performs on vector<Font>, not the font description string. i.e. The internal state of original and derived FontList has vector<Font>. b) The font style and size returned by GetFontDescriptionString() come from the first font in the list. The rest of fonts are ignored. This code tests second font's style and size, too. So I think the original author intentionally avoided use of GetFontDescriptionString(). I'd like to follow that way.
LGTM; thanks! https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list_unittest.cc File ui/gfx/font_list_unittest.cc (right): https://codereview.chromium.org/21868004/diff/1/ui/gfx/font_list_unittest.cc#... ui/gfx/font_list_unittest.cc:266: EXPECT_EQ("Arial|13|italic", FontToString(derived_fonts[0])); On 2013/08/07 14:34:03, Yuki wrote: > On 2013/08/06 05:10:14, msw wrote: > > nit: Check GetFontDescriptionString() instead, where possible. > > I guess that the original author intended to test if each derived_fonts[i] is in > the right status or not, because: > > a) If the original FontList is made from vector<Font>, DeriveFontList* performs > on vector<Font>, not the font description string. i.e. The internal state of > original and derived FontList has vector<Font>. > > b) The font style and size returned by GetFontDescriptionString() come from the > first font in the list. The rest of fonts are ignored. This code tests second > font's style and size, too. > > So I think the original author intentionally avoided use of > GetFontDescriptionString(). I'd like to follow that way. Okay, that's fair; thanks for the explanation.
LGTM, but see my comment below about GetStringWidth() https://codereview.chromium.org/21868004/diff/11001/ui/gfx/font_list.cc File ui/gfx/font_list.cc (right): https://codereview.chromium.org/21868004/diff/11001/ui/gfx/font_list.cc#newco... ui/gfx/font_list.cc:182: return GetPrimaryFont().GetStringWidth(text); This function is pretty bad, since GetStringWidth() ends up using CanvasSkia::GetStringWidth() which ends up constructing a RenderText instance to layout the text and get its width. The problem is that this implementation now loses the FontList info, since it only ends up passing the first font to RenderText. Ideally, callers should stop using this API entirely and go through CanvasSkia directly (to remove the circular dependency), to get this info, or as a stop-gap to add a CanvasSkia::GetStringWidth() API that takes a FontList and passes it to RenderText and then call that version of the API from here. Please do that either in this CL or in a follow-up CL with a TODO here for now. https://codereview.chromium.org/21868004/diff/11001/ui/gfx/font_list.h File ui/gfx/font_list.h (right): https://codereview.chromium.org/21868004/diff/11001/ui/gfx/font_list.h#newcode70 ui/gfx/font_list.h:70: // style. |size_delta| is the size in pixels to add to the current font size. Nit: Single space between sentences to be consistent with the rest of the file.
Thanks for the review. Committing. https://codereview.chromium.org/21868004/diff/11001/ui/gfx/font_list.cc File ui/gfx/font_list.cc (right): https://codereview.chromium.org/21868004/diff/11001/ui/gfx/font_list.cc#newco... ui/gfx/font_list.cc:182: return GetPrimaryFont().GetStringWidth(text); On 2013/08/07 19:30:44, Alexei Svitkine wrote: > This function is pretty bad, since GetStringWidth() ends up using > CanvasSkia::GetStringWidth() which ends up constructing a RenderText instance to > layout the text and get its width. > > The problem is that this implementation now loses the FontList info, since it > only ends up passing the first font to RenderText. > > Ideally, callers should stop using this API entirely and go through CanvasSkia > directly (to remove the circular dependency), to get this info, or as a stop-gap > to add a CanvasSkia::GetStringWidth() API that takes a FontList and passes it to > RenderText and then call that version of the API from here. > > Please do that either in this CL or in a follow-up CL with a TODO here for now. Let me work on it in a separate CL. Added a TODO comment. https://codereview.chromium.org/21868004/diff/11001/ui/gfx/font_list.h File ui/gfx/font_list.h (right): https://codereview.chromium.org/21868004/diff/11001/ui/gfx/font_list.h#newcode70 ui/gfx/font_list.h:70: // style. |size_delta| is the size in pixels to add to the current font size. On 2013/08/07 19:30:44, Alexei Svitkine wrote: > Nit: Single space between sentences to be consistent with the rest of the file. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yukishiino@chromium.org/21868004/21001
Retried try job too often on linux_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yukishiino@chromium.org/21868004/21001
Message was sent while issue was closed.
Change committed as 216563 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
