Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16867016:
Windows implementation of multiline RenderText (Closed)
|
Created:
7 years, 6 months ago by ckocagil Modified:
7 years, 3 months ago CC:
chromium-reviews, tfarina Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionWindows implementation of multiline RenderText
Multi-line text rendering in the Chromium UI is currently done by slicing the string into multiple lines by using text metrics from cross-platform libraries and rendering text by using different RenderText instances - one for each line. This approach has poor performance and is very bug-prone.
This CL adds a cross-platform interface and data structures to RenderText to support multi-line text rendering. This CL also implements the required platform-specific bits for Windows. Support for other platforms will be implemented in subsequent CLs.
Multi-line rendering as implemented in this CL is limited. Newline characters are ignored. RTL and complex scripts are supported, while there are be issues with selection highlights. Text-space <-> view-space mappings do not support RTL/complex scripts.
BUG=248597
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223394
Patch Set 1 #Patch Set 2 : added switch #Patch Set 3 : fix alignments #Patch Set 4 : selection highlight #Patch Set 5 : api cleanup, moved stuff from _win #Patch Set 6 : refactored #Patch Set 7 : fix ComputeLines, add checkbox #Patch Set 8 : fix ComputeLines and rects #
Total comments: 113
Patch Set 9 : comments addressed #Patch Set 10 : fit whole runs, remove single-line stuff #
Total comments: 63
Patch Set 11 : refactored ComputeLines, addressed comments #Patch Set 12 : rebased #
Total comments: 4
Patch Set 13 : rebased; lazy line break calculation; refactored LineBreaker #
Total comments: 26
Patch Set 14 : comments addressed #
Total comments: 30
Patch Set 15 : rebased #Patch Set 16 : Alexei's comments #
Total comments: 2
Patch Set 17 : Alexei's comments 2 #
Total comments: 130
Patch Set 18 : fix existing tests #Patch Set 19 : added unittest #Patch Set 20 : comments addressed #
Total comments: 22
Patch Set 21 : comments #Patch Set 22 : Mike's comments #
Total comments: 29
Patch Set 23 : Mike's comments 2 #
Total comments: 70
Patch Set 24 : Mike's comments 3 #Patch Set 25 : Alexei's comments #
Total comments: 4
Patch Set 26 : Alexei and Mike's comments #Patch Set 27 : removed GetMultilineTextSize #
Total comments: 115
Patch Set 28 : Mike's comments #
Total comments: 20
Patch Set 29 : Mike's comments 2 #
Total comments: 14
Patch Set 30 : nits; rebased #Patch Set 31 : fix other platforms #Patch Set 32 : remaining style nit #
Total comments: 14
Patch Set 33 : Scott's comments #Patch Set 34 : Scott's comments 2 #Patch Set 35 : fix init order #Patch Set 36 : min height/baseline; update tests #Messages
Total messages: 71 (0 generated)
This CL should be ready for review now.
I'm going on vacation this afternoon, so I'll defer the rest of the review to msw@. Some comments below and I also suggest adding some unit tests for this new functionality. Thanks! https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:313: // TODO: we might be displaying obscured_text... also move somewhere better. I think this should be done at the end of UpdateLayoutText() with GetLayoutText() as input. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:891: // TODO: implement multiline TODO(ckocagil) https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:902: x -= LineWidth(line); Can you refactor this code to not call LineWidth() twice per line? https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:115: struct LineSegment { This should live in the class unless you're planning to expose this externally. I suggest in the protected section. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:376: int LineWidth(int line) { This is not a trivial method so the implementation should live in the .cc file. Also, should be named GetLineWidth() and have a comment. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:378: for (size_t i = 0; i < lines()[line].size(); ++i) Add a DCHECK() that |line| is within a valid range. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:618: std::vector<ScopedVector<internal::LineSegment> > lines_; Can you use a regular vector instead of a ScopedVector to represent a line? (I guess you can't because there's a platform specific subclass of LineSegment). Also, consider making a typedef Line. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:620: bool lines_valid_; Add a comment. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:128: ui::Range CharRangeToGlyphRange(const ui::Range& range, Add a comment. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:129: internal::TextRun* run) { This should be const and possibly by ref to prevent NULL from being passed. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:133: bool rtl = run->script_analysis.fRTL; Inline this, since you only use it once. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:135: int end_char = range.end(); Nit: You don't need local vars for start_char and end_char, just inline these below. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:136: int start, end; Nit: Separate lines. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:217: return Size(string_size_.width() + 1, string_size_.height()); Can you explain why this is needed (and why it wasn't before)? Also, remember that cursor may not be visible, so at the very least this should be conditional. Also, TODO should be TODO(ckocagil) https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:226: string_size_.height() * lines().size()); Are all lines guaranteed to have the same height? What if one line has taller runs then the others? https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:490: std::unique(breaks.begin(), breaks.end()); Is this needed? If so, perhaps add a test that will cover this case. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:516: for (size_t c = 0; c < run->range.length(); ++c) { Use i, j, k for loops, so this should be j and the inner loop again should be k. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:518: while (offset >= *(last_break + 1)) { Can you you use ui/gfx/break_list.h or are the semantics different here? (And if they are, do they have to be?) https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:645: for (size_t line = 0; line < lines().size(); ++line) { Ideally, I'd like to see a single codepath that handles both the multline and single line scenarios (i.e. the singleline scenario will just be the multiline scenario with a single line), but I'm okay with keeping them separate at this stage (as this is still being developed). Can you add a TODO to unify them in the future? (Also, with that TODO in place, I won't ask you to try to refactor things to make the two loops share common code since the eventual goal would be them being unified). https://codereview.chromium.org/16867016/diff/39001/ui/views/examples/multili... File ui/views/examples/multiline_example.cc (right): https://codereview.chromium.org/16867016/diff/39001/ui/views/examples/multili... ui/views/examples/multiline_example.cc:44: render_text_->MoveCursorTo(gfx::SelectionModel(ui::Range(1, 11), Nit: Extract the range to a local variable. https://codereview.chromium.org/16867016/diff/39001/ui/views/examples/multili... ui/views/examples/multiline_example.cc:158: container_->Layout(); // TODO: we seem to require laying out twice. why? TODO -> TODO(ckocagil) Also, consider filing a bug to track this so that it's not forgotten.
You should sync and merge your changes to ToT (past 209286). As I mention in an inline comment, we should probably step back and outline a high-level strategy before committing to a character-by-character layout pass. It might be helpful to define cross-platform interfaces or abstract classes to organize and manage some of the concepts here (adding a Line struct, expanding upon and formalizing the LineSegment struct, and possibly making a cross-platform TextRun). These could cache their bounds and other relevant information for a more cross-platform Layout and ComputeLines pass. I'm starting to wonder if we should even use ICU for text itemization, rather than leaving that up to Pango and Uniscribe (this would yield more cross-platform code and may not even be too difficult, though I may be getting ahead of myself). Anyway, try to digest the more high-level comments here, and let's chat soon. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:721: int height = GetStringSize().height(); This is fine for the single line case, but each line in multi-line should have its height computed as the max of the LineSegments' heights. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:730: ComputeLines(); EnsureLayout should ComputeLines as needed, don't invoke it explicitly here. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:732: for (size_t line = 0; line < lines().size(); ++line) { Add a comment to explain what's happening here, it seems odd. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:739: y + line * height, intersection.length(), height); You need to use the proper per-line heights; at least add a TODO. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:740: rect += GetTextOffset(LineWidth(line)); Cache the line's offset in the outer loop. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:883: if (!multiline()) Why ignore the display offset for multi-line? Eventually we'll need to be able to scroll multi-line text too. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:898: ComputeLines(); Don't explicitly call this here, EnsureLayout or [D]CHECK(lines_valid_). https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:905: return Point(x, point.y() + line * GetStringSize().height()) + Again, this needs per-line heights. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:910: nit: remove blank line. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:913: offset.set_x(display_rect().width() - (multiline() ? line_width Always use |line_width|, you pass in GetContentWidth() correctly. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:115: struct LineSegment { Add a struct comment to describe what this is. (a reader might wonder how this differs from a run) https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:115: struct LineSegment { On 2013/07/09 15:35:13, Alexei Svitkine wrote: > This should live in the class unless you're planning to expose this externally. > > I suggest in the protected section. I disagree; SkiaTextRenderer and StyleIterator aren't used elsewhere either, but we shouldn't make all of these protected to RenderText. I think the internal namespace is fine for now, and I'd rather eventually see these components moved to other files, instead of cramming everything in the RenderText class. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:300: virtual Size RenderText::GetMultilineTextSize() = 0; Remove "RenderText::" and add a comment (explain the width cap). https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:372: void set_lines_valid(bool lines_valid) { nit: rename the param to |valid| to fit this on one line. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:447: virtual void ComputeLines() = 0; Add a comment, move this above DrawVisualText (it's more like Layout). https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:458: Vector2d GetTextOffset(int line_width); Comment on what |line_width| is used for here. It's needed to get the offsets of individually centered lines, right? But it's also used to get the more general offset for a whole block of left/right-aligned text with GetContentWidth, right? Rename this and the param for GetAlignmentOffset |text_width| or simply |width|. I would really suggest that you leave these functions as-is and make per-line offsets a separate matter dealt with elsewhere (like each Line storing it's bounds, etc.). https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:465: std::vector<Rect> RangeToViewRects(const ui::Range& x, int y); nit: Comment on what this does and what the args are. |y| is only ever 0 so far, is it needed? https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:469: Vector2d GetAlignmentOffset(int line_width); Ditto comment from GetTextOffset: Comment and rename. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:584: // Whether we should break the text to multiple lines. optional nit: I prefer avoiding "we" in comments. Consider "Whether the text should be broken into multiple lines." Constider mentioning the use of the display rect's width. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:618: std::vector<ScopedVector<internal::LineSegment> > lines_; I'm wondering if it might help in the long run to use a BreakList for merging/splitting existing lines rather than frequently reconstructing the Lines/LineSegments. I don't have a solid recommendation but maybe you already thought about this. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:131: DCHECK_LE(range.end(), run->range.length()); nit: Can you just [D]CHECK(run->range.Contains(range)) instead? You might also need to handle reversed or empty |range|s or CHECK against them. You might want to favor CHECK over DCHECK to avoid using invalid memory. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:136: int start, end; On 2013/07/09 15:35:13, Alexei Svitkine wrote: > Nit: Separate lines. Also, explicitly init each to 0. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:143: end = end_char < run->glyph_count ? run->logical_clusters[end_char] Shouldn't this be end_char < run->range.length() ? https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:217: return Size(string_size_.width() + 1, string_size_.height()); On 2013/07/09 15:35:13, Alexei Svitkine wrote: > Can you explain why this is needed (and why it wasn't before)? > > Also, remember that cursor may not be visible, so at the very least this should > be conditional. > > Also, TODO should be TODO(ckocagil) I think it's already handled by RenderText::GetContentWidth, no? https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:224: ComputeLines(); Do this as needed in EnsureLayout(). https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:409: if (!bounds.empty() && bounds.back().GetMax() == range_x.GetMin()) { This will miss cases where range_x.GetMax() == bounds.back().GetMin(). (I'm not sure that happens in practice, but it's worth investigating) https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:458: set_lines_valid(false); // TODO: do we need this? I think lines_valid_ should implicitly be part of needs_layout_. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:479: std::vector<ScopedVector<internal::LineSegment> > lines; Should these be LineSegmentWin? https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:487: breaks.push_back(0); // TODO: necessary? Probably not necessary, resolve the TODO. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:488: breaks.push_back(-1); What's this unsigned wraparound max size_t used for? The text length should never equal/exceed this value. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:501: for (size_t i = 0; i < runs_.size(); ++i) { This function needs comments explaining what it's doing, it's rather tough to follow. I think formalizing a Line structure and potentially making a cross-platform notion of a TextRun could yield some clarity here. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:502: // TODO: might need |line_break = range.start| to break at run beginnings nit: resolve TODO https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:513: typedef std::pair<internal::LineSegment*, int> RtlWidth; Comment here, what's an RtlWidth? https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:516: for (size_t c = 0; c < run->range.length(); ++c) { There must be a better approach than fitting lines character by character. First, the code should try fitting entire runs into a line. Then, only if a run exceeds some substantial remaining available width of the line, try splitting the run to fit some portion of its content onto that line before moving to the next line. You could either destructively modify the runs to split them up as needed (and ensure they're rebuilt on edits/display_rect.width changes), or if we don't want to split runs, draw different clipped portions of the run for each line it occurs on. Also, it'd probably be worthwhile to do a binary search to find a suitable length of the run to use, and/or breaking by word before breaking by character. And it should start the binary search at some reasonable estimate from the font's average character width and the remaining available width. Overall, I think it'd be best to outline a high level strategy and discuss it with us before implementing the complex and hairy details. This might really be the right way to go, but I think discussing the alternatives would be enlightening. I know you must have put a lot of work into this already, and I'm sorry for letting your work progress this far without more discussion :( That's my failing, not yours! https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:596: ComputeLines(); I think ComputeLines should be done as a step in EnsureLayout. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.h File ui/gfx/render_text_win.h (right): https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.h#... ui/gfx/render_text_win.h:60: struct LineSegmentWin : LineSegment { I think this can be declared in the .cc file. https://codereview.chromium.org/16867016/diff/39001/ui/views/examples/multili... File ui/views/examples/multiline_example.cc (right): https://codereview.chromium.org/16867016/diff/39001/ui/views/examples/multili... ui/views/examples/multiline_example.cc:37: return gfx::Size(render_text_->GetMultilineTextSize().width(), This probably creates an odd feedback loop, since the GetMultilineTextSize width is clamped by the display rect. This should probably just use the StringSize as the preferred width (or determine the text's actual preferred width and height from the unclamped width and height of the text, which may contain newline chars), and support something like View::GetHeightForWidth().
Comments addressed, rebased. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:313: // TODO: we might be displaying obscured_text... also move somewhere better. On 2013/07/09 15:35:13, Alexei Svitkine wrote: > I think this should be done at the end of UpdateLayoutText() with > GetLayoutText() as input. Done. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:721: int height = GetStringSize().height(); On 2013/07/10 04:01:56, msw wrote: > This is fine for the single line case, but each line in multi-line should have > its height computed as the max of the LineSegments' heights. Done. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:730: ComputeLines(); On 2013/07/10 04:01:56, msw wrote: > EnsureLayout should ComputeLines as needed, don't invoke it explicitly here. Done. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:732: for (size_t line = 0; line < lines().size(); ++line) { On 2013/07/10 04:01:56, msw wrote: > Add a comment to explain what's happening here, it seems odd. Done. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:739: y + line * height, intersection.length(), height); On 2013/07/10 04:01:56, msw wrote: > You need to use the proper per-line heights; at least add a TODO. Done, it uses per-line heights now. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:740: rect += GetTextOffset(LineWidth(line)); On 2013/07/10 04:01:56, msw wrote: > Cache the line's offset in the outer loop. Done. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:883: if (!multiline()) On 2013/07/10 04:01:56, msw wrote: > Why ignore the display offset for multi-line? Eventually we'll need to be able > to scroll multi-line text too. I ignored it for now because multi-line vertical scrolling requires further work in UpdateCachedBoundsAndOffset. Added a TODO there. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:891: // TODO: implement multiline On 2013/07/09 15:35:13, Alexei Svitkine wrote: > TODO(ckocagil) Done. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:898: ComputeLines(); On 2013/07/10 04:01:56, msw wrote: > Don't explicitly call this here, EnsureLayout or [D]CHECK(lines_valid_). Done. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:902: x -= LineWidth(line); On 2013/07/09 15:35:13, Alexei Svitkine wrote: > Can you refactor this code to not call LineWidth() twice per line? I will rewrite this method to support RTL soon (before we land this). https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:905: return Point(x, point.y() + line * GetStringSize().height()) + On 2013/07/10 04:01:56, msw wrote: > Again, this needs per-line heights. Done. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:910: On 2013/07/10 04:01:56, msw wrote: > nit: remove blank line. Done. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:913: offset.set_x(display_rect().width() - (multiline() ? line_width On 2013/07/10 04:01:56, msw wrote: > Always use |line_width|, you pass in GetContentWidth() correctly. I now pass 0 for single-line mode. Single-line mode will generate a Line struct in the future and we won't need this conditional expression. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:115: struct LineSegment { On 2013/07/10 04:01:56, msw wrote: > On 2013/07/09 15:35:13, Alexei Svitkine wrote: > > This should live in the class unless you're planning to expose this > externally. > > > > I suggest in the protected section. > > I disagree; SkiaTextRenderer and StyleIterator aren't used elsewhere either, but > we shouldn't make all of these protected to RenderText. I think the internal > namespace is fine for now, and I'd rather eventually see these components moved > to other files, instead of cramming everything in the RenderText class. Keeping it in internal namespace for now. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:300: virtual Size RenderText::GetMultilineTextSize() = 0; On 2013/07/10 04:01:56, msw wrote: > Remove "RenderText::" and add a comment (explain the width cap). Done. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:372: void set_lines_valid(bool lines_valid) { On 2013/07/10 04:01:56, msw wrote: > nit: rename the param to |valid| to fit this on one line. Actually it fits without renaming. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:376: int LineWidth(int line) { On 2013/07/09 15:35:13, Alexei Svitkine wrote: > This is not a trivial method so the implementation should live in the .cc file. > > Also, should be named GetLineWidth() and have a comment. Removed since we now have a Line struct that caches width. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:378: for (size_t i = 0; i < lines()[line].size(); ++i) On 2013/07/09 15:35:13, Alexei Svitkine wrote: > Add a DCHECK() that |line| is within a valid range. This method no longer exists. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:447: virtual void ComputeLines() = 0; On 2013/07/10 04:01:56, msw wrote: > Add a comment, move this above DrawVisualText (it's more like Layout). I moved this to render_text_win since we now call EnsureLayout to compute the lines. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:458: Vector2d GetTextOffset(int line_width); On 2013/07/10 04:01:56, msw wrote: > Comment on what |line_width| is used for here. It's needed to get the offsets of > individually centered lines, right? But it's also used to get the more general > offset for a whole block of left/right-aligned text with GetContentWidth, right? > Rename this and the param for GetAlignmentOffset |text_width| or simply |width|. > I would really suggest that you leave these functions as-is and make per-line > offsets a separate matter dealt with elsewhere (like each Line storing it's > bounds, etc.). I renamed this to |GetLineOffset| and it now takes a line number. |GetAlignmentOffset| now takes a line number as well. These should make things easier since I aim for making everything line-oriented and make the single-line mode use mostly the same code paths as multi-line. I would rather not have each Line store its own view bounds to keep the struct simple. Sounds good? https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:465: std::vector<Rect> RangeToViewRects(const ui::Range& x, int y); On 2013/07/10 04:01:56, msw wrote: > nit: Comment on what this does and what the args are. > |y| is only ever 0 so far, is it needed? Comment added, |y| removed. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:469: Vector2d GetAlignmentOffset(int line_width); On 2013/07/10 04:01:56, msw wrote: > Ditto comment from GetTextOffset: Comment and rename. This method now takes a line number. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:584: // Whether we should break the text to multiple lines. On 2013/07/10 04:01:56, msw wrote: > optional nit: I prefer avoiding "we" in comments. > Consider "Whether the text should be broken into multiple lines." > Constider mentioning the use of the display rect's width. Done. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:618: std::vector<ScopedVector<internal::LineSegment> > lines_; On 2013/07/09 15:35:13, Alexei Svitkine wrote: > Can you use a regular vector instead of a ScopedVector to represent a line? (I > guess you can't because there's a platform specific subclass of LineSegment). I can't, we need pointers as long as we have LineSegmentWin. > Also, consider making a typedef Line. We now have a Line struct. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:618: std::vector<ScopedVector<internal::LineSegment> > lines_; On 2013/07/10 04:01:56, msw wrote: > I'm wondering if it might help in the long run to use a BreakList for > merging/splitting existing lines rather than frequently reconstructing the > Lines/LineSegments. I don't have a solid recommendation but maybe you already > thought about this. I'm not sure how this would work but I'm adding a TODO to the document in Google Docs and I'll give it some thought later. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:620: bool lines_valid_; On 2013/07/09 15:35:13, Alexei Svitkine wrote: > Add a comment. Done. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:128: ui::Range CharRangeToGlyphRange(const ui::Range& range, On 2013/07/09 15:35:13, Alexei Svitkine wrote: > Add a comment. Done. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:129: internal::TextRun* run) { On 2013/07/09 15:35:13, Alexei Svitkine wrote: > This should be const and possibly by ref to prevent NULL from being passed. Done. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:131: DCHECK_LE(range.end(), run->range.length()); On 2013/07/10 04:01:56, msw wrote: > nit: Can you just [D]CHECK(run->range.Contains(range)) instead? > You might also need to handle reversed or empty |range|s or CHECK against them. > You might want to favor CHECK over DCHECK to avoid using invalid memory. Done. Done. Done. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:133: bool rtl = run->script_analysis.fRTL; On 2013/07/09 15:35:13, Alexei Svitkine wrote: > Inline this, since you only use it once. Done. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:135: int end_char = range.end(); On 2013/07/09 15:35:13, Alexei Svitkine wrote: > Nit: You don't need local vars for start_char and end_char, just inline these > below. Done. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:136: int start, end; On 2013/07/10 04:01:56, msw wrote: > On 2013/07/09 15:35:13, Alexei Svitkine wrote: > > Nit: Separate lines. > > Also, explicitly init each to 0. Both done. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:143: end = end_char < run->glyph_count ? run->logical_clusters[end_char] On 2013/07/10 04:01:56, msw wrote: > Shouldn't this be end_char < run->range.length() ? Shame on me, this would be a subtle bug. Thanks! Done. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:217: return Size(string_size_.width() + 1, string_size_.height()); On 2013/07/10 04:01:56, msw wrote: > On 2013/07/09 15:35:13, Alexei Svitkine wrote: > > Can you explain why this is needed (and why it wasn't before)? > > > > Also, remember that cursor may not be visible, so at the very least this > should > > be conditional. > > > > Also, TODO should be TODO(ckocagil) > > I think it's already handled by RenderText::GetContentWidth, no? Yes, I later noticed it. Removing the +1 here. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:224: ComputeLines(); On 2013/07/10 04:01:56, msw wrote: > Do this as needed in EnsureLayout(). Done. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:226: string_size_.height() * lines().size()); On 2013/07/09 15:35:13, Alexei Svitkine wrote: > Are all lines guaranteed to have the same height? What if one line has taller > runs then the others? Done, we use per-line heights now. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:409: if (!bounds.empty() && bounds.back().GetMax() == range_x.GetMin()) { On 2013/07/10 04:01:56, msw wrote: > This will miss cases where range_x.GetMax() == bounds.back().GetMin(). > (I'm not sure that happens in practice, but it's worth investigating) Added a DCHECK for this case but it doesn't seem to be hit. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:458: set_lines_valid(false); // TODO: do we need this? On 2013/07/10 04:01:56, msw wrote: > I think lines_valid_ should implicitly be part of needs_layout_. Done. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:479: std::vector<ScopedVector<internal::LineSegment> > lines; On 2013/07/10 04:01:56, msw wrote: > Should these be LineSegmentWin? We now use Line structs here. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:487: breaks.push_back(0); // TODO: necessary? On 2013/07/10 04:01:56, msw wrote: > Probably not necessary, resolve the TODO. We need at least one element, now I'm only adding 0 if braeks is empty. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:488: breaks.push_back(-1); On 2013/07/10 04:01:56, msw wrote: > What's this unsigned wraparound max size_t used for? > The text length should never equal/exceed this value. This was used as an anchor so we didn't go past the vector, but this was too cryptic, I'm now explicitly checking for that case below. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:490: std::unique(breaks.begin(), breaks.end()); On 2013/07/09 15:35:13, Alexei Svitkine wrote: > Is this needed? If so, perhaps add a test that will cover this case. This shouldn't be needed. Removing. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:501: for (size_t i = 0; i < runs_.size(); ++i) { On 2013/07/10 04:01:56, msw wrote: > This function needs comments explaining what it's doing, it's rather tough to > follow. I think formalizing a Line structure and potentially making a > cross-platform notion of a TextRun could yield some clarity here. Added a comment explaining what this function does. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:502: // TODO: might need |line_break = range.start| to break at run beginnings On 2013/07/10 04:01:56, msw wrote: > nit: resolve TODO Done. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:513: typedef std::pair<internal::LineSegment*, int> RtlWidth; On 2013/07/10 04:01:56, msw wrote: > Comment here, what's an RtlWidth? Done. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:516: for (size_t c = 0; c < run->range.length(); ++c) { On 2013/07/10 04:01:56, msw wrote: > There must be a better approach than fitting lines character by character. > First, the code should try fitting entire runs into a line. Then, only if a run > exceeds some substantial remaining available width of the line, try splitting > the run to fit some portion of its content onto that line before moving to the > next line. > > You could either destructively modify the runs to split them up as needed (and > ensure they're rebuilt on edits/display_rect.width changes), or if we don't want > to split runs, draw different clipped portions of the run for each line it > occurs on. > > Also, it'd probably be worthwhile to do a binary search to find a suitable > length of the run to use, and/or breaking by word before breaking by character. > And it should start the binary search at some reasonable estimate from the > font's average character width and the remaining available width. > > Overall, I think it'd be best to outline a high level strategy and discuss it > with us before implementing the complex and hairy details. This might really be > the right way to go, but I think discussing the alternatives would be > enlightening. I know you must have put a lot of work into this already, and I'm > sorry for letting your work progress this far without more discussion :( That's > my failing, not yours! Actually I have thought about most of the stuff you outlined here, including the binary search part (but for some reason I didn't think of adding runs that fit without iterating over their characters). It felt like it would be more complicated without gaining us much in performance. Also, after adding line segment structs I felt comfortable with going ahead with the most straightforward implementation since it would be easy to refactor this method without affecting any other part (and it looks like it is!). Do you think we need these changes before landing this CL? I have no problems with having to refactor this method at all. I can refactor, simplify and do the optimizations you mentioned in this comment. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:516: for (size_t c = 0; c < run->range.length(); ++c) { On 2013/07/09 15:35:13, Alexei Svitkine wrote: > Use i, j, k for loops, so this should be j and the inner loop again should be k. Done. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:518: while (offset >= *(last_break + 1)) { On 2013/07/09 15:35:13, Alexei Svitkine wrote: > Can you you use ui/gfx/break_list.h or are the semantics different here? (And if > they are, do they have to be?) I can use a break list but then every time I use GetBreak it will do a search, right? It should be faster to use a vector and iterator/pointer, but of course I can change it to a break list for clarity if you want. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:596: ComputeLines(); On 2013/07/10 04:01:56, msw wrote: > I think ComputeLines should be done as a step in EnsureLayout. Done. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:645: for (size_t line = 0; line < lines().size(); ++line) { On 2013/07/09 15:35:13, Alexei Svitkine wrote: > Ideally, I'd like to see a single codepath that handles both the multline and > single line scenarios (i.e. the singleline scenario will just be the multiline > scenario with a single line), but I'm okay with keeping them separate at this > stage (as this is still being developed). > > Can you add a TODO to unify them in the future? > > (Also, with that TODO in place, I won't ask you to try to refactor things to > make the two loops share common code since the eventual goal would be them being > unified). Done, added TODO. https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.h File ui/gfx/render_text_win.h (right): https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text_win.h#... ui/gfx/render_text_win.h:60: struct LineSegmentWin : LineSegment { On 2013/07/10 04:01:56, msw wrote: > I think this can be declared in the .cc file. Done. https://codereview.chromium.org/16867016/diff/39001/ui/views/examples/multili... File ui/views/examples/multiline_example.cc (right): https://codereview.chromium.org/16867016/diff/39001/ui/views/examples/multili... ui/views/examples/multiline_example.cc:37: return gfx::Size(render_text_->GetMultilineTextSize().width(), On 2013/07/10 04:01:56, msw wrote: > This probably creates an odd feedback loop, since the GetMultilineTextSize width > is clamped by the display rect. This should probably just use the StringSize as > the preferred width (or determine the text's actual preferred width and height > from the unclamped width and height of the text, which may contain newline > chars), and support something like View::GetHeightForWidth(). Done except for the newline characters: this class now supports GetHeightForWidth. https://codereview.chromium.org/16867016/diff/39001/ui/views/examples/multili... ui/views/examples/multiline_example.cc:44: render_text_->MoveCursorTo(gfx::SelectionModel(ui::Range(1, 11), On 2013/07/09 15:35:13, Alexei Svitkine wrote: > Nit: Extract the range to a local variable. Done. https://codereview.chromium.org/16867016/diff/39001/ui/views/examples/multili... ui/views/examples/multiline_example.cc:158: container_->Layout(); // TODO: we seem to require laying out twice. why? On 2013/07/09 15:35:13, Alexei Svitkine wrote: > TODO -> TODO(ckocagil) > > Also, consider filing a bug to track this so that it's not forgotten. Done. I don't think I will forget this, resolving these TODOs will be the first thing I'll work on after landing this CL.
Making good progress, keep it up! https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/39001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:458: Vector2d GetTextOffset(int line_width); On 2013/07/13 16:05:10, ckocagil wrote: > On 2013/07/10 04:01:56, msw wrote: > > Comment on what |line_width| is used for here. It's needed to get the offsets > of > > individually centered lines, right? But it's also used to get the more general > > offset for a whole block of left/right-aligned text with GetContentWidth, > right? > > Rename this and the param for GetAlignmentOffset |text_width| or simply > |width|. > > I would really suggest that you leave these functions as-is and make per-line > > offsets a separate matter dealt with elsewhere (like each Line storing it's > > bounds, etc.). > > I renamed this to |GetLineOffset| and it now takes a line number. > |GetAlignmentOffset| now takes a line number as well. These should make things > easier since I aim for making everything line-oriented and make the single-line > mode use mostly the same code paths as multi-line. I would rather not have each > Line store its own view bounds to keep the struct simple. Sounds good? Sounds good. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:711: std::vector<Rect> RenderText::RangeToViewRects(const ui::Range& x) { This function should be defined in the order it's declared in the header (move below ToViewPoint) https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:716: rects.push_back(Rect(ToViewPoint(start), Size(x.length(), nit: inline start and break the line before 'Size(' https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:881: offset.Add(Vector2d(0, lines()[line].preceding_heights)); Does this need to add a per-line horizontal offset? https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:907: int width = lines()[line].width; nit: make this const, maybe DCHECK lines_ validity. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:122: Line() : width(0), nit: make this all one line or indent intitializers to match |width|. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:135: segments.swap(other.segments); Swap makes this destructive to the right hand side of an assignment. Can you avoid that, or is this okay because it's only used for STL containers? (if so note that in a comment). https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:140: int height; // Maximum text height of all runs on this line. See Yuki's ongoing work to find common heights and baselines in: https://codereview.chromium.org/19352002/ We'll want to use that logic to find per-line values. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:470: Vector2d GetLineOffset(int line); Rename to |line_number| to clarify the arg; and use size_t. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:473: // Handles the display area, display offset, and the application LTR/RTL mode. Update the comment, to reflect the new multi-line handling. Consider: "Handles display area, display offset, LTR/RTL, and multi-line." https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:479: std::vector<Rect> RangeToViewRects(const ui::Range& x); I'm trying to think of a better name, maybe: TextBoundsToViewBounds? https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:483: Vector2d GetAlignmentOffset(int line); Ditto: Rename to |line_number| to clarify the arg; and use size_t. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:637: // Whether the current |lines_| is valid. |lines_| is invalidated whenever Since we reconstruct |lines_| on ComputeLines, can we just use an empty vector via lines_.clear() to mark |lines_| invalid? https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:477: if (!lines_valid()) { Why is this a separate step at all? I figure multi-line layout should be part of the layout process. Are you worried about display_rect changes that would invalidate only the lines and not the text-space layout? I guess that's valid, but deserves a comment. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:489: int max_width = display_rect().width(); nit: const. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:496: std::sort(breaks.begin(), breaks.end()); Is this necessary? The BreakIterator advancement should keep them in order, and 0 is only added if there are no other breaks. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:517: for (size_t i = 0; i < runs_.size(); ++i) { This whole loop is still pretty difficult to follow, even with the top comment. Though the behavior is complex, additional comments throughout the loop explaining each step could add a lot of clarity. It might help to factor out parts of this code into separate functions and ensure all variable names are consistent and clear. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:520: if (!multiline() || line_x + run->width <= max_width) { nit: an example comment here would be: // The whole run fits on this line; add a corresponding line segment. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:527: lines.back().height = std::max(lines.back().height, This height and baseline calculation should work similar to https://codereview.chromium.org/19352002/diff/1007/ui/gfx/font_list.cc (I suppose we'd need to track each line's max ascent too?) https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:539: int break_x = 0; Comment on what this is. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:547: // To find the text coordinates of RTL LineSegments, store their widths in a I'm still having trouble understanding exactly how this works. I wonder why an LTR run in a predominantly RTL string doesn't need this same process (e.g. positioning an english phrase LineSegment amid an arabic sentence). Could you enlighten me here, extrapolate on this comment (perhaps with a diagram), and/or revise the code? https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:552: for (size_t j = 0; j < run->range.length(); ++j) { I think you'll need to check IsCursorablePosition or use a char iterator or something that actually breaks by characters instead of incrementing j by one. This will need to avoid advancing or rolling back by half of a surrogate pair, etc. If it helps, you could abandon per-character fitting for now and only break on line breaks. That'd cause excessively long words to overflow the display rect, but I think it'd solve the most common and simple line wrapping behavior. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:554: while (last_break != &breaks.back() && offset >= *(last_break + 1)) { Comment on what this is doing; it's not obvious. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:556: break_x = 0; Must this be reset in the loop to correspond with break incrementation? https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:559: ui::Range glyphs = CharRangeToGlyphRange(ui::Range(j, j + 1), *run); CharRangeToGlyphRange is a decent example of a simple abstraction that really helps clarify the calling code. I think the rest of this code would greatly benefit from additional helper function building blocks, even if they're only called once. For example, wouldn't it be awesome to have a function that takes a char index and a width, and returns the next LineSegment that fits within the width? That's a reasonably small and self-contained task that could make this code simpler, and not be too complex itself. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:569: if (line_x > max_width) { Could this check "|| j == (run.range.length() - 1)" to add the last LineSegment of a multi-line run onto the current line? https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:575: } else if (line_x != char_width) { nit: should this be line_x > char_width? https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:579: } // else: Assume the current character fits; continue from next line. Does this mean that the if last condition failed then a single char did not fit on the line? Can you clarify and/or move this comment? I don't quite follow. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:607: if (segment_start_char != run->range.end()) { Add a blank line above and comment here, what is this doing? Is it adding the final remaining run characters that didn't fit on the previous line to the current Line's first line segment? I'm not sure I follow, sorry. This code would be much clearer and more maintainable with high-level comments. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:663: // TODO(ckocagil): necessary? maybe prevent this from happening. Does it currently happen? Can you just [D]CHECK against it if not? https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:669: DCHECK_LT(segment->char_pos.start(), Did you mean segment->char_pos.end()? https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:670: run->range.start() + run->glyph_count); Did you mean run->range.start() + run->range.length()? https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:671: int start_char = segment->char_pos.start() - run->range.start(); nit: const here and for the three ints below too. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:675: int start = glyphs.start(); Consider just inlining use of the |glyphs| range if it's not too ugly. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:677: DCHECK_LE(start, end); Will this code work okay if start == end? https://codereview.chromium.org/16867016/diff/57001/ui/views/examples/multili... File ui/views/examples/multiline_example.cc (right): https://codereview.chromium.org/16867016/diff/57001/ui/views/examples/multili... ui/views/examples/multiline_example.cc:53: ui::Range test_range(1, 11); nit: const. and use this for lines 59 and 60. https://codereview.chromium.org/16867016/diff/57001/ui/views/examples/multili... ui/views/examples/multiline_example.cc:131: layout->AddView(new View); I think you can skip a column instead of adding a dummy view. https://codereview.chromium.org/16867016/diff/57001/ui/views/examples/multili... ui/views/examples/multiline_example.cc:162: if (label_checkbox_->checked()) { Could you set the Label text to empty or the textfield text instead of swapping the Label with a dummy View? https://codereview.chromium.org/16867016/diff/57001/ui/views/examples/multili... File ui/views/examples/multiline_example.h (right): https://codereview.chromium.org/16867016/diff/57001/ui/views/examples/multili... ui/views/examples/multiline_example.h:43: View* container_; Can you use a more descriptive name?
Refactored the horrible ComputeLines and addressed comments. Please review! https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:711: std::vector<Rect> RenderText::RangeToViewRects(const ui::Range& x) { On 2013/07/17 06:47:18, msw wrote: > This function should be defined in the order it's declared in the header (move > below ToViewPoint) Done. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:716: rects.push_back(Rect(ToViewPoint(start), Size(x.length(), On 2013/07/17 06:47:18, msw wrote: > nit: inline start and break the line before 'Size(' Done. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:881: offset.Add(Vector2d(0, lines()[line].preceding_heights)); On 2013/07/17 06:47:18, msw wrote: > Does this need to add a per-line horizontal offset? Which horizontal offset? I don't see your point. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:907: int width = lines()[line].width; On 2013/07/17 06:47:18, msw wrote: > nit: make this const, maybe DCHECK lines_ validity. Done. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:122: Line() : width(0), On 2013/07/17 06:47:18, msw wrote: > nit: make this all one line or indent intitializers to match |width|. Done. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:135: segments.swap(other.segments); On 2013/07/17 06:47:18, msw wrote: > Swap makes this destructive to the right hand side of an assignment. Can you > avoid that, or is this okay because it's only used for STL containers? (if so > note that in a comment). Yes, these are for STL containers only. I'm adding a comment for now. One way to avoid this is to keep "ScopedVector<Line>" instead of "std::vector<Line>" which would allow us to disallow copying and assigning. Let me know if you think the latter solution would be better. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:470: Vector2d GetLineOffset(int line); On 2013/07/17 06:47:18, msw wrote: > Rename to |line_number| to clarify the arg; and use size_t. Done. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:473: // Handles the display area, display offset, and the application LTR/RTL mode. On 2013/07/17 06:47:18, msw wrote: > Update the comment, to reflect the new multi-line handling. Consider: > "Handles display area, display offset, LTR/RTL, and multi-line." Done. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:479: std::vector<Rect> RangeToViewRects(const ui::Range& x); On 2013/07/17 06:47:18, msw wrote: > I'm trying to think of a better name, maybe: TextBoundsToViewBounds? Changed to TextBoundsToViewBounds. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:483: Vector2d GetAlignmentOffset(int line); On 2013/07/17 06:47:18, msw wrote: > Ditto: Rename to |line_number| to clarify the arg; and use size_t. Done. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:637: // Whether the current |lines_| is valid. |lines_| is invalidated whenever On 2013/07/17 06:47:18, msw wrote: > Since we reconstruct |lines_| on ComputeLines, can we just use an empty vector > via lines_.clear() to mark |lines_| invalid? Done. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:477: if (!lines_valid()) { On 2013/07/17 06:47:18, msw wrote: > Why is this a separate step at all? I figure multi-line layout should be part of > the layout process. Are you worried about display_rect changes that would > invalidate only the lines and not the text-space layout? I guess that's valid, > but deserves a comment. Yes, the separate step is to avoid itemizing, glyph generation and other stuff every time we resize the display_rect(). Added a comment. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:483: void RenderTextWin::ComputeLines() { I refactored this method, sorry for not doing this earlier. Please review the new version, thanks! https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:663: // TODO(ckocagil): necessary? maybe prevent this from happening. On 2013/07/17 06:47:18, msw wrote: > Does it currently happen? Can you just [D]CHECK against it if not? Done. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:669: DCHECK_LT(segment->char_pos.start(), On 2013/07/17 06:47:18, msw wrote: > Did you mean segment->char_pos.end()? I changed this to use ui::Range::Contains. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:670: run->range.start() + run->glyph_count); On 2013/07/17 06:47:18, msw wrote: > Did you mean run->range.start() + run->range.length()? Same as above. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:671: int start_char = segment->char_pos.start() - run->range.start(); On 2013/07/17 06:47:18, msw wrote: > nit: const here and for the three ints below too. I inlined these three ints. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:675: int start = glyphs.start(); On 2013/07/17 06:47:18, msw wrote: > Consider just inlining use of the |glyphs| range if it's not too ugly. Done. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:677: DCHECK_LE(start, end); On 2013/07/17 06:47:18, msw wrote: > Will this code work okay if start == end? It now skips this case and DCHECKs that the segment wasn't empty (which is something I don't want to see). https://codereview.chromium.org/16867016/diff/57001/ui/views/examples/multili... File ui/views/examples/multiline_example.cc (right): https://codereview.chromium.org/16867016/diff/57001/ui/views/examples/multili... ui/views/examples/multiline_example.cc:53: ui::Range test_range(1, 11); On 2013/07/17 06:47:18, msw wrote: > nit: const. and use this for lines 59 and 60. Done. https://codereview.chromium.org/16867016/diff/57001/ui/views/examples/multili... ui/views/examples/multiline_example.cc:131: layout->AddView(new View); On 2013/07/17 06:47:18, msw wrote: > I think you can skip a column instead of adding a dummy view. Done. https://codereview.chromium.org/16867016/diff/57001/ui/views/examples/multili... ui/views/examples/multiline_example.cc:162: if (label_checkbox_->checked()) { On 2013/07/17 06:47:18, msw wrote: > Could you set the Label text to empty or the textfield text instead of swapping > the Label with a dummy View? Done. https://codereview.chromium.org/16867016/diff/57001/ui/views/examples/multili... File ui/views/examples/multiline_example.h (right): https://codereview.chromium.org/16867016/diff/57001/ui/views/examples/multili... ui/views/examples/multiline_example.h:43: View* container_; On 2013/07/17 06:47:18, msw wrote: > Can you use a more descriptive name? Changed to |top_container_|.
I synced and rebased after landing the color drawing patch. Could you also review the merged RenderTextWin::DrawVisualText()? Other parts of the code haven't changed.
(I'm back from vacation now.) I haven't had a chance to fully review this, but I think it would be worthwhile to first land a simple line splitting algorithm (using only newlines via base::i18n::BreakIterator::BREAK_NEWLINE) along with the changes needed to the classes to support multi-line operation (i.e. the new structs) and then follow-up with another CL that actually implements display_rect based line breaking. That way, the more complicated line breaking logic can be reviewed separately from the infrastructure changes (new structs, etc.) to keep track of and draw multipline lines of text. https://codereview.chromium.org/16867016/diff/88001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/88001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:1027: base::i18n::BreakIterator::BREAK_LINE); There's a comment in download_item_view.cc that BREAK_LINE can be slow (especially for CJK). As such, I'm worried that you're now doing it unconditionally every time we do text layout. At the very least, you shouldn't do it unless the instance is multi-line enabled. But also, it might make sense to do it lazily completely. For example, if your algorithm first tries to add a run completely to see if it fits in the current line and if it does, then there's no line break analysis needed to be done on that run at all. So perhaps this should be done per-run on an as-needed basis. https://codereview.chromium.org/16867016/diff/88001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/88001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:281: while (multiline && It would be less confusing if the multline check was in an if statement surrounding this code. Consider refactoring the while loop out into a separate function.
> (I'm back from vacation now.) > > I haven't had a chance to fully review this, but I think it would be worthwhile > to first land a simple line splitting algorithm (using only newlines via > base::i18n::BreakIterator::BREAK_NEWLINE) along with the changes needed to the > classes to support multi-line operation (i.e. the new structs) and then > follow-up with another CL that actually implements display_rect based line > breaking. > > That way, the more complicated line breaking logic can be reviewed separately > from the infrastructure changes (new structs, etc.) to keep track of and draw > multipline lines of text. I tried to simplify the current code by breaking only at newlines. It didn't work out. The most complicated part of the LineBreaker is segment calculation, which is something we can't simplify by only breaking at newlines. The break-at-width logic itself is actually simple. https://codereview.chromium.org/16867016/diff/88001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/88001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:1027: base::i18n::BreakIterator::BREAK_LINE); On 2013/07/22 20:52:05, Alexei Svitkine wrote: > There's a comment in download_item_view.cc that BREAK_LINE can be slow > (especially for CJK). > > As such, I'm worried that you're now doing it unconditionally every time we do > text layout. > > At the very least, you shouldn't do it unless the instance is multi-line > enabled. But also, it might make sense to do it lazily completely. For example, > if your algorithm first tries to add a run completely to see if it fits in the > current line and if it does, then there's no line break analysis needed to be > done on that run at all. So perhaps this should be done per-run on an as-needed > basis. Done, it's completely lazy now. https://codereview.chromium.org/16867016/diff/88001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/88001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:281: while (multiline && On 2013/07/22 20:52:05, Alexei Svitkine wrote: > It would be less confusing if the multline check was in an if statement > surrounding this code. Consider refactoring the while loop out into a separate > function. Done (well, it's a ternary operation now). Done.
Looking better, some more comments your way. https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:115: struct LineSegment { Add a brief comment explaining this class. https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:121: Line() : width(0), height(0), preceding_heights(0), baseline(0) {} Please define the bodies of these in the .cc file. https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:137: int height; // Maximum text height of all runs on this line. Please move the comments on their own line before the variable they're commenting. (throughout) https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:211: void SetMultiline(bool multiline); Add a TODO comment here that this currently only supported on Windows. https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:168: const BreakList<size_t>& breaks, size_t start_char, One parameter per line in the declaration, please. https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:171: int* word_rollback_width) { This is quite a lot of parameters to pass to this function. Maybe you can make a struct RunBreakState that keeps all of these around (with line comments for the members instead of commenting them above the signature). Or perhaps make another helper class (RunBreaker) that keeps this state around and is responsible for breaking a single run (rather than a line). WDYT? https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:271: std::vector<internal::Line>* lines) { Instead of passing all of these params to a single Break() function, how about making the API AddRun(run) and leave the iteration logic to the caller? This way, the visual_to_logical stuff mapping and iteration can left to the caller and this can process runs as they come, with maybe a Finalize() method that returns the lines as output. In this case, add the other parameters like |words|, |max_width|, etc to the ctor. https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:427: return Size(string_size_.width(), string_size_.height()); This is equivalent to return string_size_;, no? https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:430: Size RenderTextWin::GetMultilineTextSize() { I think you should make GetStringSize() return the multiline size by default when in multi-line mode. i.e. remove this extra method and fold the logic. (Else, you'd at least need to add implementations of this to the other render text subclasses.) https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:690: set_lines(&lines); Can you add a DCHECK() that !lines().empty()? (Else this function will keep getting called from EnsureLayout() if it can return an empty list of lines).
Thanks for the nice review! https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:115: struct LineSegment { On 2013/08/08 15:01:03, Alexei Svitkine wrote: > Add a brief comment explaining this class. Done. https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:121: Line() : width(0), height(0), preceding_heights(0), baseline(0) {} On 2013/08/08 15:01:03, Alexei Svitkine wrote: > Please define the bodies of these in the .cc file. Done. https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:137: int height; // Maximum text height of all runs on this line. On 2013/08/08 15:01:03, Alexei Svitkine wrote: > Please move the comments on their own line before the variable they're > commenting. (throughout) Done. https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:211: void SetMultiline(bool multiline); On 2013/08/08 15:01:03, Alexei Svitkine wrote: > Add a TODO comment here that this currently only supported on Windows. Done. https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:168: const BreakList<size_t>& breaks, size_t start_char, On 2013/08/08 15:01:03, Alexei Svitkine wrote: > One parameter per line in the declaration, please. Done. https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:171: int* word_rollback_width) { On 2013/08/08 15:01:03, Alexei Svitkine wrote: > This is quite a lot of parameters to pass to this function. Maybe you can make a > struct RunBreakState that keeps all of these around (with line comments for the > members instead of commenting them above the signature). Or perhaps make another > helper class (RunBreaker) that keeps this state around and is responsible for > breaking a single run (rather than a line). WDYT? There isn't a lot of state tracking and the function itself is straightforward, so I think adding a class would add unnecessary complexity. I only added a struct that holds the result fields. Does it look better now? https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:271: std::vector<internal::Line>* lines) { On 2013/08/08 15:01:03, Alexei Svitkine wrote: > Instead of passing all of these params to a single Break() function, how about > making the API AddRun(run) and leave the iteration logic to the caller? > > This way, the visual_to_logical stuff mapping and iteration can left to the > caller and this can process runs as they come, with maybe a Finalize() method > that returns the lines as output. > > In this case, add the other parameters like |words|, |max_width|, etc to the > ctor. Done. https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:427: return Size(string_size_.width(), string_size_.height()); On 2013/08/08 15:01:03, Alexei Svitkine wrote: > This is equivalent to return string_size_;, no? Oops, leftover from before. Done. https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:430: Size RenderTextWin::GetMultilineTextSize() { On 2013/08/08 15:01:03, Alexei Svitkine wrote: > I think you should make GetStringSize() return the multiline size by default > when in multi-line mode. > > i.e. remove this extra method and fold the logic. > > (Else, you'd at least need to add implementations of this to the other render > text subclasses.) But if GetStringSize returns the display width and not single line string width, the controls using RenderText won't be able to know their preferred size. I think users of RenderText will always need to know 1) the single-line size of the text (for their preferred size) and 2) the multi-line height (for GetHeightForWidth). I will add empty NOTIMPLEMENTED() methods to all platforms that need them, probably after the review to not make this CL any harder to review. https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:690: set_lines(&lines); On 2013/08/08 15:01:03, Alexei Svitkine wrote: > Can you add a DCHECK() that !lines().empty()? (Else this function will keep > getting called from EnsureLayout() if it can return an empty list of lines). Done.
I'm still reviewing this code, but could you work on some basic tests for this in the meantime?
Thanks for addressing my comments from before, I have some more for you. https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:181: BreakRunResults BreakRunAtWidth(const internal::TextRun& run, Returning the whole struct by value is a bit more expensive than I'd like. How about making the function return a bool indicating whether an overflow occurred (i.e. same as your current |overflowed| member) and taking a pointer to an OverflowInfo* struct that has the other variables. https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:184: int width_cap) { Nit: width_cap -> available_width https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:186: BreakRunResults results; Either add a default ctor that initializes the members or memset the struct to 0 at the start of the function. https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:191: I'd like like to see this code try the whole run first, before resorting to per-word iteration. Or at the very least, add a TODO about it. https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:193: size_t next_word = std::max(breaks.GetBreak(i)->first, start_char); You only need to do this if i < previous next_word value, right? If so, move next_word outside the loop and put this in an if statement. https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:299: lines->swap(lines_); Add a DCHECK() that !lines_.empty(). https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:309: int run_x_; // Beginning of the RTL run in text-space. It's a struct, so no need for trailing _'s. Although, see my comments below about getting rid of this struct. https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:327: if (line_x_ > 0 || results.word_rollback_width < results.width) I don't understand this logic. Why do we Rollback() if line_x_ > 0 or even results.word_rollback_width < results.width. If the word is smaller than the width without it? It's not clear to me. Can you clarify how this works? Thanks. https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:343: while (!rtl_data_.segments_.empty()) { I think it would be cleaner to just iterate over them by index in reverse order and then just .clear() them. Although, see my comment below in CommitSegment() about not needing rtl segments vector. https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:392: rtl_data_.segments_.push_back(segment); It seems rtl_data_.segments_ is just a list of segments for this run, in the same order as they are in lines_.back().segments. So I think it's not actually necessary to store them in a separate vector. Instead, you just need to store the index of the first segment for the current run. Then, in PopRtl() you can simply iterate via: for (size_t i = lines_.back().segments.size() - 1; i >= first_run_segment_; i--) To process them. https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:407: size_t pos_; Can you document these vars? I'd like to e.g. understand what pos_, current_pos_, text_x_, line_x_ current_delta_ refers to, for example. If there were clear comments about them, it would be much easier to understand. Alternatively or additionally, better names might make this clearer. For example, current_delta_ -> current_width_ (assuming that's what it means) and other such changes. https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:412: RtlData rtl_data_; Hmm, I'm not convinced this needs to be its own struct, if its just one entry here. Just inline the members - i.e. rtl_run_x_ and rtl_segments_, with appropriate comments. Although, see my comment above in CommitSegment() about not needing rtl segments vector. https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:702: multiline() ? &GetLineBreaks() : 0); Nit: Align with previous' line's params. https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.h File ui/gfx/render_text_win.h (right): https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.h... ui/gfx/render_text_win.h:95: void ComputeLines(); Can you add a 1-line comment? Thanks.
https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:181: BreakRunResults BreakRunAtWidth(const internal::TextRun& run, On 2013/08/12 20:42:33, Alexei Svitkine wrote: > Returning the whole struct by value is a bit more expensive than I'd like. > > How about making the function return a bool indicating whether an overflow > occurred (i.e. same as your current |overflowed| member) and taking a pointer to > an OverflowInfo* struct that has the other variables. > Done. https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:184: int width_cap) { On 2013/08/12 20:42:33, Alexei Svitkine wrote: > Nit: width_cap -> available_width Done. https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:186: BreakRunResults results; On 2013/08/12 20:42:33, Alexei Svitkine wrote: > Either add a default ctor that initializes the members or memset the struct to 0 > at the start of the function. Done, added memset. https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:191: On 2013/08/12 20:42:33, Alexei Svitkine wrote: > I'd like like to see this code try the whole run first, before resorting to > per-word iteration. > > Or at the very least, add a TODO about it. I added run width checking to LineBreaker::AddRun. Adding that check here would require new parameters and more code here, and I would like to keep this function simple. lgty? https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:193: size_t next_word = std::max(breaks.GetBreak(i)->first, start_char); On 2013/08/12 20:42:33, Alexei Svitkine wrote: > You only need to do this if i < previous next_word value, right? If so, move > next_word outside the loop and put this in an if statement. I did something different, this now stores two BreakList iterators instead of indices. It avoids the GetBreak call in the for loop. https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:299: lines->swap(lines_); On 2013/08/12 20:42:33, Alexei Svitkine wrote: > Add a DCHECK() that !lines_.empty(). Done. https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:309: int run_x_; // Beginning of the RTL run in text-space. On 2013/08/12 20:42:33, Alexei Svitkine wrote: > It's a struct, so no need for trailing _'s. > > Although, see my comments below about getting rid of this struct. Got rid of this struct. https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:327: if (line_x_ > 0 || results.word_rollback_width < results.width) On 2013/08/12 20:42:33, Alexei Svitkine wrote: > I don't understand this logic. Why do we Rollback() if line_x_ > 0 or even > results.word_rollback_width < results.width. If the word is smaller than the > width without it? It's not clear to me. > > Can you clarify how this works? Thanks. This condition is to avoid rolling back the entire text on the line, since that would cause an infinite loop and output infinitely many lines (rollback everything, skip line, rollback everything, skip line, ...). |line_x_ > 0| means there is some text before the current run, so we won't end up with an empty line even if we rollback the entire word. If this condition isn't satisfied, we then check |word_rollback_width < width|, which tells us that the current segment has text other than the last word, so rolling back the word wouldn't leave us with an empty line. The char rollback condition below is similar. https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:343: while (!rtl_data_.segments_.empty()) { On 2013/08/12 20:42:33, Alexei Svitkine wrote: > I think it would be cleaner to just iterate over them by index in reverse order > and then just .clear() them. > > Although, see my comment below in CommitSegment() about not needing rtl segments > vector. Done, it now iterates and .clear()s. https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:392: rtl_data_.segments_.push_back(segment); On 2013/08/12 20:42:33, Alexei Svitkine wrote: > It seems rtl_data_.segments_ is just a list of segments for this run, in the > same order as they are in lines_.back().segments. > > So I think it's not actually necessary to store them in a separate vector. > Instead, you just need to store the index of the first segment for the current > run. > > Then, in PopRtl() you can simply iterate via: > > for (size_t i = lines_.back().segments.size() - 1; i >= first_run_segment_; i--) > > To process them. I realized that |rtl_data_.run_x_| isn't really needed since the first segment's x_pos.start() gives the same value. I'm getting rid of RtlData and moving the vector to LineBreaker. The reason I'm keeping a vector is because the line segments can be on different lines. So if I were to get rid of the vector, I would have to reverse iterate over the lines too. wdyt? https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:407: size_t pos_; On 2013/08/12 20:42:33, Alexei Svitkine wrote: > Can you document these vars? I'd like to e.g. understand what pos_, > current_pos_, text_x_, line_x_ current_delta_ refers to, for example. If there > were clear comments about them, it would be much easier to understand. > > Alternatively or additionally, better names might make this clearer. For > example, current_delta_ -> current_width_ (assuming that's what it means) and > other such changes. Done both. https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:412: RtlData rtl_data_; On 2013/08/12 20:42:33, Alexei Svitkine wrote: > Hmm, I'm not convinced this needs to be its own struct, if its just one entry > here. > > Just inline the members - i.e. rtl_run_x_ and rtl_segments_, with appropriate > comments. > > Although, see my comment above in CommitSegment() about not needing rtl segments > vector. Got rid of the struct. https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:702: multiline() ? &GetLineBreaks() : 0); On 2013/08/12 20:42:33, Alexei Svitkine wrote: > Nit: Align with previous' line's params. Done. https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.h File ui/gfx/render_text_win.h (right): https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.h... ui/gfx/render_text_win.h:95: void ComputeLines(); On 2013/08/12 20:42:33, Alexei Svitkine wrote: > Can you add a 1-line comment? Thanks. Done.
https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:327: if (line_x_ > 0 || results.word_rollback_width < results.width) On 2013/08/13 20:22:28, ckocagil wrote: > On 2013/08/12 20:42:33, Alexei Svitkine wrote: > > I don't understand this logic. Why do we Rollback() if line_x_ > 0 or even > > results.word_rollback_width < results.width. If the word is smaller than the > > width without it? It's not clear to me. > > > > Can you clarify how this works? Thanks. > > This condition is to avoid rolling back the entire text on the line, since that > would cause an infinite loop and output infinitely many lines (rollback > everything, skip line, rollback everything, skip line, ...). > > |line_x_ > 0| means there is some text before the current run, so we won't end > up with an empty line even if we rollback the entire word. If this condition > isn't satisfied, we then check |word_rollback_width < width|, which tells us > that the current segment has text other than the last word, so rolling back the > word wouldn't leave us with an empty line. > > The char rollback condition below is similar. I see. I think I understand how this works now. You call BreakRunAtWidth(), which returns as soon as it finds a character that doesn't fit. Yet, the issue is you want to break by word, rather than character. So to make that work, you have to rollback to the previous word that you got info about from BreakRunAtWidth(). If my understanding is correct, I think it should be possible to simplify this by moving this logic into BreakRunAtWidth(). That is, when BreakRunAtWidth() finds it can't fit any more chars, then it will rollback to the last word internally, returning the new info from there. This way, you can avoid passing back the extra state information about the word rollback position and width (keep this info only in BreakRunAtWidth) and remove the extra Rollback() functions on this class. It may even make sense to make BreakRunAtWidth() a member function on this class so that it can operate on |current_pos_| and such directly - once everything is simplified. Though, this one I'm not 100% sure about. WDYT? https://codereview.chromium.org/16867016/diff/130001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/130001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:402: // end character position. |text_x_| is the text-space and |line_x| is the Nit: |line_x| -> |line_x_|
https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/115001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:327: if (line_x_ > 0 || results.word_rollback_width < results.width) On 2013/08/13 20:56:32, Alexei Svitkine wrote: > On 2013/08/13 20:22:28, ckocagil wrote: > > On 2013/08/12 20:42:33, Alexei Svitkine wrote: > > > I don't understand this logic. Why do we Rollback() if line_x_ > 0 or even > > > results.word_rollback_width < results.width. If the word is smaller than the > > > width without it? It's not clear to me. > > > > > > Can you clarify how this works? Thanks. > > > > This condition is to avoid rolling back the entire text on the line, since > that > > would cause an infinite loop and output infinitely many lines (rollback > > everything, skip line, rollback everything, skip line, ...). > > > > |line_x_ > 0| means there is some text before the current run, so we won't end > > up with an empty line even if we rollback the entire word. If this condition > > isn't satisfied, we then check |word_rollback_width < width|, which tells us > > that the current segment has text other than the last word, so rolling back > the > > word wouldn't leave us with an empty line. > > > > The char rollback condition below is similar. > > I see. I think I understand how this works now. You call BreakRunAtWidth(), > which returns as soon as it finds a character that doesn't fit. Yet, the issue > is you want to break by word, rather than character. So to make that work, you > have to rollback to the previous word that you got info about from > BreakRunAtWidth(). > > If my understanding is correct, I think it should be possible to simplify this > by moving this logic into BreakRunAtWidth(). > > That is, when BreakRunAtWidth() finds it can't fit any more chars, then it will > rollback to the last word internally, returning the new info from there. This > way, you can avoid passing back the extra state information about the word > rollback position and width (keep this info only in BreakRunAtWidth) and remove > the extra Rollback() functions on this class. Good call. Done. > It may even make sense to make BreakRunAtWidth() a member function on this class > so that it can operate on |current_pos_| and such directly - once everything is > simplified. Though, this one I'm not 100% sure about. > > WDYT? I think there is value in having this as a separate, self-contained helper function. It makes the job of LineBreaker much easier without making the class any more complicated. It also doesn't have much to do with lines (it operates on a single run), so I don't think it belongs to LineBreaker. https://codereview.chromium.org/16867016/diff/130001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/130001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:402: // end character position. |text_x_| is the text-space and |line_x| is the On 2013/08/13 20:56:32, Alexei Svitkine wrote: > Nit: |line_x| -> |line_x_| Done.
Thanks, that looks much better now. Here's a couple more comments. (I hope you can forgive me for doing the review incrementally like this, but it took me a while to really understand all of this CL and I have other stuff on my plate as well - so I chose to send you a set of comments as I make them rather than having you wait for me to finish the whole review at once.) https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:255: const internal::TextRun* run; How about instead of storing a pointer to the run, store a run_index? Since this is used only in the draw function of RenderTextWin, it should be able to get back at the actual run pointer from there. If you store only the run_index, then you can actually do it in LineSegment and get rid of LineSegmentWin entirely and in fact no longer allocate LineSegments on the heap - i.e. change ScopedVector<LineSegment> segments; to std::vector<LineSegment>, which should be more efficient. (However, you do now have to pass a run_index to LineBreaker::AddRun(), but I think that's worth doing given the benefits above.) https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:284: CommitSegment(run); Advance() and CommitSegment() always get called together. Can you just merge them?
https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:255: const internal::TextRun* run; On 2013/08/14 12:59:43, Alexei Svitkine wrote: > How about instead of storing a pointer to the run, store a run_index? Since this > is used only in the draw function of RenderTextWin, it should be able to get > back at the actual run pointer from there. > > If you store only the run_index, then you can actually do it in LineSegment and > get rid of LineSegmentWin entirely and in fact no longer allocate LineSegments > on the heap - i.e. change ScopedVector<LineSegment> segments; to > std::vector<LineSegment>, which should be more efficient. > > (However, you do now have to pass a run_index to LineBreaker::AddRun(), but I > think that's worth doing given the benefits above.) I'm not sure about this, can you guess what kind of data LineSegment implementations on other platforms will need to keep? I haven't checked how RenderText works on other platforms yet. If you're confident that a run index will be sufficient, I can make this change. Otherwise, I can add a TODO to remove the subclass in the future if possible.
On 2013/08/14 14:26:30, ckocagil wrote: > https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.cc > File ui/gfx/render_text_win.cc (right): > > https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... > ui/gfx/render_text_win.cc:255: const internal::TextRun* run; > On 2013/08/14 12:59:43, Alexei Svitkine wrote: > > How about instead of storing a pointer to the run, store a run_index? Since > this > > is used only in the draw function of RenderTextWin, it should be able to get > > back at the actual run pointer from there. > > > > If you store only the run_index, then you can actually do it in LineSegment > and > > get rid of LineSegmentWin entirely and in fact no longer allocate LineSegments > > on the heap - i.e. change ScopedVector<LineSegment> segments; to > > std::vector<LineSegment>, which should be more efficient. > > > > (However, you do now have to pass a run_index to LineBreaker::AddRun(), but I > > think that's worth doing given the benefits above.) > > I'm not sure about this, can you guess what kind of data LineSegment > implementations on other platforms will need to keep? I haven't checked how > RenderText works on other platforms yet. If you're confident that a run index > will be sufficient, I can make this change. Otherwise, I can add a TODO to > remove the subclass in the future if possible. That's a good point - run_index is currently not useful to RenderTextLinux since RenderTextLinux doesn't keep around an array of runs (they're a linked list managed by Pango). I still think it would be good to have a design where we can have a common LineSegment structure between platforms and it seems really all it needs is a pointer to the platform-specific run. In other parts of the codebase we solve this by having a platform-specific typedef - for example NativeView is typedef'd to HWND on Windows vs. NSView* on Mac. Perhaps we can do the same for RenderText runs - i.e. typedef NativeTextRun to internal::TextRun* on Windows and to PangoLayoutRun* on Linux. That way, we can have a common LineSegment struct that has a NativeTextRun member that will be the correct type for each platform (which avoids having to have LineSegmentWin and having to dynamically allocate them all). This will also allow us to possibly move some function signatures to the base RenderText class that take NativeTextRun as a param, with subclasses implementing the specific ones. Mike, what do you think?
On 2013/08/14 19:55:36, Alexei Svitkine wrote: > On 2013/08/14 14:26:30, ckocagil wrote: > > https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.cc > > File ui/gfx/render_text_win.cc (right): > > > > > https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... > > ui/gfx/render_text_win.cc:255: const internal::TextRun* run; > > On 2013/08/14 12:59:43, Alexei Svitkine wrote: > > > How about instead of storing a pointer to the run, store a run_index? Since > > this > > > is used only in the draw function of RenderTextWin, it should be able to get > > > back at the actual run pointer from there. > > > > > > If you store only the run_index, then you can actually do it in LineSegment > > and > > > get rid of LineSegmentWin entirely and in fact no longer allocate > LineSegments > > > on the heap - i.e. change ScopedVector<LineSegment> segments; to > > > std::vector<LineSegment>, which should be more efficient. > > > > > > (However, you do now have to pass a run_index to LineBreaker::AddRun(), but > I > > > think that's worth doing given the benefits above.) > > > > I'm not sure about this, can you guess what kind of data LineSegment > > implementations on other platforms will need to keep? I haven't checked how > > RenderText works on other platforms yet. If you're confident that a run index > > will be sufficient, I can make this change. Otherwise, I can add a TODO to > > remove the subclass in the future if possible. > > That's a good point - run_index is currently not useful to RenderTextLinux since > RenderTextLinux doesn't keep around an array of runs (they're a linked list > managed by Pango). > > I still think it would be good to have a design where we can have a common > LineSegment structure between platforms and it seems really all it needs is a > pointer to the platform-specific run. > > In other parts of the codebase we solve this by having a platform-specific > typedef - for example NativeView is typedef'd to HWND on Windows vs. NSView* on > Mac. Perhaps we can do the same for RenderText runs - i.e. typedef NativeTextRun > to internal::TextRun* on Windows and to PangoLayoutRun* on Linux. That way, we > can have a common LineSegment struct that has a NativeTextRun member that will > be the correct type for each platform (which avoids having to have > LineSegmentWin and having to dynamically allocate them all). > > This will also allow us to possibly move some function signatures to the base > RenderText class that take NativeTextRun as a param, with subclasses > implementing the specific ones. > > Mike, what do you think? I'm very much in favor of cross-platform TextRun and LineSegment structs/classes/interfaces; while neither is strictly necessary, should be considered now, and they'd help with consolidating existing platform-specific code and porting this new multiline code. For now, I'm ambivalent about using a run index or TextRun pointer, but I think interfaces or abstract classes/structs would help more for consolidating code than typedefs, which really just make defining interfaces (move some function signatures to the base RenderText class) easier, afaict.
I'm sorry it took me absurdly long for this review round, but I'm happy that Alexei has given such good reviews and your work has made the code much more solid. https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:881: offset.Add(Vector2d(0, lines()[line].preceding_heights)); On 2013/07/19 19:40:50, ckocagil wrote: > On 2013/07/17 06:47:18, msw wrote: > > Does this need to add a per-line horizontal offset? > > Which horizontal offset? I don't see your point. Nevermind! The line's alignment offset is added right below this! https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/57001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:135: segments.swap(other.segments); On 2013/07/19 19:40:50, ckocagil wrote: > On 2013/07/17 06:47:18, msw wrote: > > Swap makes this destructive to the right hand side of an assignment. Can you > > avoid that, or is this okay because it's only used for STL containers? (if so > > note that in a comment). > > Yes, these are for STL containers only. I'm adding a comment for now. One way to > avoid this is to keep "ScopedVector<Line>" instead of "std::vector<Line>" which > would allow us to disallow copying and assigning. Let me know if you think the > latter solution would be better. Use ScopedVector and avoid copy/assign if possible and not too complex. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:852: line_breaks_.SetMax(layout_text.length()); nit: cache the length for use here and in the loop below. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:856: CHECK(success); Should this really crash? Perhaps DCHECK and supply a valid empty list on failure? Otherwise, CHECK runs in all builds, so you can inline iter.Init(). https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:913: unsigned int line = 0; nit: use size_t https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:914: while (line < lines_.size() && x > lines()[line].width) { nit: A for loop might be a little simpler. Also, avoid mixing lines_ and lines(), use one or the other. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:930: EnsureLayout(); Should this be run even in the non-multiline case? https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:932: // Each line segments keeps its position in text coordinates. Traverse all nit: s/segements/segment/ https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:940: ui::Range intersection = segment->x_pos.Intersect(x); nit: const https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:944: rect += offset; nit: inline |rect + offset| into the next line. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:956: DCHECK(!lines_.empty()); nit: instead, DCHECK_LT(line_number, lines.size()); https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1076: line_breaks_.SetMax(0); Do this at the top with layout_text_.clear() https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:130: // Destructive. Empties the |segments| field of the given Line. As per my comment on the old patch set, I hope we can use ScopedVector instead of an std::vector of Line structs and then dissalow copy and assign here. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:140: int width; optional nit: combine |width| and |height| as a gfx::Size. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:222: bool multiline() const { return multiline_; } Order these after SetObscuredRevealIndex, ditto for the .cc def. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:400: void set_lines(std::vector<internal::Line>* lines) { lines_.swap(*lines); } Hmm, I'm not sure if vector ops are allowed for inlining. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:481: // Handles the display area, display offset, application LTR/RTL mode and nit: wrap this sentence above for a two-line comment. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:159: // true, this function is careful to not roll back to the line beginning. nit: careful or guaranteed? That may matter to callers. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:172: int current_word_x = 0; |word_width| or |current_word_width| seems clearer. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:174: int x = 0; just use |*width| instead of declaring |x|. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:176: for (size_t i = start_char; i < run.range.end(); ++i) { 1) I suspect you'll need to check IsCursorablePosition, use a char iterator, supply a |char_breaks|, or similar to actually break at valid character indices. This process should not split surrogate or diacritic code points from their base code points. 2) Also, I suspect that if we support breaking by character, we'll need to reshape the glyphs to prevent ligatures meant to join two characters now placed on separate lines. 3) We'll eventually need to skip reserving width for whitespace characters at the beginnings and ends of lines. These could potentially be followup tasks, but they each deserve TODOs in the code and either corresponding bugs or very clear comments in a meta bug. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:261: LineBreaker(int max_width, bool multiline, const BreakList<size_t>* words) nit: one param per line when the initialization list doesn't also fit. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:263: multiline_(multiline), Why is LineBreaker even used if multiline is false? If needed, add a file-local static function that makes a single line: internal::Line CreateLine(std::vector<internal::TextRun> runs) https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:275: // each instance. If |multiline| is false, doesn't do any breaking and fills nit: "each run instance", right? Better to be explicit there. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:277: void AddRun(const internal::TextRun& run) { Make this private, scrap Finalize and only expose a function like: std::vector<internal::Line> CreateLines(std::vector<internal::TextRun> runs) (or maybe std::vector<internal::TextRun*> to avoid copying) Then ComputeLines just calls that with a visually ordered run vector. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:284: CommitSegment(run); On 2013/08/14 12:59:43, Alexei Svitkine wrote: > Advance() and CommitSegment() always get called together. Can you just merge > them? Agreed, I like "commit" for adding text, and this should also hopefully eliminate |current_pos_| and |current_width_| members, which seem more suitable as function-local variables for modifying |pos_|/|line_x_|/|text_x_| (the true current committed values). https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:294: // Breaks a run into segments of at most |max_width| width, adds the segments, nit: |max_width_| https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:301: while (BreakRunAtWidth(run, *words_, pos_, max_width_ - line_x_, This is de-referencing words_, which might technically be NULL, but should actually be possible, given the logic to call BreakRun. I guess it's okay to not DCHECK/CHECK before a potential NULL derefernce, but I wouldn't be against that sort of an explicit check here. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:303: DCHECK(pos_ < run.range.end()); nit: DCHECK_LT. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:328: void ResetPos(size_t new_pos) { This is called once, consider inlining it. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:339: void SkipLine() { nit: AdvanceLine seems more appropriate. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:348: DCHECK(current_width_ == 0); nit: DCHECK_EQ https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:355: lines_.back().segments.push_back(segment); nit: cache lines_.back() as a local ptr |line|. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:359: lines_.back().baseline = std::max(lines_.back().baseline, I think the line height and baseline might have to be calculated as done in FontList::CacheCommonFontHeightAndBaseline, by calculating common ascent and descent values, then determining the resulting height and baseline. That's to handle cases where the ascents and descents mismatch while heights and/or baselines might match. Alternately, each line could store a FontList of the Fonts used to derive these values as needed. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:361: lines_.back().preceding_heights = preceding_line_heights_; Must this be altered on each committed segment? Can't it be done in SkipLine, allowing you to eliminate preceding_line_heights_ altogether? https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:373: int max_width_; nit: const https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:374: bool multiline_; nit: const https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:381: // end character position. |text_x_| is the text-space and |line_x_| is the nit: |text_x_| and |line_x_| are text-space and line-space x coordinates of |pos_|. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:391: size_t current_pos_; Would |next_pos_| and |next_width_| be more appropriate? Or perhaps make current_pos_ and pos_ a Range, etc? (only if you have to keep these, I hope they can be removed) https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:432: return Size(display_rect().width(), Should this actually be dislpay_rect().width() if the multi-line text actually wraps earlier, yielding a smaller actual width? Similar question for the single-line case above. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:594: std::vector<ui::Range> bounds; nit: declare |bounds| after the early return. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:681: void RenderTextWin::ComputeLines() { Inline this in EnsureLayout https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:685: multiline() ? &GetLineBreaks() : 0); Use NULL, not 0. Moreso, avoid LineBreaker for single-line scenarios. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:715: Vector2d text_offset = line_offset + Vector2d(0, line.baseline); Hmm, I'm not clear on why we add the baseline to text_offset. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:735: // Skip painting segments outside the display rect area. This should only happen for visible lines if the line actually extended beyond the display rect width, which should only happen for the single-line case (since we don't allow multi-line lines to extend beyond the display-rect width at the moment). So I think you can wrap |segment_bounds| and this check in a !multiline() check for performance. (if you think it's better to support the multiline case where lines may extend beyond the display_rect width, then this might be okay as-is) https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:736: Rect segment_bounds(PointAtOffsetFromOrigin(line_offset) + nit: const https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:745: pos.resize(glyphs.length()); Does it help to keep using a terminal pos entry? https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:761: ui::Range intersection = colors().GetRange(it).Intersect( nit: const https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:763: ui::Range colored_glyphs = CharRangeToGlyphRange(*run, intersection); nit: const https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:771: SkScalar width = (colored_glyphs.end() < glyphs.end() nit: const https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:772: ? pos[colored_glyphs.end() - glyphs.start()].x() nit: '?' goes on the line above, ':' goes at the end of this line, and both this line and the line below should be indented 4 more spaces than the ternary's conditional, I think :P https://codereview.chromium.org/16867016/diff/138001/ui/views/examples/multil... File ui/views/examples/multiline_example.cc (right): https://codereview.chromium.org/16867016/diff/138001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:10: #include "ui/views/controls/button/checkbox.h" nit: remove this (included by header) https://codereview.chromium.org/16867016/diff/138001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:13: #include "ui/views/layout/fill_layout.h" nit: remove this (unused) https://codereview.chromium.org/16867016/diff/138001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:28: render_text_->set_clip_to_display_rect(false); Hmm, this is no longer used and it's underlying code/settings should be removed altogether, the PDF bug was resolved in another manner. Let's remove it in a separate CL. Feel free to remove this call, or leave it as-is if it's otherwise causing trouble. https://codereview.chromium.org/16867016/diff/138001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:43: if (w == 0) // TODO(ckocagil): Why does this happen? nit: resolve this (you can just remove the TODO if you want) https://codereview.chromium.org/16867016/diff/138001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:52: ui::Range test_range(1, 21); nit: move declaration below the SetText call. https://codereview.chromium.org/16867016/diff/138001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:92: top_container_ = container; nit: |example_container_| seems clearer. https://codereview.chromium.org/16867016/diff/138001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:94: gfx::RenderText* render_text = gfx::RenderText::CreateInstance(); Move this to the RenderTextView ctor initialization list. https://codereview.chromium.org/16867016/diff/138001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:145: top_container_->Layout(); Can you just InvalidateLayout instead of Layout+SchedulePaint? ditto below. https://codereview.chromium.org/16867016/diff/138001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:155: if (sender != label_checkbox_) nit: just DCHECK this
https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:130: // Destructive. Empties the |segments| field of the given Line. On 2013/08/15 02:44:17, msw wrote: > As per my comment on the old patch set, I hope we can use ScopedVector instead > of an std::vector of Line structs and then dissalow copy and assign here. Hmm, I'm not sure I like your proposed approach - then every line would have to be be alloc'd from the heap, which will add even more overhead and memory trashing. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:277: void AddRun(const internal::TextRun& run) { On 2013/08/15 02:44:17, msw wrote: > Make this private, scrap Finalize and only expose a function like: > std::vector<internal::Line> CreateLines(std::vector<internal::TextRun> runs) > (or maybe std::vector<internal::TextRun*> to avoid copying) > Then ComputeLines just calls that with a visually ordered run vector. I disagree. I think it's wasteful to create a temporary vector ordered visually to use the API. I think the current API is absolutely fine, it's still simple enough and avoids doing extra work for no benefit.
https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:130: // Destructive. Empties the |segments| field of the given Line. On 2013/08/15 14:51:05, Alexei Svitkine wrote: > On 2013/08/15 02:44:17, msw wrote: > > As per my comment on the old patch set, I hope we can use ScopedVector instead > > of an std::vector of Line structs and then dissalow copy and assign here. > > Hmm, I'm not sure I like your proposed approach - then every line would have to > be be alloc'd from the heap, which will add even more overhead and memory > trashing. Do you have qualms with the ScopedVector<LineSegment> already used here? There's always fewer lines than LineSegments, so I don't quite buy the argument. I'd like DISALLOW_COPY_AND_ASSIGN to remove the destructive copy ctor and assignment op, since I'm always reluctant to do premature performance optimization that adds code or complexity, unless it's super obvious. I don't offhand suspect that heap allocations have much perf impact compared to Layout overall, but since we Layout for each Canvas::*Text call, I'd hear out your argument if you had data showing a significant perf win of std::vector over ScopedVector in common cases. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:277: void AddRun(const internal::TextRun& run) { On 2013/08/15 14:51:05, Alexei Svitkine wrote: > On 2013/08/15 02:44:17, msw wrote: > > Make this private, scrap Finalize and only expose a function like: > > std::vector<internal::Line> CreateLines(std::vector<internal::TextRun> runs) > > (or maybe std::vector<internal::TextRun*> to avoid copying) > > Then ComputeLines just calls that with a visually ordered run vector. > > I disagree. I think it's wasteful to create a temporary vector ordered visually > to use the API. I think the current API is absolutely fine, it's still simple > enough and avoids doing extra work for no benefit. I'll concede here, LineBreaker isn't too complex for an internal API as-is.
https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:130: // Destructive. Empties the |segments| field of the given Line. On 2013/08/15 15:42:09, msw wrote: > On 2013/08/15 14:51:05, Alexei Svitkine wrote: > > On 2013/08/15 02:44:17, msw wrote: > > > As per my comment on the old patch set, I hope we can use ScopedVector > instead > > > of an std::vector of Line structs and then dissalow copy and assign here. > > > > Hmm, I'm not sure I like your proposed approach - then every line would have > to > > be be alloc'd from the heap, which will add even more overhead and memory > > trashing. > > Do you have qualms with the ScopedVector<LineSegment> already used here? Yes, I do. I suggested merging LineSegmentWin to LineSegment, so that it's a common struct and can be used in a regular std::vector. I proposed storing run_index instead of a run pointer. But Cem pointed outed that this may not be enough for other platforms than RenderTextWin, which is true since a run_index is not useful to RenderTextLinux which uses Pango's linked list of runs. Which is why I suggested that perhaps we should have a typedef for NativeTextRun, so that LineSegment can have a member of that type and not need platform-specific subclasses, so that we can use a std::vector for LineSegments.
https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:130: // Destructive. Empties the |segments| field of the given Line. On 2013/08/15 17:27:42, Alexei Svitkine wrote: > On 2013/08/15 15:42:09, msw wrote: > > On 2013/08/15 14:51:05, Alexei Svitkine wrote: > > > On 2013/08/15 02:44:17, msw wrote: > > > > As per my comment on the old patch set, I hope we can use ScopedVector > > instead > > > > of an std::vector of Line structs and then dissalow copy and assign here. > > > > > > Hmm, I'm not sure I like your proposed approach - then every line would have > > to > > > be be alloc'd from the heap, which will add even more overhead and memory > > > trashing. > > > > Do you have qualms with the ScopedVector<LineSegment> already used here? > > Yes, I do. I suggested merging LineSegmentWin to LineSegment, so that it's a > common struct and can be used in a regular std::vector. I proposed storing > run_index instead of a run pointer. But Cem pointed outed that this may not be > enough for other platforms than RenderTextWin, which is true since a run_index > is not useful to RenderTextLinux which uses Pango's linked list of runs. Which > is why I suggested that perhaps we should have a typedef for NativeTextRun, so > that LineSegment can have a member of that type and not need platform-specific > subclasses, so that we can use a std::vector for LineSegments. Okay, after more thought and talking with Alexei, I agree that std::vector likely has measurably better performance for both Line and LineSegment lists. Cem, ignore my ScopedVector suggestion and instead use a run index instead of a specialized LineSegmentWin with a internal::TextRun* member so the LineSegment ScopedVector can become an std::vector and we can trash the destructive copy ctor and assignment operator for the Line struct. Run indicies should be okay for now since RenderTextLinux will likely able to use built-in Pango functionality to support multi-line layout instead of porting our RenderTextWin multi-line solution. But if a cross-platform base class/struct for TextRun makes more sense, then a std::vector<TextRun*> works for me too. Let me/us know if any of that direction is unclear; thanks!
Alexei's platform-typedef suggestion sounds good. For the cross-platform TextRun, I will first have to start writing the Linux multiline implementation. I'll address your comments and update soon. Also, I have fixed the existing unit tests and added a new one. Please review; thanks!
On 2013/08/15 18:43:54, ckocagil wrote: > Alexei's platform-typedef suggestion sounds good. For the cross-platform > TextRun, I will first have to start writing the Linux multiline implementation. > I'll address your comments and update soon. > > Also, I have fixed the existing unit tests and added a new one. Please review; > thanks! Let's go with run_index for now and we can easily change this to typedef later when the time comes, if desired.
https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:159: // true, this function is careful to not roll back to the line beginning. On 2013/08/15 02:44:17, msw wrote: > nit: careful or guaranteed? That may matter to callers. Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:172: int current_word_x = 0; On 2013/08/15 02:44:17, msw wrote: > |word_width| or |current_word_width| seems clearer. Done, it's |word_width| now. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:174: int x = 0; On 2013/08/15 02:44:17, msw wrote: > just use |*width| instead of declaring |x|. Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:176: for (size_t i = start_char; i < run.range.end(); ++i) { On 2013/08/15 02:44:17, msw wrote: > 1) I suspect you'll need to check IsCursorablePosition, use a char iterator, > supply a |char_breaks|, or similar to actually break at valid character indices. > This process should not split surrogate or diacritic code points from their base > code points. TextRun::logical_clusters has cluster information, perhaps it can be used to avoid breaking at surrogates/diacritics. > 2) Also, I suspect that if we support breaking by character, we'll need to > reshape the glyphs to prevent ligatures meant to join two characters now placed > on separate lines. Wouldn't fixing 1) also fix this issue? > 3) We'll eventually need to skip reserving width for whitespace characters at > the beginnings and ends of lines. > > These could potentially be followup tasks, but they each deserve TODOs in the > code and either corresponding bugs or very clear comments in a meta bug. I'm adding TODOs here and comments to the bug. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:255: const internal::TextRun* run; On 2013/08/14 12:59:43, Alexei Svitkine wrote: > How about instead of storing a pointer to the run, store a run_index? Since this > is used only in the draw function of RenderTextWin, it should be able to get > back at the actual run pointer from there. > > If you store only the run_index, then you can actually do it in LineSegment and > get rid of LineSegmentWin entirely and in fact no longer allocate LineSegments > on the heap - i.e. change ScopedVector<LineSegment> segments; to > std::vector<LineSegment>, which should be more efficient. > > (However, you do now have to pass a run_index to LineBreaker::AddRun(), but I > think that's worth doing given the benefits above.) Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:261: LineBreaker(int max_width, bool multiline, const BreakList<size_t>* words) On 2013/08/15 02:44:17, msw wrote: > nit: one param per line when the initialization list doesn't also fit. Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:263: multiline_(multiline), On 2013/08/15 02:44:17, msw wrote: > Why is LineBreaker even used if multiline is false? > If needed, add a file-local static function that makes a single line: > internal::Line CreateLine(std::vector<internal::TextRun> runs) I don't think we should create a new function for this case when implementing the single-line mode here doesn't even cost a line (see the ternary operator in AddRun). If needed, we should either change the class name to something like LineGenerator and/or add a better description of |multiline|. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:275: // each instance. If |multiline| is false, doesn't do any breaking and fills On 2013/08/15 02:44:17, msw wrote: > nit: "each run instance", right? Better to be explicit there. Oh, this comment doesn't even apply anymore. Removed. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:284: CommitSegment(run); On 2013/08/15 02:44:17, msw wrote: > On 2013/08/14 12:59:43, Alexei Svitkine wrote: > > Advance() and CommitSegment() always get called together. Can you just merge > > them? > > Agreed, I like "commit" for adding text, and this should also hopefully > eliminate |current_pos_| and |current_width_| members, which seem more suitable > as function-local variables for modifying |pos_|/|line_x_|/|text_x_| (the true > current committed values). All done! Advance() and CommitSegment() are now merged into AddSegment(). There is no |current_pos_|, |current_width|, ResetPos() and thus the "commit" logic is gone. I like it, it is much simpler now. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:294: // Breaks a run into segments of at most |max_width| width, adds the segments, On 2013/08/15 02:44:17, msw wrote: > nit: |max_width_| Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:301: while (BreakRunAtWidth(run, *words_, pos_, max_width_ - line_x_, On 2013/08/15 02:44:17, msw wrote: > This is de-referencing words_, which might technically be NULL, but should > actually be possible, given the logic to call BreakRun. I guess it's okay to not > DCHECK/CHECK before a potential NULL derefernce, but I wouldn't be against that > sort of an explicit check here. Added DCHECK. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:303: DCHECK(pos_ < run.range.end()); On 2013/08/15 02:44:17, msw wrote: > nit: DCHECK_LT. Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:328: void ResetPos(size_t new_pos) { On 2013/08/15 02:44:17, msw wrote: > This is called once, consider inlining it. Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:339: void SkipLine() { On 2013/08/15 02:44:17, msw wrote: > nit: AdvanceLine seems more appropriate. Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:348: DCHECK(current_width_ == 0); On 2013/08/15 02:44:17, msw wrote: > nit: DCHECK_EQ Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:355: lines_.back().segments.push_back(segment); On 2013/08/15 02:44:17, msw wrote: > nit: cache lines_.back() as a local ptr |line|. Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:359: lines_.back().baseline = std::max(lines_.back().baseline, On 2013/08/15 02:44:17, msw wrote: > I think the line height and baseline might have to be calculated as done in > FontList::CacheCommonFontHeightAndBaseline, by calculating common ascent and > descent values, then determining the resulting height and baseline. That's to > handle cases where the ascents and descents mismatch while heights and/or > baselines might match. Alternately, each line could store a FontList of the > Fonts used to derive these values as needed. Done. FontList looks like it can only contain fonts of the same style and size, so I went with implementing that calculation here. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:361: lines_.back().preceding_heights = preceding_line_heights_; On 2013/08/15 02:44:17, msw wrote: > Must this be altered on each committed segment? Can't it be done in SkipLine, > allowing you to eliminate preceding_line_heights_ altogether? Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:373: int max_width_; On 2013/08/15 02:44:17, msw wrote: > nit: const Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:374: bool multiline_; On 2013/08/15 02:44:17, msw wrote: > nit: const Done. Also done for |words_| below. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:381: // end character position. |text_x_| is the text-space and |line_x_| is the On 2013/08/15 02:44:17, msw wrote: > nit: |text_x_| and |line_x_| are text-space and line-space x coordinates of > |pos_|. Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:391: size_t current_pos_; On 2013/08/15 02:44:17, msw wrote: > Would |next_pos_| and |next_width_| be more appropriate? > Or perhaps make current_pos_ and pos_ a Range, etc? > (only if you have to keep these, I hope they can be removed) They're gone. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:432: return Size(display_rect().width(), On 2013/08/15 02:44:17, msw wrote: > Should this actually be dislpay_rect().width() if the multi-line text actually > wraps earlier, yielding a smaller actual width? Similar question for the > single-line case above. Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:594: std::vector<ui::Range> bounds; On 2013/08/15 02:44:17, msw wrote: > nit: declare |bounds| after the early return. Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:681: void RenderTextWin::ComputeLines() { On 2013/08/15 02:44:17, msw wrote: > Inline this in EnsureLayout Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:685: multiline() ? &GetLineBreaks() : 0); On 2013/08/15 02:44:17, msw wrote: > Use NULL, not 0. Moreso, avoid LineBreaker for single-line scenarios. Replaced 0 with NULL. I'm using LineBreaker for single-line to have a single drawing path. Why are you suggesting this? If it's for performance, LineBreaker doesn't do any text inspection (or actual line breaking) in the single-line case, so I'm not sure if it's worth having two paths for the performance gain here. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:715: Vector2d text_offset = line_offset + Vector2d(0, line.baseline); On 2013/08/15 02:44:17, msw wrote: > Hmm, I'm not clear on why we add the baseline to text_offset. AFAICT Skia vertically aligns (0, 0) with the baseline, so not adding the baseline would draw the ascent above 0 and the ascent wouldn't be visible. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:735: // Skip painting segments outside the display rect area. On 2013/08/15 02:44:17, msw wrote: > This should only happen for visible lines if the line actually extended beyond > the display rect width, which should only happen for the single-line case (since > we don't allow multi-line lines to extend beyond the display-rect width at the > moment). So I think you can wrap |segment_bounds| and this check in a > !multiline() check for performance. (if you think it's better to support the > multiline case where lines may extend beyond the display_rect width, then this > might be okay as-is) Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:736: Rect segment_bounds(PointAtOffsetFromOrigin(line_offset) + On 2013/08/15 02:44:17, msw wrote: > nit: const Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:745: pos.resize(glyphs.length()); On 2013/08/15 02:44:17, msw wrote: > Does it help to keep using a terminal pos entry? I don't see a place where it would. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:761: ui::Range intersection = colors().GetRange(it).Intersect( On 2013/08/15 02:44:17, msw wrote: > nit: const Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:763: ui::Range colored_glyphs = CharRangeToGlyphRange(*run, intersection); On 2013/08/15 02:44:17, msw wrote: > nit: const Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:771: SkScalar width = (colored_glyphs.end() < glyphs.end() On 2013/08/15 02:44:17, msw wrote: > nit: const Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:772: ? pos[colored_glyphs.end() - glyphs.start()].x() On 2013/08/15 02:44:17, msw wrote: > nit: '?' goes on the line above, ':' goes at the end of this line, and both this > line and the line below should be indented 4 more spaces than the ternary's > conditional, I think :P Done.
Looking good, here's a few more comments. https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:957: const int width = lines()[line_number].width + (cursor_enabled_ ? 1 : 0); Nit: lines_ https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:16: #include "base/memory/scoped_vector.h" No longer needed, please remove. https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:123: ui::Range x_pos; Nit: Rename to x_range. https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:126: ui::Range char_pos; Nit: Rename to char_range. https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:129: NativeRunData run; Just make it a size_t run_index; for this change and get rid of NativeRunData. (We can re-introduce it at a later time if needed.) https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:140: Line& operator=(Line& other); You can get rid of this and the copy ctor now that all members are either POD or std::vector<POD>, since the default ones will do the right thing. https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1688: kRtlLtr, kRtlLtrRtl }; Nit: Align with previous line. https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:280: internal::TextRun* run = runs_[run_index]; Nit: No need for internal::, since this is in the same namespace. Please apply throughout this class. https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:296: lines_.back().preceding_heights + lines_.back().height); I don't think Line::preceding_heights is needed - you can just use preceding_line_heights_ that you're keeping track of in this class and remove the variable from Line. Maybe rename |preceding_line_heights_| to |total_height_| as well. https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:351: void AddSegment(int run_index, size_t new_pos, int width) { Rename new_pos to segment_end. https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:385: // Position information of the last added segment. |pos_| is the segment's end I think it would be clearer to describe these in terms of the next segment, rather than the last segment (especially since the last segment may not exist - i.e. at the start). Also, style guide discourages abbreviations - how about renaming pos_ to segment_start_?
I'll take another more complete look after you address Alexei's comments. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:685: multiline() ? &GetLineBreaks() : 0); On 2013/08/16 16:33:15, ckocagil wrote: > On 2013/08/15 02:44:17, msw wrote: > > Use NULL, not 0. Moreso, avoid LineBreaker for single-line scenarios. > > Replaced 0 with NULL. > > I'm using LineBreaker for single-line to have a single drawing path. Why are you > suggesting this? If it's for performance, LineBreaker doesn't do any text > inspection (or actual line breaking) in the single-line case, so I'm not sure if > it's worth having two paths for the performance gain here. It's just weird to create and use a LineBreaker to not breaks lines. I guess it's fine with a comment or renamed LineMaker, LineGenerator, etc. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:745: pos.resize(glyphs.length()); On 2013/08/16 16:33:15, ckocagil wrote: > On 2013/08/15 02:44:17, msw wrote: > > Does it help to keep using a terminal pos entry? > > I don't see a place where it would. At the SkScalar width = (colored_glyphs.end() < glyphs.end() ? ... : ...) I think an earlier patch set added the terminal value for this case.
https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:745: pos.resize(glyphs.length()); On 2013/08/16 19:05:02, msw wrote: > On 2013/08/16 16:33:15, ckocagil wrote: > > On 2013/08/15 02:44:17, msw wrote: > > > Does it help to keep using a terminal pos entry? > > > > I don't see a place where it would. > > At the SkScalar width = (colored_glyphs.end() < glyphs.end() ? ... : ...) > I think an earlier patch set added the terminal value for this case. Done. https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:957: const int width = lines()[line_number].width + (cursor_enabled_ ? 1 : 0); On 2013/08/16 18:32:34, Alexei Svitkine wrote: > Nit: lines_ Done (throughout the file). https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:16: #include "base/memory/scoped_vector.h" On 2013/08/16 18:32:34, Alexei Svitkine wrote: > No longer needed, please remove. Done. https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:123: ui::Range x_pos; On 2013/08/16 18:32:34, Alexei Svitkine wrote: > Nit: Rename to x_range. Done. https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:126: ui::Range char_pos; On 2013/08/16 18:32:34, Alexei Svitkine wrote: > Nit: Rename to char_range. Done. https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:129: NativeRunData run; On 2013/08/16 18:32:34, Alexei Svitkine wrote: > Just make it a size_t run_index; for this change and get rid of NativeRunData. > (We can re-introduce it at a later time if needed.) Done. https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:140: Line& operator=(Line& other); On 2013/08/16 18:32:34, Alexei Svitkine wrote: > You can get rid of this and the copy ctor now that all members are either POD or > std::vector<POD>, since the default ones will do the right thing. Done. https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1688: kRtlLtr, kRtlLtrRtl }; On 2013/08/16 18:32:34, Alexei Svitkine wrote: > Nit: Align with previous line. Done. https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:280: internal::TextRun* run = runs_[run_index]; On 2013/08/16 18:32:34, Alexei Svitkine wrote: > Nit: No need for internal::, since this is in the same namespace. Please apply > throughout this class. Done. https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:296: lines_.back().preceding_heights + lines_.back().height); On 2013/08/16 18:32:34, Alexei Svitkine wrote: > I don't think Line::preceding_heights is needed - you can just use > preceding_line_heights_ that you're keeping track of in this class and remove > the variable from Line. It is needed for |RenderText::GetLineOffset()|. I think that function looks good as it is, do you still think this change is required? > Maybe rename |preceding_line_heights_| to |total_height_| as well. Done. https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:351: void AddSegment(int run_index, size_t new_pos, int width) { On 2013/08/16 18:32:34, Alexei Svitkine wrote: > Rename new_pos to segment_end. Done. https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:385: // Position information of the last added segment. |pos_| is the segment's end On 2013/08/16 18:32:34, Alexei Svitkine wrote: > I think it would be clearer to describe these in terms of the next segment, > rather than the last segment (especially since the last segment may not exist - > i.e. at the start). Done. > Also, style guide discourages abbreviations - how about renaming pos_ to > segment_start_? Done.
Thanks! I'll let Mike take a look and comment before I'll have another detailed look, but I think this is looking quite good now. On Mon, Aug 19, 2013 at 8:09 PM, <ckocagil@chromium.org> wrote: > > https://codereview.chromium.**org/16867016/diff/138001/ui/** > gfx/render_text_win.cc<https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.cc> > File ui/gfx/render_text_win.cc (right): > > https://codereview.chromium.**org/16867016/diff/138001/ui/** > gfx/render_text_win.cc#**newcode745<https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.cc#newcode745> > ui/gfx/render_text_win.cc:745: pos.resize(glyphs.length()); > On 2013/08/16 19:05:02, msw wrote: > >> On 2013/08/16 16:33:15, ckocagil wrote: >> > On 2013/08/15 02:44:17, msw wrote: >> > > Does it help to keep using a terminal pos entry? >> > >> > I don't see a place where it would. >> > > At the SkScalar width = (colored_glyphs.end() < glyphs.end() ? ... : >> > ...) > >> I think an earlier patch set added the terminal value for this case. >> > > Done. > > > https://codereview.chromium.**org/16867016/diff/166001/ui/** > gfx/render_text.cc<https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text.cc> > File ui/gfx/render_text.cc (right): > > https://codereview.chromium.**org/16867016/diff/166001/ui/** > gfx/render_text.cc#newcode957<https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text.cc#newcode957> > ui/gfx/render_text.cc:957: const int width = lines()[line_number].width > + (cursor_enabled_ ? 1 : 0); > On 2013/08/16 18:32:34, Alexei Svitkine wrote: > >> Nit: lines_ >> > > Done (throughout the file). > > > https://codereview.chromium.**org/16867016/diff/166001/ui/** > gfx/render_text.h<https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text.h> > File ui/gfx/render_text.h (right): > > https://codereview.chromium.**org/16867016/diff/166001/ui/** > gfx/render_text.h#newcode16<https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text.h#newcode16> > ui/gfx/render_text.h:16: #include "base/memory/scoped_vector.h" > On 2013/08/16 18:32:34, Alexei Svitkine wrote: > >> No longer needed, please remove. >> > > Done. > > > https://codereview.chromium.**org/16867016/diff/166001/ui/** > gfx/render_text.h#newcode123<https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text.h#newcode123> > ui/gfx/render_text.h:123: ui::Range x_pos; > On 2013/08/16 18:32:34, Alexei Svitkine wrote: > >> Nit: Rename to x_range. >> > > Done. > > > https://codereview.chromium.**org/16867016/diff/166001/ui/** > gfx/render_text.h#newcode126<https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text.h#newcode126> > ui/gfx/render_text.h:126: ui::Range char_pos; > On 2013/08/16 18:32:34, Alexei Svitkine wrote: > >> Nit: Rename to char_range. >> > > Done. > > > https://codereview.chromium.**org/16867016/diff/166001/ui/** > gfx/render_text.h#newcode129<https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text.h#newcode129> > ui/gfx/render_text.h:129: NativeRunData run; > On 2013/08/16 18:32:34, Alexei Svitkine wrote: > >> Just make it a size_t run_index; for this change and get rid of >> > NativeRunData. > >> (We can re-introduce it at a later time if needed.) >> > > Done. > > > https://codereview.chromium.**org/16867016/diff/166001/ui/** > gfx/render_text.h#newcode140<https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text.h#newcode140> > ui/gfx/render_text.h:140: Line& operator=(Line& other); > On 2013/08/16 18:32:34, Alexei Svitkine wrote: > >> You can get rid of this and the copy ctor now that all members are >> > either POD or > >> std::vector<POD>, since the default ones will do the right thing. >> > > Done. > > > https://codereview.chromium.**org/16867016/diff/166001/ui/** > gfx/render_text_unittest.cc<https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text_unittest.cc> > File ui/gfx/render_text_unittest.cc (right): > > https://codereview.chromium.**org/16867016/diff/166001/ui/** > gfx/render_text_unittest.cc#**newcode1688<https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text_unittest.cc#newcode1688> > ui/gfx/render_text_unittest.**cc:1688: kRtlLtr, kRtlLtrRtl }; > On 2013/08/16 18:32:34, Alexei Svitkine wrote: > >> Nit: Align with previous line. >> > > Done. > > > https://codereview.chromium.**org/16867016/diff/166001/ui/** > gfx/render_text_win.cc<https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text_win.cc> > File ui/gfx/render_text_win.cc (right): > > https://codereview.chromium.**org/16867016/diff/166001/ui/** > gfx/render_text_win.cc#**newcode280<https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text_win.cc#newcode280> > ui/gfx/render_text_win.cc:280: internal::TextRun* run = > runs_[run_index]; > On 2013/08/16 18:32:34, Alexei Svitkine wrote: > >> Nit: No need for internal::, since this is in the same namespace. >> > Please apply > >> throughout this class. >> > > Done. > > > https://codereview.chromium.**org/16867016/diff/166001/ui/** > gfx/render_text_win.cc#**newcode296<https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text_win.cc#newcode296> > ui/gfx/render_text_win.cc:296: lines_.back().preceding_**heights + > lines_.back().height); > On 2013/08/16 18:32:34, Alexei Svitkine wrote: > >> I don't think Line::preceding_heights is needed - you can just use >> preceding_line_heights_ that you're keeping track of in this class and >> > remove > >> the variable from Line. >> > > It is needed for |RenderText::GetLineOffset()|. I think that function > looks good as it is, do you still think this change is required? > > > Maybe rename |preceding_line_heights_| to |total_height_| as well. >> > > Done. > > > https://codereview.chromium.**org/16867016/diff/166001/ui/** > gfx/render_text_win.cc#**newcode351<https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text_win.cc#newcode351> > ui/gfx/render_text_win.cc:351: void AddSegment(int run_index, size_t > new_pos, int width) { > On 2013/08/16 18:32:34, Alexei Svitkine wrote: > >> Rename new_pos to segment_end. >> > > Done. > > > https://codereview.chromium.**org/16867016/diff/166001/ui/** > gfx/render_text_win.cc#**newcode385<https://codereview.chromium.org/16867016/diff/166001/ui/gfx/render_text_win.cc#newcode385> > ui/gfx/render_text_win.cc:385: // Position information of the last added > segment. |pos_| is the segment's end > On 2013/08/16 18:32:34, Alexei Svitkine wrote: > >> I think it would be clearer to describe these in terms of the next >> > segment, > >> rather than the last segment (especially since the last segment may >> > not exist - > >> i.e. at the start). >> > > Done. > > > Also, style guide discourages abbreviations - how about renaming pos_ >> > to > >> segment_start_? >> > > Done. > > https://codereview.chromium.**org/16867016/<https://codereview.chromium.org/1... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:852: line_breaks_.SetMax(layout_text.length()); On 2013/08/15 02:44:17, msw wrote: > nit: cache the length for use here and in the loop below. Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:856: CHECK(success); On 2013/08/15 02:44:17, msw wrote: > Should this really crash? Perhaps DCHECK and supply a valid empty list on > failure? Otherwise, CHECK runs in all builds, so you can inline iter.Init(). You're right, DCHECKing and returning an empty result should be enough here. Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:913: unsigned int line = 0; On 2013/08/15 02:44:17, msw wrote: > nit: use size_t Can I ignore the nits for this method? I will rewrite this method soon after this CL lands. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:914: while (line < lines_.size() && x > lines()[line].width) { On 2013/08/15 02:44:17, msw wrote: > nit: A for loop might be a little simpler. Also, avoid mixing lines_ and > lines(), use one or the other. Same as above. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:930: EnsureLayout(); On 2013/08/15 02:44:17, msw wrote: > Should this be run even in the non-multiline case? GetStringSize implicitly calls EnsureLayout (not sure if your comment is about this line). https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:932: // Each line segments keeps its position in text coordinates. Traverse all On 2013/08/15 02:44:17, msw wrote: > nit: s/segements/segment/ Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:940: ui::Range intersection = segment->x_pos.Intersect(x); On 2013/08/15 02:44:17, msw wrote: > nit: const Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:944: rect += offset; On 2013/08/15 02:44:17, msw wrote: > nit: inline |rect + offset| into the next line. Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:956: DCHECK(!lines_.empty()); On 2013/08/15 02:44:17, msw wrote: > nit: instead, DCHECK_LT(line_number, lines.size()); Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1076: line_breaks_.SetMax(0); On 2013/08/15 02:44:17, msw wrote: > Do this at the top with layout_text_.clear() Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:130: // Destructive. Empties the |segments| field of the given Line. On 2013/08/15 17:59:14, msw wrote: > On 2013/08/15 17:27:42, Alexei Svitkine wrote: > > On 2013/08/15 15:42:09, msw wrote: > > > On 2013/08/15 14:51:05, Alexei Svitkine wrote: > > > > On 2013/08/15 02:44:17, msw wrote: > > > > > As per my comment on the old patch set, I hope we can use ScopedVector > > > instead > > > > > of an std::vector of Line structs and then dissalow copy and assign > here. > > > > > > > > Hmm, I'm not sure I like your proposed approach - then every line would > have > > > to > > > > be be alloc'd from the heap, which will add even more overhead and memory > > > > trashing. > > > > > > Do you have qualms with the ScopedVector<LineSegment> already used here? > > > > Yes, I do. I suggested merging LineSegmentWin to LineSegment, so that it's a > > common struct and can be used in a regular std::vector. I proposed storing > > run_index instead of a run pointer. But Cem pointed outed that this may not be > > enough for other platforms than RenderTextWin, which is true since a run_index > > is not useful to RenderTextLinux which uses Pango's linked list of runs. Which > > is why I suggested that perhaps we should have a typedef for NativeTextRun, so > > that LineSegment can have a member of that type and not need platform-specific > > subclasses, so that we can use a std::vector for LineSegments. > > Okay, after more thought and talking with Alexei, I agree that std::vector > likely has measurably better performance for both Line and LineSegment lists. > Cem, ignore my ScopedVector suggestion and instead use a run index instead of a > specialized LineSegmentWin with a internal::TextRun* member so the LineSegment > ScopedVector can become an std::vector and we can trash the destructive copy > ctor and assignment operator for the Line struct. Run indicies should be okay > for now since RenderTextLinux will likely able to use built-in Pango > functionality to support multi-line layout instead of porting our RenderTextWin > multi-line solution. But if a cross-platform base class/struct for TextRun makes > more sense, then a std::vector<TextRun*> works for me too. Let me/us know if any > of that direction is unclear; thanks! Done; using std::vector and a run index now. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:140: int width; On 2013/08/15 02:44:17, msw wrote: > optional nit: combine |width| and |height| as a gfx::Size. Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:222: bool multiline() const { return multiline_; } On 2013/08/15 02:44:17, msw wrote: > Order these after SetObscuredRevealIndex, ditto for the .cc def. Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:400: void set_lines(std::vector<internal::Line>* lines) { lines_.swap(*lines); } On 2013/08/15 02:44:17, msw wrote: > Hmm, I'm not sure if vector ops are allowed for inlining. This is okay as we talked on IM. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:481: // Handles the display area, display offset, application LTR/RTL mode and On 2013/08/15 02:44:17, msw wrote: > nit: wrap this sentence above for a two-line comment. Done. https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:685: multiline() ? &GetLineBreaks() : 0); On 2013/08/16 19:05:02, msw wrote: > On 2013/08/16 16:33:15, ckocagil wrote: > > On 2013/08/15 02:44:17, msw wrote: > > > Use NULL, not 0. Moreso, avoid LineBreaker for single-line scenarios. > > > > Replaced 0 with NULL. > > > > I'm using LineBreaker for single-line to have a single drawing path. Why are > you > > suggesting this? If it's for performance, LineBreaker doesn't do any text > > inspection (or actual line breaking) in the single-line case, so I'm not sure > if > > it's worth having two paths for the performance gain here. > > It's just weird to create and use a LineBreaker to not breaks lines. > I guess it's fine with a comment or renamed LineMaker, LineGenerator, etc. Expanded the comment above the definition of LineBreaker. Looks better now? https://codereview.chromium.org/16867016/diff/138001/ui/views/examples/multil... File ui/views/examples/multiline_example.cc (right): https://codereview.chromium.org/16867016/diff/138001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:10: #include "ui/views/controls/button/checkbox.h" On 2013/08/15 02:44:17, msw wrote: > nit: remove this (included by header) Done. https://codereview.chromium.org/16867016/diff/138001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:13: #include "ui/views/layout/fill_layout.h" On 2013/08/15 02:44:17, msw wrote: > nit: remove this (unused) Done. https://codereview.chromium.org/16867016/diff/138001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:28: render_text_->set_clip_to_display_rect(false); On 2013/08/15 02:44:17, msw wrote: > Hmm, this is no longer used and it's underlying code/settings should be removed > altogether, the PDF bug was resolved in another manner. Let's remove it in a > separate CL. Feel free to remove this call, or leave it as-is if it's otherwise > causing trouble. Done, removed. https://codereview.chromium.org/16867016/diff/138001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:43: if (w == 0) // TODO(ckocagil): Why does this happen? On 2013/08/15 02:44:17, msw wrote: > nit: resolve this (you can just remove the TODO if you want) Leaving the TODO here as we decided on IM. https://codereview.chromium.org/16867016/diff/138001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:52: ui::Range test_range(1, 21); On 2013/08/15 02:44:17, msw wrote: > nit: move declaration below the SetText call. Done. https://codereview.chromium.org/16867016/diff/138001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:92: top_container_ = container; On 2013/08/15 02:44:17, msw wrote: > nit: |example_container_| seems clearer. Done. https://codereview.chromium.org/16867016/diff/138001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:94: gfx::RenderText* render_text = gfx::RenderText::CreateInstance(); On 2013/08/15 02:44:17, msw wrote: > Move this to the RenderTextView ctor initialization list. Done. https://codereview.chromium.org/16867016/diff/138001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:145: top_container_->Layout(); On 2013/08/15 02:44:17, msw wrote: > Can you just InvalidateLayout instead of Layout+SchedulePaint? ditto below. It seems to need to be both laid out and painted. AFAIK neither of these imply the other, so I can't get rid of them. https://codereview.chromium.org/16867016/diff/138001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:155: if (sender != label_checkbox_) On 2013/08/15 02:44:17, msw wrote: > nit: just DCHECK this Done.
This is indeed looking pretty good. See my comments and expand testing (or add TODOs and expect to address those immediately after landing). https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/138001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:913: unsigned int line = 0; On 2013/08/20 21:25:07, ckocagil wrote: > On 2013/08/15 02:44:17, msw wrote: > > nit: use size_t > > Can I ignore the nits for this method? I will rewrite this method soon after > this CL lands. That's fine. https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:944: const int width = lines_[line_number].size.width() + nit: move |width| into the the first if block, it's only used there. https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:117: struct LineSegment { nit: I suppose this needs an explicit default ctor for |run| (= 0?). https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:638: // or |display_rect_| changes. nit: also whenever |font_list_| changes. https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:83: bool CheckLineIntegrity(const std::vector<internal::Line>& lines, nit: add a comment about what it actually verifies. https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:94: previous_segment = 0; What? Use NULL. Also, won't |previous_segment| *always* be NULL, since it's only set to something valid in the else block (which can only be reached by |previous_segment| being not NULL)? https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:98: return false; Consider turning these into EXPECT_EQ or ASSERT_EQ and using SCOPED_TRACE to give context to the line number and segment number, then this function can drop the bool return value: https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl... https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1686: TEST_F(RenderTextTest, Multiline) { Add some more tests for the approximate size and/or line count with: 1) A huge display rect and text with '\n', '\r', '\r\n', etc. (will it respect line breaks in the text if wrapping isn't *needed*?) 2) A reasonable display rect and strings of various length. (does wrapping work reasonably well for common strings?) 3) A reasonable display rect and strings with very long words. (how are long words, perhaps with hyphen/punctuation, handled?) 4) A reasonable display rect and LTR/RTL/BiDi/Complex strings. (do line breaks around LTR/RTL in each directionality work ok, etc.) And probably more, but that seems like a good start... https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1692: render_text->SetDisplayRect(Rect()); Why does this set an empty display rect? Wouldn't that be an edge case? https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1700: "For kTestStrings[" << i << "]"; Use SCOPED_TRACE here too, instead of piping output to EXPECT_TRUE, to give each CheckLineIntegrity call its context. https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:354: if (segment_start_ == segment_end) { Does this actually happen? Should it DCHECK_NE instead? https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:393: int total_height_; nit: combine total_height_ and common_width_ as a gfx::Size. https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:751: int segment_x = 0; Init this to |preceding_segment_widths|.
https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:944: const int width = lines_[line_number].size.width() + On 2013/08/21 16:46:23, msw wrote: > nit: move |width| into the the first if block, it's only used there. Done. https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:117: struct LineSegment { On 2013/08/21 16:46:23, msw wrote: > nit: I suppose this needs an explicit default ctor for |run| (= 0?). Why do we need a ctor? Why does it only have to initialize |run| and not the other fields? https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:638: // or |display_rect_| changes. On 2013/08/21 16:46:23, msw wrote: > nit: also whenever |font_list_| changes. Updated the comment here. https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:83: bool CheckLineIntegrity(const std::vector<internal::Line>& lines, On 2013/08/21 16:46:23, msw wrote: > nit: add a comment about what it actually verifies. Done. https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:94: previous_segment = 0; On 2013/08/21 16:46:23, msw wrote: > What? Use NULL. Also, won't |previous_segment| *always* be NULL, since it's only > set to something valid in the else block (which can only be reached by > |previous_segment| being not NULL)? Oops. Fixed the logic here and it uses NULL now. While fixing this test I discovered some bugs and did a little refactoring on the way. Sorry for doing it this late in the review process. https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:98: return false; On 2013/08/21 16:46:23, msw wrote: > Consider turning these into EXPECT_EQ or ASSERT_EQ and using SCOPED_TRACE to > give context to the line number and segment number, then this function can drop > the bool return value: > https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl... Done. https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1686: TEST_F(RenderTextTest, Multiline) { On 2013/08/21 16:46:23, msw wrote: > Add some more tests for the approximate size and/or line count with: > 1) A huge display rect and text with '\n', '\r', '\r\n', etc. > (will it respect line breaks in the text if wrapping isn't *needed*?) > 2) A reasonable display rect and strings of various length. > (does wrapping work reasonably well for common strings?) > 3) A reasonable display rect and strings with very long words. > (how are long words, perhaps with hyphen/punctuation, handled?) > 4) A reasonable display rect and LTR/RTL/BiDi/Complex strings. > (do line breaks around LTR/RTL in each directionality work ok, etc.) > And probably more, but that seems like a good start... I added some new test cases. Note that I can't add 1) yet because line break characters aren't implemented yet. I'm trying to come up with more test cases and other ways of testing this class. https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1692: render_text->SetDisplayRect(Rect()); On 2013/08/21 16:46:23, msw wrote: > Why does this set an empty display rect? Wouldn't that be an edge case? An empty (or 1-pixel wide) display rect forces RenderTextWin to put every character in a new line. This generates the maximum number of lines, which is hard for the class to do correctly and a good case to test. https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1700: "For kTestStrings[" << i << "]"; On 2013/08/21 16:46:23, msw wrote: > Use SCOPED_TRACE here too, instead of piping output to EXPECT_TRUE, to give each > CheckLineIntegrity call its context. Done. https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:354: if (segment_start_ == segment_end) { On 2013/08/21 16:46:23, msw wrote: > Does this actually happen? Should it DCHECK_NE instead? I checked it, it happens. I can move this to the callers but I don't see the value in doing so. https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:393: int total_height_; On 2013/08/21 16:46:23, msw wrote: > nit: combine total_height_ and common_width_ as a gfx::Size. Done. https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:751: int segment_x = 0; On 2013/08/21 16:46:23, msw wrote: > Init this to |preceding_segment_widths|. Done.
Here's a few more comments your way. Still more to come since I didn't get a chance to look everything over again yet. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:838: if (line_breaks_.max() == 0) { Nit: Make this an early return and put the above the preceding two lines. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:172: size_t* pos) { Nit: Rename |pos| to |next_char|, or better yet |next_char_index| and change |start_char| to |start_char_index|. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:679: // Compute lines if they're not valid. This is separate from the layout steps Nit: Add a blank line above this. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:731: ui::Range glyphs = CharRangeToGlyphRange(*run, segment->char_range); Nit: |glyph_range| https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:733: DCHECK(segment_width == 0); DCHECK_EQ https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:751: for (size_t g = glyphs.start(); g < glyphs.end(); ++g) { Either use a descriptive name (like glyph or glyph_index), or use k (following i, j, k). https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:776: renderer.DrawPosText(&pos[colored_glyphs.start() - glyphs.start()], You repeat |colored_glyphs.start() - glyphs.start()| and |colored_glyphs.end() - glyphs.start()| a few times below. Please extra things into local variable(s) to avoid the repetition. For example: const SkPoint& start_pos = &pos[colored_glyphs.start() - glyph_range.start()]; const SkPoint& end_pos = &pos[colored_glyphs.end() - glyph_range.start()]; renderer.DrawPosText(&start_pos, ...); const int width = SkScalarCeilToInt(end_pos.x() - start_pos.x()); renderer.DrawDecorations(start_pos.x(), text_offset.y(), width, ...); https://codereview.chromium.org/16867016/diff/205001/ui/views/examples/multil... File ui/views/examples/multiline_example.cc (right): https://codereview.chromium.org/16867016/diff/205001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:21: explicit RenderTextView() Nit: Remove explicit, since no params now.
https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:776: renderer.DrawPosText(&pos[colored_glyphs.start() - glyphs.start()], On 2013/08/26 21:34:41, Alexei Svitkine wrote: > You repeat |colored_glyphs.start() - glyphs.start()| and |colored_glyphs.end() - > glyphs.start()| a few times below. Please extra things into local variable(s) to > avoid the repetition. > > For example: > > const SkPoint& start_pos = &pos[colored_glyphs.start() - glyph_range.start()]; > const SkPoint& end_pos = &pos[colored_glyphs.end() - glyph_range.start()]; > renderer.DrawPosText(&start_pos, ...); > const int width = SkScalarCeilToInt(end_pos.x() - start_pos.x()); > renderer.DrawDecorations(start_pos.x(), text_offset.y(), width, ...); Er, rather: const SkPoint& start_pos = pos[colored_glyphs.start() - glyph_range.start()]; const SkPoint& end_pos = pos[colored_glyphs.end() - glyph_range.start()]; renderer.DrawPosText(&start_pos, ...); const int width = SkScalarCeilToInt(end_pos.x() - start_pos.x()); renderer.DrawDecorations(start_pos.x(), text_offset.y(), width, ...);
The patches are looking more stable with fewer comments :) I think you should add a CL description that elaborates on the high-level approach, the capabilities and limitations of the implementation, and thoughts for follow-up work (particularly testing this as a viable replacement for existing multi-line labels). I'll try to test your code with the example soon, and maybe start looking into similar support on Linux. https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:117: struct LineSegment { On 2013/08/23 21:45:34, ckocagil wrote: > On 2013/08/21 16:46:23, msw wrote: > > nit: I suppose this needs an explicit default ctor for |run| (= 0?). > > Why do we need a ctor? Why does it only have to initialize |run| and not the > other fields? Explicit init of basic types is highly encouraged, as some compilers may not perform default value initialization, and an explicit initial value is much clearer/nicer. Implicitly invoking the ui::Range default ctor (which zeros out its int members) is fine. https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1686: TEST_F(RenderTextTest, Multiline) { On 2013/08/23 21:45:34, ckocagil wrote: > On 2013/08/21 16:46:23, msw wrote: > > Add some more tests for the approximate size and/or line count with: > > 1) A huge display rect and text with '\n', '\r', '\r\n', etc. > > (will it respect line breaks in the text if wrapping isn't *needed*?) > > 2) A reasonable display rect and strings of various length. > > (does wrapping work reasonably well for common strings?) > > 3) A reasonable display rect and strings with very long words. > > (how are long words, perhaps with hyphen/punctuation, handled?) > > 4) A reasonable display rect and LTR/RTL/BiDi/Complex strings. > > (do line breaks around LTR/RTL in each directionality work ok, etc.) > > And probably more, but that seems like a good start... > > I added some new test cases. Note that I can't add 1) yet because line break > characters aren't implemented yet. I'm trying to come up with more test cases > and other ways of testing this class. Hmm, I think base::i18n::BreakIterator should handle newline characters. Also, you might look at break_iterator_unittest.cc or existing strings used in multi-line labels for test ideas. I think you're off to a good start for testing, but this definitely needs more test cases and varied expectations. My first thought is that you need to thoroughly test multi-line cases where each line has more than one segment. It's ok to leave some testing ideas in a TODO or bug comment for a followup CL. https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1692: render_text->SetDisplayRect(Rect()); On 2013/08/23 21:45:34, ckocagil wrote: > On 2013/08/21 16:46:23, msw wrote: > > Why does this set an empty display rect? Wouldn't that be an edge case? > > An empty (or 1-pixel wide) display rect forces RenderTextWin to put every > character in a new line. This generates the maximum number of lines, which is > hard for the class to do correctly and a good case to test. Cool, 1px makes more sense; I figure 0px should perhaps not break lines. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:451: // Reset the layout and |lines_| to be invalid. I'd avoid describing the internals of a pure virtual function, especially since Linux/Mac doesn't do this. (I'd just remove this comment change for now). Eventually a onsolidated RenderText impl should simply mark the layout (and lines_) as invalid, and then that can be explained here. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:638: // Lines computed by EnsureLayout. |ResetLayout()| implicitly invalidates nit: I liked the old comment format better; ResetLayout() is pure virtual, so we can't really describe its behavior here. But describing when lines *should* be reset in terms of layout and display rect updates seems okay. Consider: "Lines computed by EnsureLayout; these should be invalidated with ResetLayout and on |display_rect_| changes." https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:105: "current x_range: [%" PRIuS ", %" PRIuS ")", nit: printing ui::Range ints with PRIuS (unsigned size) is wrong here and below. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1713: render_text->Draw(&canvas); Is it sufficient to EnsureLayout instead of calling Draw here and below? https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1724: { L"abc defg hijkl mnop qrstuv wxyz", ui::Range(0, 15), ui::Range(15, 31) }, nit: these test cases could be covered with 50px width and half-length strings. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:304: // to |lines_.back()|. nit: This doesn't sound right. BreakRun appends lines as needed too. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:313: const int current_char = next_char; nit: size_t ?
Here's another volley of comments. Looking quite good, though. https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:430: Size RenderTextWin::GetMultilineTextSize() { On 2013/08/10 13:39:23, ckocagil wrote: > On 2013/08/08 15:01:03, Alexei Svitkine wrote: > > I think you should make GetStringSize() return the multiline size by default > > when in multi-line mode. > > > > i.e. remove this extra method and fold the logic. > > > > (Else, you'd at least need to add implementations of this to the other render > > text subclasses.) > > But if GetStringSize returns the display width and not single line string width, > the controls using RenderText won't be able to know their preferred size. I > think users of RenderText will always need to know 1) the single-line size of > the text (for their preferred size) and 2) the multi-line height (for > GetHeightForWidth). > > I will add empty NOTIMPLEMENTED() methods to all platforms that need them, > probably after the review to not make this CL any harder to review. Re-visiting this discussion (I didn't follow-up on this when you replied originally). I'm still skeptical that we really need both. I'm not following you when you say "GetStringSize returns the display width and not single line string width". It seems GetStringSize does return the actual width, not the display width (i.e. not the width of the display rect). Unless you meant something else by saying "returns the display width"? Perhaps you meant that it currently doesn't cap the returned width to the display rect's width? I think that's currently true, but it doesn't have to be - we could change it (though not in this CL). But even if we don't, I think it's okay if semantics are slightly different in multiline mode - i.e. if multiline is set, return the equivalent of the current GetMultilineTextSize() from GetStringSize(). Or is there something I'm missing about why we would want to keep the non-multiline-aware GetStringSize() functionality in multi-line mode? https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:902: unsigned int line = 0; size_t? https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:137: gfx::Size size; Nit: Remove gfx. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:95: const internal::LineSegment* const segment = &lines[i].segments[j]; Nit: I don't think we usually put const after the *. Please remove, here and on the line below. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:181: if (next_word != breaks.breaks().end() && i >= next_word->first) { Add a comment above this explaining the logic. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:186: ui::Range glyphs = CharRangeToGlyphRange(run, ui::Range(i, i + 1)); Nit: glyph_range https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:258: // the given runs. Nit: "the given run." -> "a given run." https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:259: class LineBreaker { I think it may be worth exposing the interface of this class in the header file, so that its logic can be tested directly (rather than just through RenderTextWin) - which should be possible since this class is in the internal namespace (i.e. like TextRun). However, I think at this point we can leave that to a follow-up CL, since this CL is looking quite good now and is getting ready to land. However, can you add a TODO about it in the comment above? Thanks! https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:278: TextRun* run = runs_[run_index]; Nit: const https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:326: void PopRtl() { I think a more descriptive name would be better here. How about: UpdateRTLSegmentRanges() https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:331: LineSegment* const segment = SegmentFromHandle(rtl_segments_[i - 1]); Nit: Remove const here, since |segment|'s data is changing and marking the variable itself as const is not very useful here. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:332: segment->x_range = ui::Range(x, x + segment->x_range.length()); Nit: Extract segment->x_range.length() into a local var |segment_width|, since you use it below too (and it doesn't change). https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:338: void AdvanceLine() { Nit: Add a brief comment. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:353: void AddSegment(int run_index, ui::Range char_range, int width) { Nit: Add a brief comment. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:362: LineSegment segment; Nit: Add a blank line above this. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:366: Line* line = &lines_.back(); Nit: Add a blank line above this. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:372: if (char_range.end() == run->range.end()) Add a comment about this (i.e. mention that this reprocesses the run ranges at the end of the run). https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:399: // Segments to be applied by |PopRtl()|. Mention that this contains segments of the current run, if it's RTL.
I'll address the rest of Alexei's comments very soon. https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:117: struct LineSegment { On 2013/08/27 01:30:52, msw wrote: > On 2013/08/23 21:45:34, ckocagil wrote: > > On 2013/08/21 16:46:23, msw wrote: > > > nit: I suppose this needs an explicit default ctor for |run| (= 0?). > > > > Why do we need a ctor? Why does it only have to initialize |run| and not the > > other fields? > > Explicit init of basic types is highly encouraged, as some compilers may not > perform default value initialization, and an explicit initial value is much > clearer/nicer. Implicitly invoking the ui::Range default ctor (which zeros out > its int members) is fine. Done. https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/16867016/diff/195001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1686: TEST_F(RenderTextTest, Multiline) { On 2013/08/27 01:30:52, msw wrote: > On 2013/08/23 21:45:34, ckocagil wrote: > > On 2013/08/21 16:46:23, msw wrote: > > > Add some more tests for the approximate size and/or line count with: > > > 1) A huge display rect and text with '\n', '\r', '\r\n', etc. > > > (will it respect line breaks in the text if wrapping isn't *needed*?) > > > 2) A reasonable display rect and strings of various length. > > > (does wrapping work reasonably well for common strings?) > > > 3) A reasonable display rect and strings with very long words. > > > (how are long words, perhaps with hyphen/punctuation, handled?) > > > 4) A reasonable display rect and LTR/RTL/BiDi/Complex strings. > > > (do line breaks around LTR/RTL in each directionality work ok, etc.) > > > And probably more, but that seems like a good start... > > > > I added some new test cases. Note that I can't add 1) yet because line break > > characters aren't implemented yet. I'm trying to come up with more test cases > > and other ways of testing this class. > > Hmm, I think base::i18n::BreakIterator should handle newline characters. Also, > you might look at break_iterator_unittest.cc or existing strings used in > multi-line labels for test ideas. I think you're off to a good start for > testing, but this definitely needs more test cases and varied expectations. My > first thought is that you need to thoroughly test multi-line cases where each > line has more than one segment. It's ok to leave some testing ideas in a TODO or > bug comment for a followup CL. BreakIterator only gives positions that are suitable for breaking lines at, it doesn't tell where the newline characters are. I was planning on adding newline support in another CL. I'm adding a bug comment to extend the tests. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:838: if (line_breaks_.max() == 0) { On 2013/08/26 21:34:41, Alexei Svitkine wrote: > Nit: Make this an early return and put the above the preceding two lines. Done. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:451: // Reset the layout and |lines_| to be invalid. On 2013/08/27 01:30:52, msw wrote: > I'd avoid describing the internals of a pure virtual function, especially since > Linux/Mac doesn't do this. (I'd just remove this comment change for now). > Eventually a onsolidated RenderText impl should simply mark the layout (and > lines_) as invalid, and then that can be explained here. Done. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:638: // Lines computed by EnsureLayout. |ResetLayout()| implicitly invalidates On 2013/08/27 01:30:52, msw wrote: > nit: I liked the old comment format better; ResetLayout() is pure virtual, so we > can't really describe its behavior here. But describing when lines *should* be > reset in terms of layout and display rect updates seems okay. Consider: "Lines > computed by EnsureLayout; these should be invalidated with ResetLayout and on > |display_rect_| changes." Done. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:105: "current x_range: [%" PRIuS ", %" PRIuS ")", On 2013/08/27 01:30:52, msw wrote: > nit: printing ui::Range ints with PRIuS (unsigned size) is wrong here and below. Is it? ui::Range::start() and end() both return size_t. Is there a variadic arg default conversion I'm overlooking? Anyway, I'll just use ui::Range::ToString here since it's simpler. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1713: render_text->Draw(&canvas); On 2013/08/27 01:30:52, msw wrote: > Is it sufficient to EnsureLayout instead of calling Draw here and below? Without Draw() we won't hit the DrawVisualText() DCHECKs. I'm not sure whether EnsureLayout is good enough. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1724: { L"abc defg hijkl mnop qrstuv wxyz", ui::Range(0, 15), ui::Range(15, 31) }, On 2013/08/27 01:30:52, msw wrote: > nit: these test cases could be covered with 50px width and half-length strings. Done. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:172: size_t* pos) { On 2013/08/26 21:34:41, Alexei Svitkine wrote: > Nit: Rename |pos| to |next_char|, or better yet |next_char_index| and change > |start_char| to |start_char_index|. Done. I made it |next_char| instead of |next_char_index|. The latter looks a bit too verbose to me. Not sure though, I can change it if you (or Mike) think the latter looks better. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:304: // to |lines_.back()|. On 2013/08/27 01:30:52, msw wrote: > nit: This doesn't sound right. BreakRun appends lines as needed too. Updated. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:313: const int current_char = next_char; On 2013/08/27 01:30:52, msw wrote: > nit: size_t ? Done. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:679: // Compute lines if they're not valid. This is separate from the layout steps On 2013/08/26 21:34:41, Alexei Svitkine wrote: > Nit: Add a blank line above this. Done. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:731: ui::Range glyphs = CharRangeToGlyphRange(*run, segment->char_range); On 2013/08/26 21:34:41, Alexei Svitkine wrote: > Nit: |glyph_range| Done. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:733: DCHECK(segment_width == 0); On 2013/08/26 21:34:41, Alexei Svitkine wrote: > DCHECK_EQ Done. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:751: for (size_t g = glyphs.start(); g < glyphs.end(); ++g) { On 2013/08/26 21:34:41, Alexei Svitkine wrote: > Either use a descriptive name (like glyph or glyph_index), or use k (following > i, j, k). Done, I went with "k". https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:776: renderer.DrawPosText(&pos[colored_glyphs.start() - glyphs.start()], On 2013/08/26 21:35:33, Alexei Svitkine wrote: > On 2013/08/26 21:34:41, Alexei Svitkine wrote: > > You repeat |colored_glyphs.start() - glyphs.start()| and |colored_glyphs.end() > - > > glyphs.start()| a few times below. Please extra things into local variable(s) > to > > avoid the repetition. > > > > For example: > > > > const SkPoint& start_pos = &pos[colored_glyphs.start() - > glyph_range.start()]; > > const SkPoint& end_pos = &pos[colored_glyphs.end() - glyph_range.start()]; > > renderer.DrawPosText(&start_pos, ...); > > const int width = SkScalarCeilToInt(end_pos.x() - start_pos.x()); > > renderer.DrawDecorations(start_pos.x(), text_offset.y(), width, ...); > > Er, rather: > > const SkPoint& start_pos = pos[colored_glyphs.start() - glyph_range.start()]; > const SkPoint& end_pos = pos[colored_glyphs.end() - glyph_range.start()]; > renderer.DrawPosText(&start_pos, ...); > const int width = SkScalarCeilToInt(end_pos.x() - start_pos.x()); > renderer.DrawDecorations(start_pos.x(), text_offset.y(), width, ...); Done. https://codereview.chromium.org/16867016/diff/205001/ui/views/examples/multil... File ui/views/examples/multiline_example.cc (right): https://codereview.chromium.org/16867016/diff/205001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:21: explicit RenderTextView() On 2013/08/26 21:34:41, Alexei Svitkine wrote: > Nit: Remove explicit, since no params now. Done.
https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:430: Size RenderTextWin::GetMultilineTextSize() { On 2013/08/27 15:40:38, Alexei Svitkine wrote: > On 2013/08/10 13:39:23, ckocagil wrote: > > On 2013/08/08 15:01:03, Alexei Svitkine wrote: > > > I think you should make GetStringSize() return the multiline size by default > > > when in multi-line mode. > > > > > > i.e. remove this extra method and fold the logic. > > > > > > (Else, you'd at least need to add implementations of this to the other > render > > > text subclasses.) > > > > But if GetStringSize returns the display width and not single line string > width, > > the controls using RenderText won't be able to know their preferred size. I > > think users of RenderText will always need to know 1) the single-line size of > > the text (for their preferred size) and 2) the multi-line height (for > > GetHeightForWidth). > > > > I will add empty NOTIMPLEMENTED() methods to all platforms that need them, > > probably after the review to not make this CL any harder to review. > > Re-visiting this discussion (I didn't follow-up on this when you replied > originally). I'm still skeptical that we really need both. > > I'm not following you when you say "GetStringSize returns the display width and > not single line string width". It seems GetStringSize does return the actual > width, not the display width (i.e. not the width of the display rect). Unless > you meant something else by saying "returns the display width"? I said "_if_ GetStringSize returns the display width", that "if" is important there :-) > Perhaps you meant that it currently doesn't cap the returned width to the > display rect's width? I think that's currently true, but it doesn't have to be - > we could change it (though not in this CL). But even if we don't, I think it's > okay if semantics are slightly different in multiline mode - i.e. if multiline > is set, return the equivalent of the current GetMultilineTextSize() from > GetStringSize(). > > Or is there something I'm missing about why we would want to keep the > non-multiline-aware GetStringSize() functionality in multi-line mode? In multi-line mode, there are two uses for the RenderText string size functions: - Getting the single-line string size so that labels can pass this value to View::GetPreferredSize(). - Getting the current multi-line height so that labels can pass this value to View::GetHeightForWidth(). We can't give both values with a single function, so we need a new one. I don't know Views well though, so please let me know if we don't need to supply both of these values. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:902: unsigned int line = 0; On 2013/08/27 15:40:38, Alexei Svitkine wrote: > size_t? Done. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:137: gfx::Size size; On 2013/08/27 15:40:38, Alexei Svitkine wrote: > Nit: Remove gfx. Done. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:95: const internal::LineSegment* const segment = &lines[i].segments[j]; On 2013/08/27 15:40:38, Alexei Svitkine wrote: > Nit: I don't think we usually put const after the *. Please remove, here and on > the line below. The const after the * makes the pointer itself const. There is already other code in this file that declares const pointers. Should I remove all these instances or keep them? https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:181: if (next_word != breaks.breaks().end() && i >= next_word->first) { On 2013/08/27 15:40:38, Alexei Svitkine wrote: > Add a comment above this explaining the logic. Done. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:186: ui::Range glyphs = CharRangeToGlyphRange(run, ui::Range(i, i + 1)); On 2013/08/27 15:40:38, Alexei Svitkine wrote: > Nit: glyph_range Done. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:258: // the given runs. On 2013/08/27 15:40:38, Alexei Svitkine wrote: > Nit: "the given run." -> "a given run." I don't understand. A Line can contain multiple runs. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:259: class LineBreaker { On 2013/08/27 15:40:38, Alexei Svitkine wrote: > I think it may be worth exposing the interface of this class in the header file, > so that its logic can be tested directly (rather than just through > RenderTextWin) - which should be possible since this class is in the internal > namespace (i.e. like TextRun). > > However, I think at this point we can leave that to a follow-up CL, since this > CL is looking quite good now and is getting ready to land. > > However, can you add a TODO about it in the comment above? Thanks! Good idea, that should make testing easier and better. Adding a TODO here. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:278: TextRun* run = runs_[run_index]; On 2013/08/27 15:40:38, Alexei Svitkine wrote: > Nit: const Done. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:326: void PopRtl() { On 2013/08/27 15:40:38, Alexei Svitkine wrote: > I think a more descriptive name would be better here. > > How about: UpdateRTLSegmentRanges() Done. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:331: LineSegment* const segment = SegmentFromHandle(rtl_segments_[i - 1]); On 2013/08/27 15:40:38, Alexei Svitkine wrote: > Nit: Remove const here, since |segment|'s data is changing and marking the > variable itself as const is not very useful here. Done. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:332: segment->x_range = ui::Range(x, x + segment->x_range.length()); On 2013/08/27 15:40:38, Alexei Svitkine wrote: > Nit: Extract segment->x_range.length() into a local var |segment_width|, since > you use it below too (and it doesn't change). Done. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:338: void AdvanceLine() { On 2013/08/27 15:40:38, Alexei Svitkine wrote: > Nit: Add a brief comment. Done. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:353: void AddSegment(int run_index, ui::Range char_range, int width) { On 2013/08/27 15:40:38, Alexei Svitkine wrote: > Nit: Add a brief comment. Done. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:362: LineSegment segment; On 2013/08/27 15:40:38, Alexei Svitkine wrote: > Nit: Add a blank line above this. Done. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:366: Line* line = &lines_.back(); On 2013/08/27 15:40:38, Alexei Svitkine wrote: > Nit: Add a blank line above this. Done. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:372: if (char_range.end() == run->range.end()) On 2013/08/27 15:40:38, Alexei Svitkine wrote: > Add a comment about this (i.e. mention that this reprocesses the run ranges at > the end of the run). Done. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:399: // Segments to be applied by |PopRtl()|. On 2013/08/27 15:40:38, Alexei Svitkine wrote: > Mention that this contains segments of the current run, if it's RTL. Done.
https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:430: Size RenderTextWin::GetMultilineTextSize() { On 2013/08/28 19:16:26, ckocagil wrote: > In multi-line mode, there are two uses for the RenderText string size functions: > - Getting the single-line string size so that labels can pass this value to > View::GetPreferredSize(). > - Getting the current multi-line height so that labels can pass this value to > View::GetHeightForWidth(). > > We can't give both values with a single function, so we need a new one. I don't > know Views well though, so please let me know if we don't need to supply both of > these values. I think for GetHeightForWidth() with different widths passed in, we would already be resetting the display rect before querying the new size - so we can similarly reset the multiline_ state for GetPreferredSize() and get the new string size. I think for now this CL shouldn't add the new API and instead make GetStringSize return the multi-line size when in multi-line mode. If we do find that we need the additional API (i.e. at the time that we make Label use it or something else), then we can always re-visit this decision. But for now, let's not add the new API for this. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:95: const internal::LineSegment* const segment = &lines[i].segments[j]; On 2013/08/28 19:16:26, ckocagil wrote: > On 2013/08/27 15:40:38, Alexei Svitkine wrote: > > Nit: I don't think we usually put const after the *. Please remove, here and > on > > the line below. > > The const after the * makes the pointer itself const. There is already other > code in this file that declares const pointers. Should I remove all these > instances or keep them? The other ones only seem to do it for arrays of const char* strings, rather than const pointers to an actual class instance. I'd keep those other ones, since there's precedent (in this file and elsewhere in the codebase) for those, but remove these ones (the const keywords after the * only). https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:258: // the given runs. On 2013/08/28 19:16:26, ckocagil wrote: > On 2013/08/27 15:40:38, Alexei Svitkine wrote: > > Nit: "the given run." -> "a given run." > > I don't understand. A Line can contain multiple runs. Ah right - my comment was wrong. Disregard. https://codereview.chromium.org/16867016/diff/231001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/231001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:407: ResetLayout(); Is it necessary to reset the complete layout here or can you get away with clearing lines_?
Just some quick comments, I think I'll take another deep look when you've addressed the latest round. https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:430: Size RenderTextWin::GetMultilineTextSize() { On 2013/08/28 21:27:46, Alexei Svitkine wrote: > On 2013/08/28 19:16:26, ckocagil wrote: > > In multi-line mode, there are two uses for the RenderText string size > functions: > > - Getting the single-line string size so that labels can pass this value to > > View::GetPreferredSize(). > > - Getting the current multi-line height so that labels can pass this value to > > View::GetHeightForWidth(). > > > > We can't give both values with a single function, so we need a new one. I > don't > > know Views well though, so please let me know if we don't need to supply both > of > > these values. > > I think for GetHeightForWidth() with different widths passed in, we would > already be resetting the display rect before querying the new size - so we can > similarly reset the multiline_ state for GetPreferredSize() and get the new > string size. > > I think for now this CL shouldn't add the new API and instead make GetStringSize > return the multi-line size when in multi-line mode. If we do find that we need > the additional API (i.e. at the time that we make Label use it or something > else), then we can always re-visit this decision. But for now, let's not add the > new API for this. I think the uses of View's GetPreferredSize and GetHeightForWidth will make it tough to have a single function, but following Alexei's advice for now should be okay. Eventually, caching sizes for given/unlimited widths would likely be helpful to avoid actually resizing text that's already displayed with a different width. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:105: "current x_range: [%" PRIuS ", %" PRIuS ")", On 2013/08/28 18:01:45, ckocagil wrote: > On 2013/08/27 01:30:52, msw wrote: > > nit: printing ui::Range ints with PRIuS (unsigned size) is wrong here and > below. > > Is it? ui::Range::start() and end() both return size_t. Is there a variadic arg > default conversion I'm overlooking? Anyway, I'll just use ui::Range::ToString > here since it's simpler. Hmm, I mistakenly thought they were ints; glad you improved it anyway. https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:172: size_t* pos) { On 2013/08/28 18:01:45, ckocagil wrote: > On 2013/08/26 21:34:41, Alexei Svitkine wrote: > > Nit: Rename |pos| to |next_char|, or better yet |next_char_index| and change > > |start_char| to |start_char_index|. > > Done. I made it |next_char| instead of |next_char_index|. The latter looks a bit > too verbose to me. Not sure though, I can change it if you (or Mike) think the > latter looks better. |end_char| corresponds w/|start_char| better, but |next_char| is ok. https://codereview.chromium.org/16867016/diff/231001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/231001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:838: if (line_breaks_.max()) nit: add an explicit "!= 0"
https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:430: Size RenderTextWin::GetMultilineTextSize() { On 2013/08/30 00:42:28, msw wrote: > On 2013/08/28 21:27:46, Alexei Svitkine wrote: > > On 2013/08/28 19:16:26, ckocagil wrote: > > > In multi-line mode, there are two uses for the RenderText string size > > functions: > > > - Getting the single-line string size so that labels can pass this value to > > > View::GetPreferredSize(). > > > - Getting the current multi-line height so that labels can pass this value > to > > > View::GetHeightForWidth(). > > > > > > We can't give both values with a single function, so we need a new one. I > > don't > > > know Views well though, so please let me know if we don't need to supply > both > > of > > > these values. > > > > I think for GetHeightForWidth() with different widths passed in, we would > > already be resetting the display rect before querying the new size - so we can > > similarly reset the multiline_ state for GetPreferredSize() and get the new > > string size. > > > > I think for now this CL shouldn't add the new API and instead make > GetStringSize > > return the multi-line size when in multi-line mode. If we do find that we need > > the additional API (i.e. at the time that we make Label use it or something > > else), then we can always re-visit this decision. But for now, let's not add > the > > new API for this. > > I think the uses of View's GetPreferredSize and GetHeightForWidth will make it > tough to have a single function, but following Alexei's advice for now should be > okay. Eventually, caching sizes for given/unlimited widths would likely be > helpful to avoid actually resizing text that's already displayed with a > different width. Alexei's advice on change multiline mode to get single-line size is a good idea (if it doesn't affect performance too much). However, I can't get rid of the need for the single-line size in RenderText::GetCursorBounds() (see the GetStringSize() call there). Thoughts? https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/16867016/diff/205001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:95: const internal::LineSegment* const segment = &lines[i].segments[j]; On 2013/08/28 21:27:46, Alexei Svitkine wrote: > On 2013/08/28 19:16:26, ckocagil wrote: > > On 2013/08/27 15:40:38, Alexei Svitkine wrote: > > > Nit: I don't think we usually put const after the *. Please remove, here and > > on > > > the line below. > > > > The const after the * makes the pointer itself const. There is already other > > code in this file that declares const pointers. Should I remove all these > > instances or keep them? > > The other ones only seem to do it for arrays of const char* strings, rather than > const pointers to an actual class instance. I'd keep those other ones, since > there's precedent (in this file and elsewhere in the codebase) for those, but > remove these ones (the const keywords after the * only). Done. https://codereview.chromium.org/16867016/diff/231001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/231001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:407: ResetLayout(); On 2013/08/28 21:27:46, Alexei Svitkine wrote: > Is it necessary to reset the complete layout here or can you get away with > clearing lines_? Done. https://codereview.chromium.org/16867016/diff/231001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:838: if (line_breaks_.max()) On 2013/08/30 00:42:28, msw wrote: > nit: add an explicit "!= 0" Done.
https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:430: Size RenderTextWin::GetMultilineTextSize() { On 2013/08/30 16:53:36, ckocagil wrote: > Alexei's advice on change multiline mode to get single-line size is a good idea > (if it doesn't affect performance too much). However, I can't get rid of the > need for the single-line size in RenderText::GetCursorBounds() (see the > GetStringSize() call there). Thoughts? I think the current GetCursorBounds() logic is incorrect in multi-line mode. I suspect more work will be needed to enable editable multi-line text to work (vs just the support being added in this CL for drawing it). Given the above, I think it's fine to file a bug and add a TODO to GetCursorBounds() to make it work in multiline mode and keep it as-is for now (i.e. still calling GetStringSize() which would give the wrong results in multiline mode but will continue to work for single-line mode).
On 2013/08/30 17:07:24, Alexei Svitkine wrote: > https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc > File ui/gfx/render_text_win.cc (right): > > https://codereview.chromium.org/16867016/diff/96001/ui/gfx/render_text_win.cc... > ui/gfx/render_text_win.cc:430: Size RenderTextWin::GetMultilineTextSize() { > On 2013/08/30 16:53:36, ckocagil wrote: > > Alexei's advice on change multiline mode to get single-line size is a good > idea > > (if it doesn't affect performance too much). However, I can't get rid of the > > need for the single-line size in RenderText::GetCursorBounds() (see the > > GetStringSize() call there). Thoughts? > > I think the current GetCursorBounds() logic is incorrect in multi-line mode. I > suspect more work will be needed to enable editable multi-line text to work (vs > just the support being added in this CL for drawing it). > > Given the above, I think it's fine to file a bug and add a TODO to > GetCursorBounds() to make it work in multiline mode and keep it as-is for now > (i.e. still calling GetStringSize() which would give the wrong results in > multiline mode but will continue to work for single-line mode). I see, the cursor height should be equal to the height of the line it is on. There might be other issues in GetCursorBounds() in multi-line mode. Adding TODO, opening a bug, and removing GetMultilineTextSize().
Looks great, with just one comment below. Will wait for Mike's review before giving a real el gee tee em. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:328: // Returns the width of multi-line content. Reserves room for the cursor if I think "of the multi-line content" is misleading here, since it also works in single-line mode. Perhaps something like "Returns the width of the content (which is the wrapped width in multi-line mode)."
There are a lot of comments from this full pass, but they are mostly minor comment nits. Overall, this is looking very good; your hard work to produce a high-quality CL is plainly visible. I wish it were more comprehensively tested right off the bat, instead of leaving that for followup CLs, but I'd still be quite happy to see this land soon with the minimal initial testing it already has (plus the one I suggest). https://codereview.chromium.org/16867016/diff/246001/ui/gfx/break_list.h File ui/gfx/break_list.h (right): https://codereview.chromium.org/16867016/diff/246001/ui/gfx/break_list.h#newc... ui/gfx/break_list.h:39: size_t max() const { return max_; } nit: move this directly below SetMax. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:702: // TODO(ckocagil): Support multi-line. This function should return the height of nit: move this comment inside the function or to the header's function decl. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:849: base::i18n::BreakIterator::BREAK_LINE); nit: out-dent one space. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:850: bool success = iter.Init(); nit: const. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:852: if (success) { optional: I think the first break would always be 0, so you could just do: while (success && iter.Advance()) line_breaks_.ApplyValue(iter.pos(), ui::Range(iter.pos(), text_length)); But that's a little non-obvious, so it's entirely optional. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:889: offset.Add(GetUpdatedDisplayOffset()); Shouldn't it be okay to add the display offset even in multiline? I think that'd give us horizontal scrolling of multiline text for free. Otherwise, add a TODO here like the one in UpdateCachedBoundsAndOffset. // TODO(ckocagil): Implement multiline text scrolling. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:898: // TODO(ckocagil): implement multiline. nit: capitalize "Implement", consider expanding the TODO: // TODO(ckocagil): Convert multi-line view space points to text space. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:905: // TODO(ckocagil): refactor this to traverse all line segments to support rtl. nit: capitalize 'Refactor' and 'RTL'; consider: // TODO(ckocagil): Traverse individual line segments for RTL support. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:909: while (line < lines_.size() && x > lines_[line].size.width()) { Optionally, you could do: for (line = 0; line < lines_.size() && x > lines_[line].size.width(); ++line) x -= lines_[line].size.width(); https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:932: Vector2d offset = GetLineOffset(line); nit: const https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1076: // TODO(ckocagil): Add vertical offset support for scrolling multi-line text. nit: Address my GetLineOffset comment or remove "Vertical". Multi-line doesn't use the display offset and thus doesn't support horizontal scrolling either. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:115: // This structure holds the logical and visual position information for a line nit: remove this second sentence of the comment; it's superfluous. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:120: // X coordinates of this line segment in text coordinate space. nit: "text space" https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:130: // Holds a list of line segments and metrics for a line. nit: consider: // A line of layout text, comprised of a line segment list and some metrics. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:137: // Total size of this line. Width is the sum of |segment| widths, height is nit: consider: // A line size is the sum of segment widths and the maximum of segment heights. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:141: // Sum of the |height| fields of preceding lines. nit: |height| doesn't exist anymore; use "Sum of the preceding lines' heights." https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:144: // Maximum baseline of all runs on this line. nit: s/runs/segments/ https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:223: // TODO(ckocagil): Multiline text rendering is currently only supported on nit: try to be consistent about using either "multiline" or "multi-line" in your comments throughout the patch. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:328: // Returns the width of multi-line content. Reserves room for the cursor if On 2013/08/30 18:45:25, Alexei Svitkine wrote: > I think "of the multi-line content" is misleading here, since it also works in > single-line mode. > > Perhaps something like "Returns the width of the content (which is the wrapped > width in multi-line mode)." Agreed, and ditto for the GetStringSize() comment. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:459: // Returns layout text positions that are suitable for breaking lines at. nit: remove trailing "at". https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:466: // Returns the line offset from the origin after applying text alignment and nit: please fix this to "the text alignment and the display offset." https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:475: // Convert the range in text coordinates to rects in view coordinates that nit: s/coordinates/space/ (times two) to match the comment above. Or consider: // Convert a text space x-coordinate range to corresponding rects in view space. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:479: // Returns the line offset from the origin, taking into account text alignment nit: // Returns the line offset from the origin, accounting for text alignment only. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:631: // BreakList to find valid positions to break the line at. nit: consider: // A list of valid layout text line break positions. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:113: else if (i != previous_segment_line) I realize that we shouldn't break a run into separate segments, except to split the run onto different lines, but I don't think this check on the else is necessary. Shouldn't the text-space start/end x-coordinates of the segments match, regardless of the line that the consecutive segments are placed on? https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1697: TEST_F(RenderTextTest, Multiline_MinWidth) { I'm looking forward to many more tests. Can you add this one now? Add a Multiline_SufficientWidth or similar to test trivial behavior: SetMultiline(true) but use strings (LTR, RTL, spaces, no spaces) that fit on one line and don't wrap with the sufficiently wide display rect width. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1711: CheckLineIntegrity(render_text->lines_, render_text->runs_); Can you EXPECT_GT(render_text->lines_.size(), 1U) here? https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1717: wchar_t* text; nit: make all these members const. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1718: ui::Range first_line; nit: |first_line_char_range| and |second_line_char_range| https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1722: { L"qwertyuiop", ui::Range(0, 8), ui::Range(8, 10) }, nit: indent the ui::Range entries to match, even on line 1724. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:160: // be greater than |start_char|. nit: add "(to avoid constructing empty lines)." https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:165: // TODO(ckocagil): Do not reserve space for whitespace at the end of lines. nit: s/space/width/ https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:176: // Width from |std::max(word->first, start_char)|. nit: ... to the current character. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:313: TextRun* const run = runs_[run_index]; nit: can this be const TextRun* const? Sorry if you already answered this. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:320: BreakRunAtWidth(*run, *words_, next_char, max_width_ - line_x_, nit: send |current_char| instead of |next_char|, just for clarity. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:401: // |text_x_| and |line_x_| are text-space and line-space x coordinates of the nit: remove "|text_x_| and |line_x_| are ". https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:413: // Segments to be applied by |UpdateRTLSegmentRanges()|. Contains the segments nit: consider "// The current RTL run segments, to be applied by |UpdateRTLSegmentRanges()|." https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:430: common_baseline_(0), Remove this and just return the first line's baseline in GetBaseline(). https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:446: // |string_size_| if possible. nit: add some detail about why this should/can't be done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:626: DCHECK(bounds.empty() || bounds.back().GetMin() != range_x.GetMax()); nit: Use DCHECK(bounds.empty() || bounds.back().GetMax() <= range_x.GetMin()); https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:708: DCHECK(!multiline() || !lines().empty()); Shouldn't lines() be non-empty here, even in the single-line case? https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:728: Vector2d text_offset = line_offset + Vector2d(0, line.baseline); nit: declare |text_offset| and |preceding_segment_widths| at line 736 https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:743: if (glyph_range.is_empty()) { Can you instead DCHECK(!glyph_range.is_empty())? Sorry if you already answered. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:764: SkIntToScalar(text_offset.x() + segment_x + run->offsets[k].du), nit: swap the order of |segment_x| and |run->offsets[k].du| https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:775: colors().GetBreak(run->range.start()); Should run->range.start() be changed to segment->char_range.start()? https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:776: it != colors().breaks().end() && it->first < run->range.end(); Should run->range.end() be changed to segment->char_range.end()? https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:783: if (colored_glyphs.is_empty()) With the above changes to use segment->char_range, can this be a DCHECK? https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.h File ui/gfx/render_text_win.h (right): https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.h... ui/gfx/render_text_win.h:127: Size multiline_string_size_; Disambiguate these two sizes in a comment, what does each hold? https://codereview.chromium.org/16867016/diff/246001/ui/views/examples/multil... File ui/views/examples/multiline_example.cc (right): https://codereview.chromium.org/16867016/diff/246001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:35: gfx::Size size(render_text_->GetContentWidth(), Can you simply init |size| to render_text_->GetStringSize()? https://codereview.chromium.org/16867016/diff/246001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:43: if (w == 0) // TODO(ckocagil): Why does this happen? nit: Move the comment up to the previous line. https://codereview.chromium.org/16867016/diff/246001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:53: ui::Range test_range(1, 21); Ensure new_contents is at least 12 chars. Also nit: const. https://codereview.chromium.org/16867016/diff/246001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:54: render_text_->MoveCursorTo(gfx::SelectionModel(test_range, nit: can you call RenderText::ApplyColor additionally or instead? https://codereview.chromium.org/16867016/diff/246001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:116: column_set->AddColumn(GridLayout::LEADING, GridLayout::CENTER, Instead of adding a new column to place the empty-labeled Checkbox next to the "views::Label:" Label, simply set the Checkbox text to "views::Label:". https://codereview.chromium.org/16867016/diff/246001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:153: if (label_checkbox_->checked()) optional nit: label_->SetText(label_checkbox_->checked() ? textfield_->text() : string16());
https://codereview.chromium.org/16867016/diff/246001/ui/gfx/break_list.h File ui/gfx/break_list.h (right): https://codereview.chromium.org/16867016/diff/246001/ui/gfx/break_list.h#newc... ui/gfx/break_list.h:39: size_t max() const { return max_; } On 2013/09/06 23:47:45, msw wrote: > nit: move this directly below SetMax. Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:702: // TODO(ckocagil): Support multi-line. This function should return the height of On 2013/09/06 23:47:45, msw wrote: > nit: move this comment inside the function or to the header's function decl. Done, moved inside the function. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:849: base::i18n::BreakIterator::BREAK_LINE); On 2013/09/06 23:47:45, msw wrote: > nit: out-dent one space. Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:850: bool success = iter.Init(); On 2013/09/06 23:47:45, msw wrote: > nit: const. Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:852: if (success) { On 2013/09/06 23:47:45, msw wrote: > optional: I think the first break would always be 0, so you could just do: > while (success && iter.Advance()) > line_breaks_.ApplyValue(iter.pos(), ui::Range(iter.pos(), text_length)); > But that's a little non-obvious, so it's entirely optional. I would rather keep the explicit if block. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:889: offset.Add(GetUpdatedDisplayOffset()); On 2013/09/06 23:47:45, msw wrote: > Shouldn't it be okay to add the display offset even in multiline? > I think that'd give us horizontal scrolling of multiline text for free. > Otherwise, add a TODO here like the one in UpdateCachedBoundsAndOffset. > // TODO(ckocagil): Implement multiline text scrolling. I'm disabling it in multiline because I need to test it with many cases to see whether it currently works or not, which would probably be better done in a followup CL. I have a TODO in UpdateCachedBoundsAndOffset() about this, should I move it here? https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:898: // TODO(ckocagil): implement multiline. On 2013/09/06 23:47:45, msw wrote: > nit: capitalize "Implement", consider expanding the TODO: > // TODO(ckocagil): Convert multi-line view space points to text space. Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:905: // TODO(ckocagil): refactor this to traverse all line segments to support rtl. On 2013/09/06 23:47:45, msw wrote: > nit: capitalize 'Refactor' and 'RTL'; consider: > // TODO(ckocagil): Traverse individual line segments for RTL support. Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:909: while (line < lines_.size() && x > lines_[line].size.width()) { On 2013/09/06 23:47:45, msw wrote: > Optionally, you could do: > for (line = 0; line < lines_.size() && x > lines_[line].size.width(); ++line) > x -= lines_[line].size.width(); Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:932: Vector2d offset = GetLineOffset(line); On 2013/09/06 23:47:45, msw wrote: > nit: const Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:1076: // TODO(ckocagil): Add vertical offset support for scrolling multi-line text. On 2013/09/06 23:47:45, msw wrote: > nit: Address my GetLineOffset comment or remove "Vertical". Multi-line doesn't > use the display offset and thus doesn't support horizontal scrolling either. Done, removed "vertical". https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:115: // This structure holds the logical and visual position information for a line On 2013/09/06 23:47:45, msw wrote: > nit: remove this second sentence of the comment; it's superfluous. Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:120: // X coordinates of this line segment in text coordinate space. On 2013/09/06 23:47:45, msw wrote: > nit: "text space" Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:130: // Holds a list of line segments and metrics for a line. On 2013/09/06 23:47:45, msw wrote: > nit: consider: // A line of layout text, comprised of a line segment list and > some metrics. Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:137: // Total size of this line. Width is the sum of |segment| widths, height is On 2013/09/06 23:47:45, msw wrote: > nit: consider: // A line size is the sum of segment widths and the maximum of > segment heights. Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:141: // Sum of the |height| fields of preceding lines. On 2013/09/06 23:47:45, msw wrote: > nit: |height| doesn't exist anymore; use "Sum of the preceding lines' heights." Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:144: // Maximum baseline of all runs on this line. On 2013/09/06 23:47:45, msw wrote: > nit: s/runs/segments/ Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:223: // TODO(ckocagil): Multiline text rendering is currently only supported on On 2013/09/06 23:47:45, msw wrote: > nit: try to be consistent about using either "multiline" or "multi-line" in your > comments throughout the patch. Done, used "multiline". https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:328: // Returns the width of multi-line content. Reserves room for the cursor if On 2013/08/30 18:45:25, Alexei Svitkine wrote: > I think "of the multi-line content" is misleading here, since it also works in > single-line mode. > > Perhaps something like "Returns the width of the content (which is the wrapped > width in multi-line mode)." Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:328: // Returns the width of multi-line content. Reserves room for the cursor if On 2013/09/06 23:47:45, msw wrote: > On 2013/08/30 18:45:25, Alexei Svitkine wrote: > > I think "of the multi-line content" is misleading here, since it also works in > > single-line mode. > > > > Perhaps something like "Returns the width of the content (which is the wrapped > > width in multi-line mode)." > > Agreed, and ditto for the GetStringSize() comment. Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:459: // Returns layout text positions that are suitable for breaking lines at. On 2013/09/06 23:47:45, msw wrote: > nit: remove trailing "at". Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:466: // Returns the line offset from the origin after applying text alignment and On 2013/09/06 23:47:45, msw wrote: > nit: please fix this to "the text alignment and the display offset." Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:475: // Convert the range in text coordinates to rects in view coordinates that On 2013/09/06 23:47:45, msw wrote: > nit: s/coordinates/space/ (times two) to match the comment above. Or consider: > // Convert a text space x-coordinate range to corresponding rects in view space. Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:479: // Returns the line offset from the origin, taking into account text alignment On 2013/09/06 23:47:45, msw wrote: > nit: // Returns the line offset from the origin, accounting for text alignment > only. Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:631: // BreakList to find valid positions to break the line at. On 2013/09/06 23:47:45, msw wrote: > nit: consider: // A list of valid layout text line break positions. Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:113: else if (i != previous_segment_line) On 2013/09/06 23:47:45, msw wrote: > I realize that we shouldn't break a run into separate segments, except to split > the run onto different lines, but I don't think this check on the else is > necessary. Shouldn't the text-space start/end x-coordinates of the segments > match, regardless of the line that the consecutive segments are placed on? It doesn't seem necessary to me either. Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1697: TEST_F(RenderTextTest, Multiline_MinWidth) { On 2013/09/06 23:47:45, msw wrote: > I'm looking forward to many more tests. Can you add this one now? > Add a Multiline_SufficientWidth or similar to test trivial behavior: > SetMultiline(true) but use strings (LTR, RTL, spaces, no spaces) that fit on one > line and don't wrap with the sufficiently wide display rect width. Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1711: CheckLineIntegrity(render_text->lines_, render_text->runs_); On 2013/09/06 23:47:45, msw wrote: > Can you EXPECT_GT(render_text->lines_.size(), 1U) here? Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1717: wchar_t* text; On 2013/09/06 23:47:45, msw wrote: > nit: make all these members const. Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1718: ui::Range first_line; On 2013/09/06 23:47:45, msw wrote: > nit: |first_line_char_range| and |second_line_char_range| Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1722: { L"qwertyuiop", ui::Range(0, 8), ui::Range(8, 10) }, On 2013/09/06 23:47:45, msw wrote: > nit: indent the ui::Range entries to match, even on line 1724. Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:160: // be greater than |start_char|. On 2013/09/06 23:47:45, msw wrote: > nit: add "(to avoid constructing empty lines)." Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:165: // TODO(ckocagil): Do not reserve space for whitespace at the end of lines. On 2013/09/06 23:47:45, msw wrote: > nit: s/space/width/ Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:176: // Width from |std::max(word->first, start_char)|. On 2013/09/06 23:47:45, msw wrote: > nit: ... to the current character. Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:313: TextRun* const run = runs_[run_index]; On 2013/09/06 23:47:45, msw wrote: > nit: can this be const TextRun* const? Sorry if you already answered this. It can be; done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:320: BreakRunAtWidth(*run, *words_, next_char, max_width_ - line_x_, On 2013/09/06 23:47:45, msw wrote: > nit: send |current_char| instead of |next_char|, just for clarity. Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:401: // |text_x_| and |line_x_| are text-space and line-space x coordinates of the On 2013/09/06 23:47:45, msw wrote: > nit: remove "|text_x_| and |line_x_| are ". Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:413: // Segments to be applied by |UpdateRTLSegmentRanges()|. Contains the segments On 2013/09/06 23:47:45, msw wrote: > nit: consider "// The current RTL run segments, to be applied by > |UpdateRTLSegmentRanges()|." Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:430: common_baseline_(0), On 2013/09/06 23:47:45, msw wrote: > Remove this and just return the first line's baseline in GetBaseline(). Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:446: // |string_size_| if possible. On 2013/09/06 23:47:45, msw wrote: > nit: add some detail about why this should/can't be done. I resolved this TODO by updating LineBreaker and the unit tests. I also changed "Size string_size_" to "int string_width_". https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:626: DCHECK(bounds.empty() || bounds.back().GetMin() != range_x.GetMax()); On 2013/09/06 23:47:45, msw wrote: > nit: Use DCHECK(bounds.empty() || bounds.back().GetMax() <= range_x.GetMin()); Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:708: DCHECK(!multiline() || !lines().empty()); On 2013/09/06 23:47:45, msw wrote: > Shouldn't lines() be non-empty here, even in the single-line case? Done; the "!multiline()" was a leftover from a previous iteration. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:728: Vector2d text_offset = line_offset + Vector2d(0, line.baseline); On 2013/09/06 23:47:45, msw wrote: > nit: declare |text_offset| and |preceding_segment_widths| at line 736 Done. (assuming you meant "before the loop") https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:743: if (glyph_range.is_empty()) { On 2013/09/06 23:47:45, msw wrote: > Can you instead DCHECK(!glyph_range.is_empty())? Sorry if you already answered. Done. We don't seem to hit that DCHECK with the strings I tried. Do you think we should make it a CHECK to see whether it's hit on Canary? https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:764: SkIntToScalar(text_offset.x() + segment_x + run->offsets[k].du), On 2013/09/06 23:47:45, msw wrote: > nit: swap the order of |segment_x| and |run->offsets[k].du| Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:775: colors().GetBreak(run->range.start()); On 2013/09/06 23:47:45, msw wrote: > Should run->range.start() be changed to segment->char_range.start()? Yes. Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:776: it != colors().breaks().end() && it->first < run->range.end(); On 2013/09/06 23:47:45, msw wrote: > Should run->range.end() be changed to segment->char_range.end()? Yes. Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:783: if (colored_glyphs.is_empty()) On 2013/09/06 23:47:45, msw wrote: > With the above changes to use segment->char_range, can this be a DCHECK? Done. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.h File ui/gfx/render_text_win.h (right): https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.h... ui/gfx/render_text_win.h:127: Size multiline_string_size_; On 2013/09/06 23:47:45, msw wrote: > Disambiguate these two sizes in a comment, what does each hold? Done. https://codereview.chromium.org/16867016/diff/246001/ui/views/examples/multil... File ui/views/examples/multiline_example.cc (right): https://codereview.chromium.org/16867016/diff/246001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:35: gfx::Size size(render_text_->GetContentWidth(), On 2013/09/06 23:47:45, msw wrote: > Can you simply init |size| to render_text_->GetStringSize()? If I do that, |size| won't reserve space for the cursor. https://codereview.chromium.org/16867016/diff/246001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:43: if (w == 0) // TODO(ckocagil): Why does this happen? On 2013/09/06 23:47:45, msw wrote: > nit: Move the comment up to the previous line. Done. https://codereview.chromium.org/16867016/diff/246001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:53: ui::Range test_range(1, 21); On 2013/09/06 23:47:45, msw wrote: > Ensure new_contents is at least 12 chars. Also nit: const. I now ensure that test_range.start() and test_range.end() are not bigger than new_contents.length(). https://codereview.chromium.org/16867016/diff/246001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:54: render_text_->MoveCursorTo(gfx::SelectionModel(test_range, On 2013/09/06 23:47:45, msw wrote: > nit: can you call RenderText::ApplyColor additionally or instead? Done, called instead. https://codereview.chromium.org/16867016/diff/246001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:116: column_set->AddColumn(GridLayout::LEADING, GridLayout::CENTER, On 2013/09/06 23:47:45, msw wrote: > Instead of adding a new column to place the empty-labeled Checkbox next to the > "views::Label:" Label, simply set the Checkbox text to "views::Label:". Done. https://codereview.chromium.org/16867016/diff/246001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:153: if (label_checkbox_->checked()) On 2013/09/06 23:47:45, msw wrote: > optional nit: label_->SetText(label_checkbox_->checked() ? textfield_->text() : > string16()); Done.
This patch is looking really, really good! Sadly a few of my last comment suggestions were 81/82 chars, but oh well. Just a small number of comments. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:889: offset.Add(GetUpdatedDisplayOffset()); On 2013/09/11 14:59:49, ckocagil wrote: > On 2013/09/06 23:47:45, msw wrote: > > Shouldn't it be okay to add the display offset even in multiline? > > I think that'd give us horizontal scrolling of multiline text for free. > > Otherwise, add a TODO here like the one in UpdateCachedBoundsAndOffset. > > // TODO(ckocagil): Implement multiline text scrolling. > > I'm disabling it in multiline because I need to test it with many cases to see > whether it currently works or not, which would probably be better done in a > followup CL. I have a TODO in UpdateCachedBoundsAndOffset() about this, should I > move it here? I think an additional TODO here would help call out this part of that change. // TODO(ckocagil): Apply the display offset for multiline scrolling. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:446: // |string_size_| if possible. On 2013/09/11 14:59:49, ckocagil wrote: > On 2013/09/06 23:47:45, msw wrote: > > nit: add some detail about why this should/can't be done. > > I resolved this TODO by updating LineBreaker and the unit tests. I also changed > "Size string_size_" to "int string_width_". Awesome, but I have some comments about the next behavior. https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:743: if (glyph_range.is_empty()) { On 2013/09/11 14:59:49, ckocagil wrote: > On 2013/09/06 23:47:45, msw wrote: > > Can you instead DCHECK(!glyph_range.is_empty())? Sorry if you already > answered. > > Done. We don't seem to hit that DCHECK with the strings I tried. Do you think we > should make it a CHECK to see whether it's hit on Canary? Leave it as a DCHECK for now; we safely handle it below. https://codereview.chromium.org/16867016/diff/246001/ui/views/examples/multil... File ui/views/examples/multiline_example.cc (right): https://codereview.chromium.org/16867016/diff/246001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:35: gfx::Size size(render_text_->GetContentWidth(), On 2013/09/11 14:59:49, ckocagil wrote: > On 2013/09/06 23:47:45, msw wrote: > > Can you simply init |size| to render_text_->GetStringSize()? > > If I do that, |size| won't reserve space for the cursor. Okay https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (left): https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1147: render_text->SetText(UTF8ToUTF16(" ")); Why did you remove this? https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1183: // metrics of the smaller font. Why has this changed? I think it was correct before, so RenderText heights/baselines won't change if text changes wind up using other fonts in the list. Perhaps that's not needed if RenderText users adjust the placement to account for font list metrics, but that's not the case right now, afaik, and this change will probably break some omnibox/textfield alignment. Can you restore the previous behavior? https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1650: TEST_F(RenderTextTest, Multiline_MinWidth) { nit: Add high-level comments explaining what each test does, like: // Ensure strings wrap onto multiple lines for a small available width. https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1690: CheckLineIntegrity(render_text->lines_, render_text->runs_); You shouldn't need to call this explicitly here, it happens all the time. https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:261: // the given runs. |empty_baseline| and |empty_height| are the baseline and I think these will have to be |min_baseline| and |min_height| (via the font list's height and baseline, as you're already doing), and then clamp each Line's values to be max(line_ascent_, min_baseline_), etc. This should hopefully fix the behavior change I commented about in the unit tests. I'm glad we had those tests around to catch this! https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:431: void CheckLineIntegrity(const std::vector<Line>& lines, Move this up into the anonymous namespace, unless it's needed in internal. https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:448: if (!run->script_analysis.fRTL) { nit: remove braces. https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:744: CheckLineIntegrity(this->lines(), runs_); nit: do this on |lines| before calling set_lines(&lines). https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:849: string_width_ = 0; Clear multiline_string_size_ here, this seems like the best place.
https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/16867016/diff/246001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:889: offset.Add(GetUpdatedDisplayOffset()); On 2013/09/11 17:32:37, msw wrote: > On 2013/09/11 14:59:49, ckocagil wrote: > > On 2013/09/06 23:47:45, msw wrote: > > > Shouldn't it be okay to add the display offset even in multiline? > > > I think that'd give us horizontal scrolling of multiline text for free. > > > Otherwise, add a TODO here like the one in UpdateCachedBoundsAndOffset. > > > // TODO(ckocagil): Implement multiline text scrolling. > > > > I'm disabling it in multiline because I need to test it with many cases to see > > whether it currently works or not, which would probably be better done in a > > followup CL. I have a TODO in UpdateCachedBoundsAndOffset() about this, should > I > > move it here? > > I think an additional TODO here would help call out this part of that change. > // TODO(ckocagil): Apply the display offset for multiline scrolling. Done. https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (left): https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1147: render_text->SetText(UTF8ToUTF16(" ")); On 2013/09/11 17:32:37, msw wrote: > Why did you remove this? Added back. See my comment above LineBreaker in render_text_win.cc. https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1183: // metrics of the smaller font. On 2013/09/11 17:32:37, msw wrote: > Why has this changed? I think it was correct before, so RenderText > heights/baselines won't change if text changes wind up using other fonts in the > list. Perhaps that's not needed if RenderText users adjust the placement to > account for font list metrics, but that's not the case right now, afaik, and > this change will probably break some omnibox/textfield alignment. Can you > restore the previous behavior? Reverted the changes to existing tests. See my comment above LineBreaker in render_text_win.cc. https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1650: TEST_F(RenderTextTest, Multiline_MinWidth) { On 2013/09/11 17:32:37, msw wrote: > nit: Add high-level comments explaining what each test does, like: > // Ensure strings wrap onto multiple lines for a small available width. Done. https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1690: CheckLineIntegrity(render_text->lines_, render_text->runs_); On 2013/09/11 17:32:37, msw wrote: > You shouldn't need to call this explicitly here, it happens all the time. Done. https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:261: // the given runs. |empty_baseline| and |empty_height| are the baseline and On 2013/09/11 17:32:37, msw wrote: > I think these will have to be |min_baseline| and |min_height| (via the font > list's height and baseline, as you're already doing), and then clamp each Line's > values to be max(line_ascent_, min_baseline_), etc. This should hopefully fix > the behavior change I commented about in the unit tests. I'm glad we had those > tests around to catch this! Wouldn't this cause every line to have the same size (i.e font_list.GetHeight())? The best solution here would be to leave it like this and let the omnibox use the information from GetStringSize() and GetBaseline() to calculate offsets. But that would perhaps require changes in the Omnibox code too, and this CL is already big. So I'm forcing the Line to have font_list's height and baseline in single line mode, PTAL. https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:431: void CheckLineIntegrity(const std::vector<Line>& lines, On 2013/09/11 17:32:37, msw wrote: > Move this up into the anonymous namespace, unless it's needed in internal. This is no longer called from _unittests.cc anymore. Done. https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:448: if (!run->script_analysis.fRTL) { On 2013/09/11 17:32:37, msw wrote: > nit: remove braces. These are necessary here. DCHECK_EQ expands to a naked if block, so our else here sticks to that block instead of our block. Yes. Macros. https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:744: CheckLineIntegrity(this->lines(), runs_); On 2013/09/11 17:32:37, msw wrote: > nit: do this on |lines| before calling set_lines(&lines). Done. https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:849: string_width_ = 0; On 2013/09/11 17:32:37, msw wrote: > Clear multiline_string_size_ here, this seems like the best place. Done.
LGTM with a TODO; thanks for the amazing work so far!!! https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:261: // the given runs. |empty_baseline| and |empty_height| are the baseline and On 2013/09/11 20:31:16, ckocagil wrote: > On 2013/09/11 17:32:37, msw wrote: > > I think these will have to be |min_baseline| and |min_height| (via the font > > list's height and baseline, as you're already doing), and then clamp each > Line's > > values to be max(line_ascent_, min_baseline_), etc. This should hopefully fix > > the behavior change I commented about in the unit tests. I'm glad we had those > > tests around to catch this! > > Wouldn't this cause every line to have the same size (i.e > font_list.GetHeight())? > > The best solution here would be to leave it like this and let the omnibox use > the information from GetStringSize() and GetBaseline() to calculate offsets. But > that would perhaps require changes in the Omnibox code too, and this CL is > already big. So I'm forcing the Line to have font_list's height and baseline in > single line mode, PTAL. I'm not sure what the optimal behavior is, but it's probably desirable for every line to use the font list height/baseline as a *minimum*. I suspect this new multiline code has a feature/bug where lines dynamically change heights as the fonts used on each line changes (ex/ "ab" vs "えあ"); that's considered a bug for single-line textfields, because text can 'jump' while typing. This requires some follow-up thought/work, but what you have seems fine for now. Perhaps add a TODO(ckocagil): Determine optimal multi-line height behavior.
LGTM modulo some nits. Good work! https://codereview.chromium.org/16867016/diff/266001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/266001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:117: Nit: Add an explicit out-of-line destructor for this (in the .cc file), so that an implicit one isn't generated for every include of this header. https://codereview.chromium.org/16867016/diff/266001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:130: Line(); Nit: Add an explicit out-of-line destructor for this (in the .cc file), so that an implicit one isn't generated for every include of this header. https://codereview.chromium.org/16867016/diff/266001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/16867016/diff/266001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1683: { L"qwertyuiop", ui::Range(0, 8), ui::Range(8, 10) }, Nit: Don't align. https://codereview.chromium.org/16867016/diff/266001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1685: ui::Range(0, 7), ui::Range(7, 12) } Nit: Just indent this 4 more than the previous line (aligning with something else doesn't make sense). https://codereview.chromium.org/16867016/diff/266001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1690: render_text->SetDisplayRect(Rect(Point(), Size(50, 1000))); Nit: You can just do Rect(50, 1000). https://codereview.chromium.org/16867016/diff/266001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1712: L".", L" . ", Nit: This wrapping is off. Either move some strings up that still fit on the line above or do 1 string per line.
https://codereview.chromium.org/16867016/diff/266001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/16867016/diff/266001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1679: const ui::Range first_line_char_range; Also, you'll need to re-base this. Range has been moved under gfx (so the ui:: prefixes when using it need to be removed).
https://codereview.chromium.org/16867016/diff/266001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/16867016/diff/266001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1683: { L"qwertyuiop", ui::Range(0, 8), ui::Range(8, 10) }, On 2013/09/12 00:37:00, Alexei Svitkine wrote: > Nit: Don't align. I asked for this; it helps readability, but I'll concede if you disagree. https://codereview.chromium.org/16867016/diff/266001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1685: ui::Range(0, 7), ui::Range(7, 12) } On 2013/09/12 00:37:00, Alexei Svitkine wrote: > Nit: Just indent this 4 more than the previous line (aligning with something > else doesn't make sense). ditto to my statement above.
https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/16867016/diff/256001/ui/gfx/render_text_win.c... ui/gfx/render_text_win.cc:261: // the given runs. |empty_baseline| and |empty_height| are the baseline and On 2013/09/11 21:26:50, msw wrote: > On 2013/09/11 20:31:16, ckocagil wrote: > > On 2013/09/11 17:32:37, msw wrote: > > > I think these will have to be |min_baseline| and |min_height| (via the font > > > list's height and baseline, as you're already doing), and then clamp each > > Line's > > > values to be max(line_ascent_, min_baseline_), etc. This should hopefully > fix > > > the behavior change I commented about in the unit tests. I'm glad we had > those > > > tests around to catch this! > > > > Wouldn't this cause every line to have the same size (i.e > > font_list.GetHeight())? > > > > The best solution here would be to leave it like this and let the omnibox use > > the information from GetStringSize() and GetBaseline() to calculate offsets. > But > > that would perhaps require changes in the Omnibox code too, and this CL is > > already big. So I'm forcing the Line to have font_list's height and baseline > in > > single line mode, PTAL. > > I'm not sure what the optimal behavior is, but it's probably desirable for every > line to use the font list height/baseline as a *minimum*. I suspect this new > multiline code has a feature/bug where lines dynamically change heights as the > fonts used on each line changes (ex/ "ab" vs "えあ"); that's considered a bug for > single-line textfields, because text can 'jump' while typing. This requires some > follow-up thought/work, but what you have seems fine for now. Perhaps add a > TODO(ckocagil): Determine optimal multi-line height behavior. Done, added TODO. https://codereview.chromium.org/16867016/diff/266001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/16867016/diff/266001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:117: On 2013/09/12 00:37:00, Alexei Svitkine wrote: > Nit: Add an explicit out-of-line destructor for this (in the .cc file), so that > an implicit one isn't generated for every include of this header. Done. https://codereview.chromium.org/16867016/diff/266001/ui/gfx/render_text.h#new... ui/gfx/render_text.h:130: Line(); On 2013/09/12 00:37:00, Alexei Svitkine wrote: > Nit: Add an explicit out-of-line destructor for this (in the .cc file), so that > an implicit one isn't generated for every include of this header. Done. https://codereview.chromium.org/16867016/diff/266001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/16867016/diff/266001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1679: const ui::Range first_line_char_range; On 2013/09/12 00:38:08, Alexei Svitkine wrote: > Also, you'll need to re-base this. Range has been moved under gfx (so the ui:: > prefixes when using it need to be removed). Done. https://codereview.chromium.org/16867016/diff/266001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1690: render_text->SetDisplayRect(Rect(Point(), Size(50, 1000))); On 2013/09/12 00:37:00, Alexei Svitkine wrote: > Nit: You can just do Rect(50, 1000). Done, throughout. https://codereview.chromium.org/16867016/diff/266001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1712: L".", L" . ", On 2013/09/12 00:37:00, Alexei Svitkine wrote: > Nit: This wrapping is off. Either move some strings up that still fit on the > line above or do 1 string per line. Done.
+sky: PTAL multiline_example.(h|cc), thanks!
https://codereview.chromium.org/16867016/diff/289001/ui/views/examples/multil... File ui/views/examples/multiline_example.cc (right): https://codereview.chromium.org/16867016/diff/289001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:34: render_text_->SetMultiline(false); Why do you need to toggle multiline here? https://codereview.chromium.org/16867016/diff/289001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:46: gfx::Rect rect = render_text_->display_rect(); Why do you need the display rect here? If you need a rect shouldn't it have an infinite height and width as you have on 47? https://codereview.chromium.org/16867016/diff/289001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:53: ui::Range test_range(1, 21); Where does 1,21 come from? https://codereview.chromium.org/16867016/diff/289001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:95: example_container_ = container; Isn't this the same as ExampleBase::container_? If you need that, expose it. https://codereview.chromium.org/16867016/diff/289001/ui/views/examples/multil... File ui/views/examples/multiline_example.h (right): https://codereview.chromium.org/16867016/diff/289001/ui/views/examples/multil... ui/views/examples/multiline_example.h:43: View* example_container_; Add description for example_container_ and label_checkbox_.
https://codereview.chromium.org/16867016/diff/289001/ui/views/examples/multil... File ui/views/examples/multiline_example.cc (right): https://codereview.chromium.org/16867016/diff/289001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:34: render_text_->SetMultiline(false); On 2013/09/12 18:44:15, sky wrote: > Why do you need to toggle multiline here? The preferred size of this view is the single-line size. So we turn off multiline, get the single-line size, then turn it back on. https://codereview.chromium.org/16867016/diff/289001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:46: gfx::Rect rect = render_text_->display_rect(); On 2013/09/12 18:44:15, sky wrote: > Why do you need the display rect here? If you need a rect shouldn't it have an > infinite height and width as you have on 47? In multiline mode, RenderText wraps the lines after |display_rect().width()| pixels. Roughly, this function changes |display_rect().width()| to |w|, so that we can get the multiline text height with |GetStringSize()|. https://codereview.chromium.org/16867016/diff/289001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:53: ui::Range test_range(1, 21); On 2013/09/12 18:44:15, sky wrote: > Where does 1,21 come from? Nowhere in particular, it is just a useful value for testing. |test_range| is used to color and style some part of the text to see whether colored/styled texts are rendered correctly. (Also changed ui::Range to gfx::Range, I forgot this one here while rebasing.) https://codereview.chromium.org/16867016/diff/289001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:95: example_container_ = container; On 2013/09/12 18:44:15, sky wrote: > Isn't this the same as ExampleBase::container_? If you need that, expose it. Done. https://codereview.chromium.org/16867016/diff/289001/ui/views/examples/multil... File ui/views/examples/multiline_example.h (right): https://codereview.chromium.org/16867016/diff/289001/ui/views/examples/multil... ui/views/examples/multiline_example.h:43: View* example_container_; On 2013/09/12 18:44:15, sky wrote: > Add description for example_container_ and label_checkbox_. |example_container_| is gone. Added description for |label_checkbox_|.
https://codereview.chromium.org/16867016/diff/289001/ui/views/examples/multil... File ui/views/examples/multiline_example.cc (right): https://codereview.chromium.org/16867016/diff/289001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:34: render_text_->SetMultiline(false); On 2013/09/12 20:02:08, ckocagil wrote: > On 2013/09/12 18:44:15, sky wrote: > > Why do you need to toggle multiline here? > > The preferred size of this view is the single-line size. So we turn off > multiline, get the single-line size, then turn it back on. Add a comment to that effect. https://codereview.chromium.org/16867016/diff/289001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:53: ui::Range test_range(1, 21); On 2013/09/12 20:02:08, ckocagil wrote: > On 2013/09/12 18:44:15, sky wrote: > > Where does 1,21 come from? > > Nowhere in particular, it is just a useful value for testing. |test_range| is > used to color and style some part of the text to see whether colored/styled > texts are rendered correctly. > > (Also changed ui::Range to gfx::Range, I forgot this one here while rebasing.) Again with the comment.
https://codereview.chromium.org/16867016/diff/289001/ui/views/examples/multil... File ui/views/examples/multiline_example.cc (right): https://codereview.chromium.org/16867016/diff/289001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:34: render_text_->SetMultiline(false); On 2013/09/12 21:23:39, sky wrote: > On 2013/09/12 20:02:08, ckocagil wrote: > > On 2013/09/12 18:44:15, sky wrote: > > > Why do you need to toggle multiline here? > > > > The preferred size of this view is the single-line size. So we turn off > > multiline, get the single-line size, then turn it back on. > > Add a comment to that effect. Done. https://codereview.chromium.org/16867016/diff/289001/ui/views/examples/multil... ui/views/examples/multiline_example.cc:53: ui::Range test_range(1, 21); On 2013/09/12 21:23:39, sky wrote: > On 2013/09/12 20:02:08, ckocagil wrote: > > On 2013/09/12 18:44:15, sky wrote: > > > Where does 1,21 come from? > > > > Nowhere in particular, it is just a useful value for testing. |test_range| is > > used to color and style some part of the text to see whether colored/styled > > texts are rendered correctly. > > > > (Also changed ui::Range to gfx::Range, I forgot this one here while rebasing.) > > Again with the comment. Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckocagil@chromium.org/16867016/293001
Failed to trigger a try job on linux_chromeos HTTP Error 400: Bad Request
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckocagil@chromium.org/16867016/306001
Retried try job too often on win7_aura for step(s) views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckocagil@chromium.org/16867016/246002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckocagil@chromium.org/16867016/246002
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckocagil@chromium.org/16867016/256002
Message was sent while issue was closed.
Change committed as 223394 |
