As a follow-up to https://skia-review.googlesource.com/c/skia/+/476219,
this sketches out how we can maybe use cc_library for the things
in //modules to make sure something in //src doesn't depend on
anything in //modules, for example.
The following succeeds:
bazel build //modules/skparagraph:skparagraph --config=clang \
--shaper_backend=harfbuzz_shaper --with_icu
As does `make bazel_canvaskit_debug` in //modules/canvaskit
Suggested Review Order:
- third_party/BUILD.bazel for ICU and harfbuzz rules. Pay
special attention to the genrules used to call the python
script for turning the icu .dat file into .S or .cpp.
- bazelrc and bazel/ for new flags and defines that control
use of ICU and harfbuzz. Unlike GN, with the public_defines
that get added in automatically if icu or harfbuzz is
depended upon, we need to set the defines at the top level.
This necessity might go away if we change the atoms to
depend on //modules/skshaper, which could define that flag.
- Top level BUILD.bazel files in //modules/skparagraph,
//modules/skshaper, //modules/skunicode, //modules/canvaskit
- All other .bazel file changes are automatic.
Bug: skia:12541
Change-Id: I38a9e0a9261d7e142eeb271c2ddb23f362f91473
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/478116
Reviewed-by: Ben Wagner <bungeman@google.com>
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
To make the atomic rules a bit easier to work with, in many
of the folders, this adds in cc_library rules to group
together the sources from that folder (and subfolders
where prudent). We only needs sources because those atoms
should have their headers as deps.
One issue that was pointed out is that there is currently
no way to restrict the inclusion of certain packages,
a la, `gn check`. For example, there is no mechanism from
stopping a dev from adding
#include "modules/canvaskit/WasmCommon.h"
to something in //src/core (except circular dependencies).
We can probably address that using Bazel's visibility
rules as needed:
https://docs.bazel.build/versions/main/visibility.htmlhttps://docs.bazel.build/versions/main/be/functions.html#package_group
It is recommended to look at this CL patchset by patchset.
PS1: Update gazelle command to generate rules in more folders.
PS2: A few changes to make generation work better.
PS3: The result of running make generate in //bazel
PS4: Adding the rules to build sksllex, the simplest binary I
could find in the Skia repo.
PS5: Adding the rules to build skdiff, a more complex binary.
I tried a few approaches, but ended up gravitating back
towards the layout where we have each folder/package
group up the sources. I imagine at some point, we'll have
skdiff depend on skia_core or something, which will
have things like //src/core, //src/codecs, //src/pathops
all bundled together.
PS7: Added in the groupings of sources, similar to what we had
earlier. I liked these for readability. These helped fix
up the //:skia_core build, and by extension, the CanvasKit
build.
Change-Id: I3faa7c4e821c876b243617aacf0246efa524cbde
Bug: skia:12541
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/476219
Reviewed-by: Ben Wagner <bungeman@google.com>
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
We currently default to kClamp, but for mask feathers we want kDecal.
Bug: skia:12676
Change-Id: I7e3e21c310823987819e7374b494de4a48d9cae6
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/477297
Commit-Queue: Florin Malita <fmalita@google.com>
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Reviewed-by: Jorge Betancourt <jmbetancourt@google.com>
This uses the gazelle extension from
https://skia-review.googlesource.com/c/buildbot/+/473357
Review Tips:
- Ignore any changes to .h or .cpp files. Those have been
pulled out into their own CLs.
- Start with bazel/macros.bzl.
- Read the CL with the generation code, if you haven't already.
- Look at third_party/file_map_for_bazel.json.
- See experimental/bazel_test for an idea of how a cc_binary
would be made.
- Spot check one or two of the BUILD.bazel files.
This CL generates the "atomic" rules for src/, include/ and
modules/skshaper, as a starting point.
`bazel build --config clang //include/...` works
`bazel build --config clang //src/...` starts compiling,
(which verifies that the BUILD.bazel files are all valid),
but runs into errors because not all third_party deps have
been resolved, and there are some files missing from the
toolchain still (e.g. EGL headers).
Change-Id: Ib7e0fb0efdb9f08655f06cbc56e9bb4cf416294b
Bug: skia:12541
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/474240
Reviewed-by: Ben Wagner <bungeman@google.com>
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
Bug: skia:12559
Change-Id: I76b225c9ca81264a15869324007d774d210053b5
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/473416
Commit-Queue: Jim Van Verth <jvanverth@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This will hopefully let us pre-package certain binaries (e.g. DM,
fuzz) with more sensible defaults and not make the developer
type out all the settings.
For CanvasKit, which specifies its own build flags, I think I'll
need to make another transition setup, which would go in something
like modules/canvaskit/ck_binary_with_flags.bzl or something.
Some sausage-case-names were converted to snake_case_names as per
go/build-style#target-naming
The example this is based off is worth a look through before
diving into this:
https://github.com/bazelbuild/examples/tree/main/rules/starlark_configurations/cc_binary_selectable_copts
Change-Id: Ia919d47f4d1aa25cf294af7918e36d38838c179e
Bug: skia:12541
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/472688
Reviewed-by: Ben Wagner <bungeman@google.com>
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
Importantly, this adds options for encoding using
certain codecs, not just decoding.
Change-Id: I4a610ebf985b67d4545c71b3f3eed4c7807e6a26
Bug: skia:12541
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/472277
Reviewed-by: Ben Wagner <bungeman@google.com>
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
This re-works src/ports/BUILD.bazel to work like our other
BUILD files, i.e. one rule "srcs" that brings in the necessary
private filegroups.
To work around an abort with LLVM [1], we have to go back to an
earlier version of emscripten (temporarily?).
Future work should look at using transitions [2] to allow various
executables (e.g. CanvasKit, DM) to set their own set of Bazel
flags, w/o the build invokers having to specify them.
These transitions might be able to handle more complex cases
that we currently use if statements in GN to deal with.
The Freetype build rule was created by taking the BUILD.gn
rule, adding in all the sources listed there and then playing
compile-whack-a-mole to add in all the headers and included
.c files.
Suggested Review Order:
- third_party/BUILD.bazel to see freetype build rules
- bazel/common_config_settings/ to see treatment of fontmgr
like codecs (many possible) and fontmgr_factory (only one).
- src/ports/BUILD.bazel
- BUILD.bazel
- modules/canvaskit/BUILD.bazel. Take note of the gen_rule that
calls tools/embed_resources.py to produce the .cpp file
containing the embedded font data.
- Everything else.
[1] https://github.com/emscripten-core/emscripten/issues/15528
[2] https://github.com/bazelbuild/examples/tree/main/rules/starlark_configurations/cc_binary_selectable_copts
Bug: skia:12541
Change-Id: I08dab82a901d80507007b354ca20cbfad2c2388f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/471636
Reviewed-by: Ben Wagner <bungeman@google.com>
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
Many things are not enabled currently (e.g. Skottie, Paragraph),
but we can render many APIs using WebGL.
To turn on Paragraph, etc, we'll need to tackle fonts, which
is a separate effort.
This also changes where the build artifacts go. ./build/ is
easier to deal with than the old way of sticking them in
./npm_build/bin
Change-Id: Ia377360af580a887d03630670438fea2e3157e90
Bug: skia:12541
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/470682
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
Reviewed-by: Ben Wagner <bungeman@google.com>
The defaults were true for these, but if there was no
support, they should be set to false.
This was causing some warnings from the console, like:
WebGL: INVALID_ENUM: enable: invalid capability
Change-Id: I53b6120c969174a9ecd6d47df001870c71231352
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/470737
Reviewed-by: Greg Daniel <egdaniel@google.com>
When calling ProgramToSkVM, you can now pass in a pointer to an
SkVMDebugInfo struct. Passing in this pointer now enables trace opcodes,
removing the need for a dedicated `fSkVMDebugTrace` flag.
In this CL, the struct is empty, but in followup CLs it will contain
mapping tables that can be used to translate trace instructions into
human-readable results (e.g. slot numbers to variable names).
Change-Id: I6b38ec559723babed1f6d2efc4c5c2579c3b99db
Bug: skia:12614
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/470398
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This ports the third_party BUILD.gn files related to codecs
(with a best-effort on arm/SIMD stuff). This includes:
- libpng
- libjpeg-turbo
- libwebp
- wuffs (gif)
- libgifcodec
- dng_sdk and piex (raw codec)
This expands the string_flag_with_values macro to allow
multiple values to be set at once. This was added in Bazel 5.0.0,
however the latest pre-release version of that has a bug [1]
which slows down compilation dramatically. This was fixed at
ToT, but not released. As a result, I started using the Bazel
6 pre-release (via bazelisk).
The macro select_multi makes writing select() where multiple
elements could be on possible/easier.
One can try compiling certain codecs by running:
bazel build :skia-core --config clang --include_codec=raw_codec --include_codec=png_codec
Suggested Review Order:
- bazel/macros.bzl
- bazel/common_config_settings/defs.bzl and its BUILD.bazel
to see how the codec options are defined.
- BUILD.bazel to see how the codec settings are used.
- src/codec/BUILD.bazel to see the inclusion of Skia files to
deal with specific codecs.
- third_party/BUILD.bazel (while referencing the corresponding
BUILD.gn files, such as third_party/libwebp/BUILD.gn)
- Everything else.
Change-Id: I797375a35fa345d9835e7b2a2ab23371c45953c3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/469456
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
Reviewed-by: Ben Wagner <bungeman@google.com>
Reviewed-by: Leon Scroggins <scroggo@google.com>
Replaced by Graphite.
Change-Id: Iac0ba212b078904a591677c9ce839a90562d0240
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/470305
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>
Change-Id: Ibeea56ebd5dce53af1252ca3ecf6cc6f010bd461
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/469902
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
These rules can be used to build our GMs on WASM+WebGL and
libskia.a with just the CPU backend (and most other features
turned off).
This can be done with the following commands:
- bazel build //modules/canvaskit:gm-bindings-wasm --gpu_backend=gl_backend --with_gl_standard=webgl_standard
- bazel build :skia-core --config clang
This pivots slightly from http://review.skia.org/463517
by using config_settings [1] instead of platforms for
the optional features that we control. This pivot was
suggested in [2]
We have BUILD.bazel files in many of the subdirectories
that specify filegroups for the appropriate files. In
an effort to make //BUILD.bazel more readable, it is
the responsibility of these subfolders to deal with
conditionally including certain .h or .cpp files.
This is done using select statements and config_settings
or platform constraints as necessary.
For example, src/gpu/BUILD.bazel will different private
filegroups for each of the supported gpu backends [3]
and a more-visible filegroup called "srcs" that has
the right selection of the private files to be used
for compilation.
An effort has been made to avoid using glob() in our
BUILD.bazel files. These file lists were made by using
`ls -1` and some regex to add in quotes. We might want
to make a helper script to assist with that, if necessary.
To specify which options we have, the settings in
//bazel/common_config_settings/BUILD.bazel have been
redesigned. They make use of a macro `string_flag_with_values`
that removes the boilerplate. Patchset 36 shows what the
file looks like w/o the macro.
The top level BUILD.bazel file will still need to use
some logic to handle defines, because local_defines is
a list of strings, not a list of labels [4].
Suggested Review Order:
- WORKSPACE.bazel to see the new dependencies on the
emsdk toolchain and bazel_skylib
- bazel/common_config_settings/* to see the few settings
defined (we have more to define, see BUILD.gn and
//gn/skia.gni for ideas)
- BUILD.bazel to see the "skia-core" cc_library rule.
See also "gms" and "tests"
- modules/canvaskit/BUILD.bazel to see the use of
the emscripten "wasm_cc_binary" rule, which depends
on the "skia-core", "gms", and "tests" rule. Note that
it only builds some of the gms as a proof of concept.
- The other BUILD.bazel files. Some of these are not
platform or feature dependent (e.g. pathops). Others
are (e.g. gpu).
- All other files.
[1] https://docs.bazel.build/versions/4.2.1/skylark/config.html#user-defined-build-settings
[2] https://github.com/emscripten-core/emsdk/pull/920
[3] In this CL, that's just the webgl one.
[4] https://docs.bazel.build/versions/main/be/c-cpp.html#cc_library.local_defines
Change-Id: Ieecf9c106d5e3a6ae97d13d66be06b4b3c207089
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/458637
Reviewed-by: Ben Wagner <bungeman@google.com>
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
Owners-Override: Kevin Lubick <kjlubick@google.com>
This adds a way to test/demonstrate how an external client
using Typescript can make use of CanvasKit in a browser
(currently a JS library with Typescript bindings supplied).
See: modules/canvaskit/external_test/typescript_browser/
This also adds a way that we hope CanvasKit might be used
in a future release, that is, via ES6 modules.
See: modules/canvaskit/external_test/typescript_browser_es6/
These TS files can be built into the JS files needed by the
respective index.html files using the appropriate target
in modules/canvaskit/external_test/Makefile. Then, `make serve`
will bring up a HTTP Server that will make the folders
accessible on localhost:8000
Change-Id: Ie4ddc2588b4bdccd4a727f11b540289cf4f85795
Bug: skia:11077, skia:12539
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/468214
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
This effect does not yet support pinned edges or taper
also sneak in a filename change (CCToner) to keep all effect names standard
Change-Id: I17f2b5463408556775bc12a972358abd4d8d8690
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/467319
Commit-Queue: Jorge Betancourt <jmbetancourt@google.com>
Reviewed-by: Florin Malita <fmalita@google.com>
Pass inval controller and ctm to child nodes during revalidation.
Change-Id: Iee6862af78c77d5164e29b0e41ed6dae9a7f4235
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/466760
Auto-Submit: Florin Malita <fmalita@chromium.org>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Long term plan is to expose a plugin (standalone or with bodymovin) that allows motion artists to write sksl into a composition.
This is the first step where we test how we'd read in the json data under the hood.
Change-Id: I300d3af5d01e12f5b495970f89fd12b5f464a9a1
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/464368
Commit-Queue: Jorge Betancourt <jmbetancourt@google.com>
Reviewed-by: Florin Malita <fmalita@google.com>
warning: use of bitwise '|' with boolean operands [-Wbitwise-instead-of-logical]
inherit_if_needed(currentNode->fX , attrs->fX) |
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
note: cast one or both operands to int to silence this warning
Change-Id: I6d59a5c33826af29560c0e30ddf9f4e16581882c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/463896
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Florin Malita <fmalita@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Florin Malita <fmalita@google.com>
Halfs are lacking sufficient precision for this effect.
Change-Id: If3c2cac34d465d74b7ad8228417c42950f48adfd
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/461177
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Florin Malita <fmalita@google.com>
Hoist all evolution-related calculations out of the shader, and pass
precomputed noise planes and weight uniforms.
Renders ~20% faster locally (w/ software rasterization).
Change-Id: I771bea8f3425440515d2cb9a8623336d18bcc7b1
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/460336
Commit-Queue: Florin Malita <fmalita@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
AE allows for optional cycling of evolution, after a certain number of
revolutions.
To support:
- split off the base/offset component into a separate uniform
(currently front-loaded into evolution)
- introduce an additional "cycle" (period) uniform to mod() the noise
plane calculations
Change-Id: Ib412027114c467934c549cc1438a7d4560aa14bc
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/460116
Reviewed-by: Jorge Betancourt <jmbetancourt@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Florin Malita <fmalita@google.com>
The locale is malloc'ed using `cacheOrCopyString`, which should never
be explicitly freed.
Change-Id: I6a911c52f1f485b34fad86303a1f4be199195858
Cq-Include-Trybots: luci.skia.skia.primary:Test-Debian10-EMCC-GCE-CPU-AVX2-wasm-Release-All-CanvasKit,Test-Debian10-EMCC-GCE-GPU-AVX2-wasm-Release-All-CanvasKit
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/459835
Reviewed-by: Kevin Lubick <kjlubick@google.com>
Commit-Queue: Kevin Lubick <kjlubick@google.com>
`a || b` only evaluates b if a is false. `a | b` always evaluates
both a and b. If a and b are of type bool, `||` is usually what you
want, so clang now warns on `|` where both arguments are of type bool.
In Skia, 3 of 3 uses of `|` were intentional as far as I can tell (one
had an explicit comment, the other two didn't). Rewrite them slightly
to make this intent more clear and to suppress the warning.
There was also one use of `&`. That one looks like a (benign) typo for
`&&`, so change it.
Bug: chromium:1255745
Change-Id: I9ac37075311005c0a8fcb8d1379f516510929423
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/459456
Commit-Queue: Nico Weber <thakis@chromium.org>
Commit-Queue: Florin Malita <fmalita@google.com>
Auto-Submit: Nico Weber <thakis@chromium.org>
Reviewed-by: Florin Malita <fmalita@google.com>
If text layout fails (e.g. due to unsatisfiable scaling constraints),
log an error to notify clients.
Change-Id: Ic7a028196b3bfb8f45b91c88766f0059145a202f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/458722
Commit-Queue: Florin Malita <fmalita@google.com>
Reviewed-by: Florin Malita <fmalita@google.com>
Change-Id: Iffb757bdc8b28d14aae176b1d2448e72ccde7c3f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/459116
Reviewed-by: Kevin Lubick <kjlubick@google.com>
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
With this CL we can run as:
dm --src gm skp tests --config grmtl -v --nocpu --nogpu
and not get all the non-Graphite unit tests.
Bug: skia:12466
Change-Id: Ib3f04f315fe4b5731a54e4c72979a0c1e00baf24
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/457898
Commit-Queue: Robert Phillips <robertphillips@google.com>
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
1) CachingResourceProvider - The immediate user of this is ChromeOS,
but it seems like a generally useful ResourceProvider implementation
that all can use.
2) Animation::Builder - See this CL:
https://chromium-review.googlesource.com/c/chromium/src/+/3171517
It seems that windows builds fail with "undefined reference" linker
errors when trying to use the Animation::Builder directly. My guess
is that this is because the SK_API macro is needed to export it.
Change-Id: Ief39fe6ec03f992a0be73e5be54b0119d2d82930
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/458407
Commit-Queue: Florin Malita <fmalita@google.com>
Reviewed-by: Florin Malita <fmalita@google.com>
Classes of issues addressed:
1. static constexpr class variables aren't automatically inline. Already handled in a separate CL
2. Lack of C++17 copy elision means classes of objects constructed at function return need a copy or move constructor even if RVO will mean it isn't called.
3. Nested braced init no longer allowed for base classes of subclasses without constructors.
4. template static constexpr var in template class throws error about redundant initialization. Adding inline and removing defn outside of class fixes it.
5. Some places that should have been including std headers now actually need to include them.
6. No auto template parameters.
7. No lambdas in constexpr funcs.
Bug: chromium:1257145
Change-Id: Icb24c6b4ed039287fb4cf27a21a1bb7dc9821728
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/457298
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Ben Wagner <bungeman@google.com>
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>
This is in prep for compiling with -std=c++14 and -Wno-c++17-extensions
when building with clang. Chrome has encountered problems with
third_party headers that are included both in Skia and other Chrome
sources that produce different code based on whether preprocessor macros
indicate a C++14 or C++17 compilation.
In C++17 they are already inline implicitly. When compiling with C++14
we can get linker errors unless they're explicitly inlined or defined
outside the class. With -Wno-c++17-extensions we can explicitly inline
them in the C++14 build because the warning that would be generated
about using a C++17 language extension is suppressed.
We cannot do this in public headers because we support compiling with
C++14 without suppressing the C++17 language extension warnings.
Bug: chromium:1257145
Change-Id: Iaf5f4c62a398f98dd4ca9b7dfb86f2d5cab21d66
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/457498
Reviewed-by: Ben Wagner <bungeman@google.com>
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>
In addition to single line (point) text, AE also supports path layout
for paragraph text.
At a high level, the paragraph box top is mapped to the path (following
alignment rules), and each glyph is displaced along its path positioning
vector, post orientation.
The main difference compared to point text, is that the distance on path
is based on the fragment position relative to the paragraph left edge.
The paragraph box also plays a role in alignment: left/center/right
aligns with path start/mid/end.
This includes a tangential optimization: instead of validating cached
contour data in each PathInfo::getMatrix() call, we only check once at
a higher level (onSync) -- to avoid performing a shape vector comparison
for each fragment.
Change-Id: I2c31ce3b0a525a3cd2d4525abcf88d5fc943bb6e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/457656
Commit-Queue: Florin Malita <fmalita@google.com>
Reviewed-by: Ben Wagner <bungeman@google.com>
Reviewed-by: Jorge Betancourt <jmbetancourt@google.com>
