Populate versions on individual nodes in sync model and native bookmark model.

Populate versions on individual nodes in sync model and native bookmark model.

Update transaction versions of changed sync models and entries in
syncable::WriteTransaction after change delegate calculates changes and returns
handles of changed entries. For syncer changes, updated version is passed to
change processor and set on native model and nodes. For sync API changes, model
processor queries for new version and set on native model and nodes after write
transaction closes. 

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

[haitaol1, 2012-10-30 22:18:41]

time: please take a look and let me know if you are fine with the approach.

[haitaol1, 2012-10-31 22:21:28]

Make WriteTransaction not allocatable on heap again. PTAL

[tim (not reviewing), 2012-11-01 18:31:35]

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

http://codereview.chromium.org/11341048/diff/3001/chrome/browser/sync/glue/bo...
chrome/browser/sync/glue/bookmark_change_processor.cc:117: int64 old_version;
Should initialize this, if not to zero, to -1 or something.  Could also DCHECK
before calling UpdateTransactionVersion that it was set.

http://codereview.chromium.org/11341048/diff/3001/chrome/browser/sync/glue/bo...
chrome/browser/sync/glue/bookmark_change_processor.cc:179: int64 sync_id;
initialize these two

http://codereview.chromium.org/11341048/diff/3001/chrome/browser/sync/glue/bo...
chrome/browser/sync/glue/bookmark_change_processor.cc:182:
syncer::WriteTransaction trans(FROM_HERE, share_handle());
Hmm, I see the corner you're in here - you need to get this value from the
transaction, but it is only incremented on destruction, so you don't have an
object to query.  Ideally though, we would not have to open a second transaction
/ mutext to achieve this.  I think it would be cleaner to add a parameter
(perhaps an int64* out param) to the transaction constructor (we could overload
the constructor at least temporarily until we get all data types / callsites
onboard).  On destruction, the transaction would simply set that parameter to
the updated transaction version.

http://codereview.chromium.org/11341048/diff/3001/chrome/browser/sync/glue/bo...
File chrome/browser/sync/glue/bookmark_change_processor.h (right):

