sync: Specialize functions that fetch type root

sync: Specialize functions that fetch type root

Adds functions to look up the root node of a give type.  This replaces
most of the use cases for the functions that look up a node by unique
server tag, so those functions have been renamed to indicate their
near-deprecation.

This is part of the effort to remove the client's dependence on the
server for the creation of type root nodes.  Although it's unlikely that
we can make this work for existing types, new types should be prepared
to work properly in the absence of tyep root nodes.  The first step on
that path is to abstract away the mechanism by which we look up the type
roots.

Renaming the old lookup by tag functions allows us to ensure that there
no remaining cases where the code base looks up type roots by server
tag.  This CL also adds a few DCHECKs to ensure type roots are never
looked up using the old lookup by server tag functions.

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

[rlarocque, 2014-06-02 17:47:43]

I had initially discussed this plan with Nicolas, but he's busy with the
non-blocking sync engine reviews, so I'm going to switch it up and send this
review to Tim.

This patch puts the very common operation of fetching a type's root node behind
an abstraction layer that knows nothing about server tags.  This will allow us
to support different kinds of root nodes in the future.

There's still one use case for server tags, and that's for some top-level
bookmark folders.  I don't foresee anyone using server tags in the future, so I
decided to mark those operations as deprecated.  This is intended to prevent
anyone from accidentally relying on the server tag <-> root node association in
the future.

Tim: Please review.

[tim (not reviewing), 2014-06-03 15:21:21]

https://codereview.chromium.org/302173004/diff/1/sync/syncable/entry.h
File sync/syncable/entry.h (right):

https://codereview.chromium.org/302173004/diff/1/sync/syncable/entry.h#newcode39
sync/syncable/entry.h:39: GET_BY_SERVER_TAG_DEPRECATED
The decision to deprecate this seems separate from wanting to specialize the
root fetching functions, which I agree makes sense.  Removing GetByServerTag
from the sync API ReadNode seems more than sufficient to prevent accidental use
by API clients, which is also a goal I agree makes sense.  I don't see as strong
of a case to rename this field to 'DEPRECATED' or even remove it from Entry, in
particular since we have no plan to remove it (at least for bookmarks) yet.  I
think it should just stay as-is at the lower levels here.

[rlarocque, 2014-06-03 17:29:39]

Patch updated.  PTAL.

https://codereview.chromium.org/302173004/diff/1/sync/syncable/entry.h
File sync/syncable/entry.h (right):

https://codereview.chromium.org/302173004/diff/1/sync/syncable/entry.h#newcode39
sync/syncable/entry.h:39: GET_BY_SERVER_TAG_DEPRECATED
On 2014/06/03 15:21:22, timsteele wrote:
> The decision to deprecate this seems separate from wanting to specialize the
> root fetching functions, which I agree makes sense.  Removing GetByServerTag
> from the sync API ReadNode seems more than sufficient to prevent accidental
use
> by API clients, which is also a goal I agree makes sense.  I don't see as
strong
> of a case to rename this field to 'DEPRECATED' or even remove it from Entry,
in
> particular since we have no plan to remove it (at least for bookmarks) yet.  I
> think it should just stay as-is at the lower levels here.

I added a suffix because I wanted the compiler to warn me if I missed a spot. 
_FOR_BOOKMARKS didn't seem quite right, since this layer is attempting to be
type-agnostic.

I suppose now that I've done the update, there's no harm in changing it back. 
That's uploaded in the latest patch.

As a separate note, though, are you sure server tags aren't deprecated?  I
thought there was a reason we didn't use them for experiment nodes.  If they
couldn't fit that use case, then I don't know what purpose they serve.

[tim (not reviewing), 2014-06-03 17:38:43]

