Use delete journal to remove bookmarks that are already deleted in sync model

Use delete journal to remove bookmarks that are already deleted in sync model
before assocating bookmark model and sync model.

 

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

[haitaol1, 2012-12-11 18:25:41]

Tim: I'm using BaseTransaction as bridge between Directory and model associator
to pass delete journals as SyncData. This unfortunately adds dependency from
internal_api/public to sync/api. Any suggestions for better place?

[tim (not reviewing), 2012-12-11 19:58:33]

Taking a look at the two reviews, just trying to understand the full picture at
the moment.

https://codereview.chromium.org/11533008/diff/1/chrome/browser/sync/glue/book...
File chrome/browser/sync/glue/bookmark_model_associator.cc (right):

https://codereview.chromium.org/11533008/diff/1/chrome/browser/sync/glue/book...
chrome/browser/sync/glue/bookmark_model_associator.cc:561:
trans->GetDeletedSyncData(syncer::BOOKMARKS, &deleted_sync_bookmarks);
How can we make it so that this data is *sent* to the glue layer
(ModelAssociators, SyncableService) by the sync engine only when needed? Versus
having to add an instruction to Building a Sync Data type of the form "Don't
forget to look up the delete journal"..  It would be better if this information
was part of / embedded in GetAllSyncData (for example), or even sent via a
separate callback (though I prefer the former).

[haitaol1, 2012-12-12 17:40:00]

Delete journal is only maintained for the data types that support it. I added
IsDeleteJournalEnabled() on Directory to enable for bookmarks only currently.
When enabling delete journal for a data type, we need to be careful to purge
unused delete journal so that it won't keep growing. With sync api, delete
journal will be sent together with initial sync data. For disabled data type,
the journal would be empty.

On 2012/12/11 19:58:33, timsteele wrote:
> Taking a look at the two reviews, just trying to understand the full picture
at
> the moment.
> 
>
https://codereview.chromium.org/11533008/diff/1/chrome/browser/sync/glue/book...
> File chrome/browser/sync/glue/bookmark_model_associator.cc (right):
> 
>
https://codereview.chromium.org/11533008/diff/1/chrome/browser/sync/glue/book...
> chrome/browser/sync/glue/bookmark_model_associator.cc:561:
> trans->GetDeletedSyncData(syncer::BOOKMARKS, &deleted_sync_bookmarks);
> How can we make it so that this data is *sent* to the glue layer
> (ModelAssociators, SyncableService) by the sync engine only when needed?
Versus
> having to add an instruction to Building a Sync Data type of the form "Don't
> forget to look up the delete journal"..  It would be better if this
information
> was part of / embedded in GetAllSyncData (for example), or even sent via a
> separate callback (though I prefer the former).

[haitaol1, 2013-01-08 17:45:41]

ping

[tim (not reviewing), 2013-01-10 19:19:31]

On 2013/01/08 17:45:41, haitaol1 wrote:
> ping

DeleteJournal is currently in sync/syncable, right?  Do we really need a
dependency on the whole class?  I'd rather keep it hidden, since we don't want
other types to use DeleteJournal directly (internal_api will pass things to the
right place).  I'd prefer keeping it in syncable and adding a bookmarks only
bridge, which would clearly need to be removed once we transition bookmarks to
sync/api.

[haitaol1, 2013-01-10 22:00:12]

But even with sync api, a bridge is still needed to return journal from syncable
space to browser space, I think.

On 2013/01/10 19:19:31, timsteele wrote:
> On 2013/01/08 17:45:41, haitaol1 wrote:
> > ping
> 
> DeleteJournal is currently in sync/syncable, right?  Do we really need a
> dependency on the whole class?  I'd rather keep it hidden, since we don't want
> other types to use DeleteJournal directly (internal_api will pass things to
the
> right place).  I'd prefer keeping it in syncable and adding a bookmarks only
> bridge, which would clearly need to be removed once we transition bookmarks to
> sync/api.

[tim (not reviewing), 2013-01-10 22:02:20]

Hmm... can't we construct a SyncChangeList?


On Thu, Jan 10, 2013 at 2:00 PM, <haitaol@chromium.org> wrote:

> But even with sync api, a bridge is still needed to return journal from
> syncable
> space to browser space, I think.
>
>
> On 2013/01/10 19:19:31, timsteele wrote:
>
>> On 2013/01/08 17:45:41, haitaol1 wrote:
>> > ping
>>
>
>  DeleteJournal is currently in sync/syncable, right?  Do we really need a
>> dependency on the whole class?  I'd rather keep it hidden, since we don't
>> want
>> other types to use DeleteJournal directly (internal_api will pass things
>> to
>>
> the
>
>> right place).  I'd prefer keeping it in syncable and adding a bookmarks
>> only
>> bridge, which would clearly need to be removed once we transition
>> bookmarks to
>> sync/api.
>>
>
>
>
>
https://codereview.chromium.**org/11533008/<https://codereview.chromium.org/1...
>

[haitaol1, 2013-01-10 23:16:25]

