I'm not 100% sure we should preprocess with the C preprocessor itself. I think we probably want to limit this to #if / #ifdef / #elif / #else / #endif / defined() / #define (?). I'm pretty sure we don't want C preprocessor macros.

Another open question is how we'll pass the list of build defines for the bindings generator to understand.

Though I guess it's worth noting that buildflags currently require macro evaluation, and that's something we probably want to support.

 Issue 671901  has been merged into this issue.

$ grep "defined(" $(git ls-files | grep -P "_messages.(cc|h)|_param_traits.(cc|h)") | grep -v // | cut -d: -f2 | sort | uniq
#elif defined(OS_MACOSX)
#if BUILDFLAG(ENABLE_PRINTING) && defined(OS_WIN)
#if !defined(FULL_SAFE_BROWSING)
#if defined(FULL_SAFE_BROWSING)
#if defined(OS_ANDROID)
#if defined(OS_LINUX)
#if defined(OS_LINUX) || defined(OS_NACL_NONSFI)
#if defined(OS_MACOSX)
#if defined(OS_MACOSX) && !defined(OS_IOS)
#if !defined(OS_NACL) && !defined(NACL_WIN64)
#if defined(OS_POSIX)
#if defined(OS_WIN)
#if defined(WEBRTC_WIN)

$ grep "#ifdef" $(git ls-files | grep -P "_messages.(cc|h)|_param_traits.(cc|h)") | grep -v // 
| cut -d: -f2 | sort | uniq
#ifdef IPC_MESSAGE_START
#ifdef SK_MSCALAR_IS_FLOAT

So it seems like this is not needed a lot. Both yzshen and nverne suggested having a simple annotation-based syntax. My proposal:
- Add a new EnabledIf=<flag> annotation to mojom syntax
- Populate a default set of flags based on target OS.
- Add a conditional_flags argument to the GN mojom template
- Use this to express other conditionals (including anything that requires !, ||, or &&).

Tricky cases:
- SK_MSCALAR_IS_FLOAT: configured in a header file. It's probably OK to dupe this somewhere if it's really needed--though it appears that this should always be set for Chrome builds anyway? So it's probably not needed.
- NACL_WIN64: maybe this is set by the NaCl toolchain? The only place that explicitly defines it is //components/nacl/broker, but it's used in //ppapi/proxy.
- FULL_SAFE_BROWSING: will probably become a buildflag (and has a corresponding GN variable safe_browsing_mode, so this one is easy to port)
- BUILDFLAG(ENABLE_PRINTING): buildflag, so just use the corresponding GN variable directly
- WEBRTC_WIN: Used in Chromoting but defined in //third_party/webrtc's build config? Probably solvable...

RE #10: This sounds good to me. Thanks for looking into the details!

Going to hand this off to the Sydney team. I've uploaded the WIP CL: https://chromium-review.googlesource.com/c/chromium/src/+/783704

The current CL removes disabled definitions at parse time. However, this is problematic--the mojom template EnabledIf arguments are local to each target. If a mojom file (bar.mojom) is imported by another mojo file (foo.mojom), foo.mojom doesn't know about the EnabledIf flags if bar.mojom is defined in a different target. This is problematic at build time, because different mojoms will try to import a different view of the mojom.

To solve this, yzshen and I discussed splitting mojom code generation into two GN steps:
1. The first step parses the source mojom into a parse tree. We remove any disabled definitions from the parse tree at this stage, and then pickle the pruned parse tree into gen/.
2. The second step performs the actual code generation. In this step, we unpickle the previously generated parse trees. Imports are resolved using the pickled parse trees.

At a high-level, I think this means:
- Changing the GN template for mojom to express these two distinct stages
- Refactor MojomProcessor to separate the parsing and generation steps.
  - For parsing, _ParseFileAndImport probably doesn't need to parse imports now. Instead, this step can be changed to pickle the resulting parse tree directly into the gen/ dir.
  - For code generation, _GenerateModule() needs to be changed to unpickle the parse tree from the gen/ dir as well as any imports used by the mojom.
- Update the mojom compiler to expose these separate stages to GN

(For historical purposes, the initial version of the CL is also at https://chromium-review.googlesource.com/c/chromium/src/+/695002. It used a different approach that was simpler, but had issues of its own.)

watk: nice to see you again here :)

Hey :)

