Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
916203002:
Reduce the number of text reshaping in RenderText (Closed)
DescriptionReduce the number of text reshaping in RenderText
Short Design Doc: http://goo.gl/w6Q9k8
1) Compute ElidedText on demand
2) Separate elided text and non elided text. Create elided text and runs only when necessary.
3) Use known width in RenderText::Elide
4) Don't reshape to get GetGraphemeIterator.
This CL also adds a few trace event so that we can identify the UI jank caused by text rendering performance.
BUG=451459
R=msw@chromium.org
Committed: https://crrev.com/0a243a4040f27b7be0f4c5bb92da4e2893f03b1c
Cr-Commit-Position: refs/heads/master@{#316375}
Patch Set 1 : #Patch Set 2 : #
Total comments: 29
Patch Set 3 : #Patch Set 4 : #
Total comments: 140
Patch Set 5 : #
Total comments: 10
Patch Set 6 : #
Total comments: 7
Patch Set 7 : #
Total comments: 2
Patch Set 8 : #
Messages
Total messages: 41 (22 generated)
Patchset #3 (id:40001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:80002) has been deleted
oshima@chromium.org changed reviewers: + mukai@chromium.org
https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1417: "431326 RenderTextHarfBuzz::EnsureLayout1")); This is not a typo. I usede the same trace string as in EnsureLayout() so that we can compare with old data.
mukai@chromium.org changed reviewers: + ckocagil@chromium.org
+ckocagil https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:1270: TRACE_EVENT0("ui", "RenderText::Elide"); Is this intentionally added? https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:270: void set_truncate_length(size_t length) { truncate_length_ = length; } Doesn't this have to update the layout text?
https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:1270: TRACE_EVENT0("ui", "RenderText::Elide"); On 2015/02/12 21:56:46, Jun Mukai wrote: > Is this intentionally added? Yes. This is very expensive operation and I think it's worth showing this in chrome://tracing. https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:270: void set_truncate_length(size_t length) { truncate_length_ = length; } On 2015/02/12 21:56:46, Jun Mukai wrote: > Doesn't this have to update the layout text? This is used as safe guard now. I'd like to change this method for test only and set the hard limit in ctor. msw@, what do you think?
Initial comments just for render_text.[h|cc] https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:1263: float visual_width, nit: match the argument name with the declaration's |text_width| https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:1284: if (visual_width <= available_width) nit: return at the top if a non-zero visual_width <= available_width https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:1294: const float ellipsis_width = render_text->GetContentWidthF(); I doubt it, but maybe it'd be helpful to cache the ellipses width? (I doubt this function is called with repeated layout attributes) https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:1451: const std::vector<Rect> selections = GetSubstringBounds(selection()); nit: I think you can inline this in the range-based for loop below without re-evaluating GetSubStringBounds on each iteration. https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:270: void set_truncate_length(size_t length) { truncate_length_ = length; } On 2015/02/12 22:09:17, oshima wrote: > On 2015/02/12 21:56:46, Jun Mukai wrote: > > Doesn't this have to update the layout text? > > This is used as safe guard now. I'd like to change this method for test only and > set the hard limit in ctor. msw@, what do you think? That's fine with me. https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:534: // |layouttext_| has changed, not just attributes. nit: |layout_text_| https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:537: // Notifies that attributes that affects elided text shape has changed. nit: s/affects/affect/ and s/has/have/ https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:625: // This will triggers |elided_text_| update. nit: merge the comments to something simpler, like "Updates |layout_text_| and |elided_text_| as needed." https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:626: void UpdateLayoutText(); I like the pattern of On*Changed, and I could see this being renamed OnText[Content|Attribute]Changed or maybe OnLayoutAttributeChanged or similar. WDYT? https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:714: // The obscured and/or truncated text, before elided. Rename this, it doesn't match the semantics of GetLayoutText, which elides the text as needed... Perhaps rename them as an intermediate |unelided_layout_text_| and a definitive |layout_text_|, or some other pattern. https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:718: base::string16 elided_text_; nit: reorder |elide_behavior_| to come immediately before |elided_text_| and |text_elided_|. https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:720: bool text_elided_; nit: add a comment like "True if the text is elided given the current behavior and display area." https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:764: // ResetLayout and on |display_rect_| changes. nit: Update the comment referencing ResetLayout.
Patchset #4 (id:200001) has been deleted
Patchset #4 (id:180002) has been deleted
https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:1263: float visual_width, On 2015/02/12 22:45:06, msw wrote: > nit: match the argument name with the declaration's |text_width| Done. https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:1284: if (visual_width <= available_width) On 2015/02/12 22:45:06, msw wrote: > nit: return at the top if a non-zero visual_width <= available_width Done. https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:1294: const float ellipsis_width = render_text->GetContentWidthF(); On 2015/02/12 22:45:06, msw wrote: > I doubt it, but maybe it'd be helpful to cache the ellipses width? > (I doubt this function is called with repeated layout attributes) The ellipse size depends on the font list, so we can't cache it easily. https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:1451: const std::vector<Rect> selections = GetSubstringBounds(selection()); On 2015/02/12 22:45:06, msw wrote: > nit: I think you can inline this in the range-based for loop below without > re-evaluating GetSubStringBounds on each iteration. Done. https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:534: // |layouttext_| has changed, not just attributes. On 2015/02/12 22:45:06, msw wrote: > nit: |layout_text_| Done. https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:537: // Notifies that attributes that affects elided text shape has changed. On 2015/02/12 22:45:06, msw wrote: > nit: s/affects/affect/ and s/has/have/ Done. https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:625: // This will triggers |elided_text_| update. On 2015/02/12 22:45:06, msw wrote: > nit: merge the comments to something simpler, like "Updates |layout_text_| and > |elided_text_| as needed." Done. https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:626: void UpdateLayoutText(); On 2015/02/12 22:45:06, msw wrote: > I like the pattern of On*Changed, and I could see this being renamed > OnText[Content|Attribute]Changed or maybe OnLayoutAttributeChanged or similar. > WDYT? Changed to OnTextAttributeChanged https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:714: // The obscured and/or truncated text, before elided. On 2015/02/12 22:45:06, msw wrote: > Rename this, it doesn't match the semantics of GetLayoutText, which elides the > text as needed... Perhaps rename them as an intermediate |unelided_layout_text_| > and a definitive |layout_text_|, or some other pattern. kept layout_text_ and changed elided_text_ to display_text_ as discussed offline. let me know if you misunderstood something. https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:718: base::string16 elided_text_; On 2015/02/12 22:45:06, msw wrote: > nit: reorder |elide_behavior_| to come immediately before |elided_text_| and > |text_elided_|. Done. https://codereview.chromium.org/916203002/diff/160001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:720: bool text_elided_; On 2015/02/12 22:45:06, msw wrote: > nit: add a comment like "True if the text is elided given the current behavior > and display area." Done.
Despite the comment deluge, this CL looks pretty good. You can defer some renaming if you leave very clear TODOs and follow up soon. Remember to search for occurrences of renamed identifiers in comments. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:665: OnLayoutTextAttributeChanged(false); nit: Maybe add a TODO comment here related to my doc suggestion. Something like: "Altering composition underlines shouldn't require layout changes. It's currently necessary because RenderTextHarfBuzz paints text decorations by run, and RenderTextMac applies all styles during layout." https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:680: OnLayoutTextAttributeChanged(false); nit: Maybe add a TODO comment here related to my doc suggestion. Something like: "Applying text decorations (strike, diagonal strike, and underline) shouldn't require layout changes. It's currently necessary because RenderTextMac applies all styles during layout." https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:692: OnLayoutTextAttributeChanged(false); nit: Maybe add a TODO comment here related to my doc suggestion. Something like: "Applying text decorations (strike, diagonal strike, and underline) shouldn't require layout changes. It's currently necessary because RenderTextMac applies all styles during layout." https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:977: if (multiline_ || Add a TODO here to support eliding multi-line text for views::Label, etc. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:992: elide_behavior_)); nit: indent one space https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:1270: if (text_width && text_width < available_width) nit: explicitly check "text_width > 0" https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:139: // Line segments are slices of the layout text to be rendered on a single line. nit: s/layout/display/ https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:159: // A line of layout text, comprised of a line segment list and some metrics. nit: s/layout/display/ https://codereview.chromium.org/916203002/diff/230001/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: s/layout/display/ https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:359: // Returns the text used for layout, which may be obscured, nit: s/layout/display https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:362: virtual const base::string16& GetLayoutText() = 0; I think this should be renamed GetDisplayText(). https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:486: virtual int GetLayoutTextBaseline() = 0; nit: Rename this as GetDisplayTextBaseLine or maybe just GetCurrentTextBaseLine? (update any mentions of the function name in comments too) https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:526: // GetLayoutText(), which differ when the text is obscured, nit: GetDisplayText() https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:529: virtual size_t TextIndexToLayoutIndex(size_t index) = 0; nit: Rename these TextIndexToDisplayIndex and DisplayIndexToTextIndex. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:549: // Returns layout text positions that are suitable for breaking lines. nit: s/layout/display/ https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:624: // Updates |layout_text_| and |display_text_| as needed. nit: maybe this should say "as needed (or marks them dirty)."... Since calling this for RTHB doesn't actually update |display_text_|, so using that or display_text() immediately afterwards is incorrect. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:692: // BreakList positions are stored with text indices, not layout indices. nit: s/layout/display/ https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:715: base::string16 display_text_; It's a little unfortunate that we need to keep an additional copy of the text when it's elided (especially if we're space-conscious enough to the point of using bit-field bools), but I see how this makes the layout passes more piecemeal and allows more efficient handling of display attributes changes. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:721: bool text_elided_; This is nearly equivalent to display_text_.empty(), but I guess we can't simply replace uses of this with !display_text_.empty(), for the case where eliding the text causes it to vanish completely (not even an ellipsis can fit). https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:724: bool replace_newline_chars_with_symbols_; You can remove this, SetReplaceNewlineCharsWithSymbols is never called. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:761: // A list of valid layout text line break positions. nit: s/layout/display/ https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:765: // OnLayoutTextAttributeChanged and OnDisplayTextAttributeChanged call. nit: "calls" https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (left): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:833: DCHECK_LE(index, GetLayoutText().length()); Why remove this? Isn't it still reasonable and correct? Are you worried that this may trigger EnsureLayoutRunList when it's not otherwise needed? https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:583: void TextRunList::Add(internal::TextRunHarfBuzz* run) { nit: remove internal:: https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:584: runs_.push_back(run); nit: can this be in-lined? https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:588: runs_.clear(); ditto nit: can this be in-lined? (and maybe renamed to TextRunList::clear) https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:610: internal::TextRunHarfBuzz* run = nit: remove internal:: here for a one-liner https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:617: int TextRunList::GetWidth() const { Replace GetWidth with a simple |float width_| and accessor, and initialize it in ComputePrecedingRunWidths (which could be renamed ComputeWidths). https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:618: int width = 0; nit: TextRunHarfBuzz is a float, shouldn't this be a float as well? https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:643: if (multiline() || Add a TODO here to support eliding multi-line text for views::Label, etc https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:646: UpdateDisplayText(0); nit: It's odd to use UpdateDisplayText just to clear |display_text_| and |text_elided_| on the base class... maybe add a comment to that effect? https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:654: return text_elided() ? display_text() : layout_text(); Add DCHECK(!update_display_text_) just above this. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:675: nit: remove blank line. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:703: nit: remove blank line. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:837: nit: remove blank line. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:929: update_display_text_ = true; I'm worried that lazily updating |display_text_| and offering access via RenderText::display_text() may allow use of stale data. Even though that doesn't seem to occur in this CL, it could easily occur in the future, right? https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:941: if (update_display_run_list_) { nit: optionally split this out to EnsureDisplayRunList, like EnsureLayoutRunList https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:943: const base::string16& elided_text = GetLayoutText(); nit: display_text = GetDisplayText(); https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:963: update_display_run_list_ = false; Perhaps set this at the top of the "if (update_display_run_list_)" block above? That seems to be the pattern for all the other update_* flags. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:974: if (lines().empty()) { nit: Maybe optionally split this out to EnsureLines? https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:981: nit: remove blank line https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1004: DCHECK(!update_display_run_list_); nit: also DCHECK(!update_display_text_) for completeness? https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1465: // Clamp layout indices to the length of the text actually used for layout. nit: // Clamp indices to the length of the given layout or display text. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1469: internal::TextRunList* RenderTextHarfBuzz::GetRunList() { nit: maybe return const_cast<RenderText*>(this)->GetRunList(); ? https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:98: const ScopedVector<internal::TextRunHarfBuzz>& runs() const { nit: this can be a one-liner, ditto for the non-const version. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:121: ScopedVector<internal::TextRunHarfBuzz> runs_; nit: keep the comment: "// Text runs in logical order." https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:202: // all needed glyphs and false otherwise. Additionally updates |best_family|, nit: "all the glyphs needed for |run|, and false otherwise." (since providing |text| as an arg makes the subject of this function a bit more ambiguous). https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:208: internal::TextRunHarfBuzz* run, nit: I think this would be better as the 2nd param (after |text|). https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:217: // Shape the glyphs needed for the |text| and |run|. nit: "needed for the |run| within the |text|." or similar. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:223: internal::TextRunHarfBuzz* run); nit: I think this would be better as the 2nd param (after |text|). https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:226: // Makes sure that text runs for layout text are shaped. This also elides the text as needed, and that should be mentioned in the function name/comment or split off to another helper. I'd recommend splitting out a separate EnsureDisplayText() helper and calling that directly from EnsureLayout and GetLayoutText (which ought to be renamed GetDisplayText). Both functions should be as rigorous as possible about ensuring that their prerequisite conditions are satisfied (EnsureDisplayText should DCHECK(!update_layout_run_list_), etc.) https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:233: // |given_text| should be either |eilded_text_| or |layout_text_| nit: |display_text_| or |layout_text_| https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:243: // Text run list sorted in logical order for |layout text_| and nit: |layout_text_|, and you can probably remove "sorted in logical order" if you add my suggested comment to the TextRunList::runs_ member. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:249: bool update_layout_run_list_ : 1; It might be nicer to just clear/Reset the TextRunList (or reset its scoped_ptr) to mark these as invalid (and in need of an update), but I realize that conflates empty layout text (or no display text in the un-elided case) with invalid run lists... https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_mac.cc File ui/gfx/render_text_mac.cc (right): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:32: if (elide_behavior() == NO_ELIDE || Is this check and early return really needed? Won't text_elided be false in these cases anyway and cause layout_text() to be returned below anyway? https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:115: if (text_changed && Can you explain the reasoning here? Why only conditionally UpdateDisplayText? https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:119: UpdateDisplayText(GetContentWidth()); Won't passing GetContentWidth here cause the text to never elide? https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:388: const wchar_t* layout_text; nit: display_text https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2001: // Make sure that multiple mode ignores elide behavior. "multi-line", not "multiple". Ultimately, we want multi-line render text to support eliding, so I'm not sure this test is worthwhile.
Patchset #6 (id:270001) has been deleted
Patchset #5 (id:250001) has been deleted
PTAL. I'll split functions in a separate CL as this is already big. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:665: OnLayoutTextAttributeChanged(false); On 2015/02/13 05:53:34, msw wrote: > nit: Maybe add a TODO comment here related to my doc suggestion. Something like: > "Altering composition underlines shouldn't require layout changes. It's > currently necessary because RenderTextHarfBuzz paints text decorations by run, > and RenderTextMac applies all styles during layout." Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:680: OnLayoutTextAttributeChanged(false); On 2015/02/13 05:53:34, msw wrote: > nit: Maybe add a TODO comment here related to my doc suggestion. Something like: > "Applying text decorations (strike, diagonal strike, and underline) shouldn't > require layout changes. It's currently necessary because RenderTextMac applies > all styles during layout." Setting BOLD type changes the layout. See RenderTextTest.StringSizeBoldWidth for example. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:692: OnLayoutTextAttributeChanged(false); On 2015/02/13 05:53:34, msw wrote: > nit: Maybe add a TODO comment here related to my doc suggestion. Something like: > "Applying text decorations (strike, diagonal strike, and underline) shouldn't > require layout changes. It's currently necessary because RenderTextMac applies > all styles during layout." same here. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:977: if (multiline_ || On 2015/02/13 05:53:34, msw wrote: > Add a TODO here to support eliding multi-line text for views::Label, etc. Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:992: elide_behavior_)); On 2015/02/13 05:53:34, msw wrote: > nit: indent one space Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:1270: if (text_width && text_width < available_width) On 2015/02/13 05:53:34, msw wrote: > nit: explicitly check "text_width > 0" Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:139: // Line segments are slices of the layout text to be rendered on a single line. On 2015/02/13 05:53:34, msw wrote: > nit: s/layout/display/ Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:159: // A line of layout text, comprised of a line segment list and some metrics. On 2015/02/13 05:53:34, msw wrote: > nit: s/layout/display/ Done. https://codereview.chromium.org/916203002/diff/230001/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/13 05:53:35, msw wrote: > nit: s/layout/display/ This should layout text because the display text is the result of eliding. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:273: // The layout text may be shortened further by the truncate length. I remove this line because it's not correct. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:359: // Returns the text used for layout, which may be obscured, On 2015/02/13 05:53:35, msw wrote: > nit: s/layout/display Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:362: virtual const base::string16& GetLayoutText() = 0; On 2015/02/13 05:53:35, msw wrote: > I think this should be renamed GetDisplayText(). Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:486: virtual int GetLayoutTextBaseline() = 0; On 2015/02/13 05:53:34, msw wrote: > nit: Rename this as GetDisplayTextBaseLine or maybe just GetCurrentTextBaseLine? > (update any mentions of the function name in comments too) Chagned to GetDisplayTextBaseLine. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:526: // GetLayoutText(), which differ when the text is obscured, On 2015/02/13 05:53:34, msw wrote: > nit: GetDisplayText() Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:529: virtual size_t TextIndexToLayoutIndex(size_t index) = 0; On 2015/02/13 05:53:35, msw wrote: > nit: Rename these TextIndexToDisplayIndex and DisplayIndexToTextIndex. Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:549: // Returns layout text positions that are suitable for breaking lines. On 2015/02/13 05:53:34, msw wrote: > nit: s/layout/display/ Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:624: // Updates |layout_text_| and |display_text_| as needed. On 2015/02/13 05:53:34, msw wrote: > nit: maybe this should say "as needed (or marks them dirty)."... Since calling > this for RTHB doesn't actually update |display_text_|, so using that or > display_text() immediately afterwards is incorrect. Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:692: // BreakList positions are stored with text indices, not layout indices. On 2015/02/13 05:53:34, msw wrote: > nit: s/layout/display/ Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:715: base::string16 display_text_; On 2015/02/13 05:53:34, msw wrote: > It's a little unfortunate that we need to keep an additional copy of the text > when it's elided (especially if we're space-conscious enough to the point of > using bit-field bools), but I see how this makes the layout passes more > piecemeal and allows more efficient handling of display attributes changes. Because just drawing text requires only display info, not layout info, it's possible to clear layout side upon paint. Added TODO. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:721: bool text_elided_; On 2015/02/13 05:53:34, msw wrote: > This is nearly equivalent to display_text_.empty(), but I guess we can't simply > replace uses of this with !display_text_.empty(), for the case where eliding the > text causes it to vanish completely (not even an ellipsis can fit). Yep, I tried it and reverted thanks to broken test. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:724: bool replace_newline_chars_with_symbols_; On 2015/02/13 05:53:34, msw wrote: > You can remove this, SetReplaceNewlineCharsWithSymbols is never called. Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:761: // A list of valid layout text line break positions. On 2015/02/13 05:53:34, msw wrote: > nit: s/layout/display/ Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:765: // OnLayoutTextAttributeChanged and OnDisplayTextAttributeChanged call. On 2015/02/13 05:53:34, msw wrote: > nit: "calls" Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (left): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:833: DCHECK_LE(index, GetLayoutText().length()); On 2015/02/13 05:53:35, msw wrote: > Why remove this? Isn't it still reasonable and correct? Are you worried that > this may trigger EnsureLayoutRunList when it's not otherwise needed? I removed this because this get called while computing DisplayText from layout text. Maybe we should rename this to something like CurrentIndexToTextIndex ? https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:583: void TextRunList::Add(internal::TextRunHarfBuzz* run) { On 2015/02/13 05:53:36, msw wrote: > nit: remove internal:: Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:584: runs_.push_back(run); On 2015/02/13 05:53:35, msw wrote: > nit: can this be in-lined? Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:588: runs_.clear(); On 2015/02/13 05:53:35, msw wrote: > ditto nit: can this be in-lined? (and maybe renamed to TextRunList::clear) Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:610: internal::TextRunHarfBuzz* run = On 2015/02/13 05:53:35, msw wrote: > nit: remove internal:: here for a one-liner Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:617: int TextRunList::GetWidth() const { On 2015/02/13 05:53:35, msw wrote: > Replace GetWidth with a simple |float width_| and accessor, and initialize it in > ComputePrecedingRunWidths (which could be renamed ComputeWidths). I can't precompute the width because run's with may change later. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:618: int width = 0; On 2015/02/13 05:53:35, msw wrote: > nit: TextRunHarfBuzz is a float, shouldn't this be a float as well? Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:643: if (multiline() || On 2015/02/13 05:53:35, msw wrote: > Add a TODO here to support eliding multi-line text for views::Label, etc Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:646: UpdateDisplayText(0); On 2015/02/13 05:53:36, msw wrote: > nit: It's odd to use UpdateDisplayText just to clear |display_text_| and > |text_elided_| on the base class... maybe add a comment to that effect? Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:654: return text_elided() ? display_text() : layout_text(); On 2015/02/13 05:53:35, msw wrote: > Add DCHECK(!update_display_text_) just above this. Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:675: On 2015/02/13 05:53:35, msw wrote: > nit: remove blank line. Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:703: On 2015/02/13 05:53:35, msw wrote: > nit: remove blank line. Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:837: On 2015/02/13 05:53:35, msw wrote: > nit: remove blank line. Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:929: update_display_text_ = true; On 2015/02/13 05:53:35, msw wrote: > I'm worried that lazily updating |display_text_| and offering access via > RenderText::display_text() may allow use of stale data. Even though that doesn't > seem to occur in this CL, it could easily occur in the future, right? They're protected, so only implementation can access them, and I don't see big difference between text and runs. They both can be stale and wrong. I think unit tests would be good enough because using stale text most likely ends with wrong result. I can add DCHECK in accessor if you want though. Please let me know. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:941: if (update_display_run_list_) { On 2015/02/13 05:53:36, msw wrote: > nit: optionally split this out to EnsureDisplayRunList, like EnsureLayoutRunList Let me do that in separate CL. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:943: const base::string16& elided_text = GetLayoutText(); On 2015/02/13 05:53:36, msw wrote: > nit: display_text = GetDisplayText(); Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:963: update_display_run_list_ = false; On 2015/02/13 05:53:35, msw wrote: > Perhaps set this at the top of the "if (update_display_run_list_)" block above? > That seems to be the pattern for all the other update_* flags. I moved others. I think this is better to catch unexpected recursive calls to itself, and indeed it found one. We're calling GetTextDirection without context, which calls GetDisplayText(), which in turn calls this. I split GetTextDirection to GetDisplayTextDirection (for the place it expect the direction of the display text), and GetTextDirection. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:974: if (lines().empty()) { On 2015/02/13 05:53:36, msw wrote: > nit: Maybe optionally split this out to EnsureLines? Let me do that in a separate CL, along with EnsureLayoutTextRuns split. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:981: On 2015/02/13 05:53:35, msw wrote: > nit: remove blank line Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1004: DCHECK(!update_display_run_list_); On 2015/02/13 05:53:35, msw wrote: > nit: also DCHECK(!update_display_text_) for completeness? Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1465: // Clamp layout indices to the length of the text actually used for layout. On 2015/02/13 05:53:35, msw wrote: > nit: // Clamp indices to the length of the given layout or display text. Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1469: internal::TextRunList* RenderTextHarfBuzz::GetRunList() { On 2015/02/13 05:53:35, msw wrote: > nit: maybe return const_cast<RenderText*>(this)->GetRunList(); ? Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:98: const ScopedVector<internal::TextRunHarfBuzz>& runs() const { On 2015/02/13 05:53:36, msw wrote: > nit: this can be a one-liner, ditto for the non-const version. Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:121: ScopedVector<internal::TextRunHarfBuzz> runs_; On 2015/02/13 05:53:36, msw wrote: > nit: keep the comment: "// Text runs in logical order." Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:202: // all needed glyphs and false otherwise. Additionally updates |best_family|, On 2015/02/13 05:53:36, msw wrote: > nit: "all the glyphs needed for |run|, and false otherwise." (since providing > |text| as an arg makes the subject of this function a bit more ambiguous). Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:208: internal::TextRunHarfBuzz* run, On 2015/02/13 05:53:36, msw wrote: > nit: I think this would be better as the 2nd param (after |text|). Style guide says: When defining a function, parameter order is: inputs, then outputs. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Para... https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:217: // Shape the glyphs needed for the |text| and |run|. On 2015/02/13 05:53:36, msw wrote: > nit: "needed for the |run| within the |text|." or similar. Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:223: internal::TextRunHarfBuzz* run); On 2015/02/13 05:53:37, msw wrote: > nit: I think this would be better as the 2nd param (after |text|). same here. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:226: // Makes sure that text runs for layout text are shaped. On 2015/02/13 05:53:36, msw wrote: > This also elides the text as needed, and that should be mentioned in the > function name/comment or split off to another helper. I'd recommend splitting > out a separate EnsureDisplayText() helper and calling that directly from > EnsureLayout and GetLayoutText (which ought to be renamed GetDisplayText). Both > functions should be as rigorous as possible about ensuring that their > prerequisite conditions are satisfied (EnsureDisplayText should > DCHECK(!update_layout_run_list_), etc.) Let me do that in separate CL. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:233: // |given_text| should be either |eilded_text_| or |layout_text_| On 2015/02/13 05:53:36, msw wrote: > nit: |display_text_| or |layout_text_| Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:243: // Text run list sorted in logical order for |layout text_| and On 2015/02/13 05:53:36, msw wrote: > nit: |layout_text_|, and you can probably remove "sorted in logical order" if > you add my suggested comment to the TextRunList::runs_ member. Done. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:249: bool update_layout_run_list_ : 1; On 2015/02/13 05:53:36, msw wrote: > It might be nicer to just clear/Reset the TextRunList (or reset its scoped_ptr) > to mark these as invalid (and in need of an update), but I realize that > conflates empty layout text (or no display text in the un-elided case) with > invalid run lists... Yep. run can be empty if the text is empty, and the text can be empty when it may or may not have to be updated. Given that these conditions, I thought having explicit flag is better in terms of readability. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_mac.cc File ui/gfx/render_text_mac.cc (right): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:32: if (elide_behavior() == NO_ELIDE || On 2015/02/13 05:53:37, msw wrote: > Is this check and early return really needed? Won't text_elided be false in > these cases anyway and cause layout_text() to be returned below anyway? Nope. This was from attempt to do something similar on mac, but i gave up. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:115: if (text_changed && On 2015/02/13 05:53:37, msw wrote: > Can you explain the reasoning here? Why only conditionally UpdateDisplayText? RenderText used to elide text when text changed, and this is to keep the same behavior. I tried to change it but it's hard to do it without having mac. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:119: UpdateDisplayText(GetContentWidth()); On 2015/02/13 05:53:37, msw wrote: > Won't passing GetContentWidth here cause the text to never elide? This is one of the issue I mentioned in the doc. The very first GetContentWith() computes the width without eliding, which gives the full width, and then this re-computes the layout text based on that width and display rect. As a matter of fact, this is causing an issue to mukai's label caching. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2001: // Make sure that multiple mode ignores elide behavior. On 2015/02/13 05:53:37, msw wrote: > "multi-line", not "multiple". Ultimately, we want multi-line render text to > support eliding, so I'm not sure this test is worthwhile. Done.
Replying to existing comments... I'll look at the latest patch set now. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:680: OnLayoutTextAttributeChanged(false); On 2015/02/13 21:03:09, oshima wrote: > On 2015/02/13 05:53:34, msw wrote: > > nit: Maybe add a TODO comment here related to my doc suggestion. Something > like: > > "Applying text decorations (strike, diagonal strike, and underline) shouldn't > > require layout changes. It's currently necessary because RenderTextMac applies > > all styles during layout." > > Setting BOLD type changes the layout. See RenderTextTest.StringSizeBoldWidth for > example. I realize that, but the styles I mentioned (strike, diagonal strike, and underline) should not change the layout. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/916203002/diff/230001/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/13 21:03:09, oshima wrote: > On 2015/02/13 05:53:35, msw wrote: > > nit: s/layout/display/ > > This should layout text because the display text is the result of eliding. That's fair, the comment is ambiguous about where the resulting elided text is stored. Perhaps it should just say something like: "// Get or set the elide behavior used to generate the display text." https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:273: // The layout text may be shortened further by the truncate length. On 2015/02/13 21:03:09, oshima wrote: > I remove this line because it's not correct. It could say something like "Note that eliding is performed after truncation." https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (left): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:833: DCHECK_LE(index, GetLayoutText().length()); On 2015/02/13 21:03:10, oshima wrote: > On 2015/02/13 05:53:35, msw wrote: > > Why remove this? Isn't it still reasonable and correct? Are you worried that > > this may trigger EnsureLayoutRunList when it's not otherwise needed? > > I removed this because this get called while computing DisplayText from layout > text. Maybe we should rename this to something like CurrentIndexToTextIndex ? I liked the parity of TextIndexToLayoutIndex and LayoutIndexToTextIndex, but I see why you added TextIndexToGivenTextIndex to support the use of the intermediate layout_text... I guess you could do something similar for this, but I guess it's not really necessary just to support re-adding a DCHECK... Maybe it's fine to just remove the DCHECK for now. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:617: int TextRunList::GetWidth() const { On 2015/02/13 21:03:11, oshima wrote: > On 2015/02/13 05:53:35, msw wrote: > > Replace GetWidth with a simple |float width_| and accessor, and initialize it > in > > ComputePrecedingRunWidths (which could be renamed ComputeWidths). > > I can't precompute the width because run's with may change later. Huh? Isn't ComputePrecedingRunWidths run after shaping? I don't think any runs are re-shaped after that, right? When could the runs' widths change? https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:929: update_display_text_ = true; On 2015/02/13 21:03:10, oshima wrote: > On 2015/02/13 05:53:35, msw wrote: > > I'm worried that lazily updating |display_text_| and offering access via > > RenderText::display_text() may allow use of stale data. Even though that > doesn't > > seem to occur in this CL, it could easily occur in the future, right? > > They're protected, so only implementation can access them, and > I don't see big difference between text and runs. They both can be stale and > wrong. I think unit tests would be good enough because using stale text most > likely ends with wrong result. > > I can add DCHECK in accessor if you want though. Please let me know. I don't think you can easily DCHECK the RenderTextHarfBuzz::update_display_text_ flag in RenderText::display_text() accessor, but go ahead if I'm missing something. Yeah, using stale data is my biggest concern here, but I guess the code will just have to be well-maintained and authors will have to be cautious. Maybe add NOTE:/WARNING: comments to the text/run/list members/accessors? https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:941: if (update_display_run_list_) { On 2015/02/13 21:03:10, oshima wrote: > On 2015/02/13 05:53:36, msw wrote: > > nit: optionally split this out to EnsureDisplayRunList, like > EnsureLayoutRunList > > Let me do that in separate CL. Add a TODO. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:963: update_display_run_list_ = false; On 2015/02/13 21:03:11, oshima wrote: > On 2015/02/13 05:53:35, msw wrote: > > Perhaps set this at the top of the "if (update_display_run_list_)" block > above? > > That seems to be the pattern for all the other update_* flags. > > I moved others. I think this is better to catch unexpected recursive calls to > itself, and indeed it found one. > > We're calling GetTextDirection without context, which calls GetDisplayText(), > which in turn calls this. I split GetTextDirection to GetDisplayTextDirection > (for the place it expect the direction of the display text), and > GetTextDirection. Acknowledged. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:974: if (lines().empty()) { On 2015/02/13 21:03:10, oshima wrote: > On 2015/02/13 05:53:36, msw wrote: > > nit: Maybe optionally split this out to EnsureLines? > > Let me do that in a separate CL, along with EnsureLayoutTextRuns split. Add a TODO. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:208: internal::TextRunHarfBuzz* run, On 2015/02/13 21:03:11, oshima wrote: > On 2015/02/13 05:53:36, msw wrote: > > nit: I think this would be better as the 2nd param (after |text|). > > Style guide says: > When defining a function, parameter order is: inputs, then outputs. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Para... Hmm, it's really a mutable input parameter, not an output... but I guess you can keep it like this if you really consider it an output... https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:226: // Makes sure that text runs for layout text are shaped. On 2015/02/13 21:03:11, oshima wrote: > On 2015/02/13 05:53:36, msw wrote: > > This also elides the text as needed, and that should be mentioned in the > > function name/comment or split off to another helper. I'd recommend splitting > > out a separate EnsureDisplayText() helper and calling that directly from > > EnsureLayout and GetLayoutText (which ought to be renamed GetDisplayText). > Both > > functions should be as rigorous as possible about ensuring that their > > prerequisite conditions are satisfied (EnsureDisplayText should > > DCHECK(!update_layout_run_list_), etc.) > > Let me do that in separate CL. Add a TODO https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_mac.cc File ui/gfx/render_text_mac.cc (right): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:119: UpdateDisplayText(GetContentWidth()); On 2015/02/13 21:03:11, oshima wrote: > On 2015/02/13 05:53:37, msw wrote: > > Won't passing GetContentWidth here cause the text to never elide? > > This is one of the issue I mentioned in the doc. > The very first GetContentWith() computes the width without eliding, > which gives the full width, and then this re-computes the layout text based on > that width and display rect. > > As a matter of fact, this is causing an issue to mukai's label caching. Acknowledged. (I misread this late last night...)
https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:1253: // Replace the newline character with a newline symbol in single line mode. DO NOT REMOVE THIS! It's okay to remove |replace_newline_chars_with_symbols_|, since that is always true and never modified by a client. ReplaceChars *should* be run for single-line text, but not multiline text... leave this code and change the if statement to "if (!multiline_)" https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:715: base::string16 display_text_; On 2015/02/13 21:03:10, oshima wrote: > On 2015/02/13 05:53:34, msw wrote: > > It's a little unfortunate that we need to keep an additional copy of the text > > when it's elided (especially if we're space-conscious enough to the point of > > using bit-field bools), but I see how this makes the layout passes more > > piecemeal and allows more efficient handling of display attributes changes. > > Because just drawing text requires only display info, not layout info, > it's possible to clear layout side upon paint. Added TODO. True, but then any display attribute changes would require not just updates to the display text, but a full layout pass too, right? (I'm not sure this would be a good idea to implement) https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_mac.cc File ui/gfx/render_text_mac.cc (right): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:115: if (text_changed && On 2015/02/13 21:03:11, oshima wrote: > On 2015/02/13 05:53:37, msw wrote: > > Can you explain the reasoning here? Why only conditionally UpdateDisplayText? > > RenderText used to elide text when text changed, and this is to > keep the same behavior. I tried to change it but it's hard to do it > without having mac. I wonder if this needs to unconditionally update the display text... (eg. what if the text didn't change but the elide_behavior changed from ELIDE_TAIL to NO_ELIDE? Won't the display text be left elided?) https://codereview.chromium.org/916203002/diff/290001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/916203002/diff/290001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:355: // Returns the text used to display, which may be obscured, nit: update line wrapping (can fit "truncated or") https://codereview.chromium.org/916203002/diff/290001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:727: bool replace_newline_chars_with_symbols_; You didn't actually remove this... https://codereview.chromium.org/916203002/diff/290001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/916203002/diff/290001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:670: nit: I meant to remove this blank line. https://codereview.chromium.org/916203002/diff/290001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/916203002/diff/290001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:113: // Get the total width of runs. This is not useful when multiline is enabled. nit: consider // Get the total width of runs, as if they were shown on one line. https://codereview.chromium.org/916203002/diff/290001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/916203002/diff/290001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:388: const wchar_t* layout_text; ping nit: rename this |display_text|
Patchset #6 (id:310001) has been deleted
https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:1253: // Replace the newline character with a newline symbol in single line mode. On 2015/02/13 22:19:05, msw wrote: > DO NOT REMOVE THIS! It's okay to remove |replace_newline_chars_with_symbols_|, > since that is always true and never modified by a client. ReplaceChars *should* > be run for single-line text, but not multiline text... leave this code and > change the if statement to "if (!multiline_)" Done. I think I should have done this in a separate CL. It's getting harder to keep thing in sync. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:715: base::string16 display_text_; On 2015/02/13 22:19:05, msw wrote: > On 2015/02/13 21:03:10, oshima wrote: > > On 2015/02/13 05:53:34, msw wrote: > > > It's a little unfortunate that we need to keep an additional copy of the > text > > > when it's elided (especially if we're space-conscious enough to the point of > > > using bit-field bools), but I see how this makes the layout passes more > > > piecemeal and allows more efficient handling of display attributes changes. > > > > Because just drawing text requires only display info, not layout info, > > it's possible to clear layout side upon paint. Added TODO. > > True, but then any display attribute changes would require not just updates to > the display text, but a full layout pass too, right? (I'm not sure this would be > a good idea to implement) It's rare to apply style change after text has been displayed. Many texts rarely gets new content (menu, tray), and when you get the new text, you have to update them all anyway. We should definitely measure how many of usages can benefit from this though. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:617: int TextRunList::GetWidth() const { On 2015/02/13 21:49:23, msw wrote: > On 2015/02/13 21:03:11, oshima wrote: > > On 2015/02/13 05:53:35, msw wrote: > > > Replace GetWidth with a simple |float width_| and accessor, and initialize > it > > in > > > ComputePrecedingRunWidths (which could be renamed ComputeWidths). > > > > I can't precompute the width because run's with may change later. > > Huh? Isn't ComputePrecedingRunWidths run after shaping? I don't think any runs > are re-shaped after that, right? When could the runs' widths change? I think I know why the total size changed, due to Reset(). I somehow thought that run can be updated later (that's why I added non const runs() method), but it was probably my imagination. Removed non const runs() method. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:929: update_display_text_ = true; On 2015/02/13 21:49:23, msw wrote: > On 2015/02/13 21:03:10, oshima wrote: > > On 2015/02/13 05:53:35, msw wrote: > > > I'm worried that lazily updating |display_text_| and offering access via > > > RenderText::display_text() may allow use of stale data. Even though that > > doesn't > > > seem to occur in this CL, it could easily occur in the future, right? > > > > They're protected, so only implementation can access them, and > > I don't see big difference between text and runs. They both can be stale and > > wrong. I think unit tests would be good enough because using stale text most > > likely ends with wrong result. > > > > I can add DCHECK in accessor if you want though. Please let me know. > > I don't think you can easily DCHECK the RenderTextHarfBuzz::update_display_text_ > flag in RenderText::display_text() accessor, but go ahead if I'm missing > something. What I had in mind was to change accessor virtual and let subclass check it, but I think you still need a way to access without check, so it probably doesn't make much sense, and you're right. > Yeah, using stale data is my biggest concern here, but I guess the > code will just have to be well-maintained and authors will have to be cautious. > Maybe add NOTE:/WARNING: comments to the text/run/list members/accessors? Added comment. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:208: internal::TextRunHarfBuzz* run, On 2015/02/13 21:49:23, msw wrote: > On 2015/02/13 21:03:11, oshima wrote: > > On 2015/02/13 05:53:36, msw wrote: > > > nit: I think this would be better as the 2nd param (after |text|). > > > > Style guide says: > > When defining a function, parameter order is: inputs, then outputs. > > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Para... > > Hmm, it's really a mutable input parameter, not an output... but I guess you can > keep it like this if you really consider it an output... It's "input-output" parameter, and that's why I placed in between. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_mac.cc File ui/gfx/render_text_mac.cc (right): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text_mac.... ui/gfx/render_text_mac.cc:115: if (text_changed && On 2015/02/13 22:19:05, msw wrote: > On 2015/02/13 21:03:11, oshima wrote: > > On 2015/02/13 05:53:37, msw wrote: > > > Can you explain the reasoning here? Why only conditionally > UpdateDisplayText? > > > > RenderText used to elide text when text changed, and this is to > > keep the same behavior. I tried to change it but it's hard to do it > > without having mac. > > I wonder if this needs to unconditionally update the display text... (eg. what > if the text didn't change but the elide_behavior changed from ELIDE_TAIL to > NO_ELIDE? Won't the display text be left elided?) Ah, you're right. I have to call UpdateDisplayText(0) for the same reason RTHB is doing in that case. I think it should do the same as original version now. https://codereview.chromium.org/916203002/diff/290001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/916203002/diff/290001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:355: // Returns the text used to display, which may be obscured, On 2015/02/13 22:19:05, msw wrote: > nit: update line wrapping (can fit "truncated or") Done. https://codereview.chromium.org/916203002/diff/290001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:727: bool replace_newline_chars_with_symbols_; On 2015/02/13 22:19:05, msw wrote: > You didn't actually remove this... Done. https://codereview.chromium.org/916203002/diff/290001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/916203002/diff/290001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:670: On 2015/02/13 22:19:05, msw wrote: > nit: I meant to remove this blank line. Done. https://codereview.chromium.org/916203002/diff/290001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/916203002/diff/290001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:113: // Get the total width of runs. This is not useful when multiline is enabled. On 2015/02/13 22:19:05, msw wrote: > nit: consider // Get the total width of runs, as if they were shown on one line. Done. https://codereview.chromium.org/916203002/diff/290001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/916203002/diff/290001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:388: const wchar_t* layout_text; On 2015/02/13 22:19:05, msw wrote: > ping nit: rename this |display_text| Done.
lgtm with minor remaining comments. https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/916203002/diff/230001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:715: base::string16 display_text_; On 2015/02/13 23:24:11, oshima wrote: > On 2015/02/13 22:19:05, msw wrote: > > On 2015/02/13 21:03:10, oshima wrote: > > > On 2015/02/13 05:53:34, msw wrote: > > > > It's a little unfortunate that we need to keep an additional copy of the > > text > > > > when it's elided (especially if we're space-conscious enough to the point > of > > > > using bit-field bools), but I see how this makes the layout passes more > > > > piecemeal and allows more efficient handling of display attributes > changes. > > > > > > Because just drawing text requires only display info, not layout info, > > > it's possible to clear layout side upon paint. Added TODO. > > > > True, but then any display attribute changes would require not just updates to > > the display text, but a full layout pass too, right? (I'm not sure this would > be > > a good idea to implement) > > It's rare to apply style change after text has been displayed. Many texts rarely > gets new content (menu, tray), and when you get the new text, you have to update > them all anyway. > > We should definitely measure how many of usages can benefit from this though. Good point. Agreed that measurements would help. https://codereview.chromium.org/916203002/diff/330001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/916203002/diff/330001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:450: const base::string16& layout_text() const { return layout_text_; } Please add NOTE/WARNING comments for these accessors, as discussed. https://codereview.chromium.org/916203002/diff/330001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/916203002/diff/330001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1463: RenderTextHarfBuzz* that = const_cast<RenderTextHarfBuzz*>(this); nit: inline |that| below for: return const_cast<RenderTextHarfBuzz*>(this)->GetRunList(); https://codereview.chromium.org/916203002/diff/330001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/916203002/diff/330001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:113: // This is not useful when multiline is enabled. I was hoping that you'd remove this part of the comment, why is this not useful when multi-line is enabled? It still provides the width of the entire text, as needed to determine the initial line-wrapping, doesn't it? I'm not sure what this comment is actually trying to say, something like: "The text may wrap at a smaller width when multiline is enabled."?
New patchsets have been uploaded after l-g-t-m from msw@chromium.org
Patchset #7 (id:340001) has been deleted
Patchset #7 (id:360001) has been deleted
https://codereview.chromium.org/916203002/diff/330001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/916203002/diff/330001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:450: const base::string16& layout_text() const { return layout_text_; } On 2015/02/14 00:11:26, msw wrote: > Please add NOTE/WARNING comments for these accessors, as discussed. Hmm, I'm pretty sure I added once, but I probably have undone them while editing... Sorry. Done. https://codereview.chromium.org/916203002/diff/330001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/916203002/diff/330001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1463: RenderTextHarfBuzz* that = const_cast<RenderTextHarfBuzz*>(this); On 2015/02/14 00:11:26, msw wrote: > nit: inline |that| below for: > return const_cast<RenderTextHarfBuzz*>(this)->GetRunList(); Done. https://codereview.chromium.org/916203002/diff/330001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/916203002/diff/330001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:113: // This is not useful when multiline is enabled. On 2015/02/14 00:11:26, msw wrote: > I was hoping that you'd remove this part of the comment, why is this not useful > when multi-line is enabled? It still provides the width of the entire text, as > needed to determine the initial line-wrapping, doesn't it? No, it's not. This is not used when multi-line is enabled. And this will never useful for multi-line because you need to use the width for each line (or last line) to decide if and how to elide. When we add eliding support for multi-line text, the implementation will be quite different, and most likely this method will no longer be necessary. > I'm not sure what > this comment is actually trying to say, something like: "The text may wrap at a > smaller width when multiline is enabled."? I wanted to say "Don't use when mult-line is enabled", but it was't consistent with the code. I changed the comment "Don't use" and update the code to be consistent. Can you take a look at EnsureLayoutRunList() ?
still lgtm with one nit https://codereview.chromium.org/916203002/diff/330001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/916203002/diff/330001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:113: // This is not useful when multiline is enabled. On 2015/02/14 00:40:51, oshima wrote: > On 2015/02/14 00:11:26, msw wrote: > > I was hoping that you'd remove this part of the comment, why is this not > useful > > when multi-line is enabled? It still provides the width of the entire text, as > > needed to determine the initial line-wrapping, doesn't it? > > No, it's not. This is not used when multi-line is enabled. And this will never > useful for multi-line because you need to use the width for each line (or last > line) > to decide if and how to elide. > > When we add eliding support for multi-line text, the implementation will be > quite different, and most likely this method will no longer be necessary. > > > I'm not sure what > > this comment is actually trying to say, something like: "The text may wrap at > a > > smaller width when multiline is enabled."? > > I wanted to say "Don't use when mult-line is enabled", but it was't consistent > with the code. > > I changed the comment "Don't use" and update the code to be consistent. > Can you take a look at EnsureLayoutRunList() ? > > Ah, I suppose that you're right. https://codereview.chromium.org/916203002/diff/380001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/916203002/diff/380001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:450: // NOTE: The value of these accessors may contain stale values. Please nit: "The values of these accessors may be stale."
New patchsets have been uploaded after l-g-t-m from msw@chromium.org
thanks a lot for thorough review for such big CL! https://codereview.chromium.org/916203002/diff/380001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/916203002/diff/380001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:450: // NOTE: The value of these accessors may contain stale values. Please On 2015/02/14 02:02:15, msw wrote: > nit: "The values of these accessors may be stale." Done.
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/916203002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by oshima@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/916203002/400001
Message was sent while issue was closed.
Committed patchset #8 (id:400001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/0a243a4040f27b7be0f4c5bb92da4e2893f03b1c Cr-Commit-Position: refs/heads/master@{#316375} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