We can pass delete journal using SyncDataList after SyncData supports bookmarks.
But we still need to convert delete journal from kernel entry to SyncData
somewhere. Data in sync model uses Node class for that but it seems too complex
to wrap delete journal in Node because delete journals are basically independent
entries with no hierarchy. You are right that current GetDeleteJournals() is a
special case for bookmarks. I have added bookmarks to the name and todo to
replace with generic version later.


On 2013/01/10 22:02:20, timsteele wrote:
> Hmm... can't we construct a SyncChangeList?
> 
> 
> On Thu, Jan 10, 2013 at 2:00 PM, <mailto:haitaol@chromium.org> wrote:
> 
> > But even with sync api, a bridge is still needed to return journal from
> > syncable
> > space to browser space, I think.
> >
> >
> > On 2013/01/10 19:19:31, timsteele wrote:
> >
> >> On 2013/01/08 17:45:41, haitaol1 wrote:
> >> > ping
> >>
> >
> >  DeleteJournal is currently in sync/syncable, right?  Do we really need a
> >> dependency on the whole class?  I'd rather keep it hidden, since we don't
> >> want
> >> other types to use DeleteJournal directly (internal_api will pass things
> >> to
> >>
> > the
> >
> >> right place).  I'd prefer keeping it in syncable and adding a bookmarks
> >> only
> >> bridge, which would clearly need to be removed once we transition
> >> bookmarks to
> >> sync/api.
> >>
> >
> >
> >
> >
>
https://codereview.chromium.**org/11533008/%3Chttps://codereview.chromium.org...>
> >

[tim (not reviewing), 2013-01-14 23:32:48]

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

https://codereview.chromium.org/11533008/diff/24002/chrome/browser/sync/glue/...
chrome/browser/sync/glue/bookmark_model_associator.cc:88: // Finds best matching
node for the given sync node.
Update comment.

https://codereview.chromium.org/11533008/diff/24002/chrome/browser/sync/glue/...
chrome/browser/sync/glue/bookmark_model_associator.cc:575: FolderInfoList
folders_matched;
Comment explaining why we keep this, which I think is so that the 2-pass
algorithm below works; 1st pass to identify folders but not remove them. I think
(?) we avoid removal so that we can add their children to the dfs? Could we not
Remove in the same iteration of the loop after we add the children, avoid the
extra scan and avoid the need for the  FolderInfoList?)  Could we also then add
the id to journals_to_purge from the same loop?

https://codereview.chromium.org/11533008/diff/24002/chrome/browser/sync/glue/...
chrome/browser/sync/glue/bookmark_model_associator.cc:581: // Iterate through
journals from back to front and keep unmatched journals
Mention that the reason we do this shuffling is so we can conveniently read off
all remaining unmatched entries after the loop.

https://codereview.chromium.org/11533008/diff/24002/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_bookmark_unittest.cc (right):

https://codereview.chromium.org/11533008/diff/24002/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:1042:
LoadBookmarkModel(LOAD_FROM_STORAGE, DONT_SAVE_TO_STORAGE);
cool! that part was easy :)

https://codereview.chromium.org/11533008/diff/24002/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:1048: // Remove
all folders/bookmarks added above.
Maybe leave one behind to make it a bit more interesting?

https://codereview.chromium.org/11533008/diff/24002/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:1057:
MessageLoop::current()->RunUntilIdle();
Comment this line.

https://codereview.chromium.org/11533008/diff/24002/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:1068:
ASSERT_TRUE(test_user_share_.Reload());
I'm not sure what this line and the one above are trying to achieve?

https://codereview.chromium.org/11533008/diff/24002/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:1071: // u2, f2,
u1 are removed by delete journal. f1 remains because it's not
Why is it non-empty?  Because AddURL chooses it behind-the-scenes to add your
youtube.com URL above?  (Might add comment if that's the case).

https://codereview.chromium.org/11533008/diff/24002/sync/internal_api/public/...
File sync/internal_api/public/delete_journal.h (right):

https://codereview.chromium.org/11533008/diff/24002/sync/internal_api/public/...
sync/internal_api/public/delete_journal.h:18: int64 id;
What is this id? (Metahandle? syncable::Id? BookmarkNode id?)

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

https://codereview.chromium.org/11533008/diff/24002/sync/syncable/directory.h...
sync/syncable/directory.h:135: friend class syncer::DeleteJournal;
Hmm.. let's avoid friend declarations if we can.  It'd be better to expose
delete_journals() publicly, assuming that's what this is for. (or figure out a
way to reverse the dependency / inject it, but I'm not sure that's an option
here.)

https://codereview.chromium.org/11533008/diff/24002/sync/test/engine/test_dir...
File sync/test/engine/test_directory_setter_upper.h (right):

https://codereview.chromium.org/11533008/diff/24002/sync/test/engine/test_dir...
sync/test/engine/test_directory_setter_upper.h:58: virtual void
SetUp(syncer::syncable::DirectoryBackingStore* directory_store);
Could we have a separate SetUpWith(DBS*) function, and keep the normal SetUp?
Seems like SetUp(NULL) is still the common case.