For anyone following along: I'm hosting an intern who will be picking this up in two weeks.

The assigned owner "evem@chromium.org" is not able to receive e-mails, please re-triage.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Hi, Chris. Please see comment #19. Is evem@ the correct email address?

evem@chromium.org is the right address.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f83c71b725032a95451bb9cbaa99fe8de2f9ca5e

commit f83c71b725032a95451bb9cbaa99fe8de2f9ca5e
Author: Eve Martin-Jones <evem@chromium.org>
Date: Thu Feb 01 20:22:45 2018

Support EnableIf attribute to conditionally disable definitions.

Add support for a new EnableIf attribute to conditionally
enable fields/types in mojom definitions.

In order to do this, mojom parsing and code generation has
been split into two separate GN stages. The mojom bindings
generator has been refactored to separate the logic for
parsing and code generation and the GN mojom template has
been updated to express these two distinct stages.

The parse stage now prunes a mojom's AST - filtering
definitions based on the enabled features. These
intermediate ASTs are then pickled to gen/ to be later
read by the code generation stage.

Bug: 676224
Change-Id: I5fc106b43e8ac48339c63c48f7ce42ba5145d174
Reviewed-on: https://chromium-review.googlesource.com/846399
Commit-Queue: Eve Martin-Jones <evem@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Yuzhu Shen <yzshen@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Chris Watkins <watk@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533796}
[modify] https://crrev.com/f83c71b725032a95451bb9cbaa99fe8de2f9ca5e/mojo/public/tools/bindings/README.md
[modify] https://crrev.com/f83c71b725032a95451bb9cbaa99fe8de2f9ca5e/mojo/public/tools/bindings/mojom.gni
[modify] https://crrev.com/f83c71b725032a95451bb9cbaa99fe8de2f9ca5e/mojo/public/tools/bindings/mojom_bindings_generator.py
[add] https://crrev.com/f83c71b725032a95451bb9cbaa99fe8de2f9ca5e/mojo/public/tools/bindings/pylib/mojom/parse/conditional_features.py
[add] https://crrev.com/f83c71b725032a95451bb9cbaa99fe8de2f9ca5e/mojo/public/tools/bindings/pylib/mojom_tests/parse/conditional_features_unittest.py

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0a7a4ec60acbbb856c1d762f6f73dc3319652e25

commit 0a7a4ec60acbbb856c1d762f6f73dc3319652e25
Author: Daniel Cheng <dcheng@chromium.org>
Date: Tue Feb 13 23:30:27 2018

Update Mojo security best practices to document EnableIf.

Bug: 676224
Change-Id: I3501027857320f6eace3aea9e53939807a2a618e
Reviewed-on: https://chromium-review.googlesource.com/910029
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Chris Palmer <palmer@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536545}
[modify] https://crrev.com/0a7a4ec60acbbb856c1d762f6f73dc3319652e25/docs/security/mojo.md

One question: how to conditionally import another mojom file?

For example:

bar.mojom:

[EnableIf=is_mac]
import foo.mojom

interface Bar {
  Func(
    [EnableIf=is_mac]
    Foo foo
  );
}

But I just tried. It seems EnableIf doesn't work with "import". Any recommendations how to solve the problem? Or shall I just import foo.mojom unconditionally?

Mojom annotations (i.e. using the []) aren't supported for imports meaning that you won't be able to use the EnableIf attribute to conditionally import another mojom.

I've made a CL to support attribute lists for mojom imports - https://chromium-review.googlesource.com/c/chromium/src/+/934004 - so if there's no reason why they shouldn't have attribute lists we can start supporting them and then EnableIf will work :)

However, I'm an intern and today is my last day so I'm not sure if I'll personally be able to land this change but maybe someone else can once I'm gone :)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/927f318c2e616e48b600d03200f6cd6ea4f377a6

commit 927f318c2e616e48b600d03200f6cd6ea4f377a6
Author: Daniel Cheng <dcheng@chromium.org>
Date: Fri Feb 23 10:24:42 2018

Guard GpuInfo::dx_diagnostics with EnableIf=is_win

Bug: 676224
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I93ac557f666c181a76e4057c49ba85c8a1b1e456
Reviewed-on: https://chromium-review.googlesource.com/933282
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538750}
[modify] https://crrev.com/927f318c2e616e48b600d03200f6cd6ea4f377a6/gpu/ipc/common/gpu_info.mojom
[modify] https://crrev.com/927f318c2e616e48b600d03200f6cd6ea4f377a6/gpu/ipc/common/gpu_info_struct_traits.h

