Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
351963002:
RenderTextHarfBuzz: Allow mid-glyph cursors in multi-grapheme clusters (Closed)
|
Created:
6 years, 6 months ago by ckocagil Modified:
6 years, 4 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, jshin+watch_chromium.org, behdad Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionRenderTextHarfBuzz: Allow mid-glyph cursors in multi-grapheme clusters
Use the Unicode grapheme boundary algorithm via ICU BreakIterator to count graphemes in clusters
BUG=383265
TBR=jshin
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286569
Patch Set 1 #Patch Set 2 : reuse breakiterator #
Total comments: 43
Patch Set 3 : rebased #Patch Set 4 : rebased #Patch Set 5 : comments addressed #
Total comments: 26
Patch Set 6 : rebase #Patch Set 7 : comments addressed 2 #Patch Set 8 : remove todo #Patch Set 9 : compile fix #
Messages
Total messages: 18 (0 generated)
I've suggested some ideas to reorganize the code that seem reasonable to me, but let me know if you think they'd make things worse. https://codereview.chromium.org/351963002/diff/20001/base/i18n/break_iterator.cc File base/i18n/break_iterator.cc (right): https://codereview.chromium.org/351963002/diff/20001/base/i18n/break_iterator... base/i18n/break_iterator.cc:167: if (break_type_ != BREAK_CHARACTER) q: Wouldn't any other break type work, except perhaps RULE_BASED? (ex. wouldn't any line/word break also be a character break?) https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.cc (left): https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:710: // Disallow indices amid multi-character graphemes by checking glyph bounds. Please verify that these examples and Issue 327903 haven't regressed. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:414: DCHECK(element != elements_begin); nit: can you DCHECK_NE? https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:416: --element; nit: maybe just make this a one-liner "while(--element != ...);"? https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:417: } while (element != elements_begin && *element == element[-1]); nit: Checking for |element[-1]| is a bit odd, can you use *(element - 1)? https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:419: glyphs->set_start(reversed ? nit: move "reversed ?" to the next line. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:449: void TextRunHarfBuzz::GetClusterAt(size_t pos, Hmm, It might make more sense to just offer GetCharsAt and GetGlyphsAt. (you could avoid temp Ranges, and might be able to inline the impls) https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:499: Range TextRunHarfBuzz::GetGraphemeBounds(const base::string16& text, Perhaps GetGraphemeBounds could also be a RenderText method. (avoids the need to pass in the text, or an iterator) https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:510: if (chars.length() != 1) { This block needs a comment about the bounds treatment of multi-char graphemes. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:515: for (size_t i = chars.start(); i < chars.end(); ++i) { Should we instead calculate and store all the grapheme bounds in the run, instead of doing this extra logic each time the bounds are requested? https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:525: if (total <= 1) { Could total ever really be 0? (DCHECK against that if not). If we are getting the range of chars for the whole grapheme, and iterating over all of them above, at least the first char should be a boundary, right? https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:526: return Range(preceding_run_widths + cluster_begin_x, Optionally reverse the conditional to execute the code below, and otherwise fall through to the return statement at the bottom of the function. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:529: DCHECK_GE(before, 0); Wouldn't this be false if the index was the first of a multi-char grapheme? https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:552: base::i18n::BreakIterator* TextRunHarfBuzz::GetGraphemeIterator( It seems like the RenderText should have an iterator, not each run. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:790: base::i18n::BreakIterator* grapheme_iterator = nit: Name this |iter| for a one-liner and below "return iter && iter->Is...". https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.h:36: // Returns bounds of the grapheme at |text_index|. Handles multi-grapheme nit: s/bounds of the grapheme/the grapheme bounds/ for a one-liner. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1927: TEST_F(RenderTextTest, HarfBuzz_Clusters) { nit: can you add high-level comments for each of these tests? https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1988: { // From string "A B C D" to glyphs "a bcd". Are there still cases where we'd expect a cursor not to be placed between characters of a multi-char grapheme, like in the old comment "\x0915\x093f - (ki) - one of many Devanagari biconsonantal conjuncts."? If so, can we test something like that?
https://codereview.chromium.org/351963002/diff/20001/base/i18n/break_iterator.cc File base/i18n/break_iterator.cc (right): https://codereview.chromium.org/351963002/diff/20001/base/i18n/break_iterator... base/i18n/break_iterator.cc:167: if (break_type_ != BREAK_CHARACTER) On 2014/06/29 22:29:09, msw wrote: > q: Wouldn't any other break type work, except perhaps RULE_BASED? > (ex. wouldn't any line/word break also be a character break?) Makes sense, but I'm not sure. We can ask the BreakIterator owner after your review. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.cc (left): https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:710: // Disallow indices amid multi-character graphemes by checking glyph bounds. On 2014/06/29 22:29:09, msw wrote: > Please verify that these examples and Issue 327903 haven't regressed. Done. Added a test too. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:414: DCHECK(element != elements_begin); On 2014/06/29 22:29:09, msw wrote: > nit: can you DCHECK_NE? It doesn't compile with DCHECK_NE due to some template error. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:416: --element; On 2014/06/29 22:29:09, msw wrote: > nit: maybe just make this a one-liner "while(--element != ...);"? Done. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:417: } while (element != elements_begin && *element == element[-1]); On 2014/06/29 22:29:09, msw wrote: > nit: Checking for |element[-1]| is a bit odd, can you use *(element - 1)? Done. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:419: glyphs->set_start(reversed ? On 2014/06/29 22:29:10, msw wrote: > nit: move "reversed ?" to the next line. Done. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:449: void TextRunHarfBuzz::GetClusterAt(size_t pos, On 2014/06/29 22:29:09, msw wrote: > Hmm, It might make more sense to just offer GetCharsAt and GetGlyphsAt. > (you could avoid temp Ranges, and might be able to inline the impls) But in the cases where we need both, we'd have to make two calls and do twice the computation. Maybe I should make the pointers non-optional to prevent temps. I changed the temp here a bit, please take another look. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:499: Range TextRunHarfBuzz::GetGraphemeBounds(const base::string16& text, On 2014/06/29 22:29:09, msw wrote: > Perhaps GetGraphemeBounds could also be a RenderText method. > (avoids the need to pass in the text, or an iterator) I actually want to move more logic to TextRunHarfBuzz and eventually make it a proper class, and moving this function to RTHB would be against that direction. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:510: if (chars.length() != 1) { On 2014/06/29 22:29:09, msw wrote: > This block needs a comment about the bounds treatment of multi-char graphemes. Done. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:515: for (size_t i = chars.start(); i < chars.end(); ++i) { On 2014/06/29 22:29:09, msw wrote: > Should we instead calculate and store all the grapheme bounds in the run, > instead of doing this extra logic each time the bounds are requested? It seems like ICU is caching bounds internally. Even if I'm wrong and it doesn't, I would still say this is okay and prefer not doing any optimization without profiling. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:525: if (total <= 1) { On 2014/06/29 22:29:10, msw wrote: > Could total ever really be 0? (DCHECK against that if not). If we are getting > the range of chars for the whole grapheme, and iterating over all of them above, > at least the first char should be a boundary, right? You're right, I added a DCHECK. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:526: return Range(preceding_run_widths + cluster_begin_x, On 2014/06/29 22:29:09, msw wrote: > Optionally reverse the conditional to execute the code below, and otherwise fall > through to the return statement at the bottom of the function. Done. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:529: DCHECK_GE(before, 0); On 2014/06/29 22:29:09, msw wrote: > Wouldn't this be false if the index was the first of a multi-char grapheme? I don't understand the question. Perhaps you read this as DCHECK_GT? https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:552: base::i18n::BreakIterator* TextRunHarfBuzz::GetGraphemeIterator( On 2014/06/29 22:29:09, msw wrote: > It seems like the RenderText should have an iterator, not each run. Done. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:790: base::i18n::BreakIterator* grapheme_iterator = On 2014/06/29 22:29:09, msw wrote: > nit: Name this |iter| for a one-liner and below "return iter && iter->Is...". Done. (I made it "return !iter || iter->...") since we return true when iter is null. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.h:36: // Returns bounds of the grapheme at |text_index|. Handles multi-grapheme On 2014/06/29 22:29:10, msw wrote: > nit: s/bounds of the grapheme/the grapheme bounds/ for a one-liner. Done. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1927: TEST_F(RenderTextTest, HarfBuzz_Clusters) { On 2014/06/29 22:29:10, msw wrote: > nit: can you add high-level comments for each of these tests? Done. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:1988: { // From string "A B C D" to glyphs "a bcd". On 2014/06/29 22:29:10, msw wrote: > Are there still cases where we'd expect a cursor not to be placed between > characters of a multi-char grapheme, like in the old comment "\x0915\x093f - > (ki) - one of many Devanagari biconsonantal conjuncts."? If so, can we test > something like that? Done, see HarfBuzz_SubglyphGraphemeCases.
Looking pretty good, mostly simple comments remain. +jshin for BreakIterator. https://codereview.chromium.org/351963002/diff/20001/base/i18n/break_iterator.cc File base/i18n/break_iterator.cc (right): https://codereview.chromium.org/351963002/diff/20001/base/i18n/break_iterator... base/i18n/break_iterator.cc:167: if (break_type_ != BREAK_CHARACTER) On 2014/07/24 21:46:55, ckocagil wrote: > On 2014/06/29 22:29:09, msw wrote: > > q: Wouldn't any other break type work, except perhaps RULE_BASED? > > (ex. wouldn't any line/word break also be a character break?) > > Makes sense, but I'm not sure. We can ask the BreakIterator owner after your > review. +jshin for thoughts here and general BreakIterator review. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:414: DCHECK(element != elements_begin); On 2014/07/24 21:46:56, ckocagil wrote: > On 2014/06/29 22:29:09, msw wrote: > > nit: can you DCHECK_NE? > > It doesn't compile with DCHECK_NE due to some template error. Acknowledged. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:449: void TextRunHarfBuzz::GetClusterAt(size_t pos, On 2014/07/24 21:46:55, ckocagil wrote: > On 2014/06/29 22:29:09, msw wrote: > > Hmm, It might make more sense to just offer GetCharsAt and GetGlyphsAt. > > (you could avoid temp Ranges, and might be able to inline the impls) > > But in the cases where we need both, we'd have to make two calls and do twice > the computation. Maybe I should make the pointers non-optional to prevent temps. > > I changed the temp here a bit, please take another look. That's fair; it's a careful balance between perf and simplicity sometimes. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:499: Range TextRunHarfBuzz::GetGraphemeBounds(const base::string16& text, On 2014/07/24 21:46:56, ckocagil wrote: > On 2014/06/29 22:29:09, msw wrote: > > Perhaps GetGraphemeBounds could also be a RenderText method. > > (avoids the need to pass in the text, or an iterator) > > I actually want to move more logic to TextRunHarfBuzz and eventually make it a > proper class, and moving this function to RTHB would be against that direction. Acknowledged. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:515: for (size_t i = chars.start(); i < chars.end(); ++i) { On 2014/07/24 21:46:56, ckocagil wrote: > On 2014/06/29 22:29:09, msw wrote: > > Should we instead calculate and store all the grapheme bounds in the run, > > instead of doing this extra logic each time the bounds are requested? > > It seems like ICU is caching bounds internally. Even if I'm wrong and it > doesn't, I would still say this is okay and prefer not doing any optimization > without profiling. Acknowledged. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:529: DCHECK_GE(before, 0); On 2014/07/24 21:46:56, ckocagil wrote: > On 2014/06/29 22:29:09, msw wrote: > > Wouldn't this be false if the index was the first of a multi-char grapheme? > > I don't understand the question. Perhaps you read this as DCHECK_GT? Yeah, I think you're right, ignore that question. https://codereview.chromium.org/351963002/diff/20001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:790: base::i18n::BreakIterator* grapheme_iterator = On 2014/07/24 21:46:56, ckocagil wrote: > On 2014/06/29 22:29:09, msw wrote: > > nit: Name this |iter| for a one-liner and below "return iter && iter->Is...". > > Done. (I made it "return !iter || iter->...") since we return true when iter is > null. Acknowledged. https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:442: return grapheme_iterator_.get(); nit: fix indent https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:719: scoped_ptr<base::i18n::BreakIterator> grapheme_iterator_; Move this to RenderTextHarfBuzz if it's only used in that impl for now. You should be able to initalize it on ItemizeText, right? https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:404: void GetClusterAtImpl(size_t pos, nit: Add a simple comment. https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:453: Range temp_range; optional nit: The only NULL being sent in is |chars| from CharRangeToGlyphRange. Just make that send in an otherwise unused non-NULL Range, then you can remove this |temp_range| behavior (and maybe DCHECK for |chars| and |glyphs|). https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:459: if (is_rtl) { optional nit: instead do: Iterator begin = glyph_to_char.begin(); Iterator end = glyph_to_char.begin(); if (rtl) { begin = glyph_to_char.rbegin(); end = glyph_to_char.rend(); } GetClusterAtImpl(pos, range, begin, end, is_rtl, chars, glyphs); https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:491: Range TextRunHarfBuzz::GetGraphemeBounds(const base::string16& text, Remove |text|, it's no longer used here. https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:506: // cluster, we simple divide the cluster width by the number of graphemes. nit: s/simple/simply https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:507: if (chars.length() != 1 && grapheme_iterator) { nit: explicitly check |chars.length() > 1|. https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:706: // TODO(msw): The bounds should probably not always be leading the range ends. nit: I think you're resolving this TODO here by getting the outer grapheme bounds of the range ends; if you concur, you can remove the TODO. https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:719: Range range_x(leftmost_character_x.start(), rightmost_character_x.end()); nit: It looks like the revised logic should always yield a non-reversed x-coordinate range; if you concur, please DCHECK(!range_x.is_reversed()) and remove the line 723 operation to fix reversed ranges. https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2116: { // From string "A B C D" to glyphs "dcb a". nit: maybe add RTL case "dc ba" for good measure? https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2133: if (!iter->Init()) Make this ASSERT_TRUE(iter-Init());
Hopefully this is all done. Should I TBR to jshin or another owner? I'd like to land this soon. https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:442: return grapheme_iterator_.get(); On 2014/07/25 00:37:00, msw wrote: > nit: fix indent Done. https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:719: scoped_ptr<base::i18n::BreakIterator> grapheme_iterator_; On 2014/07/25 00:37:00, msw wrote: > Move this to RenderTextHarfBuzz if it's only used in that impl for now. You > should be able to initalize it on ItemizeText, right? Done. https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:404: void GetClusterAtImpl(size_t pos, On 2014/07/25 00:37:01, msw wrote: > nit: Add a simple comment. Done. https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:453: Range temp_range; On 2014/07/25 00:37:01, msw wrote: > optional nit: The only NULL being sent in is |chars| from CharRangeToGlyphRange. > Just make that send in an otherwise unused non-NULL Range, then you can remove > this |temp_range| behavior (and maybe DCHECK for |chars| and |glyphs|). Done. https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:459: if (is_rtl) { On 2014/07/25 00:37:01, msw wrote: > optional nit: instead do: > > Iterator begin = glyph_to_char.begin(); > Iterator end = glyph_to_char.begin(); > if (rtl) { > begin = glyph_to_char.rbegin(); > end = glyph_to_char.rend(); > } > > GetClusterAtImpl(pos, range, begin, end, is_rtl, chars, glyphs); .begin() and .rbegin() return incompatible types (which is why we have the template function GetClusterAtImpl). https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:491: Range TextRunHarfBuzz::GetGraphemeBounds(const base::string16& text, On 2014/07/25 00:37:01, msw wrote: > Remove |text|, it's no longer used here. Done. https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:506: // cluster, we simple divide the cluster width by the number of graphemes. On 2014/07/25 00:37:00, msw wrote: > nit: s/simple/simply Done. https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:507: if (chars.length() != 1 && grapheme_iterator) { On 2014/07/25 00:37:01, msw wrote: > nit: explicitly check |chars.length() > 1|. Done. https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:706: // TODO(msw): The bounds should probably not always be leading the range ends. On 2014/07/25 00:37:00, msw wrote: > nit: I think you're resolving this TODO here by getting the outer grapheme > bounds of the range ends; if you concur, you can remove the TODO. I don't quite understand this TODO, I will remove it if you can confirm it is resolved. https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:719: Range range_x(leftmost_character_x.start(), rightmost_character_x.end()); On 2014/07/25 00:37:01, msw wrote: > nit: It looks like the revised logic should always yield a non-reversed > x-coordinate range; if you concur, please DCHECK(!range_x.is_reversed()) and > remove the line 723 operation to fix reversed ranges. Done. https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2116: { // From string "A B C D" to glyphs "dcb a". On 2014/07/25 00:37:01, msw wrote: > nit: maybe add RTL case "dc ba" for good measure? Done. https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2133: if (!iter->Init()) On 2014/07/25 00:37:01, msw wrote: > Make this ASSERT_TRUE(iter-Init()); Done.
LGTM with a nit. +CC behdad and yeah, you can TBR jshin. https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:706: // TODO(msw): The bounds should probably not always be leading the range ends. On 2014/07/29 23:13:41, ckocagil wrote: > On 2014/07/25 00:37:00, msw wrote: > > nit: I think you're resolving this TODO here by getting the outer grapheme > > bounds of the range ends; if you concur, you can remove the TODO. > > I don't quite understand this TODO, I will remove it if you can confirm it is > resolved. Yeah, you can remove this TODO (at worst the bug will live on for now).
https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/351963002/diff/120001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:706: // TODO(msw): The bounds should probably not always be leading the range ends. On 2014/07/29 23:41:13, msw wrote: > On 2014/07/29 23:13:41, ckocagil wrote: > > On 2014/07/25 00:37:00, msw wrote: > > > nit: I think you're resolving this TODO here by getting the outer grapheme > > > bounds of the range ends; if you concur, you can remove the TODO. > > > > I don't quite understand this TODO, I will remove it if you can confirm it is > > resolved. > > Yeah, you can remove this TODO (at worst the bug will live on for now). Done.
The CQ bit was checked by ckocagil@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckocagil@chromium.org/351963002/190001
The CQ bit was unchecked by ckocagil@chromium.org
The CQ bit was checked by ckocagil@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckocagil@chromium.org/351963002/210001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by ckocagil@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckocagil@chromium.org/351963002/210001
Message was sent while issue was closed.
Change committed as 286569 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