[haitaol1, 2013-01-15 19:44:31]

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

https://codereview.chromium.org/11533008/diff/24002/chrome/browser/sync/glue/...
chrome/browser/sync/glue/bookmark_model_associator.cc:88: // Finds best matching
node for the given sync node.
On 2013/01/14 23:32:48, timsteele wrote:
> Update comment.

Done.

https://codereview.chromium.org/11533008/diff/24002/chrome/browser/sync/glue/...
chrome/browser/sync/glue/bookmark_model_associator.cc:575: FolderInfoList
folders_matched;
Added comments. Do it in one pass is difficult because folders could embed in
each other, making it difficult to decide whether the upper-level folder should
be removed. Second scan should not cost too much because it only looks at
folders found in first pass.

On 2013/01/14 23:32:48, timsteele wrote:
> Comment explaining why we keep this, which I think is so that the 2-pass
> algorithm below works; 1st pass to identify folders but not remove them. I
think
> (?) we avoid removal so that we can add their children to the dfs? Could we
not
> Remove in the same iteration of the loop after we add the children, avoid the
> extra scan and avoid the need for the  FolderInfoList?)  Could we also then
add
> the id to journals_to_purge from the same loop?

https://codereview.chromium.org/11533008/diff/24002/chrome/browser/sync/glue/...
chrome/browser/sync/glue/bookmark_model_associator.cc:581: // Iterate through
journals from back to front and keep unmatched journals
On 2013/01/14 23:32:48, timsteele wrote:
> Mention that the reason we do this shuffling is so we can conveniently read
off
> all remaining unmatched entries after the loop.

Done.

https://codereview.chromium.org/11533008/diff/24002/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_bookmark_unittest.cc (right):

https://codereview.chromium.org/11533008/diff/24002/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:1042:
LoadBookmarkModel(LOAD_FROM_STORAGE, DONT_SAVE_TO_STORAGE);
On 2013/01/14 23:32:48, timsteele wrote:
> cool! that part was easy :)

Done.

https://codereview.chromium.org/11533008/diff/24002/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:1048: // Remove
all folders/bookmarks added above.
Added another bookmark under bookmark bar that's not deleted. 

On 2013/01/14 23:32:48, timsteele wrote:
> Maybe leave one behind to make it a bit more interesting?

https://codereview.chromium.org/11533008/diff/24002/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:1057:
MessageLoop::current()->RunUntilIdle();
On 2013/01/14 23:32:48, timsteele wrote:
> Comment this line.

Turns out not needed.

https://codereview.chromium.org/11533008/diff/24002/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:1068:
ASSERT_TRUE(test_user_share_.Reload());
To test that non-empty folder that matches delete journal is not deleted.

On 2013/01/14 23:32:48, timsteele wrote:
> I'm not sure what this line and the one above are trying to achieve?

https://codereview.chromium.org/11533008/diff/24002/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_bookmark_unittest.cc:1071: // u2, f2,
u1 are removed by delete journal. f1 remains because it's not
On 2013/01/14 23:32:48, timsteele wrote:
> Why is it non-empty?  Because AddURL chooses it behind-the-scenes to add your
> http://youtube.com URL above?  (Might add comment if that's the case).

Done.

https://codereview.chromium.org/11533008/diff/24002/sync/internal_api/public/...
File sync/internal_api/public/delete_journal.h (right):

https://codereview.chromium.org/11533008/diff/24002/sync/internal_api/public/...
sync/internal_api/public/delete_journal.h:18: int64 id;
It's metahandle of delete journal entry. Added comments.

On 2013/01/14 23:32:48, timsteele wrote:
> What is this id? (Metahandle? syncable::Id? BookmarkNode id?)

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

https://codereview.chromium.org/11533008/diff/24002/sync/syncable/directory.h...
sync/syncable/directory.h:135: friend class syncer::DeleteJournal;
On 2013/01/14 23:32:48, timsteele wrote:
> Hmm.. let's avoid friend declarations if we can.  It'd be better to expose
> delete_journals() publicly, assuming that's what this is for. (or figure out a
> way to reverse the dependency / inject it, but I'm not sure that's an option
> here.)  

Done.

https://codereview.chromium.org/11533008/diff/24002/sync/test/engine/test_dir...
File sync/test/engine/test_directory_setter_upper.h (right):

https://codereview.chromium.org/11533008/diff/24002/sync/test/engine/test_dir...
sync/test/engine/test_directory_setter_upper.h:58: virtual void
SetUp(syncer::syncable::DirectoryBackingStore* directory_store);
On 2013/01/14 23:32:48, timsteele wrote:
> Could we have a separate SetUpWith(DBS*) function, and keep the normal SetUp?
> Seems like SetUp(NULL) is still the common case.

Done.

[tim (not reviewing), 2013-01-18 00:29:06]

LGTM

[commit-bot: I haz the power, 2013-01-22 21:16:50]

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

[commit-bot: I haz the power, 2013-01-22 23:53:57]

Change committed as 178157