[sync] Componentize sync: Part 1: Clean up sync.gyp and update chrome / test dependencies

[sync] Componentize sync: Part 1: Clean up sync.gyp and update chrome / test dependencies

One of the long term goals of the sync team is to pull sync code out of
chrome.dll and into its own component. As of today, several chrome tests
depend on various sync targets as defined in sync.gyp.

This is the first in a series of CLs to achieve the above goal.
This patch does the following:

- Renames / reorganizes several of the targets in sync.gyp
- Updates the declarations in various chrome gyp files by depending on
  the smallest sync target possible to reflect the actual dependency.
- Takes a step closer toward the ideal IWYU guideline.

TBR=erikwright@chromium.org
BUG=136928
TEST=all chrome targets build on all platforms; all tests pass.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167558

[Raghu Simha, 2012-11-01 23:00:52]

Fred / Tim / Ryan, please review.

Fred / Tim: Changes to sync.gyp + overall review.
Ryan: overall review.

PS: Ryan, I'm adding you as a reviewer since you made several useful comments on
http://codereview.chromium.org/10805046. I've loosely used that patch as a
starting point, but the scope of this CL is smaller, in that all I'm trying to
do in this step is to reorganize sync.gyp and get more chrome targets to follow
IWYU by removing as much as I could from chrome_browser.gypi:browser.

[Ryan Sleevi, 2012-11-03 21:50:06]

http://codereview.chromium.org/11348052/diff/27012/sync/sync.gyp
File sync/sync.gyp (right):

http://codereview.chromium.org/11348052/diff/27012/sync/sync.gyp#newcode407
sync/sync.gyp:407: # The componentized sync library.
??? This isn't a 'type': '<(component)' target, so I'm not sure what this
comment means

http://codereview.chromium.org/11348052/diff/27012/sync/sync.gyp#newcode429
sync/sync.gyp:429: {
It's not clear to me what this target actually provides/does for callers. Did I
miss a target depending on this?

[Raghu Simha, 2012-11-05 23:04:22]

http://codereview.chromium.org/11348052/diff/27012/sync/sync.gyp
File sync/sync.gyp (right):

http://codereview.chromium.org/11348052/diff/27012/sync/sync.gyp#newcode407
sync/sync.gyp:407: # The componentized sync library.
On 2012/11/03 21:50:06, Ryan Sleevi wrote:
> ??? This isn't a 'type': '<(component)' target, so I'm not sure what this
> comment means

Sorry, this was from an earlier attempt to fully componentize sync in one go.
This patch doesn't do so, due to trickiness associated with exporting the
sync_proto target. It is my intent to change the type of this target to
'<(component)'. Until then, I've added a TODO below.

http://codereview.chromium.org/11348052/diff/27012/sync/sync.gyp#newcode429
sync/sync.gyp:429: {
On 2012/11/03 21:50:06, Ryan Sleevi wrote:
> It's not clear to me what this target actually provides/does for callers. Did
I
> miss a target depending on this?

There are a couple of standalone sync tools and sync test targets that depend on
this. See 'sync_unit_tests', 'sync_listen_notifications' and 'sync_client'.

[Ryan Sleevi, 2012-11-06 13:20:22]

Please do not rebase in the middle of a codereview, and then also make changes.

If you're going to rebase, it's better to
1) Make the changes based on review feedback
2) Upload the patchset (so that it can easily be diffed with the feedback from
the previous patchset)
3) Upload the rebased patchset

However, step 3 can still cause hassles, so it is better to avoid it if it's not
absolutely necessary.

Otherwise, I think these changes look fine to me, in that they don't really
change much, and I don't see any glaringly obvious issues. Since this is still
an iteration before componentization, I have not carefully reviewed that you're
exporting all of the correct dependencies and targets, since the current
structure does not require it. Before turning it into a component though, you
will have to worry about header ordering and dependency exporting.

One issue below remains regarding a misleading comment.

http://codereview.chromium.org/11348052/diff/15020/sync/sync.gyp
File sync/sync.gyp (right):

http://codereview.chromium.org/11348052/diff/15020/sync/sync.gyp#newcode14
sync/sync.gyp:14: # dependencies on the sync_proto target.
This comment seems to conflict with 'sync_component'

It seems like only one of them should become a <(component) - otherwise you need
two DLL decls (SYNC_IMPLEMENTATION or otherwise), and would need to worry about
ODR violations about sync_proto appearing in two different DLLs.

