Wire up a LoginDatabase for PasswordStoreMac

Wire up a LoginDatabase for PasswordStoreMac, and start using it for lookup.
(Although it will always be empty at this point, since storage isn't implemented yet.)

BUG=11745
TEST=none (no user-visible effect yet)

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

[tim (not reviewing), 2009-06-27 19:51:18]

http://codereview.chromium.org/147044/diff/1/4
File chrome/browser/password_manager/password_store_mac.cc (right):

http://codereview.chromium.org/147044/diff/1/4#newcode577
Line 577: if
(internal_keychain_helpers::FillPasswordFormFromKeychainItem(keychain,
don't need the namespace prefix

http://codereview.chromium.org/147044/diff/1/4#newcode590
Line 590: internal_keychain_helpers::FindMatchingKeychainItems(
don't need the namespace prefix

http://codereview.chromium.org/147044/diff/1/4#newcode602
Line 602: }  // internal_keychain_helpers
could you please make this // namespace internal_keychain_helpers

http://codereview.chromium.org/147044/diff/1/4#newcode613
Line 613: PasswordStoreMac::~PasswordStoreMac() {}
this is fine, but did you mean to move it from the .h?

http://codereview.chromium.org/147044/diff/1/4#newcode643
Line 643: i != keychain_forms.end(); ++i) {
you can use STLDeleteElements(&keychain_forms);  from stl_util-inl.h

http://codereview.chromium.org/147044/diff/1/4#newcode649
Line 649: delete *i;
i'd prefer adding a line database_forms.erase(i++) and nix the ++i in the for
loop, just so the vector is empty in case someone tries to add code after this
loop later on.

[stuartmorgan, 2009-06-30 18:10:49]

http://codereview.chromium.org/147044/diff/1/4
File chrome/browser/password_manager/password_store_mac.cc (right):

http://codereview.chromium.org/147044/diff/1/4#newcode577
Line 577: if
(internal_keychain_helpers::FillPasswordFormFromKeychainItem(keychain,
On 2009/06/27 19:51:18, timsteele wrote:
> don't need the namespace prefix

Done, and fixed a bunch of other instances throughout the namespace.

http://codereview.chromium.org/147044/diff/1/4#newcode602
Line 602: }  // internal_keychain_helpers
On 2009/06/27 19:51:18, timsteele wrote:
> could you please make this // namespace internal_keychain_helpers

Done.

http://codereview.chromium.org/147044/diff/1/4#newcode613
Line 613: PasswordStoreMac::~PasswordStoreMac() {}
On 2009/06/27 19:51:18, timsteele wrote:
> this is fine, but did you mean to move it from the .h?

Yes, I ran into compile issues from having the destructor trying to operate on a
scoped_ptr of an incomplete type (the forward-declared LoginDatabaseMac). I'm
not sure why MacKeychain didn't have the same problem; probably an accident of
include ordering in the files that use it.

http://codereview.chromium.org/147044/diff/1/4#newcode643
Line 643: i != keychain_forms.end(); ++i) {
On 2009/06/27 19:51:18, timsteele wrote:
> you can use STLDeleteElements(&keychain_forms);  from stl_util-inl.h

Done.

http://codereview.chromium.org/147044/diff/1/4#newcode649
Line 649: delete *i;
On 2009/06/27 19:51:18, timsteele wrote:
> i'd prefer adding a line database_forms.erase(i++) and nix the ++i in the for
> loop, just so the vector is empty in case someone tries to add code after this
> loop later on.

I split out the database removal and the deletion so it could use
STLDeleteElements as well; the double-iteration will have negligible cost, so I
opted for improved readability.

[tim (not reviewing), 2009-06-30 22:47:20]

LGTM