[Mac, Win] Don't allow blank spaces in profile names in the new avatar bubble.

[Mac, Win] Don't allow blank spaces in profile names in the new avatar bubble.

TrimString ftw.

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

[noms (inactive), 2014-08-14 23:58:26]

Two line CLs for everyone! 
scott: views
alexei: cocoa.

Thanks! :)

[sky, 2014-08-15 04:38:41]

https://codereview.chromium.org/474123002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/474123002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:803:
base::TrimString(new_profile_name, base::ASCIIToUTF16(" "),
Might it be possible to get other characters in the name you don't want by way
of pasting? I'm wondering if this should trim all whitespace.

[noms (inactive), 2014-08-15 13:32:41]

https://codereview.chromium.org/474123002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/474123002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:803:
base::TrimString(new_profile_name, base::ASCIIToUTF16(" "),
On 2014/08/15 04:38:41, sky wrote:
> Might it be possible to get other characters in the name you don't want by way
> of pasting? I'm wondering if this should trim all whitespace.

But if you paste it in, isn't it still in the text() of the textfield? Do you
mean trimming \t and \n?

[Alexei Svitkine (slow), 2014-08-15 17:25:26]

https://codereview.chromium.org/474123002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/474123002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:630:
base::TrimString(newProfileName, base::ASCIIToUTF16(" "), &newProfileName);
Why not use base::TrimWhitespace() instead, so it handles other whitespace
chars?

[noms (inactive), 2014-08-15 17:37:40]

Oh, there's a better function! Thanks Alexei!

https://codereview.chromium.org/474123002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/474123002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:630:
base::TrimString(newProfileName, base::ASCIIToUTF16(" "), &newProfileName);
On 2014/08/15 17:25:26, Alexei Svitkine wrote:
> Why not use base::TrimWhitespace() instead, so it handles other whitespace
> chars?

Done.

[Alexei Svitkine (slow), 2014-08-15 17:40:47]

https://codereview.chromium.org/474123002/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/474123002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:635: [self
setTitle:originalText];
If this really needs to be |originalText| instead of |newProfileName|, please
add a comment explaining why, else it looks like a bug.

https://codereview.chromium.org/474123002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:642: // entered a
string that was rejected (eg. all spaces).
If the previous string was rejected, then it wouldn't ever be set as the title,
so this comment doesn't seem correct to me.

[noms (inactive), 2014-08-15 18:14:28]

Alexei: done! I'm sorry for the rebase that also when in this patch. :(

https://codereview.chromium.org/474123002/diff/40001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/474123002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:635: [self
setTitle:originalText];
Oh, now that I am doing the trimming you're right, it's worth converting back to
an NSString*. I was just trying to (incorrectly) save a conversion.

On 2014/08/15 17:40:46, Alexei Svitkine wrote:
> If this really needs to be |originalText| instead of |newProfileName|, please
> add a comment explaining why, else it looks like a bug.

https://codereview.chromium.org/474123002/diff/40001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:642: // entered a
string that was rejected (eg. all spaces).
If the string is incorrect, we cancel editing, don't set the button title, but
the old, rejected text is still in the textbox and shouldn't be. I have moved
this to the  "if it's rejected" case above to make it clearer.

On 2014/08/15 17:40:47, Alexei Svitkine wrote:
> If the previous string was rejected, then it wouldn't ever be set as the
title,
> so this comment doesn't seem correct to me.

[Alexei Svitkine (slow), 2014-08-15 18:26:26]

LGTM, new code is much clearer, thanks!

[noms (inactive), 2014-08-15 19:14:19]

sky: ping

[sky, 2014-08-15 19:28:37]

LGTM

https://codereview.chromium.org/474123002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/474123002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:803:
base::TrimString(new_profile_name, base::ASCIIToUTF16(" "),
On 2014/08/15 13:32:41, Monica Dinculescu wrote:
> On 2014/08/15 04:38:41, sky wrote:
> > Might it be possible to get other characters in the name you don't want by
way
> > of pasting? I'm wondering if this should trim all whitespace.
> 
> But if you paste it in, isn't it still in the text() of the textfield? Do you
> mean trimming \t and \n?

Exactly.

[noms (inactive), 2014-08-15 19:28:56]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-08-15 19:32:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/noms@chromium.org/474123002/60001

[commit-bot: I haz the power, 2014-08-16 05:58:43]

Committed patchset #3 (60001) as 290099