Password bubble: Add a test for displayed password length.

Password bubble: Obscure password values via Label::SetObscured.

Rather than obscuring the password value manually, we can now rely on
Label's built-in obfuscation. This has the nice side-effect of dealing
correctly with surrogate pairs.

This does mean that we're relying on the built-in eliding functionality
rather than capping the displayed length of passwords at 22 characters.
I think that's a pretty reasonable thing to do, actually; if I have a 
thousand-character password (it could happen!), seeing an ellipsis would
match my expectations around what the bubble should display.

BUG=328339
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261799

[Mike West, 2014-04-02 09:28:19]

Hi Markus and Patrick!

Regardless of what we decide to do with
https://codereview.chromium.org/221003002/, we should have a test for the
result. This CL adds one.

It's fairly trivial, but it adds a file we can build upon, and hey: one test is
better than none at all, right?

-mike

[Patrick Dubroy, 2014-04-02 09:41:50]

https://codereview.chromium.org/221853003/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right):

https://codereview.chromium.org/221853003/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:104: base::string16
ManagePasswordsBubbleModel::GetPasswordDisplayString(
I definitely think adding some tests for the ManagePasswordsBubbleModel is a
good idea, but I'm not sure this method actually belongs on the model. On OS X,
for example, we'd probably use NSSecureTextField rather than manually replacing
the characters with bullets.

[Mike West, 2014-04-03 06:45:27]

Putting this on hold until https://codereview.chromium.org/222033002/ is worked
out.

[Mike West, 2014-04-04 07:52:44]

Vaclav, can I fall back on you for a pre-review so I can get someone overseas to
stamp it?

[Mike West, 2014-04-04 07:53:13]

On 2014/04/04 07:52:44, Mike West wrote:
> Vaclav, can I fall back on you for a pre-review so I can get someone overseas
to
> stamp it?

(Note that this relies on https://codereview.chromium.org/222033002/ landing, so
I can't throw it to the bots quite yet...)

[vabr (Chromium), 2014-04-04 08:14:00]

LGTM, in particular I see no concerns with not capping the length of the
password at 22 characters.

Having said that, I remember people suggesting always displaying a fixed number
of bullets/asterisks, regardless of the password length, to increase security.
Did we consider this?

Also, please make sure to quickly check by running Chrome with your patch, that
the passwords are obscured when they should be (I'm afraid I could have missed a
code branch when checking the patch for the presence of the SetObscured).

Thanks!

Vaclav

[Mike West, 2014-04-04 08:44:12]

On 2014/04/04 08:14:00, vabr (Chromium) wrote:
> LGTM, in particular I see no concerns with not capping the length of the
> password at 22 characters.
> 
> Having said that, I remember people suggesting always displaying a fixed
number
> of bullets/asterisks, regardless of the password length, to increase security.
> Did we consider this?

Once we come to agreement on whether or not we'd like to do that, I'll take care
of it in a separate patch: https://codereview.chromium.org/221003002/
 
> Also, please make sure to quickly check by running Chrome with your patch,
that
> the passwords are obscured when they should be (I'm afraid I could have missed
a
> code branch when checking the patch for the presence of the SetObscured).

Will do!

[vabr (Chromium), 2014-04-04 08:47:16]

On 2014/04/04 08:44:12, Mike West wrote:
> On 2014/04/04 08:14:00, vabr (Chromium) wrote:
> > LGTM, in particular I see no concerns with not capping the length of the
> > password at 22 characters.
> > 
> > Having said that, I remember people suggesting always displaying a fixed
> number
> > of bullets/asterisks, regardless of the password length, to increase
security.
> > Did we consider this?
> 
> Once we come to agreement on whether or not we'd like to do that, I'll take
care
> of it in a separate patch: https://codereview.chromium.org/221003002/
SGTM!
>  
> > Also, please make sure to quickly check by running Chrome with your patch,
> that
> > the passwords are obscured when they should be (I'm afraid I could have
missed
> a
> > code branch when checking the patch for the presence of the SetObscured).
> 
> Will do!

Thanks, LGTM.

[markusheintz_, 2014-04-04 12:48:15]

LGTM.

[markusheintz_, 2014-04-04 12:49:09]

On 2014/04/04 12:48:15, markusheintz_ wrote:
> LGTM.

Oh BTW: The CL description seems to be not

[markusheintz_, 2014-04-04 12:49:09]

On 2014/04/04 12:48:15, markusheintz_ wrote:
> LGTM.

Oh BTW: The CL description seems to be not

[markusheintz_, 2014-04-04 12:49:10]

On 2014/04/04 12:48:15, markusheintz_ wrote:
> LGTM.

Oh BTW: The CL description seems to be not

[markusheintz_, 2014-04-04 12:49:45]

The CL description is a bit off. Could you fix it?
Thanks

[Mike West, 2014-04-04 12:51:25]

On 2014/04/04 12:49:45, markusheintz_ wrote:
> The CL description is a bit off. Could you fix it?
> Thanks

The title of the review is old; the CL description itself looks reasonable:
"Password bubble: Obscure password values via Label::SetObscured."?

[Mike West, 2014-04-04 12:51:39]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-04-04 12:52:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/221853003/20001

[commit-bot: I haz the power, 2014-04-04 15:28:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/221853003/20001

[commit-bot: I haz the power, 2014-04-04 18:34:29]

Change committed as 261799