evem: Thank you so much for the info! Really nice job done!

watk: Any plan to support conditional imports? Seems like it's a missing feature for EnableIf, e.g. my CL cannot land without it:
https://chromium-review.googlesource.com/c/chromium/src/+/933842/2/media/mojo/interfaces/cdm_service.mojom#9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/55b8e2e73a7448a10a8b751c450eb636b2abb7d3

commit 55b8e2e73a7448a10a8b751c450eb636b2abb7d3
Author: Eve Martin-Jones <evem@chromium.org>
Date: Fri Feb 23 23:08:21 2018

Support attribute lists for mojom imports.

Update the definition of the Import class in
//mojo/public/tools/bindings/pylib/mojom/parse/ast.py to have
an attribute list.

Update the grammar in
//mojo/public/tools/bindings/pylib/mojo/parse/parser.py so that
imports are now preceded by an attribute_section.

Update existing tests in parser_unittests.py and add
a unit test for Imports with attributes to
conditional_features_unittest.py

Bug: 676224
Change-Id: If347ef41f9149d3587db1f79abba84ccf552dbc2
Reviewed-on: https://chromium-review.googlesource.com/934004
Commit-Queue: Eve Martin-Jones <evem@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538937}
[modify] https://crrev.com/55b8e2e73a7448a10a8b751c450eb636b2abb7d3/mojo/public/tools/bindings/pylib/mojom/parse/ast.py
[modify] https://crrev.com/55b8e2e73a7448a10a8b751c450eb636b2abb7d3/mojo/public/tools/bindings/pylib/mojom/parse/conditional_features.py
[modify] https://crrev.com/55b8e2e73a7448a10a8b751c450eb636b2abb7d3/mojo/public/tools/bindings/pylib/mojom/parse/parser.py
[modify] https://crrev.com/55b8e2e73a7448a10a8b751c450eb636b2abb7d3/mojo/public/tools/bindings/pylib/mojom_tests/parse/conditional_features_unittest.py
[modify] https://crrev.com/55b8e2e73a7448a10a8b751c450eb636b2abb7d3/mojo/public/tools/bindings/pylib/mojom_tests/parse/parser_unittest.py

xhwang: There you go, conditional imports should work now! Let me know if you come across any more problems and I'll have a look :)

evem: Just tried it and it's working! Thank you so much!

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/688f49d345f78f475b30c9455433a796f4b99b3d

commit 688f49d345f78f475b30c9455433a796f4b99b3d
Author: Xiaohan Wang <xhwang@chromium.org>
Date: Mon Feb 26 19:39:53 2018

Update EnableIf documentation on imports

Now EnableIf works with imports. Update the documentation on this.

BUG=676224

Change-Id: Ib3cee7bbca3584fe1b537afc26b31a10d8d51dec
Reviewed-on: https://chromium-review.googlesource.com/937943
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539237}
[modify] https://crrev.com/688f49d345f78f475b30c9455433a796f4b99b3d/docs/security/mojo.md

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/86bee0e84d0dea2f05c689550ca965f4be2f3d52

commit 86bee0e84d0dea2f05c689550ca965f4be2f3d52
Author: Xiaohan Wang <xhwang@chromium.org>
Date: Tue Feb 27 00:44:44 2018

media: Merge cdm_service_mac.mojom into cdm_service.mojom

Now with EnableIf we can merge these two.

BUG= 771791 ,676224

Change-Id: I5c2cdbb1712b3761fb4d09ffcb88ca86ee69a909
Reviewed-on: https://chromium-review.googlesource.com/933842
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: John Rummell <jrummell@chromium.org>
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539353}
[modify] https://crrev.com/86bee0e84d0dea2f05c689550ca965f4be2f3d52/content/browser/media/media_interface_proxy.cc
[modify] https://crrev.com/86bee0e84d0dea2f05c689550ca965f4be2f3d52/media/mojo/interfaces/BUILD.gn
[modify] https://crrev.com/86bee0e84d0dea2f05c689550ca965f4be2f3d52/media/mojo/interfaces/cdm_service.mojom
[delete] https://crrev.com/6fe938788694fa992ad5c56e087e3539b9637a3a/media/mojo/interfaces/cdm_service_mac.mojom
[modify] https://crrev.com/86bee0e84d0dea2f05c689550ca965f4be2f3d52/media/mojo/services/cdm_service.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a75606f0da03cd529fc5dd5d415047cef793e02e

