Send out notifications when logins are changed through PasswordStoreMac

Send out notifications when logins are changed through PasswordStoreMac
so that the sync backend can register as an observer.

BUG=35038
TEST=none

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

[stuartmorgan, 2010-04-26 15:31:46]

http://codereview.chromium.org/1752007/diff/5001/6001
File chrome/browser/password_manager/password_store_mac.cc (right):

http://codereview.chromium.org/1752007/diff/5001/6001#newcode787
chrome/browser/password_manager/password_store_mac.cc:787: }
This block might be simpler/clearer as:
  PasswordStoreChangeList changes;
  if (update_count == 0) {
    if (login_metadata_db_->AddLogin(form)) {
       changes.push_back(PasswordStoreChange(PasswordStoreChange::ADD,
                                           form));
    }
  } else {
    changes.push_back(PasswordStoreChange(PasswordStoreChange::UPDATE,
                                        form))
  }
  if (!changes.empty()) {
    NotificationService::current()->Notify(
        NotificationType::LOGINS_CHANGED,
        NotificationService::AllSources(),
        Details<PasswordStoreChangeList>(&changes));
  }

http://codereview.chromium.org/1752007/diff/5001/6001#newcode824
chrome/browser/password_manager/password_store_mac.cc:824: if
(login_metadata_db_->GetLoginsCreatedBetween(delete_begin, delete_end,
I'm slightly concerned about this change. Getting logins will trigger
authorization UI for each returned keychain entry that Chrome isn't already
authorized for (whereas just deleting them won't). In practice this will
probably be okay most of the time, since we are likely authorized for items we
have DB entries for, but there's no guarantee.

Do you actually read the password for a deletion change notification? If not
maybe we could have a version of this call that doesn't read passwords.

http://codereview.chromium.org/1752007/diff/5001/6001#newcode847
chrome/browser/password_manager/password_store_mac.cc:847:
NotificationService::current()->Notify(
This call should be outside the loop.

[albertb, 2010-04-26 21:33:55]

http://codereview.chromium.org/1752007/diff/5001/6001
File chrome/browser/password_manager/password_store_mac.cc (right):

http://codereview.chromium.org/1752007/diff/5001/6001#newcode787
chrome/browser/password_manager/password_store_mac.cc:787: }
On 2010/04/26 15:31:46, stuartmorgan wrote:
> This block might be simpler/clearer as:
>   PasswordStoreChangeList changes;
>   if (update_count == 0) {
>     if (login_metadata_db_->AddLogin(form)) {
>        changes.push_back(PasswordStoreChange(PasswordStoreChange::ADD,
>                                            form));
>     }
>   } else {
>     changes.push_back(PasswordStoreChange(PasswordStoreChange::UPDATE,
>                                         form))
>   }
>   if (!changes.empty()) {
>     NotificationService::current()->Notify(
>         NotificationType::LOGINS_CHANGED,
>         NotificationService::AllSources(),
>         Details<PasswordStoreChangeList>(&changes));
>   }

Good idea. Done.

http://codereview.chromium.org/1752007/diff/5001/6001#newcode824
chrome/browser/password_manager/password_store_mac.cc:824: if
(login_metadata_db_->GetLoginsCreatedBetween(delete_begin, delete_end,
On 2010/04/26 15:31:46, stuartmorgan wrote:
> I'm slightly concerned about this change. Getting logins will trigger
> authorization UI for each returned keychain entry that Chrome isn't already
> authorized for (whereas just deleting them won't). In practice this will
> probably be okay most of the time, since we are likely authorized for items we
> have DB entries for, but there's no guarantee.
> 
> Do you actually read the password for a deletion change notification? If not
> maybe we could have a version of this call that doesn't read passwords.

Maybe I'm misreading the code, but getting the logins from the login db doesn't
involve talking to Keychain.

http://codereview.chromium.org/1752007/diff/5001/6001#newcode847
chrome/browser/password_manager/password_store_mac.cc:847:
NotificationService::current()->Notify(
On 2010/04/26 15:31:46, stuartmorgan wrote:
> This call should be outside the loop.

Done.

[stuartmorgan, 2010-04-26 21:44:29]

LGTM

http://codereview.chromium.org/1752007/diff/5001/6001
File chrome/browser/password_manager/password_store_mac.cc (right):

http://codereview.chromium.org/1752007/diff/5001/6001#newcode824
chrome/browser/password_manager/password_store_mac.cc:824: if
(login_metadata_db_->GetLoginsCreatedBetween(delete_begin, delete_end,
On 2010/04/26 21:33:56, albertb wrote:
> Maybe I'm misreading the code, but getting the logins from the login db
doesn't
> involve talking to Keychain.

Oops, of course not. Sorry, I was thinking of a PasswordStore query for all
logins, not a DB query.