[Extensions Bindings] Move signature parsing tests to their own file

[Extensions Bindings] Move signature parsing tests to their own file

Move tests for parsing API calls against the expected signature from
api_binding_unittest.cc to a new api_signature_unittest.cc. This is
more correct since the parsing now happens in that file, and will also
make subsequent tests cleaner.

BUG=653596
Review-Url: https://codereview.chromium.org/2847843002
Cr-Commit-Position: refs/heads/master@{#468044}
Committed: https://chromium.googlesource.com/chromium/src/+/0d25864a9695b631e4c6da7037ba0c6cb0854f30

[Devlin, 2017-04-27 18:31:52]

rdevlin.cronin@chromium.org changed reviewers:
+ jbroman@chromium.org, lazyboy@chromium.org

[Devlin, 2017-04-27 18:31:52]

Mind taking a quick look?  Mostly just juggling unit tests around to make them
more local (since signature parsing is no longer handled directly in the
binding).  This will also make surfacing signature parsing errors cleaner to
test.

[Devlin, 2017-04-27 18:31:56]

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

[commit-bot: I haz the power, 2017-04-27 18:32:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 19:01:33]

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

[commit-bot: I haz the power, 2017-04-27 19:01:34]

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

[jbroman, 2017-04-27 19:18:18]

lgtm with a bikeshed :)

https://codereview.chromium.org/2847843002/diff/1/extensions/renderer/api_sig...
File extensions/renderer/api_signature_unittest.cc (right):

https://codereview.chromium.org/2847843002/diff/1/extensions/renderer/api_sig...
extensions/renderer/api_signature_unittest.cc:66: return
base::MakeUnique<APISignature>(std::move(specs));
super-nit: Or this could just be, if you'd want to skip the explicit push_back
calls (here and elsewhere):

return base::MakeUnique<APISignature>({std::move(object_spec)});

Or if you take my other suggestion, this might be moot.

https://codereview.chromium.org/2847843002/diff/1/extensions/renderer/api_sig...
extensions/renderer/api_signature_unittest.cc:127: }
nit: Hmm. This is only unit tests, so maybe it's not a big deal, but this is
hard enough to follow that I start to wonder if we don't want a builder so that
this can look something like:

return base::MakeUnique<APISignature>(
    {ArgumentSpecBuilder(ArgumentType::INTEGER).set_name("int").Build(),
     ArgumentSpecBuilder(ArgumentType::ANY).set_name("any").Build(),
     ArgumentSpecBuilder(ArgumentType::OBJECT)
         .set_name("obj")
         .set_optional(true)
         .AddProperty("prop",
                      ArgumentSpecBuilder(ArgumentType::INTEGER)
                          .set_optional(true)
                          .Build())
         .Build(),
     ArgumentSpecBuilder(ArgumentType::FUNCTION)
         .set_name("callback")
         .set_optional(true)
         .Build()});

Might be useful elsewhere, though it would be mildly annoying to have to forward
all the setters. Could go further, by:
- adding helpers that set the name and optional fields, which are the most
common
- using implicit conversion to avoid having to explicitly Build() -- probably
going too far, but does make it pretty

return base::MakeUnique<APISignature>(
    {RequiredArg(ArgumentType::INTEGER, "int"),
     RequiredArg(ArgumentType::ANY, "any"),
     OptionalArg(ArgumentType::OBJECT, "obj")
         .AddProperty("prop", OptionalArg(ArgumentType::INTEGER)),
     OptionalArg(ArgumentType::FUNCTION, "callback")});

If these become sufficiently readable, it might be practical to inline them into
the tests in which they are used, so that the definition and assertions are
close together.

[jbroman, 2017-04-27 19:59:05]

https://codereview.chromium.org/2847843002/diff/1/extensions/renderer/api_sig...
File extensions/renderer/api_signature_unittest.cc (right):

https://codereview.chromium.org/2847843002/diff/1/extensions/renderer/api_sig...
extensions/renderer/api_signature_unittest.cc:66: return
base::MakeUnique<APISignature>(std::move(specs));
On 2017/04/27 at 19:18:17, jbroman wrote:
> super-nit: Or this could just be, if you'd want to skip the explicit push_back
calls (here and elsewhere):
> 
> return base::MakeUnique<APISignature>({std::move(object_spec)});
> 
> Or if you take my other suggestion, this might be moot.

