New build flag system, convert Google Now flag

New build flag system, convert Google Now flag

This generates headers with build flags rather than forcing them all to be global. It includes an accessor wrapper so that references to the flags will fail if the proper header is not included.

Converts Google Now to use this and remove the global google now define and grit define.

Adds support for grit define values of "true" and "false" for ease of integration with GN (they are mapped to the corresponding Python "True" and "False").

Adds dependencies from the main gyp targets to the new generated feature define target. Since GYP only does hard dependencies one level, this should reduce the chance that somebody adds more of these cases and forces to add a dependency.

Committed: https://crrev.com/3118dde528359280fa0cb3c6fc5b6323e834c514
Cr-Commit-Position: refs/heads/master@{#361527}

[brettw, 2015-11-18 23:06:53]

Description was changed from

==========
New build flag proof of concept

BUG=
==========

to

==========
New build flag system, convert google now

This generates headers with build flags rather than forcing them all to be
global. It includes an accessor wrapper so that references to the flags will
fail if the proper header is not included.

Converts Google Now to use this and remove the global google now define and grit
define.

Adds support for grit define values of "true" and "false" for ease of
integration with GN (they are mapped to the corresponding Python "True" and
"False").

BUG=
==========

[brettw, 2015-11-18 23:24:02]

Description was changed from

==========
New build flag system, convert google now

This generates headers with build flags rather than forcing them all to be
global. It includes an accessor wrapper so that references to the flags will
fail if the proper header is not included.

Converts Google Now to use this and remove the global google now define and grit
define.

Adds support for grit define values of "true" and "false" for ease of
integration with GN (they are mapped to the corresponding Python "True" and
"False").

BUG=
==========

to

==========
New build flag system, convert google now

This generates headers with build flags rather than forcing them all to be
global. It includes an accessor wrapper so that references to the flags will
fail if the proper header is not included.

Converts Google Now to use this and remove the global google now define and grit
define.

Adds support for grit define values of "true" and "false" for ease of
integration with GN (they are mapped to the corresponding Python "True" and
"False").

Adds dependencies from the main gyp targets to the new generated feature define
target. Since GYP only does hard dependencies one level, this should reduce the
chance that somebody adds more of these cases and forces to add a dependency.
==========

[brettw, 2015-11-18 23:24:15]

brettw@chromium.org changed reviewers:
+ mcolbert@chromium.org

[brettw, 2015-11-18 23:24:26]

brettw@chromium.org changed reviewers:
+ mark@chromium.org
- mcolbert@chromium.org

[brettw, 2015-11-18 23:25:05]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-18 23:25:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1458653002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1458653002/60001

[chromium-reviews, 2015-11-18 23:27:04]

Wrong Mark I think?

On Wed, Nov 18, 2015 at 3:24 PM <brettw@chromium.org> wrote:

> Reviewers: Mark
> CL: https://codereview.chromium.org/1458653002/
>
> Description:
> New build flag system, convert google now
>
> This generates headers with build flags rather than forcing them all to be
> global. It includes an accessor wrapper so that references to the flags
> will
> fail if the proper header is not included.
>
> Converts Google Now to use this and remove the global google now define and
> grit
> define.
>
> Adds support for grit define values of "true" and "false" for ease of
> integration with GN (they are mapped to the corresponding Python "True" and
> "False").
>
> Adds dependencies from the main gyp targets to the new generated feature
> define
> target. Since GYP only does hard dependencies one level, this should reduce
> the
> chance that somebody adds more of these cases and forces to add a
> dependency.
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+521, -38 lines):
>    A build/build_header.h
>    A build/build_header.gni
>    A build/build_header.gypi
>    M build/common.gypi
>    M build/config/BUILD.gn
>    M build/config/features.gni
>    M build/gypi_to_gn.py
>    A build/write_build_header.py
>    M chrome/browser/BUILD.gn
>    M chrome/browser/extensions/component_loader.cc
>    M chrome/browser/profiles/profile.cc
>    M chrome/browser/resources/BUILD.gn
>    M chrome/browser/ui/BUILD.gn
>    M chrome/browser/ui/webui/options/content_settings_handler.cc
>    M chrome/browser/ui/webui/options/options_ui.cc
>    M chrome/chrome_android.gypi
>    M chrome/chrome_browser.gypi
>    M chrome/chrome_browser_extensions.gypi
>    M chrome/chrome_browser_ui.gypi
>    M chrome/chrome_child.gypi
>    M chrome/chrome_common.gypi
>    M chrome/chrome_debugger.gypi
>    M chrome/chrome_exe.gypi
>    A chrome/chrome_features.gypi
>    M chrome/chrome_plugin.gypi
>    M chrome/chrome_renderer.gypi
>    M chrome/chrome_resources.gyp
>    M chrome/chrome_tests.gypi
>    M chrome/chrome_tests_unit.gypi
>    M chrome/chrome_utility.gypi
>    M chrome/common/BUILD.gn
>    A chrome/common/features.gni
>    M chrome/common/pref_names.h
>    M chrome/common/pref_names.cc
>    M tools/grit/grit/util.py
>    M tools/grit/grit_rule.gni
>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brettw, 2015-11-18 23:32:38]