commit a75606f0da03cd529fc5dd5d415047cef793e02e
Author: Daniel Cheng <dcheng@chromium.org>
Date: Tue Mar 13 05:01:46 2018

[EnableIf] Guard some more mojo methods in //content/common

Bug: 676224
Change-Id: I9b0c8266d1b45fa0371cea4fc0cd94e9909b6534
Reviewed-on: https://chromium-review.googlesource.com/957863
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542734}
[modify] https://crrev.com/a75606f0da03cd529fc5dd5d415047cef793e02e/content/browser/renderer_host/render_message_filter.cc
[modify] https://crrev.com/a75606f0da03cd529fc5dd5d415047cef793e02e/content/browser/renderer_host/render_message_filter.h
[modify] https://crrev.com/a75606f0da03cd529fc5dd5d415047cef793e02e/content/child/child_thread_impl.cc
[modify] https://crrev.com/a75606f0da03cd529fc5dd5d415047cef793e02e/content/child/child_thread_impl.h
[modify] https://crrev.com/a75606f0da03cd529fc5dd5d415047cef793e02e/content/common/BUILD.gn
[modify] https://crrev.com/a75606f0da03cd529fc5dd5d415047cef793e02e/content/common/child_control.mojom
[modify] https://crrev.com/a75606f0da03cd529fc5dd5d415047cef793e02e/content/common/render_message_filter.mojom
[modify] https://crrev.com/a75606f0da03cd529fc5dd5d415047cef793e02e/content/common/renderer.mojom
[modify] https://crrev.com/a75606f0da03cd529fc5dd5d415047cef793e02e/content/public/test/mock_render_thread.cc
[modify] https://crrev.com/a75606f0da03cd529fc5dd5d415047cef793e02e/ipc/BUILD.gn
[add] https://crrev.com/a75606f0da03cd529fc5dd5d415047cef793e02e/ipc/features.gni

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b5e470c2b687c1a02d8e2e977b8bf0d7f2a9a662

commit b5e470c2b687c1a02d8e2e977b8bf0d7f2a9a662
Author: Sam McNally <sammc@chromium.org>
Date: Tue May 22 23:48:21 2018

Change FilePath to use EnableIf instead of native typemapping.

Bug: 676224
Change-Id: I101d138ac8ca72815cc6aa0a1b5ad4218b3c3001
Reviewed-on: https://chromium-review.googlesource.com/1065753
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Sam McNally <sammc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560839}
[modify] https://crrev.com/b5e470c2b687c1a02d8e2e977b8bf0d7f2a9a662/chrome/services/util_win/public/mojom/shell_util_win.typemap
[modify] https://crrev.com/b5e470c2b687c1a02d8e2e977b8bf0d7f2a9a662/mojo/public/cpp/base/BUILD.gn
[modify] https://crrev.com/b5e470c2b687c1a02d8e2e977b8bf0d7f2a9a662/mojo/public/cpp/base/file_path.typemap
[add] https://crrev.com/b5e470c2b687c1a02d8e2e977b8bf0d7f2a9a662/mojo/public/cpp/base/file_path_mojom_traits.cc
[add] https://crrev.com/b5e470c2b687c1a02d8e2e977b8bf0d7f2a9a662/mojo/public/cpp/base/file_path_mojom_traits.h
[modify] https://crrev.com/b5e470c2b687c1a02d8e2e977b8bf0d7f2a9a662/mojo/public/cpp/base/file_path_unittest.cc
[modify] https://crrev.com/b5e470c2b687c1a02d8e2e977b8bf0d7f2a9a662/mojo/public/mojom/base/BUILD.gn
[modify] https://crrev.com/b5e470c2b687c1a02d8e2e977b8bf0d7f2a9a662/mojo/public/mojom/base/file_path.mojom
[modify] https://crrev.com/b5e470c2b687c1a02d8e2e977b8bf0d7f2a9a662/ui/display/mojo/display_snapshot_struct_traits.cc
[modify] https://crrev.com/b5e470c2b687c1a02d8e2e977b8bf0d7f2a9a662/ui/display/mojo/display_struct_traits_unittest.cc