Update: urgh, nope, I'm wrong about this point.

https://codereview.chromium.org/2847843002/diff/1/extensions/renderer/api_sig...
extensions/renderer/api_signature_unittest.cc:127: }
On 2017/04/27 at 19:18:17, jbroman wrote:
> nit: Hmm. This is only unit tests, so maybe it's not a big deal, but this is
hard enough to follow that I start to wonder if we don't want a builder so that
this can look something like:
> 
> return base::MakeUnique<APISignature>(
>     {ArgumentSpecBuilder(ArgumentType::INTEGER).set_name("int").Build(),
>      ArgumentSpecBuilder(ArgumentType::ANY).set_name("any").Build(),
>      ArgumentSpecBuilder(ArgumentType::OBJECT)
>          .set_name("obj")
>          .set_optional(true)
>          .AddProperty("prop",
>                       ArgumentSpecBuilder(ArgumentType::INTEGER)
>                           .set_optional(true)
>                           .Build())
>          .Build(),
>      ArgumentSpecBuilder(ArgumentType::FUNCTION)
>          .set_name("callback")
>          .set_optional(true)
>          .Build()});
> 
> Might be useful elsewhere, though it would be mildly annoying to have to
forward all the setters. Could go further, by:
> - adding helpers that set the name and optional fields, which are the most
common
> - using implicit conversion to avoid having to explicitly Build() -- probably
going too far, but does make it pretty
> 
> return base::MakeUnique<APISignature>(
>     {RequiredArg(ArgumentType::INTEGER, "int"),
>      RequiredArg(ArgumentType::ANY, "any"),
>      OptionalArg(ArgumentType::OBJECT, "obj")
>          .AddProperty("prop", OptionalArg(ArgumentType::INTEGER)),
>      OptionalArg(ArgumentType::FUNCTION, "callback")});
> 
> If these become sufficiently readable, it might be practical to inline them
into the tests in which they are used, so that the definition and assertions are
close together.

On 2017/04/27 at 19:18:18, jbroman wrote:
> lgtm with a bikeshed :)
> 
>
https://codereview.chromium.org/2847843002/diff/1/extensions/renderer/api_sig...
> File extensions/renderer/api_signature_unittest.cc (right):
> 
>
https://codereview.chromium.org/2847843002/diff/1/extensions/renderer/api_sig...
> extensions/renderer/api_signature_unittest.cc:66: return
base::MakeUnique<APISignature>(std::move(specs));
> super-nit: Or this could just be, if you'd want to skip the explicit push_back
calls (here and elsewhere):
> 
> return base::MakeUnique<APISignature>({std::move(object_spec)});
> 
> Or if you take my other suggestion, this might be moot.
> 
>
https://codereview.chromium.org/2847843002/diff/1/extensions/renderer/api_sig...
> extensions/renderer/api_signature_unittest.cc:127: }
> nit: Hmm. This is only unit tests, so maybe it's not a big deal, but this is
hard enough to follow that I start to wonder if we don't want a builder so that
this can look something like:
> 
> return base::MakeUnique<APISignature>(
>     {ArgumentSpecBuilder(ArgumentType::INTEGER).set_name("int").Build(),
>      ArgumentSpecBuilder(ArgumentType::ANY).set_name("any").Build(),
>      ArgumentSpecBuilder(ArgumentType::OBJECT)
>          .set_name("obj")
>          .set_optional(true)
>          .AddProperty("prop",
>                       ArgumentSpecBuilder(ArgumentType::INTEGER)
>                           .set_optional(true)
>                           .Build())
>          .Build(),
>      ArgumentSpecBuilder(ArgumentType::FUNCTION)
>          .set_name("callback")
>          .set_optional(true)
>          .Build()});
> 
> Might be useful elsewhere, though it would be mildly annoying to have to
forward all the setters. Could go further, by:
> - adding helpers that set the name and optional fields, which are the most
common
> - using implicit conversion to avoid having to explicitly Build() -- probably
going too far, but does make it pretty
> 
> return base::MakeUnique<APISignature>(
>     {RequiredArg(ArgumentType::INTEGER, "int"),
>      RequiredArg(ArgumentType::ANY, "any"),
>      OptionalArg(ArgumentType::OBJECT, "obj")
>          .AddProperty("prop", OptionalArg(ArgumentType::INTEGER)),
>      OptionalArg(ArgumentType::FUNCTION, "callback")});
> 
> If these become sufficiently readable, it might be practical to inline them
into the tests in which they are used, so that the definition and assertions are
close together.

