Linux: bring GNOME Keyring and KWallet integration back into a compilable state.

Linux: bring GNOME Keyring and KWallet integration back into a compilable state. Still disabled, however.
BUG=12351, 25404
TEST=both now seem to work correctly when enabled (but this CL does not enable them)


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

[Evan Stade, 2010-06-02 03:51:53]

thanks

sorry for nit-storm

http://codereview.chromium.org/2407001/diff/6001/7002
File chrome/browser/password_manager/password_store_gnome.cc (right):

http://codereview.chromium.org/2407001/diff/6001/7002#newcode1
chrome/browser/password_manager/password_store_gnome.cc:1: // Copyright (c)
2006-2010 The Chromium Authors. All rights reserved.
ditto

http://codereview.chromium.org/2407001/diff/6001/7002#newcode25
chrome/browser/password_manager/password_store_gnome.cc:25: { "origin_url",
GNOME_KEYRING_ATTRIBUTE_TYPE_STRING },
seems odd to me that there's no ID field. But I guess that's the api...

http://codereview.chromium.org/2407001/diff/6001/7002#newcode99
chrome/browser/password_manager/password_store_gnome.cc:99: if
(AddLoginUnlocked(updated, updated.date_created)) {
nit: no curlies for single line ifs

http://codereview.chromium.org/2407001/diff/6001/7002#newcode105
chrome/browser/password_manager/password_store_gnome.cc:105: } else if (result
== GNOME_KEYRING_RESULT_NO_MATCH) {
we don't care about other errors?

http://codereview.chromium.org/2407001/diff/6001/7002#newcode121
chrome/browser/password_manager/password_store_gnome.cc:121: // Search gnome
keyring for all passwords, then delete the ones in the range.
what is gnome? oh, did you mean GNOME?

:)

http://codereview.chromium.org/2407001/diff/6001/7002#newcode243
chrome/browser/password_manager/password_store_gnome.cc:243: bool
PasswordStoreGnome::FillSomeLogins(bool autofillable,
nit: put first parameter on a newline as well

http://codereview.chromium.org/2407001/diff/6001/7003
File chrome/browser/password_manager/password_store_gnome.h (right):

http://codereview.chromium.org/2407001/diff/6001/7003#newcode1
chrome/browser/password_manager/password_store_gnome.h:1: // Copyright (c)
2006-2010 The Chromium Authors. All rights reserved.
nit: just 2010

http://codereview.chromium.org/2407001/diff/6001/7003#newcode36
chrome/browser/password_manager/password_store_gnome.h:36: void
AddLoginImpl(const webkit_glue::PasswordForm& form);
mark these virtual

http://codereview.chromium.org/2407001/diff/6001/7003#newcode70
chrome/browser/password_manager/password_store_gnome.h:70: Lock
gnome_keyring_lock_;
why do you need this? aren't all interactions synchronous and on the same
thread?

http://codereview.chromium.org/2407001/diff/6001/7004
File chrome/browser/password_manager/password_store_kwallet.cc (right):

http://codereview.chromium.org/2407001/diff/6001/7004#newcode1
chrome/browser/password_manager/password_store_kwallet.cc:1: // Copyright (c)
2006-2010 The Chromium Authors. All rights reserved.
ditto

http://codereview.chromium.org/2407001/diff/6001/7004#newcode168
chrome/browser/password_manager/password_store_kwallet.cc:168:
FreeLoginsInListExcept(forms, &form);
why don't you just dupe |form|, so you can use STLDeleteElements instead of
creating FreeLoginsInListExcept()

this also avoids a const cast.

http://codereview.chromium.org/2407001/diff/6001/7004#newcode337
chrome/browser/password_manager/password_store_kwallet.cc:337: if (ret)
I think this is frowned upon. Only bools or pointers should be checked without
equality operator.

seems like you could share the error checking code between the forms.empty() and
!forms.empty() cases.

http://codereview.chromium.org/2407001/diff/6001/7005
File chrome/browser/password_manager/password_store_kwallet.h (right):

http://codereview.chromium.org/2407001/diff/6001/7005#newcode1
chrome/browser/password_manager/password_store_kwallet.h:1: // Copyright (c)
2006-2010 The Chromium Authors. All rights reserved.
ditto

http://codereview.chromium.org/2407001/diff/6001/7005#newcode18
chrome/browser/password_manager/password_store_kwallet.h:18: #include
"chrome/browser/webdata/web_data_service.h"
alphabets

[Mike Mammarella, 2010-06-03 20:27:07]

http://codereview.chromium.org/2407001/diff/6001/7002
File chrome/browser/password_manager/password_store_gnome.cc (right):

http://codereview.chromium.org/2407001/diff/6001/7002#newcode1
chrome/browser/password_manager/password_store_gnome.cc:1: // Copyright (c)
2006-2010 The Chromium Authors. All rights reserved.
On 2010/06/02 03:51:53, Evan Stade wrote:
> ditto

Done.

http://codereview.chromium.org/2407001/diff/6001/7002#newcode25
chrome/browser/password_manager/password_store_gnome.cc:25: { "origin_url",
GNOME_KEYRING_ATTRIBUTE_TYPE_STRING },
On 2010/06/02 03:51:53, Evan Stade wrote:
> seems odd to me that there's no ID field. But I guess that's the api...

Yeah, the ID field is specified as the key when we insert and remove, but it's
not here in the list of data fields.

http://codereview.chromium.org/2407001/diff/6001/7002#newcode99
chrome/browser/password_manager/password_store_gnome.cc:99: if
(AddLoginUnlocked(updated, updated.date_created)) {
On 2010/06/02 03:51:53, Evan Stade wrote:
> nit: no curlies for single line ifs

My personal preference as well! Not every reviewer agrees. :) Done.

http://codereview.chromium.org/2407001/diff/6001/7002#newcode105
chrome/browser/password_manager/password_store_gnome.cc:105: } else if (result
== GNOME_KEYRING_RESULT_NO_MATCH) {
On 2010/06/02 03:51:53, Evan Stade wrote:
> we don't care about other errors?

Hmm. Indeed, this is weird. Not sure how it got that way. Fixed.

http://codereview.chromium.org/2407001/diff/6001/7002#newcode121
chrome/browser/password_manager/password_store_gnome.cc:121: // Search gnome
keyring for all passwords, then delete the ones in the range.
On 2010/06/02 03:51:53, Evan Stade wrote:
> what is gnome? oh, did you mean GNOME?
> 
> :)

