Provide script/gypi support for manifest generation

Provide script/gypi support for manifest generation

This provides a gypi action including well-defined variables, a python script, and a first client in ChromeVox (main and guest).

This generation code was born out of necessity as ChromeVox evolves and requires a varying set of manifests depending on such factors as compression strategy, debug/release builds, Chrome webstore releases, and ChromeVox next (version).

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272781

[Peter Lundblad, 2014-05-22 00:28:24]

https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/chromevox.gyp (right):

https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/chromevox.gyp:190:
'override_manifest': 'manifest_guest_overrides.json',
Shouldn't have build rules that generate files into the source tree.
Either we need to find a way to include these from the generated tree
as resources (we could actually load the manifests from the filesystem
instead of from .pak files as we do for everything else), or we
should make this a separate script and enforce it being run by PRESUBMIT.
I like the first better, but can live with the second since it is still
semi-automated.

https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/manifest_guest_overrides.json
(right):

https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/manifest_guest_overrides.json:1: {
Add a file comment.

https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/merge_manifests.gypi (right):

https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/merge_manifests.gypi:22: ],
You need to include the manifest files here as well for incremental builds to
work properly.

https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/tools/merge_manifests.py
(right):

https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/tools/merge_manifests.py:7: import
json
Care to provide a minimal docstring?

https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/tools/merge_manifests.py:12: '''
Takes two extension manifests and merges them into an output manifest.
Nit: space after '''

https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/tools/merge_manifests.py:20:
override_file = open(override)
Consider using with to have these automatically closed.

https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/tools/merge_manifests.py:24:
json.dump(base_manifest, open(output, 'w'), sort_keys=True, indent=2)
Can you put the open call out of line and close the file afterwards?

https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/tools/merge_manifests.py:37:
MergeDicts(base, override[key])
Shouldn't you merge override into base[key], creating it if it doesn't exist?

https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/tools/merge_manifests.py:45: '
<override_manifest_path> <output_manifest_path>')
I recommend making the out file an -o option to be consistent with many other
scripts.

[David Tseng, 2014-05-22 02:42:04]

https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/chromevox.gyp (right):

https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/chromevox.gyp:190:
'override_manifest': 'manifest_guest_overrides.json',
On 2014/05/22 00:28:25, Peter Lundblad wrote:
> Shouldn't have build rules that generate files into the source tree.
> Either we need to find a way to include these from the generated tree
> as resources (we could actually load the manifests from the filesystem
> instead of from .pak files as we do for everything else), or we
> should make this a separate script and enforce it being run by PRESUBMIT.
> I like the first better, but can live with the second since it is still
> semi-automated.

The first suggestion would deviate from how other component extension load their
manifests, so I'm not sure that would work. What if we made this rule a
dependency of packed resources? Let's chat about this one in person.

https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/manifest_guest_overrides.json
(right):

https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/manifest_guest_overrides.json:1: {
On 2014/05/22 00:28:25, Peter Lundblad wrote:
> Add a file comment.

Python's json loader doesn't like comments; I can strip coments as a first pass
if you think it's important.

https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/merge_manifests.gypi (right):

https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/merge_manifests.gypi:22: ],
On 2014/05/22 00:28:25, Peter Lundblad wrote:
> You need to include the manifest files here as well for incremental builds to
> work properly.

Done.

https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/tools/merge_manifests.py
(right):

https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/tools/merge_manifests.py:7: import
json
On 2014/05/22 00:28:25, Peter Lundblad wrote:
> Care to provide a minimal docstring?

Done.

https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/tools/merge_manifests.py:12: '''
Takes two extension manifests and merges them into an output manifest.
On 2014/05/22 00:28:25, Peter Lundblad wrote:
> Nit: space after '''