To correct myself slightly here, for some frustrating reason you can't use
move-only types in an initializer list. There are workarounds though (so if you
like this approach maybe I can make it work and send it to you as a followup?).

[Devlin, 2017-04-27 20:50:17]

Patchset #2 (id:20001) has been deleted

[Devlin, 2017-04-27 20:55:46]

https://codereview.chromium.org/2847843002/diff/1/extensions/renderer/api_sig...
File extensions/renderer/api_signature_unittest.cc (right):

https://codereview.chromium.org/2847843002/diff/1/extensions/renderer/api_sig...
extensions/renderer/api_signature_unittest.cc:66: return
base::MakeUnique<APISignature>(std::move(specs));
On 2017/04/27 19:59:05, jbroman wrote:
> On 2017/04/27 at 19:18:17, jbroman wrote:
> > super-nit: Or this could just be, if you'd want to skip the explicit
push_back
> calls (here and elsewhere):
> > 
> > return base::MakeUnique<APISignature>({std::move(object_spec)});
> > 
> > Or if you take my other suggestion, this might be moot.
> 
> Update: urgh, nope, I'm wrong about this point.

Yep, originally tried to do that (and failed).

https://codereview.chromium.org/2847843002/diff/1/extensions/renderer/api_sig...
extensions/renderer/api_signature_unittest.cc:127: }
On 2017/04/27 19:18:17, jbroman wrote:
> nit: Hmm. This is only unit tests, so maybe it's not a big deal, but this is
> hard enough to follow that I start to wonder if we don't want a builder so
that
> this can look something like:
> 
> return base::MakeUnique<APISignature>(
>     {ArgumentSpecBuilder(ArgumentType::INTEGER).set_name("int").Build(),
>      ArgumentSpecBuilder(ArgumentType::ANY).set_name("any").Build(),
>      ArgumentSpecBuilder(ArgumentType::OBJECT)
>          .set_name("obj")
>          .set_optional(true)
>          .AddProperty("prop",
>                       ArgumentSpecBuilder(ArgumentType::INTEGER)
>                           .set_optional(true)
>                           .Build())
>          .Build(),
>      ArgumentSpecBuilder(ArgumentType::FUNCTION)
>          .set_name("callback")
>          .set_optional(true)
>          .Build()});
> 
> Might be useful elsewhere, though it would be mildly annoying to have to
forward
> all the setters. Could go further, by:
> - adding helpers that set the name and optional fields, which are the most
> common
> - using implicit conversion to avoid having to explicitly Build() -- probably
> going too far, but does make it pretty
> 
> return base::MakeUnique<APISignature>(
>     {RequiredArg(ArgumentType::INTEGER, "int"),
>      RequiredArg(ArgumentType::ANY, "any"),
>      OptionalArg(ArgumentType::OBJECT, "obj")
>          .AddProperty("prop", OptionalArg(ArgumentType::INTEGER)),
>      OptionalArg(ArgumentType::FUNCTION, "callback")});
> 
> If these become sufficiently readable, it might be practical to inline them
into
> the tests in which they are used, so that the definition and assertions are
> close together.

Interesting thought.  Short answer: why not!  Longer answer:

This sounds good modulo a few things:
- I don't think RequiredArg/OptionalArg add quite enough value to make them
worth it.
- I think this should remain test-only, since it's only syntactic sugar (and
limited at that), and we generally want to discourage hand-construction of
arguments.

https://codereview.chromium.org/2847843002/diff/40001/extensions/renderer/arg...
File extensions/renderer/argument_spec_builder.h (right):