[Raghu Simha, 2012-11-06 20:02:17]

Thanks for the review, Ryan. PTAL.

Sorry about the mid-review rebase -- I did so in order to be able to add try
jobs to the patch. Patch set 3 was uploaded without rebasing.

http://codereview.chromium.org/11348052/diff/15020/sync/sync.gyp
File sync/sync.gyp (right):

http://codereview.chromium.org/11348052/diff/15020/sync/sync.gyp#newcode14
sync/sync.gyp:14: # dependencies on the sync_proto target.
On 2012/11/06 13:20:22, Ryan Sleevi wrote:
> This comment seems to conflict with 'sync_component'
> 
> It seems like only one of them should become a <(component) - otherwise you
need
> two DLL decls (SYNC_IMPLEMENTATION or otherwise), and would need to worry
about
> ODR violations about sync_proto appearing in two different DLLs.

Removed this comment.

[akalin, 2012-11-08 01:01:50]

few comments

http://codereview.chromium.org/11348052/diff/28015/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (left):

http://codereview.chromium.org/11348052/diff/28015/chrome/chrome_browser.gypi...
chrome/chrome_browser.gypi:37: '../sync/protocol/sync_proto.gyp:sync_proto',
here, too.  why are you removing these dependencies?  pretty sure they're all
still valid.  you should either replace them with a dependency on 'sync', or
just rename them.

http://codereview.chromium.org/11348052/diff/28015/sync/sync.gyp
File sync/sync.gyp (right):

http://codereview.chromium.org/11348052/diff/28015/sync/sync.gyp#newcode761
sync/sync.gyp:761: 'sync_core',
here and other places below

why are you removing these dependencies?  pretty sure all of them still hold.

[Raghu Simha, 2012-11-08 22:34:38]

Fred, PTAL. Thanks.

http://codereview.chromium.org/11348052/diff/28015/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (left):

http://codereview.chromium.org/11348052/diff/28015/chrome/chrome_browser.gypi...
chrome/chrome_browser.gypi:37: '../sync/protocol/sync_proto.gyp:sync_proto',
On 2012/11/08 01:01:51, akalin wrote:
> here, too.  why are you removing these dependencies?  pretty sure they're all
> still valid.  you should either replace them with a dependency on 'sync', or
> just rename them.

Same reasoning as in my comment on sync.gyp. I've re-added the targets.

http://codereview.chromium.org/11348052/diff/28015/sync/sync.gyp
File sync/sync.gyp (right):

http://codereview.chromium.org/11348052/diff/28015/sync/sync.gyp#newcode761
sync/sync.gyp:761: 'sync_core',
On 2012/11/08 01:01:51, akalin wrote:
> here and other places below
> 
> why are you removing these dependencies?  pretty sure all of them still hold.

Here's my reasoning: The target 'sync' got renamed to 'sync_core', while
'test_support_syncapi_core' got renamed to 'test_support_sync_internal_api'. The
dependencies I removed were 'sync_proto', 'sync_notifier' and 'syncapi_core'.
What I did was take a look at all the source files added by this target under
the 'sources' section and see which files they were including. It seemed to make
sense to me that a test target called 'sync_internal_api_tests' only needed to
depend on the targets that contained the files included from within its source
files, and that any indirect dependencies would get picked up via the targets it
directly depended on.

From your comment, it appears my assumption was incorrect, so I've re-added the
dependencies I've removed.

[akalin, 2012-11-09 18:14:02]

On 2012/11/08 22:34:38, rsimha wrote:
> Here's my reasoning: The target 'sync' got renamed to 'sync_core', while
> 'test_support_syncapi_core' got renamed to 'test_support_sync_internal_api'.
The
> dependencies I removed were 'sync_proto', 'sync_notifier' and 'syncapi_core'.
> What I did was take a look at all the source files added by this target under
> the 'sources' section and see which files they were including. It seemed to
make
> sense to me that a test target called 'sync_internal_api_tests' only needed to
> depend on the targets that contained the files included from within its source
> files, and that any indirect dependencies would get picked up via the targets
it
> directly depended on.
> 
> From your comment, it appears my assumption was incorrect, so I've re-added
the
> dependencies I've removed.

Yeah, I think I made a similar pass before and added a dependency when the test
files directly depended on a file in a target.

Looking...

[akalin, 2012-11-09 18:42:18]

