Separate SyncData methods into three groups, local, remote, and common.

Separate SyncData methods into three groups, local, remote, and common.

SyncData objects come in two varieties, local and remote.  Local
SyncData are those that originiate from datatype code (CreateLocalData).
Remote SyncData originate from sync (CreateRemoteData).  This change
adds SyncDataLocal and SyncDataRemote.

The purpose of this change is to make clear which methods may be called
on a given SyncData instance.

Local methods are accessed via SyncData::AsLocal().  Remote methods via
SyncData::AsRemote().

TBR=brettw

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

[maniscalco, 2014-03-28 20:59:34]

Hey Tim, mind taking a look?  This is a follow up CL to the Sync API change you
reviewed yesterday.

[tim (not reviewing), 2014-03-28 21:16:02]

https://codereview.chromium.org/217063005/diff/20001/sync/api/sync_data.h
File sync/api/sync_data.h (right):

https://codereview.chromium.org/217063005/diff/20001/sync/api/sync_data.h#new...
sync/api/sync_data.h:112: class Local {
I guess I was imagining we would organize the data at construction time, with
local fields going into Local and remote into Remote, exploiting the fact that
it can't change after construction.

Also if we keep Local/Remote to POD containers only, we wouldn't need to expose
the non-const ref (which is technically anti-style guide) and could keep
GetOrDownloadAttachments as a method on SyncData.

The other approach I was thinking of involved subclassing, and maybe in that
world we'd organize the methods on the proper subclass, but here I feel it'd be
better to keep it to data-only?

https://codereview.chromium.org/217063005/diff/20001/sync/api/sync_data.h#new...
sync/api/sync_data.h:124: friend SyncData;
Do you actually need the friend?

https://codereview.chromium.org/217063005/diff/20001/sync/api/sync_data.h#new...
sync/api/sync_data.h:145: int64 GetRemoteId() const;
I think 'Remote' in names of methods is redundant now that there's a class.

[maniscalco, 2014-03-28 21:45:13]

https://codereview.chromium.org/217063005/diff/20001/sync/api/sync_data.h
File sync/api/sync_data.h (right):

https://codereview.chromium.org/217063005/diff/20001/sync/api/sync_data.h#new...
sync/api/sync_data.h:112: class Local {
On 2014/03/28 21:16:02, timsteele wrote:
> I guess I was imagining we would organize the data at construction time, with
> local fields going into Local and remote into Remote, exploiting the fact that
> it can't change after construction.
> 
> Also if we keep Local/Remote to POD containers only, we wouldn't need to
expose
> the non-const ref (which is technically anti-style guide) and could keep
> GetOrDownloadAttachments as a method on SyncData.
> 
> The other approach I was thinking of involved subclassing, and maybe in that
> world we'd organize the methods on the proper subclass, but here I feel it'd
be
> better to keep it to data-only?

Re organizing the data at construction time... I explored that, but gave up once
I noticed GetTag (local) needs to access immutable_entity_.  I should this
approach.  I can probably make it work if the Remote and Local classes have
storage *and* have access to the common stuff via SyncData.

I'm not sure I follow your second comment about POD containers.  Maybe we should
sync up in person?

https://codereview.chromium.org/217063005/diff/20001/sync/api/sync_data.h#new...
sync/api/sync_data.h:124: friend SyncData;
On 2014/03/28 21:16:02, timsteele wrote:
> Do you actually need the friend?

Yeah, I was using it along with the private Local ctor to ensure that only
SyncData can construct SyncData::Local (or SyncData::Remote).  The motivation
was to "prevent callers from misusing SyncData::Local".

https://codereview.chromium.org/217063005/diff/20001/sync/api/sync_data.h#new...
sync/api/sync_data.h:145: int64 GetRemoteId() const;
On 2014/03/28 21:16:02, timsteele wrote:
> I think 'Remote' in names of methods is redundant now that there's a class.

Agreed.  Removed.

[maniscalco, 2014-03-28 22:03:35]

Move data members out of SyncData and into SyncData::Local and SyncData::Remote
where possible.

[maniscalco, 2014-03-31 20:31:12]

I've updated this CL based on our offline discussion.  RFAL!

[tim (not reviewing), 2014-03-31 22:38:11]

looking better!

https://codereview.chromium.org/217063005/diff/80001/sync/api/sync_data.h
File sync/api/sync_data.h (right):

https://codereview.chromium.org/217063005/diff/80001/sync/api/sync_data.h#new...
sync/api/sync_data.h:101: SyncDataLocal AsLocal() const;
Hmm. Given that there's no way (that doesn't involve friend-ing) to disallow
public creation of SyncDataLocal/Remote, it's probably best to just have one way
to build these objects and make sure it is explicit and safe.

The explicit constructors you have on SDL/R should do the trick, provided you
move the DCHECK in AsLocal/AsRemote to the appropriate constructor. I was going
to say factory functions (and a private constructor) on SDL/R would be slightly
better because you could avoid constructing an illegitimate object altogether,
but if we're using DCHECKs we shouldn't code for the failure case anyway, so the
constructors suffice.

A nice side effect is that it becomes legal to call any method on SyncData at
all times. I was going to suggest a SyncData factory function to get the same
effect until I realized the SDL/R constructors had to be public. We eliminate
"only call this when ..." cases, which was one of the original purposes of this
cl.

https://codereview.chromium.org/217063005/diff/80001/sync/api/sync_data.h#new...
sync/api/sync_data.h:165: // As SyncData going to the syncer.
nit - A SyncData

[maniscalco, 2014-03-31 23:11:07]

All feedback applied.  RFAL.

https://codereview.chromium.org/217063005/diff/80001/sync/api/sync_data.h
File sync/api/sync_data.h (right):

https://codereview.chromium.org/217063005/diff/80001/sync/api/sync_data.h#new...
sync/api/sync_data.h:101: SyncDataLocal AsLocal() const;
On 2014/03/31 22:38:12, timsteele wrote:
> Hmm. Given that there's no way (that doesn't involve friend-ing) to disallow
> public creation of SyncDataLocal/Remote, it's probably best to just have one
way
> to build these objects and make sure it is explicit and safe.
> 
> The explicit constructors you have on SDL/R should do the trick, provided you
> move the DCHECK in AsLocal/AsRemote to the appropriate constructor. I was
going
> to say factory functions (and a private constructor) on SDL/R would be
slightly
> better because you could avoid constructing an illegitimate object altogether,
> but if we're using DCHECKs we shouldn't code for the failure case anyway, so
the
> constructors suffice.
> 
> A nice side effect is that it becomes legal to call any method on SyncData at
> all times. I was going to suggest a SyncData factory function to get the same
> effect until I realized the SDL/R constructors had to be public. We eliminate
> "only call this when ..." cases, which was one of the original purposes of
this
> cl. 
> 