Remove it?
+  ''' Takes two extension manifests and merges them into an output manifest.

https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/tools/merge_manifests.py:20:
override_file = open(override)
On 2014/05/22 00:28:25, Peter Lundblad wrote:
> Consider using with to have these automatically closed.

Done. (Nice construct).

https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/tools/merge_manifests.py:24:
json.dump(base_manifest, open(output, 'w'), sort_keys=True, indent=2)
On 2014/05/22 00:28:25, Peter Lundblad wrote:
> Can you put the open call out of line and close the file afterwards?

Done.

https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/tools/merge_manifests.py:37:
MergeDicts(base, override[key])
On 2014/05/22 00:28:25, Peter Lundblad wrote:
> Shouldn't you merge override into base[key], creating it if it doesn't exist?

yYup; goofed on this case; also handle the case when base[key] is not a dict.

[Peter Lundblad, 2014-05-22 16:25:11]

dtseng@chromium.org writes:
> 
>
https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
> File chrome/browser/resources/chromeos/chromevox/chromevox.gyp (right):
> 
>
https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
> chrome/browser/resources/chromeos/chromevox/chromevox.gyp:190:
> 'override_manifest': 'manifest_guest_overrides.json',
> On 2014/05/22 00:28:25, Peter Lundblad wrote:
> > Shouldn't have build rules that generate files into the source tree.
> > Either we need to find a way to include these from the generated tree
> > as resources (we could actually load the manifests from the filesystem
> > instead of from .pak files as we do for everything else), or we
> > should make this a separate script and enforce it being run by
> PRESUBMIT.
> > I like the first better, but can live with the second since it is
> still
> > semi-automated.
> 
> The first suggestion would deviate from how other component extension
> load their manifests, so I'm not sure that would work.

There are already exceptions suhc as IME extensions.  Since we're already
dumping so many files on the image, a few little json files might not matter so
much I think.  It seems trivial to support loading the manifest from the
file system if others are ok with that.

>  What if we made
> this rule a dependency of packed resources? Let's chat about this one in
> person.

If we can find a way of specifying the path in the .grd file that's
also an option.

>
https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
> File
> chrome/browser/resources/chromeos/chromevox/manifest_guest_overrides.json
> (right):
> 
>
https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
> chrome/browser/resources/chromeos/chromevox/manifest_guest_overrides.json:1:
> {
> On 2014/05/22 00:28:25, Peter Lundblad wrote:
> > Add a file comment.
> 
> Python's json loader doesn't like comments; I can strip coments as a
> first pass if you think it's important.

Use the json_comment_eater module (in tools).  It is nice to be able to have
comments in
general.

>
https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
> File chrome/browser/resources/chromeos/chromevox/merge_manifests.gypi
> (right):
> 
>
https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
> chrome/browser/resources/chromeos/chromevox/merge_manifests.gypi:22: ],
> On 2014/05/22 00:28:25, Peter Lundblad wrote:
> > You need to include the manifest files here as well for incremental
> builds to
> > work properly.
> 
> Done.
> 
>
https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
> File
> chrome/browser/resources/chromeos/chromevox/tools/merge_manifests.py
> (right):
> 
>
https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
> chrome/browser/resources/chromeos/chromevox/tools/merge_manifests.py:7:
> import json
> On 2014/05/22 00:28:25, Peter Lundblad wrote:
> > Care to provide a minimal docstring?
> 
> Done.
> 
>
https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
> chrome/browser/resources/chromeos/chromevox/tools/merge_manifests.py:12:
> ''' Takes two extension manifests and merges them into an output
> manifest.
> On 2014/05/22 00:28:25, Peter Lundblad wrote:
> > Nit: space after '''
> 
> Remove it?
> +  ''' Takes two extension manifests and merges them into an output
> manifest.

