Generate the proto JSON converter for DOM distiller

Generate the proto JSON converter for DOM distiller

The protoc_plugins files are from:
https://github.com/chromium/dom-distiller/tree/master/protoc_plugins

This goes along with https://codereview.chromium.org/2039043003/

Roll DOM Distiller JavaScript distribution package

    Diff since last roll:
    https://github.com/chromium/dom-distiller/compare/91f9f016e0...b21e6a334a

    Picked up changes:
    b21e6a3 Remove //base dependency

BUG=572585
Committed: https://crrev.com/fa35db7c66a96892dc247a1645ef2c26650dd4fa
Cr-Commit-Position: refs/heads/master@{#412445}

[wychen, 2016-06-06 18:44:03]

Description was changed from

==========
Generate the proto JSON converter for DOM distiller

The protoc_plugins files are from:
https://github.com/chromium/dom-distiller/tree/master/protoc_plugins

BUG=572585
==========

to

==========
Generate the proto JSON converter for DOM distiller

The protoc_plugins files are from:
https://github.com/chromium/dom-distiller/tree/master/protoc_plugins

This goes along with https://codereview.chromium.org/2039043003/

BUG=572585
==========

[wychen, 2016-06-06 18:45:08]

wychen@chromium.org changed reviewers:
+ dcheng@chromium.org, mdjones@chromium.org, nyquist@chromium.org

[wychen, 2016-06-06 18:45:09]

PTAL

[mdjones, 2016-06-13 22:46:13]

lgtm

[nyquist, 2016-06-14 00:48:11]

https://codereview.chromium.org/2034373002/diff/1/third_party/dom_distiller_j...
File third_party/dom_distiller_js/protoc_plugins/README (right):

https://codereview.chromium.org/2034373002/diff/1/third_party/dom_distiller_j...
third_party/dom_distiller_js/protoc_plugins/README:1: # Copyright 2014 The
Chromium Authors. All rights reserved.
Nit: 2016

https://codereview.chromium.org/2034373002/diff/1/third_party/dom_distiller_j...
third_party/dom_distiller_js/protoc_plugins/README:9: message Foo {
Would it be easier to read this in markdown format?

https://codereview.chromium.org/2034373002/diff/1/third_party/dom_distiller_j...
File third_party/dom_distiller_js/protoc_plugins/json_values_converter.py
(right):

https://codereview.chromium.org/2034373002/diff/1/third_party/dom_distiller_j...
third_party/dom_distiller_js/protoc_plugins/json_values_converter.py:6:
"""protoc plugin to create c++ reader/writer for json-encoded protobufs
Nit: C++, JSON, etc.

https://codereview.chromium.org/2034373002/diff/1/third_party/dom_distiller_j...
third_party/dom_distiller_js/protoc_plugins/json_values_converter.py:17: class
CppConverterWriter(writer.CodeWriter):
Could we have a sanity-check style test for this to ensure that we catch
regressions? Maybe with a "golden file", similarly to what we have for JNI?

https://codereview.chromium.org/2034373002/diff/1/third_party/dom_distiller_j...
third_party/dom_distiller_js/protoc_plugins/json_values_converter.py:251: #
TODO(cjhopman): This should only be generating files for files listed in
I don't think cjhopman is planning on doing this.

https://codereview.chromium.org/2034373002/diff/1/third_party/dom_distiller_j...
File third_party/dom_distiller_js/protoc_plugins/util/plugin.py (right):

https://codereview.chromium.org/2034373002/diff/1/third_party/dom_distiller_j...
third_party/dom_distiller_js/protoc_plugins/util/plugin.py:18: return
''.join((p[0].upper() + p[1:] for p in s.split('_')))
Do you need the inner parenthesis here?

https://codereview.chromium.org/2034373002/diff/1/third_party/dom_distiller_j...
File third_party/dom_distiller_js/protoc_plugins/util/writer.py (right):

https://codereview.chromium.org/2034373002/diff/1/third_party/dom_distiller_j...
third_party/dom_distiller_js/protoc_plugins/util/writer.py:1: # Copyright 2014
The Chromium Authors. All rights reserved.
Could you in general update these numbers?

https://codereview.chromium.org/2034373002/diff/1/third_party/dom_distiller_j...
third_party/dom_distiller_js/protoc_plugins/util/writer.py:7: class
CodeWriter(object):
Could you add an explanatory comment for this class?

[dcheng, 2016-06-14 04:27:39]

Yay! Just some quick thoughts

https://codereview.chromium.org/2034373002/diff/1/third_party/protobuf/proto_...
File third_party/protobuf/proto_library.gni (right):

https://codereview.chromium.org/2034373002/diff/1/third_party/protobuf/proto_...
third_party/protobuf/proto_library.gni:156:
"--json_converter_out=output_dir=:$rel_out_dir",
Seems a bit weird to specify this here, maybe we can just have an
additional_args parameter?

Also, is it possible to pas more than one --plugin argument to protoc? It'd be
nice not to have to duplicate all the protoc_plugins.

[wychen, 2016-08-07 02:27:12]

Patchset #2 (id:20001) has been deleted

[wychen, 2016-08-07 02:57:24]

Patchset #2 (id:40001) has been deleted

[wychen, 2016-08-07 09:16:27]

https://codereview.chromium.org/2034373002/diff/1/third_party/dom_distiller_j...
File third_party/dom_distiller_js/protoc_plugins/README (right):

https://codereview.chromium.org/2034373002/diff/1/third_party/dom_distiller_j...
third_party/dom_distiller_js/protoc_plugins/README:1: # Copyright 2014 The
Chromium Authors. All rights reserved.
On 2016/06/14 00:48:11, nyquist wrote:
> Nit: 2016

Done.

https://codereview.chromium.org/2034373002/diff/1/third_party/dom_distiller_j...
third_party/dom_distiller_js/protoc_plugins/README:9: message Foo {
On 2016/06/14 00:48:11, nyquist wrote:
> Would it be easier to read this in markdown format?

Done.

https://codereview.chromium.org/2034373002/diff/1/third_party/dom_distiller_j...
File third_party/dom_distiller_js/protoc_plugins/json_values_converter.py
(right):

https://codereview.chromium.org/2034373002/diff/1/third_party/dom_distiller_j...
third_party/dom_distiller_js/protoc_plugins/json_values_converter.py:6:
"""protoc plugin to create c++ reader/writer for json-encoded protobufs
On 2016/06/14 00:48:11, nyquist wrote:
> Nit: C++, JSON, etc.

Done.

https://codereview.chromium.org/2034373002/diff/1/third_party/dom_distiller_j...
third_party/dom_distiller_js/protoc_plugins/json_values_converter.py:17: class
CppConverterWriter(writer.CodeWriter):
On 2016/06/14 00:48:11, nyquist wrote:
> Could we have a sanity-check style test for this to ensure that we catch
> regressions? Maybe with a "golden file", similarly to what we have for JNI?

Done.

https://codereview.chromium.org/2034373002/diff/1/third_party/dom_distiller_j...
third_party/dom_distiller_js/protoc_plugins/json_values_converter.py:251: #
TODO(cjhopman): This should only be generating files for files listed in
On 2016/06/14 00:48:11, nyquist wrote:
> I don't think cjhopman is planning on doing this.

Done.

https://codereview.chromium.org/2034373002/diff/1/third_party/dom_distiller_j...
File third_party/dom_distiller_js/protoc_plugins/util/plugin.py (right):

https://codereview.chromium.org/2034373002/diff/1/third_party/dom_distiller_j...
third_party/dom_distiller_js/protoc_plugins/util/plugin.py:18: return
''.join((p[0].upper() + p[1:] for p in s.split('_')))
On 2016/06/14 00:48:11, nyquist wrote:
> Do you need the inner parenthesis here?

Done.

https://codereview.chromium.org/2034373002/diff/1/third_party/dom_distiller_j...
File third_party/dom_distiller_js/protoc_plugins/util/writer.py (right):

https://codereview.chromium.org/2034373002/diff/1/third_party/dom_distiller_j...
third_party/dom_distiller_js/protoc_plugins/util/writer.py:1: # Copyright 2014
The Chromium Authors. All rights reserved.
On 2016/06/14 00:48:11, nyquist wrote:
> Could you in general update these numbers?

Done.

https://codereview.chromium.org/2034373002/diff/1/third_party/dom_distiller_j...
third_party/dom_distiller_js/protoc_plugins/util/writer.py:7: class
CodeWriter(object):
On 2016/06/14 00:48:11, nyquist wrote:
> Could you add an explanatory comment for this class?

Done.

https://codereview.chromium.org/2034373002/diff/1/third_party/protobuf/proto_...
File third_party/protobuf/proto_library.gni (right):

https://codereview.chromium.org/2034373002/diff/1/third_party/protobuf/proto_...
third_party/protobuf/proto_library.gni:156:
"--json_converter_out=output_dir=:$rel_out_dir",
On 2016/06/14 04:27:39, dcheng (OOO Aug 1 - Aug 11) wrote:
> Seems a bit weird to specify this here, maybe we can just have an
> additional_args parameter?
> 
> Also, is it possible to pas more than one --plugin argument to protoc? It'd be
> nice not to have to duplicate all the protoc_plugins.

Moving all the logic to dom_distiller_js/BUILD.gn can also work, but I'm trying
to make this work as a template for protoc plugins. I'm not sure how general
this is, since we are the first one using plugins here. The benefit of putting
the logic here is that it will be visible when there are more users of the
plugins in the future. If they happen to be using json_converter, then this can
work as is. For other kinds of plugins, we'll see how much of the logic can be
factored out when the time comes.

[wychen, 2016-08-08 19:02:14]

wychen@chromium.org changed reviewers:
+ pkasting@chromium.org

[wychen, 2016-08-08 19:02:15]

pkasting@, could you take a look at protobuf GN file? Thanks!

[Peter Kasting, 2016-08-08 19:57:38]

On 2016/08/08 19:02:15, wychen wrote:
> pkasting@, could you take a look at protobuf GN file? Thanks!

I don't really speak GN, so if you were looking for a careful syntactic review,
you'd need someone else.  LGTM in general principle.

[nyquist, 2016-08-10 01:01:42]

lgtm

https://codereview.chromium.org/2034373002/diff/140001/third_party/dom_distil...
File third_party/dom_distiller_js/BUILD.gn (right):

https://codereview.chromium.org/2034373002/diff/140001/third_party/dom_distil...
third_party/dom_distiller_js/BUILD.gn:22: json_converter =
"protoc_plugins/json_values_converter.py"
Is there any way that setting this json_converter would automatically include
some resources? (the inputs below)

[wychen, 2016-08-10 01:08:33]

wychen@chromium.org changed reviewers:
+ dpranke@chromium.org

[wychen, 2016-08-10 01:08:34]

dpranke@, could you take a look at the GN part? Thank you!

[wychen, 2016-08-10 01:15:05]

https://codereview.chromium.org/2034373002/diff/140001/third_party/dom_distil...
File third_party/dom_distiller_js/BUILD.gn (right):

https://codereview.chromium.org/2034373002/diff/140001/third_party/dom_distil...
third_party/dom_distiller_js/BUILD.gn:22: json_converter =
"protoc_plugins/json_values_converter.py"
On 2016/08/10 01:01:42, nyquist wrote:
> Is there any way that setting this json_converter would automatically include
> some resources? (the inputs below)

If the plugin is a native executive, we already have dependency covered since it
needs to be built first. For dynamic scripts, we might be able to use pkgutil to
generate the list, given it's python, but I don't know how thorough and general
it needs to be. Since we are the only python plugin here, we can wait for a few
more to appear before doing this optimization.

[Dirk Pranke, 2016-08-11 00:33:53]

https://codereview.chromium.org/2034373002/diff/140001/third_party/dom_distil...
File third_party/dom_distiller_js/BUILD.gn (right):

https://codereview.chromium.org/2034373002/diff/140001/third_party/dom_distil...
third_party/dom_distiller_js/BUILD.gn:22: json_converter =
"protoc_plugins/json_values_converter.py"
On 2016/08/10 01:15:05, wychen wrote:
> On 2016/08/10 01:01:42, nyquist wrote:
> > Is there any way that setting this json_converter would automatically
include
> > some resources? (the inputs below)
> 
> If the plugin is a native executive, we already have dependency covered since
it
> needs to be built first. For dynamic scripts, we might be able to use pkgutil
to
> generate the list, given it's python, but I don't know how thorough and
general
> it needs to be. Since we are the only python plugin here, we can wait for a
few
> more to appear before doing this optimization.

I suggest for now you create a file-scoped variable w/ this list of files, and
just reuse that, e.g.:

protoc_plugin_files = [ ... ]

proto_library("dom_distiller_proto") {
  ...
  inputs = protoc_plugin_files
  ...
}

etc.

Also, I don't see a 'json_converter' variable in the proto_library() template.
Am I missing something?

[wychen, 2016-08-11 01:32:19]

https://codereview.chromium.org/2034373002/diff/140001/third_party/dom_distil...
File third_party/dom_distiller_js/BUILD.gn (right):

https://codereview.chromium.org/2034373002/diff/140001/third_party/dom_distil...
third_party/dom_distiller_js/BUILD.gn:22: json_converter =
"protoc_plugins/json_values_converter.py"
On 2016/08/11 00:33:53, Dirk Pranke wrote:
> On 2016/08/10 01:15:05, wychen wrote:
> > On 2016/08/10 01:01:42, nyquist wrote:
> > > Is there any way that setting this json_converter would automatically
> include
> > > some resources? (the inputs below)
> > 
> > If the plugin is a native executive, we already have dependency covered
since
> it
> > needs to be built first. For dynamic scripts, we might be able to use
pkgutil
> to
> > generate the list, given it's python, but I don't know how thorough and
> general
> > it needs to be. Since we are the only python plugin here, we can wait for a
> few
> > more to appear before doing this optimization.
> 
> I suggest for now you create a file-scoped variable w/ this list of files, and
> just reuse that, e.g.:
> 
> protoc_plugin_files = [ ... ]
> 
> proto_library("dom_distiller_proto") {
>   ...
>   inputs = protoc_plugin_files
>   ...
> }
> 
> etc.
> 

Done

> Also, I don't see a 'json_converter' variable in the proto_library() template.
> Am I missing something?

I'm using invoker.json_converter without creating a local variable. Is this the
wrong way to do things?

[Dirk Pranke, 2016-08-11 01:36:28]

On 2016/08/11 01:32:19, wychen wrote:
> > Also, I don't see a 'json_converter' variable in the proto_library()
template.
> > Am I missing something?
> 
> I'm using invoker.json_converter without creating a local variable. Is this
the
> wrong way to do things?

Sigh, I just missed the proto_library.gni changes completely. Sorry.

Things look fine ( lgtm ).

[wychen, 2016-08-11 02:09:26]

On 2016/08/11 01:36:28, Dirk Pranke wrote:
> On 2016/08/11 01:32:19, wychen wrote:
> > > Also, I don't see a 'json_converter' variable in the proto_library()
> template.
> > > Am I missing something?
> > 
> > I'm using invoker.json_converter without creating a local variable. Is this
> the
> > wrong way to do things?
> 
> Sigh, I just missed the proto_library.gni changes completely. Sorry.
> 
> Things look fine ( lgtm ).

Thanks for your review!

[wychen, 2016-08-15 23:26:18]

Description was changed from

==========
Generate the proto JSON converter for DOM distiller

The protoc_plugins files are from:
https://github.com/chromium/dom-distiller/tree/master/protoc_plugins

This goes along with https://codereview.chromium.org/2039043003/

BUG=572585
==========

to

==========
Generate the proto JSON converter for DOM distiller

The protoc_plugins files are from:
https://github.com/chromium/dom-distiller/tree/master/protoc_plugins

This goes along with https://codereview.chromium.org/2039043003/

Roll DOM Distiller JavaScript distribution package
    
    Diff since last roll:
    https://github.com/chromium/dom-distiller/compare/91f9f016e0...b21e6a334a
    
    Picked up changes:
    b21e6a3 Remove //base dependency

BUG=572585
==========

[wychen, 2016-08-15 23:27:13]

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

[wychen, 2016-08-15 23:27:44]

Patchset #8 (id:180001) has been deleted

[commit-bot: I haz the power, 2016-08-15 23:27:53]

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, 2016-08-15 23:42:16]

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

[commit-bot: I haz the power, 2016-08-15 23:42:16]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[wychen, 2016-08-15 23:54:42]

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

[commit-bot: I haz the power, 2016-08-15 23:55:03]

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, 2016-08-16 00:43:45]

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

