Linux: allow using GNOME Keyring and KWallet for password storage via a command line flag.

Linux: allow using GNOME Keyring and KWallet for password storage via a command line flag, disabled by default.
BUG=12351, 25404, 45896
TEST=still uses default password store unless --password-store is given with a value of detect, gnome, or kwallet

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=50475

[Evan Stade, 2010-06-10 23:40:40]

seems like we might want the migration code in place before landing this

http://codereview.chromium.org/2730008/diff/1/2
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/2730008/diff/1/2#newcode1289
chrome/browser/profile.cc:1289: } else if (store_type == L"detect") {
this one is temporary, right?

http://codereview.chromium.org/2730008/diff/1/2#newcode1293
chrome/browser/profile.cc:1293: base::GetDesktopEnvironmentName(desktop_env);
too much indent

http://codereview.chromium.org/2730008/diff/1/2#newcode1303
chrome/browser/profile.cc:1303: ps = new PasswordStoreKWallet(login_db, this,
nit: when you wrap between args, you have to align the args that are on
different lines.

[Mike Mammarella, 2010-06-10 23:55:08]

Do you think we need the migration before even allowing this as an option behind
a command-line flag? It's true that some users will probably start using it and
then cause migration to be less clean for them when we do it later, I suppose.
But then, those users knew what they were getting into...

http://codereview.chromium.org/2730008/diff/1/2
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/2730008/diff/1/2#newcode1289
chrome/browser/profile.cc:1289: } else if (store_type == L"detect") {
On 2010/06/10 23:40:40, Evan Stade wrote:
> this one is temporary, right?

Yeah, eventually this will be the default. But this CL leaves the default as the
built-in unencrypted store.

http://codereview.chromium.org/2730008/diff/1/2#newcode1293
chrome/browser/profile.cc:1293: base::GetDesktopEnvironmentName(desktop_env);
On 2010/06/10 23:40:40, Evan Stade wrote:
> too much indent

This is what the googley vim indent rules did automatically... I think it's
supposed to be a special case for the LOG() << structure, since normally it does
only 4 spaces for this kind of thing.

http://codereview.chromium.org/2730008/diff/1/2#newcode1303
chrome/browser/profile.cc:1303: ps = new PasswordStoreKWallet(login_db, this,
On 2010/06/10 23:40:40, Evan Stade wrote:
> nit: when you wrap between args, you have to align the args that are on
> different lines.

Done.

[Evan Stade, 2010-06-10 23:59:55]

http://codereview.chromium.org/2730008/diff/1/2
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/2730008/diff/1/2#newcode1289
chrome/browser/profile.cc:1289: } else if (store_type == L"detect") {
On 2010/06/10 23:55:08, Mike Mammarella wrote:
> On 2010/06/10 23:40:40, Evan Stade wrote:
> > this one is temporary, right?
> 
> Yeah, eventually this will be the default. But this CL leaves the default as
the
> built-in unencrypted store.

I'd argue that even power-users would prefer their stuff get migrated rather
than having to type it all in from scratch. If they've lived without a wallet
store for this long, prolonging it for another day or week probably won't hurt.

http://codereview.chromium.org/2730008/diff/1/2#newcode1293
chrome/browser/profile.cc:1293: base::GetDesktopEnvironmentName(desktop_env);
On 2010/06/10 23:55:08, Mike Mammarella wrote:
> On 2010/06/10 23:40:40, Evan Stade wrote:
> > too much indent
> 
> This is what the googley vim indent rules did automatically... I think it's
> supposed to be a special case for the LOG() << structure, since normally it
does
> only 4 spaces for this kind of thing.

ok

[Evan Stade, 2010-06-11 00:01:35]

> I'd argue that even power-users [...]

(oops, responded to the wrong comment there.)

[Mike Mammarella, 2010-06-16 23:07:55]

OK, now we can migrate. This CL leaves the default behavior as it is now, using
the default store always. But with the flag, it'll migrate your passwords to the
new store.

[Mike Mammarella, 2010-06-21 21:01:33]

ping

[Evan Martin, 2010-06-21 21:05:23]

http://codereview.chromium.org/2730008/diff/14001/15001
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/2730008/diff/14001/15001#newcode1350
chrome/browser/profile.cc:1350: // GNOME keyring fails for some reason. This
scheme is kind of arbitrary.
If you're going to do something arbitrary, why not the simpler (gnome-keyring in
gnome) || (kwallet in kde) one?

http://codereview.chromium.org/2730008/diff/14001/15001#newcode1407
chrome/browser/profile.cc:1407: NOTREACHED() << "Could not initialize password
manager.";
Heh, I think the original patch was by a Scot.

[Evan Stade, 2010-06-21 21:14:00]

nits... thank you for satisfying my neuroses

http://codereview.chromium.org/2730008/diff/14001/15001
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/2730008/diff/14001/15001#newcode1369
chrome/browser/profile.cc:1369: }
nit: can you add a blank line

http://codereview.chromium.org/2730008/diff/14001/15001#newcode1370
chrome/browser/profile.cc:1370: PasswordStoreX::NativeBackend* backend;
I think this is a good place to use scoped_ptr

http://codereview.chromium.org/2730008/diff/14001/15001#newcode1381
chrome/browser/profile.cc:1381: // Fall through.
all these fall throughs and conditionalized breaks are kind of unexepected in a
switch/case. I think the code would be clearer as a series of ifs.

http://codereview.chromium.org/2730008/diff/14001/15001#newcode1395
chrome/browser/profile.cc:1395: }
nit: vertical whitespace here as well