Yes, sorry, already fixed (autocomplete fail).

[commit-bot: I haz the power, 2015-11-18 23:38:56]

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

[commit-bot: I haz the power, 2015-11-18 23:38:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[brettw, 2015-11-19 05:54:31]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-19 05:55:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1458653002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1458653002/60001

[commit-bot: I haz the power, 2015-11-19 05:59:36]

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

[commit-bot: I haz the power, 2015-11-19 05:59:36]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)

[brettw, 2015-11-19 18:54:07]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-19 18:55:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1458653002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1458653002/80001

[commit-bot: I haz the power, 2015-11-19 19:01:34]

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

[commit-bot: I haz the power, 2015-11-19 19:01:34]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)

[brettw, 2015-11-19 19:28:29]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-19 19:28:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1458653002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1458653002/100001

[commit-bot: I haz the power, 2015-11-19 19:33:42]

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

[commit-bot: I haz the power, 2015-11-19 19:33:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[brettw, 2015-11-19 20:30:05]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-19 20:31:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1458653002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1458653002/120001

[commit-bot: I haz the power, 2015-11-19 20:43:45]

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

[commit-bot: I haz the power, 2015-11-19 20:43:46]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)

[Mark Mentovai, 2015-11-19 21:52:56]

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.gni
File build/build_header.gni (right):

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.gni...
build/build_header.gni:5: # Generates a header with preprocessor defines
specified by the build file.
The gypi version points to here, which is a good hint that the two need to be
kept in sync. This file should also point to the gypi.

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.gni...
build/build_header.gni:87: #       "SPAM_SERVER_URL=\"http://www.example.com/",
You probably want a \" on the end too.

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.gni...
build/build_header.gni:100: # Compute the path from teh root to this file.
teh

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.gypi
File build/build_header.gypi (right):

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.gyp...
build/build_header.gypi:10: # IN EVERY TARGET THAT USES YOUR GENERATED HEADER
you must include a dependency
WHY ARE YOU YELLING AT ME?

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.gyp...
build/build_header.gypi:15: # Assuming your GN code looks like this:
It’d be helpful if this matched the GN example exactly, with the space laser and
all.

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.gyp...
build/build_header.gypi:20: #     variables = [
"MY_URL=\"http://www.example.com/" ]
\" as before. You got this right in the GYP example.

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.gyp...
build/build_header.gypi:23: # Write a GYP target like this:
It’d be useful to know that you’re expecting to see this in foo/bar/foobar.gyp
or whatever.

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.gyp...
build/build_header.gypi:30: #       'build_header_path':
'foo/bar/foo_features.h',
You’d be more likely to find bar_featues.h or foobar_features.h at this path.

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.gyp...
build/build_header.gypi:76: 'type': 'none',
Why do this in a separate target? If you write this so that can be included by
the target that wants to generate the header, the including target can be
responsible for generating its header itself, without the need to express any
extra dependencies. It can decide to not be hard if its header is private to the
target, for example. If some consumer does want these semantics, then they can
write things exactly as you’ve got here, with the addition of having a local
'type': 'none', 'hard_dependency': 1.

Since one motivator here is to keep private flags isolated to the target or
targets that need them, I suspect that many flags that are used in a small
number of places actually are all isolated to a single target, where it’s fine
to just run the action right in the target itself and not try to expose it to
dependents.

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.gyp...
build/build_header.gypi:90: # on the name of this target.
Where does this file live? Hopefully not in the source tree.

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.h
File build/build_header.h (right):

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.h#n...
build/build_header.h:1: // Copyright 2015 The Chromium Authors. All rights
reserved.
If this defines BUILDFLAG() as the primary macro, the file should be named
build_flag.h or similar.

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.h#n...
build/build_header.h:5: #ifndef BASE_BUILD_HEADER_H_
BUILD_BUILD_HEADER_H_ here and in two other places.

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.h#n...
build/build_header.h:16: #define BUILDFLAG_CAT2(a, b) BUILDFLAG_CAT(a, b)
I recognize this pattern (although I doubt most developers would), but I don’t
actually think it’s necessary here.

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.h#n...
build/build_header.h:23: // Accessor for boolean values. Throws an error if
undefined. Since the
“if flag is undefined”

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.h#n...
build/build_header.h:24: // invocation of a #define can't normally throw an
error, emulate that by
of a macro

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.h#n...
build/build_header.h:26: #define BUILDFLAG(flag)
(BUILDFLAG_CAT2(BUILDFLAG_FLAG_VALUE_, flag) /
BUILDFLAG_CAT2(BUILDFLAG_DEFINE_CHECK_, BUILDFLAG_CAT2(BUILDFLAG_FLAG_VALUE_,
flag)))  // If you see a division by 0 it means your flag is undefined and
you're missing the header.
Can we name this CR_BUILDFLAG to provide some semblance of namespacing?

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.h#n...
build/build_header.h:26: #define BUILDFLAG(flag)
(BUILDFLAG_CAT2(BUILDFLAG_FLAG_VALUE_, flag) /
BUILDFLAG_CAT2(BUILDFLAG_DEFINE_CHECK_, BUILDFLAG_CAT2(BUILDFLAG_FLAG_VALUE_,
flag)))  // If you see a division by 0 it means your flag is undefined and
you're missing the header.
I believe that this can be simplified by using defined() for the denominator:

