Add password store interface....

Add password store interface.
Add implementations for Windows, gnome & KDE.
BUG=8205

[Nicolas Sylvain, 2009-05-10 15:18:25]

I took a quick look, since I was CCed. It has been a long time since I touched
these files, but:

General comments:
Missing copyright headers
Not enough comments

I'm also not sure how the IE6 versus IE7 implementation is working with this
change.

I haven't looked too much at the gnome implementation. I'll leave that to Evan
and Elliot.

http://codereview.chromium.org/113189/diff/45/52
File chrome/browser/password_manager/password_form_manager.cc (right):

http://codereview.chromium.org/113189/diff/45/52#newcode277
Line 277: OnRequestDone(handle, result);
What about OnIE7RequestDone? You delete this code, but I don't see it moved
anywhere

http://codereview.chromium.org/113189/diff/45/52#newcode324
Line 324: /*WebDataService* web_data_service =
If this is not needed, this code should be removed.

http://codereview.chromium.org/113189/diff/45/52#newcode377
Line 377: return;
If you return here, you should delete the code below.

http://codereview.chromium.org/113189/diff/45/53
File chrome/browser/password_manager/password_store.h (right):

http://codereview.chromium.org/113189/diff/45/53#newcode8
Line 8: #include "base/scoped_ptr.h"
scoped_ptr.h should be before thread.h

http://codereview.chromium.org/113189/diff/45/53#newcode20
Line 20: class PasswordStore : public base::RefCountedThreadSafe<PasswordStore>
{
since this is the base class, would it make sense to add some comments to the
method here? It can be useful for someone who wants to implement it.

http://codereview.chromium.org/113189/diff/45/55
File chrome/browser/password_manager/password_store_default.cc (right):

http://codereview.chromium.org/113189/diff/45/55#newcode1
Line 1: #include "chrome/browser/password_manager/password_store_default.h"
you need to license header.

http://codereview.chromium.org/113189/diff/45/56
File chrome/browser/password_manager/password_store_default.h (right):

http://codereview.chromium.org/113189/diff/45/56#newcode1
Line 1: #ifndef CHROME_BROWSER_PASSWORD_STORE_DEFAULT
you need the license header.

http://codereview.chromium.org/113189/diff/45/56#newcode43
Line 43: #endif
i think the style guide says that you need something like #endif  //
CHROME_BROWSER_PASSWORD_STORE_DEFAULT

[johnmaguire, 2009-05-10 16:12:51]

I haven't quite worked out where the IE stuff is going to go, but it's next on
the list.

http://codereview.chromium.org/113189/diff/45/52
File chrome/browser/password_manager/password_form_manager.cc (right):

http://codereview.chromium.org/113189/diff/45/52#newcode277
Line 277: OnRequestDone(handle, result);
On 2009/05/10 15:18:26, Nicolas Sylvain wrote:
> What about OnIE7RequestDone? You delete this code, but I don't see it moved
> anywhere 

I haven't got there yet ;-)
Hopefully, this can go inside the windows password store implementation.

http://codereview.chromium.org/113189/diff/45/52#newcode324
Line 324: /*WebDataService* web_data_service =
On 2009/05/10 15:18:26, Nicolas Sylvain wrote:
> If this is not needed, this code should be removed.

Done.

http://codereview.chromium.org/113189/diff/45/52#newcode377
Line 377: return;
On 2009/05/10 15:18:26, Nicolas Sylvain wrote:
> If you return here, you should delete the code below.

Done.

http://codereview.chromium.org/113189/diff/45/53
File chrome/browser/password_manager/password_store.h (right):

http://codereview.chromium.org/113189/diff/45/53#newcode8
Line 8: #include "base/scoped_ptr.h"
On 2009/05/10 15:18:26, Nicolas Sylvain wrote:
> scoped_ptr.h should be before thread.h

Done.

http://codereview.chromium.org/113189/diff/45/53#newcode20
Line 20: class PasswordStore : public base::RefCountedThreadSafe<PasswordStore>
{
On 2009/05/10 15:18:26, Nicolas Sylvain wrote:
> since this is the base class, would it make sense to add some comments to the
> method here? It can be useful for someone who wants to implement it.

Done.

http://codereview.chromium.org/113189/diff/45/55
File chrome/browser/password_manager/password_store_default.cc (right):

http://codereview.chromium.org/113189/diff/45/55#newcode1
Line 1: #include "chrome/browser/password_manager/password_store_default.h"
On 2009/05/10 15:18:26, Nicolas Sylvain wrote:
> you need to license header.

Done.

http://codereview.chromium.org/113189/diff/45/56
File chrome/browser/password_manager/password_store_default.h (right):

http://codereview.chromium.org/113189/diff/45/56#newcode1
Line 1: #ifndef CHROME_BROWSER_PASSWORD_STORE_DEFAULT
On 2009/05/10 15:18:26, Nicolas Sylvain wrote:
> you need the license header.

Done.

http://codereview.chromium.org/113189/diff/45/56#newcode43
Line 43: #endif
On 2009/05/10 15:18:26, Nicolas Sylvain wrote:
> i think the style guide says that you need something like #endif  //
> CHROME_BROWSER_PASSWORD_STORE_DEFAULT
> 
> 

Done.

[Evan Martin, 2009-05-11 21:53:40]

(will review rest after you verify you uploaded a new patch)

http://codereview.chromium.org/113189/diff/85/92
File chrome/browser/password_manager/password_form_manager.cc (left):

http://codereview.chromium.org/113189/diff/85/92#oldcode292
Line 292: OnIE7RequestDone(h, result);
As I recall, there's a bit of a subtle state machine here, so please be very
careful that it still works.  timsteele@google is a good reviewer for the
Windows stuff once you've finished refactoring.

http://codereview.chromium.org/113189/diff/85/92
File chrome/browser/password_manager/password_form_manager.cc (right):

http://codereview.chromium.org/113189/diff/85/92#newcode324
Line 324: /*WebDataService* web_data_service =
Did you forget to upload a new patch?  I still see code commented out.

[johnmaguire, 2009-05-14 16:23:30]

New patch with IE7 stuff. Someone *really* needs to test this out on windows.

On 2009/05/11 21:53:40, Evan Martin wrote:
> (will review rest after you verify you uploaded a new patch)
> 
> http://codereview.chromium.org/113189/diff/85/92
> File chrome/browser/password_manager/password_form_manager.cc (left):
> 
> http://codereview.chromium.org/113189/diff/85/92#oldcode292
> Line 292: OnIE7RequestDone(h, result);
> As I recall, there's a bit of a subtle state machine here, so please be very
> careful that it still works.  timsteele@google is a good reviewer for the
> Windows stuff once you've finished refactoring.
> 
> http://codereview.chromium.org/113189/diff/85/92
> File chrome/browser/password_manager/password_form_manager.cc (right):
> 
> http://codereview.chromium.org/113189/diff/85/92#newcode324
> Line 324: /*WebDataService* web_data_service =
> Did you forget to upload a new patch?  I still see code commented out.

[Evan Martin, 2009-05-14 21:27:42]

I'm willing to do (or find someone to do) the legwork to finish off the Windows
bits.  Once everything else looks good, we can figure out how do to do that.

http://codereview.chromium.org/113189/diff/1128/126
File chrome/browser/password_manager/password_form_manager.cc (right):

http://codereview.chromium.org/113189/diff/1128/126#newcode362
Line 362: NOTIMPLEMENTED();
Is this fixable?  Or is your intent to fix it later?  It seems you could at
least pass this on to the new password_store interface, even if it's not
implemented there.

http://codereview.chromium.org/113189/diff/1128/128
File chrome/browser/password_manager/password_form_manager.h (right):

http://codereview.chromium.org/113189/diff/1128/128#newcode10
Line 10: #include "chrome/browser/password_manager/password_manager.h"
can you use a forward declare for this rather than including it?

http://codereview.chromium.org/113189/diff/1128/127
File chrome/browser/password_manager/password_store.h (right):

http://codereview.chromium.org/113189/diff/1128/127#newcode5
Line 5: #ifndef CHROME_BROWSER_PASSWORD_STORE
trailing _ on the define (no idea why)
also it should include the path
CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_STORE

http://codereview.chromium.org/113189/diff/1128/127#newcode35
Line 35: void AddLogin(const PasswordForm& form);
Docs on all of these, please.

http://codereview.chromium.org/113189/diff/1128/127#newcode42
Line 42: void ScheduleTask(Task* task);
docs on all of these as well

http://codereview.chromium.org/113189/diff/1128/127#newcode55
Line 55: int handle_;
docs on all member variables too

http://codereview.chromium.org/113189/diff/1128/127#newcode59
Line 59: DISALLOW_EVIL_CONSTRUCTORS(PasswordStore);
DISALLOW_COPY_AND_ASSIGN()

http://codereview.chromium.org/113189/diff/1128/130
File chrome/browser/password_manager/password_store_default.h (right):

http://codereview.chromium.org/113189/diff/1128/130#newcode6
Line 6: #define CHROME_BROWSER_PASSWORD_STORE_DEFAULT
same comments on this as the other file

http://codereview.chromium.org/113189/diff/1128/124
File chrome/browser/password_manager/password_store_gnome.cc (right):

http://codereview.chromium.org/113189/diff/1128/124#newcode52
Line 52: "Some password from Chrome",
this could probably be improved -- ?

http://codereview.chromium.org/113189/diff/1128/124#newcode80
Line 80: GnomeKeyringResult result = gnome_keyring_delete_password_sync(
I'm nervous about the "_sync" in these calls.  The webdatabase impl is async,
isn't it?

http://codereview.chromium.org/113189/diff/1128/125
File chrome/browser/password_manager/password_store_gnome.h (right):

http://codereview.chromium.org/113189/diff/1128/125#newcode9
Line 9: #include <gnome-keyring.h>
can you forward declare the type here?

http://codereview.chromium.org/113189/diff/1128/133
File chrome/browser/password_manager/password_store_win.cc (right):

http://codereview.chromium.org/113189/diff/1128/133#newcode5
Line 5: #include "chrome/browser/password_manager/password_form_manager.h"
wrong inlcude here i think

http://codereview.chromium.org/113189/diff/1128/135
File chrome/browser/password_manager/password_store_win.h (right):

http://codereview.chromium.org/113189/diff/1128/135#newcode32
Line 32: WebDataService::Handle FetchMatchingIE7LoginFromWebdatabase(
docs on all functions and member variables, please.

http://codereview.chromium.org/113189/diff/1128/135#newcode43
Line 43: DISALLOW_EVIL_CONSTRUCTORS(PasswordStoreDefault);
_COPY_AND_ASSIGN_

Should be PasswordStoreWin not PasswordStoreDefault

http://codereview.chromium.org/113189/diff/1128/131
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/113189/diff/1128/131#newcode735
Line 735: ps = new PasswordStoreDefault(GetPath());
could you put a NOTIMPLEMENTED() above this so the mac folks can find it easily?

http://codereview.chromium.org/113189/diff/1128/132
File chrome/chrome.gyp (left):

http://codereview.chromium.org/113189/diff/1128/132#oldcode957
Line 957: 'browser/password_manager/password_form_manager_win.cc',
I believe gyp style is to list *all* files here, then remove the "bad" ones
below.

http://codereview.chromium.org/113189/diff/1128/132
File chrome/chrome.gyp (right):

http://codereview.chromium.org/113189/diff/1128/132#newcode414
Line 414: '/usr/include/gnome-keyring-1',
This seems wrong.  You should be able to edit src/build/linux/system.gyp and
follow the pattern there to call pkg-config properly.

[tim (not reviewing), 2009-05-14 23:34:19]

http://codereview.chromium.org/113189/diff/1128/127
File chrome/browser/password_manager/password_store.h (right):

http://codereview.chromium.org/113189/diff/1128/127#newcode38
Line 38: int GetLogins(const PasswordForm& form,
where is this implemented?
Apologies if I'm missing something, but it looks like GetLoginsImpl is
synchronous because it calls db_ GetLogins and then notifies the consumer that
the request is done; but  if it's synchronous it can't be called from the main
thread (i.e the thread that's calling GetLogins here)... but if it's calling
into the PasswordManager (the consumer) then it needs to be on the main thread. 
What am I missing?

[johnmaguire, 2009-05-15 14:31:18]

Getting there... ;-)

http://codereview.chromium.org/113189/diff/1128/126
File chrome/browser/password_manager/password_form_manager.cc (right):

http://codereview.chromium.org/113189/diff/1128/126#newcode362
Line 362: NOTIMPLEMENTED();
On 2009/05/14 21:27:42, Evan Martin wrote:
> Is this fixable?  Or is your intent to fix it later?  It seems you could at
> least pass this on to the new password_store interface, even if it's not
> implemented there.

Done.

http://codereview.chromium.org/113189/diff/1128/128
File chrome/browser/password_manager/password_form_manager.h (right):

http://codereview.chromium.org/113189/diff/1128/128#newcode10
Line 10: #include "chrome/browser/password_manager/password_manager.h"
On 2009/05/14 21:27:42, Evan Martin wrote:
> can you use a forward declare for this rather than including it?

Done.

http://codereview.chromium.org/113189/diff/1128/127
File chrome/browser/password_manager/password_store.h (right):

http://codereview.chromium.org/113189/diff/1128/127#newcode5
Line 5: #ifndef CHROME_BROWSER_PASSWORD_STORE
On 2009/05/14 21:27:42, Evan Martin wrote:
> trailing _ on the define (no idea why)
> also it should include the path
> CHROME_BROWSER_PASSWORD_MANAGER_PASSWORD_STORE

Done.

http://codereview.chromium.org/113189/diff/1128/127#newcode35
Line 35: void AddLogin(const PasswordForm& form);
On 2009/05/14 21:27:42, Evan Martin wrote:
> Docs on all of these, please.

Done.

http://codereview.chromium.org/113189/diff/1128/127#newcode38
Line 38: int GetLogins(const PasswordForm& form,
On 2009/05/14 23:34:19, timsteele wrote:
> where is this implemented?
> Apologies if I'm missing something, but it looks like GetLoginsImpl is
> synchronous because it calls db_ GetLogins and then notifies the consumer that
> the request is done; but  if it's synchronous it can't be called from the main
> thread (i.e the thread that's calling GetLogins here)... but if it's calling
> into the PasswordManager (the consumer) then it needs to be on the main
thread. 
> What am I missing?

So the public methods are asynchronous and call *Impl() in a different thread.
I've also changed it so that the PasswordStoreConsumer is notified in the UI
thread. I think that's the right way to do it.

http://codereview.chromium.org/113189/diff/1128/127#newcode42
Line 42: void ScheduleTask(Task* task);
On 2009/05/14 21:27:42, Evan Martin wrote:
> docs on all of these as well

Done.

http://codereview.chromium.org/113189/diff/1128/127#newcode55
Line 55: int handle_;
On 2009/05/14 21:27:42, Evan Martin wrote:
> docs on all member variables too

Done.

http://codereview.chromium.org/113189/diff/1128/127#newcode59
Line 59: DISALLOW_EVIL_CONSTRUCTORS(PasswordStore);
On 2009/05/14 21:27:42, Evan Martin wrote:
> DISALLOW_COPY_AND_ASSIGN()

Done.

http://codereview.chromium.org/113189/diff/1128/130
File chrome/browser/password_manager/password_store_default.h (right):

http://codereview.chromium.org/113189/diff/1128/130#newcode6
Line 6: #define CHROME_BROWSER_PASSWORD_STORE_DEFAULT
On 2009/05/14 21:27:42, Evan Martin wrote:
> same comments on this as the other file

Done.

http://codereview.chromium.org/113189/diff/1128/124
File chrome/browser/password_manager/password_store_gnome.cc (right):

http://codereview.chromium.org/113189/diff/1128/124#newcode52
Line 52: "Some password from Chrome",
On 2009/05/14 21:27:42, Evan Martin wrote:
> this could probably be improved -- ?

Done.

http://codereview.chromium.org/113189/diff/1128/124#newcode80
Line 80: GnomeKeyringResult result = gnome_keyring_delete_password_sync(
On 2009/05/14 21:27:42, Evan Martin wrote:
> I'm nervous about the "_sync" in these calls.  The webdatabase impl is async,
> isn't it?

They don't run in the main thread.

http://codereview.chromium.org/113189/diff/1128/125
File chrome/browser/password_manager/password_store_gnome.h (right):

http://codereview.chromium.org/113189/diff/1128/125#newcode9
Line 9: #include <gnome-keyring.h>
On 2009/05/14 21:27:42, Evan Martin wrote:
> can you forward declare the type here?

Not easily.

http://codereview.chromium.org/113189/diff/1128/133
File chrome/browser/password_manager/password_store_win.cc (right):

http://codereview.chromium.org/113189/diff/1128/133#newcode5
Line 5: #include "chrome/browser/password_manager/password_form_manager.h"
On 2009/05/14 21:27:42, Evan Martin wrote:
> wrong inlcude here i think

Done.

http://codereview.chromium.org/113189/diff/1128/135
File chrome/browser/password_manager/password_store_win.h (right):

http://codereview.chromium.org/113189/diff/1128/135#newcode32
Line 32: WebDataService::Handle FetchMatchingIE7LoginFromWebdatabase(
On 2009/05/14 21:27:42, Evan Martin wrote:
> docs on all functions and member variables, please.

Done.

http://codereview.chromium.org/113189/diff/1128/135#newcode43
Line 43: DISALLOW_EVIL_CONSTRUCTORS(PasswordStoreDefault);
On 2009/05/14 21:27:42, Evan Martin wrote:
> _COPY_AND_ASSIGN_
> 
> Should be PasswordStoreWin not PasswordStoreDefault

Done.

http://codereview.chromium.org/113189/diff/1128/131
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/113189/diff/1128/131#newcode735
Line 735: ps = new PasswordStoreDefault(GetPath());
On 2009/05/14 21:27:42, Evan Martin wrote:
> could you put a NOTIMPLEMENTED() above this so the mac folks can find it
easily?

Done.

http://codereview.chromium.org/113189/diff/1128/132
File chrome/chrome.gyp (left):

http://codereview.chromium.org/113189/diff/1128/132#oldcode957
Line 957: 'browser/password_manager/password_form_manager_win.cc',
On 2009/05/14 21:27:42, Evan Martin wrote:
> I believe gyp style is to list *all* files here, then remove the "bad" ones
> below.

Done.

http://codereview.chromium.org/113189/diff/1128/132
File chrome/chrome.gyp (right):

http://codereview.chromium.org/113189/diff/1128/132#newcode414
Line 414: '/usr/include/gnome-keyring-1',
On 2009/05/14 21:27:42, Evan Martin wrote:
> This seems wrong.  You should be able to edit src/build/linux/system.gyp and
> follow the pattern there to call pkg-config properly.

Done.

[tim (not reviewing), 2009-05-15 16:00:21]

Good morning! looks like you may need to upload the latest patch set?  I'm
seeing your replies but not the changes...

[johnmaguire, 2009-05-17 16:12:32]

On 2009/05/15 16:00:21, timsteele wrote:
> Good morning! looks like you may need to upload the latest patch set?  I'm
> seeing your replies but not the changes...

Updated. Works on windows fine now + KDE implementation added thanks to David
Sansome (me@davidsansome.com).

[tim (not reviewing), 2009-05-18 16:16:03]

http://codereview.chromium.org/113189/diff/255/266
File chrome/browser/password_manager/password_store_default.cc (right):

http://codereview.chromium.org/113189/diff/255/266#newcode33
Line 33: int web_data_handle = web_data_service_->GetLogins(request->form,
this);
I once had a bug with the original impl where the
PasswordManager/PasswordFormManager gets deleted (e.g tab closing), but I wasn't
cancelling pending web_data_service requests, and so some time later a callback
to OnWebDataServiceRequestDone would get fired through the |this| pointer you
pass in here, but that object was deleted.  I don't see a call to
web_data_service->CancelRequest... could you double check this case?

[johnmaguire, 2009-05-18 16:55:30]

http://codereview.chromium.org/113189/diff/255/266
File chrome/browser/password_manager/password_store_default.cc (right):

http://codereview.chromium.org/113189/diff/255/266#newcode33
Line 33: int web_data_handle = web_data_service_->GetLogins(request->form,
this);
On 2009/05/18 16:16:03, timsteele wrote:
> I once had a bug with the original impl where the
> PasswordManager/PasswordFormManager gets deleted (e.g tab closing), but I
wasn't
> cancelling pending web_data_service requests, and so some time later a
callback
> to OnWebDataServiceRequestDone would get fired through the |this| pointer you
> pass in here, but that object was deleted.  I don't see a call to
> web_data_service->CancelRequest... could you double check this case?

Done.

[tim (not reviewing), 2009-05-18 19:51:25]

thanks for checking.  I see PasswordStore is owned by the profile, which is one
way to get around the problem I was talking about. my main question left is
about lifetime of the web_data_service member.

mostly nits otherwise!

http://codereview.chromium.org/113189/diff/255/262
File chrome/browser/password_manager/password_form_manager.cc (right):

http://codereview.chromium.org/113189/diff/255/262#newcode24
Line 24: pending_login_query_(NULL),
this should now be pending_login_query_(0)

http://codereview.chromium.org/113189/diff/255/262#newcode265
Line 265: const std::vector<PasswordForm*>& result) {
nit- I think this can go on same line as above?

http://codereview.chromium.org/113189/diff/255/265
File chrome/browser/password_manager/password_form_manager.h (right):

http://codereview.chromium.org/113189/diff/255/265#newcode63
Line 63: 
nit- add "// PasswordStoreConsumer implementation."

http://codereview.chromium.org/113189/diff/255/264
File chrome/browser/password_manager/password_store.h (right):

http://codereview.chromium.org/113189/diff/255/264#newcode58
Line 58: int h);
please rename and/or comment "int h"

http://codereview.chromium.org/113189/diff/255/266
File chrome/browser/password_manager/password_store_default.cc (right):

http://codereview.chromium.org/113189/diff/255/266#newcode18
Line 18: 
nit- extra newline and this could go in header

http://codereview.chromium.org/113189/diff/255/267
File chrome/browser/password_manager/password_store_default.h (right):

http://codereview.chromium.org/113189/diff/255/267#newcode36
Line 36: WebDataService* web_data_service_;
should this be scoped_refptr?

http://codereview.chromium.org/113189/diff/255/261
File chrome/browser/password_manager/password_store_gnome.h (right):

http://codereview.chromium.org/113189/diff/255/261#newcode34
Line 34: Lock gnome_keyring_lock_;
please comment

http://codereview.chromium.org/113189/diff/255/274
File chrome/browser/password_manager/password_store_kwallet.cc (right):

http://codereview.chromium.org/113189/diff/255/274#newcode29
Line 29: : error_(NULL),
nit- 4 spaces

http://codereview.chromium.org/113189/diff/255/274#newcode133
Line 133: if (wallet_handle == -1)
nit- -1 is used a few times, consider making a constant.

http://codereview.chromium.org/113189/diff/255/274#newcode172
Line 172: for (uint i = 0 ;i < forms.size(); ++i) {
nit- should be "i = 0; i < forms.size()"

http://codereview.chromium.org/113189/diff/255/270
File chrome/browser/password_manager/password_store_win.cc (right):

http://codereview.chromium.org/113189/diff/255/270#newcode21
Line 21: const WDTypedResult *result) {
nit- move to 4 space indent from left

[johnmaguire, 2009-05-19 19:37:06]

http://codereview.chromium.org/113189/diff/255/262
File chrome/browser/password_manager/password_form_manager.cc (right):

http://codereview.chromium.org/113189/diff/255/262#newcode24
Line 24: pending_login_query_(NULL),
On 2009/05/18 19:51:25, timsteele wrote:
> this should now be pending_login_query_(0)

Done.

http://codereview.chromium.org/113189/diff/255/262#newcode265
Line 265: const std::vector<PasswordForm*>& result) {
On 2009/05/18 19:51:25, timsteele wrote:
> nit- I think this can go on same line as above?

Done.

http://codereview.chromium.org/113189/diff/255/265
File chrome/browser/password_manager/password_form_manager.h (right):

http://codereview.chromium.org/113189/diff/255/265#newcode63
Line 63: 
On 2009/05/18 19:51:25, timsteele wrote:
> nit- add "// PasswordStoreConsumer implementation."

Done.

http://codereview.chromium.org/113189/diff/255/264
File chrome/browser/password_manager/password_store.h (right):

http://codereview.chromium.org/113189/diff/255/264#newcode58
Line 58: int h);
On 2009/05/18 19:51:25, timsteele wrote:
> please rename and/or comment "int h"

Done.

http://codereview.chromium.org/113189/diff/255/266
File chrome/browser/password_manager/password_store_default.cc (right):

http://codereview.chromium.org/113189/diff/255/266#newcode18
Line 18: 
On 2009/05/18 19:51:25, timsteele wrote:
> nit- extra newline and this could go in header

Done.

http://codereview.chromium.org/113189/diff/255/267
File chrome/browser/password_manager/password_store_default.h (right):

http://codereview.chromium.org/113189/diff/255/267#newcode36
Line 36: WebDataService* web_data_service_;
On 2009/05/18 19:51:25, timsteele wrote:
> should this be scoped_refptr?

Done.

http://codereview.chromium.org/113189/diff/255/261
File chrome/browser/password_manager/password_store_gnome.h (right):

http://codereview.chromium.org/113189/diff/255/261#newcode34
Line 34: Lock gnome_keyring_lock_;
On 2009/05/18 19:51:25, timsteele wrote:
> please comment

Done.

http://codereview.chromium.org/113189/diff/255/274
File chrome/browser/password_manager/password_store_kwallet.cc (right):

http://codereview.chromium.org/113189/diff/255/274#newcode29
Line 29: : error_(NULL),
On 2009/05/18 19:51:25, timsteele wrote:
> nit- 4 spaces

Done.

http://codereview.chromium.org/113189/diff/255/274#newcode133
Line 133: if (wallet_handle == -1)
On 2009/05/18 19:51:25, timsteele wrote:
> nit- -1 is used a few times, consider making a constant.

Done.

http://codereview.chromium.org/113189/diff/255/274#newcode172
Line 172: for (uint i = 0 ;i < forms.size(); ++i) {
On 2009/05/18 19:51:25, timsteele wrote:
> nit- should be "i = 0; i < forms.size()"

Done.

http://codereview.chromium.org/113189/diff/255/270
File chrome/browser/password_manager/password_store_win.cc (right):

http://codereview.chromium.org/113189/diff/255/270#newcode21
Line 21: const WDTypedResult *result) {
On 2009/05/18 19:51:25, timsteele wrote:
> nit- move to 4 space indent from left

Done.

[tim (not reviewing), 2009-05-19 22:29:19]

LGTM

http://codereview.chromium.org/113189/diff/255/262
File chrome/browser/password_manager/password_form_manager.cc (right):

http://codereview.chromium.org/113189/diff/255/262#newcode185
Line 185: const std::vector<PasswordForm*>& logins_result) {
nit - 4spaces

[johnmaguire, 2009-05-20 09:51:51]

http://codereview.chromium.org/113189/diff/255/262
File chrome/browser/password_manager/password_form_manager.cc (right):

http://codereview.chromium.org/113189/diff/255/262#newcode185
Line 185: const std::vector<PasswordForm*>& logins_result) {
On 2009/05/19 22:29:19, timsteele wrote:
> nit - 4spaces

Done.

[Evan Martin, 2009-05-20 17:19:13]

K, I'm gonna land this then.  Any other reviewers speak now or forever hold your
peace...

[Evan Martin, 2009-05-20 17:19:52]

Hrm, patch failed to apply -- I think you might need to resync and resolve
conflicts.

[stuartmorgan, 2009-05-20 21:50:07]

John, is there a reason that RemoveLoginsCreatedBetween and GetAllLogins aren't
part of the PasswordStore interface?

I'm guessing the reason is just "it isn't part of the password_form_manager use
so it didn't come up yet", which would certainly make sense; I just wanted to
check :)

[johnmaguire, 2009-05-20 21:52:31]

On 2009/05/20 21:50:07, stuartmorgan wrote:
> John, is there a reason that RemoveLoginsCreatedBetween and GetAllLogins
aren't
> part of the PasswordStore interface?
> 
> I'm guessing the reason is just "it isn't part of the password_form_manager
use
> so it didn't come up yet", which would certainly make sense; I just wanted to
> check :)

It isn't part of the password_form_manager use so it didn't come up yet :-)
Anyway, it can follow in another CL. Small CLs will be much quicker to review.

[Evan Martin, 2009-05-20 21:59:15]

After some discussion over here in Linux-land:
 - we're not certain we want to depend on gnome-keyring
 - we've had calling g_thread_init() at all cause breakage elsewhere in GTK in
the base

So we probably need to hammer on this a bit more.

Rather than drag this review out even further, though, and block Mac more, I'm
going to check it in with the Linux bits commented out and revisit that in a
follow-up.

http://code.google.com/p/chromium/issues/detail?id=12351

[Evan Martin, 2009-05-20 21:59:56]

you can watch trybot status at
http://build.chromium.org/buildbot/try-server/waterfall , search for "password"

[Evan Martin, 2009-05-21 00:21:33]

did this work for you on windows?

it gets a bunch of missing symbols, due to deleting passwrod_form_manager_win.cc

http://build.chromium.org/buildbot/try-server/builders/win/builds/3230/steps/...

[Evan Martin, 2009-05-21 00:23:25]

actually, no, it just needs manual tweaking of browser.vcproj :(

[Evan Martin, 2009-05-21 17:39:46]

Here's a patch against this patch that
 - makes it build on Windows
 - disables all the Linux stuff
http://codereview.chromium.org/113710

However, with this patch in place it fails the password-related unit tests.
http://build.chromium.org/buildbot/try-server/builders/win/builds/3309/steps/...
(search for "password" on that page)

It could be because I broke something in my fixup patch, which involved editing
browser.vcproj.

I tried getting a Windows build going yesterday and spent literally three hours
trying different combinations of clobbering, syncing, and rebooting without ever
getting a working Windows build.  I think either johnmaguire or someone else
will have to figure out how to land this, because I've got a lot of other stuff
going on right now.

[Evan Martin, 2009-05-22 01:55:41]

I looked into this more.  I removed a NOTREACHED and added a comment in the
below function:

void PasswordFormManager::CancelLoginsQuery() {
  PasswordStore* password_store =
      profile_->GetPasswordStore(Profile::IMPLICIT_ACCESS);
  if (!password_store) {
    // Can be NULL in unit tests.
    return;
  }
  password_store->CancelLoginsQuery(pending_login_query_);
}


I am going to check it in so I can be done with this.  :)

[Evan Martin, 2009-05-22 02:05:18]

I screwed up the commit message but got the attribution right.

Thanks, John!  Hope you find it useful, Stuart!