Include sync.pb.h in sync_data.h.

Include sync.pb.h in sync_data.h.

This change has two parts, including sync.pb.h in sync_data.h, and fixing
gyp dependencies and header includes so that everything still builds.

Include sync.pb.h in sync_data.h in preparation for using AttachmentId
(a protobuf in sync.pb.h) in sync_data.h. A forward declaration is
insufficient because sync_data.h will be getting a typedef for vector<>
of AttachmentId.

Several targets (transitively) depend on sync, however, their dependence
is not properly modeled in gyp. Update dependencies and header includes
so they can "see" sync.pb.h (a generated header) and any headers it
includes.

Without these changes, four targets fail to build:

1. extensions_browser - See extensions/extensions.gyp for the fix. Here's
the compiler error:

In file included from ../../extensions/browser/extension_function.cc:10:
In file included from ../../chrome/browser/extensions/extension_service.h:23:
In file included from ../../chrome/browser/extensions/extension_sync_service.h:12:
In file included from ../../chrome/browser/extensions/app_sync_bundle.h:15:
In file included from ../../chrome/browser/extensions/app_sync_data.h:8:
In file included from ../../chrome/browser/extensions/extension_sync_data.h:11:
In file included from ../../sync/api/sync_change.h:13:
../../sync/api/sync_data.h:17:10: fatal error: 'sync/protocol/sync.pb.h' file not found


2. debugger - See chrome/chrome.gyp for the fix. Here's the compiler
error:

In file included from ../../chrome/browser/devtools/browser_list_tabcontents_provider.cc:10:
In file included from ../../chrome/browser/history/top_sites.h:13:
In file included from ../../chrome/browser/history/history_service.h:26:
In file included from ../../chrome/browser/history/delete_directive_handler.h:12:
In file included from ../../sync/api/sync_change_processor.h:10:
../../sync/api/sync_data.h:17:10: fatal error: 'sync/protocol/sync.pb.h' file not found


3. apps - See chrome/chrome_browser_extensions.gypi for the fix. Here's
the compiler error:

In file included from ../../apps/app_load_service.cc:12:
In file included from ../../chrome/browser/extensions/extension_service.h:23:
In file included from ../../chrome/browser/extensions/extension_sync_service.h:12:
In file included from ../../chrome/browser/extensions/app_sync_bundle.h:15:
In file included from ../../chrome/browser/extensions/app_sync_data.h:8:
In file included from ../../chrome/browser/extensions/extension_sync_data.h:11:
In file included from ../../sync/api/sync_change.h:13:
../../sync/api/sync_data.h:17:10: fatal error: 'sync/protocol/sync.pb.h' file not found


4. performance_ui_tests - See chrome/chrome_tests.gypi for the fix.
Here's the compiler error:

In file included from ../../chrome/test/perf/generate_profile.cc:19:
In file included from ../../chrome/browser/history/history_service.h:26:
In file included from ../../chrome/browser/history/delete_directive_handler.h:12:
In file included from ../../sync/api/sync_change_processor.h:10:
In file included from ../../sync/api/sync_data.h:17:
gen/protoc_out/sync/protocol/sync.pb.h:9:10: fatal error: 'google/protobuf/stubs/common.h' file not found

BUG=

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

[maniscalco, 2014-02-27 17:44:12]

tim@chromium.org: Please review changes in sync/

asargent@chromium.org: Please review changes in extensions/

thakis@chromium.org: Please review changes in chrome/

[asargent_no_longer_on_chrome, 2014-02-27 19:13:56]

extensions/ rubber stamp lgtm with the caveat that I am definitely not a gyp
expert

[tim (not reviewing), 2014-02-27 22:45:15]

sync/ LGTM, same caveat as Antony.

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

https://codereview.chromium.org/182313002/diff/1/sync/api/sync_data.h#newcode17
sync/api/sync_data.h:17: #include "sync/protocol/sync.pb.h"
one small step for a man...

[Nico, 2014-03-03 21:20:48]

Does this work reliably in incremental builds, given that sync isn't marked
'hard_dependency': 1?

[maniscalco, 2014-03-04 00:26:57]

On 2014/03/03 21:20:48, Nico wrote:
> Does this work reliably in incremental builds, given that sync isn't marked
> 'hard_dependency': 1?

Good question. I don't know. How can I find out?

Looks like 'hard_dependency':1 was removed from sync_proto here:
https://codereview.chromium.org/7677034/

I see that sync depends on sync_proto, which includes protoc.gypi, which has
'hard_dependency':1. Is that sufficient to get reliable incremental builds?

[Nico, 2014-03-07 19:12:59]

Good point about protoc.gypi already setting hard_dependency. That's likely why
all these explicit dependencies on sync are needed – I think this is how we
usually do this (Ryan?), so this should be fine.

(But if it's easy to not include pb.h files in headers, not needing these
dependencies is a good reason for not including pb.h files in headers.)

