Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9358049:
Implement STYLE_LOWERCASE style for Aura NativeTextfield. (Closed)
|
Created:
8 years, 10 months ago by kochi Modified:
8 years, 8 months ago CC:
chromium-reviews, tfarina, James Su, penghuang+watch_chromium.org, yusukes+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement STYLE_LOWERCASE style for Aura NativeTextfield.
BUG=109308
TEST=views_unittest --gtest_filter="NativeTextfieldViews*"
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132525
Patch Set 1 #Patch Set 2 : Refine the patch. #
Total comments: 20
Patch Set 3 : rebase. #
Total comments: 4
Patch Set 4 : rebase. #Patch Set 5 : update for comments. #
Total comments: 9
Patch Set 6 : Fix for comments, added locale-dependent conversion test. #Patch Set 7 : rebase. #Patch Set 8 : Fix comments for Paste(). #
Total comments: 3
Patch Set 9 : fix for comments. #
Total comments: 4
Patch Set 10 : Add MaybeLowerCase(), test descriptions and cursor position assertion. #
Total comments: 10
Patch Set 11 : Change MaybeLowerCase() to GetTextForDisplay() #
Total comments: 8
Patch Set 12 : fix for comments. #
Messages
Total messages: 30 (0 generated)
Hi, Could you review this?
The assumption is: lower-cased text is saved in RenderText, and there will never be any operations done to the original text. (please refer to http://code.google.com/p/chromium/issues/detail?id=109308#c14. And I will double check the assumption.). Ideally, the lower-case flag and case mapping should be handled in RenderText. But since case mapping can change the number of code points and/or code units of a string, and the correct cursor position is calculated in TextfieldViewModel using number of code points in string (text.length()), seems even if we change those related functions (SetText, ExecuteAndRecordReplaceSelection, InsertEdit, RepalceEdit etc.) in TextfieldViewsModel, we still can not get rid of case mapping (in order to get the correct position to set the cursor to) in upper layer - TextfieldViewModel. For example, insert a character when there is selection. Since lower-cased text is the one saved in RenderText and is the one user operates on, I think doing case mapping in NativeTextfieldView sounds fine. Mike, what do you think? http://codereview.chromium.org/9358049/diff/2001/ui/views/controls/textfield/... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/9358049/diff/2001/ui/views/controls/textfield/... ui/views/controls/textfield/native_textfield_views.cc:194: text = base::i18n::ToLower(text); I think we should do language specific case mapping. ToLower() is not. Please refer to http://userguide.icu-project.org/transforms/casemappings http://codereview.chromium.org/9358049/diff/2001/ui/views/controls/textfield/... ui/views/controls/textfield/native_textfield_views.cc:687: new_text = base::i18n::ToLower(new_text); nit: string16 new_text = (textfield_->style() & Textfield::STYLE_LOWERCASE) ? base::i18n::ToLower(text) : text; http://codereview.chromium.org/9358049/diff/2001/ui/views/controls/textfield/... ui/views/controls/textfield/native_textfield_views.cc:1065: } this should not be needed since model_->GetText() should be lower case. http://codereview.chromium.org/9358049/diff/2001/ui/views/controls/textfield/... ui/views/controls/textfield/native_textfield_views.cc:1069: if (success && GetText() == textfield_->text()) { In case of STYLE_LOWERCASE, GetText() is lower-cased, but not textfeild_->text(). http://codereview.chromium.org/9358049/diff/2001/ui/views/controls/textfield/... File ui/views/controls/textfield/native_textfield_views_unittest.cc (right): http://codereview.chromium.org/9358049/diff/2001/ui/views/controls/textfield/... ui/views/controls/textfield/native_textfield_views_unittest.cc:388: EXPECT_STR_EQ("THIS IS A TEST", textfield_->text()); could you please add tests for all modified functions in NativeTextfieldViews? UpdateText(), AppendText(), InsertText/Char(), Paste(), etc.
Why not transform the text at the Textfield level (ui/views/controls/textfield/textfield.cc)? That would make this work regardless of the underlying native textfield type. That said, doing the text transformations at either level seems like it should work for RenderText. http://codereview.chromium.org/9358049/diff/2001/ui/views/controls/textfield/... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/9358049/diff/2001/ui/views/controls/textfield/... ui/views/controls/textfield/native_textfield_views.cc:687: new_text = base::i18n::ToLower(new_text); On 2012/02/17 22:38:24, xji wrote: > nit: string16 new_text = (textfield_->style() & Textfield::STYLE_LOWERCASE) ? > base::i18n::ToLower(text) : text; why not just reassign to text? new_text seems redundant: if (textfield_->style() & Textfield::STYLE_LOWERCASE) text = base::i18n::ToLower(text); http://codereview.chromium.org/9358049/diff/2001/ui/views/controls/textfield/... ui/views/controls/textfield/native_textfield_views.cc:705: ch = u_tolower(ch); You should ensure this does the language specific case mapping that xji suggests.
On 2012/02/18 00:32:38, msw wrote: > Why not transform the text at the Textfield level > (ui/views/controls/textfield/textfield.cc)? That would make this work regardless > of the underlying native textfield type. That said, doing the text > transformations at either level seems like it should work for RenderText. > I had the same question but did not ask because I thought we probably delegate to native textfield to do their own case mapping so that they match native platform's behavior (say not all of them using ICU).
http://codereview.chromium.org/9358049/diff/2001/ui/views/controls/textfield/... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/9358049/diff/2001/ui/views/controls/textfield/... ui/views/controls/textfield/native_textfield_views.cc:687: new_text = base::i18n::ToLower(new_text); On 2012/02/18 00:32:38, msw wrote: > On 2012/02/17 22:38:24, xji wrote: > > nit: string16 new_text = (textfield_->style() & Textfield::STYLE_LOWERCASE) ? > > base::i18n::ToLower(text) : text; > > why not just reassign to text? new_text seems redundant: > if (textfield_->style() & Textfield::STYLE_LOWERCASE) > text = base::i18n::ToLower(text); |text| is a 'const'.
Sorry for the delay. I'm distracted by other issues, and this is not yet done. http://codereview.chromium.org/9358049/diff/2001/ui/views/controls/textfield/... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/9358049/diff/2001/ui/views/controls/textfield/... ui/views/controls/textfield/native_textfield_views.cc:194: text = base::i18n::ToLower(text); According to the header file, ToLower uses UnicodeString::toLower(), which uses the default locale of ICU. I thought the default locale of ICU and Chrome's app locale are synced, so this is language-specific, isn't it? (Maybe I'm still wrong, I'm new to ICU) On 2012/02/17 22:38:24, xji wrote: > I think we should do language specific case mapping. ToLower() is not. Please > refer to http://userguide.icu-project.org/transforms/casemappings http://codereview.chromium.org/9358049/diff/2001/ui/views/controls/textfield/... ui/views/controls/textfield/native_textfield_views.cc:687: new_text = base::i18n::ToLower(new_text); On 2012/02/17 22:38:24, xji wrote: > nit: string16 new_text = (textfield_->style() & Textfield::STYLE_LOWERCASE) ? > base::i18n::ToLower(text) : text; Done. http://codereview.chromium.org/9358049/diff/2001/ui/views/controls/textfield/... ui/views/controls/textfield/native_textfield_views.cc:687: new_text = base::i18n::ToLower(new_text); Yes, that's right. On 2012/02/18 01:33:38, xji wrote: > On 2012/02/18 00:32:38, msw wrote: > > On 2012/02/17 22:38:24, xji wrote: > > > nit: string16 new_text = (textfield_->style() & Textfield::STYLE_LOWERCASE) > ? > > > base::i18n::ToLower(text) : text; > > > > why not just reassign to text? new_text seems redundant: > > if (textfield_->style() & Textfield::STYLE_LOWERCASE) > > text = base::i18n::ToLower(text); > > |text| is a 'const'. http://codereview.chromium.org/9358049/diff/2001/ui/views/controls/textfield/... ui/views/controls/textfield/native_textfield_views.cc:705: ch = u_tolower(ch); On 2012/02/18 00:32:38, msw wrote: > You should ensure this does the language specific case mapping that xji > suggests. This one doesn't seem to handle language-specific case conversion, and there seems to be no corresponding API variant for one-character. Let me look into this further. http://codereview.chromium.org/9358049/diff/2001/ui/views/controls/textfield/... ui/views/controls/textfield/native_textfield_views.cc:1065: } The model directly gets the text from clipboard in the model_->Paste() call above, and modifies the text internally. So at this point the string may contain lower case characters. http://codereview.chromium.org/9358049/diff/2001/ui/views/controls/textfield/... ui/views/controls/textfield/native_textfield_views.cc:1069: if (success && GetText() == textfield_->text()) { On 2012/02/17 22:38:24, xji wrote: > In case of STYLE_LOWERCASE, GetText() is lower-cased, but not > textfeild_->text(). Right, will fix this.
http://codereview.chromium.org/9358049/diff/2001/ui/views/controls/textfield/... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/9358049/diff/2001/ui/views/controls/textfield/... ui/views/controls/textfield/native_textfield_views.cc:194: text = base::i18n::ToLower(text); On 2012/02/23 10:49:07, Takayoshi Kochi wrote: > According to the header file, > ToLower uses UnicodeString::toLower(), which uses the default locale of ICU. I > thought the default locale of ICU > and Chrome's app locale are synced, so this is language-specific, isn't it? > > (Maybe I'm still wrong, I'm new to ICU) > > On 2012/02/17 22:38:24, xji wrote: > > I think we should do language specific case mapping. ToLower() is not. Please > > refer to http://userguide.icu-project.org/transforms/casemappings > You are right! sorry for the false alarm. http://codereview.chromium.org/9358049/diff/2001/ui/views/controls/textfield/... ui/views/controls/textfield/native_textfield_views.cc:1065: } On 2012/02/23 10:49:07, Takayoshi Kochi wrote: > The model directly gets the text from clipboard > in the model_->Paste() call above, and modifies the text > internally. So at this point the string may contain > lower case characters. Ah, I see. I thought RenderText will always keep a lower-case text, which is actually not true (at least on this Paste() operation). I do not like this inconsistency, but I can not think of a better simple/clean solution regarding to where to do all the case mappings (not sure whether Textfield is an option).
Sorry for the terribly late reply. http://codereview.chromium.org/9358049/diff/16001/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/9358049/diff/16001/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:697: void NativeTextfieldViews::InsertChar(char16 ch, int flags) { This is not a part of your change, but this API seems not right. This should not accept char16 here. Instead, it should take 'char32' or a pair of char16 (of which the second can be optional. http://codereview.chromium.org/9358049/diff/16001/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views_unittest.cc (right): http://codereview.chromium.org/9358049/diff/16001/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views_unittest.cc:376: EXPECT_STR_EQ("THIS IS A TEST", textfield_->text()); It'd be nice to have non-ASCII tests here. It'd be even better to have locale-dependent tests as well.
Sorry for the long interval. I was distracted by other tasks. Please take another look. http://chromiumcodereview.appspot.com/9358049/diff/2001/ui/views/controls/tex... File ui/views/controls/textfield/native_textfield_views.cc (right): http://chromiumcodereview.appspot.com/9358049/diff/2001/ui/views/controls/tex... ui/views/controls/textfield/native_textfield_views.cc:1065: } OK, I agree this is not intuitive to understand. Added a comment. On 2012/02/23 21:59:17, xji wrote: > On 2012/02/23 10:49:07, Takayoshi Kochi wrote: > > The model directly gets the text from clipboard > > in the model_->Paste() call above, and modifies the text > > internally. So at this point the string may contain > > lower case characters. > > Ah, I see. I thought RenderText will always keep a lower-case text, which is > actually not true (at least on this Paste() operation). I do not like this > inconsistency, but I can not think of a better simple/clean solution regarding > to where to do all the case mappings (not sure whether Textfield is an option). http://chromiumcodereview.appspot.com/9358049/diff/2001/ui/views/controls/tex... ui/views/controls/textfield/native_textfield_views.cc:1069: if (success && GetText() == textfield_->text()) { Ah, no. GetText() in this class is just does model_->GetText(), so the one in 1062 should be GetText(). And now model_ should have lower cased string, this code is fine. On 2012/02/23 10:49:07, Takayoshi Kochi wrote: > On 2012/02/17 22:38:24, xji wrote: > > In case of STYLE_LOWERCASE, GetText() is lower-cased, but not > > textfeild_->text(). > > Right, will fix this. http://chromiumcodereview.appspot.com/9358049/diff/16001/ui/views/controls/te... File ui/views/controls/textfield/native_textfield_views.cc (right): http://chromiumcodereview.appspot.com/9358049/diff/16001/ui/views/controls/te... ui/views/controls/textfield/native_textfield_views.cc:697: void NativeTextfieldViews::InsertChar(char16 ch, int flags) { According to the native_textfield_views.h comment, there is a work to be done for support surrogate pair. So yeah, this is not a part of this CL, but thanks! On 2012/03/26 22:04:34, Jungshik Shin wrote: > This is not a part of your change, but this API seems not right. This should not > accept char16 here. Instead, it should take 'char32' or a pair of char16 (of > which the second can be optional. http://chromiumcodereview.appspot.com/9358049/diff/16001/ui/views/controls/te... File ui/views/controls/textfield/native_textfield_views_unittest.cc (right): http://chromiumcodereview.appspot.com/9358049/diff/16001/ui/views/controls/te... ui/views/controls/textfield/native_textfield_views_unittest.cc:376: EXPECT_STR_EQ("THIS IS A TEST", textfield_->text()); Sounds good. I added Japanese test. I left the locale-dependent tests in TODO. On 2012/03/26 22:04:34, Jungshik Shin wrote: > It'd be nice to have non-ASCII tests here. It'd be even better to have > locale-dependent tests as well.
http://chromiumcodereview.appspot.com/9358049/diff/24001/ui/views/controls/te... File ui/views/controls/textfield/native_textfield_views.cc (right): http://chromiumcodereview.appspot.com/9358049/diff/24001/ui/views/controls/te... ui/views/controls/textfield/native_textfield_views.cc:1063: // lower-case characters. This is not consistent with other places I think you mean "upper case" here, not "lower-case". Also remove the dash here for consistency with "lower case" on the line below. http://chromiumcodereview.appspot.com/9358049/diff/24001/ui/views/controls/te... ui/views/controls/textfield/native_textfield_views.cc:1069: // action did not change the content at all. See http://crbug.com/79002 This isn't part of your change, but I don't understand how this makes sense... |success| is only true if the text changes, so how does this block run in the case that "the paste action did not change the content at all"? I'll post a note on the bug. http://chromiumcodereview.appspot.com/9358049/diff/24001/ui/views/controls/te... File ui/views/controls/textfield/native_textfield_views_unittest.cc (right): http://chromiumcodereview.appspot.com/9358049/diff/24001/ui/views/controls/te... ui/views/controls/textfield/native_textfield_views_unittest.cc:406: // TODO(kochi): Add locale-specific lower-case conversion tests. Did you mean to address this before landing?
http://chromiumcodereview.appspot.com/9358049/diff/24001/ui/views/controls/te... File ui/views/controls/textfield/native_textfield_views.cc (right): http://chromiumcodereview.appspot.com/9358049/diff/24001/ui/views/controls/te... ui/views/controls/textfield/native_textfield_views.cc:1063: // lower-case characters. This is not consistent with other places On 2012/04/09 18:29:24, msw wrote: > I think you mean "upper case" here, not "lower-case". > Also remove the dash here for consistency with "lower case" on the line below. Good catch, thanks! http://chromiumcodereview.appspot.com/9358049/diff/24001/ui/views/controls/te... ui/views/controls/textfield/native_textfield_views.cc:1069: // action did not change the content at all. See http://crbug.com/79002 I suppose that this case is for when you have selected text and paste the exactly same text to that field. In that case paste succeeds, text is substituted by the pasted text, but no text change. Even on that case, textfield controller may want to know the user interaction. On 2012/04/09 18:29:24, msw wrote: > This isn't part of your change, but I don't understand how this makes sense... > |success| is only true if the text changes, so how does this block run in the > case that "the paste action did not change the content at all"? I'll post a note > on the bug. http://chromiumcodereview.appspot.com/9358049/diff/24001/ui/views/controls/te... File ui/views/controls/textfield/native_textfield_views_unittest.cc (right): http://chromiumcodereview.appspot.com/9358049/diff/24001/ui/views/controls/te... ui/views/controls/textfield/native_textfield_views_unittest.cc:406: // TODO(kochi): Add locale-specific lower-case conversion tests. On 2012/04/09 18:29:24, msw wrote: > Did you mean to address this before landing? I'm not an expert of locale-specific case conversion, but as far as I know there is Turkish "I" conversion case that gets tricky. Added a case.
http://chromiumcodereview.appspot.com/9358049/diff/24001/ui/views/controls/te... File ui/views/controls/textfield/native_textfield_views.cc (right): http://chromiumcodereview.appspot.com/9358049/diff/24001/ui/views/controls/te... ui/views/controls/textfield/native_textfield_views.cc:1069: // action did not change the content at all. See http://crbug.com/79002 On 2012/04/10 07:38:11, Takayoshi Kochi wrote: > I suppose that this case is for when you have selected > text and paste the exactly same text to that field. > In that case paste succeeds, text is substituted by the > pasted text, but no text change. > > Even on that case, textfield controller may want to know > the user interaction. > > On 2012/04/09 18:29:24, msw wrote: > > This isn't part of your change, but I don't understand how this makes sense... > > |success| is only true if the text changes, so how does this block run in the > > case that "the paste action did not change the content at all"? I'll post a > note > > on the bug. > Yes, that's the intent, but TextfieldViewsModel::Paste() has the comment: // Pastes text from the clipboard at current cursor position. Returns true // if text has changed after pasting. Then again, it appears that the comment may just be wrong. (Oops): http://code.google.com/searchframe#OAMlx_jo-ck/src/ui/views/controls/textfiel...
http://chromiumcodereview.appspot.com/9358049/diff/2001/ui/views/controls/tex... File ui/views/controls/textfield/native_textfield_views.cc (right): http://chromiumcodereview.appspot.com/9358049/diff/2001/ui/views/controls/tex... ui/views/controls/textfield/native_textfield_views.cc:1069: if (success && GetText() == textfield_->text()) { GetText() is lower-cased. But isn't textfield_->text() the original text, which might be upper-case letters? On 2012/04/09 06:34:20, Takayoshi Kochi wrote: > Ah, no. > > GetText() in this class is just does model_->GetText(), > so the one in 1062 should be GetText(). > > And now model_ should have lower cased string, this code > is fine. > > On 2012/02/23 10:49:07, Takayoshi Kochi wrote: > > On 2012/02/17 22:38:24, xji wrote: > > > In case of STYLE_LOWERCASE, GetText() is lower-cased, but not > > > textfeild_->text(). > > > > Right, will fix this. > http://chromiumcodereview.appspot.com/9358049/diff/24001/ui/views/controls/te... File ui/views/controls/textfield/native_textfield_views.cc (right): http://chromiumcodereview.appspot.com/9358049/diff/24001/ui/views/controls/te... ui/views/controls/textfield/native_textfield_views.cc:1069: // action did not change the content at all. See http://crbug.com/79002 Looks like it is the wrong comment. As long as there is things to be pasted (non-empty clipboard), it returns true. On 2012/04/10 07:50:25, msw wrote: > On 2012/04/10 07:38:11, Takayoshi Kochi wrote: > > I suppose that this case is for when you have selected > > text and paste the exactly same text to that field. > > In that case paste succeeds, text is substituted by the > > pasted text, but no text change. > > > > Even on that case, textfield controller may want to know > > the user interaction. > > > > On 2012/04/09 18:29:24, msw wrote: > > > This isn't part of your change, but I don't understand how this makes > sense... > > > |success| is only true if the text changes, so how does this block run in > the > > > case that "the paste action did not change the content at all"? I'll post a > > note > > > on the bug. > > > > Yes, that's the intent, but TextfieldViewsModel::Paste() has the comment: > // Pastes text from the clipboard at current cursor position. Returns true > // if text has changed after pasting. > Then again, it appears that the comment may just be wrong. (Oops): > http://code.google.com/searchframe#OAMlx_jo-ck/src/ui/views/controls/textfiel...
http://chromiumcodereview.appspot.com/9358049/diff/2001/ui/views/controls/tex... File ui/views/controls/textfield/native_textfield_views.cc (right): http://chromiumcodereview.appspot.com/9358049/diff/2001/ui/views/controls/tex... ui/views/controls/textfield/native_textfield_views.cc:1069: if (success && GetText() == textfield_->text()) { I see, sorry for misunderstanding again. Done. On 2012/04/10 22:53:27, xji wrote: > GetText() is lower-cased. > But isn't textfield_->text() the original text, which might be upper-case > letters? > > > On 2012/04/09 06:34:20, Takayoshi Kochi wrote: > > Ah, no. > > > > GetText() in this class is just does model_->GetText(), > > so the one in 1062 should be GetText(). > > > > And now model_ should have lower cased string, this code > > is fine. > > > > On 2012/02/23 10:49:07, Takayoshi Kochi wrote: > > > On 2012/02/17 22:38:24, xji wrote: > > > > In case of STYLE_LOWERCASE, GetText() is lower-cased, but not > > > > textfeild_->text(). > > > > > > Right, will fix this. > > > http://chromiumcodereview.appspot.com/9358049/diff/24001/ui/views/controls/te... File ui/views/controls/textfield/native_textfield_views.cc (right): http://chromiumcodereview.appspot.com/9358049/diff/24001/ui/views/controls/te... ui/views/controls/textfield/native_textfield_views.cc:1069: // action did not change the content at all. See http://crbug.com/79002 OK, I changed the original comment in TextfieldViewsModel::Paste(). On 2012/04/10 22:53:27, xji wrote: > Looks like it is the wrong comment. > As long as there is things to be pasted (non-empty clipboard), it returns true. > > On 2012/04/10 07:50:25, msw wrote: > > On 2012/04/10 07:38:11, Takayoshi Kochi wrote: > > > I suppose that this case is for when you have selected > > > text and paste the exactly same text to that field. > > > In that case paste succeeds, text is substituted by the > > > pasted text, but no text change. > > > > > > Even on that case, textfield controller may want to know > > > the user interaction. > > > > > > On 2012/04/09 18:29:24, msw wrote: > > > > This isn't part of your change, but I don't understand how this makes > > sense... > > > > |success| is only true if the text changes, so how does this block run in > > the > > > > case that "the paste action did not change the content at all"? I'll post > a > > > note > > > > on the bug. > > > > > > > Yes, that's the intent, but TextfieldViewsModel::Paste() has the comment: > > // Pastes text from the clipboard at current cursor position. Returns true > > // if text has changed after pasting. > > Then again, it appears that the comment may just be wrong. (Oops): > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/ui/views/controls/textfiel... >
http://chromiumcodereview.appspot.com/9358049/diff/39002/ui/views/controls/te... File ui/views/controls/textfield/native_textfield_views.cc (right): http://chromiumcodereview.appspot.com/9358049/diff/39002/ui/views/controls/te... ui/views/controls/textfield/native_textfield_views.cc:1077: if (GetText() == base::i18n::ToLower(textfield_->text())) { ToLower() here should only be done for STYLE_LOWERCASE. http://chromiumcodereview.appspot.com/9358049/diff/39002/ui/views/controls/te... File ui/views/controls/textfield/textfield_views_model.h (right): http://chromiumcodereview.appspot.com/9358049/diff/39002/ui/views/controls/te... ui/views/controls/textfield/textfield_views_model.h:188: // if any text is pasted. thanks for making the change.
http://chromiumcodereview.appspot.com/9358049/diff/39002/ui/views/controls/te... File ui/views/controls/textfield/native_textfield_views.cc (right): http://chromiumcodereview.appspot.com/9358049/diff/39002/ui/views/controls/te... ui/views/controls/textfield/native_textfield_views.cc:1077: if (GetText() == base::i18n::ToLower(textfield_->text())) { On 2012/04/11 04:06:35, xji wrote: > ToLower() here should only be done for STYLE_LOWERCASE. Oops... Thanks!
lgtm
Ben or Scott, could you take a look for OWNER lgtm? Thanks!
http://codereview.chromium.org/9358049/diff/47001/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/9358049/diff/47001/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:200: if (textfield_->style() & Textfield::STYLE_LOWERCASE) You check STYLE_LOWERCASE in a number of places. How about a single method that returns either the passed string, or converts to lower case. http://codereview.chromium.org/9358049/diff/47001/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views_unittest.cc (right): http://codereview.chromium.org/9358049/diff/47001/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views_unittest.cc:364: TEST_F(NativeTextfieldViewsTest, ModelChangesTestLowerCase) { nit: add descriptions for your tests. Also, can you add assertions that the caret position doesn't get mangled.
http://codereview.chromium.org/9358049/diff/47001/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/9358049/diff/47001/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:200: if (textfield_->style() & Textfield::STYLE_LOWERCASE) Added MaybeLowerCase() and use it where possible. On 2012/04/12 03:58:33, sky wrote: > You check STYLE_LOWERCASE in a number of places. How about a single method that > returns either the passed string, or converts to lower case. http://codereview.chromium.org/9358049/diff/47001/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views_unittest.cc (right): http://codereview.chromium.org/9358049/diff/47001/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views_unittest.cc:364: TEST_F(NativeTextfieldViewsTest, ModelChangesTestLowerCase) { Added short descriptions for new tests. Also added brief checks against cursor position. On 2012/04/12 03:58:33, sky wrote: > nit: add descriptions for your tests. > > Also, can you add assertions that the caret position doesn't get mangled.
http://codereview.chromium.org/9358049/diff/37002/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/9358049/diff/37002/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:1133: void NativeTextfieldViews::MaybeLowerCase(string16* text) { I think making this return the string would make for easier to use code. For example, 675 becomes: new_text = MaybeLowercase(text); and 336 becomes: model_->Append(MaybeLowercase(text));
http://codereview.chromium.org/9358049/diff/37002/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/9358049/diff/37002/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:1065: // As Paste is handled in model_->Paste(), the RenderText may contain Every call to MaybeLowerCase is sent to the model. Should style enforcement / GetTextForDisplay should be there? Probably not, but it's worth considering. http://codereview.chromium.org/9358049/diff/37002/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.cc:1133: void NativeTextfieldViews::MaybeLowerCase(string16* text) { Ditto to Sky's comment: Each MaybeLowerCase string is sent as an arg or essentially an r-value. Take a const ref and return a string like ToLower, see your own line 336: return textfield_->style() & Textfield::STYLE_LOWERCASE ? base::i18n::ToLower(text) : text; http://codereview.chromium.org/9358049/diff/37002/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views.h (right): http://codereview.chromium.org/9358049/diff/37002/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views.h:239: void MaybeLowerCase(string16* text); Please rename this GetTextForDisplay or similar for generalization. That's more amenable to eventually supporting uppercase, password, etc. Also, put this below GetRenderText at line 175 (same for the definition). http://codereview.chromium.org/9358049/diff/37002/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_views_unittest.cc (right): http://codereview.chromium.org/9358049/diff/37002/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_views_unittest.cc:365: // Check if |model_|'s text is properly lowercased when STYLE_LOWERCASE is optional one-line comment nit: // Check if |model_|'s text is properly lowercased for STYLE_LOWERCASE.
Thanks for all the comments. https://chromiumcodereview.appspot.com/9358049/diff/37002/ui/views/controls/t... File ui/views/controls/textfield/native_textfield_views.cc (right): https://chromiumcodereview.appspot.com/9358049/diff/37002/ui/views/controls/t... ui/views/controls/textfield/native_textfield_views.cc:1065: // As Paste is handled in model_->Paste(), the RenderText may contain Sorry, I'm not clear about your comment. If the comment is not clear, I have to rewrite the comment. Or do you mean style enforcement can be done in TextfieldViewsModel? On 2012/04/12 16:15:29, msw wrote: > Every call to MaybeLowerCase is sent to the model. > Should style enforcement / GetTextForDisplay should be there? > Probably not, but it's worth considering. https://chromiumcodereview.appspot.com/9358049/diff/37002/ui/views/controls/t... ui/views/controls/textfield/native_textfield_views.cc:1133: void NativeTextfieldViews::MaybeLowerCase(string16* text) { On 2012/04/12 16:15:29, msw wrote: > Ditto to Sky's comment: > Each MaybeLowerCase string is sent as an arg or essentially an r-value. > Take a const ref and return a string like ToLower, see your own line 336: > return textfield_->style() & Textfield::STYLE_LOWERCASE ? > base::i18n::ToLower(text) : text; Done. https://chromiumcodereview.appspot.com/9358049/diff/37002/ui/views/controls/t... File ui/views/controls/textfield/native_textfield_views.h (right): https://chromiumcodereview.appspot.com/9358049/diff/37002/ui/views/controls/t... ui/views/controls/textfield/native_textfield_views.h:239: void MaybeLowerCase(string16* text); On 2012/04/12 16:15:29, msw wrote: > Please rename this GetTextForDisplay or similar for generalization. > That's more amenable to eventually supporting uppercase, password, etc. > Also, put this below GetRenderText at line 175 (same for the definition). Done. https://chromiumcodereview.appspot.com/9358049/diff/37002/ui/views/controls/t... File ui/views/controls/textfield/native_textfield_views_unittest.cc (right): https://chromiumcodereview.appspot.com/9358049/diff/37002/ui/views/controls/t... ui/views/controls/textfield/native_textfield_views_unittest.cc:365: // Check if |model_|'s text is properly lowercased when STYLE_LOWERCASE is On 2012/04/12 16:15:29, msw wrote: > optional one-line comment nit: > // Check if |model_|'s text is properly lowercased for STYLE_LOWERCASE. Done.
LGTM
https://chromiumcodereview.appspot.com/9358049/diff/37002/ui/views/controls/t... File ui/views/controls/textfield/native_textfield_views.cc (right): https://chromiumcodereview.appspot.com/9358049/diff/37002/ui/views/controls/t... ui/views/controls/textfield/native_textfield_views.cc:1065: // As Paste is handled in model_->Paste(), the RenderText may contain > Or do you mean style enforcement can be done in TextfieldViewsModel? Yeah, it's not necessarily the right approach; just worth considering. https://chromiumcodereview.appspot.com/9358049/diff/52004/ui/views/controls/t... File ui/views/controls/textfield/native_textfield_views.cc (right): https://chromiumcodereview.appspot.com/9358049/diff/52004/ui/views/controls/t... ui/views/controls/textfield/native_textfield_views.cc:325: model_->SetText(GetTextForDisplay(text)); nit: model_->SetText(GetTextForDisplay(textfield_->text())); https://chromiumcodereview.appspot.com/9358049/diff/52004/ui/views/controls/t... ui/views/controls/textfield/native_textfield_views.cc:697: model_->SetText(base::i18n::ToLower(GetText())); Should this be an unconditional model_->SetText(GetTextForDisplay(GetText())? https://chromiumcodereview.appspot.com/9358049/diff/52004/ui/views/controls/t... ui/views/controls/textfield/native_textfield_views.cc:1060: bool NativeTextfieldViews::Paste() { Gah, I think textfield_->text() versus GetText() (from the model) is confusing. I think this whole function should be: bool NativeTextfieldViews::Paste() { string16 original_text = GetText(); const bool success = model_->Paste(); if (success) { // <YOUR COMMENT ABOUT NEEDING TO LOWERCASE (ENFORCE STYLE) HERE> string16 new_text = GetTextForDisplay(GetText()); if (new_text != original_text) { model_->SetText(new_text); } else { <THE COMMENT AND [UNCONDITIONAL] CODE TO TRIGGER ContentChanged> } } return success; } Please make that change if you don't mind (and if I'm correct). https://chromiumcodereview.appspot.com/9358049/diff/52004/ui/views/controls/t... File ui/views/controls/textfield/native_textfield_views_unittest.cc (right): https://chromiumcodereview.appspot.com/9358049/diff/52004/ui/views/controls/t... ui/views/controls/textfield/native_textfield_views_unittest.cc:367: EXPECT_EQ(0U, textfield_->GetCursorPosition()); Why do these test check the cursor position at all? It seems irrelevant. Please these unnecessary checks (I count 9 overall in these new tests).
https://chromiumcodereview.appspot.com/9358049/diff/52004/ui/views/controls/t... File ui/views/controls/textfield/native_textfield_views.cc (right): https://chromiumcodereview.appspot.com/9358049/diff/52004/ui/views/controls/t... ui/views/controls/textfield/native_textfield_views.cc:325: model_->SetText(GetTextForDisplay(text)); On 2012/04/13 18:13:25, msw wrote: > nit: model_->SetText(GetTextForDisplay(textfield_->text())); Done. https://chromiumcodereview.appspot.com/9358049/diff/52004/ui/views/controls/t... ui/views/controls/textfield/native_textfield_views.cc:697: model_->SetText(base::i18n::ToLower(GetText())); On 2012/04/13 18:13:25, msw wrote: > Should this be an unconditional model_->SetText(GetTextForDisplay(GetText())? Done. https://chromiumcodereview.appspot.com/9358049/diff/52004/ui/views/controls/t... ui/views/controls/textfield/native_textfield_views.cc:1060: bool NativeTextfieldViews::Paste() { I mostly applied your comment. I felt that calling model_->SetText(new_text) only when new_text != original_text is correct but a bit hard to understand at first. Instead, what we do elsewhere, unconditionally SetText(), which is suboptimal but is easier to understand. And this also addresses asymmetry of if- and else-clauses. On 2012/04/13 18:13:25, msw wrote: > Gah, I think textfield_->text() versus GetText() (from the model) is confusing. > I think this whole function should be: > > bool NativeTextfieldViews::Paste() { > string16 original_text = GetText(); > const bool success = model_->Paste(); > if (success) { > // <YOUR COMMENT ABOUT NEEDING TO LOWERCASE (ENFORCE STYLE) HERE> > string16 new_text = GetTextForDisplay(GetText()); > if (new_text != original_text) { > model_->SetText(new_text); > } else { > <THE COMMENT AND [UNCONDITIONAL] CODE TO TRIGGER ContentChanged> > } > } > return success; > } > > Please make that change if you don't mind (and if I'm correct). https://chromiumcodereview.appspot.com/9358049/diff/52004/ui/views/controls/t... File ui/views/controls/textfield/native_textfield_views_unittest.cc (right): https://chromiumcodereview.appspot.com/9358049/diff/52004/ui/views/controls/t... ui/views/controls/textfield/native_textfield_views_unittest.cc:367: EXPECT_EQ(0U, textfield_->GetCursorPosition()); That's for addressing sky's review comment "Also, can you add assertions that the caret position doesn't get mangled." so we can make sure cursor position won't get changed by SetText() etc. accidentally. On 2012/04/13 18:13:25, msw wrote: > Why do these test check the cursor position at all? It seems irrelevant. > Please these unnecessary checks (I count 9 overall in these new tests).
LGTM; thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kochi@chromium.org/9358049/58001
On 2012/04/16 17:11:19, msw wrote: > LGTM; thanks! Thanks for your patience!
Change committed as 132525 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
