Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
968923004:
Use GetDisplayText() instead of text() for rendering text. (Closed)
DescriptionUse GetDisplayText() instead of text() for rendering text.
BUG=None
R=msw@chromium.org
Committed: https://crrev.com/fea1a11f41748a6d2281b1017d70f618bf7b1354
Cr-Commit-Position: refs/heads/master@{#319742}
Patch Set 1 #Patch Set 2 : add mac test #Patch Set 3 : mac typo fix #
Total comments: 6
Patch Set 4 : static cast in mac test #Patch Set 5 : comments addressed #
Total comments: 1
Patch Set 6 : reset text_elided_ #Patch Set 7 : another fix #
Total comments: 8
Patch Set 8 : moar mac fixes #Patch Set 9 : fix typo #Patch Set 10 : possibly fix #Patch Set 11 : fix #Patch Set 12 : fix #Patch Set 13 : fix #
Total comments: 9
Patch Set 14 : fix #
Total comments: 4
Patch Set 15 : fix && address oshima's comment #Patch Set 16 : fix #
Total comments: 10
Patch Set 17 : comments addressed #Patch Set 18 : fix #
Total comments: 1
Patch Set 19 : fix #Patch Set 20 : rebase #Patch Set 21 : rebase2 #Patch Set 22 : fix failed tests #
Total comments: 2
Patch Set 23 : fix typo #
Messages
Total messages: 64 (23 generated)
oshima@chromium.org changed reviewers: + oshima@chromium.org
Can you add/modify unit test to catch this?
https://codereview.chromium.org/968923004/diff/40001/ui/gfx/render_text_mac.cc File ui/gfx/render_text_mac.cc (right): https://codereview.chromium.org/968923004/diff/40001/ui/gfx/render_text_mac.c... ui/gfx/render_text_mac.cc:151: base::SysUTF16ToCFStringRef(GetDisplayText())); This is one of the things I tried... unfortunately it wasn't enough to fix the eliding problem. (note RenderTextMac's override of GetDisplayText() is quite different to the one in RenderTextHarfBuzz). E.g. GetDisplayText() changes its return when the value of text_elided_ changes. That happens in RenderText::UpdateDisplayText(), but UpdateDisplayText() doesn't trigger a call to RenderTextMac::OnLayoutTextAttributeChanged(), so the previously calculated runs are not invalidated.
lgtm with nits https://codereview.chromium.org/968923004/diff/40001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/968923004/diff/40001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:2620: base::string16 text(ASCIIToUTF16("This is example.")); nit: "This is an example." https://codereview.chromium.org/968923004/diff/40001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:2623: // Note that glyph count matches to the text length because all the characters nit: "// NOTE: Character and glyph counts are only comparable for simple text." (optionally use this same one-liner comment at line 2038)
https://codereview.chromium.org/968923004/diff/40001/ui/gfx/render_text_mac.cc File ui/gfx/render_text_mac.cc (right): https://codereview.chromium.org/968923004/diff/40001/ui/gfx/render_text_mac.c... ui/gfx/render_text_mac.cc:151: base::SysUTF16ToCFStringRef(GetDisplayText())); On 2015/03/02 22:36:38, tapted wrote: > This is one of the things I tried... unfortunately it wasn't enough to fix the > eliding problem. > > (note RenderTextMac's override of GetDisplayText() is quite different to the one > in RenderTextHarfBuzz). > > E.g. GetDisplayText() changes its return when the value of text_elided_ changes. > That happens in RenderText::UpdateDisplayText(), but UpdateDisplayText() doesn't > trigger a call to RenderTextMac::OnLayoutTextAttributeChanged(), so the > previously calculated runs are not invalidated. I don't think RenderTextMac is quite different here, if elide_behavior is set, display_text() and text_elided() should be updated properly in render_text.cc, and this file seems to invoke UpdateDisplayText() properly. If it doesn't return elided text, that's a regression of RenderText. Also, the new test case should catch the failure by checking the number of glyphs in |line_|. The test seems succeeding on the case, do you know what's wrong with my test case? https://codereview.chromium.org/968923004/diff/40001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/968923004/diff/40001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:2620: base::string16 text(ASCIIToUTF16("This is example.")); On 2015/03/02 22:50:55, msw wrote: > nit: "This is an example." Done. https://codereview.chromium.org/968923004/diff/40001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:2623: // Note that glyph count matches to the text length because all the characters On 2015/03/02 22:50:55, msw wrote: > nit: "// NOTE: Character and glyph counts are only comparable for simple text." > (optionally use this same one-liner comment at line 2038) Done.
tapted@chromium.org changed reviewers: + tapted@chromium.org
https://codereview.chromium.org/968923004/diff/80001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/968923004/diff/80001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:2628: gfx::Size string_size = render_text.GetStringSize(); It looks like this always returns 0-width, because the render text has a 0x0 DisplayRect at this point. With a 0x0 DisplayRect, the eliding step is effectively a no-op. If I add a SetDisplayRect(gfx::Rect(0, 0, 200, 10))` up before line 2621 like in https://codereview.chromium.org/960203004/ the test crashes. There's a bit more tracing in crrev/960203004 . E.g. You can see the eliding being skipped without that SetDisplayRect call.
ptal the test crash happens due to out-of-range index for elided text. The latest patchset contains the fix for that.
Unfortunately http://crbug.com/462492 still manifests :( I've done some more tracing - see below. Sadly, I still can't think of an easy fix for RenderTextMac (just my ideas in http://crrev.com/867003002/#msg27 e.g. caching "dumb/unelided" lines in Label::lines_, or using RenderTextHarfBuzz for views code on Mac). But, I've got a fix out for one of the blocking issues for Harfbuzz on Mac (i.e. http://crbug.com/439039 ), so it's a little bit closer to reality :) https://codereview.chromium.org/968923004/diff/120001/ui/gfx/render_text_mac.cc File ui/gfx/render_text_mac.cc (right): https://codereview.chromium.org/968923004/diff/120001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:116: UpdateDisplayText(GetContentWidth()); Gosh this is hard to explain :/ But, I think the problem boils down to this line. GetContentWidth() calls GetStringSizeF() [just above] which calls EnsureLayout() [just below] which [now] calls GetDisplayText(), but the display text isn't properly updated until *after* this line. For example, if the content rect changes from zero to non-zero while the eliding behavior is eliding (which is true whenever display_text != text, which is always true when the width is 0), then an incorrect contentwidth will be passed here. https://codereview.chromium.org/968923004/diff/120001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/968923004/diff/120001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2634: } For example, adding a check here: EXPECT_NE(0u, render_text.GetDisplayText().size()); will fail. This is because the FIRST time UpdateDisplayText is called [for the SetElideBehavior call on line 2627], the ContentRect is 0 so UpdateDisplayText gives: - display_text = "" - test = "This is an example." - elide = true Then the second time UpdateDisplayText() is called [for the SetDisplayRect call on line 2630], GetContentWidth() returns 1 (because in GetDisplayText() elided is still true, but the display_text is based on a width of 0, so it's an empty string). This results in UpdateDisplayText trying to elide the text based on a text width of 1, rather than render_text.GetStringSize().width I've had another crack at doing some tracing to make this clear -- see https://codereview.chromium.org/965393003
https://codereview.chromium.org/968923004/diff/120001/ui/gfx/render_text_mac.cc File ui/gfx/render_text_mac.cc (right): https://codereview.chromium.org/968923004/diff/120001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:116: UpdateDisplayText(GetContentWidth()); On 2015/03/03 08:11:21, tapted wrote: > Gosh this is hard to explain :/ > > But, I think the problem boils down to this line. > > GetContentWidth() calls GetStringSizeF() [just above] which calls EnsureLayout() > [just below] which [now] calls GetDisplayText(), but the display text isn't > properly updated until *after* this line. > > For example, if the content rect changes from zero to non-zero while the eliding > behavior is eliding (which is true whenever display_text != text, which is > always true when the width is 0), then an incorrect contentwidth will be passed > here. I believe this is not a problem, it looks like cyclic, but it's not actually. To work more precisely, I assume |text_elided_| to be false for the first time, then GetContentWidth() invokes EnsureLayout(), which actually computes the size based on layout_text() instead of display_text(). Then UpdateDisplayText() computes the elided display_text() (and set |text_elided_| to true), and then |line_| is cleared below, thus next EnsureLayout() will compute display_text() correctly. That said, I believe |text_elided_| should be set to false on render_text.cc properly. https://codereview.chromium.org/968923004/diff/120001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/968923004/diff/120001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2634: } On 2015/03/03 08:11:21, tapted wrote: > For example, adding a check here: > > EXPECT_NE(0u, render_text.GetDisplayText().size()); > > will fail. This is because the FIRST time UpdateDisplayText is called [for the > SetElideBehavior call on line 2627], the ContentRect is 0 so UpdateDisplayText > gives: > - display_text = "" > - test = "This is an example." > - elide = true > > Then the second time UpdateDisplayText() is called [for the SetDisplayRect call > on line 2630], GetContentWidth() returns 1 (because in GetDisplayText() elided > is still true, but the display_text is based on a width of 0, so it's an empty > string). > > This results in UpdateDisplayText trying to elide the text based on a text width > of 1, rather than render_text.GetStringSize().width > > > I've had another crack at doing some tracing to make this clear -- see > https://codereview.chromium.org/965393003 In case of non-specified display-rect, I think it should not compute text-eliding. Added display_rect_.IsEmpty() to the early-exit pattern for UpdateDisplayText(). I still suspect that these changes would fix the app-list label issue, but would fix this test case.
https://codereview.chromium.org/968923004/diff/120001/ui/gfx/render_text_mac.cc File ui/gfx/render_text_mac.cc (right): https://codereview.chromium.org/968923004/diff/120001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:116: UpdateDisplayText(GetContentWidth()); On 2015/03/03 08:34:00, Jun Mukai wrote: > On 2015/03/03 08:11:21, tapted wrote: > > Gosh this is hard to explain :/ > > > > But, I think the problem boils down to this line. > > > > GetContentWidth() calls GetStringSizeF() [just above] which calls > EnsureLayout() > > [just below] which [now] calls GetDisplayText(), but the display text isn't > > properly updated until *after* this line. > > > > For example, if the content rect changes from zero to non-zero while the > eliding > > behavior is eliding (which is true whenever display_text != text, which is > > always true when the width is 0), then an incorrect contentwidth will be > passed > > here. > > I believe this is not a problem, it looks like cyclic, but it's not actually. I think it is cyclic. But feels like it isn't because EnsureLayout() caches its result :). That it, it cycles every time there is a "change". As a proof, here add an extra UpdateDisplayText. i.e. UpdateDisplayText(GetContentWidth()); line_.reset(); UpdateDisplayText(GetContentWidth()); This will make the test in patchset 9 fail: ../../ui/gfx/render_text_unittest.cc:2633: Failure Expected: (text.size()) > (static_cast<size_t>(glyph_count)), actual: 19 vs 19
https://codereview.chromium.org/968923004/diff/120001/ui/gfx/render_text_mac.cc File ui/gfx/render_text_mac.cc (right): https://codereview.chromium.org/968923004/diff/120001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:116: UpdateDisplayText(GetContentWidth()); On 2015/03/03 09:30:32, tapted wrote: > On 2015/03/03 08:34:00, Jun Mukai wrote: > > On 2015/03/03 08:11:21, tapted wrote: > > > Gosh this is hard to explain :/ > > > > > > But, I think the problem boils down to this line. > > > > > > GetContentWidth() calls GetStringSizeF() [just above] which calls > > EnsureLayout() > > > [just below] which [now] calls GetDisplayText(), but the display text isn't > > > properly updated until *after* this line. > > > > > > For example, if the content rect changes from zero to non-zero while the > > eliding > > > behavior is eliding (which is true whenever display_text != text, which is > > > always true when the width is 0), then an incorrect contentwidth will be > > passed > > > here. > > > > I believe this is not a problem, it looks like cyclic, but it's not actually. > > I think it is cyclic. But feels like it isn't because EnsureLayout() caches its > result :). That it, it cycles every time there is a "change". > > As a proof, here add an extra UpdateDisplayText. i.e. > > UpdateDisplayText(GetContentWidth()); > line_.reset(); > UpdateDisplayText(GetContentWidth()); > > This will make the test in patchset 9 fail: > > ../../ui/gfx/render_text_unittest.cc:2633: Failure > Expected: (text.size()) > (static_cast<size_t>(glyph_count)), actual: 19 vs 19 You're right! Sorry for my previous comment. So, GetStringSize() should be computed from layout_text_ while DrawVisualText() should draw display_text_. this was done in RenderTextHarfBuzz but not correctly done in RenderTextMac. Uploaded a fix, which separates the size calculation and EnsureLayout(). PTAL.
https://codereview.chromium.org/968923004/diff/120001/ui/gfx/render_text_mac.cc File ui/gfx/render_text_mac.cc (right): https://codereview.chromium.org/968923004/diff/120001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:116: UpdateDisplayText(GetContentWidth()); On 2015/03/04 08:33:23, Jun Mukai wrote: > On 2015/03/03 09:30:32, tapted wrote: > > On 2015/03/03 08:34:00, Jun Mukai wrote: > > > On 2015/03/03 08:11:21, tapted wrote: > > > > Gosh this is hard to explain :/ > > > > > > > > But, I think the problem boils down to this line. > > > > > > > > GetContentWidth() calls GetStringSizeF() [just above] which calls > > > EnsureLayout() > > > > [just below] which [now] calls GetDisplayText(), but the display text > isn't > > > > properly updated until *after* this line. > > > > > > > > For example, if the content rect changes from zero to non-zero while the > > > eliding > > > > behavior is eliding (which is true whenever display_text != text, which is > > > > always true when the width is 0), then an incorrect contentwidth will be > > > passed > > > > here. > > > > > > I believe this is not a problem, it looks like cyclic, but it's not > actually. > > > > I think it is cyclic. But feels like it isn't because EnsureLayout() caches > its > > result :). That it, it cycles every time there is a "change". > > > > As a proof, here add an extra UpdateDisplayText. i.e. > > > > UpdateDisplayText(GetContentWidth()); > > line_.reset(); > > UpdateDisplayText(GetContentWidth()); > > > > This will make the test in patchset 9 fail: > > > > ../../ui/gfx/render_text_unittest.cc:2633: Failure > > Expected: (text.size()) > (static_cast<size_t>(glyph_count)), actual: 19 vs 19 > > You're right! Sorry for my previous comment. > So, GetStringSize() should be computed from layout_text_ while DrawVisualText() > should draw display_text_. this was done in RenderTextHarfBuzz but not correctly > done in RenderTextMac. > > Uploaded a fix, which separates the size calculation and EnsureLayout(). PTAL. FYI: This is cyclic by nature because you use the content width to compute the text that determines the content width. This works because this is called only after the text has been updated. The above scenario fails because it violates that assumption. One way to improve this would be to add separate GetLayoutTextWidth().
Fixed the buggy behaviors, and now I think it's back to the reviewable state. PTAL. https://codereview.chromium.org/968923004/diff/120001/ui/gfx/render_text_mac.cc File ui/gfx/render_text_mac.cc (right): https://codereview.chromium.org/968923004/diff/120001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:116: UpdateDisplayText(GetContentWidth()); On 2015/03/04 10:17:21, oshima wrote: > On 2015/03/04 08:33:23, Jun Mukai wrote: > > On 2015/03/03 09:30:32, tapted wrote: > > > On 2015/03/03 08:34:00, Jun Mukai wrote: > > > > On 2015/03/03 08:11:21, tapted wrote: > > > > > Gosh this is hard to explain :/ > > > > > > > > > > But, I think the problem boils down to this line. > > > > > > > > > > GetContentWidth() calls GetStringSizeF() [just above] which calls > > > > EnsureLayout() > > > > > [just below] which [now] calls GetDisplayText(), but the display text > > isn't > > > > > properly updated until *after* this line. > > > > > > > > > > For example, if the content rect changes from zero to non-zero while the > > > > eliding > > > > > behavior is eliding (which is true whenever display_text != text, which > is > > > > > always true when the width is 0), then an incorrect contentwidth will be > > > > passed > > > > > here. > > > > > > > > I believe this is not a problem, it looks like cyclic, but it's not > > actually. > > > > > > I think it is cyclic. But feels like it isn't because EnsureLayout() caches > > its > > > result :). That it, it cycles every time there is a "change". > > > > > > As a proof, here add an extra UpdateDisplayText. i.e. > > > > > > UpdateDisplayText(GetContentWidth()); > > > line_.reset(); > > > UpdateDisplayText(GetContentWidth()); > > > > > > This will make the test in patchset 9 fail: > > > > > > ../../ui/gfx/render_text_unittest.cc:2633: Failure > > > Expected: (text.size()) > (static_cast<size_t>(glyph_count)), actual: 19 vs > 19 > > > > You're right! Sorry for my previous comment. > > So, GetStringSize() should be computed from layout_text_ while > DrawVisualText() > > should draw display_text_. this was done in RenderTextHarfBuzz but not > correctly > > done in RenderTextMac. > > > > Uploaded a fix, which separates the size calculation and EnsureLayout(). PTAL. > > FYI: This is cyclic by nature because you use the content width to compute the > text that determines the content width. This works because this is called only > after the text has been updated. The above scenario fails because it violates > that assumption. > > One way to improve this would be to add separate GetLayoutTextWidth(). The latest patchset does that thing -- separating width for layout_text and computing line_ for GetDisplayText(), since they can be different when elide behavior is set.
nice! lgtm but wait for oshima/msw to have a look too There were some merge-conflicts, but I've verified http://crbug.com/462492 no longer manifests with the cached-labels change. (Thanks a ton!) https://codereview.chromium.org/968923004/diff/240001/ui/gfx/render_text_mac.cc File ui/gfx/render_text_mac.cc (right): https://codereview.chromium.org/968923004/diff/240001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:97: return std::min(index, GetDisplayText().size()); should this be `.size() - 1`? https://codereview.chromium.org/968923004/diff/240001/ui/gfx/render_text_mac.h File ui/gfx/render_text_mac.h (right): https://codereview.chromium.org/968923004/diff/240001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.h:87: void ApplyStyles(CFMutableAttributedStringRef attr_string, declare this as a const function? That would be a good safety I think. Same for EnsureLayoutInternal() https://codereview.chromium.org/968923004/diff/240001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.h:89: base::ScopedCFTypeRef<CFMutableArrayRef>* attributes_owner); It should be fine to return a base::ScopedCFTypeRef instead of taking a pointer. The same applies for EnsureLayoutInternal(..), but of course you'd still need to pass |line|. Taking |line| by pointer and returning |attributes| would allow EnsureStringSize to just ignore the return value, but it's not as consistent. So, up to you if you like this or not. https://codereview.chromium.org/968923004/diff/240001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/968923004/diff/240001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2631: render_text.EnsureLayout(); are these EnsureLayout calls needed? (and would that allow the friend dec to be dropped)
https://codereview.chromium.org/968923004/diff/240001/ui/gfx/render_text_mac.cc File ui/gfx/render_text_mac.cc (right): https://codereview.chromium.org/968923004/diff/240001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:97: return std::min(index, GetDisplayText().size()); On 2015/03/05 06:32:49, tapted wrote: > should this be `.size() - 1`? This change isn't necessary, removed. https://codereview.chromium.org/968923004/diff/240001/ui/gfx/render_text_mac.h File ui/gfx/render_text_mac.h (right): https://codereview.chromium.org/968923004/diff/240001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.h:87: void ApplyStyles(CFMutableAttributedStringRef attr_string, On 2015/03/05 06:32:49, tapted wrote: > declare this as a const function? That would be a good safety I think. Same for > EnsureLayoutInternal() Done. https://codereview.chromium.org/968923004/diff/240001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.h:89: base::ScopedCFTypeRef<CFMutableArrayRef>* attributes_owner); On 2015/03/05 06:32:49, tapted wrote: > It should be fine to return a base::ScopedCFTypeRef instead of taking a pointer. > > > The same applies for EnsureLayoutInternal(..), but of course you'd still need to > pass |line|. Taking |line| by pointer and returning |attributes| would allow > EnsureStringSize to just ignore the return value, but it's not as consistent. > So, up to you if you like this or not. Done, but I think EnsureLayoutInternal should semantically return CTLineRef rather than |attributes| (and I believe we should not ignore the return value, it seems to live as long as CTLineRef lives). https://codereview.chromium.org/968923004/diff/240001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/968923004/diff/240001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2631: render_text.EnsureLayout(); On 2015/03/05 06:32:49, tapted wrote: > are these EnsureLayout calls needed? (and would that allow the friend dec to be > dropped) They can be replaced by Draw() with gfx::Canvas, but I think anyways this case needs to be a friend of RenderTextMac because this test cases verifies the glyph count for |line_|. The checking of |line_| means this test case verifies which characters will be actually drawn, and that's important because previously RenderTextMac::EnsureLayout() creates |line_| from text() instead of GetDisplayText(). I want this test case to verify this type of mistake.
https://codereview.chromium.org/968923004/diff/260001/ui/gfx/render_text_mac.cc File ui/gfx/render_text_mac.cc (right): https://codereview.chromium.org/968923004/diff/260001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:36: EnsureStringSize(); I believe this should return the size of display text, not the layout text. https://codereview.chromium.org/968923004/diff/260001/ui/gfx/render_text_mac.h File ui/gfx/render_text_mac.h (right): https://codereview.chromium.org/968923004/diff/260001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.h:77: void EnsureStringSize(); EnsureLayoutTextSize() ?
https://codereview.chromium.org/968923004/diff/240001/ui/gfx/render_text_mac.h File ui/gfx/render_text_mac.h (right): https://codereview.chromium.org/968923004/diff/240001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.h:87: void ApplyStyles(CFMutableAttributedStringRef attr_string, On 2015/03/05 18:01:38, Jun Mukai wrote: > On 2015/03/05 06:32:49, tapted wrote: > > declare this as a const function? That would be a good safety I think. Same > for > > EnsureLayoutInternal() > > Done. Oops, this and EnsureLayoutInternal() can't be const, because ApplyStyles() invokes non-const methods (ApplyCompositionAndSelectionStyles() and UndoCompositionAndSelectionStyles()). They are safely called from ApplyStyles(), and probably some another way would be better to scope the selection styles, but for now this should be non-const. https://codereview.chromium.org/968923004/diff/260001/ui/gfx/render_text_mac.cc File ui/gfx/render_text_mac.cc (right): https://codereview.chromium.org/968923004/diff/260001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:36: EnsureStringSize(); On 2015/03/05 18:14:20, oshima wrote: > I believe this should return the size of display text, not the layout text. Reverted back to the original. https://codereview.chromium.org/968923004/diff/260001/ui/gfx/render_text_mac.h File ui/gfx/render_text_mac.h (right): https://codereview.chromium.org/968923004/diff/260001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.h:77: void EnsureStringSize(); On 2015/03/05 18:14:20, oshima wrote: > EnsureLayoutTextSize() ? Renamed to GetLayoutTextSize(). This is (currently) only necessary in OnLayoutTextAttributeChanged(), and it always needs to recompute the size in that context.
https://codereview.chromium.org/968923004/diff/300001/ui/gfx/render_text_mac.cc File ui/gfx/render_text_mac.cc (right): https://codereview.chromium.org/968923004/diff/300001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:37: return Size(std::ceil(size_f.width()), size_f.height()); nit: gfx::ToCeiledSize ? https://codereview.chromium.org/968923004/diff/300001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:116: UpdateDisplayText(std::ceil(GetLayoutTextSize().width())); It's probably better to rename it to GetLayoutTextWidthF() and do ceiling in the function because this is only the caller of the function.
lgtm https://codereview.chromium.org/968923004/diff/300001/ui/gfx/render_text_mac.cc File ui/gfx/render_text_mac.cc (right): https://codereview.chromium.org/968923004/diff/300001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:116: UpdateDisplayText(std::ceil(GetLayoutTextSize().width())); On 2015/03/05 21:27:58, oshima wrote: > It's probably better to rename it to GetLayoutTextWidthF() and do ceiling in the > function because this is only the caller of the function. The postfix 'F' implies that it returns a floating point value, so the width shouldn't be ceil'ed with that function name, right? https://codereview.chromium.org/968923004/diff/300001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:256: end = std::min(style.GetRange().end(), layout_text_length); nit q: should this use something like TextIndexToGivenTextIndex (promoted from RTHB to RT)? (no worries if that's a significant change for little benefit and this is sufficient) https://codereview.chromium.org/968923004/diff/300001/ui/gfx/render_text_mac.h File ui/gfx/render_text_mac.h (right): https://codereview.chromium.org/968923004/diff/300001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.h:77: gfx::SizeF GetLayoutTextSize(); I believe this function would be useful on the base class, gfx::RenderText, so users like views::Label can determine their preferred size without changing the display rect bounds of the RenderText... but that's not critical here.
https://codereview.chromium.org/968923004/diff/300001/ui/gfx/render_text_mac.cc File ui/gfx/render_text_mac.cc (right): https://codereview.chromium.org/968923004/diff/300001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:116: UpdateDisplayText(std::ceil(GetLayoutTextSize().width())); On 2015/03/05 21:55:29, msw wrote: > On 2015/03/05 21:27:58, oshima wrote: > > It's probably better to rename it to GetLayoutTextWidthF() and do ceiling in > the > > function because this is only the caller of the function. > > The postfix 'F' implies that it returns a floating point value, so the width > shouldn't be ceil'ed with that function name, right? Oops, yes. Just GetLayoutTextWidth.
https://codereview.chromium.org/968923004/diff/300001/ui/gfx/render_text_mac.cc File ui/gfx/render_text_mac.cc (right): https://codereview.chromium.org/968923004/diff/300001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:37: return Size(std::ceil(size_f.width()), size_f.height()); On 2015/03/05 21:27:58, oshima wrote: > nit: gfx::ToCeiledSize ? Done. https://codereview.chromium.org/968923004/diff/300001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:116: UpdateDisplayText(std::ceil(GetLayoutTextSize().width())); On 2015/03/05 23:09:11, oshima wrote: > On 2015/03/05 21:55:29, msw wrote: > > On 2015/03/05 21:27:58, oshima wrote: > > > It's probably better to rename it to GetLayoutTextWidthF() and do ceiling in > > the > > > function because this is only the caller of the function. > > > > The postfix 'F' implies that it returns a floating point value, so the width > > shouldn't be ceil'ed with that function name, right? > > Oops, yes. Just GetLayoutTextWidth. Done. https://codereview.chromium.org/968923004/diff/300001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:256: end = std::min(style.GetRange().end(), layout_text_length); On 2015/03/05 21:55:29, msw wrote: > nit q: should this use something like TextIndexToGivenTextIndex (promoted from > RTHB to RT)? (no worries if that's a significant change for little benefit and > this is sufficient) good idea. done. https://codereview.chromium.org/968923004/diff/300001/ui/gfx/render_text_mac.h File ui/gfx/render_text_mac.h (right): https://codereview.chromium.org/968923004/diff/300001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.h:77: gfx::SizeF GetLayoutTextSize(); On 2015/03/05 21:55:29, msw wrote: > I believe this function would be useful on the base class, gfx::RenderText, so > users like views::Label can determine their preferred size without changing the > display rect bounds of the RenderText... but that's not critical here. Not sure how beneficial is it, RTHB updates display text different ways and it doesn't need a method like this.
The CQ bit was checked by mukai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/968923004/#ps320001 (title: "comments addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/968923004/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by mukai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, tapted@chromium.org Link to the patchset: https://codereview.chromium.org/968923004/#ps340001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/968923004/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/968923004/diff/340001/ui/gfx/render_text_mac.h File ui/gfx/render_text_mac.h (right): https://codereview.chromium.org/968923004/diff/340001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.h:23: class RenderTextMac : public RenderText { I think you're going to need GFX_EXPORT here
The CQ bit was checked by mukai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, tapted@chromium.org Link to the patchset: https://codereview.chromium.org/968923004/#ps360001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/968923004/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mukai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, tapted@chromium.org Link to the patchset: https://codereview.chromium.org/968923004/revert#ps380001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/968923004/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) Timed out jobs are not retried to avoid causing additional load on the builders with large pending queues.
The CQ bit was checked by mukai@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/968923004/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) Timed out jobs are not retried to avoid causing additional load on the builders with large pending queues.
The CQ bit was checked by mukai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, tapted@chromium.org Link to the patchset: https://codereview.chromium.org/968923004/#ps400001 (title: "rebase2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/968923004/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) Timed out jobs are not retried to avoid causing additional load on the builders with large pending queues.
The CQ bit was checked by mukai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, tapted@chromium.org Link to the patchset: https://codereview.chromium.org/968923004/#ps420001 (title: "fix failed tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/968923004/420001
https://codereview.chromium.org/968923004/diff/420001/ui/gfx/render_text_mac.cc File ui/gfx/render_text_mac.cc (right): https://codereview.chromium.org/968923004/diff/420001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:37: return Size(std::ceil(size_f.width(), size_f.height())); I think your parenthesis here are wrong.
https://codereview.chromium.org/968923004/diff/420001/ui/gfx/render_text_mac.cc File ui/gfx/render_text_mac.cc (right): https://codereview.chromium.org/968923004/diff/420001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:37: return Size(std::ceil(size_f.width(), size_f.height())); On 2015/03/09 17:22:10, msw wrote: > I think your parenthesis here are wrong. oops, thanks...
The CQ bit was checked by mukai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, tapted@chromium.org Link to the patchset: https://codereview.chromium.org/968923004/#ps440001 (title: "fix typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/968923004/440001
Message was sent while issue was closed.
Committed patchset #23 (id:440001)
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/fea1a11f41748a6d2281b1017d70f618bf7b1354 Cr-Commit-Position: refs/heads/master@{#319742} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