On 2014/06/03 17:29:39, rlarocque wrote:
> Patch updated.  PTAL.
> 
> https://codereview.chromium.org/302173004/diff/1/sync/syncable/entry.h
> File sync/syncable/entry.h (right):
> 
>
https://codereview.chromium.org/302173004/diff/1/sync/syncable/entry.h#newcode39
> sync/syncable/entry.h:39: GET_BY_SERVER_TAG_DEPRECATED
> On 2014/06/03 15:21:22, timsteele wrote:
> > The decision to deprecate this seems separate from wanting to specialize the
> > root fetching functions, which I agree makes sense.  Removing GetByServerTag
> > from the sync API ReadNode seems more than sufficient to prevent accidental
> use
> > by API clients, which is also a goal I agree makes sense.  I don't see as
> strong
> > of a case to rename this field to 'DEPRECATED' or even remove it from Entry,
> in
> > particular since we have no plan to remove it (at least for bookmarks) yet. 
I
> > think it should just stay as-is at the lower levels here.
> 
> I added a suffix because I wanted the compiler to warn me if I missed a spot. 
> _FOR_BOOKMARKS didn't seem quite right, since this layer is attempting to be
> type-agnostic.
> 
> I suppose now that I've done the update, there's no harm in changing it back. 
> That's uploaded in the latest patch.
> 
> As a separate note, though, are you sure server tags aren't deprecated?  I
> thought there was a reason we didn't use them for experiment nodes.  If they
> couldn't fit that use case, then I don't know what purpose they serve.

They may be deprecated in that we only want to use them for one type, but we
already have a fair bit
of code in syncer that is bookmarks-specific without being annotated as
'deprecated'.  I think it is safe
to say in our code we reserve 'deprecated' for things we don't want to support
going forward *that have an
actual migration plan*, but (e.g) it's too much work to fully convert all
callsites at once.

LGTM

[rlarocque, 2014-06-03 17:41:33]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-06-03 17:41:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rlarocque@chromium.org/302173004/20001

[rlarocque, 2014-06-03 17:43:49]

On 2014/06/03 17:38:43, timsteele wrote:
> On 2014/06/03 17:29:39, rlarocque wrote:
> > Patch updated.  PTAL.
> > 
> > https://codereview.chromium.org/302173004/diff/1/sync/syncable/entry.h
> > File sync/syncable/entry.h (right):
> > 
> >
>
https://codereview.chromium.org/302173004/diff/1/sync/syncable/entry.h#newcode39
> > sync/syncable/entry.h:39: GET_BY_SERVER_TAG_DEPRECATED
> > On 2014/06/03 15:21:22, timsteele wrote:
> > > The decision to deprecate this seems separate from wanting to specialize
the
> > > root fetching functions, which I agree makes sense.  Removing
GetByServerTag
> > > from the sync API ReadNode seems more than sufficient to prevent
accidental
> > use
> > > by API clients, which is also a goal I agree makes sense.  I don't see as
> > strong
> > > of a case to rename this field to 'DEPRECATED' or even remove it from
Entry,
> > in
> > > particular since we have no plan to remove it (at least for bookmarks)
yet. 
> I
> > > think it should just stay as-is at the lower levels here.
> > 
> > I added a suffix because I wanted the compiler to warn me if I missed a
spot. 
> > _FOR_BOOKMARKS didn't seem quite right, since this layer is attempting to be
> > type-agnostic.
> > 
> > I suppose now that I've done the update, there's no harm in changing it
back. 
> > That's uploaded in the latest patch.
> > 
> > As a separate note, though, are you sure server tags aren't deprecated?  I
> > thought there was a reason we didn't use them for experiment nodes.  If they
> > couldn't fit that use case, then I don't know what purpose they serve.
> 
> They may be deprecated in that we only want to use them for one type, but we
> already have a fair bit
> of code in syncer that is bookmarks-specific without being annotated as
> 'deprecated'.  I think it is safe
> to say in our code we reserve 'deprecated' for things we don't want to support
> going forward *that have an
> actual migration plan*, but (e.g) it's too much work to fully convert all
> callsites at once.
> 

SGTM.  I would have defined it as "do not use this in new code going forward",
but either definition can work.

[commit-bot: I haz the power, 2014-06-03 20:45:31]

Change committed as 274613