[Mike Mammarella, 2010-06-21 21:50:32]

http://codereview.chromium.org/2730008/diff/14001/15001
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/2730008/diff/14001/15001#newcode1350
chrome/browser/profile.cc:1350: // GNOME keyring fails for some reason. This
scheme is kind of arbitrary.
On 2010/06/21 21:05:24, Evan Martin wrote:
> If you're going to do something arbitrary, why not the simpler (gnome-keyring
in
> gnome) || (kwallet in kde) one?

The switch statement made this logic fairly natural but since estade wants if
statements anyway, we'll go with the simpler version. My original thinking was
that I personally suspect that a reasonable number of KDE machines also have
GNOME on them, and therefore possibly GNOME Keyring.

http://codereview.chromium.org/2730008/diff/14001/15001#newcode1369
chrome/browser/profile.cc:1369: }
On 2010/06/21 21:14:01, Evan Stade wrote:
> nit: can you add a blank line

Done.

http://codereview.chromium.org/2730008/diff/14001/15001#newcode1370
chrome/browser/profile.cc:1370: PasswordStoreX::NativeBackend* backend;
On 2010/06/21 21:14:01, Evan Stade wrote:
> I think this is a good place to use scoped_ptr

I spend about the same amount of effort managing the scoped_ptr, but OK.

http://codereview.chromium.org/2730008/diff/14001/15001#newcode1381
chrome/browser/profile.cc:1381: // Fall through.
On 2010/06/21 21:14:01, Evan Stade wrote:
> all these fall throughs and conditionalized breaks are kind of unexepected in
a
> switch/case. I think the code would be clearer as a series of ifs.

OK. Since there are only two real cases it's not too bad, though usually we'd
want to use a switch for an enum.

http://codereview.chromium.org/2730008/diff/14001/15001#newcode1395
chrome/browser/profile.cc:1395: }
On 2010/06/21 21:14:01, Evan Stade wrote:
> nit: vertical whitespace here as well

Done.

[Evan Martin, 2010-06-21 21:52:18]

LGTM

[Evan Stade, 2010-06-21 22:39:37]

LGTM, with or without scoped_ptr

http://codereview.chromium.org/2730008/diff/23001/17002
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/2730008/diff/23001/17002#newcode1386
chrome/browser/profile.cc:1386: scoped_ptr<PasswordStoreX::NativeBackend>
backend;
you're right, now that I see it in action, scoped_ptr doesn't seem super
advantageous.

http://codereview.chromium.org/2730008/diff/23001/17002#newcode1402
chrome/browser/profile.cc:1402: backend.reset(NULL);
if you do keep scoped_ptr, the NULL is extraneous