Rest of the autofill work.

Rest of the autofill work.
Includes 
1. change processor.
2. Migrating code.
3. new datatype registration/enabling/disabling from the UI. (It is keyed off of autofill datatype).

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

[lipalani, 2010-11-17 06:45:22]

This is a superset of the first patch. 
If you only need to see the work that happened SINCE the first patch let me know
and I can send that diff.

[tim (not reviewing), 2010-11-17 08:33:22]

hm, wait... I started reviewing the issue you sent out at ~ 530.  Should I
abandon that?  In general if the changes need to be part of the first patch
you should re-upload to that patch with a comment explaining.  If you're
wanting to send out a review for new components that build upon stuff that
is still out for review (and you're using git), you should create a
branch-of-your-branch and upload a new issue from there...

On Tue, Nov 16, 2010 at 10:45 PM, <lipalani@google.com> wrote:

> Reviewers: timsteele,
>
> Message:
> This is a superset of the first patch.
> If you only need to see the work that happened SINCE the first patch let me
> know
> and I can send that diff.
>
> Description:
> Rest of the autofill work.
> Includes
> 1. change processor.
> 2. Migrating code.
> 3. new datatype registration/enabling/disabling from the UI. (It is keyed
> off of
> autofill datatype).
>
> Please review this at http://codereview.chromium.org/5159001/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  A chrome/browser/sync/engine/read_node_mock.h
>  M chrome/browser/sync/engine/syncapi.h
>  M chrome/browser/sync/engine/syncapi.cc
>  A chrome/browser/sync/engine/syncapi_mock.h
>  M chrome/browser/sync/glue/autofill_change_processor.h
>  M chrome/browser/sync/glue/autofill_change_processor.cc
>  A chrome/browser/sync/glue/autofill_change_processor2.h
>  A chrome/browser/sync/glue/autofill_change_processor2.cc
>  M chrome/browser/sync/glue/autofill_data_type_controller.h
>  M chrome/browser/sync/glue/autofill_data_type_controller.cc
>  M chrome/browser/sync/glue/autofill_model_associator.h
>  M chrome/browser/sync/glue/autofill_model_associator.cc
>  A chrome/browser/sync/glue/autofill_model_associator2.h
>  A chrome/browser/sync/glue/autofill_model_associator2.cc
>  M chrome/browser/sync/glue/autofill_model_associator_unittest.cc
>  A chrome/browser/sync/glue/autofill_profile_change_processor.h
>  A chrome/browser/sync/glue/autofill_profile_change_processor.cc
>  A chrome/browser/sync/glue/autofill_profile_data_type_controller.h
>  A chrome/browser/sync/glue/autofill_profile_data_type_controller.cc
>  A chrome/browser/sync/glue/autofill_profile_model_associator.h
>  A chrome/browser/sync/glue/autofill_profile_model_associator.cc
>  A chrome/browser/sync/glue/autofill_profile_model_associator_unittest.cc
>  A chrome/browser/sync/glue/do_optimistic_refresh_task.h
>  A chrome/browser/sync/glue/do_optimistic_refresh_task.cc
>  M chrome/browser/sync/profile_sync_factory.h
>  M chrome/browser/sync/profile_sync_factory_impl.h
>  M chrome/browser/sync/profile_sync_factory_impl.cc
>  M chrome/browser/sync/profile_sync_service.cc
>  M chrome/browser/sync/profile_sync_service_autofill_unittest.cc
>  M chrome/browser/sync/protocol/autofill_specifics.proto
>  M chrome/browser/sync/syncable/model_type.h
>  M chrome/browser/sync/syncable/model_type.cc
>  M chrome/browser/sync/syncable/syncable.h
>  M chrome/browser/sync/syncable/syncable.cc
>  A chrome/browser/sync/syncable/syncable_mock.h
>  M chrome/chrome_browser.gypi
>  M chrome/chrome_tests.gypi
>  M chrome/common/chrome_switches.h
>  M chrome/common/chrome_switches.cc
>  M chrome/common/pref_names.h
>  M chrome/common/pref_names.cc
>
>
>

[lipalani, 2010-12-08 21:37:46]

The comments from the previous CR that we did before couple of weeks ago.

New autofill migration code. and implementing the comments(for the most part).

I will send a seperate email on the comments not implemented and why.(should be
only couple of them and they are minor).

http://codereview.chromium.org/5159001/diff/4001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/5159001/diff/4001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.cc:2193: &model_types)) {
this needs to go back.

http://codereview.chromium.org/5159001/diff/4001/chrome/browser/sync/glue/aut...
File chrome/browser/sync/glue/autofill_data_type_controller.h (right):

http://codereview.chromium.org/5159001/diff/4001/chrome/browser/sync/glue/aut...
chrome/browser/sync/glue/autofill_data_type_controller.h:84: ProfileSyncFactory*
profile_sync_factory_;
make it a getter.

http://codereview.chromium.org/5159001/diff/4001/chrome/browser/sync/glue/aut...
File chrome/browser/sync/glue/autofill_model_associator.cc (right):

http://codereview.chromium.org/5159001/diff/4001/chrome/browser/sync/glue/aut...
chrome/browser/sync/glue/autofill_model_associator.cc:503: 
call the fn in model associator.

http://codereview.chromium.org/5159001/diff/4001/chrome/browser/sync/glue/aut...
File chrome/browser/sync/glue/autofill_profile_change_processor.cc (right):

http://codereview.chromium.org/5159001/diff/4001/chrome/browser/sync/glue/aut...
chrome/browser/sync/glue/autofill_profile_change_processor.cc:119:
DCHECK_EQ(type.value, NotificationType::AUTOFILL_PROFILE_CHANGED_GUID);
put the dcheck right at the top.

http://codereview.chromium.org/5159001/diff/4001/chrome/browser/sync/glue/aut...
chrome/browser/sync/glue/autofill_profile_change_processor.cc:220:
profile_specifics);
make sure David is ok with this.

http://codereview.chromium.org/5159001/diff/4001/chrome/browser/sync/glue/aut...
chrome/browser/sync/glue/autofill_profile_change_processor.cc:231:
AutoFillProfile *p;
see if you can move P into a scope.

http://codereview.chromium.org/5159001/diff/4001/chrome/browser/sync/glue/do_...
File chrome/browser/sync/glue/do_optimistic_refresh_task.h (right):

http://codereview.chromium.org/5159001/diff/4001/chrome/browser/sync/glue/do_...
chrome/browser/sync/glue/do_optimistic_refresh_task.h:9: #include
"chrome/browser/autofill/personal_data_manager.h"
name this to indicate that is related to autofill.