took a detailed pass over sync.gyp

https://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp
File sync/sync.gyp (left):

https://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#oldcode422
sync/sync.gyp:422: 'protocol/sync_proto.gyp:sync_proto',
there aren't any files in this target that include proto files directly?

https://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#oldcode578
sync/sync.gyp:578: 'protocol/sync_proto.gyp:sync_proto',
no files in this target that include proto files directly?

https://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#oldcode669
sync/sync.gyp:669: 'sync_notifier',
no direct deps on files from sync_notifier?

https://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp
File sync/sync.gyp (right):

https://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#newcode22
sync/sync.gyp:22: '../base/base.gyp:base',
since you moved a bunch of internal_api files out of this target, can you audit
these deps to see if any of them can be removed? (If you haven't done so
already)

https://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#newcode31
sync/sync.gyp:31: # Propagate sync_proto since our headers include its generated
is this still true?

https://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#newcode268
sync/sync.gyp:268: '../base/base.gyp:base',
similarly to the above, can you audit these deps to see if any need to be added?
(if you haven't done so already)

https://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#newcode273
sync/sync.gyp:273: 'sync_core',
put before sync_notifier

https://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#newcode381
sync/sync.gyp:381: 'sync_core',
Looks like this needs to be sync_internal_api instead

https://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#newcode460
sync/sync.gyp:460: 'internal_api/public/base/invalidation_test_util.cc',
hmm i think these files need to move to test_support_sync_internal_api

https://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#newcode516
sync/sync.gyp:516: 'sync_core',
hmm, did you discover a new direct dep on a file in sync_core?  shouldn't this
be sync_internal_api?

https://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#newcode522
sync/sync.gyp:522: 'sync_core',
here too

https://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#newcode552
sync/sync.gyp:552: 'test_support_sync_core',
may not need this anymore once you move the rest of the internal_api files in

https://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#newcode581
sync/sync.gyp:581: 'sync_core',
doesn't depend on sync_api?

https://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#newcode764
sync/sync.gyp:764: 'sync_internal_api',
reorder

[Raghu Simha, 2012-11-09 22:57:52]

Thanks for the detailed review, Fred. Several fixes in the latest patch, while
some items remain unchanged after auditing.

PTAL.

http://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp
File sync/sync.gyp (left):

http://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#oldcode422
sync/sync.gyp:422: 'protocol/sync_proto.gyp:sync_proto',
On 2012/11/09 18:42:18, akalin wrote:
> there aren't any files in this target that include proto files directly?

Good catch. I had originally searched for includes of headers named "*pb2.h" and
didn't find any, but it turns out that there are several includes of headers
named "*pb.h". Reinstated dependency.

http://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#oldcode578
sync/sync.gyp:578: 'protocol/sync_proto.gyp:sync_proto',
On 2012/11/09 18:42:18, akalin wrote:
> no files in this target that include proto files directly?

Fixed. Same reason as before. I was looking for pb2.h files. There are legit
pb.h includes.

http://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#oldcode669
sync/sync.gyp:669: 'sync_notifier',
On 2012/11/09 18:42:18, akalin wrote:
> no direct deps on files from sync_notifier?

Fixed.

http://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp
File sync/sync.gyp (right):

http://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#newcode22
sync/sync.gyp:22: '../base/base.gyp:base',
On 2012/11/09 18:42:18, akalin wrote:
> since you moved a bunch of internal_api files out of this target, can you
audit
> these deps to see if any of them can be removed? (If you haven't done so
> already)

Audited via a series of git-greps. All deps are needed due to includes in some
file or the other in engine, syncable, util, etc.

http://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#newcode31
sync/sync.gyp:31: # Propagate sync_proto since our headers include its generated
On 2012/11/09 18:42:18, akalin wrote:
> is this still true?

Still true, due to syncable/model_type.cc.

http://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#newcode268
sync/sync.gyp:268: '../base/base.gyp:base',
On 2012/11/09 18:42:18, akalin wrote:
> similarly to the above, can you audit these deps to see if any need to be
added?
> (if you haven't done so already)

Audited. All dependencies needed. For instance, googleurl and net are used by
http_bridge, pb files are used by base_node, etc.

http://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#newcode273
sync/sync.gyp:273: 'sync_core',
On 2012/11/09 18:42:18, akalin wrote:
> put before sync_notifier

Good catch. Done.

http://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#newcode381
sync/sync.gyp:381: 'sync_core',
On 2012/11/09 18:42:18, akalin wrote:
> Looks like this needs to be sync_internal_api instead

Done.

http://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#newcode460
sync/sync.gyp:460: 'internal_api/public/base/invalidation_test_util.cc',
On 2012/11/09 18:42:18, akalin wrote:
> hmm i think these files need to move to test_support_sync_internal_api

Done.

http://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#newcode516
sync/sync.gyp:516: 'sync_core',
On 2012/11/09 18:42:18, akalin wrote:
> hmm, did you discover a new direct dep on a file in sync_core?  shouldn't this
> be sync_internal_api?

You're right. Fixed. I thought I found a dependency on a file under util/ which
is in sync_core, but on looking closer, it was internal_api/public/util.

http://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#newcode522
sync/sync.gyp:522: 'sync_core',
On 2012/11/09 18:42:18, akalin wrote:
> here too

Done.

http://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#newcode552
sync/sync.gyp:552: 'test_support_sync_core',
On 2012/11/09 18:42:18, akalin wrote:
> may not need this anymore once you move the rest of the internal_api files in

Done.

http://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#newcode581
sync/sync.gyp:581: 'sync_core',
On 2012/11/09 18:42:18, akalin wrote:
> doesn't depend on sync_api?

Done.

http://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#newcode764
sync/sync.gyp:764: 'sync_internal_api',
On 2012/11/09 18:42:18, akalin wrote:
> reorder

Done.

[akalin, 2012-11-10 00:29:11]

one more pass over sync.gyp

http://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp
File sync/sync.gyp (right):

http://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#newcode31
sync/sync.gyp:31: # Propagate sync_proto since our headers include its generated
On 2012/11/09 22:57:53, rsimha wrote:
> On 2012/11/09 18:42:18, akalin wrote:
> > is this still true?
> 
> Still true, due to syncable/model_type.cc.

Note that we need to export only if header files include the generated files. 
But it looks like it's still true via engine/traffic_recorder.h and
protocol/proto_enum_conversions.h.

http://codereview.chromium.org/11348052/diff/18019/sync/sync.gyp#newcode268
sync/sync.gyp:268: '../base/base.gyp:base',
On 2012/11/09 22:57:53, rsimha wrote:
> On 2012/11/09 18:42:18, akalin wrote:
> > similarly to the above, can you audit these deps to see if any need to be
> added?
> > (if you haven't done so already)
> 
> Audited. All dependencies needed. For instance, googleurl and net are used by
> http_bridge, pb files are used by base_node, etc.

I actually meant to see if adding the new internal_api files pulled in any deps
from sync_core.  But I did the audit myself (via git greps on sync/internal_api)
and it looks like nothing needs to be added.

http://codereview.chromium.org/11348052/diff/32002/sync/sync.gyp
File sync/sync.gyp (left):

http://codereview.chromium.org/11348052/diff/32002/sync/sync.gyp#oldcode528
sync/sync.gyp:528: 'test_support_sync',
looks like this dep is still needed -- test_internal_components_factory.cc
includes fake_sync_scheduler.h

http://codereview.chromium.org/11348052/diff/32002/sync/sync.gyp
File sync/sync.gyp (right):

http://codereview.chromium.org/11348052/diff/32002/sync/sync.gyp#newcode409
sync/sync.gyp:409: 'variables': { 'enable_wexit_time_destructors': 1, },
i wonder if this variables decl is actually needed, since it's of type 'none'
(i.e., nothing gets compiled).  I think not: see sync_core_tests target, etc.

Also, do we need "'suppress_wildcard': 1," like those sync_core_tests targets? 
I recall I had to add that to prevent the 'all' build from breaking.

http://codereview.chromium.org/11348052/diff/32002/sync/sync.gyp#newcode429
sync/sync.gyp:429: 'type': 'none',
same comment as above re. suppress_wildcard: 1

http://codereview.chromium.org/11348052/diff/32002/sync/sync.gyp#newcode510
sync/sync.gyp:510: 'sync_internal_api',
nice catch

http://codereview.chromium.org/11348052/diff/32002/sync/sync.gyp#newcode549
sync/sync.gyp:549: 'sync_core',
need dependency on sync_internal_api also (from invalidation_test_util.cc)

http://codereview.chromium.org/11348052/diff/32002/sync/sync.gyp#newcode559
sync/sync.gyp:559: 'internal_api/public/sessions/sync_source_info_unittest.cc',
hunh, these are actual unittest files -- i think they belong to the appropriate
unit test target below

http://codereview.chromium.org/11348052/diff/32002/sync/sync.gyp#newcode562
sync/sync.gyp:562: 'internal_api/public/test/test_entry_factory.h',
looks like this pulls in a dep on the proto target (includes sync.pb.h)

http://codereview.chromium.org/11348052/diff/32002/sync/sync.gyp#newcode913
sync/sync.gyp:913: 'sync_internal_api',
alphabetize

[Raghu Simha, 2012-11-12 18:49:48]

Fred, PTAL. Meanwhile, I'm going to rebase this on ToT using a new local branch
to make sure the 'all' target still builds, and that we don't need the
'suppress_wildcard' declaration.

http://codereview.chromium.org/11348052/diff/32002/sync/sync.gyp
File sync/sync.gyp (left):

http://codereview.chromium.org/11348052/diff/32002/sync/sync.gyp#oldcode528
sync/sync.gyp:528: 'test_support_sync',
On 2012/11/10 00:29:11, akalin wrote:
> looks like this dep is still needed -- test_internal_components_factory.cc
> includes fake_sync_scheduler.h

Done.

http://codereview.chromium.org/11348052/diff/32002/sync/sync.gyp
File sync/sync.gyp (right):

http://codereview.chromium.org/11348052/diff/32002/sync/sync.gyp#newcode409
sync/sync.gyp:409: 'variables': { 'enable_wexit_time_destructors': 1, },
On 2012/11/10 00:29:11, akalin wrote:
> i wonder if this variables decl is actually needed, since it's of type 'none'
> (i.e., nothing gets compiled).  I think not: see sync_core_tests target, etc.

Removed.

> Also, do we need "'suppress_wildcard': 1," like those sync_core_tests targets?

> I recall I had to add that to prevent the 'all' build from breaking.

I was able to build ' all' without suppress_wildcard.

http://codereview.chromium.org/11348052/diff/32002/sync/sync.gyp#newcode429
sync/sync.gyp:429: 'type': 'none',
On 2012/11/10 00:29:11, akalin wrote:
> same comment as above re. suppress_wildcard: 1

I seem to be able to build 'all' without suppress_wildcard.

http://codereview.chromium.org/11348052/diff/32002/sync/sync.gyp#newcode549
sync/sync.gyp:549: 'sync_core',
On 2012/11/10 00:29:11, akalin wrote:
> need dependency on sync_internal_api also (from invalidation_test_util.cc)

Done.

http://codereview.chromium.org/11348052/diff/32002/sync/sync.gyp#newcode559
sync/sync.gyp:559: 'internal_api/public/sessions/sync_source_info_unittest.cc',
On 2012/11/10 00:29:11, akalin wrote:
> hunh, these are actual unittest files -- i think they belong to the
appropriate
> unit test target below

Done. Moved to sync_internal_api_tests.

http://codereview.chromium.org/11348052/diff/32002/sync/sync.gyp#newcode562
sync/sync.gyp:562: 'internal_api/public/test/test_entry_factory.h',
On 2012/11/10 00:29:11, akalin wrote:
> looks like this pulls in a dep on the proto target (includes sync.pb.h)

Done.

http://codereview.chromium.org/11348052/diff/32002/sync/sync.gyp#newcode913
sync/sync.gyp:913: 'sync_internal_api',
On 2012/11/10 00:29:11, akalin wrote:
> alphabetize

Done.

[akalin, 2012-11-12 22:55:35]

On 2012/11/12 18:49:48, rsimha wrote:
> Fred, PTAL. Meanwhile, I'm going to rebase this on ToT using a new local
branch
> to make sure the 'all' target still builds, and that we don't need the
> 'suppress_wildcard' declaration.

> I was able to build ' all' without suppress_wildcard.

You're on linux or Windows, right?  I think this was a Mac-only problem: see
https://code.google.com/p/chromium/issues/detail?id=118371 for details (and
r126995, where I added suppress_wildcard)

[akalin, 2012-11-12 22:56:57]

On 2012/11/12 22:55:35, akalin wrote:
> On 2012/11/12 18:49:48, rsimha wrote:
> > Fred, PTAL. Meanwhile, I'm going to rebase this on ToT using a new local
> branch
> > to make sure the 'all' target still builds, and that we don't need the
> > 'suppress_wildcard' declaration.
> 
> > I was able to build ' all' without suppress_wildcard.
> 
> You're on linux or Windows, right?  I think this was a Mac-only problem: see
> https://code.google.com/p/chromium/issues/detail?id=118371 for details (and
> r126995, where I added suppress_wildcard)

Ah, this might not apply since the new 'none' targets don't have a source block.
 But I think it's still worth testing on OS X with the xcode generator.

If you don't have convenient access to an OS X build, let me know and I can test
it.

[Raghu Simha, 2012-11-12 23:49:52]

On 2012/11/12 22:56:57, akalin wrote:
> On 2012/11/12 22:55:35, akalin wrote:
> > You're on linux or Windows, right?  I think this was a Mac-only problem: see
> > https://code.google.com/p/chromium/issues/detail?id=118371 for details (and
> > r126995, where I added suppress_wildcard)
> 
> Ah, this might not apply since the new 'none' targets don't have a source
block.
>  But I think it's still worth testing on OS X with the xcode generator.
> 
> If you don't have convenient access to an OS X build, let me know and I can
test
> it.

