Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23522018:
RenderTextWin: Break runs between any two characters that are not in the same code block (Closed)
|
Created:
7 years, 3 months ago by ckocagil Modified:
7 years, 3 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRenderTextWin: Break runs between any two characters that are not in the same code block
BUG=278913
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221776
Patch Set 1 #
Total comments: 6
Patch Set 2 : use CharIterator #
Total comments: 4
Patch Set 3 : addressed comments #
Total comments: 23
Patch Set 4 : fixed for partial surrogates #Patch Set 5 : added test case #
Total comments: 10
Patch Set 6 : don't handle bad ranges #
Total comments: 8
Patch Set 7 : Mike's comments #
Messages
Total messages: 21 (0 generated)
I like the general idea behind this patch, and I hope Alexei will weigh in with his thoughts on this approach. Thanks for working on this! https://codereview.chromium.org/23522018/diff/1/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/23522018/diff/1/ui/gfx/render_text_win.cc#new... ui/gfx/render_text_win.cc:602: // Break runs between any two characters that are not in the same code Can you explain why you chose to break runs between characters in different code blocks, versus between characters in different scripts? As long as fonts can usually shape all the characters in a given script, it seems like splitting by script change would be more correct. UBlockCode has 221 entries, while UScriptCode only has 159, so it would likely offer less unnecessary run breaks too. Does the ▶ (black right-pointing triangle, ▶) share the same script as english characters, but have a different code block? Resource links below. http://www.icu-project.org/apiref/icu4c/uchar_8h.html#a19f1662c9c33c31593612d... http://www.icu-project.org/apiref/icu4c/uchar_8h_source.html http://www.icu-project.org/apiref/icu4c/uscript_8h.html#a5036ff9ca2a7f1d2bde0... http://www.icu-project.org/apiref/icu4c/uscript_8h_source.html https://codereview.chromium.org/23522018/diff/1/ui/gfx/render_text_win.cc#new... ui/gfx/render_text_win.cc:603: // block. http://crbug.com/278913 nit: consider using a comment like: // Break runs between characters in different code blocks. This avoids using // fallback fonts for more characters than needed. http://crbug.com/278913 https://codereview.chromium.org/23522018/diff/1/ui/gfx/render_text_win.cc#new... ui/gfx/render_text_win.cc:606: U16_NEXT(text().c_str(), i, text().length(), current_char); Use base::i18n::UTF16CharIterator instead.
Waiting for you to answer Mike's questions. Also, whatever approach is taken should include a test.
https://codereview.chromium.org/23522018/diff/1/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/23522018/diff/1/ui/gfx/render_text_win.cc#new... ui/gfx/render_text_win.cc:602: // Break runs between any two characters that are not in the same code On 2013/09/03 16:15:31, msw wrote: > Can you explain why you chose to break runs between characters in different code > blocks, versus between characters in different scripts? As long as fonts can > usually shape all the characters in a given script, it seems like splitting by > script change would be more correct. UBlockCode has 221 entries, while > UScriptCode only has 159, so it would likely offer less unnecessary run breaks > too. Does the ▶ (black right-pointing triangle, ▶) share the same script > as english characters, but have a different code block? Resource links below. > > http://www.icu-project.org/apiref/icu4c/uchar_8h.html#a19f1662c9c33c31593612d... > http://www.icu-project.org/apiref/icu4c/uchar_8h_source.html > http://www.icu-project.org/apiref/icu4c/uscript_8h.html#a5036ff9ca2a7f1d2bde0... > http://www.icu-project.org/apiref/icu4c/uscript_8h_source.html I simply didn't know Unicode had classification by script in its standard! I tried to achieve this with UScriptCode. There is a problem. Common characters such as spaces are in USCRIPT_COMMON. I tried always accepting USCRIPT_COMMON in runs, but it turns out the YouTube play character '▶' is in USCRIPT_COMMON too! UBlockCode doesn't have either of these problems. https://codereview.chromium.org/23522018/diff/1/ui/gfx/render_text_win.cc#new... ui/gfx/render_text_win.cc:603: // block. http://crbug.com/278913 On 2013/09/03 16:15:31, msw wrote: > nit: consider using a comment like: > // Break runs between characters in different code blocks. This avoids using > // fallback fonts for more characters than needed. http://crbug.com/278913 Done. https://codereview.chromium.org/23522018/diff/1/ui/gfx/render_text_win.cc#new... ui/gfx/render_text_win.cc:606: U16_NEXT(text().c_str(), i, text().length(), current_char); On 2013/09/03 16:15:31, msw wrote: > Use base::i18n::UTF16CharIterator instead. Done.
Thanks for working on this, by the way! (Still waiting for a unit test for this.) https://codereview.chromium.org/23522018/diff/7001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/23522018/diff/7001/ui/gfx/render_text_win.cc#... ui/gfx/render_text_win.cc:601: run_break = run->range.start() + max_run_length; I noticed that this logic doesn't check for whether it's splitting the run at a surrogate pair when it's clamping the text (it should be checking ui::IsValidCodePointIndex()). Can you fix it or add a TODO here? https://codereview.chromium.org/23522018/diff/7001/ui/gfx/render_text_win.cc#... ui/gfx/render_text_win.cc:606: iter.SetPosition(run->range.start()); This should use GetLayoutText(), not text(). Also, I think you can avoid adding SetPosition() API by instead doing this: const wchar_t* run_text = &(GetLayoutText()[run->range.start()]); UTF16CharIterator iter(run_text, run->range.length()); (And adjusting the math below appropriately - i.e. adding run->range.start() to iter.array_pos()).
Also added a unit test. https://codereview.chromium.org/23522018/diff/7001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/23522018/diff/7001/ui/gfx/render_text_win.cc#... ui/gfx/render_text_win.cc:601: run_break = run->range.start() + max_run_length; On 2013/09/04 14:42:01, Alexei Svitkine wrote: > I noticed that this logic doesn't check for whether it's splitting the run at a > surrogate pair when it's clamping the text (it should be checking > ui::IsValidCodePointIndex()). > > Can you fix it or add a TODO here? Done, fixed. https://codereview.chromium.org/23522018/diff/7001/ui/gfx/render_text_win.cc#... ui/gfx/render_text_win.cc:606: iter.SetPosition(run->range.start()); On 2013/09/04 14:42:01, Alexei Svitkine wrote: > This should use GetLayoutText(), not text(). Done, throughout the function. There was another use of text() in this function (from one of my CLs before), it probably has to be changed as well. > Also, I think you can avoid adding SetPosition() API by instead doing this: > > const wchar_t* run_text = &(GetLayoutText()[run->range.start()]); > UTF16CharIterator iter(run_text, run->range.length()); > > (And adjusting the math below appropriately - i.e. adding run->range.start() to > iter.array_pos()). Done.
Looks good, thanks for fixing the other erroneous uses of text(). A few more comments. https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:577: empty_colors.SetMax(layout_text.length()); Nit: Use |layout_text_length| here or get rid of it and use layout_text.length() everywhere. https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:602: if (!ui::IsValidCodePointIndex(layout_text, run_break)) Move this into the block of the above if. https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:607: size_t run_start = run->range.start(); Nit: const https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:609: layout_text.length() - run_start); This should be run.range.length(). https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:610: const UBlockCode first_block = ublock_getCode(iter.get()); Does this work correctly if there's an empty run? https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:612: static_cast<size_t>(iter.array_pos() + run_start) < run_break) { Instead of doing iter.array_pos() + run_start < run_break everywhere, how about adding a run_end var: const int32 run_end = static_cast<int32>(run_break - run_start); while (iter.Advance() && iter.array_pos() < run_end) { ... } And you can also simplify the DCHECKs. https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:621: Add a DCHECK(ui::IsValidCodePointIndex(run_break)) here.
https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_unitte... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_unitte... ui/gfx/render_text_unittest.cc:1656: const base::string16 kTestString = WideToUTF16(L"x\x25B6y"); nit: Add another test case with spaces, like "x \x25B6 y". https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:602: if (!ui::IsValidCodePointIndex(layout_text, run_break)) Should this loop? Does it need to handle multiple consecutive invalid indices? Does this work for an empty or 1-code-point run that "isn't a valid index"? https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:614: DCHECK_LE(static_cast<size_t>(iter.array_pos() + run_start), run_break); This seems unnecessary; it's checked by the loop. I'd toss the DCHECK_GT too. https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:618: break; I actually think the break is unnecessary, since the loop will just terminate.
https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_unitte... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_unitte... ui/gfx/render_text_unittest.cc:1656: const base::string16 kTestString = WideToUTF16(L"x\x25B6y"); On 2013/09/04 22:05:38, msw wrote: > nit: Add another test case with spaces, like "x \x25B6 y". Done. https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:577: empty_colors.SetMax(layout_text.length()); On 2013/09/04 21:52:36, Alexei Svitkine wrote: > Nit: Use |layout_text_length| here or get rid of it and use layout_text.length() > everywhere. Done, used |layout_text_length| everywhere. https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:602: if (!ui::IsValidCodePointIndex(layout_text, run_break)) On 2013/09/04 21:52:36, Alexei Svitkine wrote: > Move this into the block of the above if. Done. https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:602: if (!ui::IsValidCodePointIndex(layout_text, run_break)) On 2013/09/04 22:05:38, msw wrote: > Should this loop? Does it need to handle multiple consecutive invalid indices? There can't be more than 2 words in a UTF-16 character, so this shouldn't need to loop. > Does this work for an empty or 1-code-point run that "isn't a valid index"? I don't think we should allow runs that start or end between a surrogate pair. But since you asked it we probably do allow it. I moved this block inside the if, now it shouldn't disrupt runs that start or end with surrogate halves. https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:607: size_t run_start = run->range.start(); On 2013/09/04 21:52:36, Alexei Svitkine wrote: > Nit: const We now modify this, see the updated code. https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:609: layout_text.length() - run_start); On 2013/09/04 21:52:36, Alexei Svitkine wrote: > This should be run.range.length(). Done. https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:610: const UBlockCode first_block = ublock_getCode(iter.get()); On 2013/09/04 21:52:36, Alexei Svitkine wrote: > Does this work correctly if there's an empty run? Fixed. https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:612: static_cast<size_t>(iter.array_pos() + run_start) < run_break) { On 2013/09/04 21:52:36, Alexei Svitkine wrote: > Instead of doing iter.array_pos() + run_start < run_break everywhere, how about > adding a run_end var: > > const int32 run_end = static_cast<int32>(run_break - run_start); > while (iter.Advance() && iter.array_pos() < run_end) { > ... > } > > And you can also simplify the DCHECKs. Done. Also I removed the DCHECKs. https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:614: DCHECK_LE(static_cast<size_t>(iter.array_pos() + run_start), run_break); On 2013/09/04 22:05:38, msw wrote: > This seems unnecessary; it's checked by the loop. I'd toss the DCHECK_GT too. Done. https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:618: break; On 2013/09/04 22:05:38, msw wrote: > I actually think the break is unnecessary, since the loop will just terminate. I replaced the |run_break| use in the loop conditional, so the break is necessary now. https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:621: On 2013/09/04 21:52:36, Alexei Svitkine wrote: > Add a DCHECK(ui::IsValidCodePointIndex(run_break)) here. The code above tries not to disrupt runs that start or end with surrogate halves. We should disallow such runs as I said above, but that's probably for another CL.
https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/23522018/diff/20001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:621: On 2013/09/05 20:13:15, ckocagil wrote: > On 2013/09/04 21:52:36, Alexei Svitkine wrote: > > Add a DCHECK(ui::IsValidCodePointIndex(run_break)) here. > > The code above tries not to disrupt runs that start or end with surrogate > halves. We should disallow such runs as I said above, but that's probably for > another CL. Fair enough. Could you add a TODO and file a crbug? https://codereview.chromium.org/23522018/diff/31001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/23522018/diff/31001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:603: !ui::IsValidCodePointIndex(layout_text, run_break)) Nit: Add {}'s. https://codereview.chromium.org/23522018/diff/31001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:611: if (!ui::IsValidCodePointIndex(layout_text, run_start)) I'm not a fan of this logic here. Decrementing run_start seems wrong to me, since now you're overlapping with the previous run. If we can guarantee that run_start is at a valid index (i.e. the previous run_break was at a valid index), then this one shouldn't be necessary, I think. https://codereview.chromium.org/23522018/diff/31001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:614: if (!ui::IsValidCodePointIndex(layout_text, run_break)) In what cases could this be the case? For example, your CL is fixing the clamping code above, so that shouldn't result in the run_break being bad. I guess only if Uniscribe gives us back results via the script item list? If so, it should probably be handled at that stage. https://codereview.chromium.org/23522018/diff/31001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:621: run_break = std::min(run_break, iter.array_pos() + run_start); The mins is only necessary because your code above does --run_start, right?
https://codereview.chromium.org/23522018/diff/31001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/23522018/diff/31001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:611: if (!ui::IsValidCodePointIndex(layout_text, run_start)) On 2013/09/05 20:32:41, Alexei Svitkine wrote: > I'm not a fan of this logic here. Decrementing run_start seems wrong to me, > since now you're overlapping with the previous run. If we can guarantee that > run_start is at a valid index (i.e. the previous run_break was at a valid > index), then this one shouldn't be necessary, I think. |run_start| doesn't change |run->range.start()|. This allows CharIterator to start with a valid char index (even if icu checks it, I would rather do it here explicitly). Surrogate pairs that have halves in different runs will be checked in both runs; this is intentional. https://codereview.chromium.org/23522018/diff/31001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:614: if (!ui::IsValidCodePointIndex(layout_text, run_break)) On 2013/09/05 20:32:41, Alexei Svitkine wrote: > In what cases could this be the case? For example, your CL is fixing the > clamping code above, so that shouldn't result in the run_break being bad. > > I guess only if Uniscribe gives us back results via the script item list? If so, > it should probably be handled at that stage. I'm not sure, style ranges maybe? Do they contain code-point indices? Mike, do you know what causes these runs or if they actually exist? https://codereview.chromium.org/23522018/diff/31001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:621: run_break = std::min(run_break, iter.array_pos() + run_start); On 2013/09/05 20:32:41, Alexei Svitkine wrote: > The mins is only necessary because your code above does --run_start, right? No, it is necessary because of the ++run_length (this allows "iter.pos() + run_start == run_break + 1"). I could use --run_length, but then surrogate halves wouldn't be checked in both of the runs they are in.
https://codereview.chromium.org/23522018/diff/31001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/23522018/diff/31001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:611: if (!ui::IsValidCodePointIndex(layout_text, run_start)) On 2013/09/05 21:08:57, ckocagil wrote: > On 2013/09/05 20:32:41, Alexei Svitkine wrote: > > I'm not a fan of this logic here. Decrementing run_start seems wrong to me, > > since now you're overlapping with the previous run. If we can guarantee that > > run_start is at a valid index (i.e. the previous run_break was at a valid > > index), then this one shouldn't be necessary, I think. > > |run_start| doesn't change |run->range.start()|. This allows CharIterator to > start with a valid char index (even if icu checks it, I would rather do it here > explicitly). Surrogate pairs that have halves in different runs will be checked > in both runs; this is intentional. Right, I understand this, but still am not a fan of it. It would be better if such a case was impossible (i.e. we were guaranteed that the run start is not the 2nd half of a surrogate pair). Otherwise, it makes the logic below quite strange. For example, |first_block| is assigned iter.get(), but if this happened than it would correspond to something starting at a previous run. We should just make sure not to have run boundaries split surrogate pairs and then this extra logic won't be necessary.
https://codereview.chromium.org/23522018/diff/31001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/23522018/diff/31001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:611: if (!ui::IsValidCodePointIndex(layout_text, run_start)) On 2013/09/06 13:28:42, Alexei Svitkine wrote: > On 2013/09/05 21:08:57, ckocagil wrote: > > On 2013/09/05 20:32:41, Alexei Svitkine wrote: > > > I'm not a fan of this logic here. Decrementing run_start seems wrong to me, > > > since now you're overlapping with the previous run. If we can guarantee that > > > run_start is at a valid index (i.e. the previous run_break was at a valid > > > index), then this one shouldn't be necessary, I think. > > > > |run_start| doesn't change |run->range.start()|. This allows CharIterator to > > start with a valid char index (even if icu checks it, I would rather do it > here > > explicitly). Surrogate pairs that have halves in different runs will be > checked > > in both runs; this is intentional. > > Right, I understand this, but still am not a fan of it. It would be better if > such a case was impossible (i.e. we were guaranteed that the run start is not > the 2nd half of a surrogate pair). Otherwise, it makes the logic below quite > strange. For example, |first_block| is assigned iter.get(), but if this happened > than it would correspond to something starting at a previous run. > > We should just make sure not to have run boundaries split surrogate pairs and > then this extra logic won't be necessary. Plus, looking at the UTF16CharIterator() code, it shouldn't crash or anything if we pass it a fragment that splits a surrogate pair. So I'd say just remove these isValidCodePointIndex() checks in this block and add a TODO elsewhere to ensure we don't split surrogate pairs when splitting runs.
On 2013/09/06 13:36:18, Alexei Svitkine wrote: > https://codereview.chromium.org/23522018/diff/31001/ui/gfx/render_text_win.cc > File ui/gfx/render_text_win.cc (right): > > https://codereview.chromium.org/23522018/diff/31001/ui/gfx/render_text_win.cc... > ui/gfx/render_text_win.cc:611: if (!ui::IsValidCodePointIndex(layout_text, > run_start)) > On 2013/09/06 13:28:42, Alexei Svitkine wrote: > > On 2013/09/05 21:08:57, ckocagil wrote: > > > On 2013/09/05 20:32:41, Alexei Svitkine wrote: > > > > I'm not a fan of this logic here. Decrementing run_start seems wrong to > me, > > > > since now you're overlapping with the previous run. If we can guarantee > that > > > > run_start is at a valid index (i.e. the previous run_break was at a valid > > > > index), then this one shouldn't be necessary, I think. > > > > > > |run_start| doesn't change |run->range.start()|. This allows CharIterator to > > > start with a valid char index (even if icu checks it, I would rather do it > > here > > > explicitly). Surrogate pairs that have halves in different runs will be > > checked > > > in both runs; this is intentional. > > > > Right, I understand this, but still am not a fan of it. It would be better if > > such a case was impossible (i.e. we were guaranteed that the run start is not > > the 2nd half of a surrogate pair). Otherwise, it makes the logic below quite > > strange. For example, |first_block| is assigned iter.get(), but if this > happened > > than it would correspond to something starting at a previous run. > > > > We should just make sure not to have run boundaries split surrogate pairs and > > then this extra logic won't be necessary. > > Plus, looking at the UTF16CharIterator() code, it shouldn't crash or anything if > we pass it a fragment that splits a surrogate pair. So I'd say just remove these > isValidCodePointIndex() checks in this block and add a TODO elsewhere to ensure > we don't split surrogate pairs when splitting runs. Done.
LGTM
https://codereview.chromium.org/23522018/diff/31001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/23522018/diff/31001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:614: if (!ui::IsValidCodePointIndex(layout_text, run_break)) On 2013/09/05 21:08:57, ckocagil wrote: > On 2013/09/05 20:32:41, Alexei Svitkine wrote: > > In what cases could this be the case? For example, your CL is fixing the > > clamping code above, so that shouldn't result in the run_break being bad. > > > > I guess only if Uniscribe gives us back results via the script item list? If > so, > > it should probably be handled at that stage. > > I'm not sure, style ranges maybe? Do they contain code-point indices? Mike, do > you know what causes these runs or if they actually exist? Uniscribe or style ranges theoretically could, but shouldn't in practice. It's not worth handling without repro, the TODO/DCHECK is sufficient. https://codereview.chromium.org/23522018/diff/39001/ui/gfx/render_text_unitte... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/23522018/diff/39001/ui/gfx/render_text_unitte... ui/gfx/render_text_unittest.cc:1656: const base::string16 kTestString1 = WideToUTF16(L"x\x25B6y"); nit: inline these in the SetText calls. https://codereview.chromium.org/23522018/diff/39001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/23522018/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:602: if (run_break < layout_text_length && IsValidCodePointIndex should return true if run_break == layout_text_length, and there shouldn't be any cases where run_break > layout_text_length (given that the next script item break was larger than run->range.start() + max_run_length), so please remove this first condition. You can add a DCHECK_LE(run_break, layout_text_length) beforehand if you'd like. https://codereview.chromium.org/23522018/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:614: const UBlockCode first_block = ublock_getCode(iter.get()); nit: first_block_code https://codereview.chromium.org/23522018/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:623: // TODO(ckocagil): Make sure that no run starts or ends with an unmatched Isn't this simply DCHECK(ui::IsValidCodePointIndex(layout_text, run_break));?
https://codereview.chromium.org/23522018/diff/39001/ui/gfx/render_text_unitte... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/23522018/diff/39001/ui/gfx/render_text_unitte... ui/gfx/render_text_unittest.cc:1656: const base::string16 kTestString1 = WideToUTF16(L"x\x25B6y"); On 2013/09/06 16:51:28, msw wrote: > nit: inline these in the SetText calls. Done. https://codereview.chromium.org/23522018/diff/39001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/23522018/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:602: if (run_break < layout_text_length && On 2013/09/06 16:51:28, msw wrote: > IsValidCodePointIndex should return true if run_break == layout_text_length, and > there shouldn't be any cases where run_break > layout_text_length (given that > the next script item break was larger than run->range.start() + max_run_length), > so please remove this first condition. You can add a DCHECK_LE(run_break, > layout_text_length) beforehand if you'd like. Done. This was for the |run_break == layout_text_length| case. There shouldn't be any problems given that |ui::IsValidCodePointIndex()| handles it. https://codereview.chromium.org/23522018/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:614: const UBlockCode first_block = ublock_getCode(iter.get()); On 2013/09/06 16:51:28, msw wrote: > nit: first_block_code Done. https://codereview.chromium.org/23522018/diff/39001/ui/gfx/render_text_win.cc... ui/gfx/render_text_win.cc:623: // TODO(ckocagil): Make sure that no run starts or ends with an unmatched On 2013/09/06 16:51:28, msw wrote: > Isn't this simply DCHECK(ui::IsValidCodePointIndex(layout_text, run_break));? Yes. I left it as a TODO because I was unsure whether these "bad" run ranges existed. Removed the TODO and added a DCHECK.
LGTM, very nice work!
Thanks for the nice reviews!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckocagil@chromium.org/23522018/45001
Message was sent while issue was closed.
Change committed as 221776 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
