Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(2324)

Unified Diff: ui/gfx/render_text_harfbuzz.cc

Issue 351963002: RenderTextHarfBuzz: Allow mid-glyph cursors in multi-grapheme clusters (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src
Patch Set: reuse breakiterator Created 6 years, 6 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: ui/gfx/render_text_harfbuzz.cc
diff --git a/ui/gfx/render_text_harfbuzz.cc b/ui/gfx/render_text_harfbuzz.cc
index 3d9bf58141e562af5276a9ec8fe2e9432670df65..ecf97d1f2c1bf6e7146fca98a2f5aacc28642bdc 100644
--- a/ui/gfx/render_text_harfbuzz.cc
+++ b/ui/gfx/render_text_harfbuzz.cc
@@ -399,6 +399,34 @@ inline hb_script_t ICUScriptToHBScript(UScriptCode script) {
return hb_script_from_string(uscript_getShortName(script), -1);
}
+template <class Iterator>
+void GetClusterAtImpl(size_t pos,
+ Range range,
+ Iterator elements_begin,
+ Iterator elements_end,
+ bool reversed,
+ Range* chars,
+ Range* glyphs) {
+ Iterator element = std::upper_bound(elements_begin, elements_end, pos);
+ chars->set_end(element == elements_end ? range.end() : *element);
+ glyphs->set_end(reversed ? elements_end - element : element - elements_begin);
+
+ DCHECK(element != elements_begin);
msw 2014/06/29 22:29:09 nit: can you DCHECK_NE?
ckocagil 2014/07/24 21:46:56 It doesn't compile with DCHECK_NE due to some temp
msw 2014/07/25 00:36:59 Acknowledged.
+ do {
+ --element;
msw 2014/06/29 22:29:09 nit: maybe just make this a one-liner "while(--ele
ckocagil 2014/07/24 21:46:56 Done.
+ } while (element != elements_begin && *element == element[-1]);
msw 2014/06/29 22:29:09 nit: Checking for |element[-1]| is a bit odd, can
ckocagil 2014/07/24 21:46:55 Done.
+ chars->set_start(*element);
+ glyphs->set_start(reversed ?
msw 2014/06/29 22:29:10 nit: move "reversed ?" to the next line.
ckocagil 2014/07/24 21:46:55 Done.
+ elements_end - element : element - elements_begin);
+ if (reversed)
+ *glyphs = Range(glyphs->end(), glyphs->start());
+
+ DCHECK(!chars->is_reversed());
+ DCHECK(!chars->is_empty());
+ DCHECK(!glyphs->is_reversed());
+ DCHECK(!glyphs->is_empty());
+}
+
} // namespace
namespace internal {
@@ -418,23 +446,30 @@ TextRunHarfBuzz::TextRunHarfBuzz()
TextRunHarfBuzz::~TextRunHarfBuzz() {}
-size_t TextRunHarfBuzz::CharToGlyph(size_t pos) const {
- DCHECK(range.start() <= pos && pos < range.end());
+void TextRunHarfBuzz::GetClusterAt(size_t pos,
msw 2014/06/29 22:29:09 Hmm, It might make more sense to just offer GetCha
ckocagil 2014/07/24 21:46:55 But in the cases where we need both, we'd have to
msw 2014/07/25 00:37:00 That's fair; it's a careful balance between perf a
+ Range* chars,
+ Range* glyphs) const {
+ DCHECK(range.Contains(Range(pos, pos + 1)));
- if (!is_rtl) {
- size_t cluster_start = 0;
- for (size_t i = 1; i < glyph_count && pos >= glyph_to_char[i]; ++i)
- if (glyph_to_char[i] != glyph_to_char[i - 1])
- cluster_start = i;
- return cluster_start;
+ scoped_ptr<Range> temp_chars;
+ scoped_ptr<Range> temp_glyphs;
+ if (!chars) {
+ temp_chars.reset(new Range);
+ chars = temp_chars.get();
+ }
+ if (!glyphs) {
+ temp_glyphs.reset(new Range);
+ glyphs = temp_glyphs.get();
}
- for (size_t i = 0; i < glyph_count; ++i) {
- if (pos >= glyph_to_char[i])
- return i;
+ if (is_rtl) {
+ GetClusterAtImpl(pos, range, glyph_to_char.rbegin(), glyph_to_char.rend(),
+ true, chars, glyphs);
+ return;
}
- NOTREACHED();
- return 0;
+
+ GetClusterAtImpl(pos, range, glyph_to_char.begin(), glyph_to_char.end(),
+ false, chars, glyphs);
}
Range TextRunHarfBuzz::CharRangeToGlyphRange(const Range& char_range) const {
@@ -442,29 +477,13 @@ Range TextRunHarfBuzz::CharRangeToGlyphRange(const Range& char_range) const {
DCHECK(!char_range.is_reversed());
DCHECK(!char_range.is_empty());
- size_t first = 0;
- size_t last = 0;
+ Range start_glyphs;
+ Range end_glyphs;
+ GetClusterAt(char_range.start(), NULL, &start_glyphs);
+ GetClusterAt(char_range.end() - 1, NULL, &end_glyphs);
- if (is_rtl) {
- // For RTL runs, we subtract 1 from |char_range| to get the leading edges.
- last = CharToGlyph(char_range.end() - 1);
- // Loop until we find a non-empty glyph range. For multi-character clusters,
- // the loop is needed to find the cluster end. Do the same for LTR below.
- for (size_t i = char_range.start(); i > range.start(); --i) {
- first = CharToGlyph(i - 1);
- if (first != last)
- return Range(last, first);
- }
- return Range(last, glyph_count);
- }
-
- first = CharToGlyph(char_range.start());
- for (size_t i = char_range.end(); i < range.end(); ++i) {
- last = CharToGlyph(i);
- if (first != last)
- return Range(first, last);
- }
- return Range(first, glyph_count);
+ return is_rtl ? Range(end_glyphs.start(), start_glyphs.end()) :
+ Range(start_glyphs.start(), end_glyphs.end());
}
// Returns whether the given shaped run contains any missing glyphs.
@@ -477,17 +496,71 @@ bool TextRunHarfBuzz::HasMissingGlyphs() const {
return false;
}
-int TextRunHarfBuzz::GetGlyphXBoundary(size_t text_index, bool trailing) const {
- if (text_index == range.end()) {
- trailing = true;
- --text_index;
+Range TextRunHarfBuzz::GetGraphemeBounds(const base::string16& text,
msw 2014/06/29 22:29:09 Perhaps GetGraphemeBounds could also be a RenderTe
ckocagil 2014/07/24 21:46:56 I actually want to move more logic to TextRunHarfB
msw 2014/07/25 00:37:00 Acknowledged.
+ size_t text_index) {
+ DCHECK_LT(text_index, range.end());
+
+ Range chars;
+ Range glyphs;
+ GetClusterAt(text_index, &chars, &glyphs);
+ const int cluster_begin_x = SkScalarRoundToInt(positions[glyphs.start()].x());
+ const int cluster_end_x = glyphs.end() < glyph_count ?
+ SkScalarRoundToInt(positions[glyphs.end()].x()) : width;
+
+ if (chars.length() != 1) {
msw 2014/06/29 22:29:09 This block needs a comment about the bounds treatm
ckocagil 2014/07/24 21:46:56 Done.
+ base::i18n::BreakIterator* grapheme_iterator = GetGraphemeIterator(text);
+ if (grapheme_iterator) {
+ int before = 0;
+ int total = 0;
+ for (size_t i = chars.start(); i < chars.end(); ++i) {
msw 2014/06/29 22:29:09 Should we instead calculate and store all the grap
ckocagil 2014/07/24 21:46:56 It seems like ICU is caching bounds internally. Ev
msw 2014/07/25 00:36:59 Acknowledged.
+ if (grapheme_iterator->IsGraphemeBoundary(i - range.start())) {
+ if (i < text_index)
+ ++before;
+ ++total;
+ }
+ }
+
+ if (is_rtl)
+ before = total - before - 1;
+ if (total <= 1) {
msw 2014/06/29 22:29:10 Could total ever really be 0? (DCHECK against that
ckocagil 2014/07/24 21:46:56 You're right, I added a DCHECK.
+ return Range(preceding_run_widths + cluster_begin_x,
msw 2014/06/29 22:29:09 Optionally reverse the conditional to execute the
ckocagil 2014/07/24 21:46:56 Done.
+ preceding_run_widths + cluster_end_x);
+ }
+ DCHECK_GE(before, 0);
msw 2014/06/29 22:29:09 Wouldn't this be false if the index was the first
ckocagil 2014/07/24 21:46:56 I don't understand the question. Perhaps you read
msw 2014/07/25 00:37:00 Yeah, I think you're right, ignore that question.
+ DCHECK_LE(before, total);
+
+ const int cluster_width = cluster_end_x - cluster_begin_x;
+ int grapheme_begin_x = cluster_begin_x;
+ if (before > 0) {
+ grapheme_begin_x += static_cast<int>(0.5f +
+ cluster_width * before / static_cast<float>(total));
+ }
+ int grapheme_end_x = cluster_end_x;
+ if (before != total - 1) {
+ grapheme_end_x = cluster_begin_x + static_cast<int>(0.5f +
+ cluster_width * (before + 1) / static_cast<float>(total));
+ }
+ return Range(preceding_run_widths + grapheme_begin_x,
+ preceding_run_widths + grapheme_end_x);
+ }
+ }
+
+ return Range(preceding_run_widths + cluster_begin_x,
+ preceding_run_widths + cluster_end_x);
+}
+
+base::i18n::BreakIterator* TextRunHarfBuzz::GetGraphemeIterator(
msw 2014/06/29 22:29:09 It seems like the RenderText should have an iterat
ckocagil 2014/07/24 21:46:55 Done.
+ const base::string16& text) {
+ if (!grapheme_iterator.get()) {
+ grapheme_iterator.reset(new base::i18n::BreakIterator(
+ base::string16(), base::i18n::BreakIterator::BREAK_CHARACTER));
+ if (!grapheme_iterator->Init() || !grapheme_iterator->SetText(
+ text.c_str() + range.start(), range.length())) {
+ NOTREACHED();
+ return NULL;
+ }
}
- Range glyph_range = CharRangeToGlyphRange(Range(text_index, text_index + 1));
- const size_t glyph_pos = (is_rtl == trailing) ?
- glyph_range.start() : glyph_range.end();
- const int x = glyph_pos < glyph_count ?
- SkScalarRoundToInt(positions[glyph_pos].x()) : width;
- return preceding_run_widths + x;
+ return grapheme_iterator.get();
}
} // namespace internal
@@ -633,14 +706,16 @@ SelectionModel RenderTextHarfBuzz::AdjacentWordSelectionModel(
}
Range RenderTextHarfBuzz::GetGlyphBounds(size_t index) {
+ EnsureLayout();
const size_t run_index =
GetRunContainingCaret(SelectionModel(index, CURSOR_FORWARD));
// Return edge bounds if the index is invalid or beyond the layout text size.
if (run_index >= runs_.size())
return Range(GetStringSize().width());
const size_t layout_index = TextIndexToLayoutIndex(index);
- return Range(runs_[run_index]->GetGlyphXBoundary(layout_index, false),
- runs_[run_index]->GetGlyphXBoundary(layout_index, true));
+ internal::TextRunHarfBuzz* run = runs_[run_index];
+ Range bounds = run->GetGraphemeBounds(GetLayoutText(), layout_index);
+ return run->is_rtl ? Range(bounds.end(), bounds.start()) : bounds;
}
std::vector<Rect> RenderTextHarfBuzz::GetSubstringBounds(const Range& range) {
@@ -658,12 +733,15 @@ std::vector<Rect> RenderTextHarfBuzz::GetSubstringBounds(const Range& range) {
// Add a Range for each run/selection intersection.
// TODO(msw): The bounds should probably not always be leading the range ends.
for (size_t i = 0; i < runs_.size(); ++i) {
- const internal::TextRunHarfBuzz* run = runs_[visual_to_logical_[i]];
+ internal::TextRunHarfBuzz* run = runs_[visual_to_logical_[i]];
Range intersection = run->range.Intersect(layout_range);
if (intersection.IsValid()) {
DCHECK(!intersection.is_reversed());
- Range range_x(run->GetGlyphXBoundary(intersection.start(), false),
- run->GetGlyphXBoundary(intersection.end(), false));
+ Range range_x(
+ run->GetGraphemeBounds(GetLayoutText(), run->is_rtl ?
+ intersection.end() - 1 : intersection.start()).start(),
+ run->GetGraphemeBounds(GetLayoutText(), run->is_rtl ?
+ intersection.start() : intersection.end() - 1).end());
if (range_x.is_empty())
continue;
range_x = Range(range_x.GetMin(), range_x.GetMax());
@@ -707,11 +785,13 @@ bool RenderTextHarfBuzz::IsValidCursorIndex(size_t index) {
if (!IsValidLogicalIndex(index))
return false;
EnsureLayout();
- // Disallow indices amid multi-character graphemes by checking glyph bounds.
msw 2014/06/29 22:29:09 Please verify that these examples and Issue 327903
ckocagil 2014/07/24 21:46:56 Done. Added a test too.
- // These characters are not surrogate-pairs, but may yield a single glyph:
- // \x0915\x093f - (ki) - one of many Devanagari biconsonantal conjuncts.
- // \x0e08\x0e33 - (cho chan + sara am) - a Thai consonant and vowel pair.
- return GetGlyphBounds(index) != GetGlyphBounds(index - 1);
+ internal::TextRunHarfBuzz* run =
+ runs_[GetRunContainingCaret(SelectionModel(index, CURSOR_BACKWARD))];
+ base::i18n::BreakIterator* grapheme_iterator =
msw 2014/06/29 22:29:09 nit: Name this |iter| for a one-liner and below "r
ckocagil 2014/07/24 21:46:56 Done. (I made it "return !iter || iter->...") sinc
msw 2014/07/25 00:36:59 Acknowledged.
+ run->GetGraphemeIterator(GetLayoutText());
+ if (!grapheme_iterator)
+ return true;
+ return grapheme_iterator->IsGraphemeBoundary(index - run->range.start());
}
void RenderTextHarfBuzz::ResetLayout() {
@@ -989,7 +1069,7 @@ void RenderTextHarfBuzz::ShapeRun(internal::TextRunHarfBuzz* run) {
NULL);
run->glyph_count = glyph_count;
run->glyphs.reset(new uint16[run->glyph_count]);
- run->glyph_to_char.reset(new uint32[run->glyph_count]);
+ run->glyph_to_char.resize(run->glyph_count);
run->positions.reset(new SkPoint[run->glyph_count]);
for (size_t i = 0; i < run->glyph_count; ++i) {
run->glyphs[i] = infos[i].codepoint;

Powered by Google App Engine
This is Rietveld 408576698