[commit-bot: I haz the power, 2016-08-16 00:43:46]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[wychen, 2016-08-17 01:43:33]

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

[commit-bot: I haz the power, 2016-08-17 01:44:08]

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, 2016-08-17 02:36:27]

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

[commit-bot: I haz the power, 2016-08-17 02:36:28]

Dry run: This issue passed the CQ dry run.

[wychen, 2016-08-17 03:49:33]

The CQ bit was checked by wychen@chromium.org

[wychen, 2016-08-17 03:49:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
mdjones@chromium.org, pkasting@chromium.org, nyquist@chromium.org,
dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/2034373002/#ps240001
(title: "Windows-specific fixes")

[commit-bot: I haz the power, 2016-08-17 03:49:46]

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, 2016-08-17 03:54:38]

Description was changed from

==========
Generate the proto JSON converter for DOM distiller

The protoc_plugins files are from:
https://github.com/chromium/dom-distiller/tree/master/protoc_plugins

This goes along with https://codereview.chromium.org/2039043003/

Roll DOM Distiller JavaScript distribution package
    
    Diff since last roll:
    https://github.com/chromium/dom-distiller/compare/91f9f016e0...b21e6a334a
    
    Picked up changes:
    b21e6a3 Remove //base dependency

BUG=572585
==========

