Implement Add and Update for PasswordStoreMac....

Implement Add and Update for PasswordStoreMac.
Modify LoginDatabase slightly to give PasswordStoreMac enough information to do the right thing.
Add creator code for keychain items we create, and unit tests to make sure.

BUG=11745
TEST=Visit a site for which you have a password in the Keychain. Type your username, unfocus the field, and then log in with the filled password. Log out, return to the login page, and the username and password should now autofill without user interaction.

[Mark Mentovai, 2009-07-01 23:49:31]

This is substantially LGTM, there are just a couple of small things.

http://codereview.chromium.org/151176/diff/10/20
File chrome/browser/keychain_mock_mac.cc (right):

http://codereview.chromium.org/151176/diff/10/20#newcode139
Line 139: *(static_cast<OSType*>(data)) = value;
This line is incredibly ugly.  Can you make |data| be an OSType* instead of a
void*?

http://codereview.chromium.org/151176/diff/10/20#newcode203
Line 203: void* data = attrList->attr[change_attr].data;
Same

http://codereview.chromium.org/151176/diff/10/20#newcode349
Line 349: void* data =
keychain_attr_list_[*i].attr[creator_attribute_index].data;
And once more.

http://codereview.chromium.org/151176/diff/10/20#newcode350
Line 350: if (*(static_cast<OSType*>(data)) == 0) {
0 is an integer, NULL is a pointer.

http://codereview.chromium.org/151176/diff/10/11
File chrome/browser/password_manager/login_database.h (right):

http://codereview.chromium.org/151176/diff/10/11#newcode34
Line 34: // Updates remembered password form.
Let's streamline this comment now.  "Updates remembered password form.  Returns
true on success and sets items_changed, if non-NULL, to the number of logins
updated."  Or something along those lines.

http://codereview.chromium.org/151176/diff/10/14
File chrome/browser/password_manager/password_store_mac.cc (right):

http://codereview.chromium.org/151176/diff/10/14#newcode21
Line 21: static const OSType kChromeKeychainCreatorCode = 'GCHR';
Worth using a different code for Chromium?

Does this need to be a registered creator code?  Have we registered it?  (If so,
we should set it in the Info.plist too, and stop being '????'.)

http://codereview.chromium.org/151176/diff/10/14#newcode630
Line 630: void MacKeychainPasswordFormAdapter::SetKeychainItemCreatorCode(
Is it worth making this return bool in case anyone cares, for symmetry with
SetKeychainItemPassword?  You'd just need to add |return result == noErr;|.

[stuartmorgan, 2009-07-02 00:56:49]

http://codereview.chromium.org/151176/diff/10/20
File chrome/browser/keychain_mock_mac.cc (right):

http://codereview.chromium.org/151176/diff/10/20#newcode139
Line 139: *(static_cast<OSType*>(data)) = value;
On 2009/07/01 23:49:31, Mark Mentovai wrote:
> This line is incredibly ugly.  Can you make |data| be an OSType* instead of a
> void*?

Done, everywhere this was repeated. While doing that I realized that IndexForTag
wasn't really the right abstraction, and replaced it with a better one that
makes the logic clearer and have less duplication.

http://codereview.chromium.org/151176/diff/10/20#newcode350
Line 350: if (*(static_cast<OSType*>(data)) == 0) {
On 2009/07/01 23:49:31, Mark Mentovai wrote:
> 0 is an integer, NULL is a pointer.

Right, I'm checking the value (which is a lot more obvious with your suggested
change for data's type), and OSType is an integer. The pointer is always
non-NULL, because the mock pre-allocates memory for fixed-size attributes.

http://codereview.chromium.org/151176/diff/10/11
File chrome/browser/password_manager/login_database.h (right):

http://codereview.chromium.org/151176/diff/10/11#newcode34
Line 34: // Updates remembered password form.
On 2009/07/01 23:49:31, Mark Mentovai wrote:
> Let's streamline this comment now.  "Updates remembered password form. 
Returns
> true on success and sets items_changed, if non-NULL, to the number of logins
> updated."  Or something along those lines.

Done.

http://codereview.chromium.org/151176/diff/10/14
File chrome/browser/password_manager/password_store_mac.cc (right):

http://codereview.chromium.org/151176/diff/10/14#newcode21
Line 21: static const OSType kChromeKeychainCreatorCode = 'GCHR';
On 2009/07/01 23:49:31, Mark Mentovai wrote:
> Worth using a different code for Chromium?

I considered that; my feeling is that we don't want to consider them different
for the purposes of password ownership (it will affect how we delete, according
to my current plan). It may be better to use something without the G to make it
more generic though.

> Does this need to be a registered creator code?  Have we registered it?

I forgot these are registered. We probably should, yes, and I'm not aware that
we have. I'll send around an email.

http://codereview.chromium.org/151176/diff/10/14#newcode630
Line 630: void MacKeychainPasswordFormAdapter::SetKeychainItemCreatorCode(
On 2009/07/01 23:49:31, Mark Mentovai wrote:
> Is it worth making this return bool in case anyone cares, for symmetry with
> SetKeychainItemPassword?  You'd just need to add |return result == noErr;|.

I can't imagine anyone ever caring, which is why I didn't, but I guess it can't
hurt; done.

[Mark Mentovai, 2009-07-02 03:03:29]

LGTM

http://codereview.chromium.org/151176/diff/32/42
File chrome/browser/keychain_mock_mac.cc (right):

http://codereview.chromium.org/151176/diff/32/42#newcode87
Line 87: NOTREACHED();
Do we really need the NOTREACHED?  You documented the function as returning NULL
when this happens.

http://codereview.chromium.org/151176/diff/32/42#newcode357
Line 357: if (*data == 0) {
Oh yeah.  Much clearer this way.

http://codereview.chromium.org/151176/diff/32/36
File chrome/browser/password_manager/password_store_mac.cc (right):

http://codereview.chromium.org/151176/diff/32/36#newcode559
Line 559: if (result == errSecDuplicateItem) {
This ought to be |else if| now.

http://codereview.chromium.org/151176/diff/32/36#newcode637
Line 637: return (result == noErr);
Parens aren't necessary here or in the preceding function.  I don't think they
improve clarity, but if you do, you can keep them.