Linux: refactor GNOME Keyring and KWallet integration to allow migration.

Linux: refactor GNOME Keyring and KWallet integration to allow migration from the default store, and add unit tests. Still disabled.
BUG=12351, 25404
TEST=unit tests work

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

[Mike Mammarella, 2010-06-14 08:14:26]

I can also break this into two pieces if you'd like smaller CLs: adding
PasswordStoreXdg and then converting the other two stores into NativeBackends
for it.

At first I tried to make a PasswordStore that could delegate to two other
PasswordStores and migrate between them, but due to the way NotifyConsumer() is
set up that ended up being impossible to do.

I imagine that if the ChromeOS people want to use a different "native" store, it
would be easy to make a NativeBackend and have PasswordStoreXdg migrate to it.

[Evan Martin, 2010-06-14 18:06:45]

http://codereview.chromium.org/2806002/diff/1/3
File chrome/browser/password_manager/password_store_gnome.h (right):

http://codereview.chromium.org/2806002/diff/1/3#newcode9
chrome/browser/password_manager/password_store_gnome.h:9: #include
<gnome-keyring.h>
Is there any way to forward-declare this struct?  Otherwise this header will
leak into anyone who uses this file...

http://codereview.chromium.org/2806002/diff/1/3#newcode17
chrome/browser/password_manager/password_store_gnome.h:17: #include
"webkit/glue/password_form.h"
It looks like you could maybe forward-declare this type.

http://codereview.chromium.org/2806002/diff/1/6
File chrome/browser/password_manager/password_store_xdg.cc (right):

http://codereview.chromium.org/2806002/diff/1/6#newcode248
chrome/browser/password_manager/password_store_xdg.cc:248: // file itself
though...
Why don't we delete the file?

http://codereview.chromium.org/2806002/diff/1/7
File chrome/browser/password_manager/password_store_xdg.h (right):

http://codereview.chromium.org/2806002/diff/1/7#newcode83
chrome/browser/password_manager/password_store_xdg.h:83: void FallBack();
Maybe a longer name for this function would make it more clear what it's doing.