Done.  I've removed AsLocal/AsRemote and added DCHECKs to the ctors.

https://codereview.chromium.org/217063005/diff/80001/sync/api/sync_data.h#new...
sync/api/sync_data.h:165: // As SyncData going to the syncer.
On 2014/03/31 22:38:12, timsteele wrote:
> nit - A SyncData

Done.

[tim (not reviewing), 2014-03-31 23:24:24]

LGTM

https://codereview.chromium.org/217063005/diff/100001/sync/api/sync_change.cc
File sync/api/sync_change.cc (right):

https://codereview.chromium.org/217063005/diff/100001/sync/api/sync_change.cc...
sync/api/sync_change.cc:37: if (sync_data_local.GetTag().empty() ||
SyncDataLocal(sync_data_).GetTag() ?

[maniscalco, 2014-03-31 23:33:54]

Thanks for reviewing!

https://codereview.chromium.org/217063005/diff/100001/sync/api/sync_change.cc
File sync/api/sync_change.cc (right):

https://codereview.chromium.org/217063005/diff/100001/sync/api/sync_change.cc...
sync/api/sync_change.cc:37: if (sync_data_local.GetTag().empty() ||
On 2014/03/31 23:24:24, timsteele wrote:
> SyncDataLocal(sync_data_).GetTag() ?

Done.

[maniscalco, 2014-04-01 16:15:09]

rlp@, would you please review
c/b/spellchecker/spellcheck_custom_dictionary_unittest.cc

brettw@, would you please review c/b/history/history_unittest.cc

asargent@, would you please review c/b/extensions/*

Thanks.

[rpetterson, 2014-04-01 17:56:39]

spellchecker LGTM

[asargent_no_longer_on_chrome, 2014-04-01 20:40:30]

chrome/browser/extensions LGTM

[maniscalco, 2014-04-02 17:38:20]

TBR=brettw for chrome/browser/history/history_unittest.cc

[maniscalco, 2014-04-02 17:38:43]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-04-02 17:39:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maniscalco@chromium.org/217063005/140001

[brettw, 2014-04-02 17:49:59]

lgtm

[commit-bot: I haz the power, 2014-04-02 21:05:06]

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

[commit-bot: I haz the power, 2014-04-02 21:05:07]

Retried try job too often on win_rel for step(s) base_unittests,
base_unittests_swarm, browser_tests, browser_tests_swarm, interactive_ui_tests,
interactive_ui_tests_swarm, net_unittests, net_unittests_swarm, unit_tests,
unit_tests_swarm
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[maniscalco, 2014-04-02 21:11:37]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-04-02 21:12:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maniscalco@chromium.org/217063005/140001

[commit-bot: I haz the power, 2014-04-03 04:14:30]

Change committed as 261273