Building locally on OS X right now. I've also uploaded a rebased version of
patch set 6 (with no other code changes), and have kicked off a bunch of try
jobs for it. Will keep an eye.

[Raghu Simha, 2012-11-13 01:36:06]

On 2012/11/12 23:49:52, rsimha wrote:
> Building locally on OS X right now. I've also uploaded a rebased version of
> patch set 6 (with no other code changes), and have kicked off a bunch of try
> jobs for it. Will keep an eye.

Local OS X build of target 'all' was successful after rebasing on ToT. Trybots
looking happy too.

[akalin, 2012-11-13 01:37:50]

On 2012/11/13 01:36:06, rsimha wrote:
> On 2012/11/12 23:49:52, rsimha wrote:
> > Building locally on OS X right now. I've also uploaded a rebased version of
> > patch set 6 (with no other code changes), and have kicked off a bunch of try
> > jobs for it. Will keep an eye.
> 
> Local OS X build of target 'all' was successful after rebasing on ToT. Trybots
> looking happy too.

I assume this was an XCode build? (Not sure what the default on OS X is
nowadays.)

If so, then that answers my worries.  I'll take another look!

[akalin, 2012-11-13 01:46:03]

LGTM after nits!

http://codereview.chromium.org/11348052/diff/16008/sync/sync.gyp
File sync/sync.gyp (right):