https://codereview.chromium.org/2847843002/diff/40001/extensions/renderer/arg...
extensions/renderer/argument_spec_builder.h:43: operator
std::unique_ptr<ArgumentSpec>();
This might violate
https://google.github.io/styleguide/cppguide.html#Implicit_Conversions,
depending on how lenient you are with "Implicit conversions can sometimes be
necessary and appropriate for types that are designed to transparently wrap
other types".  Given this is test-only, I'm fine with it.  But we could also
just always call the Build() method.

[Devlin, 2017-04-27 21:04:43]

Another couple followup thoughts. :)

There's also places in other unittests that might benefit from this; I'll tackle
those separately.

https://codereview.chromium.org/2847843002/diff/40001/extensions/renderer/api...
File extensions/renderer/api_signature_unittest.cc (right):

https://codereview.chromium.org/2847843002/diff/40001/extensions/renderer/api...
extensions/renderer/api_signature_unittest.cc:40: return
base::MakeUnique<APISignature>(std::move(specs));
As alluded offline, we could get to the world of
return base::MakeUnique<APISignature>({
  spec1,
  spec2
});

if we were to have a ctor of APISignature take an
initializer_list<std::unique_ptr<ArgumentSpec>>.  For now, I don't think it's
quite worth it, but if we end up using this pattern in production more, that may
change.

[jbroman, 2017-04-27 21:24:32]

Not going to bikeshed this much more. The tests are (to me at least)
substantially easier to read now. Thanks, still lgtm.

https://codereview.chromium.org/2847843002/diff/40001/extensions/renderer/api...
File extensions/renderer/api_signature_unittest.cc (right):

https://codereview.chromium.org/2847843002/diff/40001/extensions/renderer/api...
extensions/renderer/api_signature_unittest.cc:20: 
nit: not a huge win, but you could (here, in ArgumentSpecBuilder, or as an
APISignature constructor if we wanted to allow it in production):

std::unique_ptr<APISignature>
MakeAPISignature(std::initializer_list<std::unique_ptr<ArgumentSpec>> arguments)
{
  SpecVector specs(std::make_move_iterator(arguments.begin()),
std::make_move_iterator(arguments.end()));
  return base::MakeUnique<APISignature>(std::move(specs));
}

Usage:

  return MakeAPISignature({
    spec1,
    spec2
  });

But this is getting a little into the weeds.

https://codereview.chromium.org/2847843002/diff/40001/extensions/renderer/api...
extensions/renderer/api_signature_unittest.cc:127: type_refs_.AddSpec("refObj",
std::move(ref_obj_spec));
or:

std::unique_ptr<ArgumentSpec> ref_obj_spec =
    ArgumentSpecBuilder(ArgumentType::OBJECT)
        .AddProperty("prop1", ArgumentSpecBuilder(ArgumentType::STRING))
        .AddProperty("prop2",
                     ArgumentSpecBuilder(ArgumentType::INTEGER).MakeOptional());
type_refs_.AddSpec("refObj", std::move(ref_obj_spec));

https://codereview.chromium.org/2847843002/diff/40001/extensions/renderer/api...
extensions/renderer/api_signature_unittest.cc:131: type_refs_.AddSpec("refEnum",
std::move(ref_enum_spec));
or (albeit not much difference):

type_refs_.AddSpec(
    "refEnum",
    ArgumentSpecBuilder(ArgumentType::STRING).SetEnumValues({"alpha", "beta"}));

