Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(286)

Issue 9701094: [Sync] Clean up sync.gyp (Closed)

Created:
8 years, 9 months ago by akalin
Modified:
8 years, 9 months ago
CC:
chromium-reviews, tfarina
Visibility:
Public.

Description

[Sync] Clean up sync.gyp Only export dependent settings when necessary. Forward-declare crypto classes to avoid including crypto headers in sync headers (and thus neccessitating exporting crypto settings to dependents). BUG=117585 TEST= Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=129449

Patch Set 1 #

Patch Set 2 : export gmock from test_support_sync also #

Total comments: 8

Patch Set 3 : Address comments #

Total comments: 6

Patch Set 4 : Address comments #

Patch Set 5 : sync to head #

Unified diffs Side-by-side diffs Delta from patch set Stats (+13 lines, -10 lines) Patch
M sync/sync.gyp View 1 2 3 4 4 chunks +8 lines, -9 lines 0 comments Download
M sync/util/nigori.h View 1 2 3 1 chunk +4 lines, -1 line 0 comments Download
M sync/util/nigori.cc View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 14 (0 generated)
akalin
+rsleevi for review (to sanity-check my understanding of when to use export_dependent_settings) +tim for OWNERS
8 years, 9 months ago (2012-03-16 11:09:14 UTC) #1
Ryan Sleevi
GYP sanity checked. Comments below. Note: Most of Chromium code doesn't really conform to how ...
8 years, 9 months ago (2012-03-16 17:24:21 UTC) #2
akalin
Addressed comments. ping, tim? http://codereview.chromium.org/9701094/diff/6001/sync/sync.gyp File sync/sync.gyp (right): http://codereview.chromium.org/9701094/diff/6001/sync/sync.gyp#newcode194 sync/sync.gyp:194: 'protocol/sync_proto.gyp:sync_proto', On 2012/03/16 17:24:21, Ryan ...
8 years, 9 months ago (2012-03-16 19:22:52 UTC) #3
Ryan Sleevi
http://codereview.chromium.org/9701094/diff/5/sync/sync.gyp File sync/sync.gyp (right): http://codereview.chromium.org/9701094/diff/5/sync/sync.gyp#newcode193 sync/sync.gyp:193: 'protocol/sync_proto.gyp:sync_proto', Unless your header files (for this target) are ...
8 years, 9 months ago (2012-03-16 19:50:23 UTC) #4
akalin
http://codereview.chromium.org/9701094/diff/5/sync/sync.gyp File sync/sync.gyp (right): http://codereview.chromium.org/9701094/diff/5/sync/sync.gyp#newcode193 sync/sync.gyp:193: 'protocol/sync_proto.gyp:sync_proto', On 2012/03/16 19:50:23, Ryan Sleevi wrote: > Unless ...
8 years, 9 months ago (2012-03-16 20:02:00 UTC) #5
tfarina
http://codereview.chromium.org/9701094/diff/5/sync/util/nigori.h File sync/util/nigori.h (right): http://codereview.chromium.org/9701094/diff/5/sync/util/nigori.h#newcode15 sync/util/nigori.h:15: } // namespace nit: just remove "// namespace" or ...
8 years, 9 months ago (2012-03-17 14:19:17 UTC) #6
akalin
Ping, tim and rsleevi! http://codereview.chromium.org/9701094/diff/5/sync/util/nigori.h File sync/util/nigori.h (right): http://codereview.chromium.org/9701094/diff/5/sync/util/nigori.h#newcode15 sync/util/nigori.h:15: } // namespace On 2012/03/17 ...
8 years, 9 months ago (2012-03-21 00:08:51 UTC) #7
Ryan Sleevi
That was a LGTM, but since I don't own sync/, and you just asked sanity ...
8 years, 9 months ago (2012-03-21 00:13:06 UTC) #8
akalin
On 2012/03/21 00:13:06, Ryan Sleevi wrote: > That was a LGTM, but since I don't ...
8 years, 9 months ago (2012-03-26 18:55:08 UTC) #9
tim (not reviewing)
LGTM
8 years, 9 months ago (2012-03-27 17:54:04 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/akalin@chromium.org/9701094/15002
8 years, 9 months ago (2012-03-28 06:47:06 UTC) #11
commit-bot: I haz the power
Try job failure for 9701094-15002 (retry) on win_rel for steps "ui_tests, browser_tests". It's a second ...
8 years, 9 months ago (2012-03-28 09:28:02 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/akalin@chromium.org/9701094/15002
8 years, 9 months ago (2012-03-28 16:26:13 UTC) #13
commit-bot: I haz the power
8 years, 9 months ago (2012-03-28 18:32:01 UTC) #14
Change committed as 129449

Powered by Google App Engine
This is Rietveld 408576698