to

==========
Generate the proto JSON converter for DOM distiller

The protoc_plugins files are from:
https://github.com/chromium/dom-distiller/tree/master/protoc_plugins

This goes along with https://codereview.chromium.org/2039043003/

Roll DOM Distiller JavaScript distribution package
    
    Diff since last roll:
    https://github.com/chromium/dom-distiller/compare/91f9f016e0...b21e6a334a
    
    Picked up changes:
    b21e6a3 Remove //base dependency

BUG=572585
==========

[commit-bot: I haz the power, 2016-08-17 03:54:40]

Committed patchset #10 (id:240001)

[commit-bot: I haz the power, 2016-08-17 03:57:23]

Description was changed from

==========
Generate the proto JSON converter for DOM distiller

The protoc_plugins files are from:
https://github.com/chromium/dom-distiller/tree/master/protoc_plugins

This goes along with https://codereview.chromium.org/2039043003/

Roll DOM Distiller JavaScript distribution package
    
    Diff since last roll:
    https://github.com/chromium/dom-distiller/compare/91f9f016e0...b21e6a334a
    
    Picked up changes:
    b21e6a3 Remove //base dependency

BUG=572585
==========

to

==========
Generate the proto JSON converter for DOM distiller

The protoc_plugins files are from:
https://github.com/chromium/dom-distiller/tree/master/protoc_plugins

This goes along with https://codereview.chromium.org/2039043003/

Roll DOM Distiller JavaScript distribution package

    Diff since last roll:
    https://github.com/chromium/dom-distiller/compare/91f9f016e0...b21e6a334a

    Picked up changes:
    b21e6a3 Remove //base dependency

BUG=572585

Committed: https://crrev.com/fa35db7c66a96892dc247a1645ef2c26650dd4fa
Cr-Commit-Position: refs/heads/master@{#412445}
==========

[commit-bot: I haz the power, 2016-08-17 03:57:24]

Patchset 10 (id:??) landed as
https://crrev.com/fa35db7c66a96892dc247a1645ef2c26650dd4fa
Cr-Commit-Position: refs/heads/master@{#412445}

[xyzzyz, 2016-08-17 23:32:04]

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

[xyzzyz, 2016-08-17 23:32:05]

The json_converter addition in proto_library.gni overlaps the existing mechanism
which is guided by generator_plugin_* options. Please, revert the change to
proto_library.gni, and use the existing functionality, possibly updating it if
it doesn't suit your use case. Also be aware of an ongoing effort to refactor
proto generation: https://codereview.chromium.org/2239383002/ .