[sync] Componentize sync: Part 5: Eliminate filename collisions in sync.gyp

[sync] Componentize sync: Part 5: Eliminate filename collisions in sync.gyp

In order to move src/sync into its own compoent, all the static_library targets in sync.gyp need to be converted to targets of type 'none' and rolled into one sync target of type component. This is currently blocked by duplicate .cc filenames in syncable and internal_api, which flags an error while running gyp. The offending files are {internal_api|syncable}/{base|read|write}_transaction.cc.

This patch renames syncable/{base|read|write}_transaction.{h|cc} to syncable/syncable_{base|read|write}_transaction.{h|cc}.

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

[Raghu Simha, 2012-12-19 20:14:35]

Richard, please review. Thanks.

[rlarocque, 2012-12-19 22:04:49]

LGTM, though we should ping Tim about this before we commit.  He has a better
understanding of the syncable / syncer separation than I do.  

Normally I wouldn't bother double-checking but, given the nature of this patch,
I think it would be best to get this right on the first try.

https://codereview.chromium.org/11638018/diff/1/sync/sync.gyp
File sync/sync.gyp (right):

https://codereview.chromium.org/11638018/diff/1/sync/sync.gyp#newcode180
sync/sync.gyp:180: # TODO(sync): Figure out if this class is required.
I wouldn't bother with these TODOs.  It's not really clear from reading them
exactly what is to be done.  If anything, we could file a bug that talks about
what we could do with these classes.

[Raghu Simha, 2012-12-19 22:10:36]

Tim, please review. See
https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/chromium-d...
for more context. Thanks.

https://codereview.chromium.org/11638018/diff/1/sync/sync.gyp
File sync/sync.gyp (right):

https://codereview.chromium.org/11638018/diff/1/sync/sync.gyp#newcode180
sync/sync.gyp:180: # TODO(sync): Figure out if this class is required.
On 2012/12/19 22:04:49, rlarocque wrote:
> I wouldn't bother with these TODOs.  It's not really clear from reading them
> exactly what is to be done.  If anything, we could file a bug that talks about
> what we could do with these classes.

I removed the rest of the comment too, since on second thoughts, it doesn't seem
necessary. Done.

[tim (not reviewing), 2012-12-19 22:24:18]

On 2012/12/19 22:04:49, rlarocque wrote:
> LGTM, though we should ping Tim about this before we commit.  He has a better
> understanding of the syncable / syncer separation than I do.  
> 
> Normally I wouldn't bother double-checking but, given the nature of this
patch,
> I think it would be best to get this right on the first try.
> 
> https://codereview.chromium.org/11638018/diff/1/sync/sync.gyp
> File sync/sync.gyp (right):
> 
> https://codereview.chromium.org/11638018/diff/1/sync/sync.gyp#newcode180
> sync/sync.gyp:180: # TODO(sync): Figure out if this class is required.
> I wouldn't bother with these TODOs.  It's not really clear from reading them
> exactly what is to be done.  If anything, we could file a bug that talks about
> what we could do with these classes.

I'm not a gyp expert, but you're sure this is necessary?  

From a layering point of view this seems okay though.  I probably would have
renamed the internal_api versions (to ApiReadTransaction or something) given the
fact that it wraps the core syncable version, but I don't think it's any better
for real reasons.

In general we shouldn't stray from the file name <> class name convention if we
can avoid it, although this would mean renaming all instances of the classes and
added redundancy since we do already address the syncable classes with a
namespace :/  

So, if we need this, I don't have any improvement to offer and I think this
LGTM.

[Raghu Simha, 2012-12-19 22:41:28]

On 2012/12/19 22:24:18, timsteele wrote:
> I'm not a gyp expert, but you're sure this is necessary?  

Yes, it's going to be necessary for component builds, where all targets have
type 'none', and get consumed by the sync component target. Ninja barfs when
there are two translation units in a target with the same name.

> So, if we need this, I don't have any improvement to offer and I think this
> LGTM.

I considered renaming one of the classes and discussed it with Richard, but it
seemed to be a lot of work for no real benefit except sticking to the naming
convention. I'll land this patch as is. Thanks.