https://codereview.chromium.org/182313002/diff/1/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/182313002/diff/1/chrome/chrome.gyp#newcode147
chrome/chrome.gyp:147: # browser, which depends on sync. We cannot simply
declare a
In what way does debugger depend on browser? Does it include a header from
browser? Since a real dependency would be a cycle as you note, could this wrong
include be fixed instead?

https://codereview.chromium.org/182313002/diff/1/chrome/chrome_browser_extens...
File chrome/chrome_browser_extensions.gypi (right):

https://codereview.chromium.org/182313002/diff/1/chrome/chrome_browser_extens...
chrome/chrome_browser_extensions.gypi:66: # This target has header files that
include files from sync. Give our
""that include generated header files…"

https://codereview.chromium.org/182313002/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/182313002/diff/1/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:2273: 'browser',
Since 'chrome' already depends on 'browser', I'd probably have a dependency on
sync here, with a comment about generated headers.

https://codereview.chromium.org/182313002/diff/1/extensions/extensions.gyp
File extensions/extensions.gyp (right):

https://codereview.chromium.org/182313002/diff/1/extensions/extensions.gyp#ne...
extensions/extensions.gyp:161: # (browser_extension already depends on
extensions_browser).
Same question as above.

[Ryan Sleevi, 2014-03-07 19:23:00]

The way to model this is:

The actual target that runs the protoc step sets 'hard_dependency': 1
  - this is presently done by the build/protoc.gypi static_library targets
(IINM)

If a target B includes a .pb.h from A in one of B's .h "public" files, then it
should export_dependent_settings the target that built the .pb.h (aka A)
  - This would mean that the sync target should export_dependent_settings the
sync_proto_cpp target (if I remember the names correctly)

If a target C includes one of B's .h "public" files in one of C's .h "public"
files, then it should export_dependent_settings on B
  - Any targets that put sync_data.h in a *public* header (eg: they don't
forward declare the sync types) would do this
If a target C includes one of B's .h "public" files in one of C's
*implementations*, then no further steps are needed.

That is, the correct dependency steps are *not* to add explicit dependencies on
sync/ to target D, which picks up a header from C, which picks up a header from
B, which picks up a header from sync/. It's to have B export_dependent_settings
sync, C to export_dependent_settings on B, and D to do nothing. We transitively
resolve the hard_dependencies of B before compiling D, since D transitively
depends on B.

[maniscalco, 2014-03-07 23:44:36]

Nico, Ryan, thanks for the helpful feedback!  I've eliminated most of the new
dependencies on sync by changing some header includes.  I think this version of
the patch is much better than the original.  PTAL.

https://codereview.chromium.org/182313002/diff/1/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/182313002/diff/1/chrome/chrome.gyp#newcode147
chrome/chrome.gyp:147: # browser, which depends on sync. We cannot simply
declare a
On 2014/03/07 19:13:00, Nico wrote:
> In what way does debugger depend on browser? Does it include a header from
> browser? Since a real dependency would be a cycle as you note, could this
wrong
> include be fixed instead?

Good question.  I've removed/replaced some #includes and used forward
declarations where possible.  This dependency is no longer needed.

https://codereview.chromium.org/182313002/diff/1/chrome/chrome_browser_extens...
File chrome/chrome_browser_extensions.gypi (right):

https://codereview.chromium.org/182313002/diff/1/chrome/chrome_browser_extens...
chrome/chrome_browser_extensions.gypi:66: # This target has header files that
include files from sync. Give our
On 2014/03/07 19:13:00, Nico wrote:
> ""that include generated header files…"

Done.

https://codereview.chromium.org/182313002/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/182313002/diff/1/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:2273: 'browser',
On 2014/03/07 19:13:00, Nico wrote:
> Since 'chrome' already depends on 'browser', I'd probably have a dependency on
> sync here, with a comment about generated headers.

I've changed this to make the dependency more accurate and follow the pattern
Ryan described.  LMK if you think the updated version would benefit from a
comment here.

https://codereview.chromium.org/182313002/diff/1/extensions/extensions.gyp
File extensions/extensions.gyp (right):

https://codereview.chromium.org/182313002/diff/1/extensions/extensions.gyp#ne...
extensions/extensions.gyp:161: # (browser_extension already depends on
extensions_browser).
On 2014/03/07 19:13:00, Nico wrote:
> Same question as above.

No longer needed!

https://codereview.chromium.org/182313002/diff/70001/chrome/browser/history/t...
File chrome/browser/history/top_sites.h (right):

https://codereview.chromium.org/182313002/diff/70001/chrome/browser/history/t...
chrome/browser/history/top_sites.h:15: #include
"content/public/browser/notification_observer.h"
notification_observer.h comes from content/content.gyp:content_browser which is
already a dependency of browser_real (where top_sites.h lives) so this
dependency is already expressed in GYP files.

