Only set remote id during url node creation.

Only set remote id during url node creation.

Folders should only have their id set by the server, and the clients
should only set the remote id on clips they created themselves.

Additionally, EnhancedBookmarkModel now monitors the remote id field for bookmarks, and initiates a de-duping protocol whenever two (or more) nodes with the same id are detected.
BUG=413876
Committed: https://crrev.com/f7a91481ccc179cb47494c3ada5d89b4941e17dd
Cr-Commit-Position: refs/heads/master@{#295790}

[Rune Fevang, 2014-09-12 22:35:53]

rfevang@chromium.org changed reviewers:
+ noyau@chromium.org

[noyau1, 2014-09-15 08:08:02]

noyau@google.com changed reviewers:
+ noyau@google.com

[noyau1, 2014-09-15 08:08:03]

https://codereview.chromium.org/563363002/diff/30001/components/enhanced_book...
File components/enhanced_bookmarks/enhanced_bookmark_model.cc (right):

https://codereview.chromium.org/563363002/diff/30001/components/enhanced_book...
components/enhanced_bookmarks/enhanced_bookmark_model.cc:118: return
std::string();
If existing bookmarks do not have IDs yet, this will break all the logic in the
services doing cluster and search. Those needs to be modified to avoid these
IDs.

[Mark, 2014-09-15 17:18:56]

mcolbert@chromium.org changed reviewers:
+ mcolbert@chromium.org

[Mark, 2014-09-15 17:18:57]

https://codereview.chromium.org/563363002/diff/30001/components/enhanced_book...
File components/enhanced_bookmarks/enhanced_bookmark_model.cc (right):

https://codereview.chromium.org/563363002/diff/30001/components/enhanced_book...
components/enhanced_bookmarks/enhanced_bookmark_model.cc:118: return
std::string();
On 2014/09/15 08:08:02, noyau1 wrote:
> If existing bookmarks do not have IDs yet, this will break all the logic in
the
> services doing cluster and search. Those needs to be modified to avoid these
> IDs.

We will create ids server-side for clustering and search.  The only time we
really need an id instantaneously created is when we are calling the "suggest"
functionality.  Outside of that, the id generation can wait until the sync
server sees the data (since that is when the clustering and search functionality
will actually be useful).  The big change we are doing in the JS code is to
allow for a server-side ids and client-side ids to be separate so we no longer
need the tight coupling.

This is how we are eliminating most of our data corruption issues.

[noyau (Ping after 24h), 2014-09-16 07:33:04]

https://codereview.chromium.org/563363002/diff/30001/components/enhanced_book...
File components/enhanced_bookmarks/enhanced_bookmark_model.cc (right):

https://codereview.chromium.org/563363002/diff/30001/components/enhanced_book...
components/enhanced_bookmarks/enhanced_bookmark_model.cc:118: return
std::string();
On 2014/09/15 17:18:56, Mark wrote:
> On 2014/09/15 08:08:02, noyau1 wrote:
> > If existing bookmarks do not have IDs yet, this will break all the logic in
> the
> > services doing cluster and search. Those needs to be modified to avoid these
> > IDs.
> 
> We will create ids server-side for clustering and search.  The only time we
> really need an id instantaneously created is when we are calling the "suggest"
> functionality.  Outside of that, the id generation can wait until the sync
> server sees the data (since that is when the clustering and search
functionality
> will actually be useful).  The big change we are doing in the JS code is to
> allow for a server-side ids and client-side ids to be separate so we no longer
> need the tight coupling.
> 
> This is how we are eliminating most of our data corruption issues. 

Yes, yes I understand that. All I'm saying is that there is a need to add a
if(!id.empty()) before adding those objects to the map stars_id/bookmarkNodes in
bookmark_server_service.

[Rune Fevang, 2014-09-16 23:18:11]

Ah, hadn't seen the new BookmarkServerService class. Converted it to use
EnhancedBookmarkModel instead of BookmarkAccessor.

Also added the de-duplication code I was planning on sending in my next CL, as
it already contained logic for maintaining an id-to-node mapping.

PTAL.

[noyau (Ping after 24h), 2014-09-17 08:24:35]

https://codereview.chromium.org/563363002/diff/50001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_search_service.cc (right):

https://codereview.chromium.org/563363002/diff/50001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_search_service.cc:103: int index)
{
The superclass used to register for bookmark model changes, you ripped that code
off, so this methods are no longer called.

Note that all the existing subclasses (this one and the one in
https://codereview.chromium.org/539173004/) do need the model change
notification.

But the most problematic issue is that during those notifications the
extended_bookmark_model may change the maps id/bookmarks but the order in which
the observer are called is random, this could lead to subtle bugs.

But these services only needs to know when the maps id/bookmark changes (and
only a subset: id changed, id removed). Instead of being a model observer this
should be an extended model observer, only notified when the map of id to
bookmarks changes.

https://codereview.chromium.org/563363002/diff/50001/components/enhanced_book...
File components/enhanced_bookmarks/enhanced_bookmark_model.cc (right):

https://codereview.chromium.org/563363002/diff/50001/components/enhanced_book...
components/enhanced_bookmarks/enhanced_bookmark_model.cc:319:
nodes_to_reset_[node] = remote_id;
Don't you need to remove the node you found in the map from the map as well?

[Rune Fevang, 2014-09-17 22:51:04]

https://codereview.chromium.org/563363002/diff/50001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_search_service.cc (right):

https://codereview.chromium.org/563363002/diff/50001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_search_service.cc:103: int index)
{
On 2014/09/17 08:24:35, noyau wrote:
> The superclass used to register for bookmark model changes, you ripped that
code
> off, so this methods are no longer called.
> 
> Note that all the existing subclasses (this one and the one in
> https://codereview.chromium.org/539173004/) do need the model change
> notification.

Oops! Put the inheritance back in the superclass since the subclasses all need
it. I'm not convinced that the subclasses (at least this one) need these events
though, see below.

> 
> But the most problematic issue is that during those notifications the
> extended_bookmark_model may change the maps id/bookmarks but the order in
which
> the observer are called is random, this could lead to subtle bugs.

How does the order matter for this code? All I see is a clearing of a map, which
would execute the same regardless of the status of the id map in
EnhancedBookmarkModel.

As a side question, why clear the searches when the model updates? Any ids
returned in the search result returns should still be good matches, why not just
omit ids you can't find from the results? This case could happen anyway, as you
are not guaranteed that the model has received updates that the server may know
about in any case.

> 
> But these services only needs to know when the maps id/bookmark changes (and
> only a subset: id changed, id removed). Instead of being a model observer this
> should be an extended model observer, only notified when the map of id to
> bookmarks changes.

I agree that the extended bookmark model should have it's own observer class
(loaded needs to be emitted after building the id map, separate events for
different meta info changes would be useful), however I considered it out of
scope for this CL. I see you're triggering functionality on the loaded event in
the CL you linked to however, so added an observer class.

https://codereview.chromium.org/563363002/diff/50001/components/enhanced_book...
File components/enhanced_bookmarks/enhanced_bookmark_model.cc (right):

https://codereview.chromium.org/563363002/diff/50001/components/enhanced_book...
components/enhanced_bookmarks/enhanced_bookmark_model.cc:319:
nodes_to_reset_[node] = remote_id;
On 2014/09/17 08:24:35, noyau wrote:
> Don't you need to remove the node you found in the map from the map as well?

It needs to stay there in case there are more duplicates (say 3), so all three
will be found. That's also why the resetting of duplicate ids is deferred until
the loading is completed instead of being done on detection. The nodes will be
removed from the id map when OnMetaInfoChanged is received.

[noyau (Ping after 24h), 2014-09-18 08:43:22]

I'll wait to land the other server service after you land this one, so you don't
have to solve the conflicts there as well.

https://codereview.chromium.org/563363002/diff/50001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_search_service.cc (right):

https://codereview.chromium.org/563363002/diff/50001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_search_service.cc:103: int index)
{
On 2014/09/17 22:51:04, Rune Fevang wrote:
> On 2014/09/17 08:24:35, noyau wrote:
> > The superclass used to register for bookmark model changes, you ripped that
> code
> > off, so this methods are no longer called.
> > 
> > Note that all the existing subclasses (this one and the one in
> > https://codereview.chromium.org/539173004/) do need the model change
> > notification.
> 
> Oops! Put the inheritance back in the superclass since the subclasses all need
> it. I'm not convinced that the subclasses (at least this one) need these
events
> though, see below.
> 
> > 
> > But the most problematic issue is that during those notifications the
> > extended_bookmark_model may change the maps id/bookmarks but the order in
> which
> > the observer are called is random, this could lead to subtle bugs.
> 
> How does the order matter for this code? All I see is a clearing of a map,
which
> would execute the same regardless of the status of the id map in
> EnhancedBookmarkModel.
>
If one register for the model notification and do anything with the stars id by
calling into extended_bookmark_model the data they get back may be different
depending on the order the observers are called. You're right that this is not
an issue
for this existing code, but it is a potential minefield.

> As a side question, why clear the searches when the model updates? Any ids
> returned in the search result returns should still be good matches, why not
just
> omit ids you can't find from the results? This case could happen anyway, as
you
> are not guaranteed that the model has received updates that the server may
know
> about in any case.
> 
What about additions? By dropping the data, the query will be redone and the new
data
might be picked up.

> > 
> > But these services only needs to know when the maps id/bookmark changes (and
> > only a subset: id changed, id removed). Instead of being a model observer
this
> > should be an extended model observer, only notified when the map of id to
> > bookmarks changes.
> 
> I agree that the extended bookmark model should have it's own observer class
> (loaded needs to be emitted after building the id map, separate events for
> different meta info changes would be useful), however I considered it out of
> scope for this CL. I see you're triggering functionality on the loaded event
in
> the CL you linked to however, so added an observer class.

https://codereview.chromium.org/563363002/diff/50001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_search_service.cc:110:
searches_.clear();
In this case you're right, just swapping the id would be sufficient.

https://codereview.chromium.org/563363002/diff/50001/components/enhanced_book...
File components/enhanced_bookmarks/enhanced_bookmark_model.cc (right):

https://codereview.chromium.org/563363002/diff/50001/components/enhanced_book...
components/enhanced_bookmarks/enhanced_bookmark_model.cc:319:
nodes_to_reset_[node] = remote_id;
On 2014/09/17 22:51:04, Rune Fevang wrote:
> On 2014/09/17 08:24:35, noyau wrote:
> > Don't you need to remove the node you found in the map from the map as well?
> 
> It needs to stay there in case there are more duplicates (say 3), so all three
> will be found. That's also why the resetting of duplicate ids is deferred
until
> the loading is completed instead of being done on detection. The nodes will be
> removed from the id map when OnMetaInfoChanged is received.

Acknowledged.

https://codereview.chromium.org/563363002/diff/70001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_service.cc (right):

https://codereview.chromium.org/563363002/diff/70001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_service.cc:36:
model_->AddObserver(this);
Remove observer maybe :)

https://codereview.chromium.org/563363002/diff/70001/components/enhanced_book...
File components/enhanced_bookmarks/enhanced_bookmark_model_observer.h (right):

https://codereview.chromium.org/563363002/diff/70001/components/enhanced_book...
components/enhanced_bookmarks/enhanced_bookmark_model_observer.h:17: virtual
void EnhancedBookmarkModelLoaded() {};
I'll make all these method virtual to force implementation to think about all
the cases. For example in the server_service ShuttingDown() should DCHECK and
AllNodesRemoved() should be implemented to clear the search state… (and BTW I'm
painfully aware that these two issues existed before this CL even existed!)

https://codereview.chromium.org/563363002/diff/70001/components/enhanced_book...
File components/enhanced_bookmarks/enhanced_bookmark_model_unittest.cc (right):

https://codereview.chromium.org/563363002/diff/70001/components/enhanced_book...
components/enhanced_bookmarks/enhanced_bookmark_model_unittest.cc:472: 
Can you add tests to make sure the observer methods are all called properly?
Thanks.

[Mark, 2014-09-18 18:18:28]

Thanks Rune!  Some comments below.

https://codereview.chromium.org/563363002/diff/70001/components/enhanced_book...
File components/enhanced_bookmarks/enhanced_bookmark_model.cc (right):

https://codereview.chromium.org/563363002/diff/70001/components/enhanced_book...
components/enhanced_bookmarks/enhanced_bookmark_model.cc:360: for
(NodeToIdMap::iterator it = nodes_to_reset_.begin();
Just checking, are you able to get some sort of lock on Chrome Sync from the
binary code?  That would be great to ensure these deduping actions are not
getting split across request boundaries.

https://codereview.chromium.org/563363002/diff/70001/components/enhanced_book...
components/enhanced_bookmarks/enhanced_bookmark_model.cc:412: old_field->second
== it->second) {
So is all this effort really necessary?  What happens if you touch every item
again?  Why not directly replace the meta info values?

[Rune Fevang, 2014-09-19 01:18:07]

https://codereview.chromium.org/563363002/diff/50001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_search_service.cc (right):

https://codereview.chromium.org/563363002/diff/50001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_search_service.cc:103: int index)
{
> If one register for the model notification and do anything with the stars id
by
> calling into extended_bookmark_model the data they get back may be different
> depending on the order the observers are called. You're right that this is not
> an issue
> for this existing code, but it is a potential minefield.

Hmm, I think there's an additional issue with the bookmark model observers use,
if we update the model from a listener callback, events will become
unpredictable for other listeners. If we're called first they'll see two
WillChange* calls followed by two Changed calls. Changed the code so any
updating of the model will be done asynchronously. That means other code will
temporarily see remote ids that are about to be changed, but I think that's OK.

> What about additions? By dropping the data, the query will be redone and the
new
> data
> might be picked up.

OK, I didn't realize there was any requerying going on, as the subset of the
code I've been looking at just cleared a map. Since I have no idea how this code
is being used I'll leave it to you :)

https://codereview.chromium.org/563363002/diff/70001/components/enhanced_book...
File components/enhanced_bookmarks/bookmark_server_service.cc (right):

https://codereview.chromium.org/563363002/diff/70001/components/enhanced_book...
components/enhanced_bookmarks/bookmark_server_service.cc:36:
model_->AddObserver(this);
On 2014/09/18 08:43:22, noyau wrote:
> Remove observer maybe :)

Doh! Thanks.

https://codereview.chromium.org/563363002/diff/70001/components/enhanced_book...
File components/enhanced_bookmarks/enhanced_bookmark_model.cc (right):

https://codereview.chromium.org/563363002/diff/70001/components/enhanced_book...
components/enhanced_bookmarks/enhanced_bookmark_model.cc:360: for
(NodeToIdMap::iterator it = nodes_to_reset_.begin();
On 2014/09/18 18:18:27, Mark wrote:
> Just checking, are you able to get some sort of lock on Chrome Sync from the
> binary code?  That would be great to ensure these deduping actions are not
> getting split across request boundaries.

There's no sync API support for doing that, the bookmark change processor simply
listens to the bookmark model and updates/creates sync nodes as they come in.

https://codereview.chromium.org/563363002/diff/70001/components/enhanced_book...
components/enhanced_bookmarks/enhanced_bookmark_model.cc:412: old_field->second
== it->second) {
On 2014/09/18 18:18:27, Mark wrote:
> So is all this effort really necessary?  What happens if you touch every item
> again?  Why not directly replace the meta info values?

Necessary? Not really, it's more of a safeguard against setting the version
field if we didn't actually change anything, I believe we ran into that problem
on the JS client right?

Thanks for drawing my eye to this function, noticed a bug (and added a test for
it) ;)

https://codereview.chromium.org/563363002/diff/70001/components/enhanced_book...
File components/enhanced_bookmarks/enhanced_bookmark_model_observer.h (right):

https://codereview.chromium.org/563363002/diff/70001/components/enhanced_book...
components/enhanced_bookmarks/enhanced_bookmark_model_observer.h:17: virtual
void EnhancedBookmarkModelLoaded() {};
On 2014/09/18 08:43:22, noyau wrote:
> I'll make all these method virtual to force implementation to think about all
> the cases. For example in the server_service ShuttingDown() should DCHECK and
> AllNodesRemoved() should be implemented to clear the search state… (and BTW
I'm
> painfully aware that these two issues existed before this CL even existed!)

OK, made everything but remote id changed pure virtual.

Added implementations according to your comments.

https://codereview.chromium.org/563363002/diff/70001/components/enhanced_book...
File components/enhanced_bookmarks/enhanced_bookmark_model_unittest.cc (right):

https://codereview.chromium.org/563363002/diff/70001/components/enhanced_book...
components/enhanced_bookmarks/enhanced_bookmark_model_unittest.cc:472: 
On 2014/09/18 08:43:22, noyau wrote:
> Can you add tests to make sure the observer methods are all called properly?
> Thanks.

Done.

[noyau (Ping after 24h), 2014-09-19 07:38:27]

lgtm

[Rune Fevang, 2014-09-19 18:36:51]

The CQ bit was checked by rfevang@chromium.org

[commit-bot: I haz the power, 2014-09-19 18:37:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/563363002/130001

[commit-bot: I haz the power, 2014-09-19 18:52:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-19 18:52:17]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Rune Fevang, 2014-09-19 21:26:07]

The CQ bit was checked by rfevang@chromium.org

[commit-bot: I haz the power, 2014-09-19 21:27:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/563363002/130001

[commit-bot: I haz the power, 2014-09-19 22:06:47]

Committed patchset #8 (id:130001) as 8f2a4f74b9d240dc2116ba94fe98a8698fc935db

[commit-bot: I haz the power, 2014-09-19 22:06:57]

Committed patchset #8 (id:130001) as 26edeed09e906f0a6ebb7e786eba18327e0080a9

[commit-bot: I haz the power, 2014-09-19 22:07:27]

Patchset 8 (id:??) landed as
https://crrev.com/f7a91481ccc179cb47494c3ada5d89b4941e17dd
Cr-Commit-Position: refs/heads/master@{#295790}