http://codereview.chromium.org/5159001/diff/4001/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_factory_impl.cc (right):

http://codereview.chromium.org/5159001/diff/4001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_factory_impl.cc:157:
if(!command_line_->HasSwitch(switches::kDisableSyncAutofillProfile)) {
check for disable new autofill profile switch as well.

[lipalani, 2010-12-09 19:45:25]

Comments from yesterday's offline code review.

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.cc:1150: return
lookup->set_autofill_migration_state_debug_info(property_to_set, info);
change it to less than 80 characters.

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.cc:1396: SetAutofillMigrationState(
fix 80 characters

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
File chrome/browser/sync/glue/autofill_change_processor2.cc (right):

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
chrome/browser/sync/glue/autofill_change_processor2.cc:475: if
(!web_database_->GetAutoFillProfileForLabel(label, &p)) {
check to make sure you are not conflicting with some one else's changes.

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
File chrome/browser/sync/glue/autofill_model_associator.cc (right):

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
chrome/browser/sync/glue/autofill_model_associator.cc:511: 
get rid of the variable.

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
chrome/browser/sync/glue/autofill_model_associator.cc:522: 
add a redirect comment.

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
File chrome/browser/sync/glue/autofill_profile_model_associator.cc (right):

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
chrome/browser/sync/glue/autofill_profile_model_associator.cc:152: 
move this code above.

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
File chrome/browser/sync/glue/autofill_profile_model_associator.h (right):

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
chrome/browser/sync/glue/autofill_profile_model_associator.h:104: 
remove the extra space here.

[tim (not reviewing), 2010-12-09 20:04:14]

Sorry I somehow glazed over this yesterday during the walk-through :(

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncapi.h (right):

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.h:51: #include
"chrome/browser/sync/syncable/syncable.h"
we can't do this.  the whole point of syncapi is to be a facade atop syncable
and the rest of engine.  the inclusion of model_type is less egregious since we
could (and should, perhaps) put that file anywhere or in a /public/ directory. 
the cryptographer include is egregious.  there are todo's all over the code to
try and clean up cases like this :( (see sync_backend_host.cc@25).  Step 1 is to
stop the bleeding.

Given the precedence, it's not your fault for adding this :)
We have a bug to add a check_deps rule for sync so that these changes would fail
to build on try bots... we should really get on that.

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.h:571: using syncable::Directory;
using declaration not allowed in .h

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.h:832: GetAutofillMigrationState();
if you want to go through syncapi for this, you'll have to declare types here
and convert in the .cc file.  syncapi.h isn't a "master include" file for all
the guts of sync/, it's a facade that should abstract away very nitty gritty
details like PersistedKernelInfo.  It makes the whole api much more confusing
and challenging to conceptualize as a hermetic "sync" "api".

[tim (not reviewing), 2010-12-10 22:16:02]

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.cc:37: #include
"chrome/browser/sync/glue/autofill_profile_model_associator.h"
this is also strictly not allowed. glue is code chrome uses to  interact with
the syncapi, which does not know about anything at a "higher" level, like glue.
See the "stack" image here:
https://docs.google.com/Doc?docid=0Aa_pnb1d4_gnZDUzOWp4bV8yNXg2OW5oZGI&hl=en

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
File chrome/browser/sync/glue/autofill_model_associator.cc (right):

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
chrome/browser/sync/glue/autofill_model_associator.cc:193:
syncable::AutofillMigrationDebugInfo debug_info;
indent here or line above looks off

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
chrome/browser/sync/glue/autofill_model_associator.cc:195:
number_of_entries_created_;
4 spaces indent

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
chrome/browser/sync/glue/autofill_model_associator.cc:197:
syncable::AutofillMigrationDebugInfo::ENTRIES_ADDED,
4 spaces indent

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
chrome/browser/sync/glue/autofill_model_associator.cc:332: AutoFillProfile*
profile_in_web_db = FindCorrespondingNodeFromWebDB(
why no scoped_ptr?

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
File chrome/browser/sync/glue/autofill_profile_change_processor.h (right):

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
chrome/browser/sync/glue/autofill_profile_change_processor.h:36: // Virtual
methods from changeprocessor class
nit - ChangeProcessor and add period.

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
chrome/browser/sync/glue/autofill_profile_change_processor.h:53: // This ensures
that startobsrving gets called after stopobserving even
I like this. We should probably generalize it and put it in it's own file, which
would mean in it's own change.  In the interest of landing this patch can you
just move this to private: with a TODO?

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
chrome/browser/sync/glue/autofill_profile_change_processor.h:70: struct
AutofillProfileChangeRecord {
this should go in syncapi.

[lipalani, 2010-12-11 00:12:36]

CR feedback addressed. Fixing 2 comments are still baffling me(one to move the
autofillprofilechangerecord structure definiton to cc file and another why i
need the utf16toutf8 conversion for printing in DLOG). But other than that all
the comments are addressed.

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.cc:37: #include
"chrome/browser/sync/glue/autofill_profile_model_associator.h"
On 2010/12/10 22:16:02, timsteele wrote:
> this is also strictly not allowed. glue is code chrome uses to  interact with
> the syncapi, which does not know about anything at a "higher" level, like
glue.
> See the "stack" image here:
> https://docs.google.com/Doc?docid=0Aa_pnb1d4_gnZDUzOWp4bV8yNXg2OW5oZGI&hl=en

Done.

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.cc:1150: return
lookup->set_autofill_migration_state_debug_info(property_to_set, info);
On 2010/12/09 19:45:25, lipalani wrote:
> change it to less than 80 characters.

Done.

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.cc:1396: SetAutofillMigrationState(
On 2010/12/09 19:45:25, lipalani wrote:
> fix 80 characters

Done.

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncapi.h (right):

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.h:51: #include
"chrome/browser/sync/syncable/syncable.h"
On 2010/12/09 20:04:14, timsteele wrote:
> we can't do this.  the whole point of syncapi is to be a facade atop syncable
> and the rest of engine.  the inclusion of model_type is less egregious since
we
> could (and should, perhaps) put that file anywhere or in a /public/ directory.

> the cryptographer include is egregious.  there are todo's all over the code to
> try and clean up cases like this :( (see sync_backend_host.cc@25).  Step 1 is
to
> stop the bleeding.
> 
> Given the precedence, it's not your fault for adding this :)
> We have a bug to add a check_deps rule for sync so that these changes would
fail
> to build on try bots... we should really get on that.

Done.

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.h:571: using syncable::Directory;
On 2010/12/09 20:04:14, timsteele wrote:
> using declaration not allowed in .h

Done.

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.h:832: GetAutofillMigrationState();
On 2010/12/09 20:04:14, timsteele wrote:
> if you want to go through syncapi for this, you'll have to declare types here
> and convert in the .cc file.  syncapi.h isn't a "master include" file for all
> the guts of sync/, it's a facade that should abstract away very nitty gritty
> details like PersistedKernelInfo.  It makes the whole api much more confusing
> and challenging to conceptualize as a hermetic "sync" "api".

Done.

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
File chrome/browser/sync/glue/autofill_model_associator.cc (right):

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
chrome/browser/sync/glue/autofill_model_associator.cc:193:
syncable::AutofillMigrationDebugInfo debug_info;
On 2010/12/10 22:16:02, timsteele wrote:
> indent here or line above looks off

Done.

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
chrome/browser/sync/glue/autofill_model_associator.cc:195:
number_of_entries_created_;
On 2010/12/10 22:16:02, timsteele wrote:
> 4 spaces indent

Done.

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
chrome/browser/sync/glue/autofill_model_associator.cc:197:
syncable::AutofillMigrationDebugInfo::ENTRIES_ADDED,
On 2010/12/10 22:16:02, timsteele wrote:
> 4 spaces indent

Done.

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
chrome/browser/sync/glue/autofill_model_associator.cc:332: AutoFillProfile*
profile_in_web_db = FindCorrespondingNodeFromWebDB(
The pointer we are getting is part of the vector all_profiles_from_db. when that
is destroyed it would destroy its children. we should not use scope and destroy
it here.
On 2010/12/10 22:16:02, timsteele wrote:
> why no scoped_ptr?

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
chrome/browser/sync/glue/autofill_model_associator.cc:511: 
we need the transaction variable. this comment was probably from a real old CR.
On 2010/12/09 19:45:25, lipalani wrote:
> get rid of the variable.

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
chrome/browser/sync/glue/autofill_model_associator.cc:522: 
On 2010/12/09 19:45:25, lipalani wrote:
> add a redirect comment.

Done.

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
File chrome/browser/sync/glue/autofill_profile_change_processor.h (right):

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
chrome/browser/sync/glue/autofill_profile_change_processor.h:36: // Virtual
methods from changeprocessor class
On 2010/12/10 22:16:02, timsteele wrote:
> nit - ChangeProcessor and add period.

Done.

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
chrome/browser/sync/glue/autofill_profile_change_processor.h:53: // This ensures
that startobsrving gets called after stopobserving even
On 2010/12/10 22:16:02, timsteele wrote:
> I like this. We should probably generalize it and put it in it's own file,
which
> would mean in it's own change.  In the interest of landing this patch can you
> just move this to private: with a TODO?

Done.

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
chrome/browser/sync/glue/autofill_profile_change_processor.h:70: struct
AutofillProfileChangeRecord {
moved the definition to the .cc file. consistent with autofillchangerecord.

But that is giving a weird compile error that I am trying to figure out now. But
I did not want to hold up the rest of the review for this single thing. So
sending it as such.
On 2010/12/10 22:16:02, timsteele wrote:
> this should go in syncapi.

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
File chrome/browser/sync/glue/autofill_profile_model_associator.cc (right):

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
chrome/browser/sync/glue/autofill_profile_model_associator.cc:152: 
On 2010/12/09 19:45:25, lipalani wrote:
> move this code above.

Done.

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
File chrome/browser/sync/glue/autofill_profile_model_associator.h (right):

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/au...
chrome/browser/sync/glue/autofill_profile_model_associator.h:104: 
On 2010/12/09 19:45:25, lipalani wrote:
> remove the extra space here.

Done.

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/bo...
File chrome/browser/sync/glue/bookmark_change_processor.cc (right):

http://codereview.chromium.org/5159001/diff/64001/chrome/browser/sync/glue/bo...
chrome/browser/sync/glue/bookmark_change_processor.cc:448: << " Node title: " <<
UTF16ToUTF8(src->GetTitle())
Pretty weird. In the repo it is compiling with out having to have a conversion.
But in my local machine if we dont have that compilation fails...
Looking into it.

[lipalani, 2010-12-11 00:25:03]

Couple of lint errors sneaked in my previous patch. fixed it. this should be
lint error free.

[tim (not reviewing), 2010-12-13 19:24:33]

many style nits.  You probably want to reply to each one individually with
'Done' or a comment to make sure you don't miss any.

Also there are compile failures on the try runs.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/engine...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.cc:1113: GetAutofillMigrationState() {
indent or previous line

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.cc:1118: syncable::NOT_MIGRATED;
previous line?

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.cc:1125: syncable::AutofillMigrationState
state) {
previous line? or indent 4

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.cc:1145:
lookup->set_autofill_migration_state_debug_info(property_to_set, info);
should start this on the previous line and then indent 4 spaces

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/engine...
File chrome/browser/sync/engine/syncapi.h (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.h:74: class Directory;
dont think this is necessary

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_change_processor.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_change_processor.cc:13: #include
"chrome/browser/autofill/personal_data_manager.h"
alphabetical

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_change_processor.cc:292: 
extra newline

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_change_processor.cc:447: 
extra newline

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_change_processor2.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_change_processor2.cc:477: <<
UTF16ToUTF8(label);
how could this be necessary if the build isn't broken?

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_data_type_controller.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_data_type_controller.cc:278: 
extra newline

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_model_associator.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.cc:24: #include
"chrome/browser/sync/syncable/syncable.h"
illegal include

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.cc:277: VLOG(1) << "[AUTOFILL
MIGRATION] Looking for "
it seems weird to combine vlog with another conditional log, should we just use
LOG(INFO) here instead if we're controlling it ourselves?

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.cc:546: if
(autofill_migration_state_ ==
I thought we were getting rid of this cache?

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.cc:547: syncable::MIGRATED) {
I think this fits on previous line, but it shouldn't be syncable:: namespace.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.cc:562:
syncable::NOT_MIGRATED) {
same comment as above

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.cc:598: syncable::MIGRATED;
prev line

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.cc:605: // [TODO] enable
logging via a command line flag.
what's the plan here?

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_model_associator.h (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.h:21: #include
"chrome/browser/sync/syncable/syncable.h"
illegal include

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.h:194:
syncable::AutofillMigrationState
shouldn't this be in the autofill_migration.h?

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_profile_change_processor.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_change_processor.cc:62: ScopedObserver
observer(this);
this should be ScopedStopObserving or something to denote the fact that it
begins by 'stopping'

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_change_processor.cc:71: for (int i =
0;i < change_count; ++i) {
space after 0;

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_change_processor.cc:78:
changes[i].specifics.GetExtension(sync_pb::autofill_profile);
indent 4 spaces.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_change_processor.cc:181:
autofill_changes_[i].action_) {
needs 4 space indent.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_profile_change_processor.h (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_change_processor.h:58: struct
AutofillProfileChangeRecord {
indent is off.  do we need this here or only in .cc file?

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_profile_data_type_controller.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_data_type_controller.cc:10: 
extra newline

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_profile_model_associator.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_model_associator.cc:187: new
DoOptimisticRefreshForAutofill(personal_data_));
nit indent

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/b...
File chrome/browser/sync/glue/bookmark_model_associator.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_model_associator.cc:17: #include
"chrome/browser/sync/syncable/syncable.h"
illegal include

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_model_associator.cc:423: syncable::MIGRATED) {
4 space indent

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_model_associator.cc:426:
number_of_new_sync_nodes_created_at_association_;
4 space indent

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_model_associator.cc:428:
syncable::AutofillMigrationDebugInfo::BOOKMARK_ADDED,
4 space indent

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/d...
File chrome/browser/sync/glue/do_optimistic_refresh_task.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/do_optimistic_refresh_task.cc:5: #include
"chrome/browser/sync/glue/do_optimistic_refresh_Task.h"
lowercase 'task'

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:280: == syncable::NOT_DETERMINED)
{
== should go on previous line, and then indent here 4 spaces if it doesn't all
fit up there

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:285: if
(!autofil_root_node.InitByTagLookup(browser_sync::kAutofillTag)) {
typo: autofill

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:294:
syncable::INSUFFICIENT_INFO_TO_DETERMINE);
fits on prev line? maybe?

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:314: syncable::NOT_MIGRATED);
fits on prev line.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.h (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.h:25: #include
"chrome/browser/sync/syncable/syncable.h"
illegal include.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.h:462: void
SyncBackendHost::ConfigureAutofillMigration();
I think you can remove the 'SyncBackendHost::'

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_factory_impl.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_factory_impl.cc:13: #include
"chrome/browser/sync/glue/autofill_profile_data_type_controller.h"
alphabetical

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_factory_impl.cc:186: 
extra newlines

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:464: 
remove newline

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:925: if
(!registered_types.count(syncable::AUTOFILL_PROFILE))
can you add a comment explaining you're trying to group autofill_profile with
autofill here?

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/sync_u...
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/sync_u...
chrome/browser/sync/sync_ui_util.cc:358: "Autofill Migrated",
indent 4 spaces

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/sync_u...
chrome/browser/sync/sync_ui_util.cc:362:
service->backend()->GetAutofillMigrationDebugInfo();
4 spaces.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
File chrome/browser/sync/syncable/autofill_migration.h (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/autofill_migration.h:12: NOT_MIGRATED,
comments for states

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
File chrome/browser/sync/syncable/directory_backing_store.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/directory_backing_store.cc:390:
info.autofill_migration_debug_info;
indent 4 spaces

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/directory_backing_store.cc:594:
static_cast<AutofillMigrationState> (query.column_int(4));
indent 4 spaces. would be nice to have dchecks on bounds of int

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/directory_backing_store.cc:977:
ExecQuery(load_dbhandle_,
prev line

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/directory_backing_store.cc:986:
ExecQuery(load_dbhandle_,
prev line

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/directory_backing_store.cc:994:
ExecQuery(load_dbhandle_,
prev line

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/directory_backing_store.cc:1003:
ExecQuery(load_dbhandle_,
prev line

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
File chrome/browser/sync/syncable/model_type.h (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/model_type.h:52: // An autofill Profile Object
AutoFill Profile

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
File chrome/browser/sync/syncable/syncable.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/syncable.cc:794:
kernel_->persisted_info.autofill_migration_debug_info;
indent 4 spaces

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/syncable.cc:796:
base::Time::Now().ToInternalValue();
indent 4 spaces

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
File chrome/browser/sync/syncable/syncable.h (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/syncable.h:772:
syncable::AutofillMigrationDebugInfo::PropertyToSet property_to_set,
nit- indent 4 spaces

http://codereview.chromium.org/5159001/diff/135001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/common/pref_names.c...
chrome/common/pref_names.cc:995: const char kSyncAutofill_profile[] =
"sync.autofillprofile";
the underscore should be on the RHS here
kSyncAutoFillProfile = "sync.autofill_profile"

http://codereview.chromium.org/5159001/diff/135001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/common/pref_names.h...
chrome/common/pref_names.h:370: extern const char kSyncAutofill_profile[];
no underscores

[lipalani, 2010-12-14 21:05:57]

fixed all the CR feedback except the issue of DLOG(warning) not taking a utf8.
should have a fix/workaround today evening.

will continue with try bots.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/engine...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.cc:1113: GetAutofillMigrationState() {
On 2010/12/13 19:24:33, timsteele wrote:
> indent or previous line

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.cc:1118: syncable::NOT_MIGRATED;
On 2010/12/13 19:24:33, timsteele wrote:
> previous line?

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.cc:1125: syncable::AutofillMigrationState
state) {
On 2010/12/13 19:24:33, timsteele wrote:
> previous line? or indent 4

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.cc:1145:
lookup->set_autofill_migration_state_debug_info(property_to_set, info);
On 2010/12/13 19:24:33, timsteele wrote:
> should start this on the previous line and then indent 4 spaces

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_data_type_controller.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_data_type_controller.cc:278: 
On 2010/12/13 19:24:33, timsteele wrote:
> extra newline

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_model_associator.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.cc:24: #include
"chrome/browser/sync/syncable/syncable.h"
On 2010/12/13 19:24:33, timsteele wrote:
> illegal include

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.cc:277: VLOG(1) << "[AUTOFILL
MIGRATION] Looking for "
Would it be OK if I dealt with logging on the next cl?
On 2010/12/13 19:24:33, timsteele wrote:
> it seems weird to combine vlog with another conditional log, should we just
use
> LOG(INFO) here instead if we're controlling it ourselves?

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.cc:546: if
(autofill_migration_state_ ==
Yeah I got rid of it! it is reflected in the current cl.
On 2010/12/13 19:24:33, timsteele wrote:
> I thought we were getting rid of this cache?

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.cc:547: syncable::MIGRATED) {
I fixed the line indentation. Not sure I understand about the syncable
namespace. The header file autofill_migration.h is in syncable namespace similar
to model_types.h
On 2010/12/13 19:24:33, timsteele wrote:
> I think this fits on previous line, but it shouldn't be syncable:: namespace.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.cc:562:
syncable::NOT_MIGRATED) {
On 2010/12/13 19:24:33, timsteele wrote:
> same comment as above

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.cc:598: syncable::MIGRATED;
Removed.
On 2010/12/13 19:24:33, timsteele wrote:
> prev line

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.cc:605: // [TODO] enable
logging via a command line flag.
Logging part of the next cl?
On 2010/12/13 19:24:33, timsteele wrote:
> what's the plan here?

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_profile_change_processor.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_change_processor.cc:62: ScopedObserver
observer(this);
On 2010/12/13 19:24:33, timsteele wrote:
> this should be ScopedStopObserving or something to denote the fact that it
> begins by 'stopping'

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_change_processor.cc:71: for (int i =
0;i < change_count; ++i) {
On 2010/12/13 19:24:33, timsteele wrote:
> space after 0; 

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_change_processor.cc:78:
changes[i].specifics.GetExtension(sync_pb::autofill_profile);
On 2010/12/13 19:24:33, timsteele wrote:
> indent 4 spaces.

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_change_processor.cc:181:
autofill_changes_[i].action_) {
On 2010/12/13 19:24:33, timsteele wrote:
> needs 4 space indent.

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_profile_change_processor.h (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_change_processor.h:58: struct
AutofillProfileChangeRecord {
I put this in the .h file alone. Is it ok?
On 2010/12/13 19:24:33, timsteele wrote:
> indent is off.  do we need this here or only in .cc file?

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/b...
File chrome/browser/sync/glue/bookmark_model_associator.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_model_associator.cc:423: syncable::MIGRATED) {
On 2010/12/13 19:24:33, timsteele wrote:
> 4 space indent

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_model_associator.cc:426:
number_of_new_sync_nodes_created_at_association_;
On 2010/12/13 19:24:33, timsteele wrote:
> 4 space indent

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_model_associator.cc:428:
syncable::AutofillMigrationDebugInfo::BOOKMARK_ADDED,
On 2010/12/13 19:24:33, timsteele wrote:
> 4 space indent

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/d...
File chrome/browser/sync/glue/do_optimistic_refresh_task.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/do_optimistic_refresh_task.cc:5: #include
"chrome/browser/sync/glue/do_optimistic_refresh_Task.h"
On 2010/12/13 19:24:33, timsteele wrote:
> lowercase 'task'

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:280: == syncable::NOT_DETERMINED)
{
On 2010/12/13 19:24:33, timsteele wrote:
> == should go on previous line, and then indent here 4 spaces if it doesn't all
> fit up there

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:285: if
(!autofil_root_node.InitByTagLookup(browser_sync::kAutofillTag)) {
On 2010/12/13 19:24:33, timsteele wrote:
> typo: autofill

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:294:
syncable::INSUFFICIENT_INFO_TO_DETERMINE);
On 2010/12/13 19:24:33, timsteele wrote:
> fits on prev line? maybe?

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:314: syncable::NOT_MIGRATED);
On 2010/12/13 19:24:33, timsteele wrote:
> fits on prev line.

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.h (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.h:462: void
SyncBackendHost::ConfigureAutofillMigration();
On 2010/12/13 19:24:33, timsteele wrote:
> I think you can remove the 'SyncBackendHost::'

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_factory_impl.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_factory_impl.cc:186: 
On 2010/12/13 19:24:33, timsteele wrote:
> extra newlines

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:464: 
On 2010/12/13 19:24:33, timsteele wrote:
> remove newline

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/sync_u...
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/sync_u...
chrome/browser/sync/sync_ui_util.cc:358: "Autofill Migrated",
On 2010/12/13 19:24:33, timsteele wrote:
> indent 4 spaces 

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/sync_u...
chrome/browser/sync/sync_ui_util.cc:362:
service->backend()->GetAutofillMigrationDebugInfo();
On 2010/12/13 19:24:33, timsteele wrote:
> 4 spaces.

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
File chrome/browser/sync/syncable/autofill_migration.h (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/autofill_migration.h:12: NOT_MIGRATED,
On 2010/12/13 19:24:33, timsteele wrote:
> comments for states

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
File chrome/browser/sync/syncable/directory_backing_store.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/directory_backing_store.cc:390:
info.autofill_migration_debug_info;
On 2010/12/13 19:24:33, timsteele wrote:
> indent 4 spaces

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/directory_backing_store.cc:977:
ExecQuery(load_dbhandle_,
On 2010/12/13 19:24:33, timsteele wrote:
> prev line

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/directory_backing_store.cc:986:
ExecQuery(load_dbhandle_,
On 2010/12/13 19:24:33, timsteele wrote:
> prev line

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/directory_backing_store.cc:994:
ExecQuery(load_dbhandle_,
On 2010/12/13 19:24:33, timsteele wrote:
> prev line

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/directory_backing_store.cc:1003:
ExecQuery(load_dbhandle_,
On 2010/12/13 19:24:33, timsteele wrote:
> prev line

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
File chrome/browser/sync/syncable/model_type.h (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/model_type.h:52: // An autofill Profile Object
On 2010/12/13 19:24:33, timsteele wrote:
> AutoFill Profile

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
File chrome/browser/sync/syncable/syncable.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/syncable.cc:794:
kernel_->persisted_info.autofill_migration_debug_info;
On 2010/12/13 19:24:33, timsteele wrote:
> indent 4 spaces

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/syncable.cc:796:
base::Time::Now().ToInternalValue();
On 2010/12/13 19:24:33, timsteele wrote:
> indent 4 spaces

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
File chrome/browser/sync/syncable/syncable.h (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/syncable.h:772:
syncable::AutofillMigrationDebugInfo::PropertyToSet property_to_set,
On 2010/12/13 19:24:33, timsteele wrote:
> nit- indent 4 spaces

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/common/pref_names.c...
chrome/common/pref_names.cc:995: const char kSyncAutofill_profile[] =
"sync.autofillprofile";
On 2010/12/13 19:24:33, timsteele wrote:
> the underscore should be on the RHS here
> kSyncAutoFillProfile = "sync.autofill_profile"

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/common/pref_names.h...
chrome/common/pref_names.h:370: extern const char kSyncAutofill_profile[];
On 2010/12/13 19:24:33, timsteele wrote:
> no underscores

Done.

[tim (not reviewing), 2010-12-14 21:46:43]

Some files weren't uploaded with changes in this patch set and several files
still have unanswered comments.  Please take another look at the last review and
make sure the comments : replies ratio matches up.

http://codereview.chromium.org/5159001/diff/152001/chrome/browser/sync/engine...
File chrome/browser/sync/engine/syncapi.h (right):

http://codereview.chromium.org/5159001/diff/152001/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.h:74: class Directory;
was in previous review, still here

http://codereview.chromium.org/5159001/diff/152001/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.h:828: GetAutofillMigrationState();
indent.

http://codereview.chromium.org/5159001/diff/152001/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.h:831: syncable::AutofillMigrationState
state);
indent.

[lipalani, 2010-12-15 09:08:33]

Hopefully fixed all the CR feedback.
try job on windows passed. The test failure on linux seems unrelated. Waiting on
mac.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/engine...
File chrome/browser/sync/engine/syncapi.h (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.h:74: class Directory;
On 2010/12/13 19:24:33, timsteele wrote:
> dont think this is necessary

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_change_processor.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_change_processor.cc:13: #include
"chrome/browser/autofill/personal_data_manager.h"
On 2010/12/13 19:24:33, timsteele wrote:
> alphabetical

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_change_processor.cc:292: 
On 2010/12/13 19:24:33, timsteele wrote:
> extra newline

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_change_processor.cc:447: 
On 2010/12/13 19:24:33, timsteele wrote:
> extra newline

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_change_processor2.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_change_processor2.cc:477: <<
UTF16ToUTF8(label);
On 2010/12/13 19:24:33, timsteele wrote:
> how could this be necessary if the build isn't broken?

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_model_associator.h (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.h:21: #include
"chrome/browser/sync/syncable/syncable.h"
On 2010/12/13 19:24:33, timsteele wrote:
> illegal include

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.h:194:
syncable::AutofillMigrationState
removed.
On 2010/12/13 19:24:33, timsteele wrote:
> shouldn't this be in the autofill_migration.h?

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_profile_data_type_controller.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_data_type_controller.cc:10: 
On 2010/12/13 19:24:33, timsteele wrote:
> extra newline

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_profile_model_associator.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_model_associator.cc:187: new
DoOptimisticRefreshForAutofill(personal_data_));
On 2010/12/13 19:24:33, timsteele wrote:
> nit indent

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/b...
File chrome/browser/sync/glue/bookmark_model_associator.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_model_associator.cc:17: #include
"chrome/browser/sync/syncable/syncable.h"
On 2010/12/13 19:24:33, timsteele wrote:
> illegal include

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.h (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.h:25: #include
"chrome/browser/sync/syncable/syncable.h"
On 2010/12/13 19:24:33, timsteele wrote:
> illegal include.

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_factory_impl.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_factory_impl.cc:13: #include
"chrome/browser/sync/glue/autofill_profile_data_type_controller.h"
On 2010/12/13 19:24:33, timsteele wrote:
> alphabetical

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:925: if
(!registered_types.count(syncable::AUTOFILL_PROFILE))
On 2010/12/13 19:24:33, timsteele wrote:
> can you add a comment explaining you're trying to group autofill_profile with
> autofill here?

Done.

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
File chrome/browser/sync/syncable/directory_backing_store.cc (right):

http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/directory_backing_store.cc:594:
static_cast<AutofillMigrationState> (query.column_int(4));
Not sure how dchecks can be added. fixed the indents.
On 2010/12/13 19:24:33, timsteele wrote:
> indent 4 spaces. would be nice to have dchecks on bounds of int

Done.

http://codereview.chromium.org/5159001/diff/152001/chrome/browser/sync/engine...
File chrome/browser/sync/engine/syncapi.h (right):

http://codereview.chromium.org/5159001/diff/152001/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.h:74: class Directory;
On 2010/12/14 21:46:43, timsteele wrote:
> was in previous review, still here

Done.

http://codereview.chromium.org/5159001/diff/152001/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.h:828: GetAutofillMigrationState();
On 2010/12/14 21:46:43, timsteele wrote:
> indent.

Done.

http://codereview.chromium.org/5159001/diff/152001/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.h:831: syncable::AutofillMigrationState
state);
On 2010/12/14 21:46:43, timsteele wrote:
> indent.

Done.

[lipalani, 2010-12-15 15:59:25]

try job on mac passed as well. I might retry the linux alone...

On Wed, Dec 15, 2010 at 1:08 AM, <lipalani@google.com> wrote:

> Hopefully fixed all the CR feedback.
> try job on windows passed. The test failure on linux seems unrelated.
> Waiting on
> mac.
>
>
>
>
http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/engine...
>
> File chrome/browser/sync/engine/syncapi.h (right):
>
>
>
http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/engine...
>
> chrome/browser/sync/engine/syncapi.h:74: class Directory;
> On 2010/12/13 19:24:33, timsteele wrote:
>
>> dont think this is necessary
>>
>
> Done.
>
>
>
http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
> File chrome/browser/sync/glue/autofill_change_processor.cc (right):
>
>
>
http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
> chrome/browser/sync/glue/autofill_change_processor.cc:13: #include
> "chrome/browser/autofill/personal_data_manager.h"
>
> On 2010/12/13 19:24:33, timsteele wrote:
>
>> alphabetical
>>
>
> Done.
>
>
>
http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
> chrome/browser/sync/glue/autofill_change_processor.cc:292:
>
> On 2010/12/13 19:24:33, timsteele wrote:
>
>> extra newline
>>
>
> Done.
>
>
>
http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
> chrome/browser/sync/glue/autofill_change_processor.cc:447:
> On 2010/12/13 19:24:33, timsteele wrote:
>
>> extra newline
>>
>
> Done.
>
>
>
http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
> File chrome/browser/sync/glue/autofill_change_processor2.cc (right):
>
>
>
http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
> chrome/browser/sync/glue/autofill_change_processor2.cc:477: <<
> UTF16ToUTF8(label);
> On 2010/12/13 19:24:33, timsteele wrote:
>
>> how could this be necessary if the build isn't broken?
>>
>
> Done.
>
>
>
http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
> File chrome/browser/sync/glue/autofill_model_associator.h (right):
>
>
>
http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
> chrome/browser/sync/glue/autofill_model_associator.h:21: #include
>
> "chrome/browser/sync/syncable/syncable.h"
> On 2010/12/13 19:24:33, timsteele wrote:
>
>> illegal include
>>
>
> Done.
>
>
>
http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
> chrome/browser/sync/glue/autofill_model_associator.h:194:
> syncable::AutofillMigrationState
> removed.
>
> On 2010/12/13 19:24:33, timsteele wrote:
>
>> shouldn't this be in the autofill_migration.h?
>>
>
>
>
http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
> File chrome/browser/sync/glue/autofill_profile_data_type_controller.cc
> (right):
>
>
>
http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
> chrome/browser/sync/glue/autofill_profile_data_type_controller.cc:10:
> On 2010/12/13 19:24:33, timsteele wrote:
>
>> extra newline
>>
>
> Done.
>
>
>
http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
> File chrome/browser/sync/glue/autofill_profile_model_associator.cc
> (right):
>
>
>
http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/a...
> chrome/browser/sync/glue/autofill_profile_model_associator.cc:187: new
> DoOptimisticRefreshForAutofill(personal_data_));
> On 2010/12/13 19:24:33, timsteele wrote:
>
>> nit indent
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/b...
> File chrome/browser/sync/glue/bookmark_model_associator.cc (right):
>
>
>
http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/b...
> chrome/browser/sync/glue/bookmark_model_associator.cc:17: #include
>
> "chrome/browser/sync/syncable/syncable.h"
> On 2010/12/13 19:24:33, timsteele wrote:
>
>> illegal include
>>
>
> Done.
>
>
>
http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/s...
> File chrome/browser/sync/glue/sync_backend_host.h (right):
>
>
>
http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/glue/s...
> chrome/browser/sync/glue/sync_backend_host.h:25: #include
>
> "chrome/browser/sync/syncable/syncable.h"
> On 2010/12/13 19:24:33, timsteele wrote:
>
>> illegal include.
>>
>
> Done.
>
>
>
http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/profil...
> File chrome/browser/sync/profile_sync_factory_impl.cc (right):
>
>
>
http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/profil...
> chrome/browser/sync/profile_sync_factory_impl.cc:13: #include
> "chrome/browser/sync/glue/autofill_profile_data_type_controller.h"
>
> On 2010/12/13 19:24:33, timsteele wrote:
>
>> alphabetical
>>
>
> Done.
>
>
>
http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/profil...
> File chrome/browser/sync/profile_sync_service.cc (right):
>
>
>
>
http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/profil...
> chrome/browser/sync/profile_sync_service.cc:925: if
> (!registered_types.count(syncable::AUTOFILL_PROFILE))
> On 2010/12/13 19:24:33, timsteele wrote:
>
>> can you add a comment explaining you're trying to group
>>
> autofill_profile with
>
>> autofill here?
>>
>
> Done.
>
>
>
http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
> File chrome/browser/sync/syncable/directory_backing_store.cc (right):
>
>
>
>
http://codereview.chromium.org/5159001/diff/135001/chrome/browser/sync/syncab...
> chrome/browser/sync/syncable/directory_backing_store.cc:594:
> static_cast<AutofillMigrationState> (query.column_int(4));
> Not sure how dchecks can be added. fixed the indents.
>
> On 2010/12/13 19:24:33, timsteele wrote:
>
>> indent 4 spaces. would be nice to have dchecks on bounds of int
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/5159001/diff/152001/chrome/browser/sync/engine...
> File chrome/browser/sync/engine/syncapi.h (right):
>
>
>
http://codereview.chromium.org/5159001/diff/152001/chrome/browser/sync/engine...
> chrome/browser/sync/engine/syncapi.h:74: class Directory;
> On 2010/12/14 21:46:43, timsteele wrote:
>
>> was in previous review, still here
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/5159001/diff/152001/chrome/browser/sync/engine...
> chrome/browser/sync/engine/syncapi.h:828: GetAutofillMigrationState();
> On 2010/12/14 21:46:43, timsteele wrote:
>
>> indent.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/5159001/diff/152001/chrome/browser/sync/engine...
> chrome/browser/sync/engine/syncapi.h:831:
> syncable::AutofillMigrationState state);
> On 2010/12/14 21:46:43, timsteele wrote:
>
>> indent.
>>
>
> Done.
>
>
> http://codereview.chromium.org/5159001/
>

[lipalani, 2010-12-15 19:34:29]

Please review the latest patch.

I am running one more try on this.

[tim (not reviewing), 2010-12-15 20:11:41]

More comments, almost entirely style nits.  (Apologies I'm slow and picky at
reviewing this.  I suggest making a couple self-review passes yourself over the
code to try and clean up other instances as you get more used to chromium
style).

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_model_associator.cc (right):

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.cc:249: <<
UTF16ToUTF8(p->GetFieldText(AutoFillType(NAME_FIRST)))
conversion shouldn't be necessary

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.cc:543:
syncable::AutofillMigrationState autofill_migration_state =
nit - indent is off.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.cc:573: << "Current autofill
migration state is NOT Migrated becausse"
couple typos: 'because' 'autofill' and 'present'

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_profile_change_processor.h (right):

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_change_processor.h:85: //
[TODO](lipalni) - generalize this and add it to other change processors.
todos should be formatted as TODO(lipalani): so we can search effectively.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_profile_data_type_controller.cc (right):

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_data_type_controller.cc:13:
ProfileSyncFactory* profile_sync_factory,
nit - too many spaces.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_profile_model_associator.cc (right):

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_model_associator.cc:57: <<
UTF16ToUTF8(p->GetFieldText(AutoFillType(NAME_FIRST)))
conversion shouldn't be necessary.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_model_associator.cc:80: <<
UTF16ToUTF8((*ix)->GetFieldText(AutoFillType(NAME_LAST)))
conversion shouldn't be necessary.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_model_associator.cc:144: << "root
node";
should have space before 'root'

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_model_associator.cc:179:
number_of_profiles_created_;
nit - indent 2 more spaces.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_model_associator.cc:181:
syncable::AutofillMigrationDebugInfo::PROFILES_ADDED,
nit - indent 2 more spaces.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_model_associator.cc:184:
syncable::MIGRATED);
indent 2 more spaces.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_model_associator.cc:473: // [TODO]
enable logging via a command line flag.
TODO(lipalani):

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_profile_model_associator.h (right):

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_model_associator.h:4: 
nit - extra newline

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_model_associator.h:114: 
nit - extra newline.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/b...
File chrome/browser/sync/glue/bookmark_change_processor.cc (right):

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_change_processor.cc:20: 
nit - extra newline.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_change_processor.cc:50: 
extra newline.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/d...
File chrome/browser/sync/glue/do_optimistic_refresh_task.cc (right):

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/do_optimistic_refresh_task.cc:13: PersonalDataManager*
pdm) : pdm_(pdm) {}
nit - extra space before ':'
PersonalDataManager* pdm): pdm_(pdm)

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:310: syncable::NOT_MIGRATED);
nit - on previous line.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/sync_u...
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/sync_u...
chrome/browser/sync/sync_ui_util.cc:219: std::string ConstructTime(int64
time_in_int) {
see time_formatting.h

I think what you want is either TimeFormatFriendlyDateAndTime or a combo of
TimeFormatTimeOfDay and TimeFormatShortDate.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/syncab...
File chrome/browser/sync/syncable/syncable.h (right):

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/syncable.h:763: get_autofill_migration_state()
const;
nit - previous line or fix indent.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/syncable.h:769: template <class T> void
TestAndSet(T* kernel_data, const T* data_to_set);
does this really need to be in public section?

[lipalani, 2010-12-15 21:28:14]

fixed the cr feedback.(21 out of 21).
The try servers all passed on the previous patch set. 

I will wait for the feedback on this. As soon as it is all clear i will run one
more try server and submit this.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_model_associator.cc (right):

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.cc:249: <<
UTF16ToUTF8(p->GetFieldText(AutoFillType(NAME_FIRST)))
On 2010/12/15 20:11:42, timsteele wrote:
> conversion shouldn't be necessary

Done.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.cc:543:
syncable::AutofillMigrationState autofill_migration_state =
On 2010/12/15 20:11:42, timsteele wrote:
> nit - indent is off.

Done.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.cc:573: << "Current autofill
migration state is NOT Migrated becausse"
On 2010/12/15 20:11:42, timsteele wrote:
> couple typos: 'because' 'autofill' and 'present'

Done.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_profile_change_processor.h (right):

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_change_processor.h:85: //
[TODO](lipalni) - generalize this and add it to other change processors.
On 2010/12/15 20:11:42, timsteele wrote:
> todos should be formatted as TODO(lipalani): so we can search effectively.

Done.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_profile_data_type_controller.cc (right):

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_data_type_controller.cc:13:
ProfileSyncFactory* profile_sync_factory,
On 2010/12/15 20:11:42, timsteele wrote:
> nit - too many spaces.

Done.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_profile_model_associator.cc (right):

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_model_associator.cc:57: <<
UTF16ToUTF8(p->GetFieldText(AutoFillType(NAME_FIRST)))
On 2010/12/15 20:11:42, timsteele wrote:
> conversion shouldn't be necessary.

Done.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_model_associator.cc:80: <<
UTF16ToUTF8((*ix)->GetFieldText(AutoFillType(NAME_LAST)))
On 2010/12/15 20:11:42, timsteele wrote:
> conversion shouldn't be necessary.

Done.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_model_associator.cc:144: << "root
node";
On 2010/12/15 20:11:42, timsteele wrote:
> should have space before 'root'

Done.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_model_associator.cc:179:
number_of_profiles_created_;
On 2010/12/15 20:11:42, timsteele wrote:
> nit - indent 2 more spaces.

Done.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_model_associator.cc:181:
syncable::AutofillMigrationDebugInfo::PROFILES_ADDED,
On 2010/12/15 20:11:42, timsteele wrote:
> nit - indent 2 more spaces.

Done.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_model_associator.cc:184:
syncable::MIGRATED);
On 2010/12/15 20:11:42, timsteele wrote:
> indent 2 more spaces.

Done.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_model_associator.cc:473: // [TODO]
enable logging via a command line flag.
On 2010/12/15 20:11:42, timsteele wrote:
> TODO(lipalani):

Done.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_profile_model_associator.h (right):

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_model_associator.h:4: 
On 2010/12/15 20:11:42, timsteele wrote:
> nit - extra newline

Done.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_model_associator.h:114: 
On 2010/12/15 20:11:42, timsteele wrote:
> nit - extra newline.

Done.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/b...
File chrome/browser/sync/glue/bookmark_change_processor.cc (right):

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_change_processor.cc:20: 
On 2010/12/15 20:11:42, timsteele wrote:
> nit - extra newline.

Done.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_change_processor.cc:50: 
On 2010/12/15 20:11:42, timsteele wrote:
> extra newline.

Done.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/d...
File chrome/browser/sync/glue/do_optimistic_refresh_task.cc (right):

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/do_optimistic_refresh_task.cc:13: PersonalDataManager*
pdm) : pdm_(pdm) {}
On 2010/12/15 20:11:42, timsteele wrote:
> nit - extra space before ':'
> PersonalDataManager* pdm): pdm_(pdm) 

Done.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:310: syncable::NOT_MIGRATED);
On 2010/12/15 20:11:42, timsteele wrote:
> nit - on previous line.

Done.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/sync_u...
File chrome/browser/sync/sync_ui_util.cc (right):

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/sync_u...
chrome/browser/sync/sync_ui_util.cc:219: std::string ConstructTime(int64
time_in_int) {
On 2010/12/15 20:11:42, timsteele wrote:
> see time_formatting.h
> 
> I think what you want is either TimeFormatFriendlyDateAndTime or a combo of
> TimeFormatTimeOfDay and TimeFormatShortDate.

Done.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/syncab...
File chrome/browser/sync/syncable/syncable.h (right):

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/syncable.h:763: get_autofill_migration_state()
const;
On 2010/12/15 20:11:42, timsteele wrote:
> nit - previous line or fix indent.

Done.

http://codereview.chromium.org/5159001/diff/196001/chrome/browser/sync/syncab...
chrome/browser/sync/syncable/syncable.h:769: template <class T> void
TestAndSet(T* kernel_data, const T* data_to_set);
On 2010/12/15 20:11:42, timsteele wrote:
> does this really need to be in public section?

Done.

[tim (not reviewing), 2010-12-15 21:45:27]

Yay! LGTM! (two final comment-only tiny typos)

Perseverance much appreciated :)

http://codereview.chromium.org/5159001/diff/202002/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_model_associator.cc (right):

http://codereview.chromium.org/5159001/diff/202002/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.cc:574: << "legacy autofill
root node is preseent whereas new "
'preseent' > 'present'

http://codereview.chromium.org/5159001/diff/202002/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_profile_change_processor.h (right):

http://codereview.chromium.org/5159001/diff/202002/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_change_processor.h:85: //
TODO(lipalni) - generalize this and add it to other change processors.
lipalni - lipalani

[lipalani, 2010-12-15 21:50:20]

fixed the last 2 comments.

Sent to try bots!!

http://codereview.chromium.org/5159001/diff/202002/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_model_associator.cc (right):

http://codereview.chromium.org/5159001/diff/202002/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_model_associator.cc:574: << "legacy autofill
root node is preseent whereas new "
On 2010/12/15 21:45:28, timsteele wrote:
> 'preseent' > 'present'

Done.

http://codereview.chromium.org/5159001/diff/202002/chrome/browser/sync/glue/a...
File chrome/browser/sync/glue/autofill_profile_change_processor.h (right):

http://codereview.chromium.org/5159001/diff/202002/chrome/browser/sync/glue/a...
chrome/browser/sync/glue/autofill_profile_change_processor.h:85: //
TODO(lipalni) - generalize this and add it to other change processors.
On 2010/12/15 21:45:28, timsteele wrote:
> lipalni - lipalani

Done.