https://codereview.chromium.org/2847843002/diff/40001/extensions/renderer/api...
extensions/renderer/api_signature_unittest.cc:179: TEST_F(APISignatureTest, Foo)
{
nit: Foo is not a helpful test name.

https://codereview.chromium.org/2847843002/diff/40001/extensions/renderer/api...
extensions/renderer/api_signature_unittest.cc:183: auto signature = OneString();
nit: since these seem to all be from above an only used once, consider inlining
them next to the assertions. The names are good enough that it's not a big deal,
but it's nice not to have to jump up and down to double-check exactly what the
names of OneObject's properties were, etc.

https://codereview.chromium.org/2847843002/diff/40001/extensions/renderer/arg...
File extensions/renderer/argument_spec_builder.h (right):

https://codereview.chromium.org/2847843002/diff/40001/extensions/renderer/arg...
extensions/renderer/argument_spec_builder.h:43: operator
std::unique_ptr<ArgumentSpec>();
On 2017/04/27 at 20:55:46, Devlin wrote:
> This might violate
https://google.github.io/styleguide/cppguide.html#Implicit_Conversions,
depending on how lenient you are with "Implicit conversions can sometimes be
necessary and appropriate for types that are designed to transparently wrap
other types".  Given this is test-only, I'm fine with it.  But we could also
just always call the Build() method.

Yeaahhh, I don't feel strongly. Agreed that it has its ups and downs, and the
style guide is...vague about how lenient this is.

[lazyboy, 2017-04-27 22:19:04]

lgtm

[Devlin, 2017-04-28 01:16:45]

https://codereview.chromium.org/2847843002/diff/40001/extensions/renderer/api...
File extensions/renderer/api_signature_unittest.cc (right):

https://codereview.chromium.org/2847843002/diff/40001/extensions/renderer/api...
extensions/renderer/api_signature_unittest.cc:20: 
On 2017/04/27 21:24:32, jbroman wrote:
> nit: not a huge win, but you could (here, in ArgumentSpecBuilder, or as an
> APISignature constructor if we wanted to allow it in production):
> 
> std::unique_ptr<APISignature>
> MakeAPISignature(std::initializer_list<std::unique_ptr<ArgumentSpec>>
arguments)
> {
>   SpecVector specs(std::make_move_iterator(arguments.begin()),
> std::make_move_iterator(arguments.end()));
>   return base::MakeUnique<APISignature>(std::move(specs));
> }
> 
> Usage:
> 
>   return MakeAPISignature({
>     spec1,
>     spec2
>   });
> 
> But this is getting a little into the weeds.

Yep, mentioned this here:
https://codereview.chromium.org/2847843002/diff/40001/extensions/renderer/api...

(though perhaps after you had already entered this)

I think for now, that's a little bit overkill (too weeds-y :)), but depending on
how things shake out, that might change.

https://codereview.chromium.org/2847843002/diff/40001/extensions/renderer/api...
extensions/renderer/api_signature_unittest.cc:127: type_refs_.AddSpec("refObj",
std::move(ref_obj_spec));
On 2017/04/27 21:24:32, jbroman wrote:
> or:
> 
> std::unique_ptr<ArgumentSpec> ref_obj_spec =
>     ArgumentSpecBuilder(ArgumentType::OBJECT)
>         .AddProperty("prop1", ArgumentSpecBuilder(ArgumentType::STRING))
>         .AddProperty("prop2",
>                     
ArgumentSpecBuilder(ArgumentType::INTEGER).MakeOptional());
> type_refs_.AddSpec("refObj", std::move(ref_obj_spec));

Whoops!  Missed this one.  Thanks.

https://codereview.chromium.org/2847843002/diff/40001/extensions/renderer/api...
extensions/renderer/api_signature_unittest.cc:131: type_refs_.AddSpec("refEnum",
std::move(ref_enum_spec));
On 2017/04/27 21:24:32, jbroman wrote:
> or (albeit not much difference):
> 
> type_refs_.AddSpec(
>     "refEnum",
>     ArgumentSpecBuilder(ArgumentType::STRING).SetEnumValues({"alpha",
"beta"}));

Done.

https://codereview.chromium.org/2847843002/diff/40001/extensions/renderer/api...
extensions/renderer/api_signature_unittest.cc:179: TEST_F(APISignatureTest, Foo)
{
On 2017/04/27 21:24:32, jbroman wrote:
> nit: Foo is not a helpful test name.

D'oh!  Thanks for catching this.  It's a terrible habit that I use these as
placeholders thinking I'll go back and fix them.  Maybe I'll finally learn.

https://codereview.chromium.org/2847843002/diff/40001/extensions/renderer/api...
extensions/renderer/api_signature_unittest.cc:183: auto signature = OneString();
On 2017/04/27 21:24:32, jbroman wrote:
> nit: since these seem to all be from above an only used once, consider
inlining
> them next to the assertions. The names are good enough that it's not a big
deal,
> but it's nice not to have to jump up and down to double-check exactly what the
> names of OneObject's properties were, etc.

If it's okay, I'd like to refrain.  When we add more tests that use these same
signatures, I think it's more helpful to have them defined once.

https://codereview.chromium.org/2847843002/diff/40001/extensions/renderer/arg...
File extensions/renderer/argument_spec_builder.h (right):

https://codereview.chromium.org/2847843002/diff/40001/extensions/renderer/arg...
extensions/renderer/argument_spec_builder.h:43: operator
std::unique_ptr<ArgumentSpec>();
On 2017/04/27 21:24:32, jbroman wrote:
> On 2017/04/27 at 20:55:46, Devlin wrote:
> > This might violate
> https://google.github.io/styleguide/cppguide.html#Implicit_Conversions,
> depending on how lenient you are with "Implicit conversions can sometimes be
> necessary and appropriate for types that are designed to transparently wrap
> other types".  Given this is test-only, I'm fine with it.  But we could also
> just always call the Build() method.
> 
> Yeaahhh, I don't feel strongly. Agreed that it has its ups and downs, and the
> style guide is...vague about how lenient this is.

Well, given we're both on the fence, let's take it out and just use Build().  I
was just so excited to be able to use operator T() in a not-totally-crazy place!
;)

[jbroman, 2017-04-28 01:30:54]

still LGTM

Thanks for putting up with the bikeshedding.

https://codereview.chromium.org/2847843002/diff/40001/extensions/renderer/api...
File extensions/renderer/api_signature_unittest.cc (right):

https://codereview.chromium.org/2847843002/diff/40001/extensions/renderer/api...
extensions/renderer/api_signature_unittest.cc:183: auto signature = OneString();
On 2017/04/28 at 01:16:44, Devlin wrote:
> On 2017/04/27 21:24:32, jbroman wrote:
> > nit: since these seem to all be from above an only used once, consider
inlining
> > them next to the assertions. The names are good enough that it's not a big
deal,
> > but it's nice not to have to jump up and down to double-check exactly what
the
> > names of OneObject's properties were, etc.
> 
> If it's okay, I'd like to refrain.  When we add more tests that use these same
signatures, I think it's more helpful to have them defined once.

Sure, sounds good then.

[Devlin, 2017-04-28 16:19:45]

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

[commit-bot: I haz the power, 2017-04-28 16:20:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 17:15:41]

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

[commit-bot: I haz the power, 2017-04-28 17:15:42]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-04-28 17:21:14]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2017-04-28 17:21:15]

