Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7458014:
Implement Uniscribe RenderText for Windows. (Closed)
|
Created:
9 years, 5 months ago by msw Modified:
9 years, 3 months ago CC:
chromium-reviews, dhollowa, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement Uniscribe RenderText for Windows.
Follow the I18N recommendations for BiDi text editing.
Visual cursor movement and logical selection over BiDi text.
Cleanup some common RenderText code and interfaces.
Fixup TextfieldExample for views_examples.
Known issues:
Word breaking is not well implemented.
Font sizes and vertical alignments are slightly off.
Text styles break runs (colors can affect glyph shaping).
Composition/selection ranges aren't stylized.
BUG=90426
TEST=--use-pure-views text editing
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98785
Patch Set 1 : Implement Uniscribe RenderText for Windows. #Patch Set 2 : Added naive styling and better caching. #Patch Set 3 : Begin cursor and selection work. #Patch Set 4 : Merge SelectionModel, refining cursor interaction. #Patch Set 5 : Add WORD_BREAK and complex text support; numerous fixes. #Patch Set 6 : Add RTL display support, add MoveCursorTo size_t, fix SelectWord. #Patch Set 7 : Add simple BreakIterator support, use scoped_array, fix home/end, invalidate on toggle insert, etc. #
Total comments: 14
Patch Set 8 : Adjust Windows-like word breaks, update tests, address comments. #Patch Set 9 : Fix Linux build error. #
Total comments: 8
Patch Set 10 : Address comments, re-layout on SetDisplayRect and ApplyDefaultStyle. #
Total comments: 8
Patch Set 11 : Address nits. #
Total comments: 37
Patch Set 12 : Address comments, move some code. #Patch Set 13 : Fix RenderText::RightEndSelectionModel. #
Total comments: 9
Patch Set 14 : Seek parity with RenderTextLinux, nix tentative word breaking, address comments. #
Total comments: 9
Patch Set 15 : Update DCHECK #Patch Set 16 : Add Skia drawPosText support. #
Messages
Total messages: 32 (0 generated)
Just an FYI; this work is still underway. I'm simply sharing my progress here, in case it's useful.
Uniscribe is mostly done. PTAL, thanks! Known notable issues: - Word breaking doesn't skip simple whitespace/punctuation. - All style changes break runs (colors can affect glyph shaping). - Composition/selection ranges aren't stylized (but selection draws).
I'm confused because I can't reproduce those unit test failures locally. I'm trying to investigate if they are a Windows XP specific issue.
On 2011/08/18 18:03:55, msw wrote: > I'm confused because I can't reproduce those unit test failures locally. I'm > trying to investigate if they are a Windows XP specific issue. Doh! I was using the old chrome/Debug/views_unittests.exe. Looking now.
let me know if I should wait or review it anyway http://codereview.chromium.org/7458014/diff/19001/base/i18n/break_iterator.h File base/i18n/break_iterator.h (right): http://codereview.chromium.org/7458014/diff/19001/base/i18n/break_iterator.h#... base/i18n/break_iterator.h:91: bool IsBreak(size_t position); IsBreakAt may be?
You can hold off if you'd like. I'm updating word breaking, tests, perhaps tweaking more.
http://codereview.chromium.org/7458014/diff/19001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/7458014/diff/19001/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:246: SetSelectionModel(sel); hmm... the functionality seems different from previous version. Why is this change? Without the change, what is wrong? Is there any case that caret_pos could be equal to position (which means index of previous grapheme is the same as current) except when postion ==0 and caret is placed at leading? http://codereview.chromium.org/7458014/diff/19001/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:323: MoveCursorTo(cursor_position, true); why this change? http://codereview.chromium.org/7458014/diff/19001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/7458014/diff/19001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:193: void MoveCursorTo(size_t position, bool select); seems that this is not used as public (and we probably should not expose such functionality to public anyway). Also, might need more description on where the caret is visually. http://codereview.chromium.org/7458014/diff/19001/views/controls/textfield/na... File views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/7458014/diff/19001/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:868: model_->MoveCursorLeft(gfx::LINE_BREAK, selection); we need make this platform dependent. in windows, the directionality of string is determined by UI's directionality, so above is working. in linux (chromeos too), the directionality of string is determined by its first strong directionality character's direction. so above wont work. Or we use model_->render_text()->GetTextDirection()
On 2011/08/17 21:42:47, msw wrote: > Uniscribe is mostly done. PTAL, thanks! Known notable issues: > - Word breaking doesn't skip simple whitespace/punctuation. do you mean word breaks stops both before and after space, for example: "abc def", the word breaks are "|abc| |def|"? > - All style changes break runs (colors can affect glyph shaping). > - Composition/selection ranges aren't stylized (but selection draws).
On 2011/08/18 22:49:10, xji wrote: > On 2011/08/17 21:42:47, msw wrote: > > Uniscribe is mostly done. PTAL, thanks! Known notable issues: > > - Word breaking doesn't skip simple whitespace/punctuation. > > do you mean word breaks stops both before and after space, for example: > "abc def", the word breaks are "|abc| |def|"? > > > - All style changes break runs (colors can affect glyph shaping). > > - Composition/selection ranges aren't stylized (but selection draws). Yeah, that was the case, but by checking that the ubrk_getRuleStatus isn't UBRK_WORD_NONE, it skips spaces, but still doesn't behave like standard/common windows text fields. Now it skips spaces/punctuation even when moving the cursor left. I'm not sure how much this matters or if there's necessarily "correct" behavior, but I'm looking for a simple-ish fix to achieve parity. I'm hoping to avoid advancing through the whole string to the position or pre-computing a list of word stops for each new string.
On 2011/08/18 23:01:14, msw wrote: > On 2011/08/18 22:49:10, xji wrote: > > On 2011/08/17 21:42:47, msw wrote: > > > Uniscribe is mostly done. PTAL, thanks! Known notable issues: > > > - Word breaking doesn't skip simple whitespace/punctuation. > > > > do you mean word breaks stops both before and after space, for example: > > "abc def", the word breaks are "|abc| |def|"? > > > > > - All style changes break runs (colors can affect glyph shaping). > > > - Composition/selection ranges aren't stylized (but selection draws). > > Yeah, that was the case, but by checking that the ubrk_getRuleStatus isn't > UBRK_WORD_NONE, it skips spaces, but still doesn't behave like standard/common > windows text fields. Now it skips spaces/punctuation even when moving the cursor > left. > > I'm not sure how much this matters or if there's necessarily "correct" behavior, > but I'm looking for a simple-ish fix to achieve parity. I'm hoping to avoid > advancing through the whole string to the position or pre-computing a list of > word stops for each new string. visual word break positions seems are platform-dependent. For windows, given "abc def hij", from left to right, the word break positions are "abc |def |hij|", from right to left, it is the same. For Mac/Linux, from left to right, word breaks are "abc| def| hij|". from right to left, they are "|abc| def| hij|"
Comments addressed, adjusted word breaks/tests. PTAL, thanks! http://codereview.chromium.org/7458014/diff/19001/base/i18n/break_iterator.h File base/i18n/break_iterator.h (right): http://codereview.chromium.org/7458014/diff/19001/base/i18n/break_iterator.h#... base/i18n/break_iterator.h:91: bool IsBreak(size_t position); On 2011/08/18 22:31:13, oshima wrote: > IsBreakAt may be? I went with IsWordBreakAt http://codereview.chromium.org/7458014/diff/19001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/7458014/diff/19001/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:246: SetSelectionModel(sel); On 2011/08/18 22:47:54, xji wrote: > hmm... the functionality seems different from previous version. Why is this > change? Without the change, what is wrong? Is there any case that caret_pos > could be equal to position (which means index of previous grapheme is the same > as current) except when postion ==0 and caret is placed at leading? This has been reincarnated as a convenience function to set the caret trailing the cursor's previous grapheme (or lead the cursor if there is no previous grapheme). This seems to be the intended behavior in numerous cases. Without the change, my primary hurdle was SetCursorPosition assuming there was a previous grapheme, and no convenient way to SetCursorPosition while retaining the selection start. I opted to check for a non-equal previous grapheme rather than checking for a cursor at 0, because if the first character is complex, the cursor could theoretically be at 1 with no previous grapheme. http://codereview.chromium.org/7458014/diff/19001/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:323: MoveCursorTo(cursor_position, true); On 2011/08/18 22:47:54, xji wrote: > why this change? Convenience of built-in checking for s previous grapheme. http://codereview.chromium.org/7458014/diff/19001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/7458014/diff/19001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:193: void MoveCursorTo(size_t position, bool select); On 2011/08/18 22:47:54, xji wrote: > seems that this is not used as public (and we probably should not expose such > functionality to public anyway). Also, might need more description on where the > caret is visually. Done. http://codereview.chromium.org/7458014/diff/19001/views/controls/textfield/na... File views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/7458014/diff/19001/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:868: model_->MoveCursorLeft(gfx::LINE_BREAK, selection); On 2011/08/18 22:47:54, xji wrote: > we need make this platform dependent. > in windows, the directionality of string is determined by UI's directionality, > so above is working. > in linux (chromeos too), the directionality of string is determined by its first > strong directionality character's direction. so above wont work. > > Or we use model_->render_text()->GetTextDirection() Done.
http://codereview.chromium.org/7458014/diff/19001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/7458014/diff/19001/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:246: SetSelectionModel(sel); On 2011/08/19 10:55:30, msw wrote: > On 2011/08/18 22:47:54, xji wrote: > > hmm... the functionality seems different from previous version. Why is this > > change? Without the change, what is wrong? Is there any case that caret_pos > > could be equal to position (which means index of previous grapheme is the same > > as current) except when postion ==0 and caret is placed at leading? > > This has been reincarnated as a convenience function to set the caret trailing > the cursor's previous grapheme (or lead the cursor if there is no previous > grapheme). This seems to be the intended behavior in numerous cases. > > Without the change, my primary hurdle was SetCursorPosition assuming there was a > previous grapheme, >and no convenient way to SetCursorPosition while retaining > the selection start. > > I opted to check for a non-equal previous grapheme rather than checking for a > cursor at 0, because if the first character is complex, the cursor could > theoretically be at 1 with no previous grapheme. make sense. but I think for complex script, say first characters are "a" and "b" which are in one grapheme, if the text is "a", when cursor is at index 1, previous grapheme is "a", index is 0. if the text is "ab", we should not allow cursor at index 1, even if cursor at index 1, previous grapheme index is 0. So, the only case that caret_placement is leading is when position==0. http://codereview.chromium.org/7458014/diff/19001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/7458014/diff/19001/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:108: return (p.x() < 0) ? LeftEndSelectionModel() : RightEndSelectionModel(); is point.x() need to be offset by display_rect() here? and does it work for RTL UI?
http://codereview.chromium.org/7458014/diff/19001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/7458014/diff/19001/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:246: SetSelectionModel(sel); On 2011/08/19 17:38:13, xji wrote: > On 2011/08/19 10:55:30, msw wrote: > > On 2011/08/18 22:47:54, xji wrote: > > > hmm... the functionality seems different from previous version. Why is this > > > change? Without the change, what is wrong? Is there any case that caret_pos > > > could be equal to position (which means index of previous grapheme is the > same > > > as current) except when postion ==0 and caret is placed at leading? > > > > This has been reincarnated as a convenience function to set the caret trailing > > the cursor's previous grapheme (or lead the cursor if there is no previous > > grapheme). This seems to be the intended behavior in numerous cases. > > > > Without the change, my primary hurdle was SetCursorPosition assuming there was > a > > previous grapheme, > >and no convenient way to SetCursorPosition while retaining > > the selection start. > > > > I opted to check for a non-equal previous grapheme rather than checking for a > > cursor at 0, because if the first character is complex, the cursor could > > theoretically be at 1 with no previous grapheme. > > make sense. > > but I think for complex script, say first characters are "a" and "b" which are > in one grapheme, > if the text is "a", when cursor is at index 1, previous grapheme is "a", index > is 0. > if the text is "ab", we should not allow cursor at index 1, even if cursor at > index 1, previous grapheme index is 0. > So, the only case that caret_placement is leading is when position==0. Interesting, I wonder how that works with the IMEs and composition text, we'll have to do some good testing around that, I think it's okay to address that in followup changes. http://codereview.chromium.org/7458014/diff/19001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/7458014/diff/19001/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:108: return (p.x() < 0) ? LeftEndSelectionModel() : RightEndSelectionModel(); On 2011/08/19 17:38:13, xji wrote: > is point.x() need to be offset by display_rect() here? > and does it work for RTL UI? See the initialization of p and function ToTextPoint.
just checked styles. I'd leave the logical verification to xji. http://codereview.chromium.org/7458014/diff/28001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/7458014/diff/28001/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:52: logical_to_visual_() { no need to list object with default constructor. http://codereview.chromium.org/7458014/diff/28001/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:70: ScriptFreeCache(&script_cache_); don't we have to clean up run_? http://codereview.chromium.org/7458014/diff/28001/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:286: bool character_break = (break_type == CHARACTER_BREAK); no need for () http://codereview.chromium.org/7458014/diff/28001/ui/gfx/render_text_win.h File ui/gfx/render_text_win.h (right): http://codereview.chromium.org/7458014/diff/28001/ui/gfx/render_text_win.h#ne... ui/gfx/render_text_win.h:16: struct TextRun { should this be in namespace internal?
Comments addressed. PTAL, thanks! http://codereview.chromium.org/7458014/diff/28001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/7458014/diff/28001/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:52: logical_to_visual_() { On 2011/08/19 18:29:10, oshima wrote: > no need to list object with default constructor. Removed all but script_* POD, which is otherwise uninitialized, added a comment. http://codereview.chromium.org/7458014/diff/28001/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:70: ScriptFreeCache(&script_cache_); On 2011/08/19 18:29:10, oshima wrote: > don't we have to clean up run_? Done. http://codereview.chromium.org/7458014/diff/28001/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:286: bool character_break = (break_type == CHARACTER_BREAK); On 2011/08/19 18:29:10, oshima wrote: > no need for () Done. http://codereview.chromium.org/7458014/diff/28001/ui/gfx/render_text_win.h File ui/gfx/render_text_win.h (right): http://codereview.chromium.org/7458014/diff/28001/ui/gfx/render_text_win.h#ne... ui/gfx/render_text_win.h:16: struct TextRun { On 2011/08/19 18:29:10, oshima wrote: > should this be in namespace internal? Done.
LGTM on styles. http://codereview.chromium.org/7458014/diff/34002/base/i18n/break_iterator.cc File base/i18n/break_iterator.cc (right): http://codereview.chromium.org/7458014/diff/34002/base/i18n/break_iterator.cc... base/i18n/break_iterator.cc:103: (next_status != UBRK_WORD_NONE)); nit: you don't need (), do you? http://codereview.chromium.org/7458014/diff/34002/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/7458014/diff/34002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:31: glyph_count(0) { I believe () and (0) are the same. It's up to you though. http://codereview.chromium.org/7458014/diff/34002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:230: bool leading = (selection.caret_placement() == SelectionModel::LEADING); nit: nuke () http://codereview.chromium.org/7458014/diff/34002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:256: bool character_break = (break_type == CHARACTER_BREAK); ditto
Done n-1 nits :) I'll wait for Xiaomei's review; thanks! http://codereview.chromium.org/7458014/diff/34002/base/i18n/break_iterator.cc File base/i18n/break_iterator.cc (right): http://codereview.chromium.org/7458014/diff/34002/base/i18n/break_iterator.cc... base/i18n/break_iterator.cc:103: (next_status != UBRK_WORD_NONE)); On 2011/08/19 21:25:24, oshima wrote: > nit: you don't need (), do you? Done. http://codereview.chromium.org/7458014/diff/34002/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/7458014/diff/34002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:31: glyph_count(0) { On 2011/08/19 21:25:24, oshima wrote: > I believe () and (0) are the same. It's up to you though. Eh, I think being explicit here is nice. http://codereview.chromium.org/7458014/diff/34002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:230: bool leading = (selection.caret_placement() == SelectionModel::LEADING); On 2011/08/19 21:25:24, oshima wrote: > nit: nuke () Done. http://codereview.chromium.org/7458014/diff/34002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:256: bool character_break = (break_type == CHARACTER_BREAK); On 2011/08/19 21:25:24, oshima wrote: > ditto Done.
http://codereview.chromium.org/7458014/diff/31002/base/i18n/break_iterator.cc File base/i18n/break_iterator.cc (right): http://codereview.chromium.org/7458014/diff/31002/base/i18n/break_iterator.cc... base/i18n/break_iterator.cc:103: next_status != UBRK_WORD_NONE); this probably wont work for Chinese, it should return true also if neither status nor next_status is UBRK_WORD_NONE. http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:123: &trailing); I am not able to tell whether the parameters are number of UTF16 or number of UTF32. The doc uses both "characters" and "code points". you might need to run test. But I think we can always fix it later. http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:134: return SelectionModel(position, position, SelectionModel::LEADING); hm.. I think we have difference here. for example: "abcFED", when click left part of 'E', you set selection model as (5, 5, leading), I set it as (5, 4, trailing). http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:170: &end_offset); is it correct that the bounds always from leading edge of start to leading edge of end? BTW, I just realized that this is a public API (although no actual usage so far). Why this is public, what would be a usage scenario? http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:227: (display_rect().height() - rect.height()) / 2); so if run is not found, rect.x() will be set to the right end of string for LTR UI, and set to the left end of string for RTL UI (done by ToViewPoint)? http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:262: size_t run_index = GetRunContainingPosition(left.selection_end()); why use selection_end to get run containing position? does it work for bidi-text? For example: abcF|ED ->abc|FED, left.selection_end() ==6, run_index == runs_.size(), so "abc|FED" will be returned as a word break position. http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:267: bool advancing = runs_[run_index]->script_analysis.fRTL; advancing seems not used. http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:300: bool advancing = !runs_[run_index]->script_analysis.fRTL; advancing seems not used. http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:319: for (size_t n = kGuessItems; hr == E_OUTOFMEMORY && n < kMaxItems; n *= 2) { kGussItem is just used for guessing the max number of runs, wont do any allocation, right? http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:331: } WCHAR is 16-bit in windows, right? from http://msdn.microsoft.com/en-us/library/dd374039(v=vs.85).aspx, seems that windows use unicode code point mean a UTF16 (WCHAR). http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:375: const int max_glyphs = static_cast<int>(1.5 * run_length + 16); why assigned like that? http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:410: run->logical_attributes.get()); we are targeting to single-line text, so do we need ScriptBreak? http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:484: // The character is at the end of its run; advance to the next visual run. s/end/begin/? s/next/previous/ http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:488: internal::TextRun* next_run = runs_[visual_to_logical_[visual_index - 1]]; s/next_run/prev_run/ http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:638: p.Subtract(GetUpdatedDisplayOffset()); p = p.Subtract(); http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:646: p.Add(GetUpdatedDisplayOffset()); p = p.Add(); I am using the above 2 functions as well, so I moved them to RenderText, and will remove them from RenderTextWin when/after I submit http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.h File ui/gfx/render_text_win.h (right): http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.h#ne... ui/gfx/render_text_win.h:42: ABC abc_widths; what is ABC?
http://codereview.chromium.org/7458014/diff/31002/base/i18n/break_iterator.cc File base/i18n/break_iterator.cc (right): http://codereview.chromium.org/7458014/diff/31002/base/i18n/break_iterator.cc... base/i18n/break_iterator.cc:103: next_status != UBRK_WORD_NONE); On 2011/08/23 07:39:51, xji wrote: > this probably wont work for Chinese, it should return true also if neither > status nor next_status is UBRK_WORD_NONE. The default Omnibox font won't even display Chinese correctly for Uniscribe, it needs a font fallback implementation. This is just temporary code to approximate the existing behavior on Windows. I've added a TODO here in addition to the TODO on the declaration in the header. http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:123: &trailing); On 2011/08/23 07:39:51, xji wrote: > I am not able to tell whether the parameters are number of UTF16 or number of > UTF32. The doc uses both "characters" and "code points". you might need to run > test. But I think we can always fix it later. Perhaps this document clarifies your concern: http://msdn.microsoft.com/en-us/library/dd374069.aspx I think it's UTF16, with |trailing| supporting values greater than one to handle complex script / surrogate pairs. Yes, this should be tested, I think we can address any issues in subsequent changes. http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:134: return SelectionModel(position, position, SelectionModel::LEADING); On 2011/08/23 07:39:51, xji wrote: > hm.. I think we have difference here. for example: "abcFED", when click left > part of 'E', you set selection model as (5, 5, leading), I set it as (5, 4, > trailing). I've adjusted the logic to (hopefully) match yours. http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:170: &end_offset); On 2011/08/23 07:39:51, xji wrote: > is it correct that the bounds always from leading edge of start to leading edge > of end? > > BTW, I just realized that this is a public API (although no actual usage so > far). Why this is public, what would be a usage scenario? I can't devise an example offhand, but perhaps it's not always correct. I'm using it to render the selection on Windows, although I could potentially use a background color as you do in RenderTextLinux. I removed the one unused external client (TextfieldViewsModel::GetSelectionBounds), made the function protected, and added a TODO to re-evaluate this function's necessity, signature, and implementation. I thought we'd need the selection bounds for operations like drag and drop, but IsPointInSelection might suffice. http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:227: (display_rect().height() - rect.height()) / 2); On 2011/08/23 07:39:51, xji wrote: > so if run is not found, rect.x() will be set to the right end of string for LTR > UI, and set to the left end of string for RTL UI (done by ToViewPoint)? Yeah, I added a comment to that effect. http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:262: size_t run_index = GetRunContainingPosition(left.selection_end()); On 2011/08/23 07:39:51, xji wrote: > why use selection_end to get run containing position? > does it work for bidi-text? For example: abcF|ED ->abc|FED, > left.selection_end() ==6, run_index == runs_.size(), so "abc|FED" will be > returned as a word break position. You're right and I've changed this to use caret_pos, but now my current word break detection has a false positive moving from |abc -> a|bc (0, 0, trailing), since the caret_pos of 0 is at a word break... I'm not entirely sure what the optimal word break algorithm is. Perhaps that's best addressed in a follow-up change? http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:267: bool advancing = runs_[run_index]->script_analysis.fRTL; On 2011/08/23 07:39:51, xji wrote: > advancing seems not used. Done. http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:300: bool advancing = !runs_[run_index]->script_analysis.fRTL; On 2011/08/23 07:39:51, xji wrote: > advancing seems not used. Done. http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:319: for (size_t n = kGuessItems; hr == E_OUTOFMEMORY && n < kMaxItems; n *= 2) { On 2011/08/23 07:39:51, xji wrote: > kGussItem is just used for guessing the max number of runs, wont do any > allocation, right? I don't know what you mean by "won't do any allocation", the line script_items.reset(new SCRIPT_ITEM[n]) is allocating a buffer of n SCRIPT_ITEMS, where n's initial value is kGuessItems. http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:331: } On 2011/08/23 07:39:51, xji wrote: > WCHAR is 16-bit in windows, right? > from http://msdn.microsoft.com/en-us/library/dd374039%28v=vs.85%29.aspx, > seems that windows use unicode code point mean a UTF16 (WCHAR). Yes. http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:375: const int max_glyphs = static_cast<int>(1.5 * run_length + 16); On 2011/08/23 07:39:51, xji wrote: > why assigned like that? From http://msdn.microsoft.com/en-us/library/dd368564.aspx cMaxGlyphs [in] - Maximum number of glyphs to generate, and the length of pwOutGlyphs. A reasonable value is (1.5 * cChars + 16), but this value might be insufficient in some circumstances. I've made this a loop like the code for ScriptItemize. http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:410: run->logical_attributes.get()); On 2011/08/23 07:39:51, xji wrote: > we are targeting to single-line text, so do we need ScriptBreak? I was previously using this for word break, and I'm still using it for character breaks. Apparently I can derive the character stops from the run's logical clusters, but I don't see a compelling advantage to doing that right now. I've added a TODO, just in case. http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:484: // The character is at the end of its run; advance to the next visual run. On 2011/08/23 07:39:51, xji wrote: > s/end/begin/? > s/next/previous/ Done. http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:488: internal::TextRun* next_run = runs_[visual_to_logical_[visual_index - 1]]; On 2011/08/23 07:39:51, xji wrote: > s/next_run/prev_run/ Done. http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:638: p.Subtract(GetUpdatedDisplayOffset()); On 2011/08/23 07:39:51, xji wrote: > p = p.Subtract(); Done. http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:646: p.Add(GetUpdatedDisplayOffset()); On 2011/08/23 07:39:51, xji wrote: > p = p.Add(); > > I am using the above 2 functions as well, so I moved them to RenderText, and > will remove them from RenderTextWin when/after I submit Done, I moved them to RenderText as well, whoever is last to land can merge. http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.h File ui/gfx/render_text_win.h (right): http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.h#ne... ui/gfx/render_text_win.h:42: ABC abc_widths; On 2011/08/23 07:39:51, xji wrote: > what is ABC? http://msdn.microsoft.com/en-us/library/dd162454.aspx http://msdn.microsoft.com/en-us/library/dd374094.aspx
The diff of render_text_win.cc is bloated by the function definitions reshuffling. I compared the code with patch 11 and looks like the main change is the word break change (GetRunContainsPosition(left.selection_end()). In case of my missing anything, please let me know if you have any changes in mind that would like me to take a look. I think the bidi part looks fine. But I am not familiar with Uniscribe (my knowledge is just from reading doc), if you know anyone is an expert on Uniscribe, you might want to ask him/her to double check. http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:123: &trailing); On 2011/08/24 10:15:40, msw wrote: > On 2011/08/23 07:39:51, xji wrote: > > I am not able to tell whether the parameters are number of UTF16 or number of > > UTF32. The doc uses both "characters" and "code points". you might need to run > > test. But I think we can always fix it later. > > Perhaps this document clarifies your concern: > http://msdn.microsoft.com/en-us/library/dd374069.aspx > > I think it's UTF16, with |trailing| supporting values greater than one to handle > complex script / surrogate pairs. Yes, this should be tested, I think we can > address any issues in subsequent changes. Thanks for the link. I think they used the wrong terminology. I checked with ICU guys when I first read Uniscribe doc on what should "code point" mean. The answer is a UTF32 character. Anyway, I think you are fine. http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:262: size_t run_index = GetRunContainingPosition(left.selection_end()); On 2011/08/24 10:15:40, msw wrote: > On 2011/08/23 07:39:51, xji wrote: > > why use selection_end to get run containing position? > > does it work for bidi-text? For example: abcF|ED ->abc|FED, > > left.selection_end() ==6, run_index == runs_.size(), so "abc|FED" will be > > returned as a word break position. > > You're right and I've changed this to use caret_pos, but now my current word > break detection has a false positive moving from |abc -> a|bc (0, 0, trailing), > since the caret_pos of 0 is at a word break... I'm not entirely sure what the > optimal word break algorithm is. Perhaps that's best addressed in a follow-up > change? it is fine to leave it for follow-up. Looks like the run_index/char_index/attributes are all for getting a grapheme boundary. If you separate moving by word from moving by character, it might be easier. Moving by character behaves the same as before (using selection_end to GetRunContaingingPosition). Moving by word will just call moving by character and check whether the returning position is a word break. hm.. that makes me wondering whether I should use the same signature of yours on Left/RightSelectionModel? Mine is done on grapheme boundary, but yours is not. Maybe I should use my old names LeftRightSelectionModelByGrapheme. http://codereview.chromium.org/7458014/diff/31002/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:319: for (size_t n = kGuessItems; hr == E_OUTOFMEMORY && n < kMaxItems; n *= 2) { On 2011/08/24 10:15:40, msw wrote: > On 2011/08/23 07:39:51, xji wrote: > > kGussItem is just used for guessing the max number of runs, wont do any > > allocation, right? > > I don't know what you mean by "won't do any allocation", the line > script_items.reset(new SCRIPT_ITEM[n]) is allocating a buffer of n SCRIPT_ITEMS, > where n's initial value is kGuessItems. I see. I was thinking since we are dealing with one line text, 100 might be more than enough for runs. But it is fine to leave it there with a TODO. http://codereview.chromium.org/7458014/diff/44013/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/7458014/diff/44013/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:242: SelectionModel end = base::i18n::IsRTL() ? LeftEndSelectionModel() : change base::i18n::IsRTL() to GetTextDirection() == base::i18n::RIGHT_TO_LEFT http://codereview.chromium.org/7458014/diff/44013/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/7458014/diff/44013/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:216: if (run_index == runs_.size() || left.Equals(LeftEndSelectionModel())) the changed logic actually has the same functionality as before (patch 11). I think use "GetRunContainingPosition(left.selection_end())" is better. As for the problem of treating position "abc|FED" as word break, please refer my other comment. http://codereview.chromium.org/7458014/diff/44013/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:251: size_t run_index = GetRunContainingPosition(position); ditto http://codereview.chromium.org/7458014/diff/44013/views/controls/textfield/na... File views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/7458014/diff/44013/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:110: #endif are the changes here related to bidi/complex script?
I removed the tentative and incomplete word break code and rewrote the cursor handling to match that of RenderTextLinux. Please take a look, thanks! I'll ask around for someone with Uniscribe expertise; maybe Skia/Webkit devs. (mac trybot broken) http://codereview.chromium.org/7458014/diff/44013/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/7458014/diff/44013/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:242: SelectionModel end = base::i18n::IsRTL() ? LeftEndSelectionModel() : On 2011/08/25 05:58:38, xji wrote: > change > base::i18n::IsRTL() > to > GetTextDirection() == base::i18n::RIGHT_TO_LEFT Done. http://codereview.chromium.org/7458014/diff/44013/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/7458014/diff/44013/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:216: if (run_index == runs_.size() || left.Equals(LeftEndSelectionModel())) On 2011/08/25 05:58:38, xji wrote: > the changed logic actually has the same functionality as before (patch 11). > I think use "GetRunContainingPosition(left.selection_end())" is better. > > As for the problem of treating position "abc|FED" as word break, please refer my > other comment. I removed the word break code and rewrote this to match RenderTextLinux. http://codereview.chromium.org/7458014/diff/44013/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:251: size_t run_index = GetRunContainingPosition(position); On 2011/08/25 05:58:38, xji wrote: > ditto See above. http://codereview.chromium.org/7458014/diff/44013/views/controls/textfield/na... File views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/7458014/diff/44013/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:110: #endif On 2011/08/25 05:58:38, xji wrote: > are the changes here related to bidi/complex script? I fixed the ui::VKEY_HOME/END code below, which now checks GetRenderText()->GetTextDirection(). http://codereview.chromium.org/7458014/diff/44013/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:110: #endif On 2011/08/25 05:58:38, xji wrote: > are the changes here related to bidi/complex script? The TOUCH_UI check? That's from crrev.com/97424, not part of this change. I fixed the ui::VKEY_HOME/END code below, which now checks GetRenderText()->GetTextDirection().
http://codereview.chromium.org/7458014/diff/53004/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/7458014/diff/53004/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:132: DCHECK_GT(cursor, 0U); DCHECK_GE() http://codereview.chromium.org/7458014/diff/53004/views/controls/textfield/na... File views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/7458014/diff/53004/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:302: if (context_menu_runner_->RunMenuAt( which bug does this address? http://codereview.chromium.org/7458014/diff/53004/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:879: (GetRenderText()->GetTextDirection() == base::i18n::RIGHT_TO_LEFT)) thanks! http://codereview.chromium.org/7458014/diff/53004/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:1008: gfx::Point end(end_cursor.x(), end_cursor.bottom() - 1); is it the right change for touch ui's selection handle? should the handle starts from bottom() - 1 (overlap with cursor)?
Hey Steve and Chris, can you review Uniscribe use in ui/gfx/render_text_win.cc or alert a proper reviewer? I noted you've worked on Skia's src/ports/SkFontHost_win.cpp. http://codereview.chromium.org/7458014/diff/53004/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/7458014/diff/53004/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:132: DCHECK_GT(cursor, 0U); On 2011/08/26 17:59:26, xji wrote: > DCHECK_GE() Done. http://codereview.chromium.org/7458014/diff/53004/views/controls/textfield/na... File views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/7458014/diff/53004/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:302: if (context_menu_runner_->RunMenuAt( On 2011/08/26 17:59:26, xji wrote: > which bug does this address? The change you see here between patchsets is not part of my CL, it's from another author/change and is on mainline. You'll see a difference here only because I synced and merged with the latest version of this file between patch sets. http://codereview.chromium.org/7458014/diff/53004/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:879: (GetRenderText()->GetTextDirection() == base::i18n::RIGHT_TO_LEFT)) On 2011/08/26 17:59:26, xji wrote: > thanks! :) http://codereview.chromium.org/7458014/diff/53004/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:1008: gfx::Point end(end_cursor.x(), end_cursor.bottom() - 1); On 2011/08/26 17:59:26, xji wrote: > is it the right change for touch ui's selection handle? should the handle starts > from bottom() - 1 (overlap with cursor)? See above, what you're seeing isn't part of my change.
lgtm. http://codereview.chromium.org/7458014/diff/53004/views/controls/textfield/na... File views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/7458014/diff/53004/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:302: if (context_menu_runner_->RunMenuAt( On 2011/08/26 20:10:43, msw wrote: > On 2011/08/26 17:59:26, xji wrote: > > which bug does this address? > > The change you see here between patchsets is not part of my CL, it's from > another author/change and is on mainline. You'll see a difference here only > because I synced and merged with the latest version of this file between patch > sets. got it. thanks for the explanation!
On 2011/08/26 20:10:43, msw wrote: > Hey Steve and Chris, can you review Uniscribe use in ui/gfx/render_text_win.cc > or alert a proper reviewer? I noted you've worked on Skia's > src/ports/SkFontHost_win.cpp. Chris will be able to comment better than I. +CC reed
If you can call SkCanvas::drawPosText instead of ScriptTextOut, then I think all of the rest of this will work fine even when we're GPU accelerated or we're recording into a PDF/XPS/etc device.
On 2011/08/26 21:24:19, reed1 wrote: > If you can call SkCanvas::drawPosText instead of ScriptTextOut, then I think all > of the rest of this will work fine even when we're GPU accelerated or we're > recording into a PDF/XPS/etc device. I'm calling SkCanvas::drawPosText instead of ScriptTextOut, but I have some TODOs about the font size translations. Please take a look as I think this is sufficient for now.
LGTM
Ping ctguil; I'm waiting on your optional Uniscribe review.
On 2011/08/29 23:34:22, msw wrote: > Ping ctguil; I'm waiting on your optional Uniscribe review. ctguil got stuck in NYC this weekend and is just now on a flight back. Since Mike is happy, I don't think you need to wait for Chris.
On 2011/08/29 23:37:51, vandebo wrote: > On 2011/08/29 23:34:22, msw wrote: > > Ping ctguil; I'm waiting on your optional Uniscribe review. > > ctguil got stuck in NYC this weekend and is just now on a flight back. Since > Mike is happy, I don't think you need to wait for Chris. Ok, thanks. I'll land late tonight, jic. Safe travels, Chris.
Change committed as 98785 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