#define BUILDFLAG(flag) ((BUILDFLAG_FLAG_VALUE_ ## flag) /
defined(BUILDFLAG_FLAG_VALUE_ ## flag))

You shouldn’t need BUILDFLAG_DEFINE_CHECK_0/1. This will make BUILDFLAG() work
equally well for BUILDFLAG_FLAG_VALUE_* values of 0 and 1 (as you have here) and
2 and -1 and everything else.

This won’t work if you intended to use BUILDFLAG() in C++ expressions and not
just preprocessor expressions, because defined() is meaningless to C++. If the
comments here had some example usage, your intentions for how these are to be
used would be clearer.

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.h#n...
build/build_header.h:29: #define BUILDVAR(var) (BUILDFLAG_VAR_VALUE_##var)
Having both BUILDFLAG_FLAG_VALUE_* and BUILDFLAG_VAR_VALUE_* is a little
confusing. I think we can unify behind one of them.

If we do have cases where you need to get at a value for use in C++ expressions,
we should be able to get at flags. For cases where you need to get a value in a
preprocessor expression, use CR_BUILDFLAG(). For cases where you need to get a
value for use in C++, use CR_BUILDFLAG_VALUE(). The comments here should explain
this and give some example usage.

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.h#n...
build/build_header.h:31: #endif  // BASE_BUILD_HEADER_H_
After I read this file, I saw that you had more documentation-like stuff in the
gni file. OK, but this file should point to that then, or the more C++-oriented
stuff should live in this file. People having a hard time with BUILDFLAG() won’t
think to look at that gn file first, they’ll probably look for the file that
defines BUILDFLAG().

https://codereview.chromium.org/1458653002/diff/120001/build/write_build_head...
File build/write_build_header.py (right):

https://codereview.chromium.org/1458653002/diff/120001/build/write_build_head...
build/write_build_header.py:36: parser.add_option('--path-for-guard',
Might be nice if this accepted the base directory and the path within the base
directory as separate arguments, basing the guard on the latter, and doing a
path join to form the output path. It’s less redundancy in the arguments to the
script, which makes it harder to screw up usage.

https://codereview.chromium.org/1458653002/diff/120001/build/write_build_head...
build/write_build_header.py:46: header_guard.replace('/', '_').replace('\\',
'_').replace('.', '_')
Three replaces in a row, maybe better as an re.sub.

https://codereview.chromium.org/1458653002/diff/120001/build/write_build_head...
build/write_build_header.py:65: value = 1
Stick with '1' or use str(value) when you flags.append() to make sure
everything’s always a str, and not rely on every user of flags to have to treat
it as a str on the way out.

https://codereview.chromium.org/1458653002/diff/120001/build/write_build_head...
build/write_build_header.py:89: output_file.write("/* Generated by
build/write_build_header.py */\n")
// comments like we usually use?

https://codereview.chromium.org/1458653002/diff/120001/build/write_build_head...
build/write_build_header.py:91: output_file.write("/* From " + options.rulename
+ " */\n")
I’m inclined to deal with jokers by scrubbing newlines and */s and other things
that might cause unpredictable results.

https://codereview.chromium.org/1458653002/diff/120001/build/write_build_head...
build/write_build_header.py:100: output_file.write('#define
BUILDFLAG_VAR_VALUE_%s %s\n' % pair)
Similarly, this should make sure there are no newlines, comment characters,
trailing backslashes…

https://codereview.chromium.org/1458653002/diff/120001/chrome/browser/profile...
File chrome/browser/profiles/profile.cc (right):

https://codereview.chromium.org/1458653002/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile.cc:101: #if BUILDFLAG(ENABLE_GOOGLE_NOW)
Include what you use.

https://codereview.chromium.org/1458653002/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

https://codereview.chromium.org/1458653002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/content_settings_handler.cc:454: #if
BUILDFLAG(ENABLE_GOOGLE_NOW)
Here too.

[Mark Mentovai, 2015-11-19 21:53:26]

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.h
File build/build_header.h (right):

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.h#n...
build/build_header.h:1: // Copyright 2015 The Chromium Authors. All rights
reserved.
In the CL description, capitalize Google Now, because otherwise, it sounds like
you’re converting Google, and you’re doing it now.

[brettw, 2015-11-19 22:23:34]

Description was changed from

==========
New build flag system, convert google now

This generates headers with build flags rather than forcing them all to be
global. It includes an accessor wrapper so that references to the flags will
fail if the proper header is not included.

Converts Google Now to use this and remove the global google now define and grit
define.

Adds support for grit define values of "true" and "false" for ease of
integration with GN (they are mapped to the corresponding Python "True" and
"False").

Adds dependencies from the main gyp targets to the new generated feature define
target. Since GYP only does hard dependencies one level, this should reduce the
chance that somebody adds more of these cases and forces to add a dependency.
==========

to

==========
New build flag system, convert Google Now flag

This generates headers with build flags rather than forcing them all to be
global. It includes an accessor wrapper so that references to the flags will
fail if the proper header is not included.

Converts Google Now to use this and remove the global google now define and grit
define.

Adds support for grit define values of "true" and "false" for ease of
integration with GN (they are mapped to the corresponding Python "True" and
"False").

Adds dependencies from the main gyp targets to the new generated feature define
target. Since GYP only does hard dependencies one level, this should reduce the
chance that somebody adds more of these cases and forces to add a dependency.
==========

[brettw, 2015-11-20 00:02:37]

This addresses the comments up to the python file. I had a few questions and
responses. Will do the python and .cc file comments tomorrow.

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.gypi
File build/build_header.gypi (right):

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.gyp...
build/build_header.gypi:10: # IN EVERY TARGET THAT USES YOUR GENERATED HEADER
you must include a dependency
On 2015/11/19 21:52:55, Mark Mentovai wrote:
> WHY ARE YOU YELLING AT ME?

Done.

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.gyp...
build/build_header.gypi:15: # Assuming your GN code looks like this:
But GYP doesn't support the space laser!

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.gyp...
build/build_header.gypi:20: #     variables = [
"MY_URL=\"http://www.example.com/" ]
On 2015/11/19 21:52:55, Mark Mentovai wrote:
> \" as before. You got this right in the GYP example.

Done.

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.gyp...
build/build_header.gypi:23: # Write a GYP target like this:
On 2015/11/19 21:52:55, Mark Mentovai wrote:
> It’d be useful to know that you’re expecting to see this in foo/bar/foobar.gyp
> or whatever.

I renamed the target too foo_features. I left out the file it should go in. The
files between GYP and GN are often quite different (GN has lots more little
ones). I this people will be able to find the most appropriate file for
foo-related stuff to put the foo header target in.

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.gyp...
build/build_header.gypi:30: #       'build_header_path':
'foo/bar/foo_features.h',
Renamed to just foo/foo_features.h

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.gyp...
build/build_header.gypi:76: 'type': 'none',
I considered the action thing but I did this on purpose.

I was going for their being as close to a 1:1 correspondence between GYP and GN
here, and in GN all actions are targets. I want to be able to automatically
force the target to be a hard dependency, and I want people to have to add
dependencies in the same way. Since this is a generated file, it means marking
the target as a hard dependency, which is not good in general for many typical
places this would hang off of (like chrome_common or whatever).

My main worry is that people will forget to add dependencies so that the header
isn't generated in time. I think having a separate target with a name that
matches GN will help with this.

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.gyp...
build/build_header.gypi:90: # on the name of this target.
On 2015/11/19 21:52:55, Mark Mentovai wrote:
> Where does this file live? Hopefully not in the source tree.

I spent a long time on this. This <| thing seems to write it into out/gypfiles,
and it will break if you specify a path. This was confusing to me but it sort of
makes sense since it will be referenced from both out/Debug and out/Release so
can't live in either.

I didn't comment this since it's just a built-in GYP thing that's not commented
in any of the other places this feature is used.

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.h
File build/build_header.h (right):

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.h#n...
build/build_header.h:1: // Copyright 2015 The Chromium Authors. All rights
reserved.
On 2015/11/19 21:52:55, Mark Mentovai wrote:
> If this defines BUILDFLAG() as the primary macro, the file should be named
> build_flag.h or similar.

It defines BUILDFLAG and BUILDVAR, and I was generalizing this class of thing as
build headers. This will normally be included only by the generated "build
headers" (I added a comment to this effect).

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.h#n...
build/build_header.h:16: #define BUILDFLAG_CAT2(a, b) BUILDFLAG_CAT(a, b)
On 2015/11/19 21:52:55, Mark Mentovai wrote:
> I recognize this pattern (although I doubt most developers would), but I don’t
> actually think it’s necessary here.

This stuff is all removed now. Yay!

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.h#n...
build/build_header.h:26: #define BUILDFLAG(flag)
(BUILDFLAG_CAT2(BUILDFLAG_FLAG_VALUE_, flag) /
BUILDFLAG_CAT2(BUILDFLAG_DEFINE_CHECK_, BUILDFLAG_CAT2(BUILDFLAG_FLAG_VALUE_,
flag)))  // If you see a division by 0 it means your flag is undefined and
you're missing the header.
On 2015/11/19 21:52:55, Mark Mentovai wrote:
> Can we name this CR_BUILDFLAG to provide some semblance of namespacing?

I thought about CR_* and decided against it. I can add it if you feel strongly.
My reasoning was that one of the goals of this is to make the build directory
reusable by v8 and nacl, etc. and currently the main impediment is the global
defines we add. So this is more "build" than "chrome". Also, I think CR_ looks
ugly and BUILDFLAG is hopefully unique enough (I thought of some simpler ones
like ENABLE or USE like WebKit uses, but they obviously fail this test).

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.h#n...
build/build_header.h:26: #define BUILDFLAG(flag)
(BUILDFLAG_CAT2(BUILDFLAG_FLAG_VALUE_, flag) /
BUILDFLAG_CAT2(BUILDFLAG_DEFINE_CHECK_, BUILDFLAG_CAT2(BUILDFLAG_FLAG_VALUE_,
flag)))  // If you see a division by 0 it means your flag is undefined and
you're missing the header.
This works. I like your way.

https://codereview.chromium.org/1458653002/diff/120001/build/write_build_head...
File build/write_build_header.py (right):

https://codereview.chromium.org/1458653002/diff/120001/build/write_build_head...
build/write_build_header.py:89: output_file.write("/* Generated by
build/write_build_header.py */\n")
On 2015/11/19 21:52:56, Mark Mentovai wrote:
> // comments like we usually use?

We do have C files floating around for various reasons I wanted to be able to
include these from those files, and since this was generated I didn't think it
would be weird since people won't normally be looking at them. However, the
build_header.h doesn't use C comments. I'll fix that to use C comments if it's
OK with you. If you feel strongly this should all be C++ only, I can change this
file instead.

[brettw, 2015-11-20 00:03:10]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-20 00:06:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1458653002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1458653002/140001

[commit-bot: I haz the power, 2015-11-20 00:18:30]

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

[commit-bot: I haz the power, 2015-11-20 00:18:30]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, no build
URL)
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, no build URL)
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED, no build URL)

[brettw, 2015-11-20 04:10:39]

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.h
File build/build_header.h (right):

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.h#n...
build/build_header.h:26: #define BUILDFLAG(flag)
(BUILDFLAG_CAT2(BUILDFLAG_FLAG_VALUE_, flag) /
BUILDFLAG_CAT2(BUILDFLAG_DEFINE_CHECK_, BUILDFLAG_CAT2(BUILDFLAG_FLAG_VALUE_,
flag)))  // If you see a division by 0 it means your flag is undefined and
you're missing the header.
Scratch that, Visual Studio doesn't like this. I spent like 30 minutes on this
and I couldn't simplify this express *at all* in a way that makes Visual Studio
happy.

For example, this passes:
  #define FOO
  #if (1 / defined(FOO))
but this throws a divide by 0:
  #define FOO
  #define DOTEST(a) (1 / defined(FOO))
  #if DOTEST(a)

[brettw, 2015-11-20 18:47:23]

New snap up.

https://codereview.chromium.org/1458653002/diff/120001/build/write_build_head...
File build/write_build_header.py (right):

https://codereview.chromium.org/1458653002/diff/120001/build/write_build_head...
build/write_build_header.py:36: parser.add_option('--path-for-guard',
Yes, this is nicer. Changed.

https://codereview.chromium.org/1458653002/diff/120001/build/write_build_head...
build/write_build_header.py:46: header_guard.replace('/', '_').replace('\\',
'_').replace('.', '_')
My brain doesn't like regular expressions and I find this a lot easier to read.
It should also be faster (not that speed matters here).

https://codereview.chromium.org/1458653002/diff/120001/build/write_build_head...
build/write_build_header.py:89: output_file.write("/* Generated by
build/write_build_header.py */\n")
I changed to //, we can switch to /* */ if necessary later.

https://codereview.chromium.org/1458653002/diff/120001/build/write_build_head...
build/write_build_header.py:91: output_file.write("/* From " + options.rulename
+ " */\n")
On 2015/11/19 21:52:56, Mark Mentovai wrote:
> I’m inclined to deal with jokers by scrubbing newlines and */s and other
things
> that might cause unpredictable results.

I think this is unnecessary. Invocations to this script are all done by my
wrappers, and it's quite hard to get things like newlines in here (it's actually
impossible to get newlines from GN in the way this is invoked). If you put a
weird thing in, you'll get the exact same thing out, which should be not
surprising and then you can fix your bug. Trying to clean up people's messes
here just encourages the mess in the first place.

[brettw, 2015-11-20 18:47:28]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-20 18:48:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1458653002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1458653002/160001

[Mark Mentovai, 2015-11-20 18:49:26]

I was going to poke at MSVC a little bit to see if I could come up with
something more elegant that’d work there, unless you’re set on what you’ve got
now.

[spang, 2015-11-20 19:00:23]

spang@chromium.org changed reviewers:
+ spang@chromium.org

[spang, 2015-11-20 19:00:24]

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.h
File build/build_header.h (right):

https://codereview.chromium.org/1458653002/diff/120001/build/build_header.h#n...
build/build_header.h:26: #define BUILDFLAG(flag)
(BUILDFLAG_CAT2(BUILDFLAG_FLAG_VALUE_, flag) /
BUILDFLAG_CAT2(BUILDFLAG_DEFINE_CHECK_, BUILDFLAG_CAT2(BUILDFLAG_FLAG_VALUE_,
flag)))  // If you see a division by 0 it means your flag is undefined and
you're missing the header.
On 2015/11/20 04:10:39, brettw wrote:
> Scratch that, Visual Studio doesn't like this. I spent like 30 minutes on this
> and I couldn't simplify this express *at all* in a way that makes Visual
Studio
> happy.
> 
> For example, this passes:
>   #define FOO
>   #if (1 / defined(FOO))
> but this throws a divide by 0:
>   #define FOO
>   #define DOTEST(a) (1 / defined(FOO))
>   #if DOTEST(a)

Was

#if BUILDFLAG_ENABLE_FOO()

considered? It's a preprocessor syntax error if BUILDFLAG_ENABLE_FOO() was not
defined.

[commit-bot: I haz the power, 2015-11-20 19:01:22]

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

[commit-bot: I haz the power, 2015-11-20 19:01:23]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[brettw, 2015-11-20 20:26:25]

On 2015/11/20 18:49:26, Mark Mentovai wrote:
> I was going to poke at MSVC a little bit to see if I could come up with
> something more elegant that’d work there, unless you’re set on what you’ve got
> now.

You're welcome to do so.

Spang: I did consider what you suggested but the error messages are even worse
than what I have here ("expected binary operator, saw '(' insead"). If Mark
likes that way better I'm OK doing that though.

[brettw, 2015-11-20 20:59:55]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-20 21:00:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1458653002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1458653002/180001

[brettw, 2015-11-20 22:11:34]

If we agree on the current approach as opposed to GET_FOO(), then it might be
easier to check this in and people can more easily experiment and propose
improvements to build_header.h in isolation.

[Mark Mentovai, 2015-11-20 22:24:58]

I do basically agree with the overall direction of where this stands now. I
still think it’s confusing to have “flags” and “vars” be two distinct things.
There’s really no protection against

#if BUILDVAR(I_FORGOT_TO_INCLUDE_THE_HEADER)

[brettw, 2015-11-20 22:29:32]

On 2015/11/20 22:24:58, Mark Mentovai wrote:
> I do basically agree with the overall direction of where this stands now. I
> still think it’s confusing to have “flags” and “vars” be two distinct things.
> There’s really no protection against
> 
> #if BUILDVAR(I_FORGOT_TO_INCLUDE_THE_HEADER)

Right, but the point of BUILDVAR is that these aren't for enable/disable things
and wouldn't be used in if statements. We don't have huge numbers of these, but
we do have them. These are things like URLs where the result is going into some
C++ expression. As in my usage examples in the .gni file, the code won't compile
if the thing isn't the type you expect.

If somebody puts a boolean value into a "var" and then starts writing
conditionals based on it, they'll potentially get wrong behavior. But they'll
also get the wrong behavior doing any number of other wrong things. I don't
think there's any way we can have "undefined" protection for arbitrary defines
without -Wundef which I don't think is possible.

[Mark Mentovai, 2015-11-20 22:40:08]

On 2015/11/20 22:29:32, brettw wrote:
> On 2015/11/20 22:24:58, Mark Mentovai wrote:
> > I do basically agree with the overall direction of where this stands now. I
> > still think it’s confusing to have “flags” and “vars” be two distinct
things.
> > There’s really no protection against
> > 
> > #if BUILDVAR(I_FORGOT_TO_INCLUDE_THE_HEADER)
> 
> Right, but the point of BUILDVAR is that these aren't for enable/disable
things
> and wouldn't be used in if statements. We don't have huge numbers of these,
but
> we do have them. These are things like URLs where the result is going into
some
> C++ expression. As in my usage examples in the .gni file, the code won't
compile
> if the thing isn't the type you expect.

I’m afraid we’re going to wind up with things that aren’t binary 0/1 off/on
things, but that can have several values. As an example of a macro used this
way, see base/logging.h’s NOTIMPLEMENTED_POLICY. With the current structure,
BUILDFLAG() won’t allow that, so they’ll have to be BUILDVAR(). That sort of
thing makes complete sense in an #if (and in fact doesn’t make much sense
outside of an #if).

[commit-bot: I haz the power, 2015-11-20 22:42:58]

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

[commit-bot: I haz the power, 2015-11-20 22:42:58]

Dry run: This issue passed the CQ dry run.

[brettw, 2015-11-20 23:00:54]

On 2015/11/20 22:40:08, Mark Mentovai wrote:
> On 2015/11/20 22:29:32, brettw wrote:
> > On 2015/11/20 22:24:58, Mark Mentovai wrote:
> > > I do basically agree with the overall direction of where this stands now.
I
> > > still think it’s confusing to have “flags” and “vars” be two distinct
> things.
> > > There’s really no protection against
> > > 
> > > #if BUILDVAR(I_FORGOT_TO_INCLUDE_THE_HEADER)
> > 
> > Right, but the point of BUILDVAR is that these aren't for enable/disable
> things
> > and wouldn't be used in if statements. We don't have huge numbers of these,
> but
> > we do have them. These are things like URLs where the result is going into
> some
> > C++ expression. As in my usage examples in the .gni file, the code won't
> compile
> > if the thing isn't the type you expect.
> 
> I’m afraid we’re going to wind up with things that aren’t binary 0/1 off/on
> things, but that can have several values. As an example of a macro used this
> way, see base/logging.h’s NOTIMPLEMENTED_POLICY. With the current structure,
> BUILDFLAG() won’t allow that, so they’ll have to be BUILDVAR(). That sort of
> thing makes complete sense in an #if (and in fact doesn’t make much sense
> outside of an #if).

I'm more optimistic that we'll have relatively few of these places. But I agree
this is a concern, I just don't know what to do about them.

We've also fixed a few of these like printing, that used to be 0/1/2 and now is
two bools. I think similar such things can be done for other cases, especialyl
as we move from the old system to the new system, or have a enable/disable flag
and then a "var" that takes only non-zero values for the variants.

[Mark Mentovai, 2015-11-20 23:05:44]

OK. Lemme spend a little time poking at MSVC to see if I can find a way to make
it work elegantly, because getting rid of BUILDFLAG_DEFINE_CHECK_0/1 would mean
that BUILDFLAG() could work with non-booleans, and would allow us to put
everything on the same axis. That’s really the only foundational concern I want
to look into. Once we’ve gotten that resolved, I’ll happily LG and we can
iterate in-tree.

[Mark Mentovai, 2015-11-23 14:14:58]

How do you like this?

#define BUILDFLAG(flag) ((BUILDFLAG_INTERNAL_ ## flag ()))

In the generated headers, you would have

// INTERNAL naming suggests that you don’t use these directly
#define BUILDFLAG_INTERNAL_ENABLE_DOOM_MELON() (0)
#define BUILDFLAG_INTERNAL_ENABLE_SPACE_LASER() (1)
#define BUILDFLAG_INTERNAL_SPAM_SERVER_URL() ("http://www.example.com/")
#define BUILDFLAG_INTERNAL_NOTIMPLEMENTED_POLICY() (4)

Benefits:

There’s only one axis, instead of flags and values. Less developer confusion!

It’s accessed in exactly the same way, whether it’s in a preprocessor #if
construct or in C++ code. Less developer confusion!

The implementation is probably simpler than anything else we’ve come up with so
far, and it doesn’t stress preprocessor corner cases. Less Brett and Mark
confusion!

Drawbacks:

“#if BUILDFLAG(SPAM_SERVER_URL)” doesn’t work. But you probably didn’t expect
for it to work anyway.

[brettw, 2015-11-23 20:16:45]

On 2015/11/23 14:14:58, Mark Mentovai wrote:
> How do you like this?
> 
> #define BUILDFLAG(flag) ((BUILDFLAG_INTERNAL_ ## flag ()))
> 
> In the generated headers, you would have
> 
> // INTERNAL naming suggests that you don’t use these directly
> #define BUILDFLAG_INTERNAL_ENABLE_DOOM_MELON() (0)
> #define BUILDFLAG_INTERNAL_ENABLE_SPACE_LASER() (1)
> #define BUILDFLAG_INTERNAL_SPAM_SERVER_URL() ("http://www.example.com/")
> #define BUILDFLAG_INTERNAL_NOTIMPLEMENTED_POLICY() (4)
> 
> Benefits:
> 
> There’s only one axis, instead of flags and values. Less developer confusion!
> 
> It’s accessed in exactly the same way, whether it’s in a preprocessor #if
> construct or in C++ code. Less developer confusion!
> 
> The implementation is probably simpler than anything else we’ve come up with
so
> far, and it doesn’t stress preprocessor corner cases. Less Brett and Mark
> confusion!
> 
> Drawbacks:
> 
> “#if BUILDFLAG(SPAM_SERVER_URL)” doesn’t work. But you probably didn’t expect
> for it to work anyway.

I'm fine with this, will work on a new patch and see how it looks.

[brettw, 2015-11-23 23:02:09]

Suggestion implemented, PTAL. This does the function-style getters, removes the
"variables" thing, and puts parens around the flag values generated by the
script to avoid mistakes.

I had to keep BUILDFLAG_CAT to keep the preprocessor happy with invoking a
function call generated by ##.

[brettw, 2015-11-23 23:02:17]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-23 23:05:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1458653002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1458653002/220001

[commit-bot: I haz the power, 2015-11-23 23:29:58]

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

[commit-bot: I haz the power, 2015-11-23 23:29:59]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[brettw, 2015-11-24 00:11:36]

The CQ bit was checked by brettw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-24 00:14:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1458653002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1458653002/240001

[commit-bot: I haz the power, 2015-11-24 03:50:27]

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

[commit-bot: I haz the power, 2015-11-24 03:50:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mark Mentovai, 2015-11-24 14:33:33]

LGTM

https://codereview.chromium.org/1458653002/diff/240001/build/build_header.gni
File build/build_header.gni (right):

https://codereview.chromium.org/1458653002/diff/240001/build/build_header.gni...
build/build_header.gni:95: #       "SPAM_SERVER_URL=\"http://www.example.com/"",
Missing a backslash.

https://codereview.chromium.org/1458653002/diff/240001/build/build_header.gypi
File build/build_header.gypi (right):

https://codereview.chromium.org/1458653002/diff/240001/build/build_header.gyp...
build/build_header.gypi:22: #      
"SPAM_SERVER_URL=\"http://www.example.com/"",
Missing backslash after example.com, here and on line 37.

https://codereview.chromium.org/1458653002/diff/240001/build/build_header.h
File build/build_header.h (right):

https://codereview.chromium.org/1458653002/diff/240001/build/build_header.h#n...
build/build_header.h:4: 
Optional, do it if you like it to prevent direct #include, since there doesn’t
ever seem to be a valid use case for that:

#ifndef BUILD_BUILD_HEADER_H_INCLUDE_OK
#error Do not #include this file directly
#endif

and in each generated .h file

#define BUILD_BUILD_HEADER_H_INCLUDE_OK
#include "build/build_header.h"
#undef BUILD_BUILD_HEADER_H_INCLUDE_OK

https://codereview.chromium.org/1458653002/diff/240001/build/build_header.h#n...
build/build_header.h:22: // Accessor for build flags.
Since the former BUILDVAR is now covered by this, the comment should have an
example where a flag is used in a C++ expression.

https://codereview.chromium.org/1458653002/diff/240001/build/build_header.h#n...
build/build_header.h:42: #define BUILDFLAG(flag)
(BUILDFLAG_CAT(BUILDFLAG_INTERNAL_, flag)())
I think that this can be buildflag.h now that the only macro exposed is
BUILDFLAG(). .gypi and .gni naming can follow.

https://codereview.chromium.org/1458653002/diff/240001/build/write_build_head...
File build/write_build_header.py (right):

https://codereview.chromium.org/1458653002/diff/240001/build/write_build_head...
build/write_build_header.py:1: # Copyright 2015 The Chromium Authors. All rights
reserved.
#!/usr/bin/env python

# Copyright 2015 …

(or whatever flavor #! line we use for Python these days.)

https://codereview.chromium.org/1458653002/diff/240001/build/write_build_head...
build/write_build_header.py:1: # Copyright 2015 The Chromium Authors. All rights
reserved.
> new file mode 100644

This ought to be executable.

https://codereview.chromium.org/1458653002/diff/240001/build/write_build_head...
build/write_build_header.py:56: # Everything after --flags are flags. Values are
0/1/true/false.
Revise to say that false and true are mapped to 0 and 1, and everything else is
passed straight through.

[brettw, 2015-11-24 23:36:01]

https://codereview.chromium.org/1458653002/diff/240001/build/build_header.gni
File build/build_header.gni (right):

https://codereview.chromium.org/1458653002/diff/240001/build/build_header.gni...
build/build_header.gni:95: #       "SPAM_SERVER_URL=\"http://www.example.com/"",
On 2015/11/24 14:33:33, Mark Mentovai wrote:
> Missing a backslash.

Done.

https://codereview.chromium.org/1458653002/diff/240001/build/build_header.gypi
File build/build_header.gypi (right):

https://codereview.chromium.org/1458653002/diff/240001/build/build_header.gyp...
build/build_header.gypi:22: #      
"SPAM_SERVER_URL=\"http://www.example.com/"",
On 2015/11/24 14:33:33, Mark Mentovai wrote:
> Missing backslash after http://example.com, here and on line 37.

Done in this case. I shouldn't need it for the GYP code because it uses the
single-quotes for the outer string.

https://codereview.chromium.org/1458653002/diff/240001/build/build_header.h
File build/build_header.h (right):

https://codereview.chromium.org/1458653002/diff/240001/build/build_header.h#n...
build/build_header.h:4: 
I think directly including this file is both useless and harmless enough that
it's not worth worrying about.

https://codereview.chromium.org/1458653002/diff/240001/build/build_header.h#n...
build/build_header.h:42: #define BUILDFLAG(flag)
(BUILDFLAG_CAT(BUILDFLAG_INTERNAL_, flag)())
On 2015/11/24 14:33:33, Mark Mentovai wrote:
> I think that this can be buildflag.h now that the only macro exposed is
> BUILDFLAG(). .gypi and .gni naming can follow.

Done. I renamed the build scripts buildflag_header*

https://codereview.chromium.org/1458653002/diff/240001/build/write_build_head...
File build/write_build_header.py (right):

https://codereview.chromium.org/1458653002/diff/240001/build/write_build_head...
build/write_build_header.py:1: # Copyright 2015 The Chromium Authors. All rights
reserved.
On 2015/11/24 14:33:33, Mark Mentovai wrote:
> > new file mode 100644
> 
> This ought to be executable.

Done.

https://codereview.chromium.org/1458653002/diff/240001/build/write_build_head...
build/write_build_header.py:56: # Everything after --flags are flags. Values are
0/1/true/false.
On 2015/11/24 14:33:33, Mark Mentovai wrote:
> Revise to say that false and true are mapped to 0 and 1, and everything else
is
> passed straight through.

Done.

[brettw, 2015-11-24 23:36:07]

The CQ bit was checked by brettw@chromium.org

[brettw, 2015-11-24 23:36:08]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org
Link to the patchset: https://codereview.chromium.org/1458653002/#ps260001
(title: " ")

[commit-bot: I haz the power, 2015-11-24 23:37:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1458653002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1458653002/260001

[commit-bot: I haz the power, 2015-11-25 02:44:34]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-11-25 02:45:32]

Patchset 14 (id:??) landed as
https://crrev.com/3118dde528359280fa0cb3c6fc5b6323e834c514
Cr-Commit-Position: refs/heads/master@{#361527}

[raymes, 2015-11-25 03:09:36]

A revert of this CL (patchset #14 id:260001) has been created in
https://codereview.chromium.org/1475883002/ by raymes@chromium.org.

The reason for reverting is: This seems to have broken webkit builders:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Builder/....