Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
882643005:
Add multiline support to RenderTextHarfBuzz. (Closed)
DescriptionAdd multiline support to RenderTextHarfBuzz.
The implementation of LineBreaker is mostly a straightforward
porting of RenderTextWin's implementation.
BUG=248597
R=ckocagil@chromium.org, msw@chromium.org
TEST=manually, customized ash_shell
Committed: https://crrev.com/2772f49714e8571e8609aa6d9d1178ce8f2ed9c7
Cr-Commit-Position: refs/heads/master@{#315010}
Patch Set 1 #Patch Set 2 : cleanup #Patch Set 3 : typo fix #
Total comments: 26
Patch Set 4 : tests #Patch Set 5 : addressed review comments #Patch Set 6 : newline #
Total comments: 20
Patch Set 7 : addressed review comments #
Total comments: 17
Patch Set 8 : fix #Patch Set 9 : fix #
Total comments: 4
Patch Set 10 : comments addressed #Patch Set 11 : win fix #Patch Set 12 : asan fix #
Messages
Total messages: 33 (9 generated)
oshima@chromium.org changed reviewers: + oshima@chromium.org
drive-by: can you re-enable tests for multiline?
I hope that Cem can take a look; and this definitely requires re-enabling the multi-line tests. You might also want to check out the Views Examples for multi-line text. https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:484: : max_width_((max_width == 0) ? kuint32max : max_width), This constant is deprecated; use std::numeric_limits<int>::max() instead. https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:503: // Uniscribe always puts newline characters in their own runs. This behavior should be re-checked with ICU/HarfBuzz itemization, and this code or comment should be updated. https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:563: // TextRun::logical_clusters might help. nit: TextRun::logical_clusters was RenderTextWin specific, this can probably be re-written generically as "// TODO(ckocagil): Check clusters to avoid breaking ligatures and diacritics." https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:578: SkScalar width_scalar = 0; Could |width| simply be an SkScalar? https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:686: std::max(line->size.height(), metrics.fDescent - metrics.fAscent)); I think you'll need to retain the logic for tracking the maximum descents and ascents across segments. That will do the proper height calculation and allow baseline-centering of text across segments; simply taking the maximum height of the segments in a line isn't sufficient. https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1188: // TODO(vadimt): Remove ScopedTracker below once crbug.com/431326 is fixed. nit: You might be able to collapse EnsureLayout2 and EnsureLayout3. https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1215: Vector2d origin = GetLineOffset(0) + Vector2d(0, lines()[0].baseline); nit: you might be able to actually use GetLineOffset(<line_number>) now. (otherwise, maybe it's not worth keeping that functionality) https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1217: SkScalar preceding_run_widths = 0; nit: rename this |preceding_segment_widths| https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1224: scoped_ptr<SkPoint[]> positions(new SkPoint[segment.char_range.length()]); The length of |positions| should correspond to the number of glyphs, not the number of characters (it was previously |run.glyph_count|). I don't think |segment.char_range.length()| is correct, it should be the number of glyphs corresponding to that char range; try CharRangeToGlyphRange. https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1228: positions[j] = run.positions[segment_start + j]; Yeah, TextRunHarfBuzz::positions is sized to the number of glyphs in the run, char-based indices may exceed the array length, besides being incorrect. https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1249: renderer.DrawPosText(&positions[colored_glyphs.start() - segment_start], The |segment_start| here should also be in glyph index space. https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1252: int start_x = SkScalarRoundToInt(positions[colored_glyphs.start()].x()); Should this use the positions[colored_glyphs.start() - segment_start], similar to above and the old RenderTextWin behavior? https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1257: : positions[colored_glyphs.end()].x()); Should this use the positions[colored_glyphs.end() - segment_start], similar to above and the old RenderTextWin behavior?
https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:484: : max_width_((max_width == 0) ? kuint32max : max_width), On 2015/02/04 01:44:18, msw wrote: > This constant is deprecated; use std::numeric_limits<int>::max() instead. Done. https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:503: // Uniscribe always puts newline characters in their own runs. On 2015/02/04 01:44:18, msw wrote: > This behavior should be re-checked with ICU/HarfBuzz itemization, and this code > or comment should be updated. Modified itemizing runs a bit so that runs are split at newline character, then the line breaker code (and this comment) is gone. https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:563: // TextRun::logical_clusters might help. On 2015/02/04 01:44:18, msw wrote: > nit: TextRun::logical_clusters was RenderTextWin specific, this can probably be > re-written generically as "// TODO(ckocagil): Check clusters to avoid breaking > ligatures and diacritics." Done. https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:578: SkScalar width_scalar = 0; On 2015/02/04 01:44:18, msw wrote: > Could |width| simply be an SkScalar? Done. https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:686: std::max(line->size.height(), metrics.fDescent - metrics.fAscent)); On 2015/02/04 01:44:18, msw wrote: > I think you'll need to retain the logic for tracking the maximum descents and > ascents across segments. That will do the proper height calculation and allow > baseline-centering of text across segments; simply taking the maximum height of > the segments in a line isn't sufficient. Done. https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1188: // TODO(vadimt): Remove ScopedTracker below once crbug.com/431326 is fixed. On 2015/02/04 01:44:18, msw wrote: > nit: You might be able to collapse EnsureLayout2 and EnsureLayout3. To keep the consistency with the previous code, I've moved line_breaker construction between 2 and 3. https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1215: Vector2d origin = GetLineOffset(0) + Vector2d(0, lines()[0].baseline); On 2015/02/04 01:44:18, msw wrote: > nit: you might be able to actually use GetLineOffset(<line_number>) now. > (otherwise, maybe it's not worth keeping that functionality) Done. https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1217: SkScalar preceding_run_widths = 0; On 2015/02/04 01:44:18, msw wrote: > nit: rename this |preceding_segment_widths| Done. https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1224: scoped_ptr<SkPoint[]> positions(new SkPoint[segment.char_range.length()]); On 2015/02/04 01:44:18, msw wrote: > The length of |positions| should correspond to the number of glyphs, not the > number of characters (it was previously |run.glyph_count|). I don't think > |segment.char_range.length()| is correct, it should be the number of glyphs > corresponding to that char range; try CharRangeToGlyphRange. Done. https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1228: positions[j] = run.positions[segment_start + j]; On 2015/02/04 01:44:18, msw wrote: > Yeah, TextRunHarfBuzz::positions is sized to the number of glyphs in the run, > char-based indices may exceed the array length, besides being incorrect. Done. https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1249: renderer.DrawPosText(&positions[colored_glyphs.start() - segment_start], On 2015/02/04 01:44:18, msw wrote: > The |segment_start| here should also be in glyph index space. Done. https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1252: int start_x = SkScalarRoundToInt(positions[colored_glyphs.start()].x()); On 2015/02/04 01:44:18, msw wrote: > Should this use the positions[colored_glyphs.start() - segment_start], similar > to above and the old RenderTextWin behavior? Done. https://codereview.chromium.org/882643005/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1257: : positions[colored_glyphs.end()].x()); On 2015/02/04 01:44:18, msw wrote: > Should this use the positions[colored_glyphs.end() - segment_start], similar to > above and the old RenderTextWin behavior? Done.
Looking pretty good, still hoping Cem will take a look, since he authored the original multi-line RenderTextWin work that this follows. https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:22: #include "ui/gfx/geometry/size_conversions.h" This is no longer needed if you no longer use gfx::ToCeiledSize(). https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:44: // The fixed width of charcters. This must not be set on the real environment. nit: "glyphs" not the misspelled "charcters". also, consider instead: "This should only be set in test environments." https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:514: if (multiline_ && U16_IS_SINGLE(first_char) && first_char == '\n') nit q: Do we actually need to check U16_IS_SINGLE(first_char) if we're explicitly testing for equality with '\n'? AFAICT, it's not necessary. https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1230: positions[j].offset(SkIntToScalar(origin.x()) - segment_base_x + optionaly nit: pre-calculate these constant offset values outside the loop. https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:125: // Specify the width of a character for test. The width of characters is very nit: use "glyph" and "glyphs", not "character" and "characters". https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:128: static void SetGlyphWidthForTest(int test_width); Make this a non-static value and setter on rthb instances; that fits your test requirements and avoids the need to cleanup with a SetGlyphWidthForTest(0) call. https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:1941: TEST_F(RenderTextTest, Multiline_Newline) { nit: add a test [case] of consecutive newline characters "a\n\nb". https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:1950: { L"ab\n", Range(0, 2), Range::InvalidRange() }, Why doesn't the second line have char Range(2, 3)? (ditto below) Don't we want segments with the newline characters? I could be persuaded otherwise... https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:1985: TEST_F(RenderTextTest, Multiline_NewlineWithoutMultilineFlag) { nit: I think you can drop the "Multiline_" test fixture prefix here.
lgtm with comment. Thanks for working on this! https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:682: min_ascent_ = std::min(min_ascent_, metrics.fAscent); Can you change |min_ascent_| to |max_ascent_|, make it a positive value, and use std::max here?
New patchsets have been uploaded after l-g-t-m from ckocagil@chromium.org
https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:22: #include "ui/gfx/geometry/size_conversions.h" On 2015/02/05 01:09:15, msw wrote: > This is no longer needed if you no longer use gfx::ToCeiledSize(). Done. https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:44: // The fixed width of charcters. This must not be set on the real environment. On 2015/02/05 01:09:15, msw wrote: > nit: "glyphs" not the misspelled "charcters". > also, consider instead: "This should only be set in test environments." Done. https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:514: if (multiline_ && U16_IS_SINGLE(first_char) && first_char == '\n') On 2015/02/05 01:09:15, msw wrote: > nit q: Do we actually need to check U16_IS_SINGLE(first_char) if we're > explicitly testing for equality with '\n'? AFAICT, it's not necessary. removed. https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:682: min_ascent_ = std::min(min_ascent_, metrics.fAscent); On 2015/02/05 04:21:33, ckocagil wrote: > Can you change |min_ascent_| to |max_ascent_|, make it a positive value, and use > std::max here? Done. https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1230: positions[j].offset(SkIntToScalar(origin.x()) - segment_base_x + On 2015/02/05 01:09:15, msw wrote: > optionaly nit: pre-calculate these constant offset values outside the loop. Done. https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:125: // Specify the width of a character for test. The width of characters is very On 2015/02/05 01:09:15, msw wrote: > nit: use "glyph" and "glyphs", not "character" and "characters". Done. https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:128: static void SetGlyphWidthForTest(int test_width); On 2015/02/05 01:09:15, msw wrote: > Make this a non-static value and setter on rthb instances; that fits your test > requirements and avoids the need to cleanup with a SetGlyphWidthForTest(0) call. Done. https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:1941: TEST_F(RenderTextTest, Multiline_Newline) { On 2015/02/05 01:09:16, msw wrote: > nit: add a test [case] of consecutive newline characters "a\n\nb". Done. https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:1950: { L"ab\n", Range(0, 2), Range::InvalidRange() }, On 2015/02/05 01:09:16, msw wrote: > Why doesn't the second line have char Range(2, 3)? (ditto below) Don't we want > segments with the newline characters? I could be persuaded otherwise... The invalid range means the empty line, as you see below for "\n" case. But I changed my mind, I think Range() (an empty range) represents the empty-ness more. https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:1985: TEST_F(RenderTextTest, Multiline_NewlineWithoutMultilineFlag) { On 2015/02/05 01:09:16, msw wrote: > nit: I think you can drop the "Multiline_" test fixture prefix here. Done.
lgtm with nits https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:512: else if (multiline_ && (line_x_ + run->width > max_width_)) nit: && (line_x_ + run_with) > max_width https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:656: DCHECK_EQ(width, 0); nit: 0, width https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1212: for (size_t i = 0; i < lines().size(); ++i) { range based for loop? https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:183: uint8 glyph_width_for_test_; instead of adding this to every instance, can you just use file scoped variable?
https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1212: for (size_t i = 0; i < lines().size(); ++i) { On 2015/02/05 21:29:40, oshima wrote: > range based for loop? That makes it tougher to use GetLineOffset() with a line index below. https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:183: uint8 glyph_width_for_test_; On 2015/02/05 21:29:40, oshima wrote: > instead of adding this to every instance, can you just use file scoped variable? That goes against my recommendation at: https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_harf... I don't think multiple RenderText instances should be affected by this test setting, nor should you have to unset the test value for subsequent tests, and perhaps sharing a file-static value could cause flaky failures of tests being run in parallel?
https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:1960: #define COMPARE_RANGE(r1, segments, message) \ Please write this as a function instead of a preprocessor directive.
New patchsets have been uploaded after l-g-t-m from oshima@chromium.org
https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:512: else if (multiline_ && (line_x_ + run->width > max_width_)) On 2015/02/05 21:29:40, oshima wrote: > nit: > > && (line_x_ + run_with) > max_width Done. https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:656: DCHECK_EQ(width, 0); On 2015/02/05 21:29:40, oshima wrote: > nit: 0, width Done. https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1212: for (size_t i = 0; i < lines().size(); ++i) { On 2015/02/05 21:50:58, msw wrote: > On 2015/02/05 21:29:40, oshima wrote: > > range based for loop? > > That makes it tougher to use GetLineOffset() with a line index below. Yeah, that's why. https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:183: uint8 glyph_width_for_test_; On 2015/02/05 21:50:58, msw wrote: > On 2015/02/05 21:29:40, oshima wrote: > > instead of adding this to every instance, can you just use file scoped > variable? > > That goes against my recommendation at: > https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_harf... > I don't think multiple RenderText instances should be affected by this test > setting, nor should you have to unset the test value for subsequent tests, and > perhaps sharing a file-static value could cause flaky failures of tests being > run in parallel? One thing actually I was a bit worried about is the memory consumption -- it would be great if we can eliminate an extra field just for testing. I don't think single uint8 would make big difference though.
https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1212: for (size_t i = 0; i < lines().size(); ++i) { On 2015/02/05 22:21:48, Jun Mukai wrote: > On 2015/02/05 21:50:58, msw wrote: > > On 2015/02/05 21:29:40, oshima wrote: > > > range based for loop? > > > > That makes it tougher to use GetLineOffset() with a line index below. > > Yeah, that's why. which seems to be getting the same Line object internally. I don't have strong opinion and fine as is though. https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:183: uint8 glyph_width_for_test_; On 2015/02/05 22:21:48, Jun Mukai wrote: > On 2015/02/05 21:50:58, msw wrote: > > On 2015/02/05 21:29:40, oshima wrote: > > > instead of adding this to every instance, can you just use file scoped > > variable? > > > > That goes against my recommendation at: > > > https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_harf... > > I don't think multiple RenderText instances should be affected by this test > > setting, nor should you have to unset the test value for subsequent tests, and > > perhaps sharing a file-static value could cause flaky failures of tests being > > run in parallel? > > One thing actually I was a bit worried about is the memory consumption -- it > would be great if we can eliminate an extra field just for testing. I don't > think single uint8 would make big difference though. How about defining virtual function that gets the width of the glyph. you can define a class for test that return a fixed value, instead of adding test only member variable. It won't add extra memory except one more vtable.
https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1212: for (size_t i = 0; i < lines().size(); ++i) { On 2015/02/05 22:42:44, oshima wrote: > On 2015/02/05 22:21:48, Jun Mukai wrote: > > On 2015/02/05 21:50:58, msw wrote: > > > On 2015/02/05 21:29:40, oshima wrote: > > > > range based for loop? > > > > > > That makes it tougher to use GetLineOffset() with a line index below. > > > > Yeah, that's why. > > which seems to be getting the same Line object internally. I don't have strong > opinion and fine as is though. RenderText::GetLineOffset() is also used in some other places including RenderTextMac, so I want to keep it as is at least for now. https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:183: uint8 glyph_width_for_test_; On 2015/02/05 22:42:44, oshima wrote: > On 2015/02/05 22:21:48, Jun Mukai wrote: > > On 2015/02/05 21:50:58, msw wrote: > > > On 2015/02/05 21:29:40, oshima wrote: > > > > instead of adding this to every instance, can you just use file scoped > > > variable? > > > > > > That goes against my recommendation at: > > > > > > https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_harf... > > > I don't think multiple RenderText instances should be affected by this test > > > setting, nor should you have to unset the test value for subsequent tests, > and > > > perhaps sharing a file-static value could cause flaky failures of tests > being > > > run in parallel? > > > > One thing actually I was a bit worried about is the memory consumption -- it > > would be great if we can eliminate an extra field just for testing. I don't > > think single uint8 would make big difference though. > > How about defining virtual function that gets the width of the glyph. you can > define a class for test that return a fixed value, instead of adding test only > member variable. It won't add extra memory except one more vtable. Rather than that, I would think that exposing the HarfBuzzLineBreaker class is the right thing to do, so that it doesn't have to care about how harfbuzz computes the width of glyphs, we can focus on linebreaker logic on the certain data of text runs. Actually this is already noted as a TODO(ckocagil) in the cc file. I am not thinking to do it here, because that would require rewrites of several test cases. https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:1960: #define COMPARE_RANGE(r1, segments, message) \ On 2015/02/05 21:59:27, msw wrote: > Please write this as a function instead of a preprocessor directive. Done.
https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/882643005/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.h:183: uint8 glyph_width_for_test_; On 2015/02/05 23:28:00, Jun Mukai wrote: > On 2015/02/05 22:42:44, oshima wrote: > > On 2015/02/05 22:21:48, Jun Mukai wrote: > > > On 2015/02/05 21:50:58, msw wrote: > > > > On 2015/02/05 21:29:40, oshima wrote: > > > > > instead of adding this to every instance, can you just use file scoped > > > > variable? > > > > > > > > That goes against my recommendation at: > > > > > > > > > > https://codereview.chromium.org/882643005/diff/100001/ui/gfx/render_text_harf... > > > > I don't think multiple RenderText instances should be affected by this > test > > > > setting, nor should you have to unset the test value for subsequent tests, > > and > > > > perhaps sharing a file-static value could cause flaky failures of tests > > being > > > > run in parallel? > > > > > > One thing actually I was a bit worried about is the memory consumption -- it > > > would be great if we can eliminate an extra field just for testing. I don't > > > think single uint8 would make big difference though. > > > > How about defining virtual function that gets the width of the glyph. you can > > define a class for test that return a fixed value, instead of adding test only > > member variable. It won't add extra memory except one more vtable. > > Rather than that, I would think that exposing the HarfBuzzLineBreaker class is > the right thing to do, so that it doesn't have to care about how harfbuzz > computes the width of glyphs, we can focus on linebreaker logic on the certain > data of text runs. > > Actually this is already noted as a TODO(ckocagil) in the cc file. I am not > thinking to do it here, because that would require rewrites of several test > cases. I'm fine as long as we can eliminate test only member variable. separate cl is fine.
lgtm with an optional test refactoring. https://codereview.chromium.org/882643005/diff/160001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/882643005/diff/160001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:89: EXPECT_EQ(expected_range.length() == 0 ? 0ul : 1ul, segments.size()) nit: expected_range.is_empty(), ditto below. https://codereview.chromium.org/882643005/diff/160001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:1980: VerifyLineSegments(kTestStrings[i].first_line_char_range, optional nit: Make kTestStrings contain an array of 3 ranges, nix |lines_count| and do something like this: for (size_t j = 0; j < 3; ++j) { SCOPED_TRACE(base::StringPrintf("Line %" PRIuS "", j)); if (kTestStrings[i].char_range[j].IsValid()) { // You can optionally inline VerifyLineSegments here. VerifyLineSegments(kTestStrings[i].char_range[j], render_text.lines_[2].segments); } }
New patchsets have been uploaded after l-g-t-m from msw@chromium.org
https://codereview.chromium.org/882643005/diff/160001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/882643005/diff/160001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:89: EXPECT_EQ(expected_range.length() == 0 ? 0ul : 1ul, segments.size()) On 2015/02/05 23:49:46, msw wrote: > nit: expected_range.is_empty(), ditto below. Done. https://codereview.chromium.org/882643005/diff/160001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:1980: VerifyLineSegments(kTestStrings[i].first_line_char_range, On 2015/02/05 23:49:47, msw wrote: > optional nit: Make kTestStrings contain an array of 3 ranges, nix |lines_count| > and do something like this: > for (size_t j = 0; j < 3; ++j) { > SCOPED_TRACE(base::StringPrintf("Line %" PRIuS "", j)); > if (kTestStrings[i].char_range[j].IsValid()) { > // You can optionally inline VerifyLineSegments here. > VerifyLineSegments(kTestStrings[i].char_range[j], > render_text.lines_[2].segments); > } > } Done.
The CQ bit was checked by mukai@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/882643005/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by mukai@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/882643005/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mukai@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/882643005/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/2772f49714e8571e8609aa6d9d1178ce8f2ed9c7 Cr-Commit-Position: refs/heads/master@{#315010} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
