Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8748001:
Make text input type and password visibility bit independent in Textfield (Closed)
|
Created:
9 years ago by benrg Modified:
7 years ago CC:
chromium-reviews, nkostylev+watch_chromium.org, tfarina, penghuang+watch_chromium.org, yusukes+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, dhollowa, Emmanuel Saint-loubert-Bié Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRename STYLE_PASSWORD to STYLE_OBSCURED and make it
independent from SetTextInputType(TEXT_INPUT_TYPE_PASSWORD).
The input type of a password field should always be
TEXT_INPUT_TYPE_PASSWORD, but STYLE_OBSCURED may not be set
(e.g., when showing a wifi password).
BUG=105054
TEST=none
Patch Set 1 : rebase #
Total comments: 4
Patch Set 2 : move IsPassword() back to .cc #
Total comments: 2
Patch Set 3 : make password style bit and input type independent; rename bit to "obscured" #Patch Set 4 : more renaming #Patch Set 5 : Textfield(STYLE_OBSCURED) sets TEXT_INPUT_TYPE_PASSWORD #
Total comments: 5
Patch Set 6 : fix comments, add a test #Patch Set 7 : rebase, fix tests of TextInputType in NativeTextfieldViews #Patch Set 8 : rebase #
Total comments: 1
Messages
Total messages: 27 (0 generated)
Can you take a look at this, since I think you wrote the original code?
I'm not very up to date on what is happening with this code. When I originally wrote those changes, the textfield code was soon to be refactored in a significant way. You should probably have oshima or someone else who is up to date on this part of the code do the review.
http://codereview.chromium.org/8748001/diff/5/ui/views/controls/textfield/tex... File ui/views/controls/textfield/textfield.h (right): http://codereview.chromium.org/8748001/diff/5/ui/views/controls/textfield/tex... ui/views/controls/textfield/textfield.h:69: return style() & STYLE_PASSWORD; nit: please use style_ here directly. Also I'd prefer if you could move this implementation to the source file instead (avoid polluting the header file with implementation details).
tfarina, thanks for looking at this. http://codereview.chromium.org/8748001/diff/5/ui/views/controls/textfield/tex... File ui/views/controls/textfield/textfield.h (right): http://codereview.chromium.org/8748001/diff/5/ui/views/controls/textfield/tex... ui/views/controls/textfield/textfield.h:69: return style() & STYLE_PASSWORD; This was one of my bullet points -- I wanted to make it clear that this method isn't primitive (I was toying with the idea of removing it). If you still think it's better as an implementation detail, I'll move it back.
Oshima, bryeung suggested you could review this.
+bryeung,penghuang Whether or no this is right thing depends on IME/VK's behavior, but my guess is that this and original code may both wrong. SetPassword is to control visual aspect of the text in the textfield, while TextInputType is to control VirtualKeyboard. I'm guessing that it's possible to have IsPassword=false but TextInputType == Password (like "show password" option in auth dialog). Bryan, Peng, what do you think? http://codereview.chromium.org/8748001/diff/5/ui/views/controls/textfield/tex... File ui/views/controls/textfield/textfield.h (right): http://codereview.chromium.org/8748001/diff/5/ui/views/controls/textfield/tex... ui/views/controls/textfield/textfield.h:69: return style() & STYLE_PASSWORD; On 2011/11/30 19:54:24, benrg wrote: > This was one of my bullet points -- I wanted to make it clear that this method > isn't primitive (I was toying with the idea of removing it). If you still think > it's better as an implementation detail, I'll move it back. This one should be in .cc
http://codereview.chromium.org/8748001/diff/5/ui/views/controls/textfield/tex... File ui/views/controls/textfield/textfield.h (right): http://codereview.chromium.org/8748001/diff/5/ui/views/controls/textfield/tex... ui/views/controls/textfield/textfield.h:69: return style() & STYLE_PASSWORD; On 2011/12/01 22:08:22, oshima wrote: > This one should be in .cc Done.
I agree with Oshima's note. IsPassword on the textfield is not synonymous with a text input type of password: they are controlling slightly different things. Oshima: why do you think the previous code was wrong? You and I talked through it in some detail when I originally made that change. http://codereview.chromium.org/8748001/diff/11001/ui/views/controls/textfield... File ui/views/controls/textfield/textfield.cc (right): http://codereview.chromium.org/8748001/diff/11001/ui/views/controls/textfield... ui/views/controls/textfield/textfield.cc:76: text_input_type_(style & STYLE_PASSWORD ? I'd rather not duplicate the logic of IsPassword here.
Yes. We use TextInputType to control virtual keyboard visibility, and we also disable IME, if text input type is None or Password. So if a text field's content is password, even if the characters are visible, the TextInoutType should be set to password. On 2011/12/01 22:08:22, oshima wrote: > +bryeung,penghuang > > Whether or no this is right thing depends on IME/VK's behavior, but my guess is > that this and original code may both wrong. > > SetPassword is to control visual aspect of the text in the textfield, while > TextInputType is to control VirtualKeyboard. I'm guessing that it's possible to > have IsPassword=false but TextInputType == Password (like "show password" option > in auth dialog). > > Bryan, Peng, what do you think? > > http://codereview.chromium.org/8748001/diff/5/ui/views/controls/textfield/tex... > File ui/views/controls/textfield/textfield.h (right): > > http://codereview.chromium.org/8748001/diff/5/ui/views/controls/textfield/tex... > ui/views/controls/textfield/textfield.h:69: return style() & STYLE_PASSWORD; > On 2011/11/30 19:54:24, benrg wrote: > > This was one of my bullet points -- I wanted to make it clear that this method > > isn't primitive (I was toying with the idea of removing it). If you still > think > > it's better as an implementation detail, I'll move it back. > > This one should be in .cc
On 2011/12/02 16:22:28, bryeung wrote: > I agree with Oshima's note. IsPassword on the textfield is not synonymous with > a text input type of password: they are controlling slightly different things. > > Oshima: why do you think the previous code was wrong? You and I talked through > it in some detail when I originally made that change. Yes, I was wrong too. SetPassword currently changes the text input type, which probably shouldn't because I may just want to show the password, rather than changing to normal text type. > > http://codereview.chromium.org/8748001/diff/11001/ui/views/controls/textfield... > File ui/views/controls/textfield/textfield.cc (right): > > http://codereview.chromium.org/8748001/diff/11001/ui/views/controls/textfield... > ui/views/controls/textfield/textfield.cc:76: text_input_type_(style & > STYLE_PASSWORD ? > I'd rather not duplicate the logic of IsPassword here.
I just talked with Oshima and he said that SetPassword shouldn't set the text input type, SetTextInputType shouldn't set the style bit, and the constructor should set the text input type to a default value independent of the style bit. I'll implement that unless someone replies to stop me. I'm also going to rename STYLE_PASSWORD and associated methods to something else -- probably "hidden" for consistency with other widget toolkits. http://codereview.chromium.org/8748001/diff/11001/ui/views/controls/textfield... File ui/views/controls/textfield/textfield.cc (right): http://codereview.chromium.org/8748001/diff/11001/ui/views/controls/textfield... ui/views/controls/textfield/textfield.cc:76: text_input_type_(style & STYLE_PASSWORD ? On 2011/12/02 16:22:28, bryeung wrote: > I'd rather not duplicate the logic of IsPassword here. I did it that way to avoid calling a method while in violation of a class invariant, but it turns out the invariant wasn't supposed to hold anyway...
On 2011/12/02 18:21:41, benrg wrote: > I just talked with Oshima and he said that SetPassword shouldn't set the text > input type, SetTextInputType shouldn't set the style bit, and the constructor > should set the text input type to a default value independent of the style bit. > I'll implement that unless someone replies to stop me. We also agreed that STYLE_PASSWORD should be renamed to describe what it does more correctly (like STYLE_OBSCURED) to avoid confusion with input type. > > I'm also going to rename STYLE_PASSWORD and associated methods to something else > -- probably "hidden" for consistency with other widget toolkits. > > http://codereview.chromium.org/8748001/diff/11001/ui/views/controls/textfield... > File ui/views/controls/textfield/textfield.cc (right): > > http://codereview.chromium.org/8748001/diff/11001/ui/views/controls/textfield... > ui/views/controls/textfield/textfield.cc:76: text_input_type_(style & > STYLE_PASSWORD ? > On 2011/12/02 16:22:28, bryeung wrote: > > I'd rather not duplicate the logic of IsPassword here. > > I did it that way to avoid calling a method while in violation of a class > invariant, but it turns out the invariant wasn't supposed to hold anyway...
The changes are now implemented. I called it "obscured", but "hidden" is fine with me if anyone wants to push for it. A lot of code creates a Textfield with the hidden/obscured bit set and it's kind of inconvenient (and perhaps bug-prone) to add SetTextInputType(ui::TEXT_INPUT_TYPE_PASSWORD) to all of these places. I wonder if we shouldn't make Textfield::Textfield(StyleFlags) do it for convenience (i.e., back out all of my changes to that constructor).
On 2011/12/02 22:45:44, benrg wrote: > The changes are now implemented. I called it "obscured", but "hidden" is fine > with me if anyone wants to push for it. In views, the word "hide" is used to make things invisible, so I prefer obscured. > A lot of code creates a Textfield with > the hidden/obscured bit set and it's kind of inconvenient (and perhaps > bug-prone) to add SetTextInputType(ui::TEXT_INPUT_TYPE_PASSWORD) to all of these > places. I wonder if we shouldn't make Textfield::Textfield(StyleFlags) do it for > convenience (i.e., back out all of my changes to that constructor). I'm ok to set default type in constructor. Please add comment to SetObscured/SetTextInptType why we're not updating each other.
PTAL. http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield... File ui/views/controls/textfield/textfield.h (right): http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield... ui/views/controls/textfield/textfield.h:58: explicit Textfield(StyleFlags style); Is it worth adding TextInputType as a second argument to this constructor, instead of guessing it?
Just a couple of comment nits. Though I am a little worried by this I guess: it seems too easy to leak password data to an IME now (i.e. we set OBSCURED somewhere but forget to set the text input type). Are there use cases where we want OBSCURED & !PASSWORD, or do we really only care about !OBSCURED & PASSWORD? http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield... File ui/views/controls/textfield/native_textfield_gtk.h (right): http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield... ui/views/controls/textfield/native_textfield_gtk.h:74: // Returns true if the textfield is for password. please update the comment now as well http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield... File ui/views/controls/textfield/textfield.h (right): http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield... ui/views/controls/textfield/textfield.h:79: // disables the virtual keyboard and IME, but does not hide the text (use it does not disable the virtual keyboard, just the IME
bryeung wrote:
>it seems too easy to leak password
>data to an IME now (i.e. we set OBSCURED somewhere but forget to set the text
>input type). Are there use cases where we want OBSCURED & !PASSWORD, or do we
>really only care about !OBSCURED & PASSWORD?
I was wondering about this. Should SetObscured(true) set the text input type?
Should SetTextInputType(anything but PASSWORD) clear STYLE_OBSCURED to enforce
!(OBSCURED & !PASSWORD)?
Also, while grepping for comments with the word "password" in them, I noticed
this:
// We don't allow the input method to retrieve or delete content from a
// password box.
if (GetTextInputType() != ui::TEXT_INPUT_TYPE_TEXT)
return false;
Is this a bug, given the existence of other input types like URL? Should I fix
it in this CL?
http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield...
File ui/views/controls/textfield/native_textfield_gtk.h (right):
http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield...
ui/views/controls/textfield/native_textfield_gtk.h:74: // Returns true if the
textfield is for password.
On 2011/12/07 19:33:15, bryeung wrote:
> please update the comment now as well
Done. I hope that's all of them.
http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield...
File ui/views/controls/textfield/textfield.h (right):
http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield...
ui/views/controls/textfield/textfield.h:79: // disables the virtual keyboard and
IME, but does not hide the text (use
On 2011/12/07 19:33:15, bryeung wrote:
> it does not disable the virtual keyboard, just the IME
Changed. I misinterpreted Peng's comment.
On Wed, Dec 7, 2011 at 1:33 PM, <benrg@chromium.org> wrote: > bryeung wrote: > >> it seems too easy to leak password >> data to an IME now (i.e. we set OBSCURED somewhere but forget to set the >> text >> input type). Are there use cases where we want OBSCURED & !PASSWORD, or >> do we >> really only care about !OBSCURED & PASSWORD? >> > > I was wondering about this. Should SetObscured(true) set the text input > type? > Should SetTextInputType(anything but PASSWORD) clear STYLE_OBSCURED to > enforce > !(OBSCURED & !PASSWORD)? > > Also, while grepping for comments with the word "password" in them, I > noticed > this: > > // We don't allow the input method to retrieve or delete content from a > // password box. > if (GetTextInputType() != ui::TEXT_INPUT_TYPE_TEXT) > return false; > > Is this a bug, given the existence of other input types like URL? Should I > fix > it in this CL? Maybe we should add new method to TextInputClient to check if text is obfuscated or not? - oshima > > > > http://codereview.chromium.**org/8748001/diff/25001/ui/** > views/controls/textfield/**native_textfield_gtk.h<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/native_textfield_gtk.h> > File ui/views/controls/textfield/**native_textfield_gtk.h (right): > > http://codereview.chromium.**org/8748001/diff/25001/ui/** > views/controls/textfield/**native_textfield_gtk.h#**newcode74<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/native_textfield_gtk.h#newcode74> > ui/views/controls/textfield/**native_textfield_gtk.h:74: // Returns true > if the textfield is for password. > On 2011/12/07 19:33:15, bryeung wrote: > >> please update the comment now as well >> > > Done. I hope that's all of them. > > > http://codereview.chromium.**org/8748001/diff/25001/ui/** > views/controls/textfield/**textfield.h<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/textfield.h> > File ui/views/controls/textfield/**textfield.h (right): > > http://codereview.chromium.**org/8748001/diff/25001/ui/** > views/controls/textfield/**textfield.h#newcode79<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/textfield.h#newcode79> > ui/views/controls/textfield/**textfield.h:79: // disables the virtual > keyboard and IME, but does not hide the text (use > On 2011/12/07 19:33:15, bryeung wrote: > >> it does not disable the virtual keyboard, just the IME >> > > Changed. I misinterpreted Peng's comment. > > http://codereview.chromium.**org/8748001/<http://codereview.chromium.org/8748... >
On 2011/12/07 21:37:12, oshima wrote: > On Wed, Dec 7, 2011 at 1:33 PM, <mailto:benrg@chromium.org> wrote: > > > bryeung wrote: > > > >> it seems too easy to leak password > >> data to an IME now (i.e. we set OBSCURED somewhere but forget to set the > >> text > >> input type). Are there use cases where we want OBSCURED & !PASSWORD, or > >> do we > >> really only care about !OBSCURED & PASSWORD? > >> > > > > I was wondering about this. Should SetObscured(true) set the text input > > type? > > Should SetTextInputType(anything but PASSWORD) clear STYLE_OBSCURED to > > enforce > > !(OBSCURED & !PASSWORD)? > > > > Also, while grepping for comments with the word "password" in them, I > > noticed > > this: > > > > // We don't allow the input method to retrieve or delete content from a > > // password box. > > if (GetTextInputType() != ui::TEXT_INPUT_TYPE_TEXT) > > return false; > > > > Is this a bug, given the existence of other input types like URL? Should I > > fix > > it in this CL? > It is a bug. Please fix it. > > Maybe we should add new method to TextInputClient to check if text is > obfuscated or not? > I don't understand why need add this method and what's the use case. > - oshima > > > > > > > > > http://codereview.chromium.**org/8748001/diff/25001/ui/** > > > views/controls/textfield/**native_textfield_gtk.h<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/native_textfield_gtk.h> > > File ui/views/controls/textfield/**native_textfield_gtk.h (right): > > > > http://codereview.chromium.**org/8748001/diff/25001/ui/** > > > views/controls/textfield/**native_textfield_gtk.h#**newcode74<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/native_textfield_gtk.h#newcode74> > > ui/views/controls/textfield/**native_textfield_gtk.h:74: // Returns true > > if the textfield is for password. > > On 2011/12/07 19:33:15, bryeung wrote: > > > >> please update the comment now as well > >> > > > > Done. I hope that's all of them. > > > > > > http://codereview.chromium.**org/8748001/diff/25001/ui/** > > > views/controls/textfield/**textfield.h<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/textfield.h> > > File ui/views/controls/textfield/**textfield.h (right): > > > > http://codereview.chromium.**org/8748001/diff/25001/ui/** > > > views/controls/textfield/**textfield.h#newcode79<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/textfield.h#newcode79> > > ui/views/controls/textfield/**textfield.h:79: // disables the virtual > > keyboard and IME, but does not hide the text (use > > On 2011/12/07 19:33:15, bryeung wrote: > > > >> it does not disable the virtual keyboard, just the IME > >> > > > > Changed. I misinterpreted Peng's comment. > > > > > http://codereview.chromium.**org/8748001/%3Chttp://codereview.chromium.org/87...> > >
On Thu, Dec 8, 2011 at 9:13 AM, <penghuang@chromium.org> wrote: > On 2011/12/07 21:37:12, oshima wrote: > > On Wed, Dec 7, 2011 at 1:33 PM, <mailto:benrg@chromium.org> wrote: >> > > > bryeung wrote: >> > >> >> it seems too easy to leak password >> >> data to an IME now (i.e. we set OBSCURED somewhere but forget to set >> the >> >> text >> >> input type). Are there use cases where we want OBSCURED & !PASSWORD, >> or >> >> do we >> >> really only care about !OBSCURED & PASSWORD? >> >> >> > >> > I was wondering about this. Should SetObscured(true) set the text input >> > type? >> > Should SetTextInputType(anything but PASSWORD) clear STYLE_OBSCURED to >> > enforce >> > !(OBSCURED & !PASSWORD)? >> > >> > Also, while grepping for comments with the word "password" in them, I >> > noticed >> > this: >> > >> > // We don't allow the input method to retrieve or delete content from a >> > // password box. >> > if (GetTextInputType() != ui::TEXT_INPUT_TYPE_TEXT) >> > return false; >> > >> > Is this a bug, given the existence of other input types like URL? >> Should I >> > fix >> > it in this CL? >> > > > It is a bug. Please fix it. > > > > > Maybe we should add new method to TextInputClient to check if text is >> obfuscated or not? >> > > > I don't understand why need add this method and what's the use case. > Sorry I meant to reply to bryan's comment. I believe we don't have OBFUSCATED && !PASSWORD use case and we probably do not have to worry about, but if we want to make sure we never leaks something we don't want to expose, we can add new API to TextInputClient. I understand that setting text to password in SetObfuscated does work but I feel it's abuse. - oshima > > - oshima >> > > > >> > >> > >> > http://codereview.chromium.****org/8748001/diff/25001/ui/** >> > >> > > views/controls/textfield/****native_textfield_gtk.h<http://** > codereview.chromium.org/**8748001/diff/25001/ui/views/** > controls/textfield/native_**textfield_gtk.h<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/native_textfield_gtk.h> > > > >> > File ui/views/controls/textfield/****native_textfield_gtk.h (right): >> > >> > http://codereview.chromium.****org/8748001/diff/25001/ui/** >> > >> > > views/controls/textfield/****native_textfield_gtk.h#****newcode74< > http://codereview.**chromium.org/8748001/diff/**25001/ui/views/controls/** > textfield/native_textfield_**gtk.h#newcode74<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/native_textfield_gtk.h#newcode74> > > > >> > ui/views/controls/textfield/****native_textfield_gtk.h:74: // Returns >> true >> >> > if the textfield is for password. >> > On 2011/12/07 19:33:15, bryeung wrote: >> > >> >> please update the comment now as well >> >> >> > >> > Done. I hope that's all of them. >> > >> > >> > http://codereview.chromium.****org/8748001/diff/25001/ui/** >> > >> > > views/controls/textfield/****textfield.h<http://codereview.** > chromium.org/8748001/diff/**25001/ui/views/controls/** > textfield/textfield.h<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/textfield.h> > > > >> > File ui/views/controls/textfield/****textfield.h (right): >> > >> > http://codereview.chromium.****org/8748001/diff/25001/ui/** >> > >> > > views/controls/textfield/****textfield.h#newcode79<http://** > codereview.chromium.org/**8748001/diff/25001/ui/views/** > controls/textfield/textfield.**h#newcode79<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/textfield.h#newcode79> > > > >> > ui/views/controls/textfield/****textfield.h:79: // disables the virtual >> >> > keyboard and IME, but does not hide the text (use >> > On 2011/12/07 19:33:15, bryeung wrote: >> > >> >> it does not disable the virtual keyboard, just the IME >> >> >> > >> > Changed. I misinterpreted Peng's comment. >> > >> > >> > > http://codereview.chromium.****org/8748001/%3Chttp://coderevi** > ew.chromium.org/8748001/ <http://codereview.chromium.org/8748001/>> > >> > >> > > > > http://codereview.chromium.**org/8748001/<http://codereview.chromium.org/8748... >
On 2011/12/08 17:40:49, oshima wrote: > On Thu, Dec 8, 2011 at 9:13 AM, <mailto:penghuang@chromium.org> wrote: > > > On 2011/12/07 21:37:12, oshima wrote: > > > > On Wed, Dec 7, 2011 at 1:33 PM, <mailto:benrg@chromium.org> wrote: > >> > > > > > bryeung wrote: > >> > > >> >> it seems too easy to leak password > >> >> data to an IME now (i.e. we set OBSCURED somewhere but forget to set > >> the > >> >> text > >> >> input type). Are there use cases where we want OBSCURED & !PASSWORD, > >> or > >> >> do we > >> >> really only care about !OBSCURED & PASSWORD? > >> >> > >> > > >> > I was wondering about this. Should SetObscured(true) set the text input > >> > type? > >> > Should SetTextInputType(anything but PASSWORD) clear STYLE_OBSCURED to > >> > enforce > >> > !(OBSCURED & !PASSWORD)? > >> > > >> > Also, while grepping for comments with the word "password" in them, I > >> > noticed > >> > this: > >> > > >> > // We don't allow the input method to retrieve or delete content from a > >> > // password box. > >> > if (GetTextInputType() != ui::TEXT_INPUT_TYPE_TEXT) > >> > return false; > >> > > >> > Is this a bug, given the existence of other input types like URL? > >> Should I > >> > fix > >> > it in this CL? > >> > > > > > > It is a bug. Please fix it. > > > > > > > > > > Maybe we should add new method to TextInputClient to check if text is > >> obfuscated or not? > >> > > > > > > I don't understand why need add this method and what's the use case. > > > > Sorry I meant to reply to bryan's comment. I believe we don't have > OBFUSCATED && !PASSWORD use case and > we probably do not have to worry about, but if we want to make sure we > never leaks something we don't want > to expose, we can add new API to TextInputClient. I understand that setting > text to password in SetObfuscated > does work but I feel it's abuse. I personal believe when textfield type is not PASSWORD, the OBFUSCATED style should be ignored. The text field should always show the text. Does it make sense? > > - oshima > > > > > > > - oshima > >> > > > > > > >> > > >> > > >> > http://codereview.chromium.****org/8748001/diff/25001/ui/** > >> > > >> > > > > views/controls/textfield/****native_textfield_gtk.h<http://** > > codereview.chromium.org/**8748001/diff/25001/ui/views/** > > > controls/textfield/native_**textfield_gtk.h<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/native_textfield_gtk.h> > > > > > > >> > File ui/views/controls/textfield/****native_textfield_gtk.h (right): > >> > > >> > http://codereview.chromium.****org/8748001/diff/25001/ui/** > >> > > >> > > > > views/controls/textfield/****native_textfield_gtk.h#****newcode74< > > http://codereview.**chromium.org/8748001/diff/**25001/ui/views/controls/** > > > textfield/native_textfield_**gtk.h#newcode74<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/native_textfield_gtk.h#newcode74> > > > > > > >> > ui/views/controls/textfield/****native_textfield_gtk.h:74: // Returns > >> true > >> > >> > if the textfield is for password. > >> > On 2011/12/07 19:33:15, bryeung wrote: > >> > > >> >> please update the comment now as well > >> >> > >> > > >> > Done. I hope that's all of them. > >> > > >> > > >> > http://codereview.chromium.****org/8748001/diff/25001/ui/** > >> > > >> > > > > views/controls/textfield/****textfield.h<http://codereview.** > > chromium.org/8748001/diff/**25001/ui/views/controls/** > > > textfield/textfield.h<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/textfield.h> > > > > > > >> > File ui/views/controls/textfield/****textfield.h (right): > >> > > >> > http://codereview.chromium.****org/8748001/diff/25001/ui/** > >> > > >> > > > > views/controls/textfield/****textfield.h#newcode79<http://** > > codereview.chromium.org/**8748001/diff/25001/ui/views/** > > > controls/textfield/textfield.**h#newcode79<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/textfield.h#newcode79> > > > > > > >> > ui/views/controls/textfield/****textfield.h:79: // disables the virtual > >> > >> > keyboard and IME, but does not hide the text (use > >> > On 2011/12/07 19:33:15, bryeung wrote: > >> > > >> >> it does not disable the virtual keyboard, just the IME > >> >> > >> > > >> > Changed. I misinterpreted Peng's comment. > >> > > >> > > >> > > > > http://codereview.chromium.****org/8748001/%253Chttp://coderevi** > > ew.chromium.org/8748001/ <http://codereview.chromium.org/8748001/>> > > > >> > > >> > > > > > > > > > http://codereview.chromium.**org/8748001/%3Chttp://codereview.chromium.org/87...> > >
On Thu, Dec 8, 2011 at 9:51 AM, <penghuang@chromium.org> wrote: > On 2011/12/08 17:40:49, oshima wrote: > > On Thu, Dec 8, 2011 at 9:13 AM, <mailto:penghuang@chromium.org**> wrote: >> > > > On 2011/12/07 21:37:12, oshima wrote: >> > >> > On Wed, Dec 7, 2011 at 1:33 PM, <mailto:benrg@chromium.org> wrote: >> >> >> > >> > > bryeung wrote: >> >> > >> >> >> it seems too easy to leak password >> >> >> data to an IME now (i.e. we set OBSCURED somewhere but forget to set >> >> the >> >> >> text >> >> >> input type). Are there use cases where we want OBSCURED & >> !PASSWORD, >> >> or >> >> >> do we >> >> >> really only care about !OBSCURED & PASSWORD? >> >> >> >> >> > >> >> > I was wondering about this. Should SetObscured(true) set the text >> input >> >> > type? >> >> > Should SetTextInputType(anything but PASSWORD) clear STYLE_OBSCURED >> to >> >> > enforce >> >> > !(OBSCURED & !PASSWORD)? >> >> > >> >> > Also, while grepping for comments with the word "password" in them, I >> >> > noticed >> >> > this: >> >> > >> >> > // We don't allow the input method to retrieve or delete content >> from a >> >> > // password box. >> >> > if (GetTextInputType() != ui::TEXT_INPUT_TYPE_TEXT) >> >> > return false; >> >> > >> >> > Is this a bug, given the existence of other input types like URL? >> >> Should I >> >> > fix >> >> > it in this CL? >> >> >> > >> > >> > It is a bug. Please fix it. >> > >> > >> > >> > >> > Maybe we should add new method to TextInputClient to check if text is >> >> obfuscated or not? >> >> >> > >> > >> > I don't understand why need add this method and what's the use case. >> > >> > > Sorry I meant to reply to bryan's comment. I believe we don't have >> OBFUSCATED && !PASSWORD use case and >> we probably do not have to worry about, but if we want to make sure we >> never leaks something we don't want >> to expose, we can add new API to TextInputClient. I understand that >> setting >> text to password in SetObfuscated >> does work but I feel it's abuse. >> > > I personal believe when textfield type is not PASSWORD, the OBFUSCATED > style > should be ignored. The text field should always show the text. Does it make > sense? > I prefer to keep them separated because they're different concept. Or if we want to do it, then we should remove SetTextInputType and move it to constructor so API is not order dependent. - oshima > > > - oshima >> > > > > > >> > - oshima >> >> >> > >> > > >> >> > >> >> > >> >> > http://codereview.chromium.******org/8748001/diff/25001/ui/** >> >> > >> >> >> > >> > views/controls/textfield/******native_textfield_gtk.h<http://**** >> > codereview.chromium.org/****8748001/diff/25001/ui/views/**<http://codereview.chromium.org/**8748001/diff/25001/ui/views/**> >> > >> > > controls/textfield/native_****textfield_gtk.h<http://** > codereview.chromium.org/**8748001/diff/25001/ui/views/** > controls/textfield/native_**textfield_gtk.h<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/native_textfield_gtk.h> > > > >> > > >> > >> >> > File ui/views/controls/textfield/******native_textfield_gtk.h >> (right): >> >> > >> >> > http://codereview.chromium.******org/8748001/diff/25001/ui/** >> >> > >> >> >> > >> > views/controls/textfield/******native_textfield_gtk.h#******newcode74< >> > http://codereview.**chromium.**org/8748001/diff/**25001/ui/** >> views/controls/**<http://chromium.org/8748001/diff/**25001/ui/views/controls/**> >> > >> > > textfield/native_textfield_****gtk.h#newcode74<http://** > codereview.chromium.org/**8748001/diff/25001/ui/views/** > controls/textfield/native_**textfield_gtk.h#newcode74<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/native_textfield_gtk.h#newcode74> > > > >> > > >> > >> >> > ui/views/controls/textfield/******native_textfield_gtk.h:74: // >> Returns >> >> >> true >> >> >> >> > if the textfield is for password. >> >> > On 2011/12/07 19:33:15, bryeung wrote: >> >> > >> >> >> please update the comment now as well >> >> >> >> >> > >> >> > Done. I hope that's all of them. >> >> > >> >> > >> >> > http://codereview.chromium.******org/8748001/diff/25001/ui/** >> >> > >> >> >> > >> > views/controls/textfield/******textfield.h<http://codereview.**** >> > chromium.org/8748001/diff/****25001/ui/views/controls/**<http://chromium.org/8748001/diff/**25001/ui/views/controls/**> >> > >> > > textfield/textfield.h<http://**codereview.chromium.org/** > 8748001/diff/25001/ui/views/**controls/textfield/textfield.h<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/textfield.h> > **> > >> > > >> > >> >> > File ui/views/controls/textfield/******textfield.h (right): >> >> > >> >> > http://codereview.chromium.******org/8748001/diff/25001/ui/** >> >> > >> >> >> > >> > views/controls/textfield/******textfield.h#newcode79<http://**** >> > codereview.chromium.org/****8748001/diff/25001/ui/views/**<http://codereview.chromium.org/**8748001/diff/25001/ui/views/**> >> > >> > > controls/textfield/textfield.****h#newcode79<http://** > codereview.chromium.org/**8748001/diff/25001/ui/views/** > controls/textfield/textfield.**h#newcode79<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/textfield.h#newcode79> > > > >> > > >> > >> >> > ui/views/controls/textfield/******textfield.h:79: // disables the >> virtual >> >> >> >> >> > keyboard and IME, but does not hide the text (use >> >> > On 2011/12/07 19:33:15, bryeung wrote: >> >> > >> >> >> it does not disable the virtual keyboard, just the IME >> >> >> >> >> > >> >> > Changed. I misinterpreted Peng's comment. >> >> > >> >> > >> >> >> > >> > http://codereview.chromium.******org/8748001/%253Chttp://**coderevi** >> > ew.chromium.org/8748001/ <http://codereview.chromium.**org/8748001/<http://codereview.chromium.org/8748... >> >> >> > >> >> > >> >> >> > >> > >> > >> > >> > > http://codereview.chromium.****org/8748001/%3Chttp://coderevi** > ew.chromium.org/8748001/ <http://codereview.chromium.org/8748001/>> > >> > >> > > > > http://codereview.chromium.**org/8748001/<http://codereview.chromium.org/8748... >
Just drive by :) On 2011/12/07 21:37:12, oshima wrote: > On Wed, Dec 7, 2011 at 1:33 PM, <mailto:benrg@chromium.org> wrote: > > > bryeung wrote: > > > >> it seems too easy to leak password > >> data to an IME now (i.e. we set OBSCURED somewhere but forget to set the > >> text > >> input type). Are there use cases where we want OBSCURED & !PASSWORD, or > >> do we > >> really only care about !OBSCURED & PASSWORD? > >> > > > > I was wondering about this. Should SetObscured(true) set the text input > > type? > > Should SetTextInputType(anything but PASSWORD) clear STYLE_OBSCURED to > > enforce > > !(OBSCURED & !PASSWORD)? > > > > Also, while grepping for comments with the word "password" in them, I > > noticed > > this: > > > > // We don't allow the input method to retrieve or delete content from a > > // password box. > > if (GetTextInputType() != ui::TEXT_INPUT_TYPE_TEXT) > > return false; > > > > Is this a bug, given the existence of other input types like URL? Should I > > fix > > it in this CL? > > > Maybe we should add new method to TextInputClient to check if text is > obfuscated or not? If we want to disable the IME in an obfuscated (but not password) text box, then we should have something in TextInputClient to let the IME know it. Otherwise we shouldn't add it to TextInputClient. I'm actually wondering is there any possibility that we want to obfuscate a non-password text box? If not, then we don't need to add anything in TextInputClient. > > - oshima > > > > > > > > > http://codereview.chromium.**org/8748001/diff/25001/ui/** > > > views/controls/textfield/**native_textfield_gtk.h<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/native_textfield_gtk.h> > > File ui/views/controls/textfield/**native_textfield_gtk.h (right): > > > > http://codereview.chromium.**org/8748001/diff/25001/ui/** > > > views/controls/textfield/**native_textfield_gtk.h#**newcode74<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/native_textfield_gtk.h#newcode74> > > ui/views/controls/textfield/**native_textfield_gtk.h:74: // Returns true > > if the textfield is for password. > > On 2011/12/07 19:33:15, bryeung wrote: > > > >> please update the comment now as well > >> > > > > Done. I hope that's all of them. > > > > > > http://codereview.chromium.**org/8748001/diff/25001/ui/** > > > views/controls/textfield/**textfield.h<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/textfield.h> > > File ui/views/controls/textfield/**textfield.h (right): > > > > http://codereview.chromium.**org/8748001/diff/25001/ui/** > > > views/controls/textfield/**textfield.h#newcode79<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/textfield.h#newcode79> > > ui/views/controls/textfield/**textfield.h:79: // disables the virtual > > keyboard and IME, but does not hide the text (use > > On 2011/12/07 19:33:15, bryeung wrote: > > > >> it does not disable the virtual keyboard, just the IME > >> > > > > Changed. I misinterpreted Peng's comment. > > > > > http://codereview.chromium.**org/8748001/%3Chttp://codereview.chromium.org/87...> > >
On Thu, Dec 8, 2011 at 5:57 PM, <suzhe@chromium.org> wrote: > Just drive by :) > > > On 2011/12/07 21:37:12, oshima wrote: > >> On Wed, Dec 7, 2011 at 1:33 PM, <mailto:benrg@chromium.org> wrote: >> > > > bryeung wrote: >> > >> >> it seems too easy to leak password >> >> data to an IME now (i.e. we set OBSCURED somewhere but forget to set >> the >> >> text >> >> input type). Are there use cases where we want OBSCURED & !PASSWORD, >> or >> >> do we >> >> really only care about !OBSCURED & PASSWORD? >> >> >> > >> > I was wondering about this. Should SetObscured(true) set the text input >> > type? >> > Should SetTextInputType(anything but PASSWORD) clear STYLE_OBSCURED to >> > enforce >> > !(OBSCURED & !PASSWORD)? >> > >> > Also, while grepping for comments with the word "password" in them, I >> > noticed >> > this: >> > >> > // We don't allow the input method to retrieve or delete content from a >> > // password box. >> > if (GetTextInputType() != ui::TEXT_INPUT_TYPE_TEXT) >> > return false; >> > >> > Is this a bug, given the existence of other input types like URL? >> Should I >> > fix >> > it in this CL? >> > > > Maybe we should add new method to TextInputClient to check if text is >> obfuscated or not? >> > If we want to disable the IME in an obfuscated (but not password) text > box, then > we should have something in TextInputClient to let the IME know it. > Otherwise we > shouldn't add it to TextInputClient. > I'm actually wondering is there any possibility that we want to obfuscate a > non-password text box? If not, then we don't need to add anything in > TextInputClient. > > This is just one approach to address Bryan's concern: > i.e. we set OBSCURED somewhere but forget to set the text input type). It is possible that password field code is regressed and textfield is set to text while textfield is still being set as obfuscated. We should have test case to catch such issue, but since this is security related, extra guard is good thing. Removing SetTextInputType is another additional option, so that we always set style and input type at the same place, which makes it less error prone. I guess there is no need to change the textfield type once it's set, although I'm not 100% sure. - oshima > > > - oshima >> > > > >> > >> > >> > http://codereview.chromium.****org/8748001/diff/25001/ui/** >> > >> > > views/controls/textfield/****native_textfield_gtk.h<http://** > codereview.chromium.org/**8748001/diff/25001/ui/views/** > controls/textfield/native_**textfield_gtk.h<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/native_textfield_gtk.h> > > > >> > File ui/views/controls/textfield/****native_textfield_gtk.h (right): >> > >> > http://codereview.chromium.****org/8748001/diff/25001/ui/** >> > >> > > views/controls/textfield/****native_textfield_gtk.h#****newcode74< > http://codereview.**chromium.org/8748001/diff/**25001/ui/views/controls/** > textfield/native_textfield_**gtk.h#newcode74<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/native_textfield_gtk.h#newcode74> > > > >> > ui/views/controls/textfield/****native_textfield_gtk.h:74: // Returns >> true >> >> > if the textfield is for password. >> > On 2011/12/07 19:33:15, bryeung wrote: >> > >> >> please update the comment now as well >> >> >> > >> > Done. I hope that's all of them. >> > >> > >> > http://codereview.chromium.****org/8748001/diff/25001/ui/** >> > >> > > views/controls/textfield/****textfield.h<http://codereview.** > chromium.org/8748001/diff/**25001/ui/views/controls/** > textfield/textfield.h<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/textfield.h> > > > >> > File ui/views/controls/textfield/****textfield.h (right): >> > >> > http://codereview.chromium.****org/8748001/diff/25001/ui/** >> > >> > > views/controls/textfield/****textfield.h#newcode79<http://** > codereview.chromium.org/**8748001/diff/25001/ui/views/** > controls/textfield/textfield.**h#newcode79<http://codereview.chromium.org/8748001/diff/25001/ui/views/controls/textfield/textfield.h#newcode79> > > > >> > ui/views/controls/textfield/****textfield.h:79: // disables the virtual >> >> > keyboard and IME, but does not hide the text (use >> > On 2011/12/07 19:33:15, bryeung wrote: >> > >> >> it does not disable the virtual keyboard, just the IME >> >> >> > >> > Changed. I misinterpreted Peng's comment. >> > >> > >> > > http://codereview.chromium.****org/8748001/%3Chttp://coderevi** > ew.chromium.org/8748001/ <http://codereview.chromium.org/8748001/>> > >> > >> > > > > http://codereview.chromium.**org/8748001/<http://codereview.chromium.org/8748... >
It's been a while, but everyone who's interested in this PTAL. Because of other checkins in the meantime, the latest patch touches only five files instead of 20. The change itself is pretty simple, but I'm not sure it was ever decided if it's the right thing to do.
http://codereview.chromium.org/8748001/diff/50001/ui/views/view_unittest.cc File ui/views/view_unittest.cc (right): http://codereview.chromium.org/8748001/diff/50001/ui/views/view_unittest.cc#n... ui/views/view_unittest.cc:969: password->SetObscured(true); can you add tests in native_textfied_views as well? This is win only and doesn't run on chromeos aura
On 2012/01/30 23:56:23, oshima wrote: > http://codereview.chromium.org/8748001/diff/50001/ui/views/view_unittest.cc > File ui/views/view_unittest.cc (right): > > http://codereview.chromium.org/8748001/diff/50001/ui/views/view_unittest.cc#n... > ui/views/view_unittest.cc:969: password->SetObscured(true); > can you add tests in native_textfied_views as well? This is win only and doesn't > run on chromeos aura change looks ok. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
