Changes the initialization logic for `PluginConfiguration` so that it
always populates the `diagnosticConfigs` field with a map that ignores
case when matching keys. This insures that diagnostic codes in a
plug-in configuration are matched in a case-insensitive fashion.
Also changes the logic in `pkg/analysis_server_plugin` so that when it
looks up names in this map, it uses `DiagnosticCode.lowerCaseName`
rather than `DiagnosticCode.name`. This in itself has no effect (since
the map ignores case), but it paves the way for deprecating (and
eventually removing) the `DiagnosticCode.name` getter.
Change-Id: I6a6a6964f8ba149f883d1e9b2c9168a0929f7803
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/466184
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Changes the logic in `pkg/analysis_server_plugin` to use
`DiagnosticCode.lowerCaseName` instead of `DiagnosticCode.name`. This
ensures that diagnostic codes are matched in a case-insensitive
fashion.
This paves the way for deprecating (and eventually removing) the
`DiagnosticCode.name` getter.
Change-Id: I6a6a6964f4733c550f9c1060377e198808190acc
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/466129
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Adjusts the logic in the lint `Registry` class so that lint names are
matched in case-insensitive fashion. For the most part this is
accomplished by adding calls to `.toLowerCase()` inside the `Registry`
class, preserving its API.
For the `enabled` method, preserving the API would have been a pain (I
would have had to translate the keys in the `ruleConfigs` parameter to
lower case). So instead I added an assertion to verify that the keys
were lower case, and pushed the responsibility to the callers to
create lower case keys.
This paves the way for a follow-up CL that will translate diagnostic
codes to `lower_snake_case` conventions.
It also solves a longstanding problem with the mixed case lint rules
`no_runtimeType_toString`, `prefer_for_elements_to_map_fromIterable`,
`prefer_iterable_whereType`. Previously, the user had to carefully
imitate the capitalization of the lint rules when specifying them in
the `analysis_options.yaml` file, even though `// ignore:` comments
for the lints matched in case insensitive fashion. With this CL, the
lint rule names in `analysis_options.yaml` are matched in case
insensitive fashion as well.
Change-Id: I6a6a6964d83241e49878bbf96ef9b94cbb12098b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/465964
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Prior to this CL, the analyzer's set of unignorable diagnostic code
names (`AnalysisOptionsImpl.unignorableDiagnosticCodeNames`) was
constructed in the following way:
- For each entry in the `cannot-ignore` section of the
`analysis_options.yaml` file that matches the name of a severity
code, the names of all diagnostic codes with matching severities
were added (1).
- All other entries in the `cannot-ignore` section were converted to
upper case and then added (2).
However, diagnostic codes associated with lints are named using
`lower_snake_case`, while analyzer diagnonstic codes are named using
`UPPER_SNAKE_CASE`.
Some of the logic that consumed
`AnalysisOptionsImpl.unignorableDiagnosticCodeNames` didn't account
for this, resulting in some subtle bugs:
- When the resolved correction producer base class
`_BaseIgnoreDiagnostic` attempted to figure out if a diagnostic was
unignorable, it compared elements of
`unignorableDiagnosticCodeNames` to `DiagnosticCode.name`, which
meant that it would successfully recognize non-lint codes as
unignorable, but it would only recognize that a lint code was
unignorable if it was included in `unignorableDiagnosticCodeNames`
by putting the name of a severity in the `cannot-ignore` section of
the `analysis_options.yaml` file.
- When `LibraryAnalyzer._filterIgnoredDiagnostics` attempted to block
unignorable diagnostics from being ignored, it compared elements of
`unignorableDiagnosticCodeNames` to `DiagnosticCode.name`,
`DiagnosticCode.uniqueName`, and
`DiagnosticCode.name.toUpperCase()`. This worked, however the
comparison to `DiagnosticCode.uniqueName` had no effect. Note that
values of `DiagnosticCode.uniqueName` always take the form
`ClassName.snake_case_diagnostic_code` or
`ClassName.SNAKE_CASE_DIAGNOSTIC_CODE`. It's impossible for any of
the values added to
`AnalysisOptionsImpl.unignorableDiagnosticCodeNames` to ever match
this, because (1) always adds values of `DiagnosticCode.name` (which
never contains a `.`), and (2) always adds strings that have been
converted to upper case.
- When `IgnoreValidator.reportErrors` attempted to report unignorable
and duplicate entries, it compared elements of
`unignorableDiagnosticCodeNames` to `IgnoredDiagnosticName.name`,
which is always lower case. That meant that it would only recognize
that a diagnostic code was unignorable if the diagnostic code was
associated with a lint and was included in
`unignorableDiagnosticCodeNames` by putting the name of a severity
in the `cannot-ignore` section of the `analysis_options.yaml`
file. (Note, however, that this bug was unobservable because the
reporting of the `unignorable_ignore` diagnostic is currently
disabled; I will address this in a follow-up CL.)
- Additionally, the logic to populate
`AnalysisOptionsImpl.unignorableDiagnosticCodeNames` based on a
severity code had a bug in its handling of error processors: if one
or more error processors were used to change the severity of a
diagnostic, then entries would be added to
`AnalysisOptionsImpl.unignorableDiagnosticCodeNames` corresponding
to both the original and the new severity.
These buggy behaviors have been fixed by:
- Streamlining and simplifying the logic that builds
`AnalysisOptionsImpl.unignorableDiagnosticCodeNames`, and ensuring
that all strings added to it are all lower case.
- Changing all logic that checks whether a string is contained in
`AnalysisOptionsImpl.unignorableDiagnosticCodeNames` so that it
first converts that string to lower case.
- Removing the ineffective logic in
`LibraryAnalyzer._filterIgnoredDiagnostics` that attempted to
compare elements of `unignorableDiagnosticCodeNames` to
`DiagnosticCode.uniqueName`.
Change-Id: I6a6a6964d89c139492dbb11d8ba3b2d33c0e2ee8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/462864
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Currently, diagnostic codes associated with lints are named using
`lower_snake_case`, while analyzer diagnonstic codes are named using
`UPPER_SNAKE_CASE`.
However, when the analyzer builds instances of the `ErrorProcessor`
class, it always uses `UPPER_SNAKE_CASE` names.
Some pieces of logic that matched up `ErrorProcessor`s to diagnostic
codes accounted for this difference; others didn't.
This led to a some buggy behaviors:
- If an instance of `ErrorProcessor` got constructed outside of the
analyzer (by an analyzer client using the analyzer public API), and
it supplied a `lower_snake_case` name, then
`ErrorProcessor.appliesTo` would only successfully match if the name
referred to a lint.
- The resolved correction producer base class `_BaseIgnoreDiagnostic`
(which forms the basis for the quick fixes "Ignore '...' in
`analysis_options.yaml`", "Ignore '...' for this line", and "Ignore
'...' for the whole file") would only notice that a diagnostic was
unignorable if the case matched exactly. In practice, this meant
that when operating on instances of `ErrorProcessor` created by the
analyzer, it wouldn't properly handle lints.
These buggy behaviors have been fixed by:
- Changing the `ErrorProcessor` constructor to always convert the
`code` to lower case.
- Changing all references to `ErrorProcessor.code` to assume lower
case.
Change-Id: I6a6a69645284f646e0c070fc2b55c4a90203d74a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/462863
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
No more flag, always parse into the new AST, always visit new AST nodes,
always return them as child entities, parent-child structure reflects
the new AST.
So, use `namePart` and `body` where possible. Deprecate previous
properties.
This is still de jure a breaking change, because `parent` of deprecated
properties changes. De facto this required very few changes in google3.
Once this CL lands, I will publish `analyzer 10.0.0`, migrate everything
to new properties, delete deprecated properties, and publish `analyzer
11.0.0`.
Maybe deprecate `NamedCompilationUnitMember.name` and migrate to
subclass specific `name` or `namePart` properties before publishing
`analyzer 10.0.0`. This part is not breaking per se.
* Deprecations in `ClassDeclaration`:
* Properties `leftBracket`, `members`, `rightBracket` are deprecated, use `body` instead.
* Properties `name`, `typeParameters` are deprecated, use `namePart` instead.
* Deprecations in `EnumDeclaration`:
* Properties `leftBracket`, `constants`, `members`, `rightBracket` are deprecated, use `body` instead.
* Properties `name`, `typeParameters` are deprecated, use `namePart` instead.
* Deprecations in `ExtensionDeclaration`:
* Properties `leftBracket`, `members`, `rightBracket` are deprecated, use `body` instead.
* Deprecations in `ExtensionTypeDeclaration`:
* Properties `leftBracket`, `constants`, `members`, `rightBracket` are deprecated, use `body` instead.
* Properties `constKeyword`, `name`, `representation`, `typeParameters` are deprecated,
use `primaryConstructor` instead.
* **Breaking Change:** While the deprecated members mentioned above still exist in the AST,
their parent nodes have changed. This means that code relying on specific parent-child
relationships for these nodes might break.
Bug: https://github.com/dart-lang/sdk/issues/61701
Change-Id: Ic48104da8b029c9b454bbd2336574b7823025565
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/461841
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Paul Berry <paulberry@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Replaces "is" tests that refer to classes derived from
`DiagnosticCode` with tests on `DiagnosticCode.type`:
- `x is ScannerErrorCode || x is ParserErrorCode` is replaced with
`x.type == DiagnosticType.SYNTACTIC_ERROR`. This is exactly
equivalent, because `ScannerErrorCode` and `ParserErrorCode` are the
only two `DiagnosticCode`-derived classes whose `type` method returns
`SYNTACTIC_ERROR`.
- `x is TodoCode` is replaced with `x.type ==
DiagnosticType.TODO`. This is exactly equivalent, because `TodoCode`
is the only `DiagnosticCode`-derived class whose `type` method
returns `TODO`.
- In `statement_completion.dart`, `x is! HintCode && x is!
WarningCode` is replaced with `x.type ==
DiagnosticType.SYNTACTIC_ERROR`. The new test is less accepting: for
example, it previously accepted diagnostics of type
`StaticWarningCode`. This test is used to short-cut the generation
of statement completions such as completing an incomplete `do`
statement. These completions are only necessary to generate if there
is a syntax error, so there should be no user-visible behavior
change.
- In `fix_processor.dart`, `x is LintCode || x is HintCode || x is
WarningCode` is replaced with `x.type == DiagnosticType.LINT ||
x.type == DiagnosticType.STATIC_WARNING`. The new test is more
accepting: it now accepts any diagnostic code whose type is
`STATIC_WARNING`, when previously it only accepted warnings of type
`WarningCode` (and rejected warnings of types like
`StaticWarningCode`). This test determines when quick fixes like
"ignore diagnostic on this line" are offered, so the behavior change
is appropriate.
- Also replaces a documentation reference to `TodoCode` (in
`todo_codes.dart`) with a reference to `DiagnosticType.TODO`.
These changes pave the way for a follow-up CL in which I plan to
eliminate these derived classes entirely.
Change-Id: I6a6a69647ff62dfe06b0219d8b292da53427cf0d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/461140
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
Changes several references to derived diagnostic types
(`CompileTimeErrorCode`, `WarningCode`, etc.) to simply
`DiagnosticCode`.
This paves the way for a follow-up CL that will change the static
types of the generated analyzer diagnostic code objects to more
general types. This is part of a longer-term effort to simplify the
`DiagnosticCode` class hierarchy.
Change-Id: I6a6a69642561aebf2607e834fa09fa608cb4fa10
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/458587
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Paul Berry <paulberry@google.com>
In testing, I saw that _added_ files were not being analyzed at all by
plugins. It turned out that `PluginManager.broadcastWatchEvent` uses
that glob to decide if watch events should be broadcast to a given
plugin isolate. It uses this Glob to determine that answer:
```dart
Glob(
_resourceProvider.pathContext.separator,
pattern,
).matches(filePath)
```
and using `'*.dart'` for that pattern, always answers "no, this file
does not match '*.dart'"; I believe `**.dart` is the correct glob
pattern we should use.
Change-Id: Id99d758151629c485884ba20e4d1f49f574f5889
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/452160
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
I had not run the first fix through manual testing. Manual testing
revealed a second location (!!) where the file path is stored, for
diagnostics. This updates the code to send a notification for each file
path, including parts.
Change-Id: Iee60330c579563f31c8d9a2c65a22c9cde4b64d8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/448881
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Fixes https://github.com/dart-lang/sdk/issues/61449
The previous code, sure enough, only created a DiagnosticReporter for
the defining unit, and only visited the main unit.
The fix is to create a DiagnosticListener for each unit, set the
DiagnosticReporters correctly and the "current unit" as we go, and then
pull the reported diagnostics from each listener.
Change-Id: I6b645284c58e5e35ccc5b89f30f6ffe97f48296c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/448644
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
This renames the old screen, "Legacy plugins" and adds a new screen,
"Plugins". On the new plugins screen, we ask the plugins isolate(s) for
their plugins details, and print the following, for eadh plugin:
* the plugin's name
* the names of the registered lint rules
* the names of the registered warning rules
* the IDs and "messages" of the registered assists
* the IDs and "messages" and associated diagnostic codes of the
registered quick fixes
More to come in follow ups:
* The resolved versions of plugin packages (coming from package_config.json)
Change-Id: Ic3dc4c5bffa64fd4da4097c042a847cc064e41ce
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/447763
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
This week we moved some other "plugin analysis options" code into the
private API. This continues that work, in order to add plugin source
options into the AnalysisOptionsImpl signature code.
The classes are deprecated as far as the public API is concerned,
encoded with a `@Deprecated` export. This is not reflected in api.txt.
Change-Id: Ie95851a7b0d54ef6abb8fb1e2b41294fbc5db836
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/447841
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
This library mostly exports _public_ API, and the to-be-deprecated
NodeLintRegistry class. So we just move private RuleContext classes
into a new library, and remove the exports.
The lint rules then are adjusted, mostly just renaming
NodeLintRegistry -> RuleVisitorRegistry. But also, this removes a lot
of implementation imports; most rules now have _no_ private imports!
Change-Id: I023f7efa979f94d208a6ae7421902145e8761c9a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/443364
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Because ChangeBuilder can be used to modify multiple files, the EOL should come from an individual files edit builder and not from the overall ChangeBuilder.
This change renames `eol` on ChangeBuilder to `defaultEol` (since it's still useful to supply a custom default, for example when moving a class from an existing file to a new file, we want to default to the same EOL as the source file), and adds an EOL to file edit builders which is computed from the file content (and falls back to the default from ChangeBuilder if there are none).
My intention here is to not introduce any breaking changes (yet) so the ChangeBuilder constructor interface still has the "eol" named parameter (marked as deprecated) as well as the new `defaultEol` parameter.
I added a new `StringExtension` for `endOfLine` to both analyzer_plugin and analysis_server_plugin as we need this in both places (but I expect one of those projects will ultimately be dropped). This slightly differs from `CorrectionUtils.endOfLine` because it can return `null` which is a signal to fall back to the ChangeBuilder default instead of forcing its own default. (CorrectionUtils.endOfLine now uses that extension, although because of the null/default, it's likely that some - if not all - uses of this should migrate to the new extension and fall back to a default from a ChangeBuilder).
Change-Id: Ie2e6eaba78c96a1cbd79eba881e12bbeef724aa8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/443122
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
The diff makes this change look bigger than it is.
Here's how the code worked before: `compute()` called `_addFromProducers()`. `_addFromProducers()` had a local function, also
called `compute()`, which is called per CorrectionProducer. The local
`compute()` function would unconditionally create a ChangeBuilder and
call each CorrectionProducer's `compute()` method, then call back out
to an instance method called `_addFixFromBuilder()`, which would drop
the computed change on the floor if `fixKind` was `null`.
This CL contains the following changes to the above system:
* The local `compute()` function and the instance method,
`_addFixFromBuilder()` are combined together, into one instance
method, `_addFromProducer()`.
* Then, the `fixKind` check is moved to the very top of that
method, bailing out _before_ computing changes if `fixKind == null`.
Change-Id: I6c61059fc36ec1587b15ca01b43830a5b179da9a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/417326
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Work towards https://github.com/dart-lang/sdk/issues/60635
I tried to keep this minimal but still sensible:
* Rename AnalysisErrorListener and all subtypes to use 'Diagnostic'.
* Rename all instantiations of such classes if they previously
contained the word 'error'.
* Rename `RecordingDiagnosticListener.errors` to `.diagnostics`.
* Rename some _testing_ instance members that had the word 'error'
to instead use 'diagnostic'.
Change-Id: I3948e27ba28ac2494092e04f4e1d201a20cc1135
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/433004
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Paul Berry <paulberry@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
This is a big CL; no code is really "changed." We basically move 3
components into the public API, which can all be reviewed concisely
in the `api.txt` file.
* The AnalysisRule classes: `AbstractAnalysisRule` (which is good to
make public as a lot of the public API is specified and documented
here), `AnalysisRule`, `MultiAnalysisRule`.
* The Pubspec classes, available for linting pubspec files:
`PubspecDependency`, `PubspecDependencyList`, `PubspecEntry`,
`PubspecEnvironment`, `PubspecGitRepo`, `PubspecHost`, `PubspecNode`,
`PubspecNodeList`, `PubspecVisitor`.
* The `RuleVisitorRegistry` class. This class is needed by analysis
rule authors, and is part of the public API of AnalysisRule.
Change-Id: Ib1803180de9469f4ff39cf1778f96787f5f74b14
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/432363
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Paul Berry <paulberry@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
For the analyzer plugin APIs, these classes should not have the word
'Lint' in them, as they are used for all analysis rules, whether they
report lints or warnings.
* LinterContext -> RuleContext
* LinterContextWithParsedResults -> RuleContextWithParsedResults
* LinterContextWithResolvedResults -> RuleContextWithResolvedResults
* LintRuleUnitContext -> RuleUnitContext
I'm open to better names for these. Some of these suffer from a
chain-of-nouns name (a "RuleUnitContext" is a Context for a Unit for
a Rule). So we could go with "UnitContextForRule" and "ContextForRule"
or other ideas like that.
Only RuleContext and RuleUnitContext will be public API.
Change-Id: I5550736beaba7e39b36e7b5584ca6d023aa4d02b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/431703
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Copying change builders in order to enable reverting changes from a
single correction producer when there's an exception is expensive.
This CL replaces that implementation with a transactional model.
Clients are not required to start a transaction (that happens
automatically), but are required to signal the end of a transaction by
invoking either `commit` or `revert`. (Actually, `commit` is assumed
if neither method is invoked before computing the `SourceChange`.)
There is some information in the Dart file edit builder related to
imports that isn't correctly handled. The reason for this is that too
much of the import computation is done up-front. I don't think this
will be a problem in practice (or at least not often), but we should
come back at some point to change the implementation so that we retain
abstract data longer and perform more processing at the very end (where
we'll have complete information and probably be able to do a better
job anyway).
This CL improved the performance of the benchmark that adds and applies
fixes for 10,000 lint violations.
Before it took an average of 11216.0 ms.
After it took an average of 4200.2 ms.
Which is about a 62% improvement.
Change-Id: I12710606455a04e34a82308ede5d9fba1c68b972
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/428060
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Before this change, we had an enforcement that either 'get lintCode' or
'get lintCodes' is implemented, but it was a runtime enforcement. And it
was a little roundabout.
The new system is simpler and more typical:
* `AbstractLintRule` is the parent type, and it's sealed. It provides
`get lintCodes` as an interface. Code outside of the lint rules
requires this getter, in registering codes, etc.
* `LintRule` is the new subclass which is the "90% case." It declares
a `get lintRule`, and offers simpler reporting methods for reporting
a single code everywhere.
* `MultiLintRule` is the new subclass which represents that last
"10% case." It only declares the reporting methods which all require
a LintCode parameter, since there is no concept of a "default lint
code among all my lint codes."
Work towards https://github.com/dart-lang/sdk/issues/50986
Change-Id: I63cbbfc1d936bb4428aa0b8f5fb0638c9b61b3dc
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/426284
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Sam Rawlins