Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8747001:
Reintroduce password support to NativeTextfieldViews (Closed)
|
Created:
9 years ago by benrg Modified:
8 years, 9 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, tfarina, penghuang+watch_chromium.org, Paweł Hajdan Jr., James Su, dhollowa, jungshik at Google Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSupport for password entry fields, with all characters
displayed as '*', was implemented in NativeTextfieldViews
via TextfieldViewsModel::GetVisibleText() (issue 5857002),
but the refactoring into RenderText (issue 7265011) removed
all calls of that method.
I moved the password handling code into RenderText because I
think it's logically a visual style (although RenderText
also contains a lot of model-related stuff right now).
RenderText now has a password property (in earlier patchsets
it was an attribute in StyleRange).
BUG=105054
TEST=unit tests and manual testing of the wifi login dialog
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=124611
Patch Set 1 : rerebase #
Total comments: 2
Patch Set 2 : fix EXPECT_EQ order #Patch Set 3 : make ApplyStyleRange fail if default_style_.password #Patch Set 4 : make password in RenderText an instance property and disable cut, copy, D&D, and word skipping #
Total comments: 23
Patch Set 5 : rebase, const syntax, password->obscured, dead methods, DCHECK, Ctrl+[CX] tests, fix other tests #
Total comments: 10
Patch Set 6 : delete more dead code, address recent comments #
Total comments: 2
Patch Set 7 : rebase #Patch Set 8 : new implementation, linux only #
Total comments: 49
Patch Set 9 : comments, Utf16OffsetToPointer bug #
Total comments: 16
Patch Set 10 : rebase to r123531, no other changes (not for review) #Patch Set 11 : minor unit test change #Patch Set 12 : tweak NativeTextfieldViews::ExecuteCommand #
Total comments: 15
Patch Set 13 : more documentation in Utf16OffsetToIndex #Patch Set 14 : documentation changes and minor fixes #Patch Set 15 : move helpers from base/ to ui/base/ #Patch Set 16 : rebase #Patch Set 17 : fix windows test #Patch Set 18 : for dcommit #
Messages
Total messages: 33 (0 generated)
Oshima, could you look at this? msw would be another possibility.
I think msw should review this too, and please implement this for windows as well. One potential issue is that password style can now be overwritten by applying custom style. Can you check and reject applying custom style if default style has password? http://codereview.chromium.org/8747001/diff/6001/ui/gfx/render_text_unittest.cc File ui/gfx/render_text_unittest.cc (right): http://codereview.chromium.org/8747001/diff/6001/ui/gfx/render_text_unittest.... ui/gfx/render_text_unittest.cc:295: EXPECT_EQ(render_text->GetCensoredText(), ASCIIToUTF16("ho******op")); EXPECT_EQ(<expected>, <actual>)
I'll work on a Windows implementation. I don't have a working Windows Aura build yet, but I should soon. Are there any password dialogs in Windows Aura? > One potential issue is that password style can now be overwritten by applying > custom style. Can you check and reject applying custom style if default style > has password? In this design callers are supposed to set the password bit by applying a style, and it's not clear to me when that kind of request ought to be rejected -- maybe the caller wants to obscure the password part of a URL or something. If you think this could be an issue then maybe I should just move the password bit from StyleRange to a member variable with its own setter. Alternatively, I could add a bitmask parameter to ApplyStyleRange (APPLY_STYLE_FONT, APPLY_STYLE_PASSWORD, etc.), which would fix some todos in the code as well. I think that would be easy to do, though it would obviously complicate this CL. http://codereview.chromium.org/8747001/diff/6001/ui/gfx/render_text_unittest.cc File ui/gfx/render_text_unittest.cc (right): http://codereview.chromium.org/8747001/diff/6001/ui/gfx/render_text_unittest.... ui/gfx/render_text_unittest.cc:295: EXPECT_EQ(render_text->GetCensoredText(), ASCIIToUTF16("ho******op")); On 2011/12/01 21:36:47, oshima wrote: > EXPECT_EQ(<expected>, <actual>) Done.
On 2011/12/01 22:36:15, benrg wrote: > I'll work on a Windows implementation. I don't have a working Windows Aura build > yet, but I should soon. Are there any password dialogs in Windows Aura? > > > One potential issue is that password style can now be overwritten by applying > > custom style. Can you check and reject applying custom style if default style > > has password? > > In this design callers are supposed to set the password bit by applying a style, > and it's not clear to me when that kind of request ought to be rejected -- maybe > the caller wants to obscure the password part of a URL or something. I consider that if default one has password, intention is clear that you want to hide entire text, always. If some of style has password=true, then it is caller's responsibility to make sure the style is set correctly. (and should have some test cases for that) If you > think this could be an issue then maybe I should just move the password bit from > StyleRange to a member variable with its own setter. I actually do like password in style, and think it's useful in general, although I'm not 100% sure if we need it in views (probably not). I'm ok if you want to move to a bit. I'll leave it to msw. > Alternatively, I could add > a bitmask parameter to ApplyStyleRange (APPLY_STYLE_FONT, APPLY_STYLE_PASSWORD, > etc.), which would fix some todos in the code as well. I think that would be > easy to do, though it would obviously complicate this CL. > > http://codereview.chromium.org/8747001/diff/6001/ui/gfx/render_text_unittest.cc > File ui/gfx/render_text_unittest.cc (right): > > http://codereview.chromium.org/8747001/diff/6001/ui/gfx/render_text_unittest.... > ui/gfx/render_text_unittest.cc:295: EXPECT_EQ(render_text->GetCensoredText(), > ASCIIToUTF16("ho******op")); > On 2011/12/01 21:36:47, oshima wrote: > > EXPECT_EQ(<expected>, <actual>) > > Done.
> I consider that if default one has password, intention is clear that you want > to hide entire text, always. Yeah, I guess that makes sense. I made ApplyStyleRange a no-op when default_style_.password is true (since any local style change could leak information, even if its password bit is set). ApplyCompositionAndSelectionStyles still works, of course.
We'll never need a portion of the text to be a password while the rest is plain text. We should just have a RenderText-level password property; that will likely simplify implementation anyway. Also, I don't think we should land something that obscures passwords visually but leaves them open to copy/cut/paste.
+xji for RenderText[Linux].
The password flag is now an instance variable. In terms of code side it's pretty much a wash because of the extra complexity of special-casing what was formerly just another style bit. ApplyStyleRange can potentially leak data now by coloring/underlining parts of the text differently. I thought about using only the default style when the password flag is set, but it seems unlikely to be an issue in practice, and the added complexity would have made the old implementation a clear win. :-) Also with this patch set it's no longer possible to cut, copy or drag-and-drop text out of a password field, or to "see" word boundaries. Windows support still isn't implemented.
http://codereview.chromium.org/8747001/diff/15001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/8747001/diff/15001/ui/gfx/render_text.h#newcode86 ui/gfx/render_text.h:86: void SetIsPassword(bool password); I thought we wanted to change "password" to something not password? http://codereview.chromium.org/8747001/diff/15001/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/8747001/diff/15001/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:565: if (editable) DCHECK to make sure this path never gets executed when obscured. http://codereview.chromium.org/8747001/diff/15001/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views_unittest.cc (right): http://codereview.chromium.org/8747001/diff/15001/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views_unittest.cc:516: EXPECT_FALSE(IsCommandIdEnabled(IDS_APP_COPY)); can you also add ctrl-c/ctrl-x?
Looking better, thanks for iterating! http://codereview.chromium.org/8747001/diff/15001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/8747001/diff/15001/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:21: const char16 PASSWORD_REPLACEMENT_CHAR = '*'; Is this the correct style for a const char identifier? I would have guess it'd be kPasswordReplacementChar. http://codereview.chromium.org/8747001/diff/15001/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:379: return default_style_.font.GetStringWidth(text()); Shouldn't this use GetCensoredText()? http://codereview.chromium.org/8747001/diff/15001/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:396: int right = font.GetStringWidth(text()); Shouldn't this use GetCensoredText()? http://codereview.chromium.org/8747001/diff/15001/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:407: int pivot = font.GetStringWidth(text().substr(0, pivot_pos)); Shouldn't this use GetCensoredText()? http://codereview.chromium.org/8747001/diff/15001/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8747001/diff/15001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:240: base::i18n::GetFirstStrongCharacterDirection(text()), We should test this behavior with RTL passwords. http://codereview.chromium.org/8747001/diff/15001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:636: u_strToUTF8(NULL, 0, &utf8_index, text().data(), index, &ec); Should this use GetCensoredText()? http://codereview.chromium.org/8747001/diff/15001/ui/views/controls/textfield... File ui/views/controls/textfield/textfield_views_model_unittest.cc (right): http://codereview.chromium.org/8747001/diff/15001/ui/views/controls/textfield... ui/views/controls/textfield/textfield_views_model_unittest.cc:519: model.MoveCursorRight(gfx::WORD_BREAK, false); Do we care to test the cursor movement in a password field (make sure surrogate pairs, move-by-word, etc. are handled as expected?) http://codereview.chromium.org/8747001/diff/15001/ui/views/controls/textfield... ui/views/controls/textfield/textfield_views_model_unittest.cc:543: EXPECT_STR_EQ("HELLO HELLO WORLD", clipboard_text); Shouldn't this fail? Why is the password text copied successfully?
http://codereview.chromium.org/8747001/diff/15001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/8747001/diff/15001/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:379: return default_style_.font.GetStringWidth(text()); On 2011/12/03 00:22:40, msw wrote: > Shouldn't this use GetCensoredText()? guess since it could be pure virtual, it does not make differences here. but it is consistent to change them too. http://codereview.chromium.org/8747001/diff/15001/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8747001/diff/15001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:636: u_strToUTF8(NULL, 0, &utf8_index, text().data(), index, &ec); On 2011/12/03 00:22:40, msw wrote: > Should this use GetCensoredText()? agree.
Thanks for all the feedback. I'm uploading a new draft and publishing comments since it's been a while, but this is still a work in progress. The things I didn't reply to are things I haven't looked at yet. http://codereview.chromium.org/8747001/diff/15001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/8747001/diff/15001/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:21: const char16 PASSWORD_REPLACEMENT_CHAR = '*'; > Is this the correct style for a const char identifier? > I would have guess it'd be kPasswordReplacementChar. Done. (I misremembered the coding guidelines.) http://codereview.chromium.org/8747001/diff/15001/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:379: return default_style_.font.GetStringWidth(text()); > Shouldn't this use GetCensoredText()? This code is dead, see below. http://codereview.chromium.org/8747001/diff/15001/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:396: int right = font.GetStringWidth(text()); > Shouldn't this use GetCensoredText()? This code is dead and seems unlikely to ever be resurrected (it's not bidi-aware). Originally this CL deleted it. Then, because there was so much dead code, I split it into a separate CL (issue 8742004). Then issue 8536047 landed, making 8742004 obsolete. But it looks like 8536047 didn't remove all of the dead code. I'll just delete RenderText::GetStringWidth and RenderText::FindCursorPosition in this CL unless there's a reason not to. http://codereview.chromium.org/8747001/diff/15001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/8747001/diff/15001/ui/gfx/render_text.h#newcode86 ui/gfx/render_text.h:86: void SetIsPassword(bool password); > I thought we wanted to change "password" to something not password? Done. I originally split off issue 8748001 because it was independent, but now it's starting to overlap... http://codereview.chromium.org/8747001/diff/15001/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/8747001/diff/15001/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:565: if (editable) > DCHECK to make sure this path never gets executed when obscured. Done (is this okay?) http://codereview.chromium.org/8747001/diff/15001/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views_unittest.cc (right): http://codereview.chromium.org/8747001/diff/15001/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views_unittest.cc:516: EXPECT_FALSE(IsCommandIdEnabled(IDS_APP_COPY)); On 2011/12/02 23:15:54, oshima wrote: > can you also add ctrl-c/ctrl-x? Done. http://codereview.chromium.org/8747001/diff/15001/ui/views/controls/textfield... File ui/views/controls/textfield/textfield_views_model_unittest.cc (right): http://codereview.chromium.org/8747001/diff/15001/ui/views/controls/textfield... ui/views/controls/textfield/textfield_views_model_unittest.cc:543: EXPECT_STR_EQ("HELLO HELLO WORLD", clipboard_text); > Shouldn't this fail? Why is the password text copied successfully? That was the text left in the clipboard from the previous copy. But you're right, these tests are broken since selecting words doesn't work any more when the password/obscured bit is set. I rewrote the whole thing in the latest update. It could still be broken.
http://codereview.chromium.org/8747001/diff/22001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/8747001/diff/22001/ui/gfx/render_text.h#newcode86 ui/gfx/render_text.h:86: void SetIsObscured(bool obscured); SetObscured http://codereview.chromium.org/8747001/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:297: // True if this is a password field. update the comment http://codereview.chromium.org/8747001/diff/22001/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (left): http://codereview.chromium.org/8747001/diff/22001/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:560: bool editable = !textfield_->read_only(); won't this break read_only behavior? http://codereview.chromium.org/8747001/diff/22001/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/8747001/diff/22001/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:537: return editable && model_->HasSelection() && !textfield_->IsPassword(); I assume you'll change IsPassword in another CL? http://codereview.chromium.org/8747001/diff/22001/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views_unittest.cc (right): http://codereview.chromium.org/8747001/diff/22001/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views_unittest.cc:523: EXPECT_STR_EQ("foo", string16(GetClipboardText())); check if textfield still has the same text.
The more I think about Utf16IndexToUtf8Index and friends, the more horrifying it becomes. This function and its friends exist because of two incompatible string-index standards in two different code bases. With obscured text we now have a third index format, and perhaps even a fourth if we ever used a non-ASCII replacement char. There's no longer just one index format per code base, so a conversion layer at the boundary isn't enough. And real text can be inserted and deleted in obscured fields, so real-text and fake-text indexes are "live" at the same time. My opinion is that strings are not arrays of characters (what's a character?), and most code has no business indexing them. The size_t indexes that currently appear in the code should be treated as abstract iterators outside of RenderText (and perhaps they already are). Then the only question is what representation is most useful to RenderTextLinux: 1. Cursorable glyph count (which should be the same as the asterisk count) 2. UTF-8 byte index in the real text. 3. UTF-16 word index in the real text. 4. Varying representation depending on whether the field is obscured. 5. list<string16>::iterator, where each string16 holds one selectable glyph... thing. #5 is not a serious proposal. I think #4 is a bad idea. I see no reason why RenderTextLinux couldn't store the text as UTF-8, eliminating the need to think about #3. I suppose #1 vs #2 is mostly an efficiency question. #1 seems more elegant, but #2 would be faster on non-password fields, I suppose. Any brilliant ideas? https://memegen.googleplex.com/2895830 http://codereview.chromium.org/8747001/diff/15001/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8747001/diff/15001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:240: base::i18n::GetFirstStrongCharacterDirection(text()), On 2011/12/03 00:22:40, msw wrote: > We should test this behavior with RTL passwords. What this is intended to do is display the whole *** field RTL if the password is (or starts out) RTL. Would always LTR be better? Matching the LTR/RTL ranges of the unobscured text would be possible but rather difficult... http://codereview.chromium.org/8747001/diff/15001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:636: u_strToUTF8(NULL, 0, &utf8_index, text().data(), index, &ec); On 2011/12/06 01:24:04, xji wrote: > On 2011/12/03 00:22:40, msw wrote: > > Should this use GetCensoredText()? > agree. See out-of-line comments. http://codereview.chromium.org/8747001/diff/15001/ui/views/controls/textfield... File ui/views/controls/textfield/textfield_views_model_unittest.cc (right): http://codereview.chromium.org/8747001/diff/15001/ui/views/controls/textfield... ui/views/controls/textfield/textfield_views_model_unittest.cc:519: model.MoveCursorRight(gfx::WORD_BREAK, false); On 2011/12/03 00:22:40, msw wrote: > Do we care to test the cursor movement in a password field (make sure surrogate > pairs, move-by-word, etc. are handled as expected?) Presumably yes, but not until it works. http://codereview.chromium.org/8747001/diff/22001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/8747001/diff/22001/ui/gfx/render_text.h#newcode86 ui/gfx/render_text.h:86: void SetIsObscured(bool obscured); On 2011/12/06 23:57:18, oshima wrote: > SetObscured Done. http://codereview.chromium.org/8747001/diff/22001/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:297: // True if this is a password field. On 2011/12/06 23:57:18, oshima wrote: > update the comment Done. http://codereview.chromium.org/8747001/diff/22001/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (left): http://codereview.chromium.org/8747001/diff/22001/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:560: bool editable = !textfield_->read_only(); On 2011/12/06 23:57:18, oshima wrote: > won't this break read_only behavior? I thought ExecuteCommand is only called when a menu item is chosen, which should never happen when IsCommandIdEnabled returns false. In the next patch set I added a release-build sanity check. If I'm missing something please let me know. http://codereview.chromium.org/8747001/diff/22001/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/8747001/diff/22001/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:537: return editable && model_->HasSelection() && !textfield_->IsPassword(); On 2011/12/06 23:57:18, oshima wrote: > I assume you'll change IsPassword in another CL? It's changed in 8748001, which should land someday... http://codereview.chromium.org/8747001/diff/22001/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views_unittest.cc (right): http://codereview.chromium.org/8747001/diff/22001/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views_unittest.cc:523: EXPECT_STR_EQ("foo", string16(GetClipboardText())); On 2011/12/06 23:57:18, oshima wrote: > check if textfield still has the same text. Done.
http://codereview.chromium.org/8747001/diff/35001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/8747001/diff/35001/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:151: // use non-BMP code points and combining diacritics in their passwords? Ben and I just talked about how to map obscured text and the underline text so that the backspace/delete works on the underline text on grapheme boundary. Think it again. I think that is wrong. For example, if the underline text is "abcd", while "ab" are 2 characters but one grapheme. The obscured text is "****", when cursor is at "**|**", if moving cursor left, we should not move cursor to "|****", which implies the first 2 "**" is one grapheme, it is probably against the security rule. The only one thing obscured text tells about password should be how many characters in the password, nothing more. Maybe that is the reason why windows, gtk etc. all move cursor from "**|**" to "*|***", and press "backspace" delete 'a' leaving the underline text as invalid. I think we should do the same, instead of fancy stuff that works on underline text grapheme boundary. Just talked to an Arabic-speaking co-worker, and he consented. But one thing he thinks is that we should show replacement characters on each Unicode character, not on each UTF16 code point.
http://codereview.chromium.org/8747001/diff/35001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/8747001/diff/35001/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:151: // use non-BMP code points and combining diacritics in their passwords? xji wrote: > For example, if the underline text is "abcd", while "ab" are 2 characters but > one grapheme. The obscured text is "****", when cursor is at "**|**", if > moving cursor left, we should not move cursor to "|****", which implies the > first 2 "**" is one grapheme, it is probably against the security rule. What I was thinking is that the obscured characters would be ***, the cursor would initially be at *|** (equivalent to ab|cd in the underlying text), and moving left would take it to |*** (equivalent to |abcd). On the other hand, if you do it that way and you don't want to leak the number of code points in the grapheme, then backspacing from *|** (ab|cd) has to take you to |** (|cd), even though backspacing from ab|cd takes you to a|cd. On the other other hand, displaying more than one asterisk per grapheme leaks information too. Either way, you're leaking a length, and probably neither kind of length is more sensitive than the other. I think that a strong password would not be significantly weakened by disclosure of any of this information. As we also discussed, both Windows and GTK do it the easy (to implement) way, which may be enough justification for doing it that way ourselves. Presumably, if anyone actually cared, Windows and/or GTK would have been "fixed" by now.
PTAL. This is a new implementation of STYLE_OBSCURED for Linux only. I wanted to
merge the RenderText{Linux,Win} cursoring code into RenderText and then hack
password support into that, but it's taking me too long and I think that this
will need to be crossported into M18, which means it needs to be as simple as
possible.
This implementation basically replaces Utf{16,8}IndexToUtf{8,16}Index with
{Text,Layout}IndexTo{Layout,Text}Index. The text index in an index into text(),
which always contains the real text as UTF-16, while the layout index is an
index into layout_text_, which contains either the real text as UTF-8 or a bunch
of '*'s.
The implementation of {Text,Layout}IndexTo{Layout,Text}Index is unnecessarily
complicated: it caches data so that algorithms that loop over the string run in
O(n) time instead of O(n^2). I think something like this is a good idea in the
long run, but it may be too much for the M18 port. The CL includes some other
unnecessary refactoring as well.
There's no Windows implementation because I think it's less urgent, it's harder
to get right because the Windows code isn't written with the assumption that
text and layout indexes are different, and a lot of it would become obsolete
when the code is merged into RenderText.
Great job! http://codereview.chromium.org/8747001/diff/55003/base/utf_offset_string_conv... File base/utf_offset_string_conversions.h (right): http://codereview.chromium.org/8747001/diff/55003/base/utf_offset_string_conv... base/utf_offset_string_conversions.h:18: BASE_EXPORT bool IsValidUtf16Index(const string16& s, size_t index); Maybe IsValidCharacterIndex or IsValidCodePointIndex is more accurate. http://codereview.chromium.org/8747001/diff/55003/base/utf_offset_string_conv... base/utf_offset_string_conversions.h:20: // |Utf16IndexToOffset| and |Utf16OffsetToIndex| are similar to GLib's Maybe add a comment like "Converts between UTF16 index and character offset". do we add "|" around function names? http://codereview.chromium.org/8747001/diff/55003/base/utf_offset_string_conv... base/utf_offset_string_conversions.h:26: // the string contains no surrogate pairs, Utf16IndexToOffset(s, i, j) == i - j == j - i http://codereview.chromium.org/8747001/diff/55003/base/utf_offset_string_conv... File base/utf_offset_string_conversions_unittest.cc (right): http://codereview.chromium.org/8747001/diff/55003/base/utf_offset_string_conv... base/utf_offset_string_conversions_unittest.cc:40: } Do you know how g_utf8_offset_to_pointer and g_utf8_pointer_to_offset handle invalid UTF16 (with non-paired surrogate)? http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:392: UpdateLayout(); guard all 3 statements under "if (obscured != obscured_)" http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:140: string16 GetDisplayText() const; change to protected. http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (left): http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:381: } will removing this cause performance hit if we do utf16 to utf8 conversion on every glyph during constant redrawing (although last_text_index_ and last_layout_pointer_ are cached)? http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:491: return NULL; Ah, nice! http://codereview.chromium.org/8747001/diff/55003/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/8747001/diff/55003/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:580: if (IsCommandIdEnabled(command_id)) { UpdateAfterChange() could be moved inside the 'if' statement. do we need "OnBeforeUserAction()" and "OnAfterUserAction" if "IsCommandIdEnabled() == false"? http://codereview.chromium.org/8747001/diff/55003/ui/views/controls/textfield... File ui/views/controls/textfield/textfield_views_model_unittest.cc (right): http://codereview.chromium.org/8747001/diff/55003/ui/views/controls/textfield... ui/views/controls/textfield/textfield_views_model_unittest.cc:530: EXPECT_FALSE(model.Cut()); s/Cut/Copy/ http://codereview.chromium.org/8747001/diff/55003/ui/views/controls/textfield... ui/views/controls/textfield/textfield_views_model_unittest.cc:555: // Paste (with obscured bit set; should be ignored). I feel the comment is a bit confusing. you mean "obscure flag is ignored in paste". not "paste is ignored with obscured bit set". http://codereview.chromium.org/8747001/diff/55003/ui/views/controls/textfield... ui/views/controls/textfield/textfield_views_model_unittest.cc:559: model.render_text()->SetObscured(true); keep the original test and add one with obscured == true.
+Alexei. Would it be simpler to implement this in NativeTextfieldViews or TextfieldViewsModel and just pass RenderText the already obscured text? Does this work in some reasonable manner for RTL/BiDi? http://codereview.chromium.org/8747001/diff/55003/base/utf_offset_string_conv... File base/utf_offset_string_conversions.cc (right): http://codereview.chromium.org/8747001/diff/55003/base/utf_offset_string_conv... base/utf_offset_string_conversions.cc:75: delta += IsValidUtf16Index(s, from++); This adds a bool? Can you make it a ternary and explicitly add 0 or 1? Same for lines 77, 84, and 86. http://codereview.chromium.org/8747001/diff/55003/base/utf_offset_string_conv... base/utf_offset_string_conversions.cc:89: DCHECK(offset == 0); DCHECK_EQ http://codereview.chromium.org/8747001/diff/55003/base/utf_offset_string_conv... File base/utf_offset_string_conversions.h (right): http://codereview.chromium.org/8747001/diff/55003/base/utf_offset_string_conv... base/utf_offset_string_conversions.h:21: // |g_utf8_pointer_to_offset| and |g_utf8_offset_to_pointer|, but using string16 I guess you're not converting a single position (like GLib's functions) so you can cache values in RenderTextLinux for efficiency. This seems like a bad solution; can't you cache offset string pointers in the places that loop over the string, or similar instead? If not, You should consult a file/dir owner to make sure this extra complexity is worth the efficiency gain in its single use case. I suspect these functions would belong somewhere less general. You should also call out the reasoning for the extra complexity here, otherwise, others might use the functions inefficiently, defeating the purpose. http://codereview.chromium.org/8747001/diff/55003/base/utf_offset_string_conv... base/utf_offset_string_conversions.h:28: BASE_EXPORT ptrdiff_t Utf16IndexToOffset(const string16& s, This function isn't converting an index to an offset, it's converting the difference between two indices into an offset. Perhaps rename this function Utf16IndicesToOffset or something that clarifies its actual operation. http://codereview.chromium.org/8747001/diff/55003/base/utf_offset_string_conv... base/utf_offset_string_conversions.h:31: BASE_EXPORT size_t Utf16OffsetToIndex(const string16& s, Similarly, this function isn't just converting an offset to an index, it's converting an offset from a position to an index... It's more complex than the function name conveys. http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:377: } else { Drop the braces and make the else block just follow the if/return. http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:379: static_cast<size_t>(Utf16IndexToOffset(text_, 0, text_.length())); Does this work on Win? If not, comment here or block this path. http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:103: int caret_pos_in_layout, trailing; Init these locals to 0, please. http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:111: selection_end, caret_pos, |caret_pos| belongs on its own line. http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text_linux.h File ui/gfx/render_text_linux.h (right): http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:73: // layout_text_. barify |layout_text_|; consider one-liner: // Convert between indices into text() and indices into |layout_text_|. http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:103: // |LayoutIndexToTextIndex|, cached so that algorithms that call these If either of these functions are called consecutively with indices further apart from one another than from the start of the string, then this has a negative performance impact, right? I don't like speculating on the severity of performance impacts, but without more info, I'm dubious that this is worth the added complexity. http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text_unittest.cc File ui/gfx/render_text_unittest.cc (right): http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:284: TEST_F(RenderTextTest, PasswordCensorship) { Also test more complex text, like RTL, BiDi, and surrogate pairs. http://codereview.chromium.org/8747001/diff/55003/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/8747001/diff/55003/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:312: if (!textfield_->enabled() || textfield_->IsObscured() || Why disable drag instead of just using GetDisplayText? http://codereview.chromium.org/8747001/diff/55003/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:556: return editable && model_->HasSelection() && !textfield_->IsObscured(); Why disable cut/copy instead of just using GetDisplayText? Same Q for key handling and TextfieldViewsModel::Cut|Copy changes. http://codereview.chromium.org/8747001/diff/55003/ui/views/controls/textfield... File ui/views/controls/textfield/textfield_views_model_unittest.cc (left): http://codereview.chromium.org/8747001/diff/55003/ui/views/controls/textfield... ui/views/controls/textfield/textfield_views_model_unittest.cc:520: // Test for copy: Empty selection. Why remove this portion of the test?
On 2012/02/22 00:33:25, msw wrote: > +Alexei. Would it be simpler to implement this in NativeTextfieldViews or > TextfieldViewsModel and just pass RenderText the already obscured text? I think that would be much better than building this into RenderText. RenderText already does a lot - adding more complexity is just making the code less maintainable. Presumably, if this is done outside of RenderText, all you'd need to know to draw the obscured text is the length of the text and the cursor position (at which visual character index is the cursor). So you could have the model have two RenderTexts - one for the underlying data and a second one that stores the obscure text and has its cursor position updated to match the underlying one. You could update the obscured RenderText with the correct length string and cursor at draw time, which sounds like it should be pretty simple.
No code changes yet, just responding to design comments. > Does this work in some reasonable manner for RTL/BiDi? There's one * per code point, displayed in logical order left to right regardless of the bidi directionality. The logical caret position or logical selection is preserved when switching STYLE_OBSCURED on and off. I think that's reasonable (it matches other platforms, except that Windows clears the selection). Alexei wrote: > On 2012/02/22 00:33:25, msw wrote: >> +Alexei. Would it be simpler to implement this in >> NativeTextfieldViews or TextfieldViewsModel and just pass >> RenderText the already obscured text? > > I think that would be much better than building this into > RenderText. RenderText already does a lot - adding more > complexity is just making the code less maintainable. It's true that RenderText does too much. Specifically, it does a lot of stuff related to cursoring and selection which ought to be in the model. If STYLE_OBSCURED is implemented in the model now and the cursoring code is moved to the model later (which I hope it will be) then we'll be in about the same situation as this CL with regard to code complexity. I still think the obscured style is a visual style, like white-on-white, and belongs with other visual styles, all convenience issues aside. I originally put it in StyleRange for that reason. On the other hand, putting it in the model would remove the platform dependency (which shouldn't exist, but it's otherwise too hard to fix in this CL). I'll implement it in the model and see how it goes. Unless I've convinced you to change your mind... > So you could have the model have two RenderTexts - one for > the underlying data and a second one that stores the > obscure text and has its cursor position updated to match > the underlying one. I think that wouldn't be necessary. You only need one RenderText, holding the real text or the asterisks as appropriate. http://codereview.chromium.org/8747001/diff/55003/base/utf_offset_string_conv... File base/utf_offset_string_conversions.h (right): http://codereview.chromium.org/8747001/diff/55003/base/utf_offset_string_conv... base/utf_offset_string_conversions.h:20: // |Utf16IndexToOffset| and |Utf16OffsetToIndex| are similar to GLib's On 2012/02/18 01:28:47, xji wrote: > do we add "|" around function names? I don't know the rules, but there are many function names surrounded with || in the source already. http://codereview.chromium.org/8747001/diff/55003/base/utf_offset_string_conv... base/utf_offset_string_conversions.h:21: // |g_utf8_pointer_to_offset| and |g_utf8_offset_to_pointer|, but using string16 On 2012/02/22 00:33:26, msw wrote: > I guess you're not converting a single position (like > GLib's functions) so you can cache values in > RenderTextLinux for efficiency. This seems like a bad > solution; can't you cache offset string pointers in the > places that loop over the string, or similar instead? These functions use (string16&, size_t) pairs where the glib functions use char*. Other than that, there's no difference: g_utf8_pointer_to_offset(s+i, s+j) <-> Utf16IndexToOffset(s, i, j) g_utf8_offset_to_pointer(s+i, d) <-> Utf16OffsetToIndex(s, i, d) I was going to use string16::iterator in place of char*, but that's unsafe (in the offset-to-index case) when the string might start with a low surrogate or end with a high surrogate. To detect the string boundary you need a reference to the string, and at that point it's easier (I think) to use size_t indices. This issue theoretically exists with g_utf8_offset_to_pointer as well, but not in the way I use it, since there's no way for an attacker to pass us bad UTF-8. http://codereview.chromium.org/8747001/diff/55003/base/utf_offset_string_conv... base/utf_offset_string_conversions.h:28: BASE_EXPORT ptrdiff_t Utf16IndexToOffset(const string16& s, On 2012/02/22 00:33:26, msw wrote: > This function isn't converting an index to an offset, it's > converting the difference between two indices into an > offset. Perhaps rename this function Utf16IndicesToOffset > or something that clarifies its actual operation. The same is true of g_utf8_pointer_to_offset. I had to choose between consistent and accurate naming. Which is not to say that I made the right choice. Utf16IndicesToOffset sounds reasonable, but it's still strange for the "offset" to be a number of code points rather than a number of array elements. http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text_linux.h File ui/gfx/render_text_linux.h (right): http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:103: // |LayoutIndexToTextIndex|, cached so that algorithms that call these On 2012/02/22 00:33:26, msw wrote: > I don't like speculating on the severity of performance > impacts, but without more info, I'm dubious that this is > worth the added complexity. Relative to caching offsets within each loop, it's less error prone (there are fewer chances to get the caching wrong) and it avoids mixing the caching optimization with whatever the loop is really trying to do. On the other hand its performance characteristics are not especially transparent, as you said. In any case, it doesn't need to be in this CL. I think I'll back it out. http://codereview.chromium.org/8747001/diff/55003/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/8747001/diff/55003/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:312: if (!textfield_->enabled() || textfield_->IsObscured() || On 2012/02/22 00:33:26, msw wrote: > Why disable drag instead of just using GetDisplayText? See below. http://codereview.chromium.org/8747001/diff/55003/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:556: return editable && model_->HasSelection() && !textfield_->IsObscured(); On 2012/02/22 00:33:26, msw wrote: > Why disable cut/copy instead of just using GetDisplayText? > Same Q for key handling and TextfieldViewsModel::Cut|Copy changes. I doubt that a user who cuts/copies/drags text from an obscured textbox wants a bunch of asterisks (or U+25CF "black circle"s, or whatever we might use in the future). The asterisks are not real. Personally I would rather allow cut/copy/drag (extracting the real text), but these are no-ops on other platforms and I think we agreed to replicate that behavior. Apparently the threat is an attacker who has access to the keyboard for long enough to copy-paste the password into Notepad, but not long enough to install a keylogger or run a window-spy utility.
I backed out all of my optimizations, so it may be easier to compare against the original source than the last patch. This is still a RenderText implementation. There was a bug in Utf16OffsetToIndex and the same bug in the unit test. :-( Now fixed. http://codereview.chromium.org/8747001/diff/55003/base/utf_offset_string_conv... File base/utf_offset_string_conversions.cc (right): http://codereview.chromium.org/8747001/diff/55003/base/utf_offset_string_conv... base/utf_offset_string_conversions.cc:75: delta += IsValidUtf16Index(s, from++); On 2012/02/22 00:33:26, msw wrote: > This adds a bool? Can you make it a ternary and explicitly add 0 or 1? > Same for lines 77, 84, and 86. Done. http://codereview.chromium.org/8747001/diff/55003/base/utf_offset_string_conv... File base/utf_offset_string_conversions.h (right): http://codereview.chromium.org/8747001/diff/55003/base/utf_offset_string_conv... base/utf_offset_string_conversions.h:18: BASE_EXPORT bool IsValidUtf16Index(const string16& s, size_t index); On 2012/02/18 01:28:47, xji wrote: > Maybe IsValidCharacterIndex or IsValidCodePointIndex is more accurate. Changed to IsValidCodePointIndex. http://codereview.chromium.org/8747001/diff/55003/base/utf_offset_string_conv... base/utf_offset_string_conversions.h:26: // the string contains no surrogate pairs, Utf16IndexToOffset(s, i, j) == i - j On 2012/02/18 01:28:47, xji wrote: > == j - i Oops. Fixed in rewritten documentation. http://codereview.chromium.org/8747001/diff/55003/base/utf_offset_string_conv... base/utf_offset_string_conversions.h:28: BASE_EXPORT ptrdiff_t Utf16IndexToOffset(const string16& s, On 2012/02/22 02:25:43, benrg wrote: > The same is true of g_utf8_pointer_to_offset. I had to choose between > consistent and accurate naming. Which is not to say that I made the > right choice. Utf16IndicesToOffset sounds reasonable, but it's still > strange for the "offset" to be a number of code points rather than a > number of array elements. I couldn't think of better names for the functions, but I renamed the parameters. I think it helps. http://codereview.chromium.org/8747001/diff/55003/base/utf_offset_string_conv... File base/utf_offset_string_conversions_unittest.cc (right): http://codereview.chromium.org/8747001/diff/55003/base/utf_offset_string_conv... base/utf_offset_string_conversions_unittest.cc:40: } On 2012/02/18 01:28:47, xji wrote: > Do you know how g_utf8_offset_to_pointer and g_utf8_pointer_to_offset handle > invalid UTF16 (with non-paired surrogate)? It's undocumented, but it doesn't matter, because UTF16ToUTF8 replaces unpaired surrogates with U+FFFD in the output. http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:377: } else { On 2012/02/22 00:33:26, msw wrote: > Drop the braces and make the else block just follow the if/return. Done. http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:379: static_cast<size_t>(Utf16IndexToOffset(text_, 0, text_.length())); On 2012/02/22 00:33:26, msw wrote: > Does this work on Win? If not, comment here or block this path. This method is currently not called on Windows (but would work if it was). http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:392: UpdateLayout(); On 2012/02/18 01:28:47, xji wrote: > guard all 3 statements under "if (obscured != obscured_)" Done. http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text.h#newcod... ui/gfx/render_text.h:140: string16 GetDisplayText() const; On 2012/02/18 01:28:47, xji wrote: > change to protected. Done. http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:103: int caret_pos_in_layout, trailing; On 2012/02/22 00:33:26, msw wrote: > Init these locals to 0, please. I backed out all changes to this code; should I still change it? http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text_linux.h File ui/gfx/render_text_linux.h (right): http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text_linux.h#... ui/gfx/render_text_linux.h:73: // layout_text_. On 2012/02/22 00:33:26, msw wrote: > barify |layout_text_|; consider one-liner: > // Convert between indices into text() and indices into |layout_text_|. Done. http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text_unittest.cc File ui/gfx/render_text_unittest.cc (right): http://codereview.chromium.org/8747001/diff/55003/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:284: TEST_F(RenderTextTest, PasswordCensorship) { On 2012/02/22 00:33:26, msw wrote: > Also test more complex text, like RTL, BiDi, and surrogate pairs. Done. http://codereview.chromium.org/8747001/diff/55003/ui/views/controls/textfield... File ui/views/controls/textfield/textfield_views_model_unittest.cc (left): http://codereview.chromium.org/8747001/diff/55003/ui/views/controls/textfield... ui/views/controls/textfield/textfield_views_model_unittest.cc:520: // Test for copy: Empty selection. On 2012/02/22 00:33:26, msw wrote: > Why remove this portion of the test? I didn't, I just moved it ("Copy with an empty selection should do nothing"). It's slightly different because it depends on leftover state from previous tests. http://codereview.chromium.org/8747001/diff/55003/ui/views/controls/textfield... File ui/views/controls/textfield/textfield_views_model_unittest.cc (right): http://codereview.chromium.org/8747001/diff/55003/ui/views/controls/textfield... ui/views/controls/textfield/textfield_views_model_unittest.cc:530: EXPECT_FALSE(model.Cut()); On 2012/02/18 01:28:47, xji wrote: > s/Cut/Copy/ Fixed. http://codereview.chromium.org/8747001/diff/55003/ui/views/controls/textfield... ui/views/controls/textfield/textfield_views_model_unittest.cc:555: // Paste (with obscured bit set; should be ignored). On 2012/02/18 01:28:47, xji wrote: > I feel the comment is a bit confusing. you mean "obscure flag is ignored in > paste". not "paste is ignored with obscured bit set". Done. http://codereview.chromium.org/8747001/diff/55003/ui/views/controls/textfield... ui/views/controls/textfield/textfield_views_model_unittest.cc:559: model.render_text()->SetObscured(true); On 2012/02/18 01:28:47, xji wrote: > keep the original test and add one with obscured == true. Done.
I uploaded a TextfieldViewsModel implementation as a separate CL: http://codereview.chromium.org/9467017/
http://codereview.chromium.org/8747001/diff/75003/base/utf_offset_string_conv... File base/utf_offset_string_conversions.h (right): http://codereview.chromium.org/8747001/diff/75003/base/utf_offset_string_conv... base/utf_offset_string_conversions.h:39: BASE_EXPORT ptrdiff_t Utf16IndexToOffset(const string16& s, since those functions only used in RenderTextLinux (at least for now), and you are no longer doing optimization (cache the last_text_index_ and last_layout_pointer_), the |base| is always 0 for the callers. Then, it would be better to remove |base| to make the function simpler and without the problems Mike concerned. It is similar as avoiding the (naming) confusion by declare Utf16IndexToOffset(const string16* s, size_t pos) and change the caller from Utf16IndexToOffset(s, base, pos) to Utf16IndexToOffset(&s+base, pos-base) Similar for Utf16OffstToIndex. http://codereview.chromium.org/8747001/diff/75003/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8747001/diff/75003/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:468: size_t RenderTextLinux::IndexOfAdjacentGrapheme( I like your previous optimization, which use Utf16IndexToOffset() directly on |index| to get the char offset instead of convert |index| to LayoutIndex then use g_utf8_pointer_to_offset (similar in function exit). But it does not need to be in this CL. http://codereview.chromium.org/8747001/diff/75003/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:483: size_t run_end = LayoutIndexToTextIndex(item->offset + item->length); I like your previous optimization, which converts 'position' from TextIndexToLayoutIndex and compare that with run_start/end. But that does not need to be in this CL. http://codereview.chromium.org/8747001/diff/75003/ui/gfx/render_text_unittest.cc File ui/gfx/render_text_unittest.cc (right): http://codereview.chromium.org/8747001/diff/75003/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:323: for (int s = 0; s <= 1; ++s) { maybe make it a function and call it with |select| as 'true' and 'false' instead of using for (s = 0; s <= 1; ..) for better readability. http://codereview.chromium.org/8747001/diff/75003/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:348: } since you are testing the visual operations here. How about test logical operation (such as delete/backspace) in upper layer to check that they are operating to the underlying text. http://codereview.chromium.org/8747001/diff/75003/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/8747001/diff/75003/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:604: UpdateAfterChange(text_changed, text_changed); UpdateAfterChange() could be moved inside the 'if' statement. do we need "OnBeforeUserAction()" and "OnAfterUserAction" if "IsCommandIdEnabled() == false"?
http://codereview.chromium.org/8747001/diff/75003/base/utf_offset_string_conv... File base/utf_offset_string_conversions.h (right): http://codereview.chromium.org/8747001/diff/75003/base/utf_offset_string_conv... base/utf_offset_string_conversions.h:39: BASE_EXPORT ptrdiff_t Utf16IndexToOffset(const string16& s, On 2012/02/25 00:06:14, xji wrote: > since those functions only used in RenderTextLinux (at least for now), and you > are no longer doing optimization (cache the last_text_index_ and > last_layout_pointer_), the |base| is always 0 for the callers. Then, it would be > better to remove |base| to make the function simpler and without the problems > Mike concerned. The problem is that restricting these functions to anchor at the start of the string makes them almost useless because any interesting algorithms based on them will become O(n^2). The RenderText code is already O(n^2) in a bunch of places and the (now) lack of optimization in this CL is justified only because it doesn't make things worse. > It is similar as avoiding the (naming) confusion by declare > Utf16IndexToOffset(const string16* s, size_t pos) > and change the caller from > Utf16IndexToOffset(s, base, pos) > to > Utf16IndexToOffset(&s+base, pos-base) > Similar for Utf16OffstToIndex. I assume you mean char16* rather than string16*. That would work for Utf16IndexToOffset, but Utf16OffsetToIndex(char16*, ptrdiff_t) is not possible because there's the danger of walking off the beginning or end of the string when there are unpaired surrogates. This would all be much simpler if we didn't have to worry about unpaired surrogates. Perhaps we don't. RenderText::SetText could map them to U+FFFD. But that might break something that relies on RenderText not munging the text. http://codereview.chromium.org/8747001/diff/75003/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8747001/diff/75003/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:468: size_t RenderTextLinux::IndexOfAdjacentGrapheme( On 2012/02/25 00:06:14, xji wrote: > I like your previous optimization, which use Utf16IndexToOffset() > directly on |index| to get the char offset instead of convert |index| > to LayoutIndex then use g_utf8_pointer_to_offset (similar in function > exit). > > But it does not need to be in this CL. I'll add it back later. http://codereview.chromium.org/8747001/diff/75003/ui/gfx/render_text_unittest.cc File ui/gfx/render_text_unittest.cc (right): http://codereview.chromium.org/8747001/diff/75003/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:323: for (int s = 0; s <= 1; ++s) { On 2012/02/25 00:06:14, xji wrote: > maybe make it a function and call it with |select| as 'true' and 'false' instead > of using for (s = 0; s <= 1; ..) for better readability. Done. http://codereview.chromium.org/8747001/diff/75003/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:348: } On 2012/02/25 00:06:14, xji wrote: > since you are testing the visual operations here. How about test logical > operation (such as delete/backspace) in upper layer to check that they are > operating to the underlying text. Not done yet. http://codereview.chromium.org/8747001/diff/75003/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/8747001/diff/75003/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:604: UpdateAfterChange(text_changed, text_changed); On 2012/02/25 00:06:14, xji wrote: > UpdateAfterChange() could be moved inside the 'if' statement. Done. > do we need "OnBeforeUserAction()" and "OnAfterUserAction" if > "IsCommandIdEnabled() == false"? I don't know. Oshima, do you know?
http://codereview.chromium.org/8747001/diff/75003/base/utf_offset_string_conv... File base/utf_offset_string_conversions.cc (right): http://codereview.chromium.org/8747001/diff/75003/base/utf_offset_string_conv... base/utf_offset_string_conversions.cc:93: if (!IsValidCodePointIndex(s, pos)) this line only reachable when |offset| == 0 at entry point, right? (assume line 89 holds true). http://codereview.chromium.org/8747001/diff/75003/ui/gfx/render_text_unittest.cc File ui/gfx/render_text_unittest.cc (right): http://codereview.chromium.org/8747001/diff/75003/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:348: } On 2012/03/01 17:18:00, benrg wrote: > On 2012/02/25 00:06:14, xji wrote: > > since you are testing the visual operations here. How about test logical > > operation (such as delete/backspace) in upper layer to check that they are > > operating to the underlying text. > > Not done yet. pls. add it in follow-up CL. And you can also check for delete/backspace on surrogate since you fixed that part. http://codereview.chromium.org/8747001/diff/75003/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/8747001/diff/75003/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:604: UpdateAfterChange(text_changed, text_changed); On 2012/03/01 17:18:00, benrg wrote: > On 2012/02/25 00:06:14, xji wrote: > > UpdateAfterChange() could be moved inside the 'if' statement. > > Done. looks like you forgot.
lgtm with comments addressed. pls. ask msw for review too.
http://codereview.chromium.org/8747001/diff/75003/base/utf_offset_string_conv... File base/utf_offset_string_conversions.cc (right): http://codereview.chromium.org/8747001/diff/75003/base/utf_offset_string_conv... base/utf_offset_string_conversions.cc:93: if (!IsValidCodePointIndex(s, pos)) On 2012/03/01 19:33:49, xji wrote: > this line only reachable when |offset| == 0 at entry point, right? (assume line > 89 holds true). No, it's also reachable when offset > 0 and the desired end point happens to be right after a surrogate pair. The loop exits after stepping over the first half of the surrogate. http://codereview.chromium.org/8747001/diff/75003/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/8747001/diff/75003/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:604: UpdateAfterChange(text_changed, text_changed); On 2012/02/25 00:06:14, xji wrote: > do we need "OnBeforeUserAction()" and "OnAfterUserAction" if > "IsCommandIdEnabled() == false"? Oshima says no, so changed. On 2012/03/01 19:33:49, xji wrote: > looks like you forgot. Actually done.
I don't understand the RenderTextLinux text<->layout index conversion subtleties sufficiently to offer meaningful feedback there, I'll trust xji on that part of the review. LGTM with nits. http://codereview.chromium.org/8747001/diff/82005/base/utf_offset_string_conv... File base/utf_offset_string_conversions.cc (right): http://codereview.chromium.org/8747001/diff/82005/base/utf_offset_string_conv... base/utf_offset_string_conversions.cc:81: size_t Utf16OffsetToIndex(const string16& s, size_t pos, ptrdiff_t offset) { s/pos/base/ to match decl and for clarity. http://codereview.chromium.org/8747001/diff/82005/base/utf_offset_string_conv... base/utf_offset_string_conversions.cc:82: DCHECK_LE(pos, s.length()); Should you DCHECK_LE(pos + offset, s.length())? Maybe also DCHECK_GE(pos + offset, 0)? Maybe the DCHECK_EQ(offset, 0) after covers these cases? http://codereview.chromium.org/8747001/diff/82005/base/utf_offset_string_conv... base/utf_offset_string_conversions.cc:88: // happen but is handled anyway for safety. It seems like this should happen with garbage input, so it's worth doing to alert the user of improper usage, maybe the comment should reflect that or omit "which shouldn't happen". http://codereview.chromium.org/8747001/diff/82005/base/utf_offset_string_conv... File base/utf_offset_string_conversions.h (right): http://codereview.chromium.org/8747001/diff/82005/base/utf_offset_string_conv... base/utf_offset_string_conversions.h:25: // Unpaired surrogates in the string are counted as BMP characters. If an Maybe use Basic Multilingual Plane for the laymen (like me)? http://codereview.chromium.org/8747001/diff/82005/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8747001/diff/82005/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:288: ptrdiff_t offset = Utf16IndexToOffset(text(), 0, position); Why not use TextIndexToLayoutIndex here? http://codereview.chromium.org/8747001/diff/82005/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:569: glong offset = g_utf8_pointer_to_offset(layout_text_, layout_pointer); Just use long here. http://codereview.chromium.org/8747001/diff/82005/ui/gfx/render_text_unittest.cc File ui/gfx/render_text_unittest.cc (right): http://codereview.chromium.org/8747001/diff/82005/ui/gfx/render_text_unittest... ui/gfx/render_text_unittest.cc:313: TEST_F(RenderTextTest, PasswordCensorship) { Awesome test cases! http://codereview.chromium.org/8747001/diff/82005/ui/views/controls/textfield... File ui/views/controls/textfield/textfield_views_model.cc (right): http://codereview.chromium.org/8747001/diff/82005/ui/views/controls/textfield... ui/views/controls/textfield/textfield_views_model.cc:367: size_t previous_char = Utf16OffsetToIndex(GetText(), cursor_position, -1); Add a comment explaining why this must be done, please.
LGTM for the views stuff. xji is sufficient for the render text.
http://codereview.chromium.org/8747001/diff/82005/base/utf_offset_string_conv... File base/utf_offset_string_conversions.cc (right): http://codereview.chromium.org/8747001/diff/82005/base/utf_offset_string_conv... base/utf_offset_string_conversions.cc:81: size_t Utf16OffsetToIndex(const string16& s, size_t pos, ptrdiff_t offset) { On 2012/03/01 22:48:54, msw wrote: > s/pos/base/ to match decl and for clarity. Done. http://codereview.chromium.org/8747001/diff/82005/base/utf_offset_string_conv... base/utf_offset_string_conversions.cc:82: DCHECK_LE(pos, s.length()); On 2012/03/01 22:48:54, msw wrote: > Should you DCHECK_LE(pos + offset, s.length())? > Maybe also DCHECK_GE(pos + offset, 0)? > Maybe the DCHECK_EQ(offset, 0) after covers these cases? It covers those cases. http://codereview.chromium.org/8747001/diff/82005/base/utf_offset_string_conv... base/utf_offset_string_conversions.cc:88: // happen but is handled anyway for safety. On 2012/03/01 22:48:54, msw wrote: > It seems like this should happen with garbage input, so it's worth doing to > alert the user of improper usage, maybe the comment should reflect that or omit > "which shouldn't happen". Rewritten. http://codereview.chromium.org/8747001/diff/82005/base/utf_offset_string_conv... File base/utf_offset_string_conversions.h (right): http://codereview.chromium.org/8747001/diff/82005/base/utf_offset_string_conv... base/utf_offset_string_conversions.h:25: // Unpaired surrogates in the string are counted as BMP characters. If an On 2012/03/01 22:48:54, msw wrote: > Maybe use Basic Multilingual Plane for the laymen (like me)? Rewritten. http://codereview.chromium.org/8747001/diff/82005/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8747001/diff/82005/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:288: ptrdiff_t offset = Utf16IndexToOffset(text(), 0, position); On 2012/03/01 22:48:54, msw wrote: > Why not use TextIndexToLayoutIndex here? log_attrs_ has one entry per code point, not per UTF-8 byte. http://codereview.chromium.org/8747001/diff/82005/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:569: glong offset = g_utf8_pointer_to_offset(layout_text_, layout_pointer); On 2012/03/01 22:48:54, msw wrote: > Just use long here. Done. http://codereview.chromium.org/8747001/diff/82005/ui/views/controls/textfield... File ui/views/controls/textfield/textfield_views_model.cc (right): http://codereview.chromium.org/8747001/diff/82005/ui/views/controls/textfield... ui/views/controls/textfield/textfield_views_model.cc:367: size_t previous_char = Utf16OffsetToIndex(GetText(), cursor_position, -1); On 2012/03/01 22:48:54, msw wrote: > Add a comment explaining why this must be done, please. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benrg@chromium.org/8747001/86001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benrg@chromium.org/8747001/83082 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
