Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7511029:
Implement Pango RenderText for Linux. (Closed)
|
Created:
9 years, 4 months ago by xji Modified:
9 years, 3 months ago CC:
chromium-reviews, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionImplement Pango RenderText for Linux.
Follow the I18N recommendations for BiDi text editing.
BUG=90426
TEST=--use-pure-views text editing
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99987
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 46
Patch Set 6 : fix cursor bounds for RTL UI #
Total comments: 133
Patch Set 7 : address comments, fix cursor bounds in non-insert-mode #Patch Set 8 : address oshima's comment missed in last upload #Patch Set 9 : '' #Patch Set 10 : fix compilation error. using ICU functions for utf8/utf16 conversion #
Total comments: 46
Patch Set 11 : fix spell error, sync to latest #Patch Set 12 : address comments on patch 20, move SetupPango to pango_util #
Total comments: 12
Patch Set 13 : '' #Patch Set 14 : update comment about u_strToUTF8, exclude pango_util from win/mac #
Total comments: 8
Patch Set 15 : '' #Patch Set 16 : add dcheck and todo #
Total comments: 44
Patch Set 17 : '' #Patch Set 18 : '' #Patch Set 19 : ClearComposition() before SetText(). DCHECK composition_range_ is invalid in SetText() #Patch Set 20 : sync, fix couple of bugs #
Total comments: 31
Patch Set 21 : address comments #
Total comments: 15
Patch Set 22 : address comments #Patch Set 23 : fix render_text_unittest.DefaultStyle failure: init composition_range_ as invalid range #Patch Set 24 : sync #
Total comments: 8
Patch Set 25 : '' #
Total comments: 1
Messages
Total messages: 36 (0 generated)
PTAL. Known issues: 1. need add tests. 2. move cursor left/right by word is not implemented. 3. select by word (double click) is not working, for languages not using space as word separator and for complex scripts. should change it to use ICU break iterator. 4. DELETE a character (which is part of a grapheme in complex script) does not work, only the character is deleted. The whole grapheme should be deleted. Guess DELETE always use pos+1 as new cursor position, it should use the next of the next grapheme as new cursor position and delete the whole grapheme. 5. pointing to position outside of text does not work correctly. 6. cursor bounds in non-insert-mode is not completed implemented yet. What is the expected behavior if cursor is at run boundary? 7. cursor position (when at boundary run) is not preserved when switching tab. Probably need to expose SelectSelectionModel (besides SelectRange) to restore/reset SelectionModel so that cursor could draw visually correct when switching from one tab content to another.
looked at just styles. I'll leave msw for logical part. http://codereview.chromium.org/7511029/diff/13008/ui/gfx/gtk_util.h File ui/gfx/gtk_util.h (right): http://codereview.chromium.org/7511029/diff/13008/ui/gfx/gtk_util.h#newcode71 ui/gfx/gtk_util.h:71: class Font; move this before "class Rect" http://codereview.chromium.org/7511029/diff/13008/ui/gfx/gtk_util.h#newcode72 ui/gfx/gtk_util.h:72: void SetupPangoLayout(PangoLayout* layout, please add comment as this is now public API http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:614: } else { no need for else. (It's chromium style) Alternatively, you can do return GetSelectionStart() < GetCursorPosition() ? SelectionModel(...) : SelectionModel(...); whicherever you like. http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:16: #define U16_IS_TRAIL(c) (((c)&0xfffffc00)==0xdc00) space around operators http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:18: #define COLOR_16_TO_32_BIT(c) ((c)/0xff * 0xffff) If this is to convert ARGB to RGB, please name accordingly. (ARGB_TO_RGB, ARGB2RGB etc) http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:51: CanvasSkia* canvas_skia = static_cast<CanvasSkia*>(canvas); canvas->AsCanvasSkia() http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:55: // TODO(xji): is this Begin/EndPlatformPaint correct? How about performance? This seems to be fine (assuming this class replaces DrawStringContet) http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:128: x += display_rect().x() + GetUpdatedDisplayOffset().x(); these can be int x = (caret_placement == SelectionModel::TRAILING ? ... : ...) + display_rect().x() + GetUpdatedDisplayOffset().x() http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:146: return GetSelectionModelForVisualLeftmost(); indent http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:169: // TODO(xji): implementation. can you update the comment to be more descriptive? http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:274: // TODO(xji): how costly those g_free is? negligible compared to other stuff we're doing. http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:288: size_t utf16_index = Utf8IndexToUtf16Index(text(), run->offset); indnet http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:297: text(), run->offset + run->length); ditto http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:306: return LastSelectionModelInsideRun(run); can these be return run->analysis.level % 2 == 0 ? .. : ..; http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:357: // TODO(xji): duplication with GetRightSelectionModelByGrapheme. do you mean you will consolidate two? http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:382: } nuke {} http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:400: } ditto http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:403: // == NumOfCharsInGrapheme(caret_pos); is this comment or dead code? http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:520: // pango_layout_line_unref(layout_line_); you may want to run test with valgrind bots. (linux_chromeos_valgrind) http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:595: size_t index) const { so there is no library which does this? http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.h File ui/gfx/render_text_linux.h (right): http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:11: #include <pango/pango.h> move this above chrome includes http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:20: // Overridden from RenderText http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:24: } please move all virtuals to .cc and OVERRIDE
I tried not to overlap with oshima's comments on the previous patch set, but I probably did. Here are my major concerns: Can you pull someone more knowledgable about UTF8/16 index positioning in to review that code and its clients? I don't feel qualified to review it, and I'd hope that Pango or Harfbuzz or something else already does this. Are you able to remove some of the GTK structures and functions being used in this code? Oshima knows better, but I think that needs to be removed as part of the go/nogtk effort. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/canvas_skia_linux.cc File ui/gfx/canvas_skia_linux.cc (left): http://codereview.chromium.org/7511029/diff/15002/ui/gfx/canvas_skia_linux.cc... ui/gfx/canvas_skia_linux.cc:152: pango_layout_set_width(layout, -1); You removed this line amidst the function move, any reason? http://codereview.chromium.org/7511029/diff/15002/ui/gfx/canvas_skia_linux.cc File ui/gfx/canvas_skia_linux.cc (right): http://codereview.chromium.org/7511029/diff/15002/ui/gfx/canvas_skia_linux.cc... ui/gfx/canvas_skia_linux.cc:351: // that the text is only on one line. Can you clarify (or at least complete) the second comment sentence here? http://codereview.chromium.org/7511029/diff/15002/ui/gfx/gtk_util.h File ui/gfx/gtk_util.h (right): http://codereview.chromium.org/7511029/diff/15002/ui/gfx/gtk_util.h#newcode72 ui/gfx/gtk_util.h:72: void SetupPangoLayout(PangoLayout* layout, Maybe move this up with GetPangoContext and GetPangoResolution, move the definition alongside their definitions too. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:128: // selection_start_'s previous grapheme. So, we do not need extra information Is that true? Isn't it always leading the selection start? What does the selection range of "abc[DE]F" look like? I don't think there's any component of that selection that trails 'c', it's just "abcF[ED]". Can you show an example of when MoveCursorLeft/Right should move the cursor&caret to trail the selection start's previous grapheme? http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:15: #define U16_IS_LEAD(c) (((c)&0xfffffc00)==0xd800) Make these functions instead of pre-processor directives, add comments. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:20: // TODO(xji): index saved in upper layer are utf16 index. Pango uses utf8 index. That's unfortunate, I believe Uniscribe is UTF16, can you help me make sure? http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:104: selection_end = g_utf8_offset_to_pointer(layout_text + caret_pos, trailing) Yuck! GTK? We're trying to remove all GTK; is there another way to accomplish this? Otherwise can everything after = fit on the next line? that'd look better; otherwise make this two statements (the second being "selection_end -= layout_text;") http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:119: pango_layout_index_to_pos( why not just call pango_layout_index_to_pos on selection.selection_end() if !insert_mode? http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:129: if (pango_layout_get_alignment(layout_) == PANGO_ALIGN_RIGHT) Can you comment on why this is offsetting by one? http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:135: int h = std::min(display_rect().height(), font.GetHeight()); Is the PangoRectangle's height inaccurate? Otherwise you should use the font at that particular point in the string (not the default font), or add a TODO for that later. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:136: Rect bounds(x, (display_rect().height() - h) / 2, 1, h); It looks like a width of 0 is okay for rendering a vertical line (at least for now) if you'd prefer that. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:146: const SelectionModel& current, BreakType break_type) { Put each argument on its own line if the whole signature doesn't fit on a single line. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:154: return RenderText::GetLeftSelectionModel(current, break_type); Sigh, word break is breaking our backs. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:158: const SelectionModel& current, BreakType break_type) { Ditto for arguments on separate lines. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:162: const char* layout_text = pango_layout_get_text(layout_); FYI, You might find it easier to implement something like RenderTextWin::RightEndSelectionModel. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:164: pango_find_base_dir(layout_text, strlen(layout_text)); Maybe add a TODO like my // TODO(msw): Implement RenderText::GetTextDirection for platforms. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:169: text().length(), text().length(), SelectionModel::LEADING); Perhaps we should discuss the merits of using text().length() for the caret position. I found it easier to avoid that except for empty strings. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:183: const char* layout_text = pango_layout_get_text(layout_); Do you need to EnsureLayout();? http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:191: reinterpret_cast<PangoLayoutRun*>(first_visual_run->data)->item; As far as I can tell, PangoLayoutRun != PangoItem: http://developer.gnome.org/pango/stable/pango-Layout-Objects.html#PangoLayoutRun typedef PangoGlyphItem PangoLayoutRun; http://developer.gnome.org/pango/stable/pango-Glyph-Storage.html#PangoGlyphItem typedef struct { PangoItem *item; PangoGlyphString *glyphs; } PangoGlyphItem; http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:198: text(), item->offset + item->length); Should this be item->length - 1? Or I guess getting the Utf16IndexOfAdjacentGrapheme of the PREVIOUS grapheme will fix that? You can put "text()," on the line above. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:201: text().length(), utf16_index, SelectionModel::TRAILING); Ditto for moving "text().length()," to the line above. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:216: PangoItem* pango_item = reinterpret_cast<PangoLayoutRun*>(run->data)->item; Same question about PangoLayoutRun and PangoItem casting. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:235: selection_end == run_end_utf16_index) { Why are we checking selection_end instead of caret_pos here? http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:247: size_t utf16_index_of_current_grapheme, RelativeLogicalPosition pos) const { Ditto for argument lines. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:255: int char_offset = static_cast<int>(g_utf8_pointer_to_offset(layout_text, ch)); Yuck, more GTK! http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:260: ch = g_utf8_find_prev_char(layout_text, ch); Yuck, more GTK! http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:269: ch = g_utf8_find_next_char(ch, NULL); Yuck, more GTK! http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:272: if (ch >= layout_text + strlen(layout_text)) Could |ch| be null here (noting your check above). http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:279: g_free(log_attrs); Yuck, more GTK! http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:284: size_t utf16_index_of_current_grapheme, RelativeLogicalPosition pos) const { Ditto for argument lines. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:292: size_t utf16_index = Utf8IndexToUtf16Index(text(), run->offset); Out-dent the function contents two spaces. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:293: return SelectionModel(Utf16IndexOfAdjacentGrapheme(utf16_index, NEXT), Can you split this up onto two lines like you did in LastSelectionModelInsideRun, that looks nicer. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:300: size_t utf16_index = Utf8IndexToUtf16Index( Out-dent the function contents two spaces. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:321: const SelectionModel& current, Out-dent these argument lines two spaces. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:332: *adjacent = RightmostSelectionModelInsideRun( Does this work in a RTL context with plain RTL text? http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:333: reinterpret_cast<PangoLayoutRun*>(last_run->data)->item); Same question about PangoLayoutRun and PangoItem casting. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:345: // (caret_pos, caret_placement, selection_end) when moving cursor left by While I can't thank you enough to writing the psuedocode for your visual cursor movement algorithm (which I also based my code on), it would be clearer if your notation for the (caret_pos, caret_placement, selection_end) triplet matched the ordering of the SelectionModel ctor: (selection_end, caret_pos, caret_placement). Also, you should make the punctuation at these comment line ends consistent. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:354: // (n, t) --> goto across run case if n is at run boundar; boundar*y* http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:355: // If n is at run boudary, get its visually left run, bou*n*dary http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:375: PangoItem* box = reinterpret_cast<PangoLayoutRun*>(run->data)->item; Same question about PangoLayoutRun and PangoItem casting. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:385: reinterpret_cast<PangoLayoutRun*>(prev_run->data)->item); Same question about PangoLayoutRun and PangoItem casting. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:403: reinterpret_cast<PangoLayoutRun*>(prev_run->data)->item); Same question about PangoLayoutRun and PangoItem casting. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:419: // Assume caret_pos in |current| is n, 'l' represents leading in Ditto on triplet order and line endings. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:431: // (n, l) --> goto across run case if n is at run boundar; boundar*y* http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:432: // If n is at run boudary, get its visually right run, bou*n*dary http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:450: PangoItem* box = reinterpret_cast<PangoLayoutRun*>(run->data)->item; Same question about PangoLayoutRun and PangoItem casting. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:460: reinterpret_cast<PangoLayoutRun*>(run->next->data)->item); Same question about PangoLayoutRun and PangoItem casting. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:478: reinterpret_cast<PangoLayoutRun*>(run->next->data)->item); Same question about PangoLayoutRun and PangoItem casting. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:517: // operation that trigger ResetLayout(). Yeah, we might want to make a more general shared Invalidate/ResetLayout + Layout/EnsureLayout function pair... We can consider that in a followup. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:519: g_object_unref(layout_); Yuck, more GTK! http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:524: // pango_layout_line_unref(layout_line_); Sounds like you should ref and unref it to retain it during a valid layout timeframe. http://developer.gnome.org/pango/stable/pango-Layout-Objects.html#pango-layou... Returns : the requested PangoLayoutLine, or NULL if the index is out of range. This layout line can be ref'ed and retained, but will become invalid if changes are made to the PangoLayout. No changes should be made to the line. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:535: COLOR_16_TO_32_BIT(SkColorGetR(selection_color)), Can you use SkColorGetR/G/B or similar instead here and below? http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:578: GSList* RenderTextLinux::GetPreviousRun(GSList* run) const { lol at singly linked lists :) http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:591: GSList* current = layout_line_->runs; i'm done loling, now i'm sighing :( http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:658: void RenderTextLinux::AdjustBoundsForNonInsertMode( I think this function should be in-lined in GetCursorBounds and you could probably simplify the resulting code. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:673: // cursor is at right of 'c'. In non-insert-mode, is the bounds around 'D'? I'd say yes, but I'm not sure if you're asking me :) http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h File ui/gfx/render_text_linux.h (right): http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:22: RenderText::SetText(text); All of these definitions are simple, but non-trivial; move them to the cc. Also, you should remove spaces between the RenderText overrides and add a comment above them that says: "// Overridden from RenderText:" (I just did the same for my change) http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:61: virtual SelectionModel GetLeftSelectionModel(const SelectionModel& current, Also add "// Overridden from RenderText:" here and remove blank line 65. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:68: // Returns the SelectionModel for visual leftmost position in the line. There are going to be lots of opportunities for us to merge similar changes or at least use the same signatures for functions like this and RenderTextWin::Left/RightEndSelectionModel. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:80: // This END position does not belong to any run. Interesting, instead of using a caret at (text().length(), LEADING) I put it leading/trailing the left-/right-most grapheme, which made things easier for me. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:81: GSList* GetRunContainsCaretPos(const SelectionModel& current, Hopefully we'll be able to merge the signatures and uses of this with RenderTextWin::GetRunContainingPosition. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:98: // caret position inside (not at bounary of) the run. "not at *a* boundary" here and elsewhere http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:117: // when |sel| is the visually rightmost END position of line, set |adjacent| Wouldn't it be left-most for RTL text/UI? http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:120: // The value set in |adjacent| represnts a cursor/caret positin without a positi*o*n http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:136: // Creates, setup, and returns pango layout and pango layout line if layout_ "Create*, setup, and return*" to match the tense of 'setup'. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:140: // Unref pango layout (layout_) and pango layout line (pango_line_). Set them Use vertical bars to indicate members, and you can omit the friendly name or the member name, both is redundant. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:141: // to NULL. nds_. what is " nds_."? http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:156: // Returns last run in Layout_line_. "*the* last run" and use bars and un-capitalize "|layout_line_|". http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:159: size_t Utf16IndexToUtf8Index(const string16& text, size_t index) const; These two functions don't use any class members, and can be implemented as file static functions local to the code file.
http://codereview.chromium.org/7511029/diff/13008/ui/gfx/gtk_util.h File ui/gfx/gtk_util.h (right): http://codereview.chromium.org/7511029/diff/13008/ui/gfx/gtk_util.h#newcode71 ui/gfx/gtk_util.h:71: class Font; On 2011/08/19 18:44:07, oshima wrote: > move this before "class Rect" Done. http://codereview.chromium.org/7511029/diff/13008/ui/gfx/gtk_util.h#newcode72 ui/gfx/gtk_util.h:72: void SetupPangoLayout(PangoLayout* layout, On 2011/08/19 18:44:07, oshima wrote: > please add comment as this is now public API Done. http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:614: } else { On 2011/08/19 18:44:07, oshima wrote: > no need for else. (It's chromium style) > Alternatively, you can do > return GetSelectionStart() < GetCursorPosition() ? > SelectionModel(...) : > SelectionModel(...); > > whicherever you like. removed else. http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:16: #define U16_IS_TRAIL(c) (((c)&0xfffffc00)==0xdc00) On 2011/08/19 18:44:07, oshima wrote: > space around operators these are actually not needed. defined in include files included in uchar.h http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:18: #define COLOR_16_TO_32_BIT(c) ((c)/0xff * 0xffff) On 2011/08/19 18:44:07, oshima wrote: > If this is to convert ARGB to RGB, please name accordingly. (ARGB_TO_RGB, > ARGB2RGB etc) It is just convert R/G/B from 16 bit to 32 bit. but since we are useing ARGB, but pango uses only RGB, we should do A massage to be more accurate. Changed the description of TODO. http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:51: CanvasSkia* canvas_skia = static_cast<CanvasSkia*>(canvas); On 2011/08/19 18:44:07, oshima wrote: > canvas->AsCanvasSkia() Done. http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:55: // TODO(xji): is this Begin/EndPlatformPaint correct? How about performance? On 2011/08/19 18:44:07, oshima wrote: > This seems to be fine (assuming this class replaces DrawStringContet) comments removed. http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:128: x += display_rect().x() + GetUpdatedDisplayOffset().x(); On 2011/08/19 18:44:07, oshima wrote: > these can be > > int x = (caret_placement == SelectionModel::TRAILING ? ... : ...) + > display_rect().x() + GetUpdatedDisplayOffset().x() I'd prefer the old way. moderate use of tertiary is ok. but mixed with other operation makes the code not much readable. Also, this piece code changed by taking care of RTL locale and non-insert-mode (but still in simple assignment) http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:146: return GetSelectionModelForVisualLeftmost(); On 2011/08/19 18:44:07, oshima wrote: > indent Done. http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:169: // TODO(xji): implementation. On 2011/08/19 18:44:07, oshima wrote: > can you update the comment to be more descriptive? changed to "not implemented yet". http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:274: // TODO(xji): how costly those g_free is? On 2011/08/19 18:44:07, oshima wrote: > negligible compared to other stuff we're doing. comment removed. http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:288: size_t utf16_index = Utf8IndexToUtf16Index(text(), run->offset); On 2011/08/19 18:44:07, oshima wrote: > indnet Done. http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:297: text(), run->offset + run->length); On 2011/08/19 18:44:07, oshima wrote: > ditto Done. http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:306: return LastSelectionModelInsideRun(run); On 2011/08/19 18:44:07, oshima wrote: > can these be > return run->analysis.level % 2 == 0 ? .. : ..; Done. http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:357: // TODO(xji): duplication with GetRightSelectionModelByGrapheme. On 2011/08/19 18:44:07, oshima wrote: > do you mean you will consolidate two? I am not sure whether to consolidate them. Although the code is duplicated. but consolidate will introduce several small function calls or make the code not very readable. http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:382: } On 2011/08/19 18:44:07, oshima wrote: > nuke {} Done. http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:400: } On 2011/08/19 18:44:07, oshima wrote: > ditto Done. http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:403: // == NumOfCharsInGrapheme(caret_pos); On 2011/08/19 18:44:07, oshima wrote: > is this comment or dead code? it is comment, changed "==" to "equals to" http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:520: // pango_layout_line_unref(layout_line_); On 2011/08/19 18:44:07, oshima wrote: > you may want to run test with valgrind bots. (linux_chromeos_valgrind) submitted. http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:595: size_t index) const { On 2011/08/19 18:44:07, oshima wrote: > so there is no library which does this? there is no ICU library does that directly without doing the actual conversion. http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.h File ui/gfx/render_text_linux.h (right): http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:11: #include <pango/pango.h> On 2011/08/19 18:44:07, oshima wrote: > move this above chrome includes Done. http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:20: On 2011/08/19 18:44:07, oshima wrote: > // Overridden from RenderText Done. http://codereview.chromium.org/7511029/diff/13008/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:24: } On 2011/08/19 18:44:07, oshima wrote: > please move all virtuals to .cc > and OVERRIDE Done.
On 2011/08/19 23:16:59, msw wrote: > I tried not to overlap with oshima's comments on the previous patch set, but I > probably did. > > Here are my major concerns: > > Can you pull someone more knowledgable about UTF8/16 index positioning in to > review that code and its clients? I don't feel qualified to review it, and I'd > hope that Pango or Harfbuzz or something else already does this. > > Are you able to remove some of the GTK structures and functions being used in > this code? Oshima knows better, but I think that needs to be removed as part of > the go/nogtk effort. oshima, are we also removing those g_ functions, includes: g_utf8_offset_to_pointer, g_utf8_pointer_to_offset, g_utf8_find_prev_char, g_utf8_find_next_char, g_object_unref, g_free
On Fri, Aug 19, 2011 at 5:58 PM, <xji@chromium.org> wrote: > On 2011/08/19 23:16:59, msw wrote: > >> I tried not to overlap with oshima's comments on the previous patch set, >> but I >> probably did. >> > > Here are my major concerns: >> > > Can you pull someone more knowledgable about UTF8/16 index positioning in >> to >> review that code and its clients? I don't feel qualified to review it, and >> I'd >> hope that Pango or Harfbuzz or something else already does this. >> > > Are you able to remove some of the GTK structures and functions being used >> in >> this code? Oshima knows better, but I think that needs to be removed as >> part >> > of > >> the go/nogtk effort. >> > > oshima, are we also removing those g_ functions, includes: > g_utf8_offset_to_pointer, > g_utf8_pointer_to_offset, > g_utf8_find_prev_char, > g_utf8_find_next_char, > g_object_unref, > g_free > > We have no plan to remove glib (there are other components that uses glib), so they're fine. - oshima > > > http://codereview.chromium.**org/7511029/<http://codereview.chromium.org/7511... >
Thanks for the review and great feedback. I will ask a co-worker to check the utf8/utf16 conversion. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/canvas_skia_linux.cc File ui/gfx/canvas_skia_linux.cc (left): http://codereview.chromium.org/7511029/diff/15002/ui/gfx/canvas_skia_linux.cc... ui/gfx/canvas_skia_linux.cc:152: pango_layout_set_width(layout, -1); On 2011/08/19 23:16:59, msw wrote: > You removed this line amidst the function move, any reason? The condition checking was changed from base:i18n:isRTL() to text_direction in r91881 http://src.chromium.org/viewvc/chrome/trunk/src/ui/gfx/canvas_skia_linux.cc?r... I thought it was setting it wrong and I changed it to use base:i18n:isRTL() again. (if you look down after this code in the removed code, you will see the width is set as -1 when base:i18n:isRTL(). But reading r91881 and r87431 again, I think I probably was wrong. without setting pango layout width, I changed the code to use ViewPort to TextPort conversion you added in RenderTextWin (thanks!) I put the 2 conversion functions in RenderText. To be more accurately, I should use layout's alignment (instead of base:i18n::isRTL), but since we align right only when isRTL() is true and I think it is less likely to change. It should be ok to have the 2 functions as common API. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/canvas_skia_linux.cc File ui/gfx/canvas_skia_linux.cc (right): http://codereview.chromium.org/7511029/diff/15002/ui/gfx/canvas_skia_linux.cc... ui/gfx/canvas_skia_linux.cc:351: // that the text is only on one line. On 2011/08/19 23:16:59, msw wrote: > Can you clarify (or at least complete) the second comment sentence here? the comments was added by davemoore in http://codereview.chromium.org/7105001/diff/1/ui/gfx/canvas_skia_linux.cc. Checked with him and changed the comment as "Ensure that the text...." http://codereview.chromium.org/7511029/diff/15002/ui/gfx/gtk_util.h File ui/gfx/gtk_util.h (right): http://codereview.chromium.org/7511029/diff/15002/ui/gfx/gtk_util.h#newcode72 ui/gfx/gtk_util.h:72: void SetupPangoLayout(PangoLayout* layout, On 2011/08/19 23:16:59, msw wrote: > Maybe move this up with GetPangoContext and GetPangoResolution, move the > definition alongside their definitions too. done. but the definition depends on skia_util.h and canvas.h, which I am not sure whether gtk_util should depend on. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:128: // selection_start_'s previous grapheme. So, we do not need extra information On 2011/08/19 23:16:59, msw wrote: > Is that true? Isn't it always leading the selection start? > What does the selection range of "abc[DE]F" look like? I don't think there's any > component of that selection that trails 'c', it's just "abcF[ED]". > > Can you show an example of when MoveCursorLeft/Right should move the > cursor&caret to trail the selection start's previous grapheme? The above example wont show any difference. But the difference shows up when you select "abc[FE]D". logically, the selection range is the same no matter you select from left to right or right to left. For the above example, the selection range is (6, 4) or (4, 6). But the visual starting point is different between select from right to left and from left to right. If you click right half of 'E' and move mouse left, the selection visually start from (4, leading). If you click left half of 'F' and move mouse right, the selection visually start from (5, trailing), but (6, leading). The difference probably wont affect the selection's substring bounds. But it affects when moving cursor left or right when there is selection, in which case, the selection should be collapsed to visual left or right. For the above example, when selection range (6, 4), selection_start is at visually left when using (5, trailing) as visual start point. But selection_end is at visually left when using (6, 0) as visual start point, which is wrong. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:15: #define U16_IS_LEAD(c) (((c)&0xfffffc00)==0xd800) On 2011/08/19 23:16:59, msw wrote: > Make these functions instead of pre-processor directives, add comments. removed. already defined in include in uchar.h http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:20: // TODO(xji): index saved in upper layer are utf16 index. Pango uses utf8 index. On 2011/08/19 23:16:59, msw wrote: > That's unfortunate, I believe Uniscribe is UTF16, can you help me make sure? I looked at those Uniscibe functions you used. In the documentation, it uses both "characters" and "code points". The meaning of "characters" is pretty ambiguous. but since Windows string is WCHAR, I think the index of characters of the IN/OUT parameters are most likely index of UTF16. If they use "code points", it should mean a UTF-32 code point. A surrogate pair consists of 2 UTF16, but is one code point. The trailing OUT parameter of ScriptXtoCP is number of code point. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:104: selection_end = g_utf8_offset_to_pointer(layout_text + caret_pos, trailing) On 2011/08/19 23:16:59, msw wrote: > Yuck! GTK? We're trying to remove all GTK; is there another way to accomplish > this? Otherwise can everything after = fit on the next line? that'd look better; > otherwise make this two statements (the second being "selection_end -= > layout_text;") those from glib or glib-object are fine from oshima. done the formatting. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:119: pango_layout_index_to_pos( On 2011/08/19 23:16:59, msw wrote: > why not just call pango_layout_index_to_pos on selection.selection_end() if > !insert_mode? Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:129: if (pango_layout_get_alignment(layout_) == PANGO_ALIGN_RIGHT) On 2011/08/19 23:16:59, msw wrote: > Can you comment on why this is offsetting by one? Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:135: int h = std::min(display_rect().height(), font.GetHeight()); On 2011/08/19 23:16:59, msw wrote: > Is the PangoRectangle's height inaccurate? Otherwise you should use the font at > that particular point in the string (not the default font), or add a TODO for > that later. Ah, yes, you are right. I can use pos.height. I did not find pos.height is different from default font's height for simple English, Chinese, and Hebrew. But guess that should be more accurate if Pango choose different font to display a character. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:136: Rect bounds(x, (display_rect().height() - h) / 2, 1, h); On 2011/08/19 23:16:59, msw wrote: > It looks like a width of 0 is okay for rendering a vertical line (at least for > now) if you'd prefer that. Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:146: const SelectionModel& current, BreakType break_type) { On 2011/08/19 23:16:59, msw wrote: > Put each argument on its own line if the whole signature doesn't fit on a single > line. Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:158: const SelectionModel& current, BreakType break_type) { On 2011/08/19 23:16:59, msw wrote: > Ditto for arguments on separate lines. Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:162: const char* layout_text = pango_layout_get_text(layout_); On 2011/08/19 23:16:59, msw wrote: > FYI, You might find it easier to implement something like > RenderTextWin::RightEndSelectionModel. Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:164: pango_find_base_dir(layout_text, strlen(layout_text)); On 2011/08/19 23:16:59, msw wrote: > Maybe add a TODO like my // TODO(msw): Implement RenderText::GetTextDirection > for platforms. I've changed it to use GetTextDirection(). it returns base:i18n:isRTL() in RenderText http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:169: text().length(), text().length(), SelectionModel::LEADING); On 2011/08/19 23:16:59, msw wrote: > Perhaps we should discuss the merits of using text().length() for the caret > position. I found it easier to avoid that except for empty strings. I've changed to your implementation. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:183: const char* layout_text = pango_layout_get_text(layout_); On 2011/08/19 23:16:59, msw wrote: > Do you need to EnsureLayout();? this one is a private function, and it is always called inside GetLeft/RightSelectionModel(), in which, ensureLayout() is done at the beginning. Or maybe I should consider race condition. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:191: reinterpret_cast<PangoLayoutRun*>(first_visual_run->data)->item; On 2011/08/19 23:16:59, msw wrote: > As far as I can tell, PangoLayoutRun != PangoItem: > > http://developer.gnome.org/pango/stable/pango-Layout-Objects.html#PangoLayoutRun > > typedef PangoGlyphItem PangoLayoutRun; > > http://developer.gnome.org/pango/stable/pango-Glyph-Storage.html#PangoGlyphItem > > typedef struct { > PangoItem *item; > PangoGlyphString *glyphs; > } PangoGlyphItem; |item| is assigned to PangoLayoutRun->item. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:198: text(), item->offset + item->length); On 2011/08/19 23:16:59, msw wrote: > Should this be item->length - 1? Or I guess getting the > Utf16IndexOfAdjacentGrapheme of the PREVIOUS grapheme will fix that? > Yes. > You can put "text()," on the line above. done. I am a bit confused on the style regarding to the format of parameters in function call and function definition/declaration. I was trying to put the argument list in one line if the whole statement does not fit in one line. If I put the first argument along with function name, do you need to put each argument in separate line? If there is enough space, should I put more argument in 1st line? http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:201: text().length(), utf16_index, SelectionModel::TRAILING); On 2011/08/19 23:16:59, msw wrote: > Ditto for moving "text().length()," to the line above. Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:216: PangoItem* pango_item = reinterpret_cast<PangoLayoutRun*>(run->data)->item; On 2011/08/19 23:16:59, msw wrote: > Same question about PangoLayoutRun and PangoItem casting. ditto http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:235: selection_end == run_end_utf16_index) { On 2011/08/19 23:16:59, msw wrote: > Why are we checking selection_end instead of caret_pos here? I am checking selecion_end == run_end, you are checking caret_pos = run_end - 1. Without complex script, they are the same. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:247: size_t utf16_index_of_current_grapheme, RelativeLogicalPosition pos) const { On 2011/08/19 23:16:59, msw wrote: > Ditto for argument lines. Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:272: if (ch >= layout_text + strlen(layout_text)) On 2011/08/19 23:16:59, msw wrote: > Could |ch| be null here (noting your check above). pango text should be null terminated. I do not think |ch| could be null here. but for safety, I added the condition as if (!ch ||...) http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:284: size_t utf16_index_of_current_grapheme, RelativeLogicalPosition pos) const { On 2011/08/19 23:16:59, msw wrote: > Ditto for argument lines. Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:292: size_t utf16_index = Utf8IndexToUtf16Index(text(), run->offset); On 2011/08/19 23:16:59, msw wrote: > Out-dent the function contents two spaces. Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:293: return SelectionModel(Utf16IndexOfAdjacentGrapheme(utf16_index, NEXT), On 2011/08/19 23:16:59, msw wrote: > Can you split this up onto two lines like you did in > LastSelectionModelInsideRun, that looks nicer. Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:300: size_t utf16_index = Utf8IndexToUtf16Index( On 2011/08/19 23:16:59, msw wrote: > Out-dent the function contents two spaces. Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:321: const SelectionModel& current, On 2011/08/19 23:16:59, msw wrote: > Out-dent these argument lines two spaces. Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:332: *adjacent = RightmostSelectionModelInsideRun( On 2011/08/19 23:16:59, msw wrote: > Does this work in a RTL context with plain RTL text? it works. but this function is no longer needed. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:345: // (caret_pos, caret_placement, selection_end) when moving cursor left by On 2011/08/19 23:16:59, msw wrote: > While I can't thank you enough to writing the psuedocode for your visual cursor > movement algorithm (which I also based my code on), it would be clearer if your > notation for the (caret_pos, caret_placement, selection_end) triplet matched the > ordering of the SelectionModel ctor: (selection_end, caret_pos, > caret_placement). Also, you should make the punctuation at these comment line > ends consistent. Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:354: // (n, t) --> goto across run case if n is at run boundar; On 2011/08/19 23:16:59, msw wrote: > boundar*y* Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:355: // If n is at run boudary, get its visually left run, On 2011/08/19 23:16:59, msw wrote: > bou*n*dary Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:419: // Assume caret_pos in |current| is n, 'l' represents leading in On 2011/08/19 23:16:59, msw wrote: > Ditto on triplet order and line endings. Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:431: // (n, l) --> goto across run case if n is at run boundar; On 2011/08/19 23:16:59, msw wrote: > boundar*y* Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:432: // If n is at run boudary, get its visually right run, On 2011/08/19 23:16:59, msw wrote: > bou*n*dary Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:524: // pango_layout_line_unref(layout_line_); On 2011/08/19 23:16:59, msw wrote: > Sounds like you should ref and unref it to retain it during a valid layout > timeframe. > > http://developer.gnome.org/pango/stable/pango-Layout-Objects.html#pango-layou... > Returns : > the requested PangoLayoutLine, or NULL if the index is out of range. This layout > line can be ref'ed and retained, but will become invalid if changes are made to > the PangoLayout. No changes should be made to the line. > Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:535: COLOR_16_TO_32_BIT(SkColorGetR(selection_color)), On 2011/08/19 23:16:59, msw wrote: > Can you use SkColorGetR/G/B or similar instead here and below? I am using SKColorGetR/G/B, but I need to convert the color from 8-bit to 16-bit, otherwise, pango wont draw correctly. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:658: void RenderTextLinux::AdjustBoundsForNonInsertMode( On 2011/08/19 23:16:59, msw wrote: > I think this function should be in-lined in GetCursorBounds and you could > probably simplify the resulting code. Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:673: // cursor is at right of 'c'. In non-insert-mode, is the bounds around 'D'? On 2011/08/19 23:16:59, msw wrote: > I'd say yes, but I'm not sure if you're asking me :) I implement as this. we can always change it if it is not expected. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h File ui/gfx/render_text_linux.h (right): http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:22: RenderText::SetText(text); On 2011/08/19 23:16:59, msw wrote: > All of these definitions are simple, but non-trivial; move them to the cc. > > Also, you should remove spaces between the RenderText overrides and add a > comment above them that says: "// Overridden from RenderText:" (I just did the > same for my change) Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:61: virtual SelectionModel GetLeftSelectionModel(const SelectionModel& current, On 2011/08/19 23:16:59, msw wrote: > Also add "// Overridden from RenderText:" here and remove blank line 65. Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:68: // Returns the SelectionModel for visual leftmost position in the line. On 2011/08/19 23:16:59, msw wrote: > There are going to be lots of opportunities for us to merge similar changes or > at least use the same signatures for functions like this and > RenderTextWin::Left/RightEndSelectionModel. Changed to the same names. But I can not declare them as "const" since they depends on TextDirection() which needs ensure layout_. I could make TextDirection() to check the first strong character's directionality (which I think is what pango does), but using pango_find_base_dir might be more accurate. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:80: // This END position does not belong to any run. On 2011/08/19 23:16:59, msw wrote: > Interesting, instead of using a caret at (text().length(), LEADING) I put it > leading/trailing the left-/right-most grapheme, which made things easier for me. I think you are right. I changed it to your way. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:81: GSList* GetRunContainsCaretPos(const SelectionModel& current, On 2011/08/19 23:16:59, msw wrote: > Hopefully we'll be able to merge the signatures and uses of this with > RenderTextWin::GetRunContainingPosition. I changed the implementation as yours. But I am returning a GSList*. Unless I have a local mapping of the single-linked list, return run's index is not helping. Also, I changed the usage (implementation of Left/RightSelectionModel) largely based on yours. It simplified the code a lot. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:98: // caret position inside (not at bounary of) the run. On 2011/08/19 23:16:59, msw wrote: > "not at *a* boundary" here and elsewhere Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:117: // when |sel| is the visually rightmost END position of line, set |adjacent| On 2011/08/19 23:16:59, msw wrote: > Wouldn't it be left-most for RTL text/UI? this handles the right (visual) end (NOT the logical END in HOME/END), which is when text is LTR text. It is no longer needed. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:120: // The value set in |adjacent| represnts a cursor/caret positin without a On 2011/08/19 23:16:59, msw wrote: > positi*o*n Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:136: // Creates, setup, and returns pango layout and pango layout line if layout_ On 2011/08/19 23:16:59, msw wrote: > "Create*, setup, and return*" to match the tense of 'setup'. Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:140: // Unref pango layout (layout_) and pango layout line (pango_line_). Set them On 2011/08/19 23:16:59, msw wrote: > Use vertical bars to indicate members, and you can omit the friendly name or the > member name, both is redundant. Done. I thought we only use vertical bar for function parameter, but not data member. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:141: // to NULL. nds_. On 2011/08/19 23:16:59, msw wrote: > what is " nds_."? must have done extra editing accidently. removed. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:156: // Returns last run in Layout_line_. On 2011/08/19 23:16:59, msw wrote: > "*the* last run" and use bars and un-capitalize "|layout_line_|". Done. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:159: size_t Utf16IndexToUtf8Index(const string16& text, size_t index) const; On 2011/08/19 23:16:59, msw wrote: > These two functions don't use any class members, and can be implemented as file > static functions local to the code file. Done.
Cool, this is looking better. I'm not so keen on adding code to gtk_util.*. Oshima, can you comment, or xji, can we ping an OWNER? Please comment on and check logic when there is no PREVIOUS/NEXT grapheme. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/gtk_util.h File ui/gfx/gtk_util.h (right): http://codereview.chromium.org/7511029/diff/15002/ui/gfx/gtk_util.h#newcode72 ui/gfx/gtk_util.h:72: void SetupPangoLayout(PangoLayout* layout, On 2011/08/22 23:57:28, xji wrote: > On 2011/08/19 23:16:59, msw wrote: > > Maybe move this up with GetPangoContext and GetPangoResolution, move the > > definition alongside their definitions too. > > done. > but the definition depends on skia_util.h and canvas.h, which I am not sure > whether gtk_util should depend on. Yeah, I don't think it should either... let's consult with an OWNER. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:128: // selection_start_'s previous grapheme. So, we do not need extra information On 2011/08/22 23:57:28, xji wrote: > On 2011/08/19 23:16:59, msw wrote: > > Is that true? Isn't it always leading the selection start? > > What does the selection range of "abc[DE]F" look like? I don't think there's > any > > component of that selection that trails 'c', it's just "abcF[ED]". > > > > Can you show an example of when MoveCursorLeft/Right should move the > > cursor&caret to trail the selection start's previous grapheme? > > The above example wont show any difference. > But the difference shows up when you select "abc[FE]D". > > logically, the selection range is the same no matter you select from left to > right or right to left. For the above example, the selection range is (6, 4) or > (4, 6). > > But the visual starting point is different between select from right to left and > from left to right. > > If you click right half of 'E' and move mouse left, the selection visually start > from (4, leading). > If you click left half of 'F' and move mouse right, the selection visually start > from (5, trailing), but (6, leading). > > The difference probably wont affect the selection's substring bounds. > But it affects when moving cursor left or right when there is selection, in > which case, the selection should be collapsed to visual left or right. > For the above example, when selection range (6, 4), selection_start is at > visually left when using (5, trailing) as visual start point. But selection_end > is at visually left when using (6, 0) as visual start point, which is wrong. Ok, I think I understand and it seems that you are correct. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:135: int h = std::min(display_rect().height(), font.GetHeight()); On 2011/08/22 23:57:28, xji wrote: > On 2011/08/19 23:16:59, msw wrote: > > Is the PangoRectangle's height inaccurate? Otherwise you should use the font > at > > that particular point in the string (not the default font), or add a TODO for > > that later. > Ah, yes, you are right. I can use pos.height. > I did not find pos.height is different from default font's height for simple > English, Chinese, and Hebrew. But guess that should be more accurate if Pango > choose different font to display a character. > Good. FYI someone could set a different font/size for a substring of the RenderText with a StyleRange. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:183: const char* layout_text = pango_layout_get_text(layout_); On 2011/08/22 23:57:28, xji wrote: > On 2011/08/19 23:16:59, msw wrote: > > Do you need to EnsureLayout();? > > this one is a private function, and it is always called inside > GetLeft/RightSelectionModel(), in which, ensureLayout() is done at the > beginning. > Or maybe I should consider race condition. I think you're okay. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:191: reinterpret_cast<PangoLayoutRun*>(first_visual_run->data)->item; On 2011/08/22 23:57:28, xji wrote: > On 2011/08/19 23:16:59, msw wrote: > > As far as I can tell, PangoLayoutRun != PangoItem: > > > > > http://developer.gnome.org/pango/stable/pango-Layout-Objects.html#PangoLayoutRun > > > > typedef PangoGlyphItem PangoLayoutRun; > > > > > http://developer.gnome.org/pango/stable/pango-Glyph-Storage.html#PangoGlyphItem > > > > typedef struct { > > PangoItem *item; > > PangoGlyphString *glyphs; > > } PangoGlyphItem; > > > |item| is assigned to PangoLayoutRun->item. > Doh, thanks for clarifying! http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:198: text(), item->offset + item->length); On 2011/08/22 23:57:28, xji wrote: > On 2011/08/19 23:16:59, msw wrote: > > Should this be item->length - 1? Or I guess getting the > > Utf16IndexOfAdjacentGrapheme of the PREVIOUS grapheme will fix that? > > > Yes. > > > You can put "text()," on the line above. > > done. I am a bit confused on the style regarding to the format of parameters in > function call and function definition/declaration. I was trying to put the > argument list in one line if the whole statement does not fit in one line. If I > put the first argument along with function name, do you need to put each > argument in separate line? If there is enough space, should I put more argument > in 1st line? Function definition/declaration is all on one line, or each on their own line (and it's okay to include the first one on the line with the name, or on the next line if it won't fit). Function calls are mostly per dev preference, these just seemed a little odd. Check out https://sites.google.com/a/chromium.org/dev/developers/coding-style http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h File ui/gfx/render_text_linux.h (right): http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:68: // Returns the SelectionModel for visual leftmost position in the line. On 2011/08/22 23:57:28, xji wrote: > On 2011/08/19 23:16:59, msw wrote: > > There are going to be lots of opportunities for us to merge similar changes or > > at least use the same signatures for functions like this and > > RenderTextWin::Left/RightEndSelectionModel. > > Changed to the same names. But I can not declare them as "const" since they > depends on TextDirection() which needs ensure layout_. > > I could make TextDirection() to check the first strong character's > directionality (which I think is what pango does), but using pango_find_base_dir > might be more accurate. Sounds good. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:80: // This END position does not belong to any run. On 2011/08/22 23:57:28, xji wrote: > On 2011/08/19 23:16:59, msw wrote: > > Interesting, instead of using a caret at (text().length(), LEADING) I put it > > leading/trailing the left-/right-most grapheme, which made things easier for > me. > > I think you are right. I changed it to your way. :) http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:81: GSList* GetRunContainsCaretPos(const SelectionModel& current, On 2011/08/22 23:57:28, xji wrote: > On 2011/08/19 23:16:59, msw wrote: > > Hopefully we'll be able to merge the signatures and uses of this with > > RenderTextWin::GetRunContainingPosition. > > I changed the implementation as yours. But I am returning a GSList*. Unless I > have a local mapping of the single-linked list, return run's index is not > helping. > > Also, I changed the usage (implementation of Left/RightSelectionModel) largely > based on yours. It simplified the code a lot. :) I'm so sorry about the GList, it's a tragedy. http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:140: // Unref pango layout (layout_) and pango layout line (pango_line_). Set them On 2011/08/22 23:57:28, xji wrote: > On 2011/08/19 23:16:59, msw wrote: > > Use vertical bars to indicate members, and you can omit the friendly name or > the > > member name, both is redundant. > > Done. > I thought we only use vertical bar for function parameter, but not data member. Hmm, you might be right; I'm still learning chromium style. Feel free to undo that change. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/gtk_util.cc File ui/gfx/gtk_util.cc (right): http://codereview.chromium.org/7511029/diff/28004/ui/gfx/gtk_util.cc#newcode94 ui/gfx/gtk_util.cc:94: const gunichar kAcceleratorChar = '&'; I think constants are usually defined above all functions in cc files. Also, give it a comment. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/gtk_util.cc#newcode98 ui/gfx/gtk_util.cc:98: cairo_font_options_t* cairo_font_options = NULL; Can you make this a function static in UpdateCairoFontOptions rather than a global? You can have UpdateCairoFontOptions return the pointer or its value for SetupPangoLayout (and I suppose you should rename UpdateCairoFontOptions to GetCairoFontOptions or something). http://codereview.chromium.org/7511029/diff/28004/ui/gfx/gtk_util.cc#newcode166 ui/gfx/gtk_util.cc:166: } // namespace Add a blank line here. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/gtk_util.cc#newcode247 ui/gfx/gtk_util.cc:247: // Pass a width > 0 to force wrapping and elliding. remove the extra L from "eliding"; use the phrase "greater than" instead of '>'. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/gtk_util.h File ui/gfx/gtk_util.h (left): http://codereview.chromium.org/7511029/diff/28004/ui/gfx/gtk_util.h#oldcode68 ui/gfx/gtk_util.h:68: replace this line. (I think the common/proper style includes it) http://codereview.chromium.org/7511029/diff/28004/ui/gfx/gtk_util.h File ui/gfx/gtk_util.h (right): http://codereview.chromium.org/7511029/diff/28004/ui/gfx/gtk_util.h#newcode52 ui/gfx/gtk_util.h:52: void SetupPangoLayout(PangoLayout* layout, Hmm, I didn't suspect that moving the declaration up here would require you to move the definition from canvas_skia_linux.cc to gtk_util.cc. I think it's generally bad to add functions to *_util files, can all this pango stuff live in canvas_skia_linux.* or render_text_linux.*? I might be wrong here, please confirm with oshima or someone else. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:197: SelectionModel selection_start = GetSelectionModelForSelectionStart(); Remove the extra space after '='. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:624: // BTW, Firefox has the same issue. No need to call out Firefox here, we can alert them if they have a similar bug. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:645: GetIndexOfPreviousGrapheme(GetSelectionStart()), What if the selection start is 0 and there is not previous grapheme? http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:289: // TODO(xji): Remove the functions from RenderTextWin. You don't need this TODO, I'll take care of merging that. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:18: #define COLOR_16_TO_32_BIT(c) ((c)/0xff * 0xffff) Can you make this a file local function instead of a pre-processor directive? Also, I'd appreciate if someone else could double-check this logic. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:86: // TODO(xji): use ScopedPlatformPaing. ScopedPlatformPain*t* http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:157: int h = std::min(display_rect().height(), pos.height/PANGO_SCALE); spaces around '/' operator. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:323: // if left run is LTR run, Capitalize "If" http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:325: // If rght run is RTL run, Don't you mean left run? http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:381: // if right run is LTR run, Capitalize "If" http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:383: // If rght run is RTL run, r*i*ght http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:409: return SelectionModel(caret - 1, caret - 1, SelectionModel::LEADING); Do you mean to get Utf16IndexOfAdjacentGrapheme(caret, PREVIOUS) here? Also, here and everywhere else, I'm curious what happens if there is no PREVIOUS or NEXT grapheme. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:534: u_strToUTF8WithSub(NULL, 0, &utf8_index, text().data(), index, I'm worried by the potential performance losses incurred by calling this function and its corresponding 8->16 frequently. RenderTextLinux and RenderTextWin will both need some performance testing and tuning, but these calls might warrant explicit TODOs, and I'd like to see official review from someone that knows this code better. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:536: return utf8_index; I think you're supposed to check ec for U_FAILURE (same below). http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.h File ui/gfx/render_text_linux.h (right): http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:58: size_t Utf8IndexOfAdjacentGrapheme(size_t utf16_index_of_current_grapheme, Can you comment on (and ensure you're properly handling all the results) when there is no previous or next grapheme?
On 2011/08/23 08:01:01, msw wrote: > Cool, this is looking better. > > I'm not so keen on adding code to gtk_util.*. > Oshima, can you comment, or xji, can we ping an OWNER? > Sounds like we need pango_util. pango is independent from gtk and I'd like to minimize importing stuff from gtk (and remove eventually).
Oshima: looks like setup pango has a dependency on gtk, please check out http://codereview.chromium.org/7511029/diff/36002/ui/gfx/pango_util.cc?contex... at line 35. Since Markus is off today, I will ask Markus to review the utf8/16 conversion tomorrow. Behdad: the cursor model is at https://cs.corp.google.com/#chrome/src/ui/gfx/render_text.h&q=5E0%20file:^src... http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h File ui/gfx/render_text_linux.h (right): http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:140: // Unref pango layout (layout_) and pango layout line (pango_line_). Set them On 2011/08/23 08:01:01, msw wrote: > On 2011/08/22 23:57:28, xji wrote: > > On 2011/08/19 23:16:59, msw wrote: > > > Use vertical bars to indicate members, and you can omit the friendly name or > > the > > > member name, both is redundant. > > > > Done. > > I thought we only use vertical bar for function parameter, but not data > member. > > Hmm, you might be right; I'm still learning chromium style. Feel free to undo > that change. I checked with my co-worker, and seems there is no hard line on this. So, I will leave it there (using | to mean I am talking about a variable). http://codereview.chromium.org/7511029/diff/28004/ui/gfx/gtk_util.cc File ui/gfx/gtk_util.cc (right): http://codereview.chromium.org/7511029/diff/28004/ui/gfx/gtk_util.cc#newcode94 ui/gfx/gtk_util.cc:94: const gunichar kAcceleratorChar = '&'; On 2011/08/23 08:01:01, msw wrote: > I think constants are usually defined above all functions in cc files. > Also, give it a comment. done comment. moved those to pango_util http://codereview.chromium.org/7511029/diff/28004/ui/gfx/gtk_util.cc#newcode98 ui/gfx/gtk_util.cc:98: cairo_font_options_t* cairo_font_options = NULL; On 2011/08/23 08:01:01, msw wrote: > Can you make this a function static in UpdateCairoFontOptions rather than a > global? You can have UpdateCairoFontOptions return the pointer or its value for > SetupPangoLayout (and I suppose you should rename UpdateCairoFontOptions to > GetCairoFontOptions or something). this is defined in unnamed scope, so should be the same as file scoped static. UpdateCairoFontOptions creates cairo_font_options_t and set attributes on it, not only Get it. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/gtk_util.cc#newcode111 ui/gfx/gtk_util.cc:111: GtkSettings* gtk_settings = gtk_settings_get_default(); hm.. this has gtk dependency. Oshima, could you check whether this cause problem? http://codereview.chromium.org/7511029/diff/28004/ui/gfx/gtk_util.cc#newcode166 ui/gfx/gtk_util.cc:166: } // namespace On 2011/08/23 08:01:01, msw wrote: > Add a blank line here. Done. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/gtk_util.cc#newcode247 ui/gfx/gtk_util.cc:247: // Pass a width > 0 to force wrapping and elliding. On 2011/08/23 08:01:01, msw wrote: > remove the extra L from "eliding"; use the phrase "greater than" instead of '>'. Done. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/gtk_util.h File ui/gfx/gtk_util.h (right): http://codereview.chromium.org/7511029/diff/28004/ui/gfx/gtk_util.h#newcode52 ui/gfx/gtk_util.h:52: void SetupPangoLayout(PangoLayout* layout, On 2011/08/23 08:01:01, msw wrote: > Hmm, I didn't suspect that moving the declaration up here would require you to > move the definition from canvas_skia_linux.cc to gtk_util.cc. I think it's > generally bad to add functions to *_util files, can all this pango stuff live in > canvas_skia_linux.* or render_text_linux.*? I might be wrong here, please > confirm with oshima or someone else. Ah, I think I mis-interpreted your previous comments. I thought you'd like move the definition into gtk_util. I do not think it is a good idea either. And it would be better to declare that in canvas_skia.h and still define that in canvas_skia_linux.cc http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:197: SelectionModel selection_start = GetSelectionModelForSelectionStart(); On 2011/08/23 08:01:01, msw wrote: > Remove the extra space after '='. Done. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:624: // BTW, Firefox has the same issue. On 2011/08/23 08:01:01, msw wrote: > No need to call out Firefox here, we can alert them if they have a similar bug. Ah, you absolutely right. removed. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:645: GetIndexOfPreviousGrapheme(GetSelectionStart()), On 2011/08/23 08:01:01, msw wrote: > What if the selection start is 0 and there is not previous grapheme? this is called when selection is not empty and selection_start > cursor_pos, so selection_start should not be 0. I added comments in header file. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:289: // TODO(xji): Remove the functions from RenderTextWin. On 2011/08/23 08:01:01, msw wrote: > You don't need this TODO, I'll take care of merging that. removed. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:18: #define COLOR_16_TO_32_BIT(c) ((c)/0xff * 0xffff) On 2011/08/23 08:01:01, msw wrote: > Can you make this a file local function instead of a pre-processor directive? > Also, I'd appreciate if someone else could double-check this logic. changed to function in unnamed namespace. checked with Behdad. changed to "c * (0xffff / 0xff)". http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:86: // TODO(xji): use ScopedPlatformPaing. On 2011/08/23 08:01:01, msw wrote: > ScopedPlatformPain*t* Done. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:157: int h = std::min(display_rect().height(), pos.height/PANGO_SCALE); On 2011/08/23 08:01:01, msw wrote: > spaces around '/' operator. Done. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:323: // if left run is LTR run, On 2011/08/23 08:01:01, msw wrote: > Capitalize "If" Done. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:325: // If rght run is RTL run, On 2011/08/23 08:01:01, msw wrote: > Don't you mean left run? yes. changed. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:381: // if right run is LTR run, On 2011/08/23 08:01:01, msw wrote: > Capitalize "If" Done. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:383: // If rght run is RTL run, On 2011/08/23 08:01:01, msw wrote: > r*i*ght Done. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:409: return SelectionModel(caret - 1, caret - 1, SelectionModel::LEADING); On 2011/08/23 08:01:01, msw wrote: > Do you mean to get Utf16IndexOfAdjacentGrapheme(caret, PREVIOUS) here? YES! thanks for catching it. Also, > here and everywhere else, I'm curious what happens if there is no PREVIOUS or > NEXT grapheme. When there is no previous grapheme, the previous grapheme call returns 0. when there is no next grapheme, the next grapheme returns the string length. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:534: u_strToUTF8WithSub(NULL, 0, &utf8_index, text().data(), index, On 2011/08/23 08:01:01, msw wrote: > I'm worried by the potential performance losses incurred by calling this > function and its corresponding 8->16 frequently. RenderTextLinux and > RenderTextWin will both need some performance testing and tuning, but these > calls might warrant explicit TODOs, and I'd like to see official review from > someone that knows this code better. I have the same concern. I checked with Markus who is very familiar with the above function, and he said that when destination buffer is NULL and destination length is 0, the code has a quick path to get the index without doing real conversion. Since pango use utf8 internally, we have to do the conversion on selection_start/end. But I have a TODO at the begin of file saying that to remove the convresion on caret_pos. I will ask Markus to take a look. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:536: return utf8_index; On 2011/08/23 08:01:01, msw wrote: > I think you're supposed to check ec for U_FAILURE (same below). From doc "If the input string is not well-formed, then the U_INVALID_CHAR_FOUND error code is set.". I think our string should be well-formed. If it is well-formed, we could use u_strToUTF8, which exits when found a not well-formed character. To make it safe, I uses u_strToUTF8WithSub(), which convert the not well-formed character to substitute character. The return value means whether the input string contains not well-formed character or not. I do not think we need to care about that. Or if there is one, what should we do?
On Tue, Aug 23, 2011 at 4:52 PM, <xji@chromium.org> wrote: > Oshima: looks like setup pango has a dependency on gtk, please check out > http://codereview.chromium.**org/7511029/diff/36002/ui/gfx/** > pango_util.cc?context=10&**column_width=80<http://codereview.chromium.org/7511029/diff/36002/ui/gfx/pango_util.cc?context=10&column_width=80> > at line 35. > > My guess is that you need gtk if you want to use pango with gtk stuff. Looks like it works without gtk under wayland, no? - oshima > Since Markus is off today, I will ask Markus to review the utf8/16 > conversion > tomorrow. > > Behdad: the cursor model is at > https://cs.corp.google.com/#**chrome/src/ui/gfx/render_text.** > h&q=5E0%20file<https://cs.corp.google.com/#chrome/src/ui/gfx/render_text.h&q=5E0%20file> > :^src/&exact_**package=chrome&type=cs&l=90 > > > > http://codereview.chromium.**org/7511029/diff/15002/ui/gfx/** > render_text_linux.h<http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h> > File ui/gfx/render_text_linux.h (right): > > http://codereview.chromium.**org/7511029/diff/15002/ui/gfx/** > render_text_linux.h#newcode140<http://codereview.chromium.org/7511029/diff/15002/ui/gfx/render_text_linux.h#newcode140> > ui/gfx/render_text_linux.h:**140: // Unref pango layout (layout_) and > pango layout line (pango_line_). Set them > On 2011/08/23 08:01:01, msw wrote: > >> On 2011/08/22 23:57:28, xji wrote: >> > On 2011/08/19 23:16:59, msw wrote: >> > > Use vertical bars to indicate members, and you can omit the >> > friendly name or > >> > the >> > > member name, both is redundant. >> > >> > Done. >> > I thought we only use vertical bar for function parameter, but not >> > data > >> member. >> > > Hmm, you might be right; I'm still learning chromium style. Feel free >> > to undo > >> that change. >> > > I checked with my co-worker, and seems there is no hard line on this. > So, I will leave it there (using | to mean I am talking about a > variable). > > > http://codereview.chromium.**org/7511029/diff/28004/ui/gfx/**gtk_util.cc<http... > File ui/gfx/gtk_util.cc (right): > > http://codereview.chromium.**org/7511029/diff/28004/ui/gfx/** > gtk_util.cc#newcode94<http://codereview.chromium.org/7511029/diff/28004/ui/gfx/gtk_util.cc#newcode94> > ui/gfx/gtk_util.cc:94: const gunichar kAcceleratorChar = '&'; > On 2011/08/23 08:01:01, msw wrote: > >> I think constants are usually defined above all functions in cc files. >> Also, give it a comment. >> > > done comment. > moved those to pango_util > > > http://codereview.chromium.**org/7511029/diff/28004/ui/gfx/** > gtk_util.cc#newcode98<http://codereview.chromium.org/7511029/diff/28004/ui/gfx/gtk_util.cc#newcode98> > ui/gfx/gtk_util.cc:98: cairo_font_options_t* cairo_font_options = NULL; > On 2011/08/23 08:01:01, msw wrote: > >> Can you make this a function static in UpdateCairoFontOptions rather >> > than a > >> global? You can have UpdateCairoFontOptions return the pointer or its >> > value for > >> SetupPangoLayout (and I suppose you should rename >> > UpdateCairoFontOptions to > >> GetCairoFontOptions or something). >> > > this is defined in unnamed scope, so should be the same as file scoped > static. > > UpdateCairoFontOptions creates cairo_font_options_t and set attributes > on it, not only Get it. > > http://codereview.chromium.**org/7511029/diff/28004/ui/gfx/** > gtk_util.cc#newcode111<http://codereview.chromium.org/7511029/diff/28004/ui/gfx/gtk_util.cc#newcode111> > ui/gfx/gtk_util.cc:111: GtkSettings* gtk_settings = > gtk_settings_get_default(); > hm.. this has gtk dependency. Oshima, could you check whether this cause > problem? > > > http://codereview.chromium.**org/7511029/diff/28004/ui/gfx/** > gtk_util.cc#newcode166<http://codereview.chromium.org/7511029/diff/28004/ui/gfx/gtk_util.cc#newcode166> > ui/gfx/gtk_util.cc:166: } // namespace > On 2011/08/23 08:01:01, msw wrote: > >> Add a blank line here. >> > > Done. > > > http://codereview.chromium.**org/7511029/diff/28004/ui/gfx/** > gtk_util.cc#newcode247<http://codereview.chromium.org/7511029/diff/28004/ui/gfx/gtk_util.cc#newcode247> > ui/gfx/gtk_util.cc:247: // Pass a width > 0 to force wrapping and > elliding. > On 2011/08/23 08:01:01, msw wrote: > >> remove the extra L from "eliding"; use the phrase "greater than" >> > instead of '>'. > > Done. > > > http://codereview.chromium.**org/7511029/diff/28004/ui/gfx/**gtk_util.h<http:... > File ui/gfx/gtk_util.h (right): > > http://codereview.chromium.**org/7511029/diff/28004/ui/gfx/** > gtk_util.h#newcode52<http://codereview.chromium.org/7511029/diff/28004/ui/gfx/gtk_util.h#newcode52> > ui/gfx/gtk_util.h:52: void SetupPangoLayout(PangoLayout* layout, > On 2011/08/23 08:01:01, msw wrote: > >> Hmm, I didn't suspect that moving the declaration up here would >> > require you to > >> move the definition from canvas_skia_linux.cc to gtk_util.cc. I think >> > it's > >> generally bad to add functions to *_util files, can all this pango >> > stuff live in > >> canvas_skia_linux.* or render_text_linux.*? I might be wrong here, >> > please > >> confirm with oshima or someone else. >> > > Ah, I think I mis-interpreted your previous comments. > I thought you'd like move the definition into gtk_util. > I do not think it is a good idea either. > > And it would be better to declare that in canvas_skia.h and still define > that in canvas_skia_linux.cc > > > http://codereview.chromium.**org/7511029/diff/28004/ui/gfx/** > render_text.cc<http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text.cc> > File ui/gfx/render_text.cc (right): > > http://codereview.chromium.**org/7511029/diff/28004/ui/gfx/** > render_text.cc#newcode197<http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text.cc#newcode197> > ui/gfx/render_text.cc:197: SelectionModel selection_start = > GetSelectionModelForSelectionS**tart(); > On 2011/08/23 08:01:01, msw wrote: > >> Remove the extra space after '='. >> > > Done. > > > http://codereview.chromium.**org/7511029/diff/28004/ui/gfx/** > render_text.cc#newcode624<http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text.cc#newcode624> > ui/gfx/render_text.cc:624: // BTW, Firefox has the same issue. > On 2011/08/23 08:01:01, msw wrote: > >> No need to call out Firefox here, we can alert them if they have a >> > similar bug. > > Ah, you absolutely right. removed. > > > http://codereview.chromium.**org/7511029/diff/28004/ui/gfx/** > render_text.cc#newcode645<http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text.cc#newcode645> > ui/gfx/render_text.cc:645: > GetIndexOfPreviousGrapheme(**GetSelectionStart()), > On 2011/08/23 08:01:01, msw wrote: > >> What if the selection start is 0 and there is not previous grapheme? >> > > this is called when selection is not empty and selection_start > > cursor_pos, so selection_start should not be 0. > I added comments in header file. > > > http://codereview.chromium.**org/7511029/diff/28004/ui/gfx/**render_text.h<ht... > File ui/gfx/render_text.h (right): > > http://codereview.chromium.**org/7511029/diff/28004/ui/gfx/** > render_text.h#newcode289<http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text.h#newcode289> > ui/gfx/render_text.h:289: // TODO(xji): Remove the functions from > RenderTextWin. > On 2011/08/23 08:01:01, msw wrote: > >> You don't need this TODO, I'll take care of merging that. >> > > removed. > > > http://codereview.chromium.**org/7511029/diff/28004/ui/gfx/** > render_text_linux.cc<http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc> > File ui/gfx/render_text_linux.cc (right): > > http://codereview.chromium.**org/7511029/diff/28004/ui/gfx/** > render_text_linux.cc#newcode18<http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc#newcode18> > ui/gfx/render_text_linux.cc:**18: #define COLOR_16_TO_32_BIT(c) ((c)/0xff > * 0xffff) > On 2011/08/23 08:01:01, msw wrote: > >> Can you make this a file local function instead of a pre-processor >> > directive? > >> Also, I'd appreciate if someone else could double-check this logic. >> > > changed to function in unnamed namespace. > checked with Behdad. changed to "c * (0xffff / 0xff)". > > > http://codereview.chromium.**org/7511029/diff/28004/ui/gfx/** > render_text_linux.cc#newcode86<http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc#newcode86> > ui/gfx/render_text_linux.cc:**86: // TODO(xji): use ScopedPlatformPaing. > On 2011/08/23 08:01:01, msw wrote: > >> ScopedPlatformPain*t* >> > > Done. > > > http://codereview.chromium.**org/7511029/diff/28004/ui/gfx/** > render_text_linux.cc#**newcode157<http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc#newcode157> > ui/gfx/render_text_linux.cc:**157: int h = > std::min(display_rect().**height(), pos.height/PANGO_SCALE); > On 2011/08/23 08:01:01, msw wrote: > >> spaces around '/' operator. >> > > Done. > > http://codereview.chromium.**org/7511029/diff/28004/ui/gfx/** > render_text_linux.cc#**newcode323<http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc#newcode323> > ui/gfx/render_text_linux.cc:**323: // if left run is LTR run, > On 2011/08/23 08:01:01, msw wrote: > >> Capitalize "If" >> > > Done. > > > http://codereview.chromium.**org/7511029/diff/28004/ui/gfx/** > render_text_linux.cc#**newcode325<http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc#newcode325> > ui/gfx/render_text_linux.cc:**325: // If rght run is RTL run, > On 2011/08/23 08:01:01, msw wrote: > >> Don't you mean left run? >> > > yes. changed. > > > http://codereview.chromium.**org/7511029/diff/28004/ui/gfx/** > render_text_linux.cc#**newcode381<http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc#newcode381> > ui/gfx/render_text_linux.cc:**381: // if right run is LTR run, > On 2011/08/23 08:01:01, msw wrote: > >> Capitalize "If" >> > > Done. > > > http://codereview.chromium.**org/7511029/diff/28004/ui/gfx/** > render_text_linux.cc#**newcode383<http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc#newcode383> > ui/gfx/render_text_linux.cc:**383: // If rght run is RTL run, > On 2011/08/23 08:01:01, msw wrote: > >> r*i*ght >> > > Done. > > > http://codereview.chromium.**org/7511029/diff/28004/ui/gfx/** > render_text_linux.cc#**newcode409<http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc#newcode409> > ui/gfx/render_text_linux.cc:**409: return SelectionModel(caret - 1, caret > - 1, SelectionModel::LEADING); > On 2011/08/23 08:01:01, msw wrote: > >> Do you mean to get Utf16IndexOfAdjacentGrapheme(**caret, PREVIOUS) here? >> > > > YES! thanks for catching it. > > > Also, > >> here and everywhere else, I'm curious what happens if there is no >> > PREVIOUS or > >> NEXT grapheme. >> > > When there is no previous grapheme, the previous grapheme call returns > 0. > > when there is no next grapheme, the next grapheme returns the string > length. > > > http://codereview.chromium.**org/7511029/diff/28004/ui/gfx/** > render_text_linux.cc#**newcode534<http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc#newcode534> > ui/gfx/render_text_linux.cc:**534: u_strToUTF8WithSub(NULL, 0, > &utf8_index, text().data(), index, > On 2011/08/23 08:01:01, msw wrote: > >> I'm worried by the potential performance losses incurred by calling >> > this > >> function and its corresponding 8->16 frequently. RenderTextLinux and >> RenderTextWin will both need some performance testing and tuning, but >> > these > >> calls might warrant explicit TODOs, and I'd like to see official >> > review from > >> someone that knows this code better. >> > > I have the same concern. I checked with Markus who is very familiar with > the above function, and he said that when destination buffer is NULL and > destination length is 0, the code has a quick path to get the index > without doing real conversion. > > Since pango use utf8 internally, we have to do the conversion on > selection_start/end. But I have a TODO at the begin of file saying that > to remove the convresion on caret_pos. > > I will ask Markus to take a look. > > > http://codereview.chromium.**org/7511029/diff/28004/ui/gfx/** > render_text_linux.cc#**newcode536<http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc#newcode536> > ui/gfx/render_text_linux.cc:**536: return utf8_index; > On 2011/08/23 08:01:01, msw wrote: > >> I think you're supposed to check ec for U_FAILURE (same below). >> > > From doc > "If the input string is not well-formed, then the U_INVALID_CHAR_FOUND > error code is set.". > > I think our string should be well-formed. If it is well-formed, we could > use u_strToUTF8, which exits when found a not well-formed character. > > To make it safe, I uses u_strToUTF8WithSub(), which convert the not > well-formed character to substitute character. > > The return value means whether the input string contains not well-formed > character or not. I do not think we need to care about that. Or if there > is one, what should we do? > > > http://codereview.chromium.**org/7511029/<http://codereview.chromium.org/7511... >
http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:409: return SelectionModel(caret - 1, caret - 1, SelectionModel::LEADING); On 2011/08/23 23:52:52, xji wrote: > Also, > > here and everywhere else, I'm curious what happens if there is no PREVIOUS or > > NEXT grapheme. > > When there is no previous grapheme, the previous grapheme call returns 0. > > when there is no next grapheme, the next grapheme returns the string length. Okay, then if the callers are trying to get the previous or next grapheme and setting trailing/leading as opposed to leading/trailing, you should catch those errors. For example if you're trying to set the caret position to trail the cursor's previous grapheme, but the cursor is already at 0, then you're going to set the caret to (0, trailing) but it really ought to be (0, leading). We ought to audit all the callers of such functions to ensure that they implement the correct behavior if the returned 'adjacent' grapheme is the same as the input value. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:536: return utf8_index; On 2011/08/23 23:52:52, xji wrote: > On 2011/08/23 08:01:01, msw wrote: > > I think you're supposed to check ec for U_FAILURE (same below). > > From doc > "If the input string is not well-formed, then the U_INVALID_CHAR_FOUND error > code is set.". > > I think our string should be well-formed. If it is well-formed, we could use > u_strToUTF8, which exits when found a not well-formed character. > > To make it safe, I uses u_strToUTF8WithSub(), which convert the not well-formed > character to substitute character. > > The return value means whether the input string contains not well-formed > character or not. I do not think we need to care about that. Or if there is one, > what should we do? Sounds like you should add a DCHECK_EQ(ec, U_ZERO_ERROR); or similar. http://codereview.chromium.org/7511029/diff/36002/ui/gfx/pango_util.cc File ui/gfx/pango_util.cc (right): http://codereview.chromium.org/7511029/diff/36002/ui/gfx/pango_util.cc#newcode1 ui/gfx/pango_util.cc:1: #include "ui/gfx/pango_util.h" Add a copyright notice. http://codereview.chromium.org/7511029/diff/36002/ui/gfx/pango_util.cc#newcode22 ui/gfx/pango_util.cc:22: cairo_font_options_t* cairo_font_options = NULL; I meant to say that this is a global class pointer, which violates our style guide, even if it's in an anonymous namespace. I think a better singleton pattern is to make this a static pointer declared inside UpdateCairoFontOptions. I might be wrong, perhaps oshima can weigh in. http://codereview.chromium.org/7511029/diff/36002/ui/gfx/pango_util.h File ui/gfx/pango_util.h (right): http://codereview.chromium.org/7511029/diff/36002/ui/gfx/pango_util.h#newcode9 ui/gfx/pango_util.h:9: #include <pango/pango-layout.h> You can actually forward declare PangoLayout and move this include to the cc. http://codereview.chromium.org/7511029/diff/36002/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/7511029/diff/36002/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:315: // when selection is not empty, i.e. selection_end_ != selection_start_. I would hope that this function does something reasonable when the selection is empty... You should remove this comment constraintor enforce it as a DCHECK in the function body. http://codereview.chromium.org/7511029/diff/36002/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/7511029/diff/36002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:18: // TODO(xji): instead of converting each R or G or B from 8-bite to 16-bit, remove 'e' from bit*e* http://codereview.chromium.org/7511029/diff/36002/ui/gfx/render_text_linux.h File ui/gfx/render_text_linux.h (right): http://codereview.chromium.org/7511029/diff/36002/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:58: // the |text_| length if there is no next grapheme. See my other comment about handling this behavior at callsites.
http://codereview.chromium.org/7511029/diff/36002/ui/gfx/pango_util.cc File ui/gfx/pango_util.cc (right): http://codereview.chromium.org/7511029/diff/36002/ui/gfx/pango_util.cc#newcode22 ui/gfx/pango_util.cc:22: cairo_font_options_t* cairo_font_options = NULL; On 2011/08/24 08:55:30, msw wrote: > I meant to say that this is a global class pointer, which violates our style > guide, even if it's in an anonymous namespace. I think a better singleton > pattern is to make this a static pointer declared inside UpdateCairoFontOptions. > I might be wrong, perhaps oshima can weigh in. This is allowed in anonymous namespace, although I'd probably do something like this: GetCairoFontOptions and do everything in there including check and creation. cairo_font_options_t* GetCairoFontOptions() { static cairo_font_options_t* cfo = NULL; if (cfo) return cfo; cfo = ... return cfo; }
On 2011/08/24 21:12:30, oshima wrote: > http://codereview.chromium.org/7511029/diff/36002/ui/gfx/pango_util.cc > File ui/gfx/pango_util.cc (right): > > http://codereview.chromium.org/7511029/diff/36002/ui/gfx/pango_util.cc#newcode22 > ui/gfx/pango_util.cc:22: cairo_font_options_t* cairo_font_options = NULL; > On 2011/08/24 08:55:30, msw wrote: > > I meant to say that this is a global class pointer, which violates our style > > guide, even if it's in an anonymous namespace. I think a better singleton > > pattern is to make this a static pointer declared inside > UpdateCairoFontOptions. > > I might be wrong, perhaps oshima can weigh in. > > This is allowed in anonymous namespace, although I'd probably do something like > this: > > GetCairoFontOptions and do everything in there including check and creation. > > cairo_font_options_t* GetCairoFontOptions() { > static cairo_font_options_t* cfo = NULL; > if (cfo) > return cfo; > cfo = ... > return cfo; > } Awesome, that's what I was suggesting :)
patch 13 address comments on patch 12. And it changed some code to avoid across boundary access, such as reset selection model in SetText(), clearCompositionRange before SetText() in TextfieldViewModel, check range when append pango attribute. @Jungshik, could you check the usage of u_strTo/FromUTF8? http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:409: return SelectionModel(caret - 1, caret - 1, SelectionModel::LEADING); On 2011/08/24 08:55:30, msw wrote: > On 2011/08/23 23:52:52, xji wrote: > > Also, > > > here and everywhere else, I'm curious what happens if there is no PREVIOUS > or > > > NEXT grapheme. > > > > When there is no previous grapheme, the previous grapheme call returns 0. > > > > when there is no next grapheme, the next grapheme returns the string length. > > Okay, then if the callers are trying to get the previous or next grapheme and > setting trailing/leading as opposed to leading/trailing, you should catch those > errors. For example if you're trying to set the caret position to trail the > cursor's previous grapheme, but the cursor is already at 0, then you're going to > set the caret to (0, trailing) but it really ought to be (0, leading). > > We ought to audit all the callers of such functions to ensure that they > implement the correct behavior if the returned 'adjacent' grapheme is the same > as the input value. To match the restrictions you added to selection model, I checked the usage of previous/next grapheme. Looks like the usages in RenderTextLinux is fine. I changed RenderText::GetRightSelectionModel(), and changed RenderText::SelectWord() to use MoveCursorTo() as yours. http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:536: return utf8_index; On 2011/08/24 08:55:30, msw wrote: > On 2011/08/23 23:52:52, xji wrote: > > On 2011/08/23 08:01:01, msw wrote: > > > I think you're supposed to check ec for U_FAILURE (same below). > > > > From doc > > "If the input string is not well-formed, then the U_INVALID_CHAR_FOUND error > > code is set.". > > > > I think our string should be well-formed. If it is well-formed, we could use > > u_strToUTF8, which exits when found a not well-formed character. > > > > To make it safe, I uses u_strToUTF8WithSub(), which convert the not > well-formed > > character to substitute character. > > > > The return value means whether the input string contains not well-formed > > character or not. I do not think we need to care about that. Or if there is > one, > > what should we do? > > Sounds like you should add a DCHECK_EQ(ec, U_ZERO_ERROR); or similar. Done. http://codereview.chromium.org/7511029/diff/36002/ui/gfx/pango_util.cc File ui/gfx/pango_util.cc (right): http://codereview.chromium.org/7511029/diff/36002/ui/gfx/pango_util.cc#newcode1 ui/gfx/pango_util.cc:1: #include "ui/gfx/pango_util.h" On 2011/08/24 08:55:30, msw wrote: > Add a copyright notice. Done. http://codereview.chromium.org/7511029/diff/36002/ui/gfx/pango_util.cc#newcode22 ui/gfx/pango_util.cc:22: cairo_font_options_t* cairo_font_options = NULL; On 2011/08/24 21:12:30, oshima wrote: > On 2011/08/24 08:55:30, msw wrote: > > I meant to say that this is a global class pointer, which violates our style > > guide, even if it's in an anonymous namespace. I think a better singleton > > pattern is to make this a static pointer declared inside > UpdateCairoFontOptions. > > I might be wrong, perhaps oshima can weigh in. > > This is allowed in anonymous namespace, although I'd probably do something like > this: > > GetCairoFontOptions and do everything in there including check and creation. > > cairo_font_options_t* GetCairoFontOptions() { > static cairo_font_options_t* cfo = NULL; > if (cfo) > return cfo; > cfo = ... > return cfo; > } Done. http://codereview.chromium.org/7511029/diff/36002/ui/gfx/pango_util.h File ui/gfx/pango_util.h (right): http://codereview.chromium.org/7511029/diff/36002/ui/gfx/pango_util.h#newcode9 ui/gfx/pango_util.h:9: #include <pango/pango-layout.h> On 2011/08/24 08:55:30, msw wrote: > You can actually forward declare PangoLayout and move this include to the cc. PangoLayout is a typedef. http://codereview.chromium.org/7511029/diff/36002/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/7511029/diff/36002/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:315: // when selection is not empty, i.e. selection_end_ != selection_start_. On 2011/08/24 08:55:30, msw wrote: > I would hope that this function does something reasonable when the selection is > empty... You should remove this comment constraintor enforce it as a DCHECK in > the function body. changed the implementation to return selection_model_ when selection is empty. http://codereview.chromium.org/7511029/diff/36002/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/7511029/diff/36002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:18: // TODO(xji): instead of converting each R or G or B from 8-bite to 16-bit, On 2011/08/24 08:55:30, msw wrote: > remove 'e' from bit*e* Done.
http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/7511029/diff/28004/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:534: u_strToUTF8WithSub(NULL, 0, &utf8_index, text().data(), index, On 2011/08/23 08:01:01, msw wrote: > I'm worried by the potential performance losses incurred by calling this > function and its corresponding 8->16 frequently. RenderTextLinux and > RenderTextWin will both need some performance testing and tuning, but these > calls might warrant explicit TODOs, and I'd like to see official review from > someone that knows this code better. Well, I wrote and rewrote these functions :-) They start with some extra tests that would not be necessary in a function which never writes any output, but starting with the second input character they fall into a tight loop that just counts the result length. These functions do need to read the string up to the input index/length, but there is no way around that for the index conversion. http://codereview.chromium.org/7511029/diff/55001/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/7511029/diff/55001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:22: return c * (0xffff / 0xff); return c << 8; http://codereview.chromium.org/7511029/diff/55001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:559: // Even given a desination buffer as NULL and destination capacity as 0, destination (one t is missing) http://codereview.chromium.org/7511029/diff/55001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:566: DCHECK(ec != U_INVALID_CHAR_FOUND); DCHECK(ec == U_BUFFER_OVERFLOW_ERROR || ec == U_STRING_NOT_TERMINATED_WARNING); http://codereview.chromium.org/7511029/diff/55001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:575: DCHECK(ec != U_INVALID_CHAR_FOUND); ditto
Thanks for the review. http://codereview.chromium.org/7511029/diff/55001/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/7511029/diff/55001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:22: return c * (0xffff / 0xff); On 2011/08/25 19:09:05, markus.icu wrote: > return c << 8; changed to (c << 8 ) + c; Behdad suggested the same. But since it was a chained macro previously, to avoid evaluate 'c' twice, we decided to do above and let compiler do optimization. http://codereview.chromium.org/7511029/diff/55001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:559: // Even given a desination buffer as NULL and destination capacity as 0, On 2011/08/25 19:09:05, markus.icu wrote: > destination (one t is missing) Done. http://codereview.chromium.org/7511029/diff/55001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:566: DCHECK(ec != U_INVALID_CHAR_FOUND); On 2011/08/25 19:09:05, markus.icu wrote: > DCHECK(ec == U_BUFFER_OVERFLOW_ERROR || ec == U_STRING_NOT_TERMINATED_WARNING); Done. http://codereview.chromium.org/7511029/diff/55001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:575: DCHECK(ec != U_INVALID_CHAR_FOUND); On 2011/08/25 19:09:05, markus.icu wrote: > ditto Done.
http://codereview.chromium.org/7511029/diff/60014/ui/gfx/pango_util.cc File ui/gfx/pango_util.cc (right): http://codereview.chromium.org/7511029/diff/60014/ui/gfx/pango_util.cc#newcod... ui/gfx/pango_util.cc:189: } } // namespace gfx Also, Lint thinks this needs a newline at the end of the file. http://codereview.chromium.org/7511029/diff/60014/ui/gfx/pango_util.h File ui/gfx/pango_util.h (right): http://codereview.chromium.org/7511029/diff/60014/ui/gfx/pango_util.h#newcode31 ui/gfx/pango_util.h:31: #endif // UI_GFX_PANGO_UTIL_H_ Lint thinks this needs an EOF newline http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:25: } } // namespace http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:98: // TODO(xji): use ScopedPlatformPaint. You should be able to do: skia::ScopedPlatformPaint scoped_platform_paint(canvas); cairo_t* cr = scoped_platform_paint.GetPlatformSurface(); http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:110: pango_cairo_show_layout(cr, layout); We ought to check with a Pango/Skia expert to determine if this is okay, we might need to tweak this for acceleration, etc. http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:120: canvas->DrawRectInt(kCursorColor, outdent this line by one space. http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:220: return SelectionModel(text().length(), caret, SelectionModel::LEADING); Why do you set the cursor to text().length() here? Shouldn't it also be at the position corresponding to item->offset? http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:225: return SelectionModel(text().length(), caret, SelectionModel::TRAILING); Shouldn't this cursor be at position corresponding to item->offset + item->length? http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:242: return SelectionModel(text().length(), caret, SelectionModel::TRAILING); Shouldn't this cursor be at item->offset + item->length? http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:245: return SelectionModel(text().length(), caret, SelectionModel::LEADING); shouldn't this cursor be at item->offset? http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:268: RelativeLogicalPosition pos) const { I implemented a similar function: size_t IndexOfAdjacentGrapheme(size_t index, bool next) const; We'll devise a common signature as we consolidate code. http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:377: if (!prev_run) // TODO(xji): check whether this is expected. If the caret is at the beginning of the first run, this will be hit. I think it is doing the correct thing even with RTL UI and/or text. http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:434: if (!next_run) // TODO(xji): what is the expected behavior? If the caret happens to be greater than or equal to text().length() (which we currently consider out-of-bounds and try to prevent), then this will move it within bounds on the right side, which seems reasonable. http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:445: // TODO(xji): can use ScopedPlatformPaint ? You should be able to do: skia::ScopedPlatformPaint scoped_platform_paint(canvas); cairo_t* cr = scoped_platform_paint.GetPlatformSurface(); http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:470: // operation that trigger ResetLayout(). trigger*s*, and we should probably add some common layout invalidation API as we begin to consolidate Win and Linux. http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:535: pango_attr->start_index = Utf16IndexToUtf8Index(start); This function is private, and all the callers check their start and end input values, so you can probably either eliminate the external checks and also check start in here (my recommendation), or eliminate the end check in this function. http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.h File ui/gfx/render_text_linux.h (right): http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:71: // Get the selection model that visually left or right of |current| by one remove "that " or change it to "that is". http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:78: // is NULL. Otherwise, return the cached |layout_|. Can you rephrase this comment? You're not returning layout_line_. http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:94: GSList* GetPreviousRun(GSList* run) const; Do you think it's worth warning that this function is O(n) on the runs since it's using GSList? It might not be necessary since it's private and I'm sure there are more important efficiency gains we could make. http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:96: // Returns the last run in |layout_line_|. Same question about an O(n) warning here. http://codereview.chromium.org/7511029/diff/60014/views/controls/textfield/te... File views/controls/textfield/textfield_views_model.cc (right): http://codereview.chromium.org/7511029/diff/60014/views/controls/textfield/te... views/controls/textfield/textfield_views_model.cc:614: ClearComposition(); Why did you make this change? I have a vague feeling that I tried this before and it caused problems, but I don't remember exactly what. Perhaps oshima can take a look.
http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:25: } On 2011/08/29 20:38:05, msw wrote: > } // namespace Done. http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:98: // TODO(xji): use ScopedPlatformPaint. On 2011/08/29 20:38:05, msw wrote: > You should be able to do: > skia::ScopedPlatformPaint scoped_platform_paint(canvas); > cairo_t* cr = scoped_platform_paint.GetPlatformSurface(); Done. http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:110: pango_cairo_show_layout(cr, layout); On 2011/08/29 20:38:05, msw wrote: > We ought to check with a Pango/Skia expert to determine if this is okay, we > might need to tweak this for acceleration, etc. talked with Mike Reed, who said that "The only way to get gpu accelerated via skia is to not draw through cairo, but only through SkCanvas". since chrome's UI drawing in Linux all use pango directly, he said that "we need to get all of chrome to try to use SkCanvas for all of their drawing." http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:120: canvas->DrawRectInt(kCursorColor, On 2011/08/29 20:38:05, msw wrote: > outdent this line by one space. Done. http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:220: return SelectionModel(text().length(), caret, SelectionModel::LEADING); On 2011/08/29 20:38:05, msw wrote: > Why do you set the cursor to text().length() here? > Shouldn't it also be at the position corresponding to item->offset? pango's text direction is determined by its first strong directionality character's direction. In this case, the text begins with a RTL character and ends in LTR character. such as "ABCdef" visually display as "defCBA". In RTL context, the left end is logical END. and the cursor should be text().length(). (it is the same as you set cursor as text().length() when base::i18n::isRTL()). In this case, it should be equivalent to the utf16 index of (item->offset + item->length). but using text().length() directly is clear and saves one conversion. http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:225: return SelectionModel(text().length(), caret, SelectionModel::TRAILING); On 2011/08/29 20:38:05, msw wrote: > Shouldn't this cursor be at position corresponding to item->offset + > item->length? same reason as above. in RTL context, Left end is logical end, cursor should be text.length. equivalent to (item->offset + item->length())'s utf16 conversion. but I think text().length() is clearer. http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:242: return SelectionModel(text().length(), caret, SelectionModel::TRAILING); On 2011/08/29 20:38:05, msw wrote: > Shouldn't this cursor be at item->offset + item->length? ditto. in LTR context, right end is logical end, cursor should be text().length(). equivalent to (item->offset + item->length). but text().length() is clearer. http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:245: return SelectionModel(text().length(), caret, SelectionModel::LEADING); On 2011/08/29 20:38:05, msw wrote: > shouldn't this cursor be at item->offset? ditto. in LTR context, right end is logical end, cursor should be text().length(). equivalent to (item->offset + item->length). http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:377: if (!prev_run) // TODO(xji): check whether this is expected. On 2011/08/29 20:38:05, msw wrote: > If the caret is at the beginning of the first run, this will be hit. I think it > is doing the correct thing even with RTL UI and/or text. Given an example, logical text "ABCdef", visually "defCBA". when click left of 'd', selection model is (3, 3, leading). The question is what happen when arrow-left. should the selection model remain unchanged, it should it change to left-end as (6, 3, leading). I do not seem to get relies from our native speakers after I send out the question. That is why the TODO. http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:434: if (!next_run) // TODO(xji): what is the expected behavior? On 2011/08/29 20:38:05, msw wrote: > If the caret happens to be greater than or equal to text().length() (which we > currently consider out-of-bounds and try to prevent), then this will move it > within bounds on the right side, which seems reasonable. ditto as above reason. consider case "abcDEF" visually as "abcFED", when click right of 'D', then click right arrow, what should be expected? specifically the logical position (selection_end)? http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:445: // TODO(xji): can use ScopedPlatformPaint ? On 2011/08/29 20:38:05, msw wrote: > You should be able to do: > skia::ScopedPlatformPaint scoped_platform_paint(canvas); > cairo_t* cr = scoped_platform_paint.GetPlatformSurface(); Done. http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:470: // operation that trigger ResetLayout(). On 2011/08/29 20:38:05, msw wrote: > trigger*s*, and we should probably add some common layout invalidation API as we > begin to consolidate Win and Linux. Done. agree on the consolidation part. http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:535: pango_attr->start_index = Utf16IndexToUtf8Index(start); On 2011/08/29 20:38:05, msw wrote: > This function is private, and all the callers check their start and end input > values, so you can probably either eliminate the external checks and also check > start in here (my recommendation), or eliminate the end check in this function. I am removing the 'end' check here. it should check in external calls. in case start >= end, no pango attribute will be created which eliminates memory leak and time cost. http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.h File ui/gfx/render_text_linux.h (right): http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:71: // Get the selection model that visually left or right of |current| by one On 2011/08/29 20:38:05, msw wrote: > remove "that " or change it to "that is". Done. http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:78: // is NULL. Otherwise, return the cached |layout_|. On 2011/08/29 20:38:05, msw wrote: > Can you rephrase this comment? You're not returning layout_line_. Done. http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:94: GSList* GetPreviousRun(GSList* run) const; On 2011/08/29 20:38:05, msw wrote: > Do you think it's worth warning that this function is O(n) on the runs since > it's using GSList? It might not be necessary since it's private and I'm sure > there are more important efficiency gains we could make. added a comment. http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:96: // Returns the last run in |layout_line_|. On 2011/08/29 20:38:05, msw wrote: > Same question about an O(n) warning here. Done. http://codereview.chromium.org/7511029/diff/60014/views/controls/textfield/te... File views/controls/textfield/textfield_views_model.cc (right): http://codereview.chromium.org/7511029/diff/60014/views/controls/textfield/te... views/controls/textfield/textfield_views_model.cc:614: ClearComposition(); On 2011/08/29 20:38:05, msw wrote: > Why did you make this change? > I have a vague feeling that I tried this before and it caused problems, but I > don't remember exactly what. Perhaps oshima can take a look. Inside SetCursorPosition(), we will need to get the previous grapheme of the last index, which depends on pango layout is set up. During set up layout, pango attribute is set up, including styles of composition text. Without ClearComposition(), we will be accessing the Composition range of previous text during the setup of current text. BTW, should not we reset composition_range_ inside SetText()?
LGTM; I hope a Pango expert will also verify this change. http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:110: pango_cairo_show_layout(cr, layout); On 2011/08/30 00:39:41, xji wrote: > On 2011/08/29 20:38:05, msw wrote: > > We ought to check with a Pango/Skia expert to determine if this is okay, we > > might need to tweak this for acceleration, etc. > > talked with Mike Reed, who said that "The only way to get gpu accelerated via > skia is to not draw through cairo, but only through SkCanvas". since chrome's UI > drawing in Linux all use pango directly, he said that "we need to get all of > chrome to try to use SkCanvas for all of their drawing." Ok, that's definitely outside the scope of this change. http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:220: return SelectionModel(text().length(), caret, SelectionModel::LEADING); On 2011/08/30 00:39:41, xji wrote: > On 2011/08/29 20:38:05, msw wrote: > > Why do you set the cursor to text().length() here? > > Shouldn't it also be at the position corresponding to item->offset? > > pango's text direction is determined by its first strong directionality > character's direction. In this case, the text begins with a RTL character and > ends in LTR character. > such as "ABCdef" visually display as "defCBA". > In RTL context, the left end is logical END. and the cursor should be > text().length(). (it is the same as you set cursor as text().length() when > base::i18n::isRTL()). > > In this case, it should be equivalent to the utf16 index of (item->offset + > item->length). but using text().length() directly is clear and saves one > conversion. > Ah, you're right. Sorry, this behavior can just be a little confusing at times. So when the user winds up at LeftEndSelectionModel, they see "|defCBA" with the logical cursor at "ABCdef|". Then typing 'G' makes sense, yielding "|GdefCBA". And typing 'g' sorta makes sense, making the caret jump and yielding "defg|CBA". http://codereview.chromium.org/7511029/diff/60014/views/controls/textfield/te... File views/controls/textfield/textfield_views_model.cc (right): http://codereview.chromium.org/7511029/diff/60014/views/controls/textfield/te... views/controls/textfield/textfield_views_model.cc:614: ClearComposition(); On 2011/08/30 00:39:41, xji wrote: > On 2011/08/29 20:38:05, msw wrote: > > Why did you make this change? > > I have a vague feeling that I tried this before and it caused problems, but I > > don't remember exactly what. Perhaps oshima can take a look. > > > Inside SetCursorPosition(), we will need to get the previous grapheme of the > last index, which depends on pango layout is set up. During set up layout, pango > attribute is set up, including styles of composition text. > Without ClearComposition(), we will be accessing the Composition range of > previous text during the setup of current text. > OK. > BTW, should not we reset composition_range_ inside SetText()? Perhaps, I'm not an expert with the IME code.
http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/7511029/diff/60014/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:220: return SelectionModel(text().length(), caret, SelectionModel::LEADING); On 2011/08/30 01:17:07, msw wrote: > On 2011/08/30 00:39:41, xji wrote: > > On 2011/08/29 20:38:05, msw wrote: > > > Why do you set the cursor to text().length() here? > > > Shouldn't it also be at the position corresponding to item->offset? > > > > pango's text direction is determined by its first strong directionality > > character's direction. In this case, the text begins with a RTL character and > > ends in LTR character. > > such as "ABCdef" visually display as "defCBA". > > In RTL context, the left end is logical END. and the cursor should be > > text().length(). (it is the same as you set cursor as text().length() when > > base::i18n::isRTL()). > > > > In this case, it should be equivalent to the utf16 index of (item->offset + > > item->length). but using text().length() directly is clear and saves one > > conversion. > > > > Ah, you're right. Sorry, this behavior can just be a little confusing at times. > So when the user winds up at LeftEndSelectionModel, they see "|defCBA" with the > logical cursor at "ABCdef|". Then typing 'G' makes sense, yielding "|GdefCBA". > And typing 'g' sorta makes sense, making the caret jump and yielding "defg|CBA". Yes. that is right.
http://codereview.chromium.org/7511029/diff/60014/views/controls/textfield/te... File views/controls/textfield/textfield_views_model.cc (right): http://codereview.chromium.org/7511029/diff/60014/views/controls/textfield/te... views/controls/textfield/textfield_views_model.cc:614: ClearComposition(); On 2011/08/30 01:17:07, msw wrote: > On 2011/08/30 00:39:41, xji wrote: > > On 2011/08/29 20:38:05, msw wrote: > > > Why did you make this change? > > > I have a vague feeling that I tried this before and it caused problems, but > I > > > don't remember exactly what. Perhaps oshima can take a look. > > > > > > Inside SetCursorPosition(), we will need to get the previous grapheme of the > > last index, which depends on pango layout is set up. During set up layout, > pango > > attribute is set up, including styles of composition text. > > Without ClearComposition(), we will be accessing the Composition range of > > previous text during the setup of current text. > > > > OK. > > > BTW, should not we reset composition_range_ inside SetText()? > > Perhaps, I'm not an expert with the IME code. From suzhe "The caller should make sure that the composition text has been committed or cancelled before calling RenderText::SetText(), so it's probably reasonable to do a DCHECK in RenderText::SetText() to make sure the composition range has been cleared."
Since I am still waiting Behdad's review, I am continuing changing the code. I will try to specify the exact changes. Patch 20 fixes: 1. set cursor width as 1 (instead of 0). Seems that 0 is not working for touch ui, cursor is invisible if width == 0. 2. use insert_mode == true in GetCursorBounds() in MoveCursorLeft/Right when there is selection.Since the cursor bounds are used to detect which (selection_start or end) is visually left/right, insert_mode should be used. 3. set_pango_width as -1. The default SetupPangoLayout() set pango width as -1 if text directionality is LTR. Since I am using ToViewPoint/ToTextPoint to adjust offset between within text and with view port, set pango width as -1 is ok and consistent.
On 2011/09/03 00:50:04, xji wrote: > Since I am still waiting Behdad's review, I am continuing changing the code. > > I will try to specify the exact changes. > > Patch 20 fixes: > 1. set cursor width as 1 (instead of 0). Seems that 0 is not working for touch > ui, cursor is invisible if width == 0. That shouldn't be necessary, Sadrul fixed that issue with crrev.com/99247 > 2. use insert_mode == true in GetCursorBounds() in MoveCursorLeft/Right when > there is selection.Since the cursor bounds are used to detect which > (selection_start or end) is visually left/right, insert_mode should be used. Thanks, that should be a good improvement! > 3. set_pango_width as -1. The default SetupPangoLayout() set pango width as -1 > if text directionality is LTR. Since I am using ToViewPoint/ToTextPoint to > adjust offset between within text and with view port, set pango width as -1 is > ok and consistent. I don't understand how this impacts your code, will GetStringWidth return -1 for empty strings now, since it just calls pango_layout_get_pixel_size? If so, that seems wrong. Perhaps add a comment with this new initialization line.
Hi Xiaomei, Thanks for the great work! I reviewed part of it, comments below. I'll study SelectionModel in detail after lunch and continue reviewing the rest. Cheers, behdad http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:20: // it should also massage A in the conversion. Right. Do we have the background color around? If we do, I can provide the massaging. http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:76: pango_find_base_dir(layout_text, strlen(layout_text)); Just pass -1 for length here. http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:85: pango_layout_get_pixel_size(layout, &width, &height); You can pass NULL for height. http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:102: cairo_clip(cr); I have a feeling that you shouldn't need the skia clipping then. Or does that affect scoped_platform_paint()? http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:118: if (!bounds.IsEmpty()) The second if is redundant since it's checked in the same if. http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:150: Utf8IndexToUtf16Index(caret_pos), I'm lost about what the two indices to SelectionModel() are. http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:155: bool insert_mode) { What does insert_mode==FALSE represent exactly? Confused. http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:166: (!insert_mode && pos.width < 0)) I can't understand why you need this if(). Why don't we handle rtl in insert_mode? And why don't we handle trailing in rtl mode? The conditions look wrong to me. http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:168: x = x / PANGO_SCALE; Use PANGO_PIXELS(). http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:170: int h = std::min(display_rect().height(), pos.height / PANGO_SCALE); Use PANGO_PIXELS(). http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:209: if (first_visual_run) { I would change the if to "if (layout_line_->runs)", and define first_visual_run as PangoLayoutRun * inside the if block. http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:212: if (item->analysis.level % 2 == 0) { // LTR. Ok, I've reviewed up to here. Will study the SelectionModel and continue reviewing down after lunch. http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.h File ui/gfx/render_text_linux.h (right): http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:54: GSList* GetRunContainingPosition(size_t position) const; Return PangoLayoutRun* instead? http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:95: GSList* GetPreviousRun(GSList* run) const; So, you search within the current line, right? I think PangoLayoutRun* both for input and output is more appropriate. http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:99: GSList* GetLastRun() const; Same here. And in any other place a single run is referenced. http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:106: PangoLayoutLine* layout_line_; Something like current_line_ may be more appropriate. Humm, or do we only care about single-line paragraphs? If that's the case, maybe call pango_layout_set_single_paragraph_mode() too.
On 2011/09/03 01:26:06, msw wrote: > On 2011/09/03 00:50:04, xji wrote: > > Since I am still waiting Behdad's review, I am continuing changing the code. > > > > I will try to specify the exact changes. > > > > Patch 20 fixes: > > 1. set cursor width as 1 (instead of 0). Seems that 0 is not working for touch > > ui, cursor is invisible if width == 0. > > That shouldn't be necessary, Sadrul fixed that issue with crrev.com/99247 I will revert it after sync again. Thanks. > > > 3. set_pango_width as -1. The default SetupPangoLayout() set pango width as -1 > > if text directionality is LTR. Since I am using ToViewPoint/ToTextPoint to > > adjust offset between within text and with view port, set pango width as -1 is > > ok and consistent. > > I don't understand how this impacts your code, will GetStringWidth return -1 for > empty strings now, since it just calls pango_layout_get_pixel_size? If so, that > seems wrong. Perhaps add a comment with this new initialization line. It wont affect GetStringWidth() which will get the pixel size of the actual text. The width set here affects the relative start point of positions in those pango functions. If width is not set (-1), the positions are relative to start of text. If width is set, the positions are relative to the start of view space. SetupPango set the width as -1 if the text is a LTR text. Since the conversion of positions between text space and view space is taken care by ToViewPoint and ToTextPoint, we can always set the width as -1. Otherwise, the point conversion is not correct for a RTL text in RTL UI (where text is right aligned) unless we special handling it. I've added comments.
On 2011/09/06 17:30:45, xji wrote: > > It wont affect GetStringWidth() which will get the pixel size of the actual > text. > The width set here affects the relative start point of positions in those pango > functions. If width is not set (-1), the positions are relative to start of > text. If width is set, the positions are relative to the start of view space. > SetupPango set the width as -1 if the text is a LTR text. Since the conversion > of positions between text space and view space is taken care by ToViewPoint and > ToTextPoint, we can always set the width as -1. Otherwise, the point conversion > is not correct for a RTL text in RTL UI (where text is right aligned) unless we > special handling it. > I've added comments. AFAIK you should be able to always set width and just keep doing what you are doing. If you set width or if you don't, eitherway for LTR text, start position is at x=0.
On 2011/09/06 17:35:11, behdad_google wrote: > On 2011/09/06 17:30:45, xji wrote: > > > > It wont affect GetStringWidth() which will get the pixel size of the actual > > text. > > The width set here affects the relative start point of positions in those > pango > > functions. If width is not set (-1), the positions are relative to start of > > text. If width is set, the positions are relative to the start of view space. > > SetupPango set the width as -1 if the text is a LTR text. Since the conversion > > of positions between text space and view space is taken care by ToViewPoint > and > > ToTextPoint, we can always set the width as -1. Otherwise, the point > conversion > > is not correct for a RTL text in RTL UI (where text is right aligned) unless > we > > special handling it. > > I've added comments. > > AFAIK you should be able to always set width and just keep doing what you are > doing. If you set width or if you don't, eitherway for LTR text, start position > is at x=0. Always set width is easier for my purpose. After which, I do not need ToViewPoint and ToTextPoint for the point conversion. But SetupPangoLayout is a shared routine. in r91881, the set width part changed. http://src.chromium.org/viewvc/chrome/trunk/src/ui/gfx/canvas_skia_linux.cc?r... Prior r91881, the width is set when UI is RTL. After r91881, the width is set when text is in right-to-left directionality. After r91881, if UI is RTL, width is set for RTL text, but width is not set for LTR text. For LTR text in RTL UI, for example, when click on the text, the pass-in parameter is a point relative to viewport, it need to be converted into a point relative to text area if width is not set. That is what I did, and I am resetting the width to -1 so that handling RTL text in RTL UI is consistent in terms of position conversion. BTW, the position conversion I mean is the position relative to text area and the position relative to viewport. It is not needed only when UI is LTR. It is needed when UI is RTL (where the text is right aligned) no matter the text is LTR or RTL, unless the width is set.
Thanks for the review! http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:20: // it should also massage A in the conversion. On 2011/09/06 16:05:03, behdad_google wrote: > Right. Do we have the background color around? If we do, I can provide the > massaging. Thanks for the offering. Looks like the background is always white (unless in selection where I set background and foreground separately). And the Alpha is always full. http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:76: pango_find_base_dir(layout_text, strlen(layout_text)); On 2011/09/06 16:05:03, behdad_google wrote: > Just pass -1 for length here. Done. http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:85: pango_layout_get_pixel_size(layout, &width, &height); On 2011/09/06 16:05:03, behdad_google wrote: > You can pass NULL for height. Done. http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:102: cairo_clip(cr); On 2011/09/06 16:05:03, behdad_google wrote: > I have a feeling that you shouldn't need the skia clipping then. Or does that > affect scoped_platform_paint()? hm... Looking at CanvasSkia::DrawStringInt() again, seems that it does not clip canvas. I am not very clear on this part. Why it is not needed? And does it cause problem if clip skia? I changed getting cairo_t from skia::BeginPlatformPaint(canvas_skia) to scoped_platform_paint. But I am not aware difference between them in terms of clipping canvas. http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:118: if (!bounds.IsEmpty()) On 2011/09/06 16:05:03, behdad_google wrote: > The second if is redundant since it's checked in the same if. Thanks for catching this. Actually, we do not check !bounds.IsEmpty(), we will draw cursor even it is width is 0. I removed this condition. http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:150: Utf8IndexToUtf16Index(caret_pos), On 2011/09/06 16:05:03, behdad_google wrote: > I'm lost about what the two indices to SelectionModel() are. Hope the comments in render_text.h explains. I will update the comments it anything not clear enough. http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:155: bool insert_mode) { On 2011/09/06 16:05:03, behdad_google wrote: > What does insert_mode==FALSE represent exactly? Confused. when insert mode is true, cursor is drawn as a vertical line. When insert_mode == FALSE, cursor bounds is a rectangular around the character that logical cursor points to. For example "abcd", when cursor position 2, cursor bounds is the rectangular around character 'c'. http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:166: (!insert_mode && pos.width < 0)) On 2011/09/06 16:05:03, behdad_google wrote: > I can't understand why you need this if(). Why don't we handle rtl in > insert_mode? And why don't we handle trailing in rtl mode? The conditions look > wrong to me. I do not quite understand your comments. As to the above condition: 1. in insert mode, cursor is drawn as a vertical line. If caret_placement is trailing, I need to adjust the x-axis to trailing position. For ltr character, pos.width > 0, x advances to character's trailing. For rtl character, pos.width < 0, x backwards to character's trailing. In non-insert-mode, x is moved to the left side of the rectangular. http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:168: x = x / PANGO_SCALE; On 2011/09/06 16:05:03, behdad_google wrote: > Use PANGO_PIXELS(). Done. http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:170: int h = std::min(display_rect().height(), pos.height / PANGO_SCALE); On 2011/09/06 16:05:03, behdad_google wrote: > Use PANGO_PIXELS(). Done. http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:209: if (first_visual_run) { On 2011/09/06 16:05:03, behdad_google wrote: > I would change the if to "if (layout_line_->runs)", and define first_visual_run > as PangoLayoutRun * inside the if block. Done. http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.h File ui/gfx/render_text_linux.h (right): http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:54: GSList* GetRunContainingPosition(size_t position) const; On 2011/09/06 16:05:03, behdad_google wrote: > Return PangoLayoutRun* instead? The comments about the 3 functions using PangoLayoutRun* instead of GSList* make sense. But the problems are: 1. the return value of GetRunContainingPosition() is also used to get previous and next run. Only GSList carries the links, PangoLayoutLine does not carry the link. http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:95: GSList* GetPreviousRun(GSList* run) const; On 2011/09/06 16:05:03, behdad_google wrote: > So, you search within the current line, right? I think PangoLayoutRun* both for > input and output is more appropriate. input can not be PangoLayoutLine since it does not carry links. So, to keep it consistent, I still keep the output as GSList . http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:99: GSList* GetLastRun() const; On 2011/09/06 16:05:03, behdad_google wrote: > Same here. And in any other place a single run is referenced. This could be changed to PangoLayoutLine*. I have not changed it yet to keep it consistent with the other functions returning GSList. If you think it is better to use PangoLayoutLine, I will change. http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:106: PangoLayoutLine* layout_line_; On 2011/09/06 16:05:03, behdad_google wrote: > Something like current_line_ may be more appropriate. Humm, or do we only care > about single-line paragraphs? If that's the case, maybe call > pango_layout_set_single_paragraph_mode() too. Done. It is single line mode. Does pango_layout_set_single_paragraph_mode() save performance as well?
On 2011/09/06 19:54:12, xji wrote: > Thanks for the review! > > http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc > File ui/gfx/render_text_linux.cc (right): > > http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... > hm... Looking at CanvasSkia::DrawStringInt() again, seems that it does not clip > canvas. > I am not very clear on this part. > Why it is not needed? You are clipping in skia, and clipping in cairo. Quite possible one should be enough. > And does it cause problem if clip skia? No. No big deal. > I changed getting cairo_t from skia::BeginPlatformPaint(canvas_skia) to > scoped_platform_paint. But I am not aware difference between them in terms of > clipping canvas. Me neither. We don't have to worry about this now though. > http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... > ui/gfx/render_text_linux.cc:150: Utf8IndexToUtf16Index(caret_pos), > On 2011/09/06 16:05:03, behdad_google wrote: > > I'm lost about what the two indices to SelectionModel() are. > > Hope the comments in render_text.h explains. I will update the comments it > anything not clear enough. Ok, I wasn't sure what caret_pos_ is. It's clear now. I don't like the SelectionModel approach, but that's separate from the Pango implementation so I'll bring that up separately. > http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... > ui/gfx/render_text_linux.cc:155: bool insert_mode) { > On 2011/09/06 16:05:03, behdad_google wrote: > > What does insert_mode==FALSE represent exactly? Confused. > > when insert mode is true, cursor is drawn as a vertical line. > When insert_mode == FALSE, cursor bounds is a rectangular around the character > that logical cursor points to. > For example "abcd", when cursor position 2, cursor bounds is the rectangular > around character 'c'. Ok, my bad. I had forgotten that we have to support a replace mode too. > http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... > ui/gfx/render_text_linux.cc:166: (!insert_mode && pos.width < 0)) > On 2011/09/06 16:05:03, behdad_google wrote: > > I can't understand why you need this if(). Why don't we handle rtl in > > insert_mode? And why don't we handle trailing in rtl mode? The conditions > look > > wrong to me. > > I do not quite understand your comments. > As to the above condition: > 1. in insert mode, cursor is drawn as a vertical line. > If caret_placement is trailing, I need to adjust the x-axis to trailing > position. For ltr character, pos.width > 0, x advances to character's trailing. > For rtl character, pos.width < 0, x backwards to character's trailing. I'll go back and restart the review based on this. > In non-insert-mode, x is moved to the left side of the rectangular. I have a vague memory that you would need special-handling for replace-mode cursor movement. It's tricky and different from insertion-mode. I'll play with it after you commit and report any issues. > http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.h#... > ui/gfx/render_text_linux.h:54: GSList* GetRunContainingPosition(size_t position) > const; > On 2011/09/06 16:05:03, behdad_google wrote: > > Return PangoLayoutRun* instead? > > The comments about the 3 functions using PangoLayoutRun* instead of GSList* make > sense. > > But the problems are: > 1. the return value of GetRunContainingPosition() is also used to get previous > and next run. > Only GSList carries the links, PangoLayoutLine does not carry the link. The GSList is PangoLayoutLine::runs, isn't it?! In fact, you are already using that. The GSList is singly-linked, so there is no way you can use it to go to the left run. > http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.h#... > ui/gfx/render_text_linux.h:95: GSList* GetPreviousRun(GSList* run) const; > On 2011/09/06 16:05:03, behdad_google wrote: > > So, you search within the current line, right? I think PangoLayoutRun* both > for > > input and output is more appropriate. > > input can not be PangoLayoutLine since it does not carry links. Not PangoLayoutLine, but PangoLayoutRun. You have the line internally, and the line has all the runs. Ah, ok, I think I understand what you mean. Currently you have this code: 540 GSList* RenderTextLinux::GetPreviousRun(GSList* run) const { 541 GSList* current = layout_line_->runs; 542 GSList* prev = NULL; 543 while (current) { 544 if (current == run) 545 return prev; 546 prev = current; 547 current = current->next; 548 } 549 return NULL; 550 } What I'm suggesting is: 540 PangoLayoutRun* RenderTextLinux::GetPreviousRun(PangoLayoutRun* run) const { 541 GSList* current = layout_line_->runs; 542 GSList* prev = NULL; 543 while (current) { 544 if (static_cast<PangoLayoutRun*>(current->data) == run) 545 return static_cast<PangoLayoutRun*>(prev)->data; 546 prev = current; 547 current = current->next; 548 } 549 return NULL; 550 } Makes sense? > So, to keep it consistent, I still keep the output as GSList . > > http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.h#... > ui/gfx/render_text_linux.h:99: GSList* GetLastRun() const; > On 2011/09/06 16:05:03, behdad_google wrote: > > Same here. And in any other place a single run is referenced. > > This could be changed to PangoLayoutLine*. You mean PangoLayoutRun* I assume. > http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.h#... > ui/gfx/render_text_linux.h:106: PangoLayoutLine* layout_line_; > On 2011/09/06 16:05:03, behdad_google wrote: > > Something like current_line_ may be more appropriate. Humm, or do we only > care > > about single-line paragraphs? If that's the case, maybe call > > pango_layout_set_single_paragraph_mode() too. > > Done. > It is single line mode. > Does pango_layout_set_single_paragraph_mode() save performance as well? Humm, I think line breaking may not be run. But I don't think performance is a big-deal with thte short strings we are dealing with here anyway.
So, here's the next batch. Nothing major, but there's some cleanup you can do. The remaining is mostly validating the SelectionModel interaction, but that takes much longer, so has to wait till tomorrow morning since I have to leave to drive back to Toronto now. behdad http://codereview.chromium.org/7511029/diff/83001/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/7511029/diff/83001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:283: } while (ch && *ch && !log_attrs[char_offset].is_cursor_position); I would change this while condition to: while (layout_text < ch && !log_attrs[char_offset].is_cursor_position); http://codereview.chromium.org/7511029/diff/83001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:285: ch = layout_text; Then you shouldn't need this. http://codereview.chromium.org/7511029/diff/83001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:292: } while (ch && *ch && !log_attrs[char_offset].is_cursor_position); I would then do this one as: while (ch < layout_text + text_len && !log_attrs[char_offset].is_cursor_position); http://codereview.chromium.org/7511029/diff/83001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:294: ch = layout_text + strlen(layout_text); You do three strlen()s in this block alone! Save the length and reuse please. http://codereview.chromium.org/7511029/diff/83001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:298: size_t utf8_index = static_cast<size_t>(ch - layout_text); So, you maintain both ch and char_offset. You can simplify the code by not tracking ch in the while loops, and do a single g_utf8_offset_to_pointer() at the end to move ch to the correct position. http://codereview.chromium.org/7511029/diff/83001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:299: g_free(log_attrs); Ok, this is no good. Please save log_attrs in the controller. While there, perhaps simply save layout_text and layout_text_len too. Since you have EnsureLayout() and ResetLayout(), keeping those updated is supereasy. http://codereview.chromium.org/7511029/diff/83001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:319: const PangoItem* run) const { Would be easier to call PangoItem instances item instead of run, to differentiate between PangoLayoutRun and PangoItem. Alternatively, just make the API get PangoLayoutRun and use run->item instead. http://codereview.chromium.org/7511029/diff/83001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:343: SelectionModel RenderTextLinux::LeftSelectionModel( I need much more time to review this one for correctness, so that has to wait till tomorrow morning.
On 2011/09/06 21:40:37, behdad_google wrote: > On 2011/09/06 19:54:12, xji wrote: > > Thanks for the review! > > > > http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc > > File ui/gfx/render_text_linux.cc (right): > > > > > http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.cc... > > hm... Looking at CanvasSkia::DrawStringInt() again, seems that it does not > clip > > canvas. > > I am not very clear on this part. > > Why it is not needed? > > You are clipping in skia, and clipping in cairo. Quite possible one should be > enough. > > > > And does it cause problem if clip skia? > > No. No big deal. > > > > I changed getting cairo_t from skia::BeginPlatformPaint(canvas_skia) to > > scoped_platform_paint. But I am not aware difference between them in terms of > > clipping canvas. > > Me neither. We don't have to worry about this now though. changed to not clip skia. > http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.h#... > > ui/gfx/render_text_linux.h:54: GSList* GetRunContainingPosition(size_t > position) > > const; > > On 2011/09/06 16:05:03, behdad_google wrote: > > > Return PangoLayoutRun* instead? > > > > The comments about the 3 functions using PangoLayoutRun* instead of GSList* > make > > sense. > > > > But the problems are: > > 1. the return value of GetRunContainingPosition() is also used to get previous > > and next run. > > Only GSList carries the links, PangoLayoutLine does not carry the link. > > The GSList is PangoLayoutLine::runs, isn't it?! In fact, you are already using > that. > The GSList is singly-linked, so there is no way you can use it to go to the left > run. > > Ah, you are right. I do not need the link when I search for previous. But I need it for next. The return value will be used to get right run too, so, I did not change the signature of this one. > > > http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.h#... > > ui/gfx/render_text_linux.h:95: GSList* GetPreviousRun(GSList* run) const; > > On 2011/09/06 16:05:03, behdad_google wrote: > > > So, you search within the current line, right? I think PangoLayoutRun* both > > for > > > input and output is more appropriate. > > > > input can not be PangoLayoutLine since it does not carry links. > > Not PangoLayoutLine, but PangoLayoutRun. You have the line internally, and the > line has all the runs. Ah, ok, I think I understand what you mean. Currently > you have this code: > > 540 GSList* RenderTextLinux::GetPreviousRun(GSList* run) const { > 541 GSList* current = layout_line_->runs; > 542 GSList* prev = NULL; > 543 while (current) { > 544 if (current == run) > 545 return prev; > 546 prev = current; > 547 current = current->next; > 548 } > 549 return NULL; > 550 } > > What I'm suggesting is: > > 540 PangoLayoutRun* RenderTextLinux::GetPreviousRun(PangoLayoutRun* run) const > { > 541 GSList* current = layout_line_->runs; > 542 GSList* prev = NULL; > 543 while (current) { > 544 if (static_cast<PangoLayoutRun*>(current->data) == run) > 545 return static_cast<PangoLayoutRun*>(prev)->data; > 546 prev = current; > 547 current = current->next; > 548 } > 549 return NULL; > 550 } > > Makes sense? You right. I do not need the link to get previous run. Changed per suggestion. > > > > So, to keep it consistent, I still keep the output as GSList . > > > > > http://codereview.chromium.org/7511029/diff/79001/ui/gfx/render_text_linux.h#... > > ui/gfx/render_text_linux.h:99: GSList* GetLastRun() const; > > On 2011/09/06 16:05:03, behdad_google wrote: > > > Same here. And in any other place a single run is referenced. > > > > This could be changed to PangoLayoutLine*. > > You mean PangoLayoutRun* I assume. Yes, I mean PangoLayoutRun*, changed per suggestions.
As we discussed, I am going to commit the current version. Please continue to review and report any issues when testing. Thanks! http://codereview.chromium.org/7511029/diff/83001/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/7511029/diff/83001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:283: } while (ch && *ch && !log_attrs[char_offset].is_cursor_position); On 2011/09/06 21:57:17, behdad_google wrote: > I would change this while condition to: > > while (layout_text < ch && !log_attrs[char_offset].is_cursor_position); Done. http://codereview.chromium.org/7511029/diff/83001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:285: ch = layout_text; On 2011/09/06 21:57:17, behdad_google wrote: > Then you shouldn't need this. Done. http://codereview.chromium.org/7511029/diff/83001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:292: } while (ch && *ch && !log_attrs[char_offset].is_cursor_position); On 2011/09/06 21:57:17, behdad_google wrote: > I would then do this one as: > > while (ch < layout_text + text_len && > !log_attrs[char_offset].is_cursor_position); Done. http://codereview.chromium.org/7511029/diff/83001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:294: ch = layout_text + strlen(layout_text); On 2011/09/06 21:57:17, behdad_google wrote: > You do three strlen()s in this block alone! Save the length and reuse please. Done. http://codereview.chromium.org/7511029/diff/83001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:298: size_t utf8_index = static_cast<size_t>(ch - layout_text); On 2011/09/06 21:57:17, behdad_google wrote: > So, you maintain both ch and char_offset. You can simplify the code by not > tracking ch in the while loops, and do a single g_utf8_offset_to_pointer() at > the end to move ch to the correct position. This function is re-written by only use char_offset in the loop. http://codereview.chromium.org/7511029/diff/83001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:299: g_free(log_attrs); On 2011/09/06 21:57:17, behdad_google wrote: > Ok, this is no good. Please save log_attrs in the controller. While there, > perhaps simply save layout_text and layout_text_len too. Since you have > EnsureLayout() and ResetLayout(), keeping those updated is supereasy. Done. http://codereview.chromium.org/7511029/diff/83001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:319: const PangoItem* run) const { On 2011/09/06 21:57:17, behdad_google wrote: > Would be easier to call PangoItem instances item instead of run, to > differentiate between PangoLayoutRun and PangoItem. Alternatively, just make > the API get PangoLayoutRun and use run->item instead. Done.
A few more. http://codereview.chromium.org/7511029/diff/83016/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/7511029/diff/83016/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:220: return SelectionModel(0, 0, SelectionModel::LEADING); Humm, if line is LTR and first run RTL, then LeftEndSelectionModel cannot have caret=0, right? http://codereview.chromium.org/7511029/diff/83016/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:234: return SelectionModel(text().length(), caret, SelectionModel::LEADING); Shouldn't the first argument also be caret instead of text().length()? http://codereview.chromium.org/7511029/diff/83016/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:289: ch = g_utf8_offset_to_pointer(layout_text_, char_offset); You can make this slightly faster by doing: ch = g_utf8_offset_to_pointer(ch, char_offset - old_char_offset); http://codereview.chromium.org/7511029/diff/83016/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:453: pango_layout_set_height(layout_, display_rect().height() * PANGO_SCALE); You don't really need this one. You want one and exactly one line, so just leave it at default -1 which means one line. Plus, this has no effect whatsoever unless you set an ellipsizing mode.
http://codereview.chromium.org/7511029/diff/83016/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/7511029/diff/83016/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:220: return SelectionModel(0, 0, SelectionModel::LEADING); On 2011/09/07 18:11:36, behdad_google wrote: > Humm, if line is LTR and first run RTL, then LeftEndSelectionModel cannot have > caret=0, right? in chrome linux and chrome os, the text's directionality is determined by its first strong character's directionality (text directionality is auto-set by pango, not manually set as in windows where text directionality is determined by UI's directionality). I did not change that, which means the line can not be LTR if first run is RTL. http://codereview.chromium.org/7511029/diff/83016/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:234: return SelectionModel(text().length(), caret, SelectionModel::LEADING); On 2011/09/07 18:11:36, behdad_google wrote: > Shouldn't the first argument also be caret instead of text().length()? This is the END position. for example, logical text "abcABC", visually "abcCBA", logical cursor should be 6, while (caret_pos, caret_placement) is (3, leading). It is different from clicking right half of 'A'. http://codereview.chromium.org/7511029/diff/83016/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:289: ch = g_utf8_offset_to_pointer(layout_text_, char_offset); On 2011/09/07 18:11:36, behdad_google wrote: > You can make this slightly faster by doing: > > ch = g_utf8_offset_to_pointer(ch, char_offset - old_char_offset); Done. http://codereview.chromium.org/7511029/diff/83016/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:453: pango_layout_set_height(layout_, display_rect().height() * PANGO_SCALE); On 2011/09/07 18:11:36, behdad_google wrote: > You don't really need this one. You want one and exactly one line, so just > leave it at default -1 which means one line. > > Plus, this has no effect whatsoever unless you set an ellipsizing mode. removed.
http://codereview.chromium.org/7511029/diff/84006/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/7511029/diff/84006/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:29: // to avoid conversion. Alternatively, we can just cache the mapping between U16 and U8 indices, if they are used often. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