Done.

http://codereview.chromium.org/2407001/diff/6001/7002#newcode243
chrome/browser/password_manager/password_store_gnome.cc:243: bool
PasswordStoreGnome::FillSomeLogins(bool autofillable,
On 2010/06/02 03:51:53, Evan Stade wrote:
> nit: put first parameter on a newline as well

Done.

http://codereview.chromium.org/2407001/diff/6001/7003
File chrome/browser/password_manager/password_store_gnome.h (right):

http://codereview.chromium.org/2407001/diff/6001/7003#newcode1
chrome/browser/password_manager/password_store_gnome.h:1: // Copyright (c)
2006-2010 The Chromium Authors. All rights reserved.
On 2010/06/02 03:51:53, Evan Stade wrote:
> nit: just 2010

Done.

http://codereview.chromium.org/2407001/diff/6001/7003#newcode36
chrome/browser/password_manager/password_store_gnome.h:36: void
AddLoginImpl(const webkit_glue::PasswordForm& form);
On 2010/06/02 03:51:53, Evan Stade wrote:
> mark these virtual

Done.

http://codereview.chromium.org/2407001/diff/6001/7003#newcode70
chrome/browser/password_manager/password_store_gnome.h:70: Lock
gnome_keyring_lock_;
On 2010/06/02 03:51:53, Evan Stade wrote:
> why do you need this? aren't all interactions synchronous and on the same
> thread?

I don't know why it is necessary, but the original code had it and I don't know
enough about GNOME keyring to be totally convinced it's not necessary. But it
does seem like the calls are all synchronous and on the same thread...

http://codereview.chromium.org/2407001/diff/6001/7004
File chrome/browser/password_manager/password_store_kwallet.cc (right):

http://codereview.chromium.org/2407001/diff/6001/7004#newcode1
chrome/browser/password_manager/password_store_kwallet.cc:1: // Copyright (c)
2006-2010 The Chromium Authors. All rights reserved.
On 2010/06/02 03:51:53, Evan Stade wrote:
> ditto

Done.

http://codereview.chromium.org/2407001/diff/6001/7004#newcode168
chrome/browser/password_manager/password_store_kwallet.cc:168:
FreeLoginsInListExcept(forms, &form);
On 2010/06/02 03:51:53, Evan Stade wrote:
> why don't you just dupe |form|, so you can use STLDeleteElements instead of
> creating FreeLoginsInListExcept()
> 
> this also avoids a const cast.

I could do that, but it seems a little bit contrived to allocate a new object
just to avoid three lines of code. If you think it's really bad to have the
const_cast I'll change it though.

http://codereview.chromium.org/2407001/diff/6001/7004#newcode337
chrome/browser/password_manager/password_store_kwallet.cc:337: if (ret)
On 2010/06/02 03:51:53, Evan Stade wrote:
> I think this is frowned upon. Only bools or pointers should be checked without
> equality operator.
> 
> seems like you could share the error checking code between the forms.empty()
and
> !forms.empty() cases.

This code was just moved here from line 191 or so, so I didn't notice this.
Fixed. I don't share the code because the error message is a little different.

http://codereview.chromium.org/2407001/diff/6001/7005
File chrome/browser/password_manager/password_store_kwallet.h (right):

http://codereview.chromium.org/2407001/diff/6001/7005#newcode1
chrome/browser/password_manager/password_store_kwallet.h:1: // Copyright (c)
2006-2010 The Chromium Authors. All rights reserved.
On 2010/06/02 03:51:53, Evan Stade wrote:
> ditto

Done.

http://codereview.chromium.org/2407001/diff/6001/7005#newcode18
chrome/browser/password_manager/password_store_kwallet.h:18: #include
"chrome/browser/webdata/web_data_service.h"
On 2010/06/02 03:51:53, Evan Stade wrote:
> alphabets

Done.

[Evan Stade, 2010-06-03 20:39:41]

http://codereview.chromium.org/2407001/diff/6001/7002
File chrome/browser/password_manager/password_store_gnome.cc (right):

http://codereview.chromium.org/2407001/diff/6001/7002#newcode25
chrome/browser/password_manager/password_store_gnome.cc:25: { "origin_url",
GNOME_KEYRING_ATTRIBUTE_TYPE_STRING },
On 2010/06/03 20:27:07, Mike Mammarella wrote:
> On 2010/06/02 03:51:53, Evan Stade wrote:
> > seems odd to me that there's no ID field. But I guess that's the api...
> 
> Yeah, the ID field is specified as the key when we insert and remove, but it's
> not here in the list of data fields.

wait, I don't see it. Which parameter in gnome_keyring_delete_password_sync() is
the ID?

http://codereview.chromium.org/2407001/diff/6001/7003
File chrome/browser/password_manager/password_store_gnome.h (right):

http://codereview.chromium.org/2407001/diff/6001/7003#newcode70
chrome/browser/password_manager/password_store_gnome.h:70: Lock
gnome_keyring_lock_;
On 2010/06/03 20:27:07, Mike Mammarella wrote:
> On 2010/06/02 03:51:53, Evan Stade wrote:
> > why do you need this? aren't all interactions synchronous and on the same
> > thread?
> 
> I don't know why it is necessary, but the original code had it and I don't
know
> enough about GNOME keyring to be totally convinced it's not necessary. But it
> does seem like the calls are all synchronous and on the same thread...

ping the original author, or the original reviewer? If neither of these people
are responsive, you can add a TODO/file a bug to investigate.

http://codereview.chromium.org/2407001/diff/6001/7004
File chrome/browser/password_manager/password_store_kwallet.cc (right):

http://codereview.chromium.org/2407001/diff/6001/7004#newcode168
chrome/browser/password_manager/password_store_kwallet.cc:168:
FreeLoginsInListExcept(forms, &form);
On 2010/06/03 20:27:07, Mike Mammarella wrote:
> On 2010/06/02 03:51:53, Evan Stade wrote:
> > why don't you just dupe |form|, so you can use STLDeleteElements instead of
> > creating FreeLoginsInListExcept()
> > 
> > this also avoids a const cast.
> 
> I could do that, but it seems a little bit contrived to allocate a new object
> just to avoid three lines of code. If you think it's really bad to have the
> const_cast I'll change it though.

the performance difference is negligible; the code complexity difference is not

and yes, const_cast is Bad.

http://codereview.chromium.org/2407001/diff/23001/24002
File chrome/browser/password_manager/password_store_gnome.cc (right):

http://codereview.chromium.org/2407001/diff/23001/24002#newcode129
chrome/browser/password_manager/password_store_gnome.cc:129:
vector<PasswordForm*> forms;
nit: declare this as late as possible

http://codereview.chromium.org/2407001/diff/23001/24002#newcode307
chrome/browser/password_manager/password_store_gnome.cc:307: DCHECK(date_ok &&
date_created != 0);
nit: separate DCHECKS

[Mike Mammarella, 2010-06-03 22:05:29]

http://codereview.chromium.org/2407001/diff/6001/7002
File chrome/browser/password_manager/password_store_gnome.cc (right):

http://codereview.chromium.org/2407001/diff/6001/7002#newcode25
chrome/browser/password_manager/password_store_gnome.cc:25: { "origin_url",
GNOME_KEYRING_ATTRIBUTE_TYPE_STRING },
On 2010/06/03 20:39:42, Evan Stade wrote:
> On 2010/06/03 20:27:07, Mike Mammarella wrote:
> > On 2010/06/02 03:51:53, Evan Stade wrote:
> > > seems odd to me that there's no ID field. But I guess that's the api...
> > 
> > Yeah, the ID field is specified as the key when we insert and remove, but
it's
> > not here in the list of data fields.
> 
> wait, I don't see it. Which parameter in gnome_keyring_delete_password_sync()
is
> the ID?

Ah, actually, I was thinking of the kwallet version. Yes, this doesn't have a
key at all - you can search by any of the fields, and we have all the relevant
fields from the PasswordForm here.

http://codereview.chromium.org/2407001/diff/6001/7003
File chrome/browser/password_manager/password_store_gnome.h (right):

http://codereview.chromium.org/2407001/diff/6001/7003#newcode70
chrome/browser/password_manager/password_store_gnome.h:70: Lock
gnome_keyring_lock_;
On 2010/06/03 20:39:42, Evan Stade wrote:
> On 2010/06/03 20:27:07, Mike Mammarella wrote:
> > On 2010/06/02 03:51:53, Evan Stade wrote:
> > > why do you need this? aren't all interactions synchronous and on the same
> > > thread?
> > 
> > I don't know why it is necessary, but the original code had it and I don't
> know
> > enough about GNOME keyring to be totally convinced it's not necessary. But
it
> > does seem like the calls are all synchronous and on the same thread...
> 
> ping the original author, or the original reviewer? If neither of these people
> are responsive, you can add a TODO/file a bug to investigate.

I'll look into that.

http://codereview.chromium.org/2407001/diff/6001/7004
File chrome/browser/password_manager/password_store_kwallet.cc (right):

http://codereview.chromium.org/2407001/diff/6001/7004#newcode168
chrome/browser/password_manager/password_store_kwallet.cc:168:
FreeLoginsInListExcept(forms, &form);
On 2010/06/03 20:39:42, Evan Stade wrote:
> On 2010/06/03 20:27:07, Mike Mammarella wrote:
> > On 2010/06/02 03:51:53, Evan Stade wrote:
> > > why don't you just dupe |form|, so you can use STLDeleteElements instead
of
> > > creating FreeLoginsInListExcept()
> > > 
> > > this also avoids a const cast.
> > 
> > I could do that, but it seems a little bit contrived to allocate a new
object
> > just to avoid three lines of code. If you think it's really bad to have the
> > const_cast I'll change it though.
> 
> the performance difference is negligible; the code complexity difference is
not
> 
> and yes, const_cast is Bad.

Done.

http://codereview.chromium.org/2407001/diff/23001/24002
File chrome/browser/password_manager/password_store_gnome.cc (right):

http://codereview.chromium.org/2407001/diff/23001/24002#newcode129
chrome/browser/password_manager/password_store_gnome.cc:129:
vector<PasswordForm*> forms;
On 2010/06/03 20:39:42, Evan Stade wrote:
> nit: declare this as late as possible

Done.

http://codereview.chromium.org/2407001/diff/23001/24002#newcode307
chrome/browser/password_manager/password_store_gnome.cc:307: DCHECK(date_ok &&
date_created != 0);
On 2010/06/03 20:39:42, Evan Stade wrote:
> nit: separate DCHECKS

Done.

[Evan Stade, 2010-06-03 22:15:36]

LGTM pending some action on the lock thing

-- Evan Stade



On Thu, Jun 3, 2010 at 3:05 PM,  <mdm@chromium.org> wrote:
>
> http://codereview.chromium.org/2407001/diff/6001/7002
> File chrome/browser/password_manager/password_store_gnome.cc (right):
>
> http://codereview.chromium.org/2407001/diff/6001/7002#newcode25
> chrome/browser/password_manager/password_store_gnome.cc:25: {
> "origin_url", GNOME_KEYRING_ATTRIBUTE_TYPE_STRING },
> On 2010/06/03 20:39:42, Evan Stade wrote:
>>
>> On 2010/06/03 20:27:07, Mike Mammarella wrote:
>> > On 2010/06/02 03:51:53, Evan Stade wrote:
>> > > seems odd to me that there's no ID field. But I guess that's the
>
> api...
>>
>> >
>> > Yeah, the ID field is specified as the key when we insert and
>
> remove, but it's
>>
>> > not here in the list of data fields.
>
>> wait, I don't see it. Which parameter in
>
> gnome_keyring_delete_password_sync() is
>>
>> the ID?
>
> Ah, actually, I was thinking of the kwallet version. Yes, this doesn't
> have a key at all - you can search by any of the fields, and we have all
> the relevant fields from the PasswordForm here.
>
> http://codereview.chromium.org/2407001/diff/6001/7003
> File chrome/browser/password_manager/password_store_gnome.h (right):
>
> http://codereview.chromium.org/2407001/diff/6001/7003#newcode70
> chrome/browser/password_manager/password_store_gnome.h:70: Lock
> gnome_keyring_lock_;
> On 2010/06/03 20:39:42, Evan Stade wrote:
>>
>> On 2010/06/03 20:27:07, Mike Mammarella wrote:
>> > On 2010/06/02 03:51:53, Evan Stade wrote:
>> > > why do you need this? aren't all interactions synchronous and on
>
> the same
>>
>> > > thread?
>> >
>> > I don't know why it is necessary, but the original code had it and I
>
> don't
>>
>> know
>> > enough about GNOME keyring to be totally convinced it's not
>
> necessary. But it
>>
>> > does seem like the calls are all synchronous and on the same
>
> thread...
>
>> ping the original author, or the original reviewer? If neither of
>
> these people
>>
>> are responsive, you can add a TODO/file a bug to investigate.
>
> I'll look into that.
>
> http://codereview.chromium.org/2407001/diff/6001/7004
> File chrome/browser/password_manager/password_store_kwallet.cc (right):
>
> http://codereview.chromium.org/2407001/diff/6001/7004#newcode168
> chrome/browser/password_manager/password_store_kwallet.cc:168:
> FreeLoginsInListExcept(forms, &form);
> On 2010/06/03 20:39:42, Evan Stade wrote:
>>
>> On 2010/06/03 20:27:07, Mike Mammarella wrote:
>> > On 2010/06/02 03:51:53, Evan Stade wrote:
>> > > why don't you just dupe |form|, so you can use STLDeleteElements
>
> instead of
>>
>> > > creating FreeLoginsInListExcept()
>> > >
>> > > this also avoids a const cast.
>> >
>> > I could do that, but it seems a little bit contrived to allocate a
>
> new object
>>
>> > just to avoid three lines of code. If you think it's really bad to
>
> have the
>>
>> > const_cast I'll change it though.
>
>> the performance difference is negligible; the code complexity
>
> difference is not
>
>> and yes, const_cast is Bad.
>
> Done.
>
> http://codereview.chromium.org/2407001/diff/23001/24002
> File chrome/browser/password_manager/password_store_gnome.cc (right):
>
> http://codereview.chromium.org/2407001/diff/23001/24002#newcode129
> chrome/browser/password_manager/password_store_gnome.cc:129:
> vector<PasswordForm*> forms;
> On 2010/06/03 20:39:42, Evan Stade wrote:
>>
>> nit: declare this as late as possible
>
> Done.
>
> http://codereview.chromium.org/2407001/diff/23001/24002#newcode307
> chrome/browser/password_manager/password_store_gnome.cc:307:
> DCHECK(date_ok && date_created != 0);
> On 2010/06/03 20:39:42, Evan Stade wrote:
>>
>> nit: separate DCHECKS
>
> Done.
>
> http://codereview.chromium.org/2407001/show
>