The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org
Link to the patchset: https://codereview.chromium.org/2847843002/#ps60001
(title: "jbroman's")

[commit-bot: I haz the power, 2017-04-28 17:22:05]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 17:28:02]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493400074884180,
"parent_rev": "305e5224d9e9b11a609e17ca11866be3a3622ad6", "commit_rev":
"0d25864a9695b631e4c6da7037ba0c6cb0854f30"}

[commit-bot: I haz the power, 2017-04-28 17:28:11]

Description was changed from

==========
[Extensions Bindings] Move signature parsing tests to their own file

Move tests for parsing API calls against the expected signature from
api_binding_unittest.cc to a new api_signature_unittest.cc. This is
more correct since the parsing now happens in that file, and will also
make subsequent tests cleaner.

BUG=653596
==========

to

==========
[Extensions Bindings] Move signature parsing tests to their own file

Move tests for parsing API calls against the expected signature from
api_binding_unittest.cc to a new api_signature_unittest.cc. This is
more correct since the parsing now happens in that file, and will also
make subsequent tests cleaner.

BUG=653596

Review-Url: https://codereview.chromium.org/2847843002
Cr-Commit-Position: refs/heads/master@{#468044}
Committed:
https://chromium.googlesource.com/chromium/src/+/0d25864a9695b631e4c6da7037ba...
==========

[commit-bot: I haz the power, 2017-04-28 17:28:12]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/0d25864a9695b631e4c6da7037ba...