Adding sync support for Passwords

Adding sync support for Passwords

BUG=none
TEST=none

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

[albertb, 2010-05-07 00:13:06]

Wow, there is an absurd amount of boilerplate code to simply sync a new data
type!

http://codereview.chromium.org/1851004/diff/8001/9002
File chrome/browser/password_manager/password_store.h (right):

http://codereview.chromium.org/1851004/diff/8001/9002#newcode20
chrome/browser/password_manager/password_store.h:20: namespace browser_sync {
Can you land your PasswordStore changes in a separate patch?

http://codereview.chromium.org/1851004/diff/8001/9002#newcode136
chrome/browser/password_manager/password_store.h:136: scoped_ptr<base::Thread>
thread_;
I don't really like this. One of the goals of my refactor was to get rid of the
private PasswordStore thread. What do you need this thread for?

http://codereview.chromium.org/1851004/diff/8001/9003
File chrome/browser/password_manager/password_store_default.cc (right):

http://codereview.chromium.org/1851004/diff/8001/9003#newcode35
chrome/browser/password_manager/password_store_default.cc:35:
PasswordStoreChangeList changes;
You should rebase, I checked that code in last week.

http://codereview.chromium.org/1851004/diff/8001/9006
File chrome/browser/sync/engine/syncapi.h (right):

http://codereview.chromium.org/1851004/diff/8001/9006#newcode78
chrome/browser/sync/engine/syncapi.h:78: class PasswordSpecifics;
I sent you a patch for the sync_api changes, I would prefer to submit it before
you land this so we can continue working independently and avoid any crazy
merges.

http://codereview.chromium.org/1851004/diff/8001/9008
File chrome/browser/sync/glue/password_change_processor.cc (right):

http://codereview.chromium.org/1851004/diff/8001/9008#newcode36
chrome/browser/sync/glue/password_change_processor.cc:36:
notification_service_.reset(new NotificationService);
Could you manage the NotificationService creation in the unit test instead and
remove this from the CP code?

http://codereview.chromium.org/1851004/diff/8001/9008#newcode81
chrome/browser/sync/glue/password_change_processor.cc:81:
sync_node.SetTitle(UTF8ToWide(tag));
We can't use this tag as the title since it contains the URL and username in
clear text. It shouldn't be possible to derive any information about the
passwords stored on the sync server without the user pass-phrase.

http://codereview.chromium.org/1851004/diff/8001/9008#newcode137
chrome/browser/sync/glue/password_change_processor.cc:137: StopObserving();
I think you could just set observing_ to false here instead of deregistering
your observer.

http://codereview.chromium.org/1851004/diff/8001/9009
File chrome/browser/sync/glue/password_change_processor.h (right):

http://codereview.chromium.org/1851004/diff/8001/9009#newcode29
chrome/browser/sync/glue/password_change_processor.h:29: // This class is
responsible for taking changes from the history backend and
password backend

http://codereview.chromium.org/1851004/diff/8001/9009#newcode31
chrome/browser/sync/glue/password_change_processor.h:31: // operations and use
of this class are from the UI thread.
This class doesn't operate on the UI thread. It's on the DB thread on Windows
and Linux and on a PasswordStore thread on Mac. Or at least, it should be?

http://codereview.chromium.org/1851004/diff/8001/9009#newcode58
chrome/browser/sync/glue/password_change_processor.h:58: const string16& name,
I don't think we need name, value and timestamps here. Are those leftovers from
the autofill CP?

http://codereview.chromium.org/1851004/diff/8001/9009#newcode79
chrome/browser/sync/glue/password_change_processor.h:79:
scoped_ptr<NotificationService> notification_service_;
Why do you need this?

http://codereview.chromium.org/1851004/diff/8001/9011
File chrome/browser/sync/glue/password_data_type_controller.h (right):

http://codereview.chromium.org/1851004/diff/8001/9011#newcode46
chrome/browser/sync/glue/password_data_type_controller.h:46: return
browser_sync::GROUP_HISTORY;
I don't think you want GROUP_HISTORY here. The DB model worker would work on
Windows and Linux, but not on Mac. So we might need a new model worker to handle
the PasswordStore thread on Mac.

http://codereview.chromium.org/1851004/diff/8001/9012
File chrome/browser/sync/glue/password_model_associator.cc (right):

http://codereview.chromium.org/1851004/diff/8001/9012#newcode57
chrome/browser/sync/glue/password_model_associator.cc:57: if
(!password_store_->FillAutofillableLogins(&passwords) ||
Because of the Keychain on Mac, calling these methods should be a last resort.
Can we store the association on disk like we do for bookmarks?

http://codereview.chromium.org/1851004/diff/8001/9012#newcode102
chrome/browser/sync/glue/password_model_associator.cc:102:
node.SetTitle(UTF8ToWide(tag));
We can't do that. See CP.

http://codereview.chromium.org/1851004/diff/8001/9012#newcode112
chrome/browser/sync/glue/password_model_associator.cc:112: passwords.clear();
STLDeleteElements already clears the container.

http://codereview.chromium.org/1851004/diff/8001/9012#newcode122
chrome/browser/sync/glue/password_model_associator.cc:122:
sync_child_node.GetPasswordSpecifics());
Indent 2 more spaces.

http://codereview.chromium.org/1851004/diff/8001/9012#newcode125
chrome/browser/sync/glue/password_model_associator.cc:125: if
(current_passwords.find(tag) == current_passwords.end()) {
Maybe add a comment here that says that if this condition is true then the
password only exists on the server-side. I was a bit confused trying to figure
that out.

http://codereview.chromium.org/1851004/diff/8001/9012#newcode144
chrome/browser/sync/glue/password_model_associator.cc:144: bool
PasswordModelAssociator::DeleteAllNodes(
Is this called by anyone?

http://codereview.chromium.org/1851004/diff/8001/9012#newcode196
chrome/browser/sync/glue/password_model_associator.cc:196: *has_nodes = true;
nit: We could probably check whether there really are any user created
passwords.

http://codereview.chromium.org/1851004/diff/8001/9012#newcode296
chrome/browser/sync/glue/password_model_associator.cc:296: // static
Insert a empty line here.

http://codereview.chromium.org/1851004/diff/8001/9012#newcode303
chrome/browser/sync/glue/password_model_associator.cc:303: if (0 ==
password.signon_realm().compare(password_form.signon_realm) &&
Can't you compare the fields that don't require UTF16ToUTF8 conversion directly?
With ==?

http://codereview.chromium.org/1851004/diff/8001/9012#newcode322
chrome/browser/sync/glue/password_model_associator.cc:322: if
(base::Time::FromInternalValue(password.date_created()) <=
Why the distinction based on date_created here? Can you add a comment?

http://codereview.chromium.org/1851004/diff/8001/9013
File chrome/browser/sync/glue/password_model_associator.h (right):

http://codereview.chromium.org/1851004/diff/8001/9013#newcode47
chrome/browser/sync/glue/password_model_associator.h:47: // merge and sync.
We can't always merge and sync because of Mac. Any call that looks up logins
might trigger a Keycgain dialog on Mac.

http://codereview.chromium.org/1851004/diff/8001/9013#newcode64
chrome/browser/sync/glue/password_model_associator.h:64: // Delete all typed url
nodes.
s/typed url/password/g

http://codereview.chromium.org/1851004/diff/8001/9013#newcode142
chrome/browser/sync/glue/password_model_associator.h:142: // Abort association
pending flag and lock.  If this is set to true
This seems unique to this MA. Is it really needed?

http://codereview.chromium.org/1851004/diff/8001/9015
File chrome/browser/sync/glue/typed_url_data_type_controller.cc (left):

http://codereview.chromium.org/1851004/diff/8001/9015#oldcode17
chrome/browser/sync/glue/typed_url_data_type_controller.cc:17: #include
"chrome/browser/webdata/web_data_service.h"
This likely belongs in a separate patch.

http://codereview.chromium.org/1851004/diff/8001/9021
File chrome/browser/sync/profile_sync_service_password_unittest.cc (right):

http://codereview.chromium.org/1851004/diff/8001/9021#newcode355
chrome/browser/sync/profile_sync_service_password_unittest.cc:355: PasswordForm*
new_form = new PasswordForm;
I don't think you delete this form. Or does it end up being deleted by the MA?
In any case have you tried running the tests under valgrind?

[Zachary Kuznia, 2010-05-11 18:04:17]

http://codereview.chromium.org/1851004/diff/8001/9002
File chrome/browser/password_manager/password_store.h (right):

http://codereview.chromium.org/1851004/diff/8001/9002#newcode20
chrome/browser/password_manager/password_store.h:20: namespace browser_sync {
On 2010/05/07 00:13:07, albertb wrote:
> Can you land your PasswordStore changes in a separate patch?

I'll split it out.

http://codereview.chromium.org/1851004/diff/8001/9002#newcode136
chrome/browser/password_manager/password_store.h:136: scoped_ptr<base::Thread>
thread_;
On 2010/05/07 00:13:07, albertb wrote:
> I don't really like this. One of the goals of my refactor was to get rid of
the
> private PasswordStore thread. What do you need this thread for?

I don't think it's needed anymore.  Removed.

http://codereview.chromium.org/1851004/diff/8001/9003
File chrome/browser/password_manager/password_store_default.cc (right):

http://codereview.chromium.org/1851004/diff/8001/9003#newcode35
chrome/browser/password_manager/password_store_default.cc:35:
PasswordStoreChangeList changes;
On 2010/05/07 00:13:07, albertb wrote:
> You should rebase, I checked that code in last week.

Done.

http://codereview.chromium.org/1851004/diff/8001/9006
File chrome/browser/sync/engine/syncapi.h (right):

http://codereview.chromium.org/1851004/diff/8001/9006#newcode78
chrome/browser/sync/engine/syncapi.h:78: class PasswordSpecifics;
On 2010/05/07 00:13:07, albertb wrote:
> I sent you a patch for the sync_api changes, I would prefer to submit it
before
> you land this so we can continue working independently and avoid any crazy
> merges.

I'll land after this.

http://codereview.chromium.org/1851004/diff/8001/9008
File chrome/browser/sync/glue/password_change_processor.cc (right):

http://codereview.chromium.org/1851004/diff/8001/9008#newcode36
chrome/browser/sync/glue/password_change_processor.cc:36:
notification_service_.reset(new NotificationService);
On 2010/05/07 00:13:07, albertb wrote:
> Could you manage the NotificationService creation in the unit test instead and
> remove this from the CP code?

It's usually in the unit test that we don't need to manage this.  In this case,
we probably only need it on non-unit test mac versions.

http://codereview.chromium.org/1851004/diff/8001/9008#newcode81
chrome/browser/sync/glue/password_change_processor.cc:81:
sync_node.SetTitle(UTF8ToWide(tag));
On 2010/05/07 00:13:07, albertb wrote:
> We can't use this tag as the title since it contains the URL and username in
> clear text. It shouldn't be possible to derive any information about the
> passwords stored on the sync server without the user pass-phrase.

removed.

http://codereview.chromium.org/1851004/diff/8001/9008#newcode137
chrome/browser/sync/glue/password_change_processor.cc:137: StopObserving();
On 2010/05/07 00:13:07, albertb wrote:
> I think you could just set observing_ to false here instead of deregistering
> your observer.

observing_ is owned by the main thread.  If we change it here, it can end up in
an inconsistent state.

The observer is removed here, because the added calls can be significant during
a large sync, and are guaranteed to be unneeded.

http://codereview.chromium.org/1851004/diff/8001/9009
File chrome/browser/sync/glue/password_change_processor.h (right):

http://codereview.chromium.org/1851004/diff/8001/9009#newcode29
chrome/browser/sync/glue/password_change_processor.h:29: // This class is
responsible for taking changes from the history backend and
On 2010/05/07 00:13:07, albertb wrote:
> password backend

Done.

http://codereview.chromium.org/1851004/diff/8001/9009#newcode31
chrome/browser/sync/glue/password_change_processor.h:31: // operations and use
of this class are from the UI thread.
On 2010/05/07 00:13:07, albertb wrote:
> This class doesn't operate on the UI thread. It's on the DB thread on Windows
> and Linux and on a PasswordStore thread on Mac. Or at least, it should be?

Done.

http://codereview.chromium.org/1851004/diff/8001/9009#newcode58
chrome/browser/sync/glue/password_change_processor.h:58: const string16& name,
On 2010/05/07 00:13:07, albertb wrote:
> I don't think we need name, value and timestamps here. Are those leftovers
from
> the autofill CP?

Yeah, cruft.  Removed.

http://codereview.chromium.org/1851004/diff/8001/9009#newcode79
chrome/browser/sync/glue/password_change_processor.h:79:
scoped_ptr<NotificationService> notification_service_;
On 2010/05/07 00:13:07, albertb wrote:
> Why do you need this?

Often, we're the only listener.  As such, we need to create the notification
service.  This will probably only be create on the Mac version.

http://codereview.chromium.org/1851004/diff/8001/9011
File chrome/browser/sync/glue/password_data_type_controller.h (right):

http://codereview.chromium.org/1851004/diff/8001/9011#newcode46
chrome/browser/sync/glue/password_data_type_controller.h:46: return
browser_sync::GROUP_HISTORY;
On 2010/05/07 00:13:07, albertb wrote:
> I don't think you want GROUP_HISTORY here. The DB model worker would work on
> Windows and Linux, but not on Mac. So we might need a new model worker to
handle
> the PasswordStore thread on Mac.

Done.

http://codereview.chromium.org/1851004/diff/8001/9012
File chrome/browser/sync/glue/password_model_associator.cc (right):

http://codereview.chromium.org/1851004/diff/8001/9012#newcode57
chrome/browser/sync/glue/password_model_associator.cc:57: if
(!password_store_->FillAutofillableLogins(&passwords) ||
On 2010/05/07 00:13:07, albertb wrote:
> Because of the Keychain on Mac, calling these methods should be a last resort.
> Can we store the association on disk like we do for bookmarks?

If we create an additional store for the passwords, we have to worry about the
data getting out of sync.  Also, we then have the problem that we're storing
encrypted data in an unencrypted place.

http://codereview.chromium.org/1851004/diff/8001/9012#newcode102
chrome/browser/sync/glue/password_model_associator.cc:102:
node.SetTitle(UTF8ToWide(tag));
On 2010/05/07 00:13:07, albertb wrote:
> We can't do that. See CP.

Done.

http://codereview.chromium.org/1851004/diff/8001/9012#newcode112
chrome/browser/sync/glue/password_model_associator.cc:112: passwords.clear();
On 2010/05/07 00:13:07, albertb wrote:
> STLDeleteElements already clears the container.

Done.

http://codereview.chromium.org/1851004/diff/8001/9012#newcode122
chrome/browser/sync/glue/password_model_associator.cc:122:
sync_child_node.GetPasswordSpecifics());
On 2010/05/07 00:13:07, albertb wrote:
> Indent 2 more spaces.

Done.

http://codereview.chromium.org/1851004/diff/8001/9012#newcode125
chrome/browser/sync/glue/password_model_associator.cc:125: if
(current_passwords.find(tag) == current_passwords.end()) {
On 2010/05/07 00:13:07, albertb wrote:
> Maybe add a comment here that says that if this condition is true then the
> password only exists on the server-side. I was a bit confused trying to figure
> that out.

Done.

http://codereview.chromium.org/1851004/diff/8001/9012#newcode144
chrome/browser/sync/glue/password_model_associator.cc:144: bool
PasswordModelAssociator::DeleteAllNodes(
On 2010/05/07 00:13:07, albertb wrote:
> Is this called by anyone?

It's used by unit tests, or at least will be.

http://codereview.chromium.org/1851004/diff/8001/9012#newcode196
chrome/browser/sync/glue/password_model_associator.cc:196: *has_nodes = true;
On 2010/05/07 00:13:07, albertb wrote:
> nit: We could probably check whether there really are any user created
> passwords.

Done.

http://codereview.chromium.org/1851004/diff/8001/9012#newcode296
chrome/browser/sync/glue/password_model_associator.cc:296: // static
On 2010/05/07 00:13:07, albertb wrote:
> Insert a empty line here.

Done.

http://codereview.chromium.org/1851004/diff/8001/9012#newcode303
chrome/browser/sync/glue/password_model_associator.cc:303: if (0 ==
password.signon_realm().compare(password_form.signon_realm) &&
On 2010/05/07 00:13:07, albertb wrote:
> Can't you compare the fields that don't require UTF16ToUTF8 conversion
directly?
> With ==?

Done.

http://codereview.chromium.org/1851004/diff/8001/9012#newcode322
chrome/browser/sync/glue/password_model_associator.cc:322: if
(base::Time::FromInternalValue(password.date_created()) <=
On 2010/05/07 00:13:07, albertb wrote:
> Why the distinction based on date_created here? Can you add a comment?

If they're different, we take the newer one.  Comment added.

http://codereview.chromium.org/1851004/diff/8001/9013
File chrome/browser/sync/glue/password_model_associator.h (right):

http://codereview.chromium.org/1851004/diff/8001/9013#newcode47
chrome/browser/sync/glue/password_model_associator.h:47: // merge and sync.
On 2010/05/07 00:13:07, albertb wrote:
> We can't always merge and sync because of Mac. Any call that looks up logins
> might trigger a Keycgain dialog on Mac.

What should we do in that case?  Fail?

http://codereview.chromium.org/1851004/diff/8001/9013#newcode64
chrome/browser/sync/glue/password_model_associator.h:64: // Delete all typed url
nodes.
On 2010/05/07 00:13:07, albertb wrote:
> s/typed url/password/g

Done.

http://codereview.chromium.org/1851004/diff/8001/9013#newcode142
chrome/browser/sync/glue/password_model_associator.h:142: // Abort association
pending flag and lock.  If this is set to true
On 2010/05/07 00:13:07, albertb wrote:
> This seems unique to this MA. Is it really needed?

Autofill does this, too.  We probably should add it to typed URLs as well.

http://codereview.chromium.org/1851004/diff/8001/9015
File chrome/browser/sync/glue/typed_url_data_type_controller.cc (left):

http://codereview.chromium.org/1851004/diff/8001/9015#oldcode17
chrome/browser/sync/glue/typed_url_data_type_controller.cc:17: #include
"chrome/browser/webdata/web_data_service.h"
On 2010/05/07 00:13:07, albertb wrote:
> This likely belongs in a separate patch.

True.  Reverting.

http://codereview.chromium.org/1851004/diff/8001/9021
File chrome/browser/sync/profile_sync_service_password_unittest.cc (right):

http://codereview.chromium.org/1851004/diff/8001/9021#newcode355
chrome/browser/sync/profile_sync_service_password_unittest.cc:355: PasswordForm*
new_form = new PasswordForm;
On 2010/05/07 00:13:07, albertb wrote:
> I don't think you delete this form. Or does it end up being deleted by the MA?
> In any case have you tried running the tests under valgrind?

I haven't run under valgrind yet, I'll send it to the bot.
This password is deleted in the model associator.

[albertb, 2010-05-12 15:26:12]

http://codereview.chromium.org/1851004/diff/8001/9008
File chrome/browser/sync/glue/password_change_processor.cc (right):

http://codereview.chromium.org/1851004/diff/8001/9008#newcode36
chrome/browser/sync/glue/password_change_processor.cc:36:
notification_service_.reset(new NotificationService);
On 2010/05/11 18:04:17, zork wrote:
> On 2010/05/07 00:13:07, albertb wrote:
> > Could you manage the NotificationService creation in the unit test instead
and
> > remove this from the CP code?
> 
> It's usually in the unit test that we don't need to manage this.  In this
case,
> we probably only need it on non-unit test mac versions.

The PasswordStoreMac creates its own NotificatioService:
<https://cs.corp.google.com/p#chrome/trunk/src/chrome/browser/password_manager...>

Are you sure you need to create one here at all?

http://codereview.chromium.org/1851004/diff/8001/9012
File chrome/browser/sync/glue/password_model_associator.cc (right):

http://codereview.chromium.org/1851004/diff/8001/9012#newcode57
chrome/browser/sync/glue/password_model_associator.cc:57: if
(!password_store_->FillAutofillableLogins(&passwords) ||
On 2010/05/11 18:04:17, zork wrote:
> On 2010/05/07 00:13:07, albertb wrote:
> > Because of the Keychain on Mac, calling these methods should be a last
resort.
> > Can we store the association on disk like we do for bookmarks?
> 
> If we create an additional store for the passwords, we have to worry about the
> data getting out of sync.  Also, we then have the problem that we're storing
> encrypted data in an unencrypted place.

The problem is that depending on how it's configured, the Keychain prompts the
user to enter their password every time Chrome looks up a login. So if we lookup
all logins every time the model associator runs then the Keychain will prompt
the user for their password every time they start Chrome.

If we can't store the association on disk and avoid looking up all logins every
time, I think the easiest way around this is to have a method on the
PasswordStoreMac that returns all the logins but doesn't call any Keychain API
(ie. it doesn't fill out the password_value fields). Then, when we do lookup a
password in Keychain (because the user navigates to a password-protected page),
we make sure that the password_value we got from Keychain matches the one in the
sync node and update either as necessary.

http://codereview.chromium.org/1851004/diff/19001/20003
File chrome/browser/password_manager/password_store_default.cc (right):

http://codereview.chromium.org/1851004/diff/19001/20003#newcode5
chrome/browser/password_manager/password_store_default.cc:5: #include
"chrome/browser/password_manager/password_store_change.h"
I probably introduced that change, but can you move password_store_change.h
below password_store_default.h

[Zachary Kuznia, 2010-05-12 21:53:54]

http://codereview.chromium.org/1851004/diff/8001/9008
File chrome/browser/sync/glue/password_change_processor.cc (right):

http://codereview.chromium.org/1851004/diff/8001/9008#newcode36
chrome/browser/sync/glue/password_change_processor.cc:36:
notification_service_.reset(new NotificationService);
If we don't need it on Mac, we don't need it.  Removed.

http://codereview.chromium.org/1851004/diff/8001/9012
File chrome/browser/sync/glue/password_model_associator.cc (right):

http://codereview.chromium.org/1851004/diff/8001/9012#newcode57
chrome/browser/sync/glue/password_model_associator.cc:57: if
(!password_store_->FillAutofillableLogins(&passwords) ||
Leaving this out of this change, for the reasons we discussed.

http://codereview.chromium.org/1851004/diff/19001/20003
File chrome/browser/password_manager/password_store_default.cc (right):

http://codereview.chromium.org/1851004/diff/19001/20003#newcode5
chrome/browser/password_manager/password_store_default.cc:5: #include
"chrome/browser/password_manager/password_store_change.h"
On 2010/05/12 15:26:13, albertb wrote:
> I probably introduced that change, but can you move password_store_change.h
> below password_store_default.h

Done.

[albertb, 2010-05-18 20:51:59]

64 bytes from albertb@chromium.org (74.125.19.147): icmp_seq=1 ttl=57 time=2h20m

LGTM with a few nits and with caveat that Mac will be broken if the Keychain is
setup to prompt the user.

http://codereview.chromium.org/1851004/diff/34001/25005
File chrome/browser/sync/glue/password_change_processor.cc (right):

http://codereview.chromium.org/1851004/diff/34001/25005#newcode32
chrome/browser/sync/glue/password_change_processor.cc:32:
DCHECK(!ChromeThread::CurrentlyOn(ChromeThread::UI));
nit- You could check that you're on the DB thread on Windows and Linux.

http://codereview.chromium.org/1851004/diff/34001/25005#newcode48
chrome/browser/sync/glue/password_change_processor.cc:48: LOG(INFO) << "Observed
password change.";
nit- Remove this.

http://codereview.chromium.org/1851004/diff/34001/25005#newcode68
chrome/browser/sync/glue/password_change_processor.cc:68: {
nit- Shouldn't the { go on the previous line with everything inside indented
back two spaces?

http://codereview.chromium.org/1851004/diff/34001/25007
File chrome/browser/sync/glue/password_data_type_controller.cc (right):

http://codereview.chromium.org/1851004/diff/34001/25007#newcode37
chrome/browser/sync/glue/password_data_type_controller.cc:37: LOG(INFO) <<
"Starting password data controller.";
nit- Remove this.

http://codereview.chromium.org/1851004/diff/34001/25007#newcode56
chrome/browser/sync/glue/password_data_type_controller.cc:56: LOG(INFO) <<
"Stopping password data type controller.";
nit- Remove this.

http://codereview.chromium.org/1851004/diff/34001/25007#newcode72
chrome/browser/sync/glue/password_data_type_controller.cc:72: LOG(INFO) <<
"Password data type controller StartImpl called.";
nit- Remove this.

http://codereview.chromium.org/1851004/diff/34001/25007#newcode105
chrome/browser/sync/glue/password_data_type_controller.cc:105: LOG(INFO) <<
"Password data type controller StartDone called.";
nit- Remove this.

http://codereview.chromium.org/1851004/diff/34001/25007#newcode117
chrome/browser/sync/glue/password_data_type_controller.cc:117: LOG(INFO) <<
"Password data type controller StartDoneImpl called.";
nit- Remove this.

http://codereview.chromium.org/1851004/diff/34001/25007#newcode125
chrome/browser/sync/glue/password_data_type_controller.cc:125: LOG(INFO) <<
"Password data type controller StopImpl called.";
nit- Remove this.

http://codereview.chromium.org/1851004/diff/34001/25009
File chrome/browser/sync/glue/password_model_associator.cc (right):

http://codereview.chromium.org/1851004/diff/34001/25009#newcode36
chrome/browser/sync/glue/password_model_associator.cc:36:
DCHECK(!ChromeThread::CurrentlyOn(ChromeThread::UI));
On Windows and Linux you could check that you're actually on the DB thread.

http://codereview.chromium.org/1851004/diff/34001/25009#newcode40
chrome/browser/sync/glue/password_model_associator.cc:40: LOG(INFO) <<
"Associating Password Models";
nit- Remove this.

http://codereview.chromium.org/1851004/diff/34001/25010
File chrome/browser/sync/glue/password_model_associator.h (right):

http://codereview.chromium.org/1851004/diff/34001/25010#newcode46
chrome/browser/sync/glue/password_model_associator.h:46: // We do not check if
we have local data before this run; we always
"before this runs"

http://codereview.chromium.org/1851004/diff/34001/25012
File chrome/browser/sync/glue/password_model_worker.h (right):

http://codereview.chromium.org/1851004/diff/34001/25012#newcode23
chrome/browser/sync/glue/password_model_worker.h:23: // from the syncapi that
need to be fulfilled on the password thread.
nit- DB thread on Windows and Linux

http://codereview.chromium.org/1851004/diff/34001/25013
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/1851004/diff/34001/25013#newcode91
chrome/browser/sync/glue/sync_backend_host.cc:91: new PasswordModelWorker(
nit- Indent two more

http://codereview.chromium.org/1851004/diff/34001/25020
File chrome/browser/sync/profile_sync_service_password_unittest.cc (right):

http://codereview.chromium.org/1851004/diff/34001/25020#newcode111
chrome/browser/sync/profile_sync_service_password_unittest.cc:111:
notification_service_ = new ThreadNotificationService(&db_thread_);
I don't see any trybot results for your patch. Do those tests pass on Mac?

http://codereview.chromium.org/1851004/diff/34001/25020#newcode194
chrome/browser/sync/profile_sync_service_password_unittest.cc:194:
std::vector<PasswordForm>* entries) {
nit- Fix indentation.