http://codereview.chromium.org/11341048/diff/3001/chrome/browser/sync/glue/bo...
chrome/browser/sync/glue/bookmark_change_processor.h:169: void
UpdateVersion(int64 old_version,
nit - UpdateTransactionVersion to keep the naming consistent / unambiguous.

[haitaol1, 2012-11-02 17:57:11]

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

http://codereview.chromium.org/11341048/diff/3001/chrome/browser/sync/glue/bo...
chrome/browser/sync/glue/bookmark_change_processor.cc:117: int64 old_version;
On 2012/11/01 18:31:36, timsteele wrote:
> Should initialize this, if not to zero, to -1 or something.  Could also DCHECK
> before calling UpdateTransactionVersion that it was set.

Done.

http://codereview.chromium.org/11341048/diff/3001/chrome/browser/sync/glue/bo...
chrome/browser/sync/glue/bookmark_change_processor.cc:179: int64 sync_id;
On 2012/11/01 18:31:36, timsteele wrote:
> initialize these two

Done.

http://codereview.chromium.org/11341048/diff/3001/chrome/browser/sync/glue/bo...
chrome/browser/sync/glue/bookmark_change_processor.cc:182:
syncer::WriteTransaction trans(FROM_HERE, share_handle());
On 2012/11/01 18:31:36, timsteele wrote:
> Hmm, I see the corner you're in here - you need to get this value from the
> transaction, but it is only incremented on destruction, so you don't have an
> object to query.  Ideally though, we would not have to open a second
transaction
> / mutext to achieve this.  I think it would be cleaner to add a parameter
> (perhaps an int64* out param) to the transaction constructor (we could
overload
> the constructor at least temporarily until we get all data types / callsites
> onboard).  On destruction, the transaction would simply set that parameter to
> the updated transaction version.

Done. See changes in write_transaction.cc

http://codereview.chromium.org/11341048/diff/3001/chrome/browser/sync/glue/bo...
File chrome/browser/sync/glue/bookmark_change_processor.h (right):

http://codereview.chromium.org/11341048/diff/3001/chrome/browser/sync/glue/bo...
chrome/browser/sync/glue/bookmark_change_processor.h:169: void
UpdateVersion(int64 old_version,
On 2012/11/01 18:31:36, timsteele wrote:
> nit - UpdateTransactionVersion to keep the naming consistent / unambiguous. 

Done.

[Nicolas Zea, 2012-11-02 21:02:25]

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

http://codereview.chromium.org/11341048/diff/10002/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_change_processor.cc:186: // NOTE(haitaol):
Siblings of added node in sync DB will also be updated to
I don't think you need to mark this as a note for yourself. It's a useful
comment anyways, so should just be a normal comment.

http://codereview.chromium.org/11341048/diff/10002/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_change_processor.cc:225: const BookmarkNode*
parent,
fix indent

http://codereview.chromium.org/11341048/diff/10002/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_change_processor.cc:651: if (new_version > 0)
{
is it possible to verify that new_version is > existing version?

http://codereview.chromium.org/11341048/diff/10002/chrome/browser/sync/glue/b...
File chrome/browser/sync/glue/bookmark_change_processor.h (right):

http://codereview.chromium.org/11341048/diff/10002/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_change_processor.h:170: // new version.
|node| -> |nodes|

"to if sync model..." -> "to |new_version| if sync model..."

http://codereview.chromium.org/11341048/diff/10002/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_change_processor.h:171: void
UpdateTransactionVersion(int64 old_version,
old_version -> new_version?

http://codereview.chromium.org/11341048/diff/10002/sync/internal_api/public/w...
File sync/internal_api/public/write_transaction.h (right):

http://codereview.chromium.org/11341048/diff/10002/sync/internal_api/public/w...
sync/internal_api/public/write_transaction.h:33: WriteTransaction(const
tracked_objects::Location& from_here,
Comment about what new_model_version is and why this constructor is separate.

http://codereview.chromium.org/11341048/diff/10002/sync/internal_api/sync_man...
File sync/internal_api/sync_manager_impl.cc (right):

http://codereview.chromium.org/11341048/diff/10002/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:874: std::vector<int64> *entry_changed) {
nit: "std::vector<int64>* entry_changed" (no space before *)

http://codereview.chromium.org/11341048/diff/10002/sync/internal_api/sync_man...
File sync/internal_api/sync_manager_impl.h (right):

http://codereview.chromium.org/11341048/diff/10002/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.h:332: // Each element of this array is a
store of change records produced by
Update comment. No longer an array, and now stored in tree order.

http://codereview.chromium.org/11341048/diff/10002/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.h:338: typedef std::map<int,
ImmutableChangeRecordList> ChangeRecordMap;
why not have ModelType as the key?

http://codereview.chromium.org/11341048/diff/10002/sync/syncable/directory_ch...
File sync/syncable/directory_change_delegate.h (right):

http://codereview.chromium.org/11341048/diff/10002/sync/syncable/directory_ch...
sync/syncable/directory_change_delegate.h:30: std::vector<int64> *entry_changed)
= 0;
nit: fix * to be before space (here and below)

http://codereview.chromium.org/11341048/diff/10002/sync/syncable/write_transa...
File sync/syncable/write_transaction.cc (right):

http://codereview.chromium.org/11341048/diff/10002/sync/syncable/write_transa...
sync/syncable/write_transaction.cc:18: : BaseTransaction(location,
"WriteTransaction", writer, directory) {
initialize new_model_version_ to NULL

http://codereview.chromium.org/11341048/diff/10002/sync/syncable/write_transa...
File sync/syncable/write_transaction.h (right):

http://codereview.chromium.org/11341048/diff/10002/sync/syncable/write_transa...
sync/syncable/write_transaction.h:21: // API changes to one model. After the
transaction is destroyed,
nit: "After the transaction is destroyed" -> "During destruction, "

http://codereview.chromium.org/11341048/diff/10002/sync/syncable/write_transa...
sync/syncable/write_transaction.h:54: int64* new_model_version_;
Comment that you don't have ownership of this.

http://codereview.chromium.org/11341048/diff/10002/sync/test/null_directory_c...
File sync/test/null_directory_change_delegate.cc (right):

http://codereview.chromium.org/11341048/diff/10002/sync/test/null_directory_c...
sync/test/null_directory_change_delegate.cc:19: std::vector<int64> *id_changed)
{}
here too

http://codereview.chromium.org/11341048/diff/10002/sync/test/null_directory_c...
File sync/test/null_directory_change_delegate.h (right):

http://codereview.chromium.org/11341048/diff/10002/sync/test/null_directory_c...
sync/test/null_directory_change_delegate.h:25: std::vector<int64> *id_changed)
OVERRIDE;
"std::vector<int64>* id_changed"

[haitaol1, 2012-11-02 22:58:18]

Still need to add tests

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

http://codereview.chromium.org/11341048/diff/10002/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_change_processor.cc:186: // NOTE(haitaol):
Siblings of added node in sync DB will also be updated to
On 2012/11/02 21:02:25, Nicolas Zea wrote:
> I don't think you need to mark this as a note for yourself. It's a useful
> comment anyways, so should just be a normal comment.

Done.

http://codereview.chromium.org/11341048/diff/10002/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_change_processor.cc:225: const BookmarkNode*
parent,
This looks fine on my desktop and I verified that they are all spaces.

On 2012/11/02 21:02:25, Nicolas Zea wrote:
> fix indent

http://codereview.chromium.org/11341048/diff/10002/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_change_processor.cc:651: if (new_version > 0)
{
To do that, I'll need to query old version after transaction's created. For
verification purpose only, I'd like to verify in tests.

On 2012/11/02 21:02:25, Nicolas Zea wrote:
> is it possible to verify that new_version is > existing version?

http://codereview.chromium.org/11341048/diff/10002/chrome/browser/sync/glue/b...
File chrome/browser/sync/glue/bookmark_change_processor.h (right):

http://codereview.chromium.org/11341048/diff/10002/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_change_processor.h:170: // new version.
On 2012/11/02 21:02:25, Nicolas Zea wrote:
> |node| -> |nodes|
> 
> "to if sync model..." -> "to |new_version| if sync model..."

Done.

http://codereview.chromium.org/11341048/diff/10002/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_change_processor.h:171: void
UpdateTransactionVersion(int64 old_version,
On 2012/11/02 21:02:25, Nicolas Zea wrote:
> old_version -> new_version?

Done.

http://codereview.chromium.org/11341048/diff/10002/sync/internal_api/public/w...
File sync/internal_api/public/write_transaction.h (right):

http://codereview.chromium.org/11341048/diff/10002/sync/internal_api/public/w...
sync/internal_api/public/write_transaction.h:33: WriteTransaction(const
tracked_objects::Location& from_here,
On 2012/11/02 21:02:25, Nicolas Zea wrote:
> Comment about what new_model_version is and why this constructor is separate.

Done.

http://codereview.chromium.org/11341048/diff/10002/sync/internal_api/sync_man...
File sync/internal_api/sync_manager_impl.cc (right):

http://codereview.chromium.org/11341048/diff/10002/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:874: std::vector<int64> *entry_changed) {
On 2012/11/02 21:02:25, Nicolas Zea wrote:
> nit: "std::vector<int64>* entry_changed" (no space before *)

Done.

http://codereview.chromium.org/11341048/diff/10002/sync/internal_api/sync_man...
File sync/internal_api/sync_manager_impl.h (right):

http://codereview.chromium.org/11341048/diff/10002/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.h:332: // Each element of this array is a
store of change records produced by
On 2012/11/02 21:02:25, Nicolas Zea wrote:
> Update comment. No longer an array, and now stored in tree order.

Done.

http://codereview.chromium.org/11341048/diff/10002/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.h:338: typedef std::map<int,
ImmutableChangeRecordList> ChangeRecordMap;
Because in HandleCalculateChangesChangeEventFromSyncer() I have:
for (int i = FIRST_REAL_MODEL_TYPE; i < MODEL_TYPE_COUNT; ++i) {
  ... change_records_[i] ...
}
can't make i to be of type ModelType because then ++i is not allowed. And looks
a little awkward to cast i to ModelType several times.


On 2012/11/02 21:02:25, Nicolas Zea wrote:
> why not have ModelType as the key?

http://codereview.chromium.org/11341048/diff/10002/sync/syncable/directory_ch...
File sync/syncable/directory_change_delegate.h (right):

http://codereview.chromium.org/11341048/diff/10002/sync/syncable/directory_ch...
sync/syncable/directory_change_delegate.h:30: std::vector<int64> *entry_changed)
= 0;
On 2012/11/02 21:02:25, Nicolas Zea wrote:
> nit: fix * to be before space (here and below)

Done. Haven't completely got off the style in my old team.

http://codereview.chromium.org/11341048/diff/10002/sync/syncable/write_transa...
File sync/syncable/write_transaction.cc (right):

http://codereview.chromium.org/11341048/diff/10002/sync/syncable/write_transa...
sync/syncable/write_transaction.cc:18: : BaseTransaction(location,
"WriteTransaction", writer, directory) {
On 2012/11/02 21:02:25, Nicolas Zea wrote:
> initialize new_model_version_ to NULL

Done.

http://codereview.chromium.org/11341048/diff/10002/sync/syncable/write_transa...
File sync/syncable/write_transaction.h (right):

http://codereview.chromium.org/11341048/diff/10002/sync/syncable/write_transa...
sync/syncable/write_transaction.h:21: // API changes to one model. After the
transaction is destroyed,
On 2012/11/02 21:02:25, Nicolas Zea wrote:
> nit: "After the transaction is destroyed" -> "During destruction, "

Done.

http://codereview.chromium.org/11341048/diff/10002/sync/syncable/write_transa...
sync/syncable/write_transaction.h:54: int64* new_model_version_;
On 2012/11/02 21:02:25, Nicolas Zea wrote:
> Comment that you don't have ownership of this.

Done.

http://codereview.chromium.org/11341048/diff/10002/sync/test/null_directory_c...
File sync/test/null_directory_change_delegate.cc (right):

http://codereview.chromium.org/11341048/diff/10002/sync/test/null_directory_c...
sync/test/null_directory_change_delegate.cc:19: std::vector<int64> *id_changed)
{}
On 2012/11/02 21:02:25, Nicolas Zea wrote:
> here too

Done.

http://codereview.chromium.org/11341048/diff/10002/sync/test/null_directory_c...
File sync/test/null_directory_change_delegate.h (right):

http://codereview.chromium.org/11341048/diff/10002/sync/test/null_directory_c...
sync/test/null_directory_change_delegate.h:25: std::vector<int64> *id_changed)
OVERRIDE;
On 2012/11/02 21:02:25, Nicolas Zea wrote:
> "std::vector<int64>* id_changed"

Done.

[haitaol1, 2012-11-06 22:42:07]

Added test to verify transaction version update of bookmark changes. PTAL

[Nicolas Zea, 2012-11-07 00:20:30]

http://codereview.chromium.org/11341048/diff/18002/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service_bookmark_unittest.cc (right):

http://codereview.chromium.org/11341048/diff/18002/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:1666: 
do children get updated when a parent is modified? Perhaps include a expectation
of that here?

Also, verify that the version does not change if the node itself doesn't change
(for example by setting specifics to the same value)? Or does the version always
change?

http://codereview.chromium.org/11341048/diff/18002/sync/syncable/syncable_uni...
File sync/syncable/syncable_unittest.cc (right):

http://codereview.chromium.org/11341048/diff/18002/sync/syncable/syncable_uni...
sync/syncable/syncable_unittest.cc:1679: update.Put(SPECIFICS, specifics);
remove?

http://codereview.chromium.org/11341048/diff/18002/sync/syncable/syncable_uni...
sync/syncable/syncable_unittest.cc:1707: (i == TRANSACTION_VERSION ? 1 : 0),
hmm, what is this for?

http://codereview.chromium.org/11341048/diff/18002/sync/test/null_directory_c...
File sync/test/null_directory_change_delegate.cc (right):

http://codereview.chromium.org/11341048/diff/18002/sync/test/null_directory_c...
sync/test/null_directory_change_delegate.cc:17:
write_transaction_info.Get().mutations.Get().begin();
fix indents (here and below)

[haitaol1, 2012-11-07 22:20:25]

http://codereview.chromium.org/11341048/diff/18002/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service_bookmark_unittest.cc (right):

http://codereview.chromium.org/11341048/diff/18002/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:1666: 
On 2012/11/07 00:20:31, Nicolas Zea wrote:
> do children get updated when a parent is modified? Perhaps include a
expectation
> of that here?
Parent is not updated but siblings do because of NEXT_ID/PREV_ID under current
implementation, but not with ordinal in the future. Added expectations to verify
versions of other nodes are unchanged with hack for sibling.

> 
> Also, verify that the version does not change if the node itself doesn't
change
> (for example by setting specifics to the same value)? Or does the version
always
> change?
The version always change in test because test uses NullDirectoryChangeDelegate,
which I changed to simply return IDs of mutations always without diffing.

http://codereview.chromium.org/11341048/diff/18002/sync/syncable/syncable_uni...
File sync/syncable/syncable_unittest.cc (right):

http://codereview.chromium.org/11341048/diff/18002/sync/syncable/syncable_uni...
sync/syncable/syncable_unittest.cc:1679: update.Put(SPECIFICS, specifics);
So |update| is left empty on purpose? Why set specifics on |create| but not on
|update|?

On 2012/11/07 00:20:31, Nicolas Zea wrote:
> remove?

http://codereview.chromium.org/11341048/diff/18002/sync/syncable/syncable_uni...
sync/syncable/syncable_unittest.cc:1707: (i == TRANSACTION_VERSION ? 1 : 0),
Because version is automatically incremented, so post entry has bigger version.

On 2012/11/07 00:20:31, Nicolas Zea wrote:
> hmm, what is this for?

http://codereview.chromium.org/11341048/diff/18002/sync/test/null_directory_c...
File sync/test/null_directory_change_delegate.cc (right):

http://codereview.chromium.org/11341048/diff/18002/sync/test/null_directory_c...
sync/test/null_directory_change_delegate.cc:17:
write_transaction_info.Get().mutations.Get().begin();
On 2012/11/07 00:20:31, Nicolas Zea wrote:
> fix indents (here and below)

Done.

[tim (not reviewing), 2012-11-08 17:38:49]

https://codereview.chromium.org/11341048/diff/15005/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/bookmark_change_processor.cc (right):

https://codereview.chromium.org/11341048/diff/15005/chrome/browser/sync/glue/...
chrome/browser/sync/glue/bookmark_change_processor.cc:324: int64 new_version =
-1;
This is used in a *lot*of places.  I think it'd be better and easier to read to
use a const (kInvalidTransactionVersion, or something).

https://codereview.chromium.org/11341048/diff/15005/chrome/browser/sync/glue/...
chrome/browser/sync/glue/bookmark_change_processor.cc:646: void
BookmarkChangeProcessor::UpdateTransactionVersion(
You could re-use this function from BookmarkModelAssociator if it were made
static, it doesn't appear to rely on any instance state.

https://codereview.chromium.org/11341048/diff/15005/chrome/browser/sync/glue/...
chrome/browser/sync/glue/bookmark_change_processor.cc:653: for (uint32 i = 0; i
< nodes.size(); ++i) {
prefer size_t to uint32 particularly when using things like vector

https://codereview.chromium.org/11341048/diff/15005/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/bookmark_model_associator.cc (right):

https://codereview.chromium.org/11341048/diff/15005/chrome/browser/sync/glue/...
chrome/browser/sync/glue/bookmark_model_associator.cc:559: for (uint32 i = 0; i
< bnodes.size(); ++i) {
Prefer size_t, although I think this could all be shared with the code in
BookmarkChangeProcessor

https://codereview.chromium.org/11341048/diff/15005/sync/internal_api/public/...
File sync/internal_api/public/write_transaction.h (right):

https://codereview.chromium.org/11341048/diff/15005/sync/internal_api/public/...
sync/internal_api/public/write_transaction.h:34: // the transaction, or -1 if
not after transaction is closed. This
kInvalidTransactionVersion here too

https://codereview.chromium.org/11341048/diff/15005/sync/internal_api/sync_ma...
File sync/internal_api/sync_manager_impl.cc (right):

https://codereview.chromium.org/11341048/diff/15005/sync/internal_api/sync_ma...
sync/internal_api/sync_manager_impl.cc:916: for (uint32 j = 0; j <
change_records_[i].Get().size(); ++j)
prefer size_t

https://codereview.chromium.org/11341048/diff/15005/sync/internal_api/sync_ma...
File sync/internal_api/sync_manager_impl.h (right):

https://codereview.chromium.org/11341048/diff/15005/sync/internal_api/sync_ma...
sync/internal_api/sync_manager_impl.h:167: std::vector<int64>* entry_changed)
OVERRIDE;
entries_changed is more accurate, I think?

https://codereview.chromium.org/11341048/diff/15005/sync/syncable/write_trans...
File sync/syncable/write_transaction.h (right):

https://codereview.chromium.org/11341048/diff/15005/sync/syncable/write_trans...
sync/syncable/write_transaction.h:22: // the new model version if model is
changed, or -1 if model is not changed.
Note that we don't technically scope a write transaction to a specific data
type, see crbug.com/36049.  If you're doing all the work to track which types
were seen, though, we're getting closer to being able to pass that in as well. 
What are the cases where you have multiple real model types 'seen', as per your
calculations in the cc file?

https://codereview.chromium.org/11341048/diff/15005/sync/syncable/write_trans...
sync/syncable/write_transaction.h:53: // Not owned.
Can you explain more what this is for?  Why is it called model version vs
transaction version (or just version)?

[Nicolas Zea, 2012-11-08 19:05:08]

http://codereview.chromium.org/11341048/diff/18002/sync/syncable/syncable_uni...
File sync/syncable/syncable_unittest.cc (right):

http://codereview.chromium.org/11341048/diff/18002/sync/syncable/syncable_uni...
sync/syncable/syncable_unittest.cc:1679: update.Put(SPECIFICS, specifics);
On 2012/11/07 22:20:25, haitaol1 wrote:
> So |update| is left empty on purpose? Why set specifics on |create| but not on
> |update|?
> 
> On 2012/11/07 00:20:31, Nicolas Zea wrote:
> > remove?
> 

Woops, sorry, I misread this to be a duplicate of the line above. Disregard.

http://codereview.chromium.org/11341048/diff/18002/sync/syncable/syncable_uni...
sync/syncable/syncable_unittest.cc:1707: (i == TRANSACTION_VERSION ? 1 : 0),
On 2012/11/07 22:20:25, haitaol1 wrote:
> Because version is automatically incremented, so post entry has bigger
version.
> 
> On 2012/11/07 00:20:31, Nicolas Zea wrote:
> > hmm, what is this for?
> 

Ah, I see.

http://codereview.chromium.org/11341048/diff/15005/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service_bookmark_unittest.cc (right):

http://codereview.chromium.org/11341048/diff/15005/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:1613:
BookmarkNodeVersionMap *node_versions) {
fix asterisk

http://codereview.chromium.org/11341048/diff/15005/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:1682:
model_->Remove(bookmark_bar, 0);
Is removing a child expected to update the parent version?

Also, I'm a bit surprised this works. I wouldn't have expected the api to let
you remove a permanent node. That's likely to change, so it'd probably be better
to remove a different node.

http://codereview.chromium.org/11341048/diff/15005/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:1706:
EXPECT_EQ(initial_versions[model_->root_node()->id()] + 2,
why has the root node version gone up here? Does modifying a child updated the
parent version?

[haitaol1, 2012-11-08 19:06:53]

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

http://codereview.chromium.org/11341048/diff/15005/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_change_processor.cc:324: int64 new_version =
-1;
On 2012/11/08 17:38:49, timsteele wrote:
> This is used in a *lot*of places.  I think it'd be better and easier to read
to
> use a const (kInvalidTransactionVersion, or something).

Done.

http://codereview.chromium.org/11341048/diff/15005/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_change_processor.cc:646: void
BookmarkChangeProcessor::UpdateTransactionVersion(
On 2012/11/08 17:38:49, timsteele wrote:
> You could re-use this function from BookmarkModelAssociator if it were made
> static, it doesn't appear to rely on any instance state.

Done.

http://codereview.chromium.org/11341048/diff/15005/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_change_processor.cc:653: for (uint32 i = 0; i
< nodes.size(); ++i) {
On 2012/11/08 17:38:49, timsteele wrote:
> prefer size_t to uint32 particularly when using things like vector

Done.

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

http://codereview.chromium.org/11341048/diff/15005/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_model_associator.cc:559: for (uint32 i = 0; i
< bnodes.size(); ++i) {
On 2012/11/08 17:38:49, timsteele wrote:
> Prefer size_t, although I think this could all be shared with the code in
> BookmarkChangeProcessor

Done.

http://codereview.chromium.org/11341048/diff/15005/sync/internal_api/public/w...
File sync/internal_api/public/write_transaction.h (right):

http://codereview.chromium.org/11341048/diff/15005/sync/internal_api/public/w...
sync/internal_api/public/write_transaction.h:34: // the transaction, or -1 if
not after transaction is closed. This
On 2012/11/08 17:38:49, timsteele wrote:
> kInvalidTransactionVersion here too

Done.

http://codereview.chromium.org/11341048/diff/15005/sync/internal_api/sync_man...
File sync/internal_api/sync_manager_impl.cc (right):

http://codereview.chromium.org/11341048/diff/15005/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:916: for (uint32 j = 0; j <
change_records_[i].Get().size(); ++j)
On 2012/11/08 17:38:49, timsteele wrote:
> prefer size_t

Done.

http://codereview.chromium.org/11341048/diff/15005/sync/internal_api/sync_man...
File sync/internal_api/sync_manager_impl.h (right):

http://codereview.chromium.org/11341048/diff/15005/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.h:167: std::vector<int64>* entry_changed)
OVERRIDE;
On 2012/11/08 17:38:49, timsteele wrote:
> entries_changed is more accurate, I think?

Done.

http://codereview.chromium.org/11341048/diff/15005/sync/syncable/write_transa...
File sync/syncable/write_transaction.h (right):

http://codereview.chromium.org/11341048/diff/15005/sync/syncable/write_transa...
sync/syncable/write_transaction.h:22: // the new model version if model is
changed, or -1 if model is not changed.
If the transaction is for processing syncer changes, there will be multiple
datat types, I think.

On 2012/11/08 17:38:49, timsteele wrote:
> Note that we don't technically scope a write transaction to a specific data
> type, see crbug.com/36049.  If you're doing all the work to track which types
> were seen, though, we're getting closer to being able to pass that in as well.

> What are the cases where you have multiple real model types 'seen', as per
your
> calculations in the cc file?

http://codereview.chromium.org/11341048/diff/15005/sync/syncable/write_transa...
sync/syncable/write_transaction.h:53: // Not owned.
On 2012/11/08 17:38:49, timsteele wrote:
> Can you explain more what this is for?  Why is it called model version vs
> transaction version (or just version)?

Done. transaction_version seems better than model version since the version is
set on changed nodes besides model.

[haitaol1, 2012-11-08 19:18:47]

http://codereview.chromium.org/11341048/diff/15005/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service_bookmark_unittest.cc (right):

http://codereview.chromium.org/11341048/diff/15005/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:1613:
BookmarkNodeVersionMap *node_versions) {
On 2012/11/08 19:05:08, Nicolas Zea wrote:
> fix asterisk

Done.

http://codereview.chromium.org/11341048/diff/15005/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:1682:
model_->Remove(bookmark_bar, 0);
On 2012/11/08 19:05:08, Nicolas Zea wrote:
> Is removing a child expected to update the parent version?
> 
No. The relationship is one-way from child to parent in directory.

> Also, I'm a bit surprised this works. I wouldn't have expected the api to let
> you remove a permanent node. That's likely to change, so it'd probably be
better
> to remove a different node.
It's the first child of bookmark bar being removed.

http://codereview.chromium.org/11341048/diff/15005/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:1706:
EXPECT_EQ(initial_versions[model_->root_node()->id()] + 2,
On 2012/11/08 19:05:08, Nicolas Zea wrote:
> why has the root node version gone up here? Does modifying a child updated the
> parent version?
The version on root node is for the whole model. So it's incremented when a
change happens.

[Nicolas Zea, 2012-11-08 19:26:00]

Tim's comments aside, LGTM

http://codereview.chromium.org/11341048/diff/15005/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service_bookmark_unittest.cc (right):

http://codereview.chromium.org/11341048/diff/15005/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:1706:
EXPECT_EQ(initial_versions[model_->root_node()->id()] + 2,
On 2012/11/08 19:18:47, haitaol1 wrote:
> On 2012/11/08 19:05:08, Nicolas Zea wrote:
> > why has the root node version gone up here? Does modifying a child updated
the
> > parent version?
> The version on root node is for the whole model. So it's incremented when a
> change happens.

Ah, I see. Maybe modify the test comment to make that clearer?

[haitaol1, 2012-11-08 19:44:25]

http://codereview.chromium.org/11341048/diff/15005/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service_bookmark_unittest.cc (right):

http://codereview.chromium.org/11341048/diff/15005/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:1706:
EXPECT_EQ(initial_versions[model_->root_node()->id()] + 2,
On 2012/11/08 19:26:00, Nicolas Zea wrote:
> On 2012/11/08 19:18:47, haitaol1 wrote:
> > On 2012/11/08 19:05:08, Nicolas Zea wrote:
> > > why has the root node version gone up here? Does modifying a child updated
> the
> > > parent version?
> > The version on root node is for the whole model. So it's incremented when a
> > change happens.
> 
> Ah, I see. Maybe modify the test comment to make that clearer?

Done.

[tim (not reviewing), 2012-11-08 19:47:41]

LGTM

[commit-bot: I haz the power, 2012-11-10 02:02:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haitaol@chromium.org/11341048/24001

[commit-bot: I haz the power, 2012-11-10 05:20:12]

Change committed as 167061