https://codereview.chromium.org/182313002/diff/70001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/182313002/diff/70001/chrome/chrome.gyp#newcod...
chrome/chrome.gyp:142: '../base/base.gyp:base_prefs',
Needed because android_rsa.cc now includes a header from base_prefs.

https://codereview.chromium.org/182313002/diff/70001/chrome/chrome_tests_unit...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/182313002/diff/70001/chrome/chrome_tests_unit...
chrome/chrome_tests_unit.gypi:15: 'browser_real',
Needed because ui_test_utils.h includes history_service.h (which is part of
browser_real).

https://codereview.chromium.org/182313002/diff/70001/chrome/chrome_tests_unit...
chrome/chrome_tests_unit.gypi:38: 'browser_real',
history_service.h (part of browser_real) includes sync/api/syncable_service.h so
we need to export browser_real's settings.

https://codereview.chromium.org/182313002/diff/70001/components/password_mana...
File components/password_manager.gypi (right):

https://codereview.chromium.org/182313002/diff/70001/components/password_mana...
components/password_manager.gypi:17: '../sync/sync.gyp:sync',
Needed because password_store.h includes sync/api/syncable_service.h.

[Nico, 2014-03-08 00:45:56]

https://codereview.chromium.org/182313002/diff/70001/chrome/chrome_tests_unit...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/182313002/diff/70001/chrome/chrome_tests_unit...
chrome/chrome_tests_unit.gypi:15: 'browser_real',
I think browser_real is supposed to be an implementation detail of the target
'browser' (as in, all 'browser' does is depend on 'browser_real' and export its
settings). So I'm not sure the changes in this file are necessary.

[maniscalco, 2014-03-08 01:26:59]

https://codereview.chromium.org/182313002/diff/70001/chrome/chrome_tests_unit...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/182313002/diff/70001/chrome/chrome_tests_unit...
chrome/chrome_tests_unit.gypi:15: 'browser_real',
On 2014/03/08 00:45:57, Nico wrote:
> I think browser_real is supposed to be an implementation detail of the target
> 'browser' (as in, all 'browser' does is depend on 'browser_real' and export
its
> settings). So I'm not sure the changes in this file are necessary.

You're right, this dependency on browser_real is redundant (browser does the
right thing and exports browser_real's dependent settings).  Removed.

However, I believe export_dependent_settings of browser below is necessary.  Of
course, I will confirm with trybots.

https://codereview.chromium.org/182313002/diff/70001/chrome/chrome_tests_unit...
chrome/chrome_tests_unit.gypi:38: 'browser_real',
On 2014/03/07 23:44:39, maniscalco wrote:
> history_service.h (part of browser_real) includes sync/api/syncable_service.h
so
> we need to export browser_real's settings.

Correction: we need to export browser's settings as we've dropped the redundant
dependency on browser_real.

[Nico, 2014-03-08 01:39:41]

Patch set 3 lgtm.

[maniscalco, 2014-03-10 23:43:08]

On 2014/03/08 01:39:41, Nico wrote:
> Patch set 3 lgtm.

Nico, thanks for the review.

Ryan, LMK if you want me to wait for your feedback. I plan to try to land this
tomorrow.

[Ryan Sleevi, 2014-03-10 23:45:18]

lgtm

[maniscalco, 2014-03-11 16:17:29]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-03-11 16:20:13]

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

[commit-bot: I haz the power, 2014-03-11 16:41:13]

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

[commit-bot: I haz the power, 2014-03-11 16:41:14]

Try jobs failed on following builders: linux_chromium_chromeos_clang_dbg

[maniscalco, 2014-03-11 17:57:44]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-03-11 18:06:49]

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

[commit-bot: I haz the power, 2014-03-11 20:45:35]

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

[commit-bot: I haz the power, 2014-03-11 20:45:37]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[maniscalco, 2014-03-11 21:10:05]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-03-11 21:58:36]

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

[commit-bot: I haz the power, 2014-03-11 23:25:49]

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

[commit-bot: I haz the power, 2014-03-11 23:25:50]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[maniscalco, 2014-03-17 18:42:25]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-03-17 18:43:04]

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

[commit-bot: I haz the power, 2014-03-17 19:59:23]

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

[commit-bot: I haz the power, 2014-03-17 19:59:24]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[maniscalco, 2014-03-17 23:34:43]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-03-17 23:35:57]

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

[commit-bot: I haz the power, 2014-03-18 00:52:19]

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

[commit-bot: I haz the power, 2014-03-18 00:52:21]

Retried try job too often on win_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[maniscalco, 2014-03-18 16:13:07]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-03-18 16:13:52]

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

[commit-bot: I haz the power, 2014-03-18 17:08:52]

Change committed as 257666

[maniscalco, 2014-03-19 00:28:16]

A revert of this CL has been created in
https://chromiumcodereview.appspot.com/196423023/ by maniscalco@chromium.org.

The reason for reverting is: Broke win_x86_rel trybots about two thirds of the
time (can't find sync.pb.h).  Appears that the build is non-deterministic, gonna
need more testing..