http://codereview.chromium.org/11348052/diff/16008/sync/sync.gyp#newcode864
sync/sync.gyp:864: 'sync_internal_api_tests',
alphabetize

http://codereview.chromium.org/11348052/diff/16008/sync/sync.gyp#newcode903
sync/sync.gyp:903: 'sync_notifier',
remove 'sync_notifier' (subsumed by sync)

http://codereview.chromium.org/11348052/diff/16008/sync/sync.gyp#newcode923
sync/sync.gyp:923: 'sync_internal_api',
remove 'sync_internal_api' and 'sync_notifier' (subsumed by sync)

[Raghu Simha, 2012-11-13 03:19:50]

+sky for chrome owners approval.
+erikwright for chrome_frame owners approval.

http://codereview.chromium.org/11348052/diff/16008/sync/sync.gyp
File sync/sync.gyp (right):

http://codereview.chromium.org/11348052/diff/16008/sync/sync.gyp#newcode864
sync/sync.gyp:864: 'sync_internal_api_tests',
On 2012/11/13 01:46:03, akalin wrote:
> alphabetize

Done.

http://codereview.chromium.org/11348052/diff/16008/sync/sync.gyp#newcode903
sync/sync.gyp:903: 'sync_notifier',
On 2012/11/13 01:46:03, akalin wrote:
> remove 'sync_notifier' (subsumed by sync)

Done.

http://codereview.chromium.org/11348052/diff/16008/sync/sync.gyp#newcode923
sync/sync.gyp:923: 'sync_internal_api',
On 2012/11/13 01:46:03, akalin wrote:
> remove 'sync_internal_api' and 'sync_notifier' (subsumed by sync)

Done.

[sky, 2012-11-13 17:21:34]

LGTM

[commit-bot: I haz the power, 2012-11-13 22:59:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsimha@chromium.org/11348052/31012

[commit-bot: I haz the power, 2012-11-14 01:23:55]

Change committed as 167558