Right, there is a space after the first ''' which looks atypical.

>
https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
> chrome/browser/resources/chromeos/chromevox/tools/merge_manifests.py:20:
> override_file = open(override)
> On 2014/05/22 00:28:25, Peter Lundblad wrote:
> > Consider using with to have these automatically closed.
> 
> Done. (Nice construct).
> 
>
https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
> chrome/browser/resources/chromeos/chromevox/tools/merge_manifests.py:24:
> json.dump(base_manifest, open(output, 'w'), sort_keys=True, indent=2)
> On 2014/05/22 00:28:25, Peter Lundblad wrote:
> > Can you put the open call out of line and close the file afterwards?
> 
> Done.
> 
>
https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
> chrome/browser/resources/chromeos/chromevox/tools/merge_manifests.py:37:
> MergeDicts(base, override[key])
> On 2014/05/22 00:28:25, Peter Lundblad wrote:
> > Shouldn't you merge override into base[key], creating it if it doesn't
> exist?
> 
> yYup; goofed on this case; also handle the case when base[key] is not a
> dict.
> 
> https://codereview.chromium.org/295123002/

-- 

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2014-05-22 18:26:01]

Addressed all other comments.

Wrt packed vs unpacked, the packed resources action gets run after we
generate the manifest, so at least for this cl, the discussion doesn't seem
relevant. The IME code has its own conventions about
manifests(ComponentExtensionIMEManager).


On Thu, May 22, 2014 at 9:25 AM, <plundblad@chromium.org> wrote:

> dtseng@chromium.org writes:
> >
> >
>
https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
> > File chrome/browser/resources/chromeos/chromevox/chromevox.gyp (right):
> >
> >
>
https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
> > chrome/browser/resources/chromeos/chromevox/chromevox.gyp:190:
> > 'override_manifest': 'manifest_guest_overrides.json',
> > On 2014/05/22 00:28:25, Peter Lundblad wrote:
> > > Shouldn't have build rules that generate files into the source tree.
> > > Either we need to find a way to include these from the generated tree
> > > as resources (we could actually load the manifests from the filesystem
> > > instead of from .pak files as we do for everything else), or we
> > > should make this a separate script and enforce it being run by
> > PRESUBMIT.
> > > I like the first better, but can live with the second since it is
> > still
> > > semi-automated.
> >
> > The first suggestion would deviate from how other component extension
> > load their manifests, so I'm not sure that would work.
>
> There are already exceptions suhc as IME extensions.  Since we're already
> dumping so many files on the image, a few little json files might not
> matter so
> much I think.  It seems trivial to support loading the manifest from the
> file system if others are ok with that.
>
> >  What if we made
> > this rule a dependency of packed resources? Let's chat about this one in
> > person.
>
> If we can find a way of specifying the path in the .grd file that's
> also an option.
>
> >
>
https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
> > File
> > chrome/browser/resources/chromeos/chromevox/manifest_guest_overrides.json
> > (right):
> >
> >
>
https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
> >
> chrome/browser/resources/chromeos/chromevox/manifest_guest_overrides.json:1:
> > {
> > On 2014/05/22 00:28:25, Peter Lundblad wrote:
> > > Add a file comment.
> >
> > Python's json loader doesn't like comments; I can strip coments as a
> > first pass if you think it's important.
>
> Use the json_comment_eater module (in tools).  It is nice to be able to
> have comments in
> general.
>
> >
>
https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
> > File chrome/browser/resources/chromeos/chromevox/merge_manifests.gypi
> > (right):
> >
> >
>
https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
> > chrome/browser/resources/chromeos/chromevox/merge_manifests.gypi:22: ],
> > On 2014/05/22 00:28:25, Peter Lundblad wrote:
> > > You need to include the manifest files here as well for incremental
> > builds to
> > > work properly.
> >
> > Done.
> >
> >
>
https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
> > File
> > chrome/browser/resources/chromeos/chromevox/tools/merge_manifests.py
> > (right):
> >
> >
>
https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
> > chrome/browser/resources/chromeos/chromevox/tools/merge_manifests.py:7:
> > import json
> > On 2014/05/22 00:28:25, Peter Lundblad wrote:
> > > Care to provide a minimal docstring?
> >
> > Done.
> >
> >
>
https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
> > chrome/browser/resources/chromeos/chromevox/tools/merge_manifests.py:12:
> > ''' Takes two extension manifests and merges them into an output
> > manifest.
> > On 2014/05/22 00:28:25, Peter Lundblad wrote:
> > > Nit: space after '''
> >
> > Remove it?
> > +  ''' Takes two extension manifests and merges them into an output
> > manifest.
>
> Right, there is a space after the first ''' which looks atypical.
>
> >
>
https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
> > chrome/browser/resources/chromeos/chromevox/tools/merge_manifests.py:20:
> > override_file = open(override)
> > On 2014/05/22 00:28:25, Peter Lundblad wrote:
> > > Consider using with to have these automatically closed.
> >
> > Done. (Nice construct).
> >
> >
>
https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
> > chrome/browser/resources/chromeos/chromevox/tools/merge_manifests.py:24:
> > json.dump(base_manifest, open(output, 'w'), sort_keys=True, indent=2)
> > On 2014/05/22 00:28:25, Peter Lundblad wrote:
> > > Can you put the open call out of line and close the file afterwards?
> >
> > Done.
> >
> >
>
https://codereview.chromium.org/295123002/diff/60001/chrome/browser/resources...
> > chrome/browser/resources/chromeos/chromevox/tools/merge_manifests.py:37:
> > MergeDicts(base, override[key])
> > On 2014/05/22 00:28:25, Peter Lundblad wrote:
> > > Shouldn't you merge override into base[key], creating it if it doesn't
> > exist?
> >
> > yYup; goofed on this case; also handle the case when base[key] is not a
> > dict.
> >
> > https://codereview.chromium.org/295123002/
>
> --
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2014-05-23 03:41:07]

+ kalman for extensions related changes,
+ darin for grd (only removing things).

@plundblad, went with Jinja2 (spin off of Jango templates) as discussed offline.

[darin (slow to review), 2014-05-23 03:43:03]

LGTM for GRD changes

(i'm a fan of jinja2 too!)

[Peter Lundblad, 2014-05-23 16:04:27]

Nice!

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/chromevox.gyp (right):

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/chromevox.gyp:191:
'output_manifest': '<(PRODUCT_DIR)/resources/chromeos/chromevox/manifest.json',
Use chromevox_dest_dir here.

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/chromevox.gyp:199:
'template_manifest': 'manifest.json.jinja2',
Move this outside to avoid duplication.

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/chromevox.gyp:200:
'output_manifest':
'<(PRODUCT_DIR)/resources/chromeos/chromevox/manifest_guest.json',
chromevox_dest_dir

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi (right):

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi:5: #
Generates an output manifest based on a Jinja2 templated manifest.
nit: s/an output/a/

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi:5: #
Generates an output manifest based on a Jinja2 templated manifest.
nit: mention that this is expected to be included inside a target.

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi:8: #
template_manifest: a valid JSON file path.
nit: it's a path to a jinja2 file.

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi:8: #
template_manifest: a valid JSON file path.
Usually file name variable end in -Ifle and _path so that relative paths are
handled correctly on merging.

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi:9: #
output_manifest: file path with the resulting manifest.
s/with/for?

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/manifest.json.jinja2 (right):

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/manifest.json.jinja2:9: {% endif %}
this is so clear and nice!

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/tools/generate_manifest.py
(right):

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/tools/generate_manifest.py:20:
sys.path.append(os.path.split(jinja2_path)[0])
Why are you including a path component that you later discard?

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/tools/generate_manifest.py:20:
sys.path.append(os.path.split(jinja2_path)[0])
I recommend prepending so we pick up our own jinja2 in case it happens
to be available on the build system.  Also, would it be better to do
this at the top level to not have this function have the side effect of
modifying the global path (every time it gets called)?

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/tools/generate_manifest.py:26:
io.open(output_file, 'w', encoding='utf-8').write(rendered).close()
nit: Why io.open?

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/tools/generate_manifest.py:26:
io.open(output_file, 'w', encoding='utf-8').write(rendered).close()
Ho would you feel about quickly loading the result into json to catch
syntax errors early.

also, split this line up and close the file on error.

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/tools/generate_manifest.py:43:
processJinjaTemplate(args[0], options.output_manifest, parser.values.__dict__)
What is parser.values.__dict__ for?

[not at google - send to devlin, 2014-05-23 16:58:44]

extensions parts lgtm

[David Tseng, 2014-05-23 19:16:36]

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/chromevox.gyp (right):

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/chromevox.gyp:191:
'output_manifest': '<(PRODUCT_DIR)/resources/chromeos/chromevox/manifest.json',
On 2014/05/23 16:04:27, Peter Lundblad wrote:
> Use chromevox_dest_dir here.

Done.

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/chromevox.gyp:199:
'template_manifest': 'manifest.json.jinja2',
On 2014/05/23 16:04:27, Peter Lundblad wrote:
> Move this outside to avoid duplication.

Done.

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/chromevox.gyp:200:
'output_manifest':
'<(PRODUCT_DIR)/resources/chromeos/chromevox/manifest_guest.json',
On 2014/05/23 16:04:27, Peter Lundblad wrote:
> chromevox_dest_dir

Done.

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi (right):

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi:5: #
Generates an output manifest based on a Jinja2 templated manifest.
On 2014/05/23 16:04:27, Peter Lundblad wrote:
> nit: mention that this is expected to be included inside a target.

Done.

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi:5: #
Generates an output manifest based on a Jinja2 templated manifest.
On 2014/05/23 16:04:27, Peter Lundblad wrote:
> nit: s/an output/a/

I don't claim to be a grammar wiz, but greping for "a output" resulted in no
results while there was one other use of "an output" (in a gypi file).

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi:8: #
template_manifest: a valid JSON file path.
On 2014/05/23 16:04:27, Peter Lundblad wrote:
> nit: it's a path to a jinja2 file.

Done.

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi:9: #
output_manifest: file path with the resulting manifest.
On 2014/05/23 16:04:27, Peter Lundblad wrote:
> s/with/for?

Sure; why not.

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
File chrome/browser/resources/chromeos/chromevox/tools/generate_manifest.py
(right):

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/tools/generate_manifest.py:20:
sys.path.append(os.path.split(jinja2_path)[0])
On 2014/05/23 16:04:27, Peter Lundblad wrote:
> Why are you including a path component that you later discard?

Took this code from remoting; revised.

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/tools/generate_manifest.py:26:
io.open(output_file, 'w', encoding='utf-8').write(rendered).close()
On 2014/05/23 16:04:27, Peter Lundblad wrote:
> nit: Why io.open?

Ditto; no I suspect it's because open doesn't let you specify encoding.

Also, 
"io module provides the Python interfaces to stream handling. Under Python 2.x,
this is proposed as an alternative to the built-in file object, but in Python
3.x it is the default interface to access files and streams."
from the python docs.

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/tools/generate_manifest.py:26:
io.open(output_file, 'w', encoding='utf-8').write(rendered).close()
On 2014/05/23 16:04:27, Peter Lundblad wrote:
> Ho would you feel about quickly loading the result into json to catch
> syntax errors early.
> 
> also, split this line up and close the file on error.

Sure. sgtm.

https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
chrome/browser/resources/chromeos/chromevox/tools/generate_manifest.py:43:
processJinjaTemplate(args[0], options.output_manifest, parser.values.__dict__)
On 2014/05/23 16:04:27, Peter Lundblad wrote:
> What is parser.values.__dict__ for?

Allows us to pass the jinja2 "renderer" all options (e.g.
{ "is_guest_mode": 1})

without having to manually list everything.
If you know of a cleaner way to do this with OptPrase, I'm open to it.

[Peter Lundblad, 2014-05-23 22:29:22]

dtseng@chromium.org writes:
> 
>
https://codereview.chromium.org/295123002/diff/90001/chrome/browser/resources...
> chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi:5: #
> Generates an output manifest based on a Jinja2 templated manifest.
> On 2014/05/23 16:04:27, Peter Lundblad wrote:
> > nit: s/an output/a/
> 
> I don't claim to be a grammar wiz, but greping for "a output" resulted
> in no results while there was one other use of "an output" (in a gypi
> file).

You need to apply my sugested substitution fully;)  I actually suggested
to get rid of 'an output' because it seemed redundant (what else do you generate
if
not output?).  Now, this is a minor nit...

//Peter

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Lundblad, 2014-05-23 22:37:18]

lgtm

Very cool!

https://codereview.chromium.org/295123002/diff/150001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi (right):

https://codereview.chromium.org/295123002/diff/150001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi:7: # The
following variables must be set before including this file:
Mention is_guest_manifest as an optional variable for completeness.

https://codereview.chromium.org/295123002/diff/150001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi:10: #
output_manifest: file path for the resulting manifest.
Variables naming files commonly end in _file or _path.  The reason for that is
so that
gyp fixes relative paths if the gypi is included from another directory and
it does that for variables with certain suffixes.  Care to rename these two?

https://codereview.chromium.org/295123002/diff/150001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/tools/generate_manifest.py
(right):

https://codereview.chromium.org/295123002/diff/150001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/tools/generate_manifest.py:32: #
Simply for validation
nit: Sentences end with a period.

[David Tseng, 2014-05-23 22:57:16]

https://codereview.chromium.org/295123002/diff/150001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi (right):

https://codereview.chromium.org/295123002/diff/150001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi:7: # The
following variables must be set before including this file:
On 2014/05/23 22:37:19, Peter Lundblad wrote:
> Mention is_guest_manifest as an optional variable for completeness.

Done.

https://codereview.chromium.org/295123002/diff/150001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/generate_manifest.gypi:10: #
output_manifest: file path for the resulting manifest.
On 2014/05/23 22:37:19, Peter Lundblad wrote:
> Variables naming files commonly end in _file or _path.  The reason for that is
> so that
> gyp fixes relative paths if the gypi is included from another directory and
> it does that for variables with certain suffixes.  Care to rename these two?

Obscure; thanks and done.

https://codereview.chromium.org/295123002/diff/150001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/tools/generate_manifest.py
(right):

https://codereview.chromium.org/295123002/diff/150001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/tools/generate_manifest.py:32: #
Simply for validation
On 2014/05/23 22:37:19, Peter Lundblad wrote:
> nit: Sentences end with a period.

Done.

[David Tseng, 2014-05-23 22:59:02]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2014-05-23 23:00:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dtseng@chromium.org/295123002/170001

[commit-bot: I haz the power, 2014-05-24 00:36:54]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/8831)

[commit-bot: I haz the power, 2014-05-24 00:42:45]

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

[commit-bot: I haz the power, 2014-05-24 00:42:46]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/8865)

[David Tseng, 2014-05-26 00:19:42]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2014-05-26 00:19:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dtseng@chromium.org/295123002/190001

[commit-bot: I haz the power, 2014-05-26 04:11:56]

Change committed as 272781