Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
867003002:
Cache gfx::RenderText instances in views::Label. (Closed)
|
Created:
5 years, 11 months ago by Jun Mukai Modified:
5 years, 9 months ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, tfarina, ckocagil, Alexei Svitkine (slow), Andre Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCache gfx::RenderText instances in views::Label.
This is a rebase of crrev.com/413823002 by msw@.
Cache RenderText instances, avoid Canvas::DrawString*.
(avoids repeating itemization, layout, etc. on each paint)
Recalculate colors and reset the layout as needed.
Large cleanup; remove obsolete flag tests; update tests.
Update App List's CachedLabel views::Label subclass use.
(SchedulePaintInRect triggered paint with color changes)
Ensure SetTitleSubpixelAA is otherwise called as needed.
(skip early return; which breaks folder reorganization)
This will increase the performance of painting
significantly. See https://docs.google.com/document/d/1q4RrBjNO52l1pNTkIZPhQ60aPfBZnV4Af0vdQNr2Juc/edit#
for the detailed analysis.
BUG=240037, 125348, 450791
TEST=no appearance changes with performance improvement as:
On daisy with 15 bookmarks, repainting of bookmark is
decreased as 135msec -> 2.8msec
R=sky@chromium.org
Committed: https://crrev.com/1dfc595ffc753b6b1c99cca58e676e8b3e1bf6d8
Cr-Commit-Position: refs/heads/master@{#320138}
Patch Set 1 #Patch Set 2 : minor polishments, recovering removed DCHECKs and comments #Patch Set 3 : background fix #Patch Set 4 : early exit for Set... methods #
Total comments: 2
Patch Set 5 : rebase #
Total comments: 3
Patch Set 6 : do not overuse the memory #
Total comments: 27
Patch Set 7 : rebase #Patch Set 8 : rebase & fix bugs #Patch Set 9 : multiple fixes #Patch Set 10 : fix overuse of render_text_ field #Patch Set 11 : polish #Patch Set 12 : fix #Patch Set 13 : fix #Patch Set 14 : rebase #
Total comments: 1
Patch Set 15 : rebase / exclude background_is_tranparent #Patch Set 16 : comments addressed #Patch Set 17 : cleanup / re-upload #
Total comments: 61
Patch Set 18 : comments addressed #
Total comments: 24
Patch Set 19 : comments addressed #
Total comments: 4
Patch Set 20 : fix #
Total comments: 2
Patch Set 21 : multiline & replace newline chars #Patch Set 22 : multi_line_ init #
Total comments: 2
Patch Set 23 : elide test #Patch Set 24 : rebase #Messages
Total messages: 34 (4 generated)
oshima@chromium.org changed reviewers: + oshima@chromium.org
https://codereview.chromium.org/867003002/diff/60001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/867003002/diff/60001/ui/views/controls/label.... ui/views/controls/label.cc:396: UpdateColorsFromTheme(ui::NativeTheme::instance()); FYI: handles_tooltips_ is missing
https://codereview.chromium.org/867003002/diff/60001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/867003002/diff/60001/ui/views/controls/label.... ui/views/controls/label.cc:396: UpdateColorsFromTheme(ui::NativeTheme::instance()); On 2015/01/23 16:58:54, oshima wrote: > FYI: handles_tooltips_ is missing Done.
This comes at the cost of making Label way more expensive in memory, right? At one point the bookmarkbar created a button for every bookmark on your bookmarkbar. So, if you had > 5000 bookmarks caching the RenderText would likely have huge consequences. I don't know if there are other places in chromes ui where we might create a ton of buttons, but I wouldn't be surprised if there are. I think it would be better to only cache on a first paint and the label is visible. Similarly the cache should be dumped if the label isn't visible. msw is back next week and he has more context here. Can you wait for him to get back to do the review?
On 2015/01/27 16:00:10, sky wrote: > This comes at the cost of making Label way more expensive in memory, right? > At one point the bookmarkbar created a button for every bookmark on your > bookmarkbar. So, if you had > 5000 bookmarks caching the RenderText would likely > have huge consequences. I don't know if there are other places in chromes ui > where we might create a ton of buttons, but I wouldn't be surprised if there > are. > I think it would be better to only cache on a first paint and the label is > visible. Similarly the cache should be dumped if the label isn't visible. > msw is back next week and he has more context here. Can you wait for him to get > back to do the review? Sure, I can wait. As I roughly checked the memory consumption with the task manager, the difference was not very noticeable. I'll double-check the number and report here (and https://docs.google.com/document/d/1q4RrBjNO52l1pNTkIZPhQ60aPfBZnV4Af0vdQNr2J...)
I suspect it's more of weird cases that would turn up unusual results. Like I said, you would have only noticed the previous problem if you had a ton of bookmarks. I still stand by caching only while visible though. -Scott On Tue, Jan 27, 2015 at 9:51 AM, <mukai@chromium.org> wrote: > On 2015/01/27 16:00:10, sky wrote: > >> This comes at the cost of making Label way more expensive in memory, >> right? >> At one point the bookmarkbar created a button for every bookmark on your >> bookmarkbar. So, if you had > 5000 bookmarks caching the RenderText would >> > likely > >> have huge consequences. I don't know if there are other places in chromes >> ui >> where we might create a ton of buttons, but I wouldn't be surprised if >> there >> are. >> I think it would be better to only cache on a first paint and the label is >> visible. Similarly the cache should be dumped if the label isn't visible. >> msw is back next week and he has more context here. Can you wait for him >> to >> > get > >> back to do the review? >> > > Sure, I can wait. > > As I roughly checked the memory consumption with the task manager, > the difference was not very noticeable. I'll double-check the number > and report here (and > https://docs.google.com/document/d/1q4RrBjNO52l1pNTkIZPhQ60aPfBZn > V4Af0vdQNr2Juc/edit#heading=h.ks70luwp2vlc) > > https://codereview.chromium.org/867003002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
msw@chromium.org changed reviewers: + msw@chromium.org
On 2015/01/27 17:52:25, sky wrote: > I still stand by caching only while visible though. I'm not sure that's entirely reasonable. The |render_text_| instance may be needed regardless of visibility for the measurement of some text, and it's definitely needed for storage of label parameters (fonts, colors, elide behavior, etc.). It *might* be okay to ignore |lines_| and calls to layout and paint if the view is not visible... https://codereview.chromium.org/867003002/diff/80001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/867003002/diff/80001/ui/views/controls/label.... ui/views/controls/label.cc:461: SkColorGetA(background_color_) != 0xFF || !subpixel_rendering_enabled_; Are you sure this change from && to || from patch set 1 is correct? IIRC, subpixel rendering and transparent backgrounds can not both be supported simultaneously; this check previously disabled background transparency when subpixel rendering was enabled.
On 2015/02/02 22:31:17, msw wrote: > On 2015/01/27 17:52:25, sky wrote: > > I still stand by caching only while visible though. > > I'm not sure that's entirely reasonable. The |render_text_| instance may be > needed regardless of visibility for the measurement of some text, and it's > definitely needed for storage of label parameters (fonts, colors, elide > behavior, etc.). It *might* be okay to ignore |lines_| and calls to layout and > paint if the view is not visible... > render_text_ instance has some internal data structures for layout text. Therefore, it might be reasonable to release those data (i.e. TextRunHarfBuzz instances and grapheme_iterator_ in RenderTextHarfBuzz) when it's not visible. This would reduce substantial amount of memory if it's actually effective, and it seems tab text are usually hidden if the user makes lots of tabs, so it would be meaningful. I am currently dealing with multi-lined RenderTextHarfBuzz btw. It seems one RTHB instance per line will consume much memory than we think. > https://codereview.chromium.org/867003002/diff/80001/ui/views/controls/label.cc > File ui/views/controls/label.cc (right): > > https://codereview.chromium.org/867003002/diff/80001/ui/views/controls/label.... > ui/views/controls/label.cc:461: SkColorGetA(background_color_) != 0xFF || > !subpixel_rendering_enabled_; > Are you sure this change from && to || from patch set 1 is correct? IIRC, > subpixel rendering and transparent backgrounds can not both be supported > simultaneously; this check previously disabled background transparency when > subpixel rendering was enabled.
On 2015/02/02 22:59:13, Jun Mukai wrote: > On 2015/02/02 22:31:17, msw wrote: > > On 2015/01/27 17:52:25, sky wrote: > > > I still stand by caching only while visible though. > > > > I'm not sure that's entirely reasonable. The |render_text_| instance may be > > needed regardless of visibility for the measurement of some text, and it's > > definitely needed for storage of label parameters (fonts, colors, elide > > behavior, etc.). It *might* be okay to ignore |lines_| and calls to layout and > > paint if the view is not visible... > > > > render_text_ instance has some internal data structures for layout text. > Therefore, it might be reasonable to release those data (i.e. TextRunHarfBuzz > instances and grapheme_iterator_ in RenderTextHarfBuzz) when it's not visible. > This would reduce substantial amount of memory if it's actually effective, > and it seems tab text are usually hidden if the user makes lots of tabs, > so it would be meaningful. Ah, that might be worthwhile. > I am currently dealing with multi-lined RenderTextHarfBuzz btw. It seems > one RTHB instance per line will consume much memory than we think. Your next best bet might be implementing multi-line within RTHB. Or, perhaps just call canvas->DrawStringRectWithShadows for multi-line?
On Mon, Feb 2, 2015 at 2:31 PM, <msw@chromium.org> wrote: > On 2015/01/27 17:52:25, sky wrote: > >> I still stand by caching only while visible though. >> > > I'm not sure that's entirely reasonable. The |render_text_| instance may be > needed regardless of visibility for the measurement of some text, and it's > definitely needed for storage of label parameters (fonts, colors, elide > behavior, etc.). It *might* be okay to ignore |lines_| and calls to layout > and > paint if the view is not visible... > My concern is the possibility of making view::Label a lot heavier in memory footprint. I'm ok with visible labels taking up more memory since presumably you're using them. If a RenderText is needed for sizing calculations one can still be created, but destroyed immediately after the sizing information is obtained. -Scott > > > https://codereview.chromium.org/867003002/diff/80001/ui/ > views/controls/label.cc > File ui/views/controls/label.cc (right): > > https://codereview.chromium.org/867003002/diff/80001/ui/ > views/controls/label.cc#newcode461 > ui/views/controls/label.cc:461: SkColorGetA(background_color_) != 0xFF > || !subpixel_rendering_enabled_; > Are you sure this change from && to || from patch set 1 is correct? > IIRC, subpixel rendering and transparent backgrounds can not both be > supported simultaneously; this check previously disabled background > transparency when subpixel rendering was enabled. > > https://codereview.chromium.org/867003002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/02/03 00:41:40, sky wrote: > On Mon, Feb 2, 2015 at 2:31 PM, <mailto:msw@chromium.org> wrote: > > > On 2015/01/27 17:52:25, sky wrote: > > > >> I still stand by caching only while visible though. > >> > > > > I'm not sure that's entirely reasonable. The |render_text_| instance may be > > needed regardless of visibility for the measurement of some text, and it's > > definitely needed for storage of label parameters (fonts, colors, elide > > behavior, etc.). It *might* be okay to ignore |lines_| and calls to layout > > and > > paint if the view is not visible... > > > > My concern is the possibility of making view::Label a lot heavier in memory > footprint. I'm ok with visible labels taking up more memory since > presumably you're using them. If a RenderText is needed for sizing > calculations one can still be created, but destroyed immediately after the > sizing information is obtained. > > -Scott > > > > > > > > https://codereview.chromium.org/867003002/diff/80001/ui/ > > views/controls/label.cc > > File ui/views/controls/label.cc (right): > > > > https://codereview.chromium.org/867003002/diff/80001/ui/ > > views/controls/label.cc#newcode461 > > ui/views/controls/label.cc:461: SkColorGetA(background_color_) != 0xFF > > || !subpixel_rendering_enabled_; > > Are you sure this change from && to || from patch set 1 is correct? > > IIRC, subpixel rendering and transparent backgrounds can not both be > > supported simultaneously; this check previously disabled background > > transparency when subpixel rendering was enabled. > > > > https://codereview.chromium.org/867003002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Uploaded a new patchset, which has two optimizations: - do not use |lines_| but use |render_text_| if it's okay (!multi_line_ or the render text supports multi lines) - release render_text_ when it gets invisible. PTAL
https://codereview.chromium.org/867003002/diff/80001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/867003002/diff/80001/ui/views/controls/label.... ui/views/controls/label.cc:461: SkColorGetA(background_color_) != 0xFF || !subpixel_rendering_enabled_; On 2015/02/02 22:31:17, msw wrote: > Are you sure this change from && to || from patch set 1 is correct? IIRC, > subpixel rendering and transparent backgrounds can not both be supported > simultaneously; this check previously disabled background transparency when > subpixel rendering was enabled. Ping! https://codereview.chromium.org/867003002/diff/100001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/867003002/diff/100001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:424: // TODO(mukai): turn on for HarfBuzz when https://crrev.com/882643005/ is Shouldn't this be non-static for RTHB=>true and RTMac=>false? https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... ui/views/controls/label.cc:214: return render_text_ ? render_text_->layout_text() : render_data_->text; I don't think that returning render_data_->text is correct here. What if it's supposed to be obscured? What if it's supposed to be elided or truncated? I think it might be more appropriate either (1) actually recreate the RenderText object and have it perform layout, or (2) return an empty string here, so that any test users will know that they've hit a scenario with undefined behavior (requesting layout text with no layout data calculated). https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... ui/views/controls/label.cc:279: if (gfx::RenderText::MultilineSupported()) { It's a bit worrisome to add code for a case that's not yet supported (and assume that it'll work). Have you flipped the MultilineSupported bit and tested this with your WIP patch applied for RTHB? https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... ui/views/controls/label.cc:295: if (rect.width() == 0 || rect.height() == 0 || !render_text_) Shouldn't this instead return early if !visible(), and otherwise recreate the RenderText? I guess that technically depends on the call order of VisibilityChanged() and Layout(), so perhaps this is sufficient. https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... ui/views/controls/label.cc:314: render_text_->SetDisplayRect(rect); I designed this patch with the idea of reserving |render_text_| as an un-elided single-line representation of the text, as I thought was necessary for calculating the text's preferred size, (etc.) and to avoid altering the display rect (etc.) and forcing additional layout cycles to calculate the text size, etc. I worry that (1) this change of behavior has performance consequences that you'll need to evaluate, and (2) functions like Label::GetTextSize will use the Label's current display rect, elide behavior, multi-line setting, etc. and affect the result of the call to render_text_->GetStringSize(). This might cause weird behavior around re-sizing multi-line or elided labels to larger (preferred) sizes (after layout using a smaller size), and other quirks... https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... ui/views/controls/label.cc:437: render_text_ = render_data_->CreateRenderText(); Flipping visibility at different states of initialization should be tested, I worry there's an edge case here where neither |render_text_| nor |render_data_| exist that will crash when accessed. https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... ui/views/controls/label.h:89: bool obscured() const { return render_text_->obscured(); } Shouldn't this fall back on RenderData? https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... ui/views/controls/label.h:155: struct RenderData { I don't think this is the best way to address the memory issue, but it *might* be okay for now. Instead, could RenderText::ResetLayout() free its layout data and expose that function publicly for competent users? That might have adverse performance effects if it's called too frequently before the layout *is* actually needed, but it seems like a balancing act that some classes, such as views::Label, might be able to perform correctly. https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... ui/views/controls/label.h:160: RenderData(const gfx::RenderText& render_text); Functions should be listed before member variables. https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... ui/views/controls/label.h:188: // This field is for memory optimization: render_text_ is memory-heavy because nit: |render_text_| to match below, or RenderText. https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... ui/views/controls/label.h:189: // it holds several data for layouts. Therefore in case it's not visible, use nit: s/several data for layouts/layout data/ also: s/Therefore in case it's not visible/When the label is not visible/ https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... ui/views/controls/label_unittest.cc:98: TEST_F(LabelTest, MultiLineProperty) { Please add visibility tests and tests re-sizing labels of various elide behaviors, multi-line states, etc. as my comments expressed concerns about.
Still working in progress, but I've made several changes so I want to upload the latest patchset which addresses some of the review comments. thanks for insightful comments. https://codereview.chromium.org/867003002/diff/80001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/867003002/diff/80001/ui/views/controls/label.... ui/views/controls/label.cc:461: SkColorGetA(background_color_) != 0xFF || !subpixel_rendering_enabled_; On 2015/02/05 20:16:41, msw wrote: > On 2015/02/02 22:31:17, msw wrote: > > Are you sure this change from && to || from patch set 1 is correct? IIRC, > > subpixel rendering and transparent backgrounds can not both be supported > > simultaneously; this check previously disabled background transparency when > > subpixel rendering was enabled. > > Ping! I was confused and I think this is really confusing, but now I'm sure this || is correct (at least, app list item label looks blurry otherwise). It's so confusing because 'background_is_transparent' flag does not explain what this means. Actually, we want to suppress subpixel rendering if the background is transparent (https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/render_text...). So if subpixel_rendering_enabled_ is false, it should always suppress subpixel rendering, therefore background_is_transparent must be true regardless of the actual background color. So I think the logic here is still correct but the field name 'background_is_transparent' isn't great naming. I've renamed as 'subpixel_rendering_enabled' instead. https://codereview.chromium.org/867003002/diff/100001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/867003002/diff/100001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:424: // TODO(mukai): turn on for HarfBuzz when https://crrev.com/882643005/ is On 2015/02/05 20:16:41, msw wrote: > Shouldn't this be non-static for RTHB=>true and RTMac=>false? Done. https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... ui/views/controls/label.cc:279: if (gfx::RenderText::MultilineSupported()) { On 2015/02/05 20:16:41, msw wrote: > It's a bit worrisome to add code for a case that's not yet supported (and assume > that it'll work). Have you flipped the MultilineSupported bit and tested this > with your WIP patch applied for RTHB? Yes, I've tested (and now it's in). https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... ui/views/controls/label.cc:314: render_text_->SetDisplayRect(rect); On 2015/02/05 20:16:41, msw wrote: > I designed this patch with the idea of reserving |render_text_| as an un-elided > single-line representation of the text, as I thought was necessary for > calculating the text's preferred size, (etc.) and to avoid altering the display > rect (etc.) and forcing additional layout cycles to calculate the text size, > etc. > > I worry that (1) this change of behavior has performance consequences that > you'll need to evaluate, and (2) functions like Label::GetTextSize will use the > Label's current display rect, elide behavior, multi-line setting, etc. and > affect the result of the call to render_text_->GetStringSize(). This might cause > weird behavior around re-sizing multi-line or elided labels to larger > (preferred) sizes (after layout using a smaller size), and other quirks... I came to realize your point finally, thank you for pointing! It seems oshima is making another changes on render text, then we can handle around display rects and eliding independently. I feel like I should wait for his change, then things will be much clearer. https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... ui/views/controls/label.cc:314: render_text_->SetDisplayRect(rect); On 2015/02/05 20:16:41, msw wrote: > I designed this patch with the idea of reserving |render_text_| as an un-elided > single-line representation of the text, as I thought was necessary for > calculating the text's preferred size, (etc.) and to avoid altering the display > rect (etc.) and forcing additional layout cycles to calculate the text size, > etc. > > I worry that (1) this change of behavior has performance consequences that > you'll need to evaluate, and (2) functions like Label::GetTextSize will use the > Label's current display rect, elide behavior, multi-line setting, etc. and > affect the result of the call to render_text_->GetStringSize(). This might cause > weird behavior around re-sizing multi-line or elided labels to larger > (preferred) sizes (after layout using a smaller size), and other quirks... Your concern is right and hits some UI (I didn't notice it). Made some changes but still a bit incorrect. I am thinking to wait for oshima's change and then see how it can fix the issue, but if not, I will revert this CL. https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... ui/views/controls/label.cc:437: render_text_ = render_data_->CreateRenderText(); On 2015/02/05 20:16:41, msw wrote: > Flipping visibility at different states of initialization should be tested, I > worry there's an edge case here where neither |render_text_| nor |render_data_| > exist that will crash when accessed. Test added. https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... ui/views/controls/label.h:89: bool obscured() const { return render_text_->obscured(); } On 2015/02/05 20:16:42, msw wrote: > Shouldn't this fall back on RenderData? Right, thanks for pointing (I've removed render_data_ so it's okay now) https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... ui/views/controls/label.h:155: struct RenderData { On 2015/02/05 20:16:42, msw wrote: > I don't think this is the best way to address the memory issue, but it *might* > be okay for now. Instead, could RenderText::ResetLayout() free its layout data > and expose that function publicly for competent users? That might have adverse > performance effects if it's called too frequently before the layout *is* > actually needed, but it seems like a balancing act that some classes, such as > views::Label, might be able to perform correctly. Done, I've realized your point really after I put a few more field into this object. https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... ui/views/controls/label_unittest.cc:98: TEST_F(LabelTest, MultiLineProperty) { On 2015/02/05 20:16:42, msw wrote: > Please add visibility tests and tests re-sizing labels of various elide > behaviors, multi-line states, etc. as my comments expressed concerns about. Ack, will do later.
Split subpixel changes off to a separate CL to land first. https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... ui/views/controls/label.cc:314: render_text_->SetDisplayRect(rect); On 2015/02/12 21:43:04, Jun Mukai wrote: > On 2015/02/05 20:16:41, msw wrote: > > I designed this patch with the idea of reserving |render_text_| as an > un-elided > > single-line representation of the text, as I thought was necessary for > > calculating the text's preferred size, (etc.) and to avoid altering the > display > > rect (etc.) and forcing additional layout cycles to calculate the text size, > > etc. > > > > I worry that (1) this change of behavior has performance consequences that > > you'll need to evaluate, and (2) functions like Label::GetTextSize will use > the > > Label's current display rect, elide behavior, multi-line setting, etc. and > > affect the result of the call to render_text_->GetStringSize(). This might > cause > > weird behavior around re-sizing multi-line or elided labels to larger > > (preferred) sizes (after layout using a smaller size), and other quirks... > > Your concern is right and hits some UI (I didn't notice it). Made some changes > but still a bit incorrect. > I am thinking to wait for oshima's change and then see how it can fix the issue, > but if not, I will revert this CL. I think oshima's change will address my performance concerns, but you might need to be careful with the meanings of RenderText::GetStringSize[F] and RenderText::GetContentWidth[F]. Both of these appear to return the wrapped width in multi-line mode, so for preferred Label sizing, you'll probably need to either: 1) Disable RenderText multi-line to get the single-line size. Or: 2) Repurpose or add a RenderText function to get the size of the single-line layout text without eliding. https://codereview.chromium.org/867003002/diff/260001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/867003002/diff/260001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:740: // Is the background transparent (either partially or fully)? nit: update comment.
PTAL https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... ui/views/controls/label.cc:214: return render_text_ ? render_text_->layout_text() : render_data_->text; On 2015/02/05 20:16:41, msw wrote: > I don't think that returning render_data_->text is correct here. What if it's > supposed to be obscured? What if it's supposed to be elided or truncated? I > think it might be more appropriate either (1) actually recreate the RenderText > object and have it perform layout, or (2) return an empty string here, so that > any test users will know that they've hit a scenario with undefined behavior > (requesting layout text with no layout data calculated). Done -- Label is reshaped and |render_data_| is gone. https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... ui/views/controls/label.cc:295: if (rect.width() == 0 || rect.height() == 0 || !render_text_) On 2015/02/05 20:16:42, msw wrote: > Shouldn't this instead return early if !visible(), and otherwise recreate the > RenderText? I guess that technically depends on the call order of > VisibilityChanged() and Layout(), so perhaps this is sufficient. Layout() now gets much simpler, so this early-quit isn't necessary anymore, removed. https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... ui/views/controls/label.cc:314: render_text_->SetDisplayRect(rect); On 2015/02/17 22:16:45, msw wrote: > On 2015/02/12 21:43:04, Jun Mukai wrote: > > On 2015/02/05 20:16:41, msw wrote: > > > I designed this patch with the idea of reserving |render_text_| as an > > un-elided > > > single-line representation of the text, as I thought was necessary for > > > calculating the text's preferred size, (etc.) and to avoid altering the > > display > > > rect (etc.) and forcing additional layout cycles to calculate the text size, > > > etc. > > > > > > I worry that (1) this change of behavior has performance consequences that > > > you'll need to evaluate, and (2) functions like Label::GetTextSize will use > > the > > > Label's current display rect, elide behavior, multi-line setting, etc. and > > > affect the result of the call to render_text_->GetStringSize(). This might > > cause > > > weird behavior around re-sizing multi-line or elided labels to larger > > > (preferred) sizes (after layout using a smaller size), and other quirks... > > > > Your concern is right and hits some UI (I didn't notice it). Made some changes > > but still a bit incorrect. > > I am thinking to wait for oshima's change and then see how it can fix the > issue, > > but if not, I will revert this CL. > > I think oshima's change will address my performance concerns, but you might need > to be careful with the meanings of RenderText::GetStringSize[F] and > RenderText::GetContentWidth[F]. Both of these appear to return the wrapped width > in multi-line mode, so for preferred Label sizing, you'll probably need to > either: > > 1) Disable RenderText multi-line to get the single-line size. Or: > 2) Repurpose or add a RenderText function to get the size of the single-line > layout text without eliding. Now my CL is reformed (which is similar to your original one) as: - |render_text_|: no eliding, used for size calculation - |lines_|: for drawing The biggest difference is that |lines_| is created for the first time PaintText() is invoked, not in Layout() anymore. It seems Layout() might be called multiple time for the initial layouting, so this approach would be better. https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... ui/views/controls/label.h:160: RenderData(const gfx::RenderText& render_text); On 2015/02/05 20:16:42, msw wrote: > Functions should be listed before member variables. the struct itself was removed. https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... ui/views/controls/label.h:188: // This field is for memory optimization: render_text_ is memory-heavy because On 2015/02/05 20:16:42, msw wrote: > nit: |render_text_| to match below, or RenderText. comment was removed. https://codereview.chromium.org/867003002/diff/100001/ui/views/controls/label... ui/views/controls/label.h:189: // it holds several data for layouts. Therefore in case it's not visible, use On 2015/02/05 20:16:42, msw wrote: > nit: s/several data for layouts/layout data/ > also: s/Therefore in case it's not visible/When the label is not visible/ comment was removed.
https://codereview.chromium.org/867003002/diff/320001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/867003002/diff/320001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:1099: #if defined(OS_WIN) Change the #ifs in this function to check MultilineSupported() instead. (move this DCHECK_LT down into the block below) https://codereview.chromium.org/867003002/diff/320001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/867003002/diff/320001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:259: // Returns true if this supports multiline rendering. nit: "this instance" https://codereview.chromium.org/867003002/diff/320001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/867003002/diff/320001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:139: bool MultilineSupported() const override; Reorder this (and the definition) above GetDisplayText to match the base class. https://codereview.chromium.org/867003002/diff/320001/ui/gfx/render_text_mac.h File ui/gfx/render_text_mac.h (right): https://codereview.chromium.org/867003002/diff/320001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.h:31: bool MultilineSupported() const override; Reorder this (and the definition) above GetDisplayText to match the base class. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:101: void Label::SetSubpixelRenderingEnabled(bool subpixel_rendering_enabled) { nit: should this be renamed to match the RenderText setting? https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:132: DCHECK(!multi_line || (elide_behavior_ == gfx::ELIDE_TAIL || Please confirm that multi-line RenderTextHarfBuzz supports ELIDE_TAIL, or make that fallback to using multiple |lines_|... I think Oshima removed that support. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:182: const base::string16& Label::GetLayoutTextForTesting() const { nit: rename this GetDisplayTextForTesting https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:205: // collapse_when_hidden_ flag is set. nit: use vertical bars for identifiers: |collapse_when_hidden_|, ditto below https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:208: // need ot respect the collapse_when_hidden_ flag. nit: s/ot/to https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:244: if (!multi_line() || text().empty() || w <= 0 || !render_text_) nit: does this still need to check for a non-null render_text_? https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:251: render_text_->SetDisplayRect(gfx::Rect(0, 0, w, 0)); Will this have a persistent effect on the |render_text_|? So calling Label::GetHeightForWidth() (ie. with a small width) could change the result of the next internal Label call to GetDisplayText? Can you audit this and add a test case to your unit test? https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:285: state->name = render_text_->GetDisplayText(); I wonder if this should use the un-elided (but still possibly obscured) layout text, instead of the elided display text. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:298: tooltip->assign(text()); I wonder if this should use the un-elided (but still possibly obscured) layout text, instead of the un-obscured text (ie. showing the password as a tooltip when the actual label shows the obscured chars as '*'). https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:442: for (size_t i = lines_.size(); i < lines.size(); ++i) Can you add a comment here like "Append the remaining text to the last visible line."... After all the time since I wrote this, I forgot what it was doing. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:459: if (text().empty() || !render_text_) { nit: does this still need to check for a null render_text_? https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:463: render_text_->SetDisplayRect(gfx::Rect()); Hmm, is this needed and does it actually cause RenderText::GetStringSize to return the value you want? I wonder if it would be helpful to add a separate RenderText function that returns the natural single-line size of the obscured layout text. It shouldn't be hard or expensive to keep that value up to date in RenderText. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:468: base::SplitString(text(), '\n', &lines); Shouldn't this be using the RenderText's layout text to respect obscuring, but ignore the current eliding? https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:475: size.set_height(size.height() + line.height()); Shouldn't this respect the minimum line_height()? https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:484: if (!render_text_) nit: still necessary? https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.h:145: void VisibilityChanged(View* starting_from, bool is_visible) override; nit: reorder to match View decl order (after OnBoundsChanged, i think) https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.h:155: // Create a single RenderText instance to be painted actually. nit: "to actually be painted." (ditto below for MaybeBuildRenderTextLines) https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label_unittest.cc:377: label.SetElideBehavior(gfx::ELIDE_TAIL); You do this just a few lines above, is there any reason to set it twice? https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label_unittest.cc:386: EXPECT_EQ(narrow_size.ToString(), label.GetPreferredSize().ToString()); nit: I guess check EXPECT_EQ(text, label.GetLayoutTextForTesting()); after this to be consistent with the checks below, but I'm really not sure why it's worth checking here... https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label_unittest.cc:388: // SetBounds() as well. nit: copy the comment for consistency: "doesn't change the preferred size." https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label_unittest.cc:393: // Paint() nit: copy the comment for consistency: "doesn't change the preferred size." https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label_unittest.cc:458: // Check that label can release its internal layout data when it gets nit: s/can release/releases/ and s/it gets/it's/ https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label_unittest.cc:502: scoped_ptr<gfx::RenderText> render_text(gfx::RenderText::CreateInstance()); Maybe exclude this test from Mac and ASSERT that the RenderText instance supports multi-line, then construct a similar test for Mac, and ASSERT that multi-line isn't supported, and then ensure that the label creates a RenderText instance for each line on Mac... https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label_unittest.cc:514: // The number of lines is just 1, because the RenderText object can handle nit: " // There's only one 'line', RenderText itself supports multiple lines."
PTAL https://codereview.chromium.org/867003002/diff/320001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/867003002/diff/320001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:1099: #if defined(OS_WIN) On 2015/02/20 01:55:53, msw wrote: > Change the #ifs in this function to check MultilineSupported() instead. > (move this DCHECK_LT down into the block below) Done. https://codereview.chromium.org/867003002/diff/320001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/867003002/diff/320001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:259: // Returns true if this supports multiline rendering. On 2015/02/20 01:55:53, msw wrote: > nit: "this instance" Done. https://codereview.chromium.org/867003002/diff/320001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/867003002/diff/320001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:139: bool MultilineSupported() const override; On 2015/02/20 01:55:53, msw wrote: > Reorder this (and the definition) above GetDisplayText to match the base class. Done. https://codereview.chromium.org/867003002/diff/320001/ui/gfx/render_text_mac.h File ui/gfx/render_text_mac.h (right): https://codereview.chromium.org/867003002/diff/320001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.h:31: bool MultilineSupported() const override; On 2015/02/20 01:55:53, msw wrote: > Reorder this (and the definition) above GetDisplayText to match the base class. Done. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:101: void Label::SetSubpixelRenderingEnabled(bool subpixel_rendering_enabled) { On 2015/02/20 01:55:54, msw wrote: > nit: should this be renamed to match the RenderText setting? Let me do that in another CL. It'll complicate this CL. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:132: DCHECK(!multi_line || (elide_behavior_ == gfx::ELIDE_TAIL || On 2015/02/20 01:55:54, msw wrote: > Please confirm that multi-line RenderTextHarfBuzz supports ELIDE_TAIL, or make > that fallback to using multiple |lines_|... I think Oshima removed that support. Actually we do not support eliding for multilined labels right now. views::Label (before this patch) simply cut off very long lines. # This means I made changes around CreateRenderText to set elide_behavior. elide_behavior_ == gfx::ELIDE_TAIL is allowed here simply because that's the default value and we don't want to cause check failure for the code like this: views::Label* label = new views::Label(text); label->SetMultiLine(true); // elide_behavior is not set label->SetElideBehavior(NO_ELIDE); : https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:182: const base::string16& Label::GetLayoutTextForTesting() const { On 2015/02/20 01:55:55, msw wrote: > nit: rename this GetDisplayTextForTesting Done. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:205: // collapse_when_hidden_ flag is set. On 2015/02/20 01:55:54, msw wrote: > nit: use vertical bars for identifiers: |collapse_when_hidden_|, ditto below Done. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:208: // need ot respect the collapse_when_hidden_ flag. On 2015/02/20 01:55:54, msw wrote: > nit: s/ot/to Done. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:244: if (!multi_line() || text().empty() || w <= 0 || !render_text_) On 2015/02/20 01:55:54, msw wrote: > nit: does this still need to check for a non-null render_text_? Done. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:251: render_text_->SetDisplayRect(gfx::Rect(0, 0, w, 0)); On 2015/02/20 01:55:55, msw wrote: > Will this have a persistent effect on the |render_text_|? So calling > Label::GetHeightForWidth() (ie. with a small width) could change the result of > the next internal Label call to GetDisplayText? Can you audit this and add a > test case to your unit test? Oops, that is true... Actually no problem for GetPreferredSize() because it explicitly reset the display rect. The same thing is added to MaybeBuildRenderTextLines(). https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:285: state->name = render_text_->GetDisplayText(); On 2015/02/20 01:55:54, msw wrote: > I wonder if this should use the un-elided (but still possibly obscured) layout > text, instead of the elided display text. Added RenderText::GetAccessibleText() and used it. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:298: tooltip->assign(text()); On 2015/02/20 01:55:54, msw wrote: > I wonder if this should use the un-elided (but still possibly obscured) layout > text, instead of the un-obscured text (ie. showing the password as a tooltip > when the actual label shows the obscured chars as '*'). Similarly, done. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:442: for (size_t i = lines_.size(); i < lines.size(); ++i) On 2015/02/20 01:55:54, msw wrote: > Can you add a comment here like "Append the remaining text to the last visible > line."... After all the time since I wrote this, I forgot what it was doing. Done. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:459: if (text().empty() || !render_text_) { On 2015/02/20 01:55:53, msw wrote: > nit: does this still need to check for a null render_text_? Done. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:463: render_text_->SetDisplayRect(gfx::Rect()); On 2015/02/20 01:55:54, msw wrote: > Hmm, is this needed and does it actually cause RenderText::GetStringSize to > return the value you want? I wonder if it would be helpful to add a separate > RenderText function that returns the natural single-line size of the obscured > layout text. It shouldn't be hard or expensive to keep that value up to date in > RenderText. This is to cancel the effect of SetDisplayRect() in GetHeightForWidth(). I think this approach (canceling by SetDisplayRect(gfx::Rect()) when necessary). Sometimes the caller may invoke SetHeightForWidth() multiple times with the same width, and then it will cache the result in that case. If the specified display_rect is same, RenderText does nothing, so it's low-cost. Added comment for this. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:468: base::SplitString(text(), '\n', &lines); On 2015/02/20 01:55:54, msw wrote: > Shouldn't this be using the RenderText's layout text to respect obscuring, but > ignore the current eliding? Used GetAccessibleText(). https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:475: size.set_height(size.height() + line.height()); On 2015/02/20 01:55:53, msw wrote: > Shouldn't this respect the minimum line_height()? Done. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:484: if (!render_text_) On 2015/02/20 01:55:54, msw wrote: > nit: still necessary? Done. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.h:145: void VisibilityChanged(View* starting_from, bool is_visible) override; On 2015/02/20 01:55:55, msw wrote: > nit: reorder to match View decl order (after OnBoundsChanged, i think) Done. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.h:155: // Create a single RenderText instance to be painted actually. On 2015/02/20 01:55:55, msw wrote: > nit: "to actually be painted." (ditto below for MaybeBuildRenderTextLines) Done. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label_unittest.cc:377: label.SetElideBehavior(gfx::ELIDE_TAIL); On 2015/02/20 01:55:55, msw wrote: > You do this just a few lines above, is there any reason to set it twice? removed https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label_unittest.cc:386: EXPECT_EQ(narrow_size.ToString(), label.GetPreferredSize().ToString()); On 2015/02/20 01:55:55, msw wrote: > nit: I guess check EXPECT_EQ(text, label.GetLayoutTextForTesting()); after this > to be consistent with the checks below, but I'm really not sure why it's worth > checking here... removed this section. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label_unittest.cc:388: // SetBounds() as well. On 2015/02/20 01:55:55, msw wrote: > nit: copy the comment for consistency: "doesn't change the preferred size." Done. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label_unittest.cc:393: // Paint() On 2015/02/20 01:55:55, msw wrote: > nit: copy the comment for consistency: "doesn't change the preferred size." Done. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label_unittest.cc:458: // Check that label can release its internal layout data when it gets On 2015/02/20 01:55:55, msw wrote: > nit: s/can release/releases/ and s/it gets/it's/ Done. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label_unittest.cc:502: scoped_ptr<gfx::RenderText> render_text(gfx::RenderText::CreateInstance()); On 2015/02/20 01:55:55, msw wrote: > Maybe exclude this test from Mac and ASSERT that the RenderText instance > supports multi-line, then construct a similar test for Mac, and ASSERT that > multi-line isn't supported, and then ensure that the label creates a RenderText > instance for each line on Mac... Done. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label_unittest.cc:514: // The number of lines is just 1, because the RenderText object can handle On 2015/02/20 01:55:55, msw wrote: > nit: " // There's only one 'line', RenderText itself supports multiple lines." Done.
https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:101: void Label::SetSubpixelRenderingEnabled(bool subpixel_rendering_enabled) { On 2015/02/25 21:15:41, Jun Mukai wrote: > On 2015/02/20 01:55:54, msw wrote: > > nit: should this be renamed to match the RenderText setting? > > Let me do that in another CL. It'll complicate this CL. Acknowledged. Consider adding a TODO in the header. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:132: DCHECK(!multi_line || (elide_behavior_ == gfx::ELIDE_TAIL || On 2015/02/25 21:15:41, Jun Mukai wrote: > On 2015/02/20 01:55:54, msw wrote: > > Please confirm that multi-line RenderTextHarfBuzz supports ELIDE_TAIL, or make > > that fallback to using multiple |lines_|... I think Oshima removed that > support. > > Actually we do not support eliding for multilined labels right now. > views::Label (before this patch) simply cut off very long lines. > # This means I made changes around CreateRenderText to set elide_behavior. > > elide_behavior_ == gfx::ELIDE_TAIL is allowed here simply because that's the > default value and we don't want to cause check failure for the code like this: > views::Label* label = new views::Label(text); > label->SetMultiLine(true); // elide_behavior is not set > label->SetElideBehavior(NO_ELIDE); > : Hmm, this is okay for now, as long as it's no worse than the status quo. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:251: render_text_->SetDisplayRect(gfx::Rect(0, 0, w, 0)); On 2015/02/25 21:15:41, Jun Mukai wrote: > On 2015/02/20 01:55:55, msw wrote: > > Will this have a persistent effect on the |render_text_|? So calling > > Label::GetHeightForWidth() (ie. with a small width) could change the result of > > the next internal Label call to GetDisplayText? Can you audit this and add a > > test case to your unit test? > > Oops, that is true... > Actually no problem for GetPreferredSize() because it explicitly reset the > display rect. The same thing is added to MaybeBuildRenderTextLines(). I don't think your change addresses my concern. The big problems with setting |render_text_|'s display rect in GetHeightForWidth are that: A) Subsequent calls to render_text_->GetDisplayText() will elide the text to fit that random width... (this needs some fixes) and B) Subsequent calls to render_text_->GetStringSize[F]() and render_text_->GetContentWidth() will similarly respect that random width for wrapping the text. (luckily the current calls should be okay as-is) These *might* be okay if we: 1) Add a NOTE/WARNING comment to GetHeightForWidth() for Label authors. 2) Call render_text_->SetDisplayRect(GetContentsBounds()) or similar in GetDisplayTextForTesting() with a comment. 3) Change the GetDisplayText() call in GetLinesForWidth() to use GetLayoutText(), I don't think that should be using the elided text anyway. https://codereview.chromium.org/867003002/diff/340001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/867003002/diff/340001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:723: return layout_text_; I think this should work as-is, since |layout_text_| is updated immediately on each call to OnTextAttributeChanged, but I'm CC'ing oshima just in case, as per the header comment for layout_text(): // NOTE: The value of these accessors may be stale. Please make sure // that these fields are up-to-date before accessing them. https://codereview.chromium.org/867003002/diff/340001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/867003002/diff/340001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:272: // The layout text will be elided to fit |display_rect| using this behavior. nit: please update this comment: s/layout/display/ https://codereview.chromium.org/867003002/diff/340001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:360: // Returns the un-elided text, but replaced when obscured. nit: consider a comment matching the format of that for GetDisplayText(): "Returns the text used for layout, which may be truncated or obscured, but it will not be elided." https://codereview.chromium.org/867003002/diff/340001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:361: const base::string16& GetAccessibleText(); Let's call this GetLayoutText(). https://codereview.chromium.org/867003002/diff/340001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/867003002/diff/340001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:653: TEST_F(RenderTextTest, AccessibleText) { nit: update naming and comments below to reflect GetLayoutText() https://codereview.chromium.org/867003002/diff/340001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/867003002/diff/340001/ui/views/controls/label... ui/views/controls/label.cc:285: state->name = render_text_->GetAccessibleText(); ditto nit: add a comment with this: // Use the layout text, which may truncated and obscured, but not elided. https://codereview.chromium.org/867003002/diff/340001/ui/views/controls/label... ui/views/controls/label.cc:297: if (ShouldShowDefaultTooltip()) { ditto nit: add a comment with this: // Use the layout text, which may truncated and obscured, but not elided. https://codereview.chromium.org/867003002/diff/340001/ui/views/controls/label... ui/views/controls/label.cc:424: gfx::ElideBehavior elide_behavior = nit: add a TODO here to support multi-line eliding. https://codereview.chromium.org/867003002/diff/340001/ui/views/controls/label... ui/views/controls/label.cc:426: // Cancel the display rect of |render_text_|. The display rect may be I don't think |render_text_|'s display rect actually matters directly below, since the instances we create use |rect| instead. You can probably remove this comment and statement. https://codereview.chromium.org/867003002/diff/340001/ui/views/controls/label... ui/views/controls/label.cc:464: gfx::ElideRectangleText(render_text_->GetDisplayText(), font_list(), width, I think this should use render_text_->GetLayoutText() instead... We don't want to use an elided version of the text to generate the lines for the old multi-line display codepath. https://codereview.chromium.org/867003002/diff/340001/ui/views/controls/label... ui/views/controls/label.cc:475: // Cancel the display rect of |render_text_| here. See the comment in nit: Move the meat of that comment down here if the same change isn't needed above, as I suggest. https://codereview.chromium.org/867003002/diff/340001/ui/views/controls/label... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/867003002/diff/340001/ui/views/controls/label... ui/views/controls/label_unittest.cc:383: // SetBounds() doesn't change the preferred size. Can you similarly test GetHeightForWidth with a small width? https://codereview.chromium.org/867003002/diff/340001/ui/views/controls/label... ui/views/controls/label_unittest.cc:495: #if !defined(OS_MACOSX) Construct a similar test for Mac that ASSERTs that multi-line isn't supported, and then ensure that the label creates a RenderText instance for each line...
https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:101: void Label::SetSubpixelRenderingEnabled(bool subpixel_rendering_enabled) { On 2015/02/25 23:54:26, msw wrote: > On 2015/02/25 21:15:41, Jun Mukai wrote: > > On 2015/02/20 01:55:54, msw wrote: > > > nit: should this be renamed to match the RenderText setting? > > > > Let me do that in another CL. It'll complicate this CL. > > Acknowledged. Consider adding a TODO in the header. TODO added. https://codereview.chromium.org/867003002/diff/320001/ui/views/controls/label... ui/views/controls/label.cc:251: render_text_->SetDisplayRect(gfx::Rect(0, 0, w, 0)); On 2015/02/25 23:54:26, msw wrote: > On 2015/02/25 21:15:41, Jun Mukai wrote: > > On 2015/02/20 01:55:55, msw wrote: > > > Will this have a persistent effect on the |render_text_|? So calling > > > Label::GetHeightForWidth() (ie. with a small width) could change the result > of > > > the next internal Label call to GetDisplayText? Can you audit this and add a > > > test case to your unit test? > > > > Oops, that is true... > > Actually no problem for GetPreferredSize() because it explicitly reset the > > display rect. The same thing is added to MaybeBuildRenderTextLines(). > > I don't think your change addresses my concern. The big problems with setting > |render_text_|'s display rect in GetHeightForWidth are that: > A) Subsequent calls to render_text_->GetDisplayText() will elide the text to fit > that random width... (this needs some fixes) and > B) Subsequent calls to render_text_->GetStringSize[F]() and > render_text_->GetContentWidth() will similarly respect that random width for > wrapping the text. (luckily the current calls should be okay as-is) > > These *might* be okay if we: > 1) Add a NOTE/WARNING comment to GetHeightForWidth() for Label authors. > 2) Call render_text_->SetDisplayRect(GetContentsBounds()) or similar in > GetDisplayTextForTesting() with a comment. > 3) Change the GetDisplayText() call in GetLinesForWidth() to use > GetLayoutText(), I don't think that should be using the elided text anyway. First, |render_text_| is explicitly specified as NO_ELIDE and that's not changed. This CL creates new RenderText instance(s) for actually painting (stored in |lines_|), see MaybeBuildRenderTextLines(). Therefore, A won't happen at all. For B, I put render_text_->GetDisplayRect(gfx::Rect()) before calling GetStringSize() in GetPreferredSize() to cancel the effect, and no one else is calling this method in this file. https://codereview.chromium.org/867003002/diff/340001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/867003002/diff/340001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:723: return layout_text_; On 2015/02/25 23:54:26, msw wrote: > I think this should work as-is, since |layout_text_| is updated immediately on > each call to OnTextAttributeChanged, but I'm CC'ing oshima just in case, as per > the header comment for layout_text(): > // NOTE: The value of these accessors may be stale. Please make sure > // that these fields are up-to-date before accessing them. This method was removed. https://codereview.chromium.org/867003002/diff/340001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/867003002/diff/340001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:272: // The layout text will be elided to fit |display_rect| using this behavior. On 2015/02/25 23:54:26, msw wrote: > nit: please update this comment: s/layout/display/ Done. https://codereview.chromium.org/867003002/diff/340001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:360: // Returns the un-elided text, but replaced when obscured. On 2015/02/25 23:54:26, msw wrote: > nit: consider a comment matching the format of that for GetDisplayText(): > "Returns the text used for layout, which may be truncated or obscured, but it > will not be elided." Removed this, adding this was my mistake, Because Label's |render_text_| is not elided at all, this method is not necessary. https://codereview.chromium.org/867003002/diff/340001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:361: const base::string16& GetAccessibleText(); On 2015/02/25 23:54:26, msw wrote: > Let's call this GetLayoutText(). ditto https://codereview.chromium.org/867003002/diff/340001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/867003002/diff/340001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:653: TEST_F(RenderTextTest, AccessibleText) { On 2015/02/25 23:54:26, msw wrote: > nit: update naming and comments below to reflect GetLayoutText() This test case was also removed since the method was removed. https://codereview.chromium.org/867003002/diff/340001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/867003002/diff/340001/ui/views/controls/label... ui/views/controls/label.cc:285: state->name = render_text_->GetAccessibleText(); On 2015/02/25 23:54:27, msw wrote: > ditto nit: add a comment with this: > // Use the layout text, which may truncated and obscured, but not elided. Reverted back to GetDisplayText(). |render_text_| is never elided, therefore GetDisplayText() returns the expected text. Added a comment for why GetDisplayText() is okay. https://codereview.chromium.org/867003002/diff/340001/ui/views/controls/label... ui/views/controls/label.cc:297: if (ShouldShowDefaultTooltip()) { On 2015/02/25 23:54:27, msw wrote: > ditto nit: add a comment with this: > // Use the layout text, which may truncated and obscured, but not elided. ditto https://codereview.chromium.org/867003002/diff/340001/ui/views/controls/label... ui/views/controls/label.cc:424: gfx::ElideBehavior elide_behavior = On 2015/02/25 23:54:27, msw wrote: > nit: add a TODO here to support multi-line eliding. Done. https://codereview.chromium.org/867003002/diff/340001/ui/views/controls/label... ui/views/controls/label.cc:426: // Cancel the display rect of |render_text_|. The display rect may be On 2015/02/25 23:54:27, msw wrote: > I don't think |render_text_|'s display rect actually matters directly below, > since the instances we create use |rect| instead. You can probably remove this > comment and statement. You're right, removed. https://codereview.chromium.org/867003002/diff/340001/ui/views/controls/label... ui/views/controls/label.cc:464: gfx::ElideRectangleText(render_text_->GetDisplayText(), font_list(), width, On 2015/02/25 23:54:27, msw wrote: > I think this should use render_text_->GetLayoutText() instead... We don't want > to use an elided version of the text to generate the lines for the old > multi-line display codepath. |render_text_| is not elided, so GetDisplayText() is correct in this case. https://codereview.chromium.org/867003002/diff/340001/ui/views/controls/label... ui/views/controls/label.cc:475: // Cancel the display rect of |render_text_| here. See the comment in On 2015/02/25 23:54:27, msw wrote: > nit: Move the meat of that comment down here if the same change isn't needed > above, as I suggest. Done.
Great catch on render_text_ not being elided, I forgot about that! I think this is finally ready for some real world testing; lgtm with nits! https://codereview.chromium.org/867003002/diff/360001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/867003002/diff/360001/ui/views/controls/label... ui/views/controls/label.cc:251: // SetDisplayRect() has a side effect for further call of GetStringSize(). nit: s/further call/later calls/ https://codereview.chromium.org/867003002/diff/360001/ui/views/controls/label... ui/views/controls/label.cc:253: // cancel this effect before next time GetStringSize() is called. nit: "the next time"
https://codereview.chromium.org/867003002/diff/360001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/867003002/diff/360001/ui/views/controls/label... ui/views/controls/label.cc:251: // SetDisplayRect() has a side effect for further call of GetStringSize(). On 2015/02/26 04:23:58, msw wrote: > nit: s/further call/later calls/ Done. https://codereview.chromium.org/867003002/diff/360001/ui/views/controls/label... ui/views/controls/label.cc:253: // cancel this effect before next time GetStringSize() is called. On 2015/02/26 04:23:58, msw wrote: > nit: "the next time" Done.
Sky, please review this again.
drive-by (just a note). also +Andre CC, since he's been poking RenderTextMac/Harfbuzz+mac. And thanks for doing this! I'm excited about being able to ditch the app_list::CachedLabel thing we have that was addressing a big performance concern on Windows :) https://codereview.chromium.org/867003002/diff/380001/ui/views/controls/label... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/867003002/diff/380001/ui/views/controls/label... ui/views/controls/label_unittest.cc:495: #if !defined(OS_MACOSX) Note that since SetMultiline now calls render_text_->SetMultiline(multi_line); without waiting for a paint, there are more test failures on Mac: 6 tests crashed: LabelTest.EmptyLabelSizing LabelTest.MultiLineProperty LabelTest.MultiLineSizing LabelTest.MultiLineSizingWithElide LabelTest.MultilineSmallAvailableWidthSizing LabelTest.TooltipProperty these all die at [FATAL:render_text_mac.cc(115)] Check failed: !multiline(). RenderTextMac does not support multi line gfx::RenderTextMac::OnLayoutTextAttributeChanged(bool) + 266 gfx::RenderText::OnTextAttributeChanged() + 2424 gfx::RenderText::SetMultiline(bool) + 115 views::Label::SetMultiLine(bool) + 376 I think this is the right way to die (e.g. as opposed to Label::SetMultiLine() checking render_text_->MultilineSupported()), so (unless there are other opinions) I don't think anything needs to change except the tests. I'm happy to do this in a follow-up. Probably we want EmptyLabelSizing and TooltipProperty enabled, but doing something that makes sense on Mac, and the MultiLine tests just disabled until we know where that's needed on Mac and how best to fix it.
https://codereview.chromium.org/867003002/diff/380001/ui/views/controls/label... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/867003002/diff/380001/ui/views/controls/label... ui/views/controls/label_unittest.cc:495: #if !defined(OS_MACOSX) On 2015/02/27 01:21:00, tapted wrote: > Note that since SetMultiline now calls render_text_->SetMultiline(multi_line); > without waiting for a paint, there are more test failures on Mac: > > 6 tests crashed: > LabelTest.EmptyLabelSizing > LabelTest.MultiLineProperty > LabelTest.MultiLineSizing > LabelTest.MultiLineSizingWithElide > LabelTest.MultilineSmallAvailableWidthSizing > LabelTest.TooltipProperty > > these all die at > [FATAL:render_text_mac.cc(115)] Check failed: !multiline(). RenderTextMac does > not support multi line > gfx::RenderTextMac::OnLayoutTextAttributeChanged(bool) + 266 > gfx::RenderText::OnTextAttributeChanged() + 2424 > gfx::RenderText::SetMultiline(bool) + 115 > views::Label::SetMultiLine(bool) + 376 > > I think this is the right way to die (e.g. as opposed to Label::SetMultiLine() > checking render_text_->MultilineSupported()), so (unless there are other > opinions) I don't think anything needs to change except the tests. > > I'm happy to do this in a follow-up. > > Probably we want EmptyLabelSizing and TooltipProperty enabled, but doing > something that makes sense on Mac, and the MultiLine tests just disabled until > we know where that's needed on Mac and how best to fix it. Thanks for the followup. I haven't noticed oshima has added a DCHECK for multiline() in RenderTextMac(). Then, for now, I think it's better to revive two flags -- multi_line_ in views::Label and replace_newline_chars_ in RenderText.
sky, please take another look.
On 2015/02/27 02:06:48, Jun Mukai wrote: > Then, for now, I think it's better to revive two flags -- multi_line_ in > views::Label and replace_newline_chars_ in RenderText. Thanks! This makes the tests happy on Mac :). [except for LabelTest.MultiLineProperty* -- but that's a good thing] There is still an issue for MacViews and text eliding (tl;dr: RenderTextMac doesn't support it). I have some ideas [see below], but I think they can be done in a follow-up, which I'm happy to explore -- don't worry about MacViews :) *label_unittest.cc:102: Failure Value of: label.multi_line() Actual: false Expected: true https://codereview.chromium.org/867003002/diff/420001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/867003002/diff/420001/ui/views/controls/label... ui/views/controls/label.cc:449: render_text->SetMultiline(multi_line()); nit: I think swapping SetDisplayRect() and SetMultiline() here will have a minor performance benefit -- UpdateDisplayText/Elide have more work to do when the size is non-zero (and setting multiline false->true essentially makes UpdateDisplayText into a no-op currently, for an even bigger win if that's done first). https://codereview.chromium.org/867003002/diff/420001/ui/views/controls/label... ui/views/controls/label.cc:450: lines_.push_back(render_text.release()); This is causing problems [just] for RenderTextMac. RenderTextMac::EnsureLayout(..) doesn't take into account DisplayText -- it only populates lines from text() whereas RenderTextHarfBuzz::EnsureLayout() is able to populate lines from GetDisplayText() [RenderTextHarfBuzz has two run lists: one for display and one for layout]. This results in the text elision being lost whenever the text is drawn on Mac (which didn't happen previously when the elision was done in CalculateDrawStringParams()). Short of updating RenderTextMac to match Harfbuzz (or finally using Harfbuzz on mac..) perhaps a workaround is for Label::lines_ to instead store "dumb" RenderText objects. E.g., on Mac, if you add a line after this one: lines_.back()->SetText(lines_[0]->GetDisplayText()); this "fixes" the issue. But we would probably want to set elide behavior to NO_ELIDE at the same time. Another option could be to do what's done for views::textfields for MacViews. These also rely on things RenderTextMac doesn't provide, so instead of RenderText::CreateInstance, they do gfx::RenderText::CreateInstanceForEditing(). Maybe we want this anyway. It could mean toolkit-views on Mac would let us gradually phase out RenderTextMac. Blink has already phased it out on Mac. Attempts at flipping it on Mac for the Chrome UI have happened in the past but it resulted it lots of hard-to-pin-down text rendering glitches, so people got scared off. But MacViews obviously gives an opportunity to address any rendering glitches as things get ported.
The problem tapted@ pointed out was gone by another CL. Please take a look.
LGTM
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 Link to the patchset: https://codereview.chromium.org/867003002/#ps460001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/867003002/460001
Message was sent while issue was closed.
Committed patchset #24 (id:460001)
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/1dfc595ffc753b6b1c99cca58e676e8b3e1bf6d8 Cr-Commit-Position: refs/heads/master@{#320138} |
