Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
24883002:
Uses and returns the fractional width in text eliding (Closed)
|
Created:
7 years, 2 months ago by jianli Modified:
7 years, 2 months ago CC:
chromium-reviews, tfarina, sadrul, ben+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUses and returns the fractional width in text eliding
On Mac, the width is repsented as CGFloat due to possible sub-pixel
rendering and extra kerning. We need to return the fractional widt,
instead of the integral width.
Some tests have been updated with the hack removed.
BUG=288987
TEST=new tests plus updating existing tests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=227905
Patch Set 1 : Patch #
Total comments: 27
Patch Set 2 : Fix per feedbacks #Patch Set 3 : Fix win bots #
Total comments: 16
Patch Set 4 : More fixes per feedback plus new tests #
Total comments: 10
Patch Set 5 : Sync #Patch Set 6 : More fixes per feedback #
Total comments: 3
Patch Set 7 : Fix #Patch Set 8 : Sync #Patch Set 9 : Fix round-down problems #
Total comments: 4
Patch Set 10 : Update comment per feedback #Patch Set 11 : Add float version only #Patch Set 12 : Fix iOS build #Patch Set 13 : Fix trybots #
Total comments: 6
Patch Set 14 : Fix per feedbacks #
Total comments: 14
Patch Set 15 : Fix per more feedback #
Total comments: 6
Patch Set 16 : Address more feedback #Patch Set 17 : Sync #
Messages
Total messages: 45 (0 generated)
msw and asvitkine for text rendering and eliding changes. sky for owner approval for all other changes related to API parameter type change.
Three larger comments/questions: 1) Add a unit test somewhere that actually retrieves a non-integral string size. 2) Should Font:: and FontList::GetStringWidth be updated to return float values too? 3) Should we be concerned over the performance hit of using floating point math now? https://codereview.chromium.org/24883002/diff/3001/ash/system/user/tray_user.cc File ash/system/user/tray_user.cc (right): https://codereview.chromium.org/24883002/diff/3001/ash/system/user/tray_user.... ash/system/user/tray_user.cc:460: static_cast<float>(contents_area.width()), Won't int->float implicit type conversion work here, and in similar cases? https://codereview.chromium.org/24883002/diff/3001/ui/gfx/canvas.h File ui/gfx/canvas.h (right): https://codereview.chromium.org/24883002/diff/3001/ui/gfx/canvas.h#newcode128 ui/gfx/canvas.h:128: static void SizeStringInt(const base::string16& text, I presume the "Int" suffix in the name of this function was meant to correlate with the integral size it previously returned. Can you update this now, since you're touching all of its references anyway? https://codereview.chromium.org/24883002/diff/3001/ui/gfx/canvas_skia.cc File ui/gfx/canvas_skia.cc (right): https://codereview.chromium.org/24883002/diff/3001/ui/gfx/canvas_skia.cc#newc... ui/gfx/canvas_skia.cc:172: float* width, float* height, nit: one arg per line here, perhaps the function should return a SizeF? https://codereview.chromium.org/24883002/diff/3001/ui/gfx/canvas_unittest_mac.mm File ui/gfx/canvas_unittest_mac.mm (right): https://codereview.chromium.org/24883002/diff/3001/ui/gfx/canvas_unittest_mac... ui/gfx/canvas_unittest_mac.mm:21: void CanvasMac_SizeStringInt(const base::string16& text, Is updating this important? Is it even worth keeping at all? If anything, maybe it's worthwhile to keep a direct comparison between Canvas::SizeStringInt and sizeWithAttributes, inlining some of this function within a single test. https://codereview.chromium.org/24883002/diff/3001/ui/gfx/canvas_unittest_mac... ui/gfx/canvas_unittest_mac.mm:66: // mac_height is the truncated height. nit: Consider "CanvasMac_SizeStringInt returns a truncated height." https://codereview.chromium.org/24883002/diff/3001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/24883002/diff/3001/ui/gfx/render_text.cc#newc... ui/gfx/render_text.cc:648: Size size = GetStringSize(); nit: const https://codereview.chromium.org/24883002/diff/3001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/24883002/diff/3001/ui/gfx/render_text.h#newco... ui/gfx/render_text.h:331: // The default implementation is same as GetStringSize. Certain platform, like nit: "Certain platforms that compute the text size as floating-point values, like Mac, will override this method." https://codereview.chromium.org/24883002/diff/3001/ui/gfx/text_elider_unittes... File ui/gfx/text_elider_unittest.cc (right): https://codereview.chromium.org/24883002/diff/3001/ui/gfx/text_elider_unittes... ui/gfx/text_elider_unittest.cc:45: float GetStringWidth(const string16& string, const Font& font) { All the callsites for this function should construct FontLists instead of constructing a Font and then making a FontList out of that. Also, each callsite can potentially use FontList::GetStringWidth or gfx::GetStringWidth directly. https://codereview.chromium.org/24883002/diff/3001/ui/gfx/text_elider_unittes... ui/gfx/text_elider_unittest.cc:46: gfx::FontList font_list(font); nit: inline this, or make it const. https://codereview.chromium.org/24883002/diff/3001/ui/gfx/text_elider_unittes... ui/gfx/text_elider_unittest.cc:416: for (float width = 0; width <= kTestStringWidth; width += 1) { nit: ++width or leave this as it was. Why "+=1"? https://codereview.chromium.org/24883002/diff/3001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/24883002/diff/3001/ui/views/controls/label.cc... ui/views/controls/label.cc:194: static_cast<int>(gfx::GetStringWidth(*iter, font_list_))); Shouldn't this retain the floating point value? Either way, use std::max<int|float> instead of static casting an arg, if possible. https://codereview.chromium.org/24883002/diff/3001/ui/views/controls/label.cc... ui/views/controls/label.cc:254: float fw = w; |fw| isn't really used after its value is updated below, because you changed line 258 to use |w| instead of |fw|/|cache_width|. Can you clean this code up? https://codereview.chromium.org/24883002/diff/3001/ui/views/widget/tooltip_ma... File ui/views/widget/tooltip_manager.cc (right): https://codereview.chromium.org/24883002/diff/3001/ui/views/widget/tooltip_ma... ui/views/widget/tooltip_manager.cc:57: static_cast<int>(gfx::GetStringWidth(elided_text, font_list))); Use std::max<int> instead (or float if that's more appropriate).
On Fri, Sep 27, 2013 at 2:54 PM, <msw@chromium.org> wrote: > Three larger comments/questions: > 1) Add a unit test somewhere that actually retrieves a non-integral string > size. > I will add a unit test for that. > 2) Should Font:: and FontList::GetStringWidth be updated to return float > values > too? > Yes, I will update Font/FontList::GetStringWidth since I just find out some test fails due to the inconsistency between integral width returned from Font::GetStringWidth and fractional width used in Elide functions. > 3) Should we be concerned over the performance hit of using floating point > math > now? > Good question. I think we only have very little perform loss here since GetStringWidth and all these Elide functions are only called several times to arrange the text in some UI controls, like buttons and labels. > > > https://codereview.chromium.**org/24883002/diff/3001/ash/** > system/user/tray_user.cc<https://codereview.chromium.org/24883002/diff/3001/ash/system/user/tray_user.cc> > File ash/system/user/tray_user.cc (right): > > https://codereview.chromium.**org/24883002/diff/3001/ash/** > system/user/tray_user.cc#**newcode460<https://codereview.chromium.org/24883002/diff/3001/ash/system/user/tray_user.cc#newcode460> > ash/system/user/tray_user.cc:**460: > static_cast<float>(contents_**area.width()), > Won't int->float implicit type conversion work here, and in similar > cases? > > https://codereview.chromium.**org/24883002/diff/3001/ui/gfx/**canvas.h<https:... > File ui/gfx/canvas.h (right): > > https://codereview.chromium.**org/24883002/diff/3001/ui/gfx/** > canvas.h#newcode128<https://codereview.chromium.org/24883002/diff/3001/ui/gfx/canvas.h#newcode128> > ui/gfx/canvas.h:128: static void SizeStringInt(const base::string16& > text, > I presume the "Int" suffix in the name of this function was meant to > correlate with the integral size it previously returned. Can you update > this now, since you're touching all of its references anyway? > > https://codereview.chromium.**org/24883002/diff/3001/ui/gfx/** > canvas_skia.cc<https://codereview.chromium.org/24883002/diff/3001/ui/gfx/canvas_skia.cc> > File ui/gfx/canvas_skia.cc (right): > > https://codereview.chromium.**org/24883002/diff/3001/ui/gfx/** > canvas_skia.cc#newcode172<https://codereview.chromium.org/24883002/diff/3001/ui/gfx/canvas_skia.cc#newcode172> > ui/gfx/canvas_skia.cc:172: float* width, float* height, > nit: one arg per line here, perhaps the function should return a SizeF? > > https://codereview.chromium.**org/24883002/diff/3001/ui/gfx/** > canvas_unittest_mac.mm<https://codereview.chromium.org/24883002/diff/3001/ui/gfx/canvas_unittest_mac.mm> > File ui/gfx/canvas_unittest_mac.mm (right): > > https://codereview.chromium.**org/24883002/diff/3001/ui/gfx/** > canvas_unittest_mac.mm#**newcode21<https://codereview.chromium.org/24883002/diff/3001/ui/gfx/canvas_unittest_mac.mm#newcode21> > ui/gfx/canvas_unittest_mac.mm:**21 <http://canvas_unittest_mac.mm:21>: > void CanvasMac_SizeStringInt(const > base::string16& text, > Is updating this important? Is it even worth keeping at all? If > anything, maybe it's worthwhile to keep a direct comparison between > Canvas::SizeStringInt and sizeWithAttributes, inlining some of this > function within a single test. > > https://codereview.chromium.**org/24883002/diff/3001/ui/gfx/** > canvas_unittest_mac.mm#**newcode66<https://codereview.chromium.org/24883002/diff/3001/ui/gfx/canvas_unittest_mac.mm#newcode66> > ui/gfx/canvas_unittest_mac.mm:**66 <http://canvas_unittest_mac.mm:66>: // > mac_height is the truncated height. > nit: Consider "CanvasMac_SizeStringInt returns a truncated height." > > https://codereview.chromium.**org/24883002/diff/3001/ui/gfx/** > render_text.cc<https://codereview.chromium.org/24883002/diff/3001/ui/gfx/render_text.cc> > File ui/gfx/render_text.cc (right): > > https://codereview.chromium.**org/24883002/diff/3001/ui/gfx/** > render_text.cc#newcode648<https://codereview.chromium.org/24883002/diff/3001/ui/gfx/render_text.cc#newcode648> > ui/gfx/render_text.cc:648: Size size = GetStringSize(); > nit: const > > https://codereview.chromium.**org/24883002/diff/3001/ui/gfx/** > render_text.h<https://codereview.chromium.org/24883002/diff/3001/ui/gfx/render_text.h> > File ui/gfx/render_text.h (right): > > https://codereview.chromium.**org/24883002/diff/3001/ui/gfx/** > render_text.h#newcode331<https://codereview.chromium.org/24883002/diff/3001/ui/gfx/render_text.h#newcode331> > ui/gfx/render_text.h:331: // The default implementation is same as > GetStringSize. Certain platform, like > nit: "Certain platforms that compute the text size as floating-point > values, like Mac, will override this method." > > https://codereview.chromium.**org/24883002/diff/3001/ui/gfx/** > text_elider_unittest.cc<https://codereview.chromium.org/24883002/diff/3001/ui/gfx/text_elider_unittest.cc> > File ui/gfx/text_elider_unittest.cc (right): > > https://codereview.chromium.**org/24883002/diff/3001/ui/gfx/** > text_elider_unittest.cc#**newcode45<https://codereview.chromium.org/24883002/diff/3001/ui/gfx/text_elider_unittest.cc#newcode45> > ui/gfx/text_elider_unittest.**cc:45: float GetStringWidth(const string16& > string, const Font& font) { > All the callsites for this function should construct FontLists instead > of constructing a Font and then making a FontList out of that. Also, > each callsite can potentially use FontList::GetStringWidth or > gfx::GetStringWidth directly. > > https://codereview.chromium.**org/24883002/diff/3001/ui/gfx/** > text_elider_unittest.cc#**newcode46<https://codereview.chromium.org/24883002/diff/3001/ui/gfx/text_elider_unittest.cc#newcode46> > ui/gfx/text_elider_unittest.**cc:46: gfx::FontList font_list(font); > nit: inline this, or make it const. > > https://codereview.chromium.**org/24883002/diff/3001/ui/gfx/** > text_elider_unittest.cc#**newcode416<https://codereview.chromium.org/24883002/diff/3001/ui/gfx/text_elider_unittest.cc#newcode416> > ui/gfx/text_elider_unittest.**cc:416: for (float width = 0; width <= > kTestStringWidth; width += 1) { > nit: ++width or leave this as it was. Why "+=1"? > > https://codereview.chromium.**org/24883002/diff/3001/ui/** > views/controls/label.cc<https://codereview.chromium.org/24883002/diff/3001/ui/views/controls/label.cc> > File ui/views/controls/label.cc (right): > > https://codereview.chromium.**org/24883002/diff/3001/ui/** > views/controls/label.cc#**newcode194<https://codereview.chromium.org/24883002/diff/3001/ui/views/controls/label.cc#newcode194> > ui/views/controls/label.cc:**194: > static_cast<int>(gfx::**GetStringWidth(*iter, font_list_))); > Shouldn't this retain the floating point value? Either way, use > std::max<int|float> instead of static casting an arg, if possible. > > https://codereview.chromium.**org/24883002/diff/3001/ui/** > views/controls/label.cc#**newcode254<https://codereview.chromium.org/24883002/diff/3001/ui/views/controls/label.cc#newcode254> > ui/views/controls/label.cc:**254: float fw = w; > |fw| isn't really used after its value is updated below, because you > changed line 258 to use |w| instead of |fw|/|cache_width|. Can you clean > this code up? > > https://codereview.chromium.**org/24883002/diff/3001/ui/** > views/widget/tooltip_manager.**cc<https://codereview.chromium.org/24883002/diff/3001/ui/views/widget/tooltip_manager.cc> > File ui/views/widget/tooltip_**manager.cc (right): > > https://codereview.chromium.**org/24883002/diff/3001/ui/** > views/widget/tooltip_manager.**cc#newcode57<https://codereview.chromium.org/24883002/diff/3001/ui/views/widget/tooltip_manager.cc#newcode57> > ui/views/widget/tooltip_**manager.cc:57: > static_cast<int>(gfx::**GetStringWidth(elided_text, font_list))); > Use std::max<int> instead (or float if that's more appropriate). > > https://codereview.chromium.**org/24883002/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/24883002/diff/3001/ash/system/user/tray_user.cc File ash/system/user/tray_user.cc (right): https://codereview.chromium.org/24883002/diff/3001/ash/system/user/tray_user.... ash/system/user/tray_user.cc:460: static_cast<float>(contents_area.width()), On 2013/09/27 21:54:48, msw wrote: > Won't int->float implicit type conversion work here, and in similar cases? Reverted. Changed INT_MAX to FLT_MAX in the following 2 places that call ElideRectangleText to avoid the compiler from complaining. https://codereview.chromium.org/24883002/diff/3001/ui/gfx/canvas.h File ui/gfx/canvas.h (right): https://codereview.chromium.org/24883002/diff/3001/ui/gfx/canvas.h#newcode128 ui/gfx/canvas.h:128: static void SizeStringInt(const base::string16& text, On 2013/09/27 21:54:48, msw wrote: > I presume the "Int" suffix in the name of this function was meant to correlate > with the integral size it previously returned. Can you update this now, since > you're touching all of its references anyway? Renamed to SizeStringToFit. https://codereview.chromium.org/24883002/diff/3001/ui/gfx/canvas_skia.cc File ui/gfx/canvas_skia.cc (right): https://codereview.chromium.org/24883002/diff/3001/ui/gfx/canvas_skia.cc#newc... ui/gfx/canvas_skia.cc:172: float* width, float* height, On 2013/09/27 21:54:48, msw wrote: > nit: one arg per line here, perhaps the function should return a SizeF? Done. We can't return SizeF since width and height parameters serve as both in and out. https://codereview.chromium.org/24883002/diff/3001/ui/gfx/canvas_unittest_mac.mm File ui/gfx/canvas_unittest_mac.mm (right): https://codereview.chromium.org/24883002/diff/3001/ui/gfx/canvas_unittest_mac... ui/gfx/canvas_unittest_mac.mm:21: void CanvasMac_SizeStringInt(const base::string16& text, On 2013/09/27 21:54:48, msw wrote: > Is updating this important? Is it even worth keeping at all? If anything, maybe > it's worthwhile to keep a direct comparison between Canvas::SizeStringInt and > sizeWithAttributes, inlining some of this function within a single test. I do not know if it is worth to keep this test. I will let the original author decide. Moved the code into ComapreSizes, as suggested. https://codereview.chromium.org/24883002/diff/3001/ui/gfx/canvas_unittest_mac... ui/gfx/canvas_unittest_mac.mm:66: // mac_height is the truncated height. On 2013/09/27 21:54:48, msw wrote: > nit: Consider "CanvasMac_SizeStringInt returns a truncated height." Done. https://codereview.chromium.org/24883002/diff/3001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/24883002/diff/3001/ui/gfx/render_text.cc#newc... ui/gfx/render_text.cc:648: Size size = GetStringSize(); On 2013/09/27 21:54:48, msw wrote: > nit: const Done. https://codereview.chromium.org/24883002/diff/3001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/24883002/diff/3001/ui/gfx/render_text.h#newco... ui/gfx/render_text.h:331: // The default implementation is same as GetStringSize. Certain platform, like On 2013/09/27 21:54:48, msw wrote: > nit: "Certain platforms that compute the text size as floating-point values, > like Mac, will override this method." Done. https://codereview.chromium.org/24883002/diff/3001/ui/gfx/text_elider_unittes... File ui/gfx/text_elider_unittest.cc (right): https://codereview.chromium.org/24883002/diff/3001/ui/gfx/text_elider_unittes... ui/gfx/text_elider_unittest.cc:45: float GetStringWidth(const string16& string, const Font& font) { On 2013/09/27 21:54:48, msw wrote: > All the callsites for this function should construct FontLists instead of > constructing a Font and then making a FontList out of that. Also, each callsite > can potentially use FontList::GetStringWidth or gfx::GetStringWidth directly. Done. https://codereview.chromium.org/24883002/diff/3001/ui/gfx/text_elider_unittes... ui/gfx/text_elider_unittest.cc:46: gfx::FontList font_list(font); On 2013/09/27 21:54:48, msw wrote: > nit: inline this, or make it const. Not needed. Removed. https://codereview.chromium.org/24883002/diff/3001/ui/gfx/text_elider_unittes... ui/gfx/text_elider_unittest.cc:416: for (float width = 0; width <= kTestStringWidth; width += 1) { On 2013/09/27 21:54:48, msw wrote: > nit: ++width or leave this as it was. Why "+=1"? I though this did not work for float. It turned out I was wrong. Corrected. https://codereview.chromium.org/24883002/diff/3001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/24883002/diff/3001/ui/views/controls/label.cc... ui/views/controls/label.cc:194: static_cast<int>(gfx::GetStringWidth(*iter, font_list_))); On 2013/09/27 21:54:48, msw wrote: > Shouldn't this retain the floating point value? Either way, use > std::max<int|float> instead of static casting an arg, if possible. I do not think fractional width will be needed for those platforms that use views. Changed to use std::max<int>. https://codereview.chromium.org/24883002/diff/3001/ui/views/controls/label.cc... ui/views/controls/label.cc:254: float fw = w; On 2013/09/27 21:54:48, msw wrote: > |fw| isn't really used after its value is updated below, because you changed > line 258 to use |w| instead of |fw|/|cache_width|. Can you clean this code up? Though |fw| is not used after its value is updated, we still need it since Canvas::SizeStringToFit needs a float parameter to pass. I changed to use w instead of cache_width since we do not need to introduce another variable to cache the old value. https://codereview.chromium.org/24883002/diff/3001/ui/views/widget/tooltip_ma... File ui/views/widget/tooltip_manager.cc (right): https://codereview.chromium.org/24883002/diff/3001/ui/views/widget/tooltip_ma... ui/views/widget/tooltip_manager.cc:57: static_cast<int>(gfx::GetStringWidth(elided_text, font_list))); On 2013/09/27 21:54:48, msw wrote: > Use std::max<int> instead (or float if that's more appropriate). Done.
Everything looks okay except the unnecessary addition of ToCGRect. https://codereview.chromium.org/24883002/diff/3001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/24883002/diff/3001/ui/views/controls/label.cc... ui/views/controls/label.cc:254: float fw = w; On 2013/10/01 00:32:58, jianli wrote: > On 2013/09/27 21:54:48, msw wrote: > > |fw| isn't really used after its value is updated below, because you changed > > line 258 to use |w| instead of |fw|/|cache_width|. Can you clean this code up? > > Though |fw| is not used after its value is updated, we still need it since > Canvas::SizeStringToFit needs a float parameter to pass. I changed to use w > instead of cache_width since we do not need to introduce another variable to > cache the old value. Ah, now I understand; this code was already caching the argument width instead of the width value that was potentially increased by SizeStringToFit... this change is okay since you're retaining the original behavior. https://codereview.chromium.org/24883002/diff/83001/ui/gfx/canvas.h File ui/gfx/canvas.h (right): https://codereview.chromium.org/24883002/diff/83001/ui/gfx/canvas.h#newcode145 ui/gfx/canvas.h:145: const FontList& font_list); nit: indent this line to match the one above. https://codereview.chromium.org/24883002/diff/83001/ui/gfx/canvas_unittest_ma... File ui/gfx/canvas_unittest_mac.mm (right): https://codereview.chromium.org/24883002/diff/83001/ui/gfx/canvas_unittest_ma... ui/gfx/canvas_unittest_mac.mm:35: NSSize string_size = [ns_string sizeWithAttributes:attributes]; nit: inline this. https://codereview.chromium.org/24883002/diff/83001/ui/gfx/font.h File ui/gfx/font.h (right): https://codereview.chromium.org/24883002/diff/83001/ui/gfx/font.h#newcode75 ui/gfx/font.h:75: // Returns the number of horizontal pixels needed to display the specified nit: s/number of pixels/pixel width/; ditto below. https://codereview.chromium.org/24883002/diff/83001/ui/gfx/font_list.h File ui/gfx/font_list.h (right): https://codereview.chromium.org/24883002/diff/83001/ui/gfx/font_list.h#newcode84 ui/gfx/font_list.h:84: // Returns the number of horizontal pixels needed to display |text|. nit: s/number of pixels/pixel width/; ditto below. https://codereview.chromium.org/24883002/diff/83001/ui/gfx/platform_font.h File ui/gfx/platform_font.h (right): https://codereview.chromium.org/24883002/diff/83001/ui/gfx/platform_font.h#ne... ui/gfx/platform_font.h:48: // Returns the number of horizontal pixels needed to display the specified nit: s/number of pixels/pixel width/; ditto below. https://codereview.chromium.org/24883002/diff/83001/ui/gfx/rect_f.h File ui/gfx/rect_f.h (right): https://codereview.chromium.org/24883002/diff/83001/ui/gfx/rect_f.h#newcode53 ui/gfx/rect_f.h:53: CGRect ToCGRect() const; I don't see this used anywhere in your patch; remove it. https://codereview.chromium.org/24883002/diff/83001/ui/gfx/text_elider.cc File ui/gfx/text_elider.cc (right): https://codereview.chromium.org/24883002/diff/83001/ui/gfx/text_elider.cc#new... ui/gfx/text_elider.cc:310: const float pixel_width_url_domain = gfx::GetStringWidth(url_domain, nit: split the line after the = symbol here and in 4 places below. https://codereview.chromium.org/24883002/diff/83001/ui/views/widget/tooltip_m... File ui/views/widget/tooltip_manager.cc (right): https://codereview.chromium.org/24883002/diff/83001/ui/views/widget/tooltip_m... ui/views/widget/tooltip_manager.cc:21: nit: remove this blank line
Also, you still need to add a unit test that returns a non-integral width, right?
My bad. I will add the test. On Mon, Sep 30, 2013 at 7:43 PM, <msw@chromium.org> wrote: > Also, you still need to add a unit test that returns a non-integral width, > right? > > https://codereview.chromium.**org/24883002/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
New tests added in canvas_unitttest_mac.mm and font_unittest.cc https://codereview.chromium.org/24883002/diff/83001/ui/gfx/canvas.h File ui/gfx/canvas.h (right): https://codereview.chromium.org/24883002/diff/83001/ui/gfx/canvas.h#newcode145 ui/gfx/canvas.h:145: const FontList& font_list); On 2013/10/01 02:42:48, msw wrote: > nit: indent this line to match the one above. Done. https://codereview.chromium.org/24883002/diff/83001/ui/gfx/canvas_unittest_ma... File ui/gfx/canvas_unittest_mac.mm (right): https://codereview.chromium.org/24883002/diff/83001/ui/gfx/canvas_unittest_ma... ui/gfx/canvas_unittest_mac.mm:35: NSSize string_size = [ns_string sizeWithAttributes:attributes]; On 2013/10/01 02:42:48, msw wrote: > nit: inline this. Done. https://codereview.chromium.org/24883002/diff/83001/ui/gfx/font.h File ui/gfx/font.h (right): https://codereview.chromium.org/24883002/diff/83001/ui/gfx/font.h#newcode75 ui/gfx/font.h:75: // Returns the number of horizontal pixels needed to display the specified On 2013/10/01 02:42:48, msw wrote: > nit: s/number of pixels/pixel width/; ditto below. Done. https://codereview.chromium.org/24883002/diff/83001/ui/gfx/font_list.h File ui/gfx/font_list.h (right): https://codereview.chromium.org/24883002/diff/83001/ui/gfx/font_list.h#newcode84 ui/gfx/font_list.h:84: // Returns the number of horizontal pixels needed to display |text|. On 2013/10/01 02:42:48, msw wrote: > nit: s/number of pixels/pixel width/; ditto below. Done. https://codereview.chromium.org/24883002/diff/83001/ui/gfx/platform_font.h File ui/gfx/platform_font.h (right): https://codereview.chromium.org/24883002/diff/83001/ui/gfx/platform_font.h#ne... ui/gfx/platform_font.h:48: // Returns the number of horizontal pixels needed to display the specified On 2013/10/01 02:42:48, msw wrote: > nit: s/number of pixels/pixel width/; ditto below. Done. https://codereview.chromium.org/24883002/diff/83001/ui/gfx/rect_f.h File ui/gfx/rect_f.h (right): https://codereview.chromium.org/24883002/diff/83001/ui/gfx/rect_f.h#newcode53 ui/gfx/rect_f.h:53: CGRect ToCGRect() const; On 2013/10/01 02:42:48, msw wrote: > I don't see this used anywhere in your patch; remove it. Indeed it is needed in autofill_popup_view_cocoa.mm and autofill_popup_view_bridge.mm (not touched): NSRect frame = NSRectFromCGRect(controller_->popup_bounds().ToCGRect()); NSRect rowBounds = NSRectFromCGRect(controller_->GetRowBounds(i).ToCGRect()); Now we return RectF for popup_bounds and GetRowBounds, so we need to port ToCGRect support to RectF. https://codereview.chromium.org/24883002/diff/83001/ui/gfx/text_elider.cc File ui/gfx/text_elider.cc (right): https://codereview.chromium.org/24883002/diff/83001/ui/gfx/text_elider.cc#new... ui/gfx/text_elider.cc:310: const float pixel_width_url_domain = gfx::GetStringWidth(url_domain, On 2013/10/01 02:42:48, msw wrote: > nit: split the line after the = symbol here and in 4 places below. Done. https://codereview.chromium.org/24883002/diff/83001/ui/views/widget/tooltip_m... File ui/views/widget/tooltip_manager.cc (right): https://codereview.chromium.org/24883002/diff/83001/ui/views/widget/tooltip_m... ui/views/widget/tooltip_manager.cc:21: On 2013/10/01 02:42:48, msw wrote: > nit: remove this blank line Done.
Looks good overall, a couple comments about your changes to the mac test. https://codereview.chromium.org/24883002/diff/122001/ui/gfx/canvas_unittest_m... File ui/gfx/canvas_unittest_mac.mm (left): https://codereview.chromium.org/24883002/diff/122001/ui/gfx/canvas_unittest_m... ui/gfx/canvas_unittest_mac.mm:23: void CanvasMac_SizeStringInt(const base::string16& text, I'd rather you keep this as a standalone function. The goal of this test is to compare that this implementation is consistent with the canvas_skia.cc one. https://codereview.chromium.org/24883002/diff/122001/ui/gfx/canvas_unittest_m... File ui/gfx/canvas_unittest_mac.mm (right): https://codereview.chromium.org/24883002/diff/122001/ui/gfx/canvas_unittest_m... ui/gfx/canvas_unittest_mac.mm:75: Nit: Remove empty line.
LGTM with nits, and Q for Alexei's comment to not inline CanvasMac_SizeStringInt for its one test usage. https://codereview.chromium.org/24883002/diff/122001/ui/gfx/canvas_unittest_m... File ui/gfx/canvas_unittest_mac.mm (left): https://codereview.chromium.org/24883002/diff/122001/ui/gfx/canvas_unittest_m... ui/gfx/canvas_unittest_mac.mm:23: void CanvasMac_SizeStringInt(const base::string16& text, On 2013/10/01 19:04:55, Alexei Svitkine wrote: > I'd rather you keep this as a standalone function. > > The goal of this test is to compare that this implementation is consistent with > the canvas_skia.cc one. Why do we care? What's so great about this implementation? It's not in actual production use elsewhere, so it's functionality has no real value. If we wish to check that we match sizeWithAttributes, that's a bit more worthwhile, but why would we need a standalone function for that? (that's all I have to say, I'll concede to whatever Alexei recommends at this point) https://codereview.chromium.org/24883002/diff/122001/ui/gfx/canvas_unittest_m... File ui/gfx/canvas_unittest_mac.mm (right): https://codereview.chromium.org/24883002/diff/122001/ui/gfx/canvas_unittest_m... ui/gfx/canvas_unittest_mac.mm:70: Font font; nit: Just construct a FontList here instead of constructing a FontList from a Font in the call below. https://codereview.chromium.org/24883002/diff/122001/ui/gfx/font.h File ui/gfx/font.h (right): https://codereview.chromium.org/24883002/diff/122001/ui/gfx/font.h#newcode72 ui/gfx/font.h:72: // Returns the pixel width for the font. nit: revert this comment change or update it to "// Returns the pixel width of an average character for the font."
https://codereview.chromium.org/24883002/diff/122001/ui/gfx/canvas_unittest_m... File ui/gfx/canvas_unittest_mac.mm (left): https://codereview.chromium.org/24883002/diff/122001/ui/gfx/canvas_unittest_m... ui/gfx/canvas_unittest_mac.mm:23: void CanvasMac_SizeStringInt(const base::string16& text, On 2013/10/01 19:14:53, msw wrote: > On 2013/10/01 19:04:55, Alexei Svitkine wrote: > > I'd rather you keep this as a standalone function. > > > > The goal of this test is to compare that this implementation is consistent > with > > the canvas_skia.cc one. > > Why do we care? What's so great about this implementation? It's not in actual > production use elsewhere, so it's functionality has no real value. If we wish to > check that we match sizeWithAttributes, that's a bit more worthwhile, but why > would we need a standalone function for that? (that's all I have to say, I'll > concede to whatever Alexei recommends at this point) I'm okay with changing the signature / simplifying this function - I agree that ultimately we want to check vs -sizeWithAttributes. However, CompareSizes() below with all of that inlines is quite messy - so I'd rather have a separate function that does the Mac code and returns the result of -sizeWithAttributes.
https://codereview.chromium.org/24883002/diff/122001/ui/gfx/canvas_unittest_m... File ui/gfx/canvas_unittest_mac.mm (left): https://codereview.chromium.org/24883002/diff/122001/ui/gfx/canvas_unittest_m... ui/gfx/canvas_unittest_mac.mm:23: void CanvasMac_SizeStringInt(const base::string16& text, On 2013/10/01 19:19:50, Alexei Svitkine wrote: > On 2013/10/01 19:14:53, msw wrote: > > On 2013/10/01 19:04:55, Alexei Svitkine wrote: > > > I'd rather you keep this as a standalone function. > > > > > > The goal of this test is to compare that this implementation is consistent > > with > > > the canvas_skia.cc one. > > > > Why do we care? What's so great about this implementation? It's not in actual > > production use elsewhere, so it's functionality has no real value. If we wish > to > > check that we match sizeWithAttributes, that's a bit more worthwhile, but why > > would we need a standalone function for that? (that's all I have to say, I'll > > concede to whatever Alexei recommends at this point) > > I'm okay with changing the signature / simplifying this function - I agree that > ultimately we want to check vs -sizeWithAttributes. However, CompareSizes() > below with all of that inlines is quite messy - so I'd rather have a separate > function that does the Mac code and returns the result of -sizeWithAttributes. Added back the helper function with simplification change. https://codereview.chromium.org/24883002/diff/122001/ui/gfx/canvas_unittest_m... File ui/gfx/canvas_unittest_mac.mm (right): https://codereview.chromium.org/24883002/diff/122001/ui/gfx/canvas_unittest_m... ui/gfx/canvas_unittest_mac.mm:70: Font font; On 2013/10/01 19:14:53, msw wrote: > nit: Just construct a FontList here instead of constructing a FontList from a > Font in the call below. Done. https://codereview.chromium.org/24883002/diff/122001/ui/gfx/canvas_unittest_m... ui/gfx/canvas_unittest_mac.mm:75: On 2013/10/01 19:04:55, Alexei Svitkine wrote: > Nit: Remove empty line. Done. https://codereview.chromium.org/24883002/diff/122001/ui/gfx/font.h File ui/gfx/font.h (right): https://codereview.chromium.org/24883002/diff/122001/ui/gfx/font.h#newcode72 ui/gfx/font.h:72: // Returns the pixel width for the font. On 2013/10/01 19:14:53, msw wrote: > nit: revert this comment change or update it to "// Returns the pixel width of > an average character for the font." Done.
Still LGTM, with one new nit. https://codereview.chromium.org/24883002/diff/162001/ui/gfx/canvas_unittest_m... File ui/gfx/canvas_unittest_mac.mm (right): https://codereview.chromium.org/24883002/diff/162001/ui/gfx/canvas_unittest_m... ui/gfx/canvas_unittest_mac.mm:23: nit: remove blank line
https://codereview.chromium.org/24883002/diff/162001/ash/system/user/tray_use... File ash/system/user/tray_user.cc (right): https://codereview.chromium.org/24883002/diff/162001/ash/system/user/tray_use... ash/system/user/tray_user.cc:546: font.GetStringWidth(text_) + space_width + link_size.width(), Won't this round down? It seems all to easy to accidentally round down this this change. Is there a way to fix that?
https://codereview.chromium.org/24883002/diff/162001/ash/system/user/tray_use... File ash/system/user/tray_user.cc (right): https://codereview.chromium.org/24883002/diff/162001/ash/system/user/tray_use... ash/system/user/tray_user.cc:546: font.GetStringWidth(text_) + space_width + link_size.width(), On 2013/10/01 23:53:03, sky wrote: > Won't this round down? It seems all to easy to accidentally round down this this > change. Is there a way to fix that? Does ash use the fractional width? If not, we should not have round down problem. Could you please illustrate what kind of problem we could have here?
My worry is a general worry that any time you expose an API that takes float and you start rounding down you are going to get rounding errors. If we're to have a float API then we should do the right thing everywhere, regardless if they are currently using floats. -Scott On Tue, Oct 1, 2013 at 5:49 PM, <jianli@chromium.org> wrote: > > https://codereview.chromium.org/24883002/diff/162001/ash/system/user/tray_use... > File ash/system/user/tray_user.cc (right): > > https://codereview.chromium.org/24883002/diff/162001/ash/system/user/tray_use... > ash/system/user/tray_user.cc:546: font.GetStringWidth(text_) + > space_width + link_size.width(), > On 2013/10/01 23:53:03, sky wrote: >> >> Won't this round down? It seems all to easy to accidentally round down > > this this >> >> change. Is there a way to fix that? > > > Does ash use the fractional width? If not, we should not have round down > problem. Could you please illustrate what kind of problem we could have > here? > > https://codereview.chromium.org/24883002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I had originally suggested to keep both the int and add the float APIs and for things where it matters (i.e. code that runs on the Mac such as text elider), to use the float API, while leaving most of views alone (i.e. still using int API). i.e. GetStringSize and GetStringSizeF. On Wed, Oct 2, 2013 at 11:58 AM, Scott Violet <sky@chromium.org> wrote: > My worry is a general worry that any time you expose an API that takes > float and you start rounding down you are going to get rounding > errors. If we're to have a float API then we should do the right thing > everywhere, regardless if they are currently using floats. > > -Scott > > On Tue, Oct 1, 2013 at 5:49 PM, <jianli@chromium.org> wrote: > > > > > https://codereview.chromium.org/24883002/diff/162001/ash/system/user/tray_use... > > File ash/system/user/tray_user.cc (right): > > > > > https://codereview.chromium.org/24883002/diff/162001/ash/system/user/tray_use... > > ash/system/user/tray_user.cc:546: font.GetStringWidth(text_) + > > space_width + link_size.width(), > > On 2013/10/01 23:53:03, sky wrote: > >> > >> Won't this round down? It seems all to easy to accidentally round down > > > > this this > >> > >> change. Is there a way to fix that? > > > > > > Does ash use the fractional width? If not, we should not have round down > > problem. Could you please illustrate what kind of problem we could have > > here? > > > > https://codereview.chromium.org/24883002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Doing this seems equally error prone in so far as folks have to be very careful to know what platform they are. In some places this is obvious, but not every where. On Wed, Oct 2, 2013 at 9:00 AM, Alexei Svitkine <asvitkine@chromium.org> wrote: > I had originally suggested to keep both the int and add the float APIs and > for things where it matters (i.e. code that runs on the Mac such as text > elider), to use the float API, while leaving most of views alone (i.e. still > using int API). i.e. GetStringSize and GetStringSizeF. > > > > On Wed, Oct 2, 2013 at 11:58 AM, Scott Violet <sky@chromium.org> wrote: >> >> My worry is a general worry that any time you expose an API that takes >> float and you start rounding down you are going to get rounding >> errors. If we're to have a float API then we should do the right thing >> everywhere, regardless if they are currently using floats. >> >> -Scott >> >> On Tue, Oct 1, 2013 at 5:49 PM, <jianli@chromium.org> wrote: >> > >> > >> > https://codereview.chromium.org/24883002/diff/162001/ash/system/user/tray_use... >> > File ash/system/user/tray_user.cc (right): >> > >> > >> > https://codereview.chromium.org/24883002/diff/162001/ash/system/user/tray_use... >> > ash/system/user/tray_user.cc:546: font.GetStringWidth(text_) + >> > space_width + link_size.width(), >> > On 2013/10/01 23:53:03, sky wrote: >> >> >> >> Won't this round down? It seems all to easy to accidentally round down >> > >> > this this >> >> >> >> change. Is there a way to fix that? >> > >> > >> > Does ash use the fractional width? If not, we should not have round down >> > problem. Could you please illustrate what kind of problem we could have >> > here? >> > >> > https://codereview.chromium.org/24883002/ > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
True. Which is why I didn't push back on this CL not doing the selective change like I originally suggested. Scott, to give some context here - this is indeed needed on Mac to make text_elider.cc and other utils work correctly - since string widths are fractional - and thus code that adds them together and such needs to operate on the float values rather than rounded up values to get correct results. That's the motivation for this change. On Wed, Oct 2, 2013 at 12:06 PM, Scott Violet <sky@chromium.org> wrote: > Doing this seems equally error prone in so far as folks have to be > very careful to know what platform they are. In some places this is > obvious, but not every where. > > On Wed, Oct 2, 2013 at 9:00 AM, Alexei Svitkine <asvitkine@chromium.org> > wrote: > > I had originally suggested to keep both the int and add the float APIs > and > > for things where it matters (i.e. code that runs on the Mac such as text > > elider), to use the float API, while leaving most of views alone (i.e. > still > > using int API). i.e. GetStringSize and GetStringSizeF. > > > > > > > > On Wed, Oct 2, 2013 at 11:58 AM, Scott Violet <sky@chromium.org> wrote: > >> > >> My worry is a general worry that any time you expose an API that takes > >> float and you start rounding down you are going to get rounding > >> errors. If we're to have a float API then we should do the right thing > >> everywhere, regardless if they are currently using floats. > >> > >> -Scott > >> > >> On Tue, Oct 1, 2013 at 5:49 PM, <jianli@chromium.org> wrote: > >> > > >> > > >> > > https://codereview.chromium.org/24883002/diff/162001/ash/system/user/tray_use... > >> > File ash/system/user/tray_user.cc (right): > >> > > >> > > >> > > https://codereview.chromium.org/24883002/diff/162001/ash/system/user/tray_use... > >> > ash/system/user/tray_user.cc:546: font.GetStringWidth(text_) + > >> > space_width + link_size.width(), > >> > On 2013/10/01 23:53:03, sky wrote: > >> >> > >> >> Won't this round down? It seems all to easy to accidentally round > down > >> > > >> > this this > >> >> > >> >> change. Is there a way to fix that? > >> > > >> > > >> > Does ash use the fractional width? If not, we should not have round > down > >> > problem. Could you please illustrate what kind of problem we could > have > >> > here? > >> > > >> > https://codereview.chromium.org/24883002/ > > > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I get why we want fractional metrics. I just want to make sure we round the right way. -Scott On Wed, Oct 2, 2013 at 9:35 AM, Alexei Svitkine <asvitkine@chromium.org> wrote: > True. Which is why I didn't push back on this CL not doing the selective > change like I originally suggested. > > Scott, to give some context here - this is indeed needed on Mac to make > text_elider.cc and other utils work correctly - since string widths are > fractional - and thus code that adds them together and such needs to operate > on the float values rather than rounded up values to get correct results. > That's the motivation for this change. > > > > On Wed, Oct 2, 2013 at 12:06 PM, Scott Violet <sky@chromium.org> wrote: >> >> Doing this seems equally error prone in so far as folks have to be >> very careful to know what platform they are. In some places this is >> obvious, but not every where. >> >> On Wed, Oct 2, 2013 at 9:00 AM, Alexei Svitkine <asvitkine@chromium.org> >> wrote: >> > I had originally suggested to keep both the int and add the float APIs >> > and >> > for things where it matters (i.e. code that runs on the Mac such as text >> > elider), to use the float API, while leaving most of views alone (i.e. >> > still >> > using int API). i.e. GetStringSize and GetStringSizeF. >> > >> > >> > >> > On Wed, Oct 2, 2013 at 11:58 AM, Scott Violet <sky@chromium.org> wrote: >> >> >> >> My worry is a general worry that any time you expose an API that takes >> >> float and you start rounding down you are going to get rounding >> >> errors. If we're to have a float API then we should do the right thing >> >> everywhere, regardless if they are currently using floats. >> >> >> >> -Scott >> >> >> >> On Tue, Oct 1, 2013 at 5:49 PM, <jianli@chromium.org> wrote: >> >> > >> >> > >> >> > >> >> > https://codereview.chromium.org/24883002/diff/162001/ash/system/user/tray_use... >> >> > File ash/system/user/tray_user.cc (right): >> >> > >> >> > >> >> > >> >> > https://codereview.chromium.org/24883002/diff/162001/ash/system/user/tray_use... >> >> > ash/system/user/tray_user.cc:546: font.GetStringWidth(text_) + >> >> > space_width + link_size.width(), >> >> > On 2013/10/01 23:53:03, sky wrote: >> >> >> >> >> >> Won't this round down? It seems all to easy to accidentally round >> >> >> down >> >> > >> >> > this this >> >> >> >> >> >> change. Is there a way to fix that? >> >> > >> >> > >> >> > Does ash use the fractional width? If not, we should not have round >> >> > down >> >> > problem. Could you please illustrate what kind of problem we could >> >> > have >> >> > here? >> >> > >> >> > https://codereview.chromium.org/24883002/ >> > >> > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
All these potential round down changes are in views/ash codebase. We
suppose to return and use the integral width here. So no round-down should
be expected. How about adding some kind of DCHECK to help catching this
problem, like:
float text_width = font.GetStringWidth(text_);
DCHECK_EQ(text_width - static_cast<int>(text_width), 0);
int max_width = std::min<int>(
text_width + space_width + link_size.width(),
bubble_view->GetMaximumSize().width() - (used_width +
insets.width()));
On Wed, Oct 2, 2013 at 1:16 PM, Scott Violet <sky@chromium.org> wrote:
> I get why we want fractional metrics. I just want to make sure we
> round the right way.
>
> -Scott
>
> On Wed, Oct 2, 2013 at 9:35 AM, Alexei Svitkine <asvitkine@chromium.org>
> wrote:
> > True. Which is why I didn't push back on this CL not doing the selective
> > change like I originally suggested.
> >
> > Scott, to give some context here - this is indeed needed on Mac to make
> > text_elider.cc and other utils work correctly - since string widths are
> > fractional - and thus code that adds them together and such needs to
> operate
> > on the float values rather than rounded up values to get correct results.
> > That's the motivation for this change.
> >
> >
> >
> > On Wed, Oct 2, 2013 at 12:06 PM, Scott Violet <sky@chromium.org> wrote:
> >>
> >> Doing this seems equally error prone in so far as folks have to be
> >> very careful to know what platform they are. In some places this is
> >> obvious, but not every where.
> >>
> >> On Wed, Oct 2, 2013 at 9:00 AM, Alexei Svitkine <asvitkine@chromium.org
> >
> >> wrote:
> >> > I had originally suggested to keep both the int and add the float APIs
> >> > and
> >> > for things where it matters (i.e. code that runs on the Mac such as
> text
> >> > elider), to use the float API, while leaving most of views alone (i.e.
> >> > still
> >> > using int API). i.e. GetStringSize and GetStringSizeF.
> >> >
> >> >
> >> >
> >> > On Wed, Oct 2, 2013 at 11:58 AM, Scott Violet <sky@chromium.org>
> wrote:
> >> >>
> >> >> My worry is a general worry that any time you expose an API that
> takes
> >> >> float and you start rounding down you are going to get rounding
> >> >> errors. If we're to have a float API then we should do the right
> thing
> >> >> everywhere, regardless if they are currently using floats.
> >> >>
> >> >> -Scott
> >> >>
> >> >> On Tue, Oct 1, 2013 at 5:49 PM, <jianli@chromium.org> wrote:
> >> >> >
> >> >> >
> >> >> >
> >> >> >
>
https://codereview.chromium.org/24883002/diff/162001/ash/system/user/tray_use...
> >> >> > File ash/system/user/tray_user.cc (right):
> >> >> >
> >> >> >
> >> >> >
> >> >> >
>
https://codereview.chromium.org/24883002/diff/162001/ash/system/user/tray_use...
> >> >> > ash/system/user/tray_user.cc:546: font.GetStringWidth(text_) +
> >> >> > space_width + link_size.width(),
> >> >> > On 2013/10/01 23:53:03, sky wrote:
> >> >> >>
> >> >> >> Won't this round down? It seems all to easy to accidentally round
> >> >> >> down
> >> >> >
> >> >> > this this
> >> >> >>
> >> >> >> change. Is there a way to fix that?
> >> >> >
> >> >> >
> >> >> > Does ash use the fractional width? If not, we should not have round
> >> >> > down
> >> >> > problem. Could you please illustrate what kind of problem we could
> >> >> > have
> >> >> > here?
> >> >> >
> >> >> > https://codereview.chromium.org/24883002/
> >> >
> >> >
> >
> >
>
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
I don't want a DCHECK like that every where. At some point aura may want fraction metrics. If you're going to add the support for this, make it do the right thing everywhere. -Scott On Wed, Oct 2, 2013 at 1:24 PM, Jian Li <jianli@chromium.org> wrote: > All these potential round down changes are in views/ash codebase. We suppose > to return and use the integral width here. So no round-down should be > expected. How about adding some kind of DCHECK to help catching this > problem, like: > float text_width = font.GetStringWidth(text_); > DCHECK_EQ(text_width - static_cast<int>(text_width), 0); > int max_width = std::min<int>( > text_width + space_width + link_size.width(), > bubble_view->GetMaximumSize().width() - (used_width + > insets.width())); > > > On Wed, Oct 2, 2013 at 1:16 PM, Scott Violet <sky@chromium.org> wrote: >> >> I get why we want fractional metrics. I just want to make sure we >> round the right way. >> >> -Scott >> >> On Wed, Oct 2, 2013 at 9:35 AM, Alexei Svitkine <asvitkine@chromium.org> >> wrote: >> > True. Which is why I didn't push back on this CL not doing the selective >> > change like I originally suggested. >> > >> > Scott, to give some context here - this is indeed needed on Mac to make >> > text_elider.cc and other utils work correctly - since string widths are >> > fractional - and thus code that adds them together and such needs to >> > operate >> > on the float values rather than rounded up values to get correct >> > results. >> > That's the motivation for this change. >> > >> > >> > >> > On Wed, Oct 2, 2013 at 12:06 PM, Scott Violet <sky@chromium.org> wrote: >> >> >> >> Doing this seems equally error prone in so far as folks have to be >> >> very careful to know what platform they are. In some places this is >> >> obvious, but not every where. >> >> >> >> On Wed, Oct 2, 2013 at 9:00 AM, Alexei Svitkine >> >> <asvitkine@chromium.org> >> >> wrote: >> >> > I had originally suggested to keep both the int and add the float >> >> > APIs >> >> > and >> >> > for things where it matters (i.e. code that runs on the Mac such as >> >> > text >> >> > elider), to use the float API, while leaving most of views alone >> >> > (i.e. >> >> > still >> >> > using int API). i.e. GetStringSize and GetStringSizeF. >> >> > >> >> > >> >> > >> >> > On Wed, Oct 2, 2013 at 11:58 AM, Scott Violet <sky@chromium.org> >> >> > wrote: >> >> >> >> >> >> My worry is a general worry that any time you expose an API that >> >> >> takes >> >> >> float and you start rounding down you are going to get rounding >> >> >> errors. If we're to have a float API then we should do the right >> >> >> thing >> >> >> everywhere, regardless if they are currently using floats. >> >> >> >> >> >> -Scott >> >> >> >> >> >> On Tue, Oct 1, 2013 at 5:49 PM, <jianli@chromium.org> wrote: >> >> >> > >> >> >> > >> >> >> > >> >> >> > >> >> >> > https://codereview.chromium.org/24883002/diff/162001/ash/system/user/tray_use... >> >> >> > File ash/system/user/tray_user.cc (right): >> >> >> > >> >> >> > >> >> >> > >> >> >> > >> >> >> > https://codereview.chromium.org/24883002/diff/162001/ash/system/user/tray_use... >> >> >> > ash/system/user/tray_user.cc:546: font.GetStringWidth(text_) + >> >> >> > space_width + link_size.width(), >> >> >> > On 2013/10/01 23:53:03, sky wrote: >> >> >> >> >> >> >> >> Won't this round down? It seems all to easy to accidentally round >> >> >> >> down >> >> >> > >> >> >> > this this >> >> >> >> >> >> >> >> change. Is there a way to fix that? >> >> >> > >> >> >> > >> >> >> > Does ash use the fractional width? If not, we should not have >> >> >> > round >> >> >> > down >> >> >> > problem. Could you please illustrate what kind of problem we could >> >> >> > have >> >> >> > here? >> >> >> > >> >> >> > https://codereview.chromium.org/24883002/ >> >> > >> >> > >> > >> > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I have fixed the round-down problems. Could you please review again? Thanks. On Wed, Oct 2, 2013 at 1:54 PM, Scott Violet <sky@chromium.org> wrote: > I don't want a DCHECK like that every where. At some point aura may > want fraction metrics. If you're going to add the support for this, > make it do the right thing everywhere. > > -Scott > > On Wed, Oct 2, 2013 at 1:24 PM, Jian Li <jianli@chromium.org> wrote: > > All these potential round down changes are in views/ash codebase. We > suppose > > to return and use the integral width here. So no round-down should be > > expected. How about adding some kind of DCHECK to help catching this > > problem, like: > > float text_width = font.GetStringWidth(text_); > > DCHECK_EQ(text_width - static_cast<int>(text_width), 0); > > int max_width = std::min<int>( > > text_width + space_width + link_size.width(), > > bubble_view->GetMaximumSize().width() - (used_width + > > insets.width())); > > > > > > On Wed, Oct 2, 2013 at 1:16 PM, Scott Violet <sky@chromium.org> wrote: > >> > >> I get why we want fractional metrics. I just want to make sure we > >> round the right way. > >> > >> -Scott > >> > >> On Wed, Oct 2, 2013 at 9:35 AM, Alexei Svitkine <asvitkine@chromium.org > > > >> wrote: > >> > True. Which is why I didn't push back on this CL not doing the > selective > >> > change like I originally suggested. > >> > > >> > Scott, to give some context here - this is indeed needed on Mac to > make > >> > text_elider.cc and other utils work correctly - since string widths > are > >> > fractional - and thus code that adds them together and such needs to > >> > operate > >> > on the float values rather than rounded up values to get correct > >> > results. > >> > That's the motivation for this change. > >> > > >> > > >> > > >> > On Wed, Oct 2, 2013 at 12:06 PM, Scott Violet <sky@chromium.org> > wrote: > >> >> > >> >> Doing this seems equally error prone in so far as folks have to be > >> >> very careful to know what platform they are. In some places this is > >> >> obvious, but not every where. > >> >> > >> >> On Wed, Oct 2, 2013 at 9:00 AM, Alexei Svitkine > >> >> <asvitkine@chromium.org> > >> >> wrote: > >> >> > I had originally suggested to keep both the int and add the float > >> >> > APIs > >> >> > and > >> >> > for things where it matters (i.e. code that runs on the Mac such as > >> >> > text > >> >> > elider), to use the float API, while leaving most of views alone > >> >> > (i.e. > >> >> > still > >> >> > using int API). i.e. GetStringSize and GetStringSizeF. > >> >> > > >> >> > > >> >> > > >> >> > On Wed, Oct 2, 2013 at 11:58 AM, Scott Violet <sky@chromium.org> > >> >> > wrote: > >> >> >> > >> >> >> My worry is a general worry that any time you expose an API that > >> >> >> takes > >> >> >> float and you start rounding down you are going to get rounding > >> >> >> errors. If we're to have a float API then we should do the right > >> >> >> thing > >> >> >> everywhere, regardless if they are currently using floats. > >> >> >> > >> >> >> -Scott > >> >> >> > >> >> >> On Tue, Oct 1, 2013 at 5:49 PM, <jianli@chromium.org> wrote: > >> >> >> > > >> >> >> > > >> >> >> > > >> >> >> > > >> >> >> > > https://codereview.chromium.org/24883002/diff/162001/ash/system/user/tray_use... > >> >> >> > File ash/system/user/tray_user.cc (right): > >> >> >> > > >> >> >> > > >> >> >> > > >> >> >> > > >> >> >> > > https://codereview.chromium.org/24883002/diff/162001/ash/system/user/tray_use... > >> >> >> > ash/system/user/tray_user.cc:546: font.GetStringWidth(text_) + > >> >> >> > space_width + link_size.width(), > >> >> >> > On 2013/10/01 23:53:03, sky wrote: > >> >> >> >> > >> >> >> >> Won't this round down? It seems all to easy to accidentally > round > >> >> >> >> down > >> >> >> > > >> >> >> > this this > >> >> >> >> > >> >> >> >> change. Is there a way to fix that? > >> >> >> > > >> >> >> > > >> >> >> > Does ash use the fractional width? If not, we should not have > >> >> >> > round > >> >> >> > down > >> >> >> > problem. Could you please illustrate what kind of problem we > could > >> >> >> > have > >> >> >> > here? > >> >> >> > > >> >> >> > https://codereview.chromium.org/24883002/ > >> >> > > >> >> > > >> > > >> > > > > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/24883002/diff/214001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/autofill_popup_controller.h (right): https://codereview.chromium.org/24883002/diff/214001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_popup_controller.h:66: virtual void SetPopupBounds(const gfx::RectF& bounds) = 0; Why do any of the changes in this file require floats? Bounds should always end up as ints, right? https://codereview.chromium.org/24883002/diff/214001/ui/gfx/canvas.h File ui/gfx/canvas.h (right): https://codereview.chromium.org/24883002/diff/214001/ui/gfx/canvas.h#newcode142 ui/gfx/canvas.h:142: // Returns the number of horizontal pixels needed to display the specified Update description, since presumably a fractional pixel doesn't make sense.
https://codereview.chromium.org/24883002/diff/214001/chrome/browser/ui/autofi... File chrome/browser/ui/autofill/autofill_popup_controller.h (right): https://codereview.chromium.org/24883002/diff/214001/chrome/browser/ui/autofi... chrome/browser/ui/autofill/autofill_popup_controller.h:66: virtual void SetPopupBounds(const gfx::RectF& bounds) = 0; On 2013/10/03 14:19:23, sky wrote: > Why do any of the changes in this file require floats? Bounds should always end > up as ints, right? On Mac, bounds are represented as floats underneath. Since these auto-fill popup bounds are computed based on string width, we should try to return float bounds in order to avoid possible round-down. Otherwise, it could be possible that the truncated bounds add up with other truncated bounds which could cause the problem. https://codereview.chromium.org/24883002/diff/214001/ui/gfx/canvas.h File ui/gfx/canvas.h (right): https://codereview.chromium.org/24883002/diff/214001/ui/gfx/canvas.h#newcode142 ui/gfx/canvas.h:142: // Returns the number of horizontal pixels needed to display the specified On 2013/10/03 14:19:23, sky wrote: > Update description, since presumably a fractional pixel doesn't make sense. Done.
On Thu, Oct 3, 2013 at 11:42 AM, <jianli@chromium.org> wrote: > > https://codereview.chromium.org/24883002/diff/214001/chrome/browser/ui/autofi... > File chrome/browser/ui/autofill/autofill_popup_controller.h (right): > > https://codereview.chromium.org/24883002/diff/214001/chrome/browser/ui/autofi... > chrome/browser/ui/autofill/autofill_popup_controller.h:66: virtual void > SetPopupBounds(const gfx::RectF& bounds) = 0; > On 2013/10/03 14:19:23, sky wrote: >> >> Why do any of the changes in this file require floats? Bounds should > > always end >> >> up as ints, right? > > > On Mac, bounds are represented as floats underneath. Since these > auto-fill popup bounds are computed based on string width, we should try > to return float bounds in order to avoid possible round-down. Otherwise, > it could be possible that the truncated bounds add up with other > truncated bounds which could cause the problem. Is that true of window bounds as well? Seems like they are going to be rounded up, no? -Scott > > > https://codereview.chromium.org/24883002/diff/214001/ui/gfx/canvas.h > File ui/gfx/canvas.h (right): > > https://codereview.chromium.org/24883002/diff/214001/ui/gfx/canvas.h#newcode142 > ui/gfx/canvas.h:142: // Returns the number of horizontal pixels needed > to display the specified > On 2013/10/03 14:19:23, sky wrote: >> >> Update description, since presumably a fractional pixel doesn't make > > sense. > > Done. > > https://codereview.chromium.org/24883002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/10/03 22:11:27, sky wrote: > On Thu, Oct 3, 2013 at 11:42 AM, <mailto:jianli@chromium.org> wrote: > > > > > https://codereview.chromium.org/24883002/diff/214001/chrome/browser/ui/autofi... > > File chrome/browser/ui/autofill/autofill_popup_controller.h (right): > > > > > https://codereview.chromium.org/24883002/diff/214001/chrome/browser/ui/autofi... > > chrome/browser/ui/autofill/autofill_popup_controller.h:66: virtual void > > SetPopupBounds(const gfx::RectF& bounds) = 0; > > On 2013/10/03 14:19:23, sky wrote: > >> > >> Why do any of the changes in this file require floats? Bounds should > > > > always end > >> > >> up as ints, right? > > > > > > On Mac, bounds are represented as floats underneath. Since these > > auto-fill popup bounds are computed based on string width, we should try > > to return float bounds in order to avoid possible round-down. Otherwise, > > it could be possible that the truncated bounds add up with other > > truncated bounds which could cause the problem. > > Is that true of window bounds as well? Seems like they are going to be > rounded up, no? > > -Scott > > > > > > https://codereview.chromium.org/24883002/diff/214001/ui/gfx/canvas.h > > File ui/gfx/canvas.h (right): > > > > > https://codereview.chromium.org/24883002/diff/214001/ui/gfx/canvas.h#newcode142 > > ui/gfx/canvas.h:142: // Returns the number of horizontal pixels needed > > to display the specified > > On 2013/10/03 14:19:23, sky wrote: > >> > >> Update description, since presumably a fractional pixel doesn't make > > > > sense. > > > > Done. > > > > https://codereview.chromium.org/24883002/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. In thinking about this it seems if you're to do it right EVERYWHERE should use floats, right? For examples, views should move to being float based. I'm a bit loathe to do that now without a compelling reason. Sorry for the run around, but I'm leaning toward ifdefs for the float based variants and keeping the int based ones else where. If we go that route how problematic is it for cross platform code? One other meta question. Do we really care about this? I can't quite tell from the bug what specific problems was encountered when using ceiling. Presumably this only matters if you're trying to position two things close together, which I can't really imagine we are when truncating. -Scott
On Thu, Oct 3, 2013 at 3:24 PM, <sky@chromium.org> wrote: > On 2013/10/03 22:11:27, sky wrote: > > On Thu, Oct 3, 2013 at 11:42 AM, <mailto:jianli@chromium.org> wrote: >> > >> > >> > > https://codereview.chromium.**org/24883002/diff/214001/** > chrome/browser/ui/autofill/**autofill_popup_controller.h<https://codereview.chromium.org/24883002/diff/214001/chrome/browser/ui/autofill/autofill_popup_controller.h> > >> > File chrome/browser/ui/autofill/**autofill_popup_controller.h (right): >> > >> > >> > > https://codereview.chromium.**org/24883002/diff/214001/** > chrome/browser/ui/autofill/**autofill_popup_controller.h#**newcode66<https://codereview.chromium.org/24883002/diff/214001/chrome/browser/ui/autofill/autofill_popup_controller.h#newcode66> > >> > chrome/browser/ui/autofill/**autofill_popup_controller.h:**66: virtual >> void >> > SetPopupBounds(const gfx::RectF& bounds) = 0; >> > On 2013/10/03 14:19:23, sky wrote: >> >> >> >> Why do any of the changes in this file require floats? Bounds should >> > >> > always end >> >> >> >> up as ints, right? >> > >> > >> > On Mac, bounds are represented as floats underneath. Since these >> > auto-fill popup bounds are computed based on string width, we should try >> > to return float bounds in order to avoid possible round-down. Otherwise, >> > it could be possible that the truncated bounds add up with other >> > truncated bounds which could cause the problem. >> > > Is that true of window bounds as well? Seems like they are going to be >> rounded up, no? >> > > -Scott >> > >> > >> > https://codereview.chromium.**org/24883002/diff/214001/ui/** >> gfx/canvas.h<https://codereview.chromium.org/24883002/diff/214001/ui/gfx/canvas.h> >> > File ui/gfx/canvas.h (right): >> > >> > >> > > https://codereview.chromium.**org/24883002/diff/214001/ui/** > gfx/canvas.h#newcode142<https://codereview.chromium.org/24883002/diff/214001/ui/gfx/canvas.h#newcode142> > >> > ui/gfx/canvas.h:142: // Returns the number of horizontal pixels needed >> > to display the specified >> > On 2013/10/03 14:19:23, sky wrote: >> >> >> >> Update description, since presumably a fractional pixel doesn't make >> > >> > sense. >> > >> > Done. >> > >> > https://codereview.chromium.**org/24883002/<https://codereview.chromium.org/2... >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+**unsubscribe@chromium.org<chromium-reviews%2Bunsubscribe@chromium.org> >> . >> > > In thinking about this it seems if you're to do it right EVERYWHERE should > use > floats, right? For examples, views should move to being float based. I'm a > bit > loathe to do that now without a compelling reason. Sorry for the run > around, but > I'm leaning toward ifdefs for the float based variants and keeping the int > based > ones else where. If we go that route how problematic is it for cross > platform > code? > I have thought about this approach. But I decide not to pursue this since it will touch far more places than this patch and it could bring up other problems. > > One other meta question. Do we really care about this? I can't quite tell > from > the bug what specific problems was encountered when using ceiling. > Presumably > this only matters if you're trying to position two things close together, > which > I can't really imagine we are when truncating. > Returning ceiling value is just a temporary workaround to fix the problem we have to elide and wrap a long text into multiple lines. The only bad thing I am aware of is that we might fit less words in one line compared with using floats for computation. We can keep using floats for computing font and string width. For bounds, we can return ceiling width. How do you think? > > -Scott > > https://codereview.chromium.**org/24883002/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, Oct 3, 2013 at 3:54 PM, Jian Li <jianli@chromium.org> wrote: > > > > On Thu, Oct 3, 2013 at 3:24 PM, <sky@chromium.org> wrote: >> >> On 2013/10/03 22:11:27, sky wrote: >> >>> On Thu, Oct 3, 2013 at 11:42 AM, <mailto:jianli@chromium.org> wrote: >>> > >>> > >> >> >> >> https://codereview.chromium.org/24883002/diff/214001/chrome/browser/ui/autofi... >>> >>> > File chrome/browser/ui/autofill/autofill_popup_controller.h (right): >>> > >>> > >> >> >> >> https://codereview.chromium.org/24883002/diff/214001/chrome/browser/ui/autofi... >>> >>> > chrome/browser/ui/autofill/autofill_popup_controller.h:66: virtual void >>> > SetPopupBounds(const gfx::RectF& bounds) = 0; >>> > On 2013/10/03 14:19:23, sky wrote: >>> >> >>> >> Why do any of the changes in this file require floats? Bounds should >>> > >>> > always end >>> >> >>> >> up as ints, right? >>> > >>> > >>> > On Mac, bounds are represented as floats underneath. Since these >>> > auto-fill popup bounds are computed based on string width, we should >>> > try >>> > to return float bounds in order to avoid possible round-down. >>> > Otherwise, >>> > it could be possible that the truncated bounds add up with other >>> > truncated bounds which could cause the problem. >> >> >>> Is that true of window bounds as well? Seems like they are going to be >>> rounded up, no? >> >> >>> -Scott >>> > >>> > >>> > https://codereview.chromium.org/24883002/diff/214001/ui/gfx/canvas.h >>> > File ui/gfx/canvas.h (right): >>> > >>> > >> >> >> >> https://codereview.chromium.org/24883002/diff/214001/ui/gfx/canvas.h#newcode142 >>> >>> > ui/gfx/canvas.h:142: // Returns the number of horizontal pixels needed >>> > to display the specified >>> > On 2013/10/03 14:19:23, sky wrote: >>> >> >>> >> Update description, since presumably a fractional pixel doesn't make >>> > >>> > sense. >>> > >>> > Done. >>> > >>> > https://codereview.chromium.org/24883002/ >> >> >>> To unsubscribe from this group and stop receiving emails from it, send an >> >> email >>> >>> to mailto:chromium-reviews+unsubscribe@chromium.org. >> >> >> In thinking about this it seems if you're to do it right EVERYWHERE should >> use >> floats, right? For examples, views should move to being float based. I'm a >> bit >> loathe to do that now without a compelling reason. Sorry for the run >> around, but >> I'm leaning toward ifdefs for the float based variants and keeping the int >> based >> ones else where. If we go that route how problematic is it for cross >> platform >> code? > > > I have thought about this approach. But I decide not to pursue this since it > will touch far more places than this patch and it could bring up other > problems. >> >> >> One other meta question. Do we really care about this? I can't quite tell >> from >> the bug what specific problems was encountered when using ceiling. >> Presumably >> this only matters if you're trying to position two things close together, >> which >> I can't really imagine we are when truncating. > > > Returning ceiling value is just a temporary workaround to fix the problem we > have to elide and wrap a long text into multiple lines. The only bad thing I > am aware of is that we might fit less words in one line compared with using > floats for computation. If we introduce a floating point api then it should be everywhere. Meaning bounds, size, point all that. Which is a huge change. Seems like this problem only applies to eliding as its largely the only place needing fractional metrics since it's doing computation at word boundaries. I say we go back to your earlier change where we constrain this to just textelider whatever it needs. Keep the int API and have it do a ceiling. That should be a much simpler, change right? Maybe close to your first. Sorry for the run around. -Scott > > We can keep using floats for computing font and string width. For bounds, we > can return ceiling width. How do you think? > >> >> >> -Scott >> >> https://codereview.chromium.org/24883002/ > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I changed to add and use the float version only, per Scott's suggestion. I need to change all the Elide methods to use floats since they all show some sort of accumulated truncation error. On 2013/10/03 23:40:13, sky wrote: > On Thu, Oct 3, 2013 at 3:54 PM, Jian Li <mailto:jianli@chromium.org> wrote: > > > > > > > > On Thu, Oct 3, 2013 at 3:24 PM, <mailto:sky@chromium.org> wrote: > >> > >> On 2013/10/03 22:11:27, sky wrote: > >> > >>> On Thu, Oct 3, 2013 at 11:42 AM, <mailto:jianli@chromium.org> wrote: > >>> > > >>> > > >> > >> > >> > >> > https://codereview.chromium.org/24883002/diff/214001/chrome/browser/ui/autofi... > >>> > >>> > File chrome/browser/ui/autofill/autofill_popup_controller.h (right): > >>> > > >>> > > >> > >> > >> > >> > https://codereview.chromium.org/24883002/diff/214001/chrome/browser/ui/autofi... > >>> > >>> > chrome/browser/ui/autofill/autofill_popup_controller.h:66: virtual void > >>> > SetPopupBounds(const gfx::RectF& bounds) = 0; > >>> > On 2013/10/03 14:19:23, sky wrote: > >>> >> > >>> >> Why do any of the changes in this file require floats? Bounds should > >>> > > >>> > always end > >>> >> > >>> >> up as ints, right? > >>> > > >>> > > >>> > On Mac, bounds are represented as floats underneath. Since these > >>> > auto-fill popup bounds are computed based on string width, we should > >>> > try > >>> > to return float bounds in order to avoid possible round-down. > >>> > Otherwise, > >>> > it could be possible that the truncated bounds add up with other > >>> > truncated bounds which could cause the problem. > >> > >> > >>> Is that true of window bounds as well? Seems like they are going to be > >>> rounded up, no? > >> > >> > >>> -Scott > >>> > > >>> > > >>> > https://codereview.chromium.org/24883002/diff/214001/ui/gfx/canvas.h > >>> > File ui/gfx/canvas.h (right): > >>> > > >>> > > >> > >> > >> > >> > https://codereview.chromium.org/24883002/diff/214001/ui/gfx/canvas.h#newcode142 > >>> > >>> > ui/gfx/canvas.h:142: // Returns the number of horizontal pixels needed > >>> > to display the specified > >>> > On 2013/10/03 14:19:23, sky wrote: > >>> >> > >>> >> Update description, since presumably a fractional pixel doesn't make > >>> > > >>> > sense. > >>> > > >>> > Done. > >>> > > >>> > https://codereview.chromium.org/24883002/ > >> > >> > >>> To unsubscribe from this group and stop receiving emails from it, send an > >> > >> email > >>> > >>> to mailto:chromium-reviews+unsubscribe@chromium.org. > >> > >> > >> In thinking about this it seems if you're to do it right EVERYWHERE should > >> use > >> floats, right? For examples, views should move to being float based. I'm a > >> bit > >> loathe to do that now without a compelling reason. Sorry for the run > >> around, but > >> I'm leaning toward ifdefs for the float based variants and keeping the int > >> based > >> ones else where. If we go that route how problematic is it for cross > >> platform > >> code? > > > > > > I have thought about this approach. But I decide not to pursue this since it > > will touch far more places than this patch and it could bring up other > > problems. > >> > >> > >> One other meta question. Do we really care about this? I can't quite tell > >> from > >> the bug what specific problems was encountered when using ceiling. > >> Presumably > >> this only matters if you're trying to position two things close together, > >> which > >> I can't really imagine we are when truncating. > > > > > > Returning ceiling value is just a temporary workaround to fix the problem we > > have to elide and wrap a long text into multiple lines. The only bad thing I > > am aware of is that we might fit less words in one line compared with using > > floats for computation. > > If we introduce a floating point api then it should be everywhere. > Meaning bounds, size, point all that. Which is a huge change. > > Seems like this problem only applies to eliding as its largely the > only place needing fractional metrics since it's doing computation at > word boundaries. I say we go back to your earlier change where we > constrain this to just textelider whatever it needs. Keep the int API > and have it do a ceiling. That should be a much simpler, change right? > Maybe close to your first. > > Sorry for the run around. > > -Scott > > > > > We can keep using floats for computing font and string width. For bounds, we > > can return ceiling width. How do you think? > > > >> > >> > >> -Scott > >> > >> https://codereview.chromium.org/24883002/ > > > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
I only poked at a couple of places since it seems you are quite done. I prefer this one. Thanks!
That should be, "aren't quite done". On Fri, Oct 4, 2013 at 4:51 PM, <sky@chromium.org> wrote: > I only poked at a couple of places since it seems you are quite done. I > prefer > this one. Thanks! > > https://codereview.chromium.org/24883002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/10/05 00:21:38, sky wrote: > That should be, "aren't quite done". > > On Fri, Oct 4, 2013 at 4:51 PM, <mailto:sky@chromium.org> wrote: > > I only poked at a couple of places since it seems you are quite done. I > > prefer > > this one. Thanks! > > > > https://codereview.chromium.org/24883002/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. I have finished all the changes and fixed the trybot failures. Could you please review again? Thanks. If I am still missing something, could you please let me know?
https://codereview.chromium.org/24883002/diff/274001/ui/gfx/font.h File ui/gfx/font.h (right): https://codereview.chromium.org/24883002/diff/274001/ui/gfx/font.h#newcode84 ui/gfx/font.h:84: float GetStringWidthF(const base::string16& text) const; Is this actually needed? Does someone need to use Font::GetStringWidthF() that can't use gfx::GetStringWidthF()? We're trying to get rid of these APIs on Font - so I'd prefer to not introduce this new version of the function on Font and PlatformFont. https://codereview.chromium.org/24883002/diff/274001/ui/gfx/font_list.cc File ui/gfx/font_list.cc (right): https://codereview.chromium.org/24883002/diff/274001/ui/gfx/font_list.cc#newc... ui/gfx/font_list.cc:198: float FontList::GetStringWidthF(const base::string16& text) const { Do we need this? Or can everything that needs it use gfx::GetStringWidthF() and avoid introducing this API? https://codereview.chromium.org/24883002/diff/274001/ui/gfx/text_utils.h File ui/gfx/text_utils.h (right): https://codereview.chromium.org/24883002/diff/274001/ui/gfx/text_utils.h#newc... ui/gfx/text_utils.h:31: //|font_list|. Note that the fractional width might be returned in some Nit: |in| -> |on|
https://codereview.chromium.org/24883002/diff/274001/ui/gfx/font.h File ui/gfx/font.h (right): https://codereview.chromium.org/24883002/diff/274001/ui/gfx/font.h#newcode84 ui/gfx/font.h:84: float GetStringWidthF(const base::string16& text) const; On 2013/10/07 21:43:39, Alexei Svitkine wrote: > Is this actually needed? Does someone need to use Font::GetStringWidthF() that > can't use gfx::GetStringWidthF()? > > We're trying to get rid of these APIs on Font - so I'd prefer to not introduce > this new version of the function on Font and PlatformFont. Done. https://codereview.chromium.org/24883002/diff/274001/ui/gfx/font_list.cc File ui/gfx/font_list.cc (right): https://codereview.chromium.org/24883002/diff/274001/ui/gfx/font_list.cc#newc... ui/gfx/font_list.cc:198: float FontList::GetStringWidthF(const base::string16& text) const { On 2013/10/07 21:43:39, Alexei Svitkine wrote: > Do we need this? Or can everything that needs it use gfx::GetStringWidthF() and > avoid introducing this API? Done. https://codereview.chromium.org/24883002/diff/274001/ui/gfx/text_utils.h File ui/gfx/text_utils.h (right): https://codereview.chromium.org/24883002/diff/274001/ui/gfx/text_utils.h#newc... ui/gfx/text_utils.h:31: //|font_list|. Note that the fractional width might be returned in some On 2013/10/07 21:43:39, Alexei Svitkine wrote: > Nit: |in| -> |on| Done.
LGTM with some nits! Thanks! https://codereview.chromium.org/24883002/diff/300001/ui/gfx/canvas.cc File ui/gfx/canvas.cc (right): https://codereview.chromium.org/24883002/diff/300001/ui/gfx/canvas.cc#newcode93 ui/gfx/canvas.cc:93: float fractional_width = *width, factional_height = *height; Nit: Separate lines. https://codereview.chromium.org/24883002/diff/300001/ui/gfx/canvas.h File ui/gfx/canvas.h (right): https://codereview.chromium.org/24883002/diff/300001/ui/gfx/canvas.h#newcode158 ui/gfx/canvas.h:158: // |font_list|. Note that the fractional width might be returned in some Maybe mention explicitly which ones like the render_text.h comment. https://codereview.chromium.org/24883002/diff/300001/ui/gfx/canvas_unittest_m... File ui/gfx/canvas_unittest_mac.mm (right): https://codereview.chromium.org/24883002/diff/300001/ui/gfx/canvas_unittest_m... ui/gfx/canvas_unittest_mac.mm:22: // sizeWithAttributes. Nit: "sizeWithAttributes" -> "-sizeWithAttributes" (with a dash per ObjC convention). https://codereview.chromium.org/24883002/diff/300001/ui/gfx/text_elider.cc File ui/gfx/text_elider.cc (right): https://codereview.chromium.org/24883002/diff/300001/ui/gfx/text_elider.cc#ne... ui/gfx/text_elider.cc:311: gfx::GetStringWidthF(url_domain, font_list); Nit: Can you remove the gfx:: prefix on all of these in this file? (text_elider.cc has recently been move to gfx, so gfx:: prefix isn't needed here). Then, this doesn't need to wrap. https://codereview.chromium.org/24883002/diff/300001/ui/gfx/text_elider_unitt... File ui/gfx/text_elider_unittest.cc (right): https://codereview.chromium.org/24883002/diff/300001/ui/gfx/text_elider_unitt... ui/gfx/text_elider_unittest.cc:56: GetStringWidthF(UTF8ToUTF16(testcases[i].output), font_list), Nit: Move this to a local variable, so that the EXPECT_EQ() statement is shorter. https://codereview.chromium.org/24883002/diff/300001/ui/gfx/text_elider_unitt... ui/gfx/text_elider_unittest.cc:122: gfx::GetStringWidthF(expected_output, font_list))); Nit: No need for gfx::. Please fix throughout this file. https://codereview.chromium.org/24883002/diff/300001/ui/gfx/text_utils.h File ui/gfx/text_utils.h (right): https://codereview.chromium.org/24883002/diff/300001/ui/gfx/text_utils.h#newc... ui/gfx/text_utils.h:31: //|font_list|. Note that the fractional width might be returned on some Maybe mention explicitly which ones like the render_text.h comment.
https://codereview.chromium.org/24883002/diff/300001/ui/gfx/canvas.cc File ui/gfx/canvas.cc (right): https://codereview.chromium.org/24883002/diff/300001/ui/gfx/canvas.cc#newcode93 ui/gfx/canvas.cc:93: float fractional_width = *width, factional_height = *height; On 2013/10/07 23:33:28, Alexei Svitkine wrote: > Nit: Separate lines. Done. https://codereview.chromium.org/24883002/diff/300001/ui/gfx/canvas.h File ui/gfx/canvas.h (right): https://codereview.chromium.org/24883002/diff/300001/ui/gfx/canvas.h#newcode158 ui/gfx/canvas.h:158: // |font_list|. Note that the fractional width might be returned in some On 2013/10/07 23:33:28, Alexei Svitkine wrote: > Maybe mention explicitly which ones like the render_text.h comment. Done. https://codereview.chromium.org/24883002/diff/300001/ui/gfx/canvas_unittest_m... File ui/gfx/canvas_unittest_mac.mm (right): https://codereview.chromium.org/24883002/diff/300001/ui/gfx/canvas_unittest_m... ui/gfx/canvas_unittest_mac.mm:22: // sizeWithAttributes. On 2013/10/07 23:33:28, Alexei Svitkine wrote: > Nit: "sizeWithAttributes" -> "-sizeWithAttributes" (with a dash per ObjC > convention). Done. https://codereview.chromium.org/24883002/diff/300001/ui/gfx/text_elider.cc File ui/gfx/text_elider.cc (right): https://codereview.chromium.org/24883002/diff/300001/ui/gfx/text_elider.cc#ne... ui/gfx/text_elider.cc:311: gfx::GetStringWidthF(url_domain, font_list); On 2013/10/07 23:33:28, Alexei Svitkine wrote: > Nit: Can you remove the gfx:: prefix on all of these in this file? > (text_elider.cc has recently been move to gfx, so gfx:: prefix isn't needed > here). Then, this doesn't need to wrap. Done. https://codereview.chromium.org/24883002/diff/300001/ui/gfx/text_elider_unitt... File ui/gfx/text_elider_unittest.cc (right): https://codereview.chromium.org/24883002/diff/300001/ui/gfx/text_elider_unitt... ui/gfx/text_elider_unittest.cc:56: GetStringWidthF(UTF8ToUTF16(testcases[i].output), font_list), On 2013/10/07 23:33:28, Alexei Svitkine wrote: > Nit: Move this to a local variable, so that the EXPECT_EQ() statement is > shorter. Done. https://codereview.chromium.org/24883002/diff/300001/ui/gfx/text_elider_unitt... ui/gfx/text_elider_unittest.cc:122: gfx::GetStringWidthF(expected_output, font_list))); On 2013/10/07 23:33:28, Alexei Svitkine wrote: > Nit: No need for gfx::. Please fix throughout this file. Done. https://codereview.chromium.org/24883002/diff/300001/ui/gfx/text_utils.h File ui/gfx/text_utils.h (right): https://codereview.chromium.org/24883002/diff/300001/ui/gfx/text_utils.h#newc... ui/gfx/text_utils.h:31: //|font_list|. Note that the fractional width might be returned on some On 2013/10/07 23:33:28, Alexei Svitkine wrote: > Maybe mention explicitly which ones like the render_text.h comment. Done.
https://codereview.chromium.org/24883002/diff/309001/ui/gfx/canvas.h File ui/gfx/canvas.h (right): https://codereview.chromium.org/24883002/diff/309001/ui/gfx/canvas.h#newcode142 ui/gfx/canvas.h:142: // This is same as SizeStringInt except that fractional size is returned. Please add a comment here and any public API you're adding about when and why someone would want the float methods. Feel free to document this once (maybe Font is the right place) and see look at XXX for when you would use this. https://codereview.chromium.org/24883002/diff/309001/ui/gfx/canvas_unittest_m... File ui/gfx/canvas_unittest_mac.mm (right): https://codereview.chromium.org/24883002/diff/309001/ui/gfx/canvas_unittest_m... ui/gfx/canvas_unittest_mac.mm:85: EXPECT_GT(width - static_cast<int>(width), 0); nit: EXPECT_GE(width, static_cast<int>(width) https://codereview.chromium.org/24883002/diff/309001/ui/gfx/render_text_mac.cc File ui/gfx/render_text_mac.cc (right): https://codereview.chromium.org/24883002/diff/309001/ui/gfx/render_text_mac.c... ui/gfx/render_text_mac.cc:27: EnsureLayout(); How about return ToCeiledSize(GetStringSizeF()) ?
https://codereview.chromium.org/24883002/diff/309001/ui/gfx/canvas.h File ui/gfx/canvas.h (right): https://codereview.chromium.org/24883002/diff/309001/ui/gfx/canvas.h#newcode142 ui/gfx/canvas.h:142: // This is same as SizeStringInt except that fractional size is returned. On 2013/10/08 02:12:56, sky wrote: > Please add a comment here and any public API you're adding about when and why > someone would want the float methods. Feel free to document this once (maybe > Font is the right place) and see look at XXX for when you would use this. Added comment in Canvas::GetStringWidthF. We do not make any new addition to Font class any more. https://codereview.chromium.org/24883002/diff/309001/ui/gfx/canvas_unittest_m... File ui/gfx/canvas_unittest_mac.mm (right): https://codereview.chromium.org/24883002/diff/309001/ui/gfx/canvas_unittest_m... ui/gfx/canvas_unittest_mac.mm:85: EXPECT_GT(width - static_cast<int>(width), 0); On 2013/10/08 02:12:56, sky wrote: > nit: EXPECT_GE(width, static_cast<int>(width) Changed to EXPECT_GT(width, static_cast<int>(width)). I think we would like to use EXPECT_GT to make sure the fractional value is checked. https://codereview.chromium.org/24883002/diff/309001/ui/gfx/render_text_mac.cc File ui/gfx/render_text_mac.cc (right): https://codereview.chromium.org/24883002/diff/309001/ui/gfx/render_text_mac.c... ui/gfx/render_text_mac.cc:27: EnsureLayout(); On 2013/10/08 02:12:56, sky wrote: > How about > return ToCeiledSize(GetStringSizeF()) > ? Returning both ceiled width and height will cause test RenderTextTest.StringSizeRespectsFontListMetrics to fail. This is because [NSLayoutManager defaultLineHeightForFont] (called by Font::GetHeight) returns the non-fractional height while CTLineGetTypographicBounds (used by RenderTextMac in height computation) returns the fractional height.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jianli@chromium.org/24883002/349001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jianli@chromium.org/24883002/349001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jianli@chromium.org/24883002/349001
Message was sent while issue was closed.
Change committed as 227905 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
