[Sync] Add support for loading, updating and querying delete journals in

[Sync] Add support for loading, updating and querying delete journals in 
Directory.

Delete journals keeps deleted metas until the persistence of the deletes in
native model is confirmed. When an entry is deleted in sync model, a copy is 
added to delete journals in memory and saved in database later. Next time when
the client restarts, if some native data doesn't match with sync data but
matches with a delete journal, it's safe to assume that it's because native
delete was not persisted and the native data should be deleted. This helps
prevent back-from-dead problem due to native model and sync model get
out-of-sync.

BUG=121928
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175248

[tim (not reviewing), 2012-12-13 02:48:10]

Couple comments.  Also, were you planning on adding the DeleteJournal wrapper to
this CL or a separate one?

https://codereview.chromium.org/11441026/diff/2001/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/11441026/diff/2001/sync/syncable/directory.cc...
sync/syncable/directory.cc:251: const EntryKernel* entry) {
How come this can't be a ref?

https://codereview.chromium.org/11441026/diff/2001/sync/syncable/directory.h
File sync/syncable/directory.h (right):

https://codereview.chromium.org/11441026/diff/2001/sync/syncable/directory.h#...
sync/syncable/directory.h:124: typedef std::set<const EntryKernel*,
EntryKernelLessByMetaHandle>
Do we really need pointers? If so, I'd prefer to use base/memory/linked_ptr for
this. There was a thread on chromium-dev about discouraging linked_ptr with
vectors, and I just re-read it, and I still come away with the belief that
linked_ptr is preferable to a raw pointer (and using STLDeleteElements), and a
main reason is it would eliminate the need for the careful delete *it code you
have to write today. (And there's no ScopedSet style class in existence yet as
far as I know.)

https://codereview.chromium.org/11441026/diff/2001/sync/syncable/directory.h#...
sync/syncable/directory.h:664: static bool IsDeleteJournalEnabled(ModelType
type);
Do you anticipate needing to control this result from a test some day, or no?

https://codereview.chromium.org/11441026/diff/2001/sync/syncable/mutable_entr...
File sync/syncable/mutable_entry.cc (right):

https://codereview.chromium.org/11441026/diff/2001/sync/syncable/mutable_entr...
sync/syncable/mutable_entry.cc:281: if (field == SERVER_IS_DEL)
This can't be IS_DEL (and hence live in PutIsDel) I presume... can you explain /
comment why?

[haitaol1, 2012-12-13 21:34:21]

Added delete_journal.h/cc in 11533008 (delete dead bookmarks using journal)

https://codereview.chromium.org/11441026/diff/2001/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/11441026/diff/2001/sync/syncable/directory.cc...
sync/syncable/directory.cc:251: const EntryKernel* entry) {
On 2012/12/13 02:48:10, timsteele wrote:
> How come this can't be a ref?

Done.

https://codereview.chromium.org/11441026/diff/2001/sync/syncable/directory.h
File sync/syncable/directory.h (right):

https://codereview.chromium.org/11441026/diff/2001/sync/syncable/directory.h#...
sync/syncable/directory.h:124: typedef std::set<const EntryKernel*,
EntryKernelLessByMetaHandle>
Changed to pointer because it's more efficient to move delete journal from entry
kernel to snapshot to save. If using linked_ptr to store delete journal in entry
kernel, other indexes are converted to linked_ptr, too because they share same
template. The overhead of linked_ptr is probably not worth the cost then.

On 2012/12/13 02:48:10, timsteele wrote:
> Do we really need pointers? If so, I'd prefer to use base/memory/linked_ptr
for
> this. There was a thread on chromium-dev about discouraging linked_ptr with
> vectors, and I just re-read it, and I still come away with the belief that
> linked_ptr is preferable to a raw pointer (and using STLDeleteElements), and a
> main reason is it would eliminate the need for the careful delete *it code you
> have to write today. (And there's no ScopedSet style class in existence yet as
> far as I know.)

https://codereview.chromium.org/11441026/diff/2001/sync/syncable/directory.h#...
sync/syncable/directory.h:664: static bool IsDeleteJournalEnabled(ModelType
type);
No. I don't think that's necessary.

On 2012/12/13 02:48:10, timsteele wrote:
> Do you anticipate needing to control this result from a test some day, or no?

https://codereview.chromium.org/11441026/diff/2001/sync/syncable/mutable_entr...
File sync/syncable/mutable_entry.cc (right):

https://codereview.chromium.org/11441026/diff/2001/sync/syncable/mutable_entr...
sync/syncable/mutable_entry.cc:281: if (field == SERVER_IS_DEL)
I think it's more accurate to use SERVER_IS_DEL to decide the status of an
entry. There are several early returns on the path from where SERVER_IS_DEL's
updated to where IS_DEL's updated. I'm concerned that we may not get update of
IS_DEL and leave or miss entries in delete journal accidentally.

On 2012/12/13 02:48:10, timsteele wrote:
> This can't be IS_DEL (and hence live in PutIsDel) I presume... can you explain
/
> comment why?

[tim (not reviewing), 2012-12-13 23:41:30]

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.cc...
sync/syncable/directory.cc:249: void
Directory::UpdateDeleteJournals(BaseTransaction* trans,
UpdateDeleteJournals sounds a bit more general than this is, since we only want
to do so / use this method when the server tells us an item is deleted. The code
assumes that is the case, and also, we should be on the sync thread when this
happens due to working on the SERVER_ namespace.
UpdateDeleteJournalForServerDelete?

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.cc...
sync/syncable/directory.cc:254:
IsDeleteJournalEnabled(entry.GetServerModelType()))) {
Do you expect local specifics and server specifics to be different when this is
called?  Can one be an assertion? It's a bit intimidating to be unable to assume
anything / any preconditions about the state that |entry| is in at the time this
is called.

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.cc...
sync/syncable/directory.cc:260:
kernel_->delete_journals_->find(const_cast<EntryKernel*>(&entry));
Why remove the const-ness?  Everything about delete_journals and your comparison
routine seem to take const EntryKernel*s.

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.cc...
sync/syncable/directory.cc:271: // * An entry was actually deleted and
undeleted: was_deleted = true.
Where would this undelete have come from?  The server?

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.cc...
sync/syncable/directory.cc:272: // * A data type was broken in last sync session
and all its entries
Define broken?

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.cc...
sync/syncable/directory.cc:646: bool
Directory::PurgeEntriesWithTypeIn(ModelTypeSet types) {
I don't see any changes here for the delete journals, meaning server-deletes are
sticky across disable / re-enable of a datatype. I'm wondering if there are any
unintended side effects when migration or chip-machine fixup occurs...

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.h
File sync/syncable/directory.h (right):

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.h#...
sync/syncable/directory.h:341: void PurgeDeleteJournals(BaseTransaction* trans,
Where is this called?

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory_ba...
File sync/syncable/directory_backing_store.cc (right):

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory_ba...
sync/syncable/directory_backing_store.cc:211:
snapshot.delete_journals_to_purge.empty() && !save_info)
Braces around if body

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory_ba...
File sync/syncable/directory_backing_store.h (right):

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory_ba...
sync/syncable/directory_backing_store.h:116: bool DeleteEntries(bool
meta_delete, const MetahandleSet& handles);
Can you make this an enum with DELETE_FROM_METAS and DELETE_FROM_JOURNAL

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/mutable_entr...
File sync/syncable/mutable_entry.cc (right):

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/mutable_entr...
sync/syncable/mutable_entry.cc:282:
dir()->UpdateDeleteJournals(write_transaction(), old_value, *kernel_);
One thing is that as-written, we could not use UpdateDeleteJournals in the event
of a local delete - first because the SERVER_IS_DEL field won't be set, but
second we shouldn't read/write things in the "SERVER_" namespace unless we know
we're on the sync thread.

[haitaol1, 2012-12-14 19:22:38]

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.cc...
sync/syncable/directory.cc:249: void
Directory::UpdateDeleteJournals(BaseTransaction* trans,
On 2012/12/13 23:41:30, timsteele wrote:
> UpdateDeleteJournals sounds a bit more general than this is, since we only
want
> to do so / use this method when the server tells us an item is deleted. The
code
> assumes that is the case, and also, we should be on the sync thread when this
> happens due to working on the SERVER_ namespace.
> UpdateDeleteJournalForServerDelete?

Done.

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.cc...
sync/syncable/directory.cc:254:
IsDeleteJournalEnabled(entry.GetServerModelType()))) {
The specifics would be different because this is called during processing update
before server specs being copied to local specs. It should be sufficient to
check only server type in reality. But not all tests are set up like real and it
seems harmless to check both.

On 2012/12/13 23:41:30, timsteele wrote:
> Do you expect local specifics and server specifics to be different when this
is
> called?  Can one be an assertion? It's a bit intimidating to be unable to
assume
> anything / any preconditions about the state that |entry| is in at the time
this
> is called.

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.cc...
sync/syncable/directory.cc:260:
kernel_->delete_journals_->find(const_cast<EntryKernel*>(&entry));
It's because IdIndex stores non-const pointer. Changed to use needle to be
absolutely safe.

On 2012/12/13 23:41:30, timsteele wrote:
> Why remove the const-ness?  Everything about delete_journals and your
comparison
> routine seem to take const EntryKernel*s.

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.cc...
sync/syncable/directory.cc:271: // * An entry was actually deleted and
undeleted: was_deleted = true.
Yes. Looks like if an entry was deleted on client A but has unsynced data on
client B, B's update sent to server will undelete the entry effectively.


On 2012/12/13 23:41:30, timsteele wrote:
> Where would this undelete have come from?  The server?

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.cc...
sync/syncable/directory.cc:272: // * A data type was broken in last sync session
and all its entries
On 2012/12/13 23:41:30, timsteele wrote:
> Define broken? 

Done.

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.cc...
sync/syncable/directory.cc:646: bool
Directory::PurgeEntriesWithTypeIn(ModelTypeSet types) {
I'm a little ahead of myself here. This is in another cl I'm gonna send out.

On 2012/12/13 23:41:30, timsteele wrote:
> I don't see any changes here for the delete journals, meaning server-deletes
are
> sticky across disable / re-enable of a datatype. I'm wondering if there are
any
> unintended side effects when migration or chip-machine fixup occurs...

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.h
File sync/syncable/directory.h (right):

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.h#...
sync/syncable/directory.h:341: void PurgeDeleteJournals(BaseTransaction* trans,
It's called from delete_journal.cc in the other patch.

On 2012/12/13 23:41:30, timsteele wrote:
> Where is this called?

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory_ba...
File sync/syncable/directory_backing_store.cc (right):

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory_ba...
sync/syncable/directory_backing_store.cc:211:
snapshot.delete_journals_to_purge.empty() && !save_info)
On 2012/12/13 23:41:30, timsteele wrote:
> Braces around if body

Done.

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory_ba...
File sync/syncable/directory_backing_store.h (right):

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory_ba...
sync/syncable/directory_backing_store.h:116: bool DeleteEntries(bool
meta_delete, const MetahandleSet& handles);
On 2012/12/13 23:41:30, timsteele wrote:
> Can you make this an enum with DELETE_FROM_METAS and DELETE_FROM_JOURNAL

Done.

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/mutable_entr...
File sync/syncable/mutable_entry.cc (right):

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/mutable_entr...
sync/syncable/mutable_entry.cc:282:
dir()->UpdateDeleteJournals(write_transaction(), old_value, *kernel_);
Comments added. 

On 2012/12/13 23:41:30, timsteele wrote:
> One thing is that as-written, we could not use UpdateDeleteJournals in the
event
> of a local delete - first because the SERVER_IS_DEL field won't be set, but
> second we shouldn't read/write things in the "SERVER_" namespace unless we
know
> we're on the sync thread.

[tim (not reviewing), 2012-12-14 20:12:38]

Ok, I'm getting the hang of this now!  Thanks for bearing with me ...

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.cc...
sync/syncable/directory.cc:105: STLDeleteElements(&dirty_metas);
Where was this being deleted before?

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.cc...
sync/syncable/directory.cc:271: // * An entry was actually deleted and
undeleted: was_deleted = true.
On 2012/12/14 19:22:38, haitaol1 wrote:
> Yes. Looks like if an entry was deleted on client A but has unsynced data on
> client B, B's update sent to server will undelete the entry effectively.
> 
> 
> On 2012/12/13 23:41:30, timsteele wrote:
> > Where would this undelete have come from?  The server?
> 

So, it was deleted (and possibly applied to the native model), but still remains
in the journal.  Can you comment that?

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.h
File sync/syncable/directory.h (right):

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.h#...
sync/syncable/directory.h:630: MetahandleSet* const delete_journals_to_purge_;
I have one somewhat significant but honest suggestion -- I think we should pull
the journal specific state (these indexes, passive_delete_journal_types_) into a
DeleteJournal class and have Directory own that. You've done a nice job of
keeping access to the member variables limited (mostly to a pair of methods,
Update and Purge) and not only would it be a shame to regress on that by some
day adding rebel code that ignores the contract you put in place, but there's a
lot of complex code in Directory already and I think it'll be really nice to
think about DeleteJournal as something separable from the core Directory work,
code, and tests.

https://codereview.chromium.org/11441026/diff/14002/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/11441026/diff/14002/sync/syncable/directory.c...
sync/syncable/directory.cc:254:
GetModelTypeFromSpecifics(entry.ref(SPECIFICS))))) {
Can you add the comment from your reply in here explaining why we check both?

[haitaol1, 2012-12-14 23:15:36]

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.cc...
sync/syncable/directory.cc:105: STLDeleteElements(&dirty_metas);
It doesn't need to be deleted because it's not a pointer before.

On 2012/12/14 20:12:38, timsteele wrote:
> Where was this being deleted before?

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.cc...
sync/syncable/directory.cc:271: // * An entry was actually deleted and
undeleted: was_deleted = true.
On 2012/12/14 20:12:38, timsteele wrote:
> On 2012/12/14 19:22:38, haitaol1 wrote:
> > Yes. Looks like if an entry was deleted on client A but has unsynced data on
> > client B, B's update sent to server will undelete the entry effectively.
> > 
> > 
> > On 2012/12/13 23:41:30, timsteele wrote:
> > > Where would this undelete have come from?  The server?
> > 
> 
> So, it was deleted (and possibly applied to the native model), but still
remains
> in the journal.  Can you comment that?

Done.

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.h
File sync/syncable/directory.h (right):

https://codereview.chromium.org/11441026/diff/8001/sync/syncable/directory.h#...
sync/syncable/directory.h:630: MetahandleSet* const delete_journals_to_purge_;
Working on it.

On 2012/12/14 20:12:38, timsteele wrote:
> I have one somewhat significant but honest suggestion -- I think we should
pull
> the journal specific state (these indexes, passive_delete_journal_types_) into
a
> DeleteJournal class and have Directory own that. You've done a nice job of
> keeping access to the member variables limited (mostly to a pair of methods,
> Update and Purge) and not only would it be a shame to regress on that by some
> day adding rebel code that ignores the contract you put in place, but there's
a
> lot of complex code in Directory already and I think it'll be really nice to
> think about DeleteJournal as something separable from the core Directory work,
> code, and tests.

https://codereview.chromium.org/11441026/diff/14002/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/11441026/diff/14002/sync/syncable/directory.c...
sync/syncable/directory.cc:254:
GetModelTypeFromSpecifics(entry.ref(SPECIFICS))))) {
On 2012/12/14 20:12:38, timsteele wrote:
> Can you add the comment from your reply in here explaining why we check both?

Done.

[haitaol1, 2012-12-17 21:05:53]

Moved delete journal code to delete_journal.h/cc. PTAL

[tim (not reviewing), 2012-12-21 03:30:40]

Looking good, nice comments in delete_journal.h.

https://codereview.chromium.org/11441026/diff/29001/sync/syncable/delete_jour...
File sync/syncable/delete_journal.cc (right):

https://codereview.chromium.org/11441026/diff/29001/sync/syncable/delete_jour...
sync/syncable/delete_journal.cc:27: // Should be sufficient to check server type
only but check for local
Could you add a unittest covering some of the logic in here?  e.g. that
delete_journals_to_purge contains the right entries after this is run (perhaps
grabbing them via TakeSnapshot)?  An added bonus is unit tests here may also
help catch leaks that creep in over time.

https://codereview.chromium.org/11441026/diff/29001/sync/syncable/delete_jour...
File sync/syncable/delete_journal.h (right):

https://codereview.chromium.org/11441026/diff/29001/sync/syncable/delete_jour...
sync/syncable/delete_journal.h:31: DeleteJournal(JournalIndex& initial_journal);
Single argument constructors should be marked explicit.  Also, non-const& isn't
permitted in Chrome, you should use a pointer, or better yet given the fact that
you are destroying the contents, a scoped_ptr (via .Pass(), if possible)

https://codereview.chromium.org/11441026/diff/29001/sync/syncable/delete_jour...
sync/syncable/delete_journal.h:90: // To protect access to journal index.
Can you add class comments and method comments explaining the expected thread
behavior / access for this class?  What methods block, what is expected to be
called on what thread (or is the entire class essentially callable from any
thread, a monitor, etc.

https://codereview.chromium.org/11441026/diff/29001/sync/syncable/entry_kernel.h
File sync/syncable/entry_kernel.h (right):

https://codereview.chromium.org/11441026/diff/29001/sync/syncable/entry_kerne...
sync/syncable/entry_kernel.h:336: EntryKernelSet;
Hm, did this move from somewhere?  EntryKernelSet already seems to exist...

[haitaol1, 2013-01-03 19:40:49]

https://codereview.chromium.org/11441026/diff/29001/sync/syncable/delete_jour...
File sync/syncable/delete_journal.cc (right):

https://codereview.chromium.org/11441026/diff/29001/sync/syncable/delete_jour...
sync/syncable/delete_journal.cc:27: // Should be sufficient to check server type
only but check for local
Tests are in syncable_unittest.cc

On 2012/12/21 03:30:40, timsteele wrote:
> Could you add a unittest covering some of the logic in here?  e.g. that
> delete_journals_to_purge contains the right entries after this is run (perhaps
> grabbing them via TakeSnapshot)?  An added bonus is unit tests here may also
> help catch leaks that creep in over time.

https://codereview.chromium.org/11441026/diff/29001/sync/syncable/delete_jour...
File sync/syncable/delete_journal.h (right):

https://codereview.chromium.org/11441026/diff/29001/sync/syncable/delete_jour...
sync/syncable/delete_journal.h:31: DeleteJournal(JournalIndex& initial_journal);
scoped_ptr is probably an overkill. Changed to pointer.

On 2012/12/21 03:30:40, timsteele wrote:
> Single argument constructors should be marked explicit.  Also, non-const&
isn't
> permitted in Chrome, you should use a pointer, or better yet given the fact
that
> you are destroying the contents, a scoped_ptr (via .Pass(), if possible)

https://codereview.chromium.org/11441026/diff/29001/sync/syncable/delete_jour...
sync/syncable/delete_journal.h:90: // To protect access to journal index.
On 2012/12/21 03:30:40, timsteele wrote:
> Can you add class comments and method comments explaining the expected thread
> behavior / access for this class?  What methods block, what is expected to be
> called on what thread (or is the entire class essentially callable from any
> thread, a monitor, etc.

Done.

https://codereview.chromium.org/11441026/diff/29001/sync/syncable/entry_kernel.h
File sync/syncable/entry_kernel.h (right):

https://codereview.chromium.org/11441026/diff/29001/sync/syncable/entry_kerne...
sync/syncable/entry_kernel.h:336: EntryKernelSet;
Yes, moved here from directory.h.

On 2012/12/21 03:30:40, timsteele wrote:
> Hm, did this move from somewhere?  EntryKernelSet already seems to exist...

[tim (not reviewing), 2013-01-04 15:35:46]

Few more comments.

https://codereview.chromium.org/11441026/diff/35001/sync/syncable/delete_jour...
File sync/syncable/delete_journal.cc (right):

https://codereview.chromium.org/11441026/diff/35001/sync/syncable/delete_jour...
sync/syncable/delete_journal.cc:23: return delete_journals_.size();
Shouldn't you be using journal_lock_ here?

https://codereview.chromium.org/11441026/diff/35001/sync/syncable/delete_jour...
sync/syncable/delete_journal.cc:36: base::AutoLock lock(journal_lock_);
It looks like we're trying to acquire a lock while holding a lock
(BaseTransaction* parameter)... in fact, this is true of the other methods (as
far as I can tell) as well, it's just more explicit here due to the parameter
being passed in.  If we can avoid this it would do wonders for simplifying the
code (see comment in .h).

https://codereview.chromium.org/11441026/diff/35001/sync/syncable/delete_jour...
sync/syncable/delete_journal.cc:117: void
DeleteJournal::AddJournals(BaseTransaction* trans,
I think AddJournalBatch would be a better / more specialized sounding name
(since this isn't the common way journals are added).

https://codereview.chromium.org/11441026/diff/35001/sync/syncable/delete_jour...
File sync/syncable/delete_journal.h (right):

https://codereview.chromium.org/11441026/diff/35001/sync/syncable/delete_jour...
sync/syncable/delete_journal.h:95: base::Lock journal_lock_;
I read through this CL, CL 11533008, and 11578017, and I haven't managed to find
a case where we invoke any method on DeleteJournal without already being inside
a BaseTransaction.  The pattern we use elsewhere to avoid having to add
yet-another-lock is to require a valid BaseTransaction parameter on all methods
you would otherwise synchronize internally.  Is there any way this is possible?
It would really help simplicity (less locks to worry about!) and more
conventional for syncable/ code.

[haitaol1, 2013-01-04 18:49:20]

https://codereview.chromium.org/11441026/diff/35001/sync/syncable/delete_jour...
File sync/syncable/delete_journal.cc (right):

https://codereview.chromium.org/11441026/diff/35001/sync/syncable/delete_jour...
sync/syncable/delete_journal.cc:23: return delete_journals_.size();
Require a transaction param now.

On 2013/01/04 15:35:46, timsteele wrote:
> Shouldn't you be using journal_lock_ here?

https://codereview.chromium.org/11441026/diff/35001/sync/syncable/delete_jour...
sync/syncable/delete_journal.cc:36: base::AutoLock lock(journal_lock_);
On 2013/01/04 15:35:46, timsteele wrote:
> It looks like we're trying to acquire a lock while holding a lock
> (BaseTransaction* parameter)... in fact, this is true of the other methods (as
> far as I can tell) as well, it's just more explicit here due to the parameter
> being passed in.  If we can avoid this it would do wonders for simplifying the
> code (see comment in .h). 

Done.

https://codereview.chromium.org/11441026/diff/35001/sync/syncable/delete_jour...
sync/syncable/delete_journal.cc:117: void
DeleteJournal::AddJournals(BaseTransaction* trans,
On 2013/01/04 15:35:46, timsteele wrote:
> I think AddJournalBatch would be a better / more specialized sounding name
> (since this isn't the common way journals are added).

Done.

https://codereview.chromium.org/11441026/diff/35001/sync/syncable/delete_jour...
File sync/syncable/delete_journal.h (right):

https://codereview.chromium.org/11441026/diff/35001/sync/syncable/delete_jour...
sync/syncable/delete_journal.h:95: base::Lock journal_lock_;
Good point. Added transaction paramenter to GetDeleteJournalSize() and
TakeSnapshotAndClear()

On 2013/01/04 15:35:46, timsteele wrote:
> I read through this CL, CL 11533008, and 11578017, and I haven't managed to
find
> a case where we invoke any method on DeleteJournal without already being
inside
> a BaseTransaction.  The pattern we use elsewhere to avoid having to add
> yet-another-lock is to require a valid BaseTransaction parameter on all
methods
> you would otherwise synchronize internally.  Is there any way this is
possible?
> It would really help simplicity (less locks to worry about!) and more
> conventional for syncable/ code.

[tim (not reviewing), 2013-01-04 18:54:14]

After addressing comments below, this LGTM!

https://codereview.chromium.org/11441026/diff/39001/sync/syncable/delete_jour...
File sync/syncable/delete_journal.cc (right):

https://codereview.chromium.org/11441026/diff/39001/sync/syncable/delete_jour...
sync/syncable/delete_journal.cc:113: void
DeleteJournal::AddJournalBatch(BaseTransaction* trans,
Add DCHECK(trans) in these methods that don't explicitly dereference / use
|trans|.

https://codereview.chromium.org/11441026/diff/39001/sync/syncable/delete_jour...
File sync/syncable/delete_journal.h (right):

https://codereview.chromium.org/11441026/diff/39001/sync/syncable/delete_jour...
sync/syncable/delete_journal.h:25: // DeleteJournal is thread-safe and can be
accessed on any thread.
Heh, sorry for the back-and-forth, but update comment to note methods on this
class should be invoked only while holding a BaseTransaction, and that each
method requires a non-null trans* for this reason.

https://codereview.chromium.org/11441026/diff/39001/sync/syncable/delete_jour...
sync/syncable/delete_journal.h:35: // For testing only
nit - end comment with period.

https://codereview.chromium.org/11441026/diff/39001/sync/syncable/delete_jour...
sync/syncable/delete_journal.h:93: ModelTypeSet passive_delete_journal_types_;
style nit - add DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/11441026/diff/39001/sync/syncable/directory_b...
File sync/syncable/directory_backing_store_unittest.cc (right):

https://codereview.chromium.org/11441026/diff/39001/sync/syncable/directory_b...
sync/syncable/directory_backing_store_unittest.cc:3258: JournalIndex 
delete_journals;;
nit - is there an extra space here?

[haitaol1, 2013-01-04 19:06:14]

https://codereview.chromium.org/11441026/diff/39001/sync/syncable/delete_jour...
File sync/syncable/delete_journal.cc (right):

https://codereview.chromium.org/11441026/diff/39001/sync/syncable/delete_jour...
sync/syncable/delete_journal.cc:113: void
DeleteJournal::AddJournalBatch(BaseTransaction* trans,
On 2013/01/04 18:54:14, timsteele wrote:
> Add DCHECK(trans) in these methods that don't explicitly dereference / use
> |trans|.

Done.

https://codereview.chromium.org/11441026/diff/39001/sync/syncable/delete_jour...
File sync/syncable/delete_journal.h (right):

https://codereview.chromium.org/11441026/diff/39001/sync/syncable/delete_jour...
sync/syncable/delete_journal.h:25: // DeleteJournal is thread-safe and can be
accessed on any thread.
On 2013/01/04 18:54:14, timsteele wrote:
> Heh, sorry for the back-and-forth, but update comment to note methods on this
> class should be invoked only while holding a BaseTransaction, and that each
> method requires a non-null trans* for this reason.

Done.

https://codereview.chromium.org/11441026/diff/39001/sync/syncable/delete_jour...
sync/syncable/delete_journal.h:35: // For testing only
On 2013/01/04 18:54:14, timsteele wrote:
> nit - end comment with period.

Done.

https://codereview.chromium.org/11441026/diff/39001/sync/syncable/delete_jour...
sync/syncable/delete_journal.h:93: ModelTypeSet passive_delete_journal_types_;
On 2013/01/04 18:54:14, timsteele wrote:
> style nit - add DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/11441026/diff/39001/sync/syncable/directory_b...
File sync/syncable/directory_backing_store_unittest.cc (right):

https://codereview.chromium.org/11441026/diff/39001/sync/syncable/directory_b...
sync/syncable/directory_backing_store_unittest.cc:3258: JournalIndex 
delete_journals;;
On 2013/01/04 18:54:14, timsteele wrote:
> nit - is there an extra space here?

Done.

[commit-bot: I haz the power, 2013-01-04 19:38:48]

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

[commit-bot: I haz the power, 2013-01-04 19:55:29]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-04 20:02:00]

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

[commit-bot: I haz the power, 2013-01-04 20:16:26]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-04 21:29:53]

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

[commit-bot: I haz the power, 2013-01-05 01:15:53]

Change committed as 175248