http://codereview.chromium.org/2806002/diff/1/7#newcode93
chrome/browser/password_manager/password_store_xdg.h:93: bool
migration_checked_;
can you add docs on these member variables?  (migration_checked i can probably
guess, but allow_fallback isn't as clear...)

[Mike Mammarella, 2010-06-14 20:58:58]

Thoughts on what to do about the WDS migration (which happened long ago) being
done in the UI thread which trips the DCHECK in the unit test? Theoretically it
could be a problem if you haven't run chrome since before the WDS migration and
then you run a debug build after this stuff gets enabled.

http://codereview.chromium.org/2806002/diff/1/3
File chrome/browser/password_manager/password_store_gnome.h (right):

http://codereview.chromium.org/2806002/diff/1/3#newcode9
chrome/browser/password_manager/password_store_gnome.h:9: #include
<gnome-keyring.h>
On 2010/06/14 18:06:45, Evan Martin wrote:
> Is there any way to forward-declare this struct?  Otherwise this header will
> leak into anyone who uses this file...

We could put kGnomeSchema in an anonymous namespace in the source file instead
of a static member variable. But also this header will only included by two
source files anyway, password_store_gnome.cc and chrome/browser/profile.cc
(though not yet). I can move the schema if you want though.

http://codereview.chromium.org/2806002/diff/1/3#newcode17
chrome/browser/password_manager/password_store_gnome.h:17: #include
"webkit/glue/password_form.h"
On 2010/06/14 18:06:45, Evan Martin wrote:
> It looks like you could maybe forward-declare this type.

Yes, although password_store_xdg.h includes password_store_default.h which
includes password_store.h which includes password_form.h also. I could forward
declare it *there* also, but there are some other files that count on it being
included there that I'd have to fix. Perhaps better left for another CL. Anyway,
forward declared here.

http://codereview.chromium.org/2806002/diff/1/6
File chrome/browser/password_manager/password_store_xdg.cc (right):

http://codereview.chromium.org/2806002/diff/1/6#newcode248
chrome/browser/password_manager/password_store_xdg.cc:248: // file itself
though...
On 2010/06/14 18:06:45, Evan Martin wrote:
> Why don't we delete the file?

I don't know enough about what else might be in that file to be confident in
writing code to do that just yet. I'll come back to this in a later CL: even if
we do delete all the passwords, SQLite probably won't overwrite them so we'll be
leaving them sitting there "deleted" in the login database.

http://codereview.chromium.org/2806002/diff/1/7
File chrome/browser/password_manager/password_store_xdg.h (right):

http://codereview.chromium.org/2806002/diff/1/7#newcode83
chrome/browser/password_manager/password_store_xdg.h:83: void FallBack();
On 2010/06/14 18:06:45, Evan Martin wrote:
> Maybe a longer name for this function would make it more clear what it's
doing.

Done.

http://codereview.chromium.org/2806002/diff/1/7#newcode93
chrome/browser/password_manager/password_store_xdg.h:93: bool
migration_checked_;
On 2010/06/14 18:06:45, Evan Martin wrote:
> can you add docs on these member variables?  (migration_checked i can probably
> guess, but allow_fallback isn't as clear...)

Done.

[Evan Stade, 2010-06-14 21:03:42]

http://codereview.chromium.org/2806002/diff/1/6
File chrome/browser/password_manager/password_store_xdg.cc (right):

http://codereview.chromium.org/2806002/diff/1/6#newcode50
chrome/browser/password_manager/password_store_xdg.cc:50: if (!backend_.get()) {
it seems a somewhat less verbose pattern would be:

if (backend_.get() && backend_->UpdateLogin(form)) {
  ...
} else {
  FallBackIfAllowed();
  if (!backend_.get())
    PasswordStoreDefault::UpdateLoginImpl(form);
}

(kind of nitpicky, but the pattern shows up so many times in this file I think
it's worth streamlining)

also, to ease readability, possibly make a function

bool has_fallen_back() {
  return !backend_.get();
}

http://codereview.chromium.org/2806002/diff/1/7
File chrome/browser/password_manager/password_store_xdg.h (right):

http://codereview.chromium.org/2806002/diff/1/7#newcode25
chrome/browser/password_manager/password_store_xdg.h:25: 
nit: no blank line

http://codereview.chromium.org/2806002/diff/1/7#newcode89
chrome/browser/password_manager/password_store_xdg.h:89: bool
MigrateLogins(size_t* migrated);
why not just return an int, < 0 for fail and 0+ for success?

[Mike Mammarella, 2010-06-14 21:21:45]

http://codereview.chromium.org/2806002/diff/1/6
File chrome/browser/password_manager/password_store_xdg.cc (right):

http://codereview.chromium.org/2806002/diff/1/6#newcode50
chrome/browser/password_manager/password_store_xdg.cc:50: if (!backend_.get()) {
On 2010/06/14 21:03:42, Evan Stade wrote:
> it seems a somewhat less verbose pattern would be:
> 
> if (backend_.get() && backend_->UpdateLogin(form)) {
>   ...
> } else {
>   FallBackIfAllowed();
>   if (!backend_.get())
>     PasswordStoreDefault::UpdateLoginImpl(form);
> }
> 
> (kind of nitpicky, but the pattern shows up so many times in this file I think
> it's worth streamlining)
> 
> also, to ease readability, possibly make a function
> 
> bool has_fallen_back() {
>   return !backend_.get();
> }

Ah, I like that. I thought briefly about how to avoid the superclass calls being
duplicated (goto would have worked), but didn't quickly see an obvious way that
was more elegant.

http://codereview.chromium.org/2806002/diff/1/7
File chrome/browser/password_manager/password_store_xdg.h (right):

http://codereview.chromium.org/2806002/diff/1/7#newcode25
chrome/browser/password_manager/password_store_xdg.h:25: 
On 2010/06/14 21:03:42, Evan Stade wrote:
> nit: no blank line

Done.

http://codereview.chromium.org/2806002/diff/1/7#newcode89
chrome/browser/password_manager/password_store_xdg.h:89: bool
MigrateLogins(size_t* migrated);
On 2010/06/14 21:03:42, Evan Stade wrote:
> why not just return an int, < 0 for fail and 0+ for success?

But what if you have >2.1 billion passwords to migrate? :) Done.

[Mike Mammarella, 2010-06-15 07:45:26]

Unit tests fixed. Also added a TODO for deleting the login database.

[Evan Stade, 2010-06-15 20:03:49]

http://codereview.chromium.org/2806002/diff/23003/34008
File chrome/browser/password_manager/password_store_xdg_unittest.cc (right):

http://codereview.chromium.org/2806002/diff/23003/34008#newcode156
chrome/browser/password_manager/password_store_xdg_unittest.cc:156: for (size_t
i = 0; i < all_forms_.size(); ++i)
nit: this for loop needs curlies

http://codereview.chromium.org/2806002/diff/23003/34009
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/2806002/diff/23003/34009#newcode1810
chrome/chrome_browser.gypi:1810:
'browser/password_manager/password_store_gnome.cc',
I think technically these files should be called password_store_gnome_linux.*,
although the class can still be PasswordStoreGnome (same for
password_store_kwallet)

(I lost a long argument about this yesterday)

http://codereview.chromium.org/2806002/diff/23003/34009#newcode1819
chrome/chrome_browser.gypi:1819:
'browser/password_manager/password_store_xdg.cc',
does _xdg automatically only compile on linux?

[Mike Mammarella, 2010-06-15 20:16:25]

http://codereview.chromium.org/2806002/diff/23003/34008
File chrome/browser/password_manager/password_store_xdg_unittest.cc (right):

http://codereview.chromium.org/2806002/diff/23003/34008#newcode156
chrome/browser/password_manager/password_store_xdg_unittest.cc:156: for (size_t
i = 0; i < all_forms_.size(); ++i)
On 2010/06/15 20:03:49, Evan Stade wrote:
> nit: this for loop needs curlies

Done.

http://codereview.chromium.org/2806002/diff/23003/34009
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/2806002/diff/23003/34009#newcode1810
chrome/chrome_browser.gypi:1810:
'browser/password_manager/password_store_gnome.cc',
On 2010/06/15 20:03:49, Evan Stade wrote:
> I think technically these files should be called password_store_gnome_linux.*,
> although the class can still be PasswordStoreGnome (same for
> password_store_kwallet)
> 
> (I lost a long argument about this yesterday)

See below, we want these also on bsd etc. and _linux will prevent that (I
think).

http://codereview.chromium.org/2806002/diff/23003/34009#newcode1819
chrome/chrome_browser.gypi:1819:
'browser/password_manager/password_store_xdg.cc',
On 2010/06/15 20:03:49, Evan Stade wrote:
> does _xdg automatically only compile on linux?

Yes, that's why I chose this name. It gets compiled on !windows && !osx, i.e.
linux and bsd etc.

[Mike Mammarella, 2010-06-15 21:40:21]

Renamed _xdg to just _x, added _x to gnome/kwallet files and renamed them to
native_backend, and cleaned up gyp files.

[Evan Stade, 2010-06-16 20:29:00]

lgtm