Tracing V2: Import support for protobuf plugin.

Tracing V2: Import support for protobuf plugin.

Introduce processing of import references in proto files.

BUG=608721
Committed: https://crrev.com/faa7234dcd0a10e53dd9313869a9aace6d651af1
Cr-Commit-Position: refs/heads/master@{#409871}

[kraynov, 2016-08-03 14:26:23]

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

[commit-bot: I haz the power, 2016-08-03 14:26:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kraynov, 2016-08-03 14:30:00]

The CQ bit was unchecked by kraynov@chromium.org

[kraynov, 2016-08-03 14:57:03]

kraynov@chromium.org changed reviewers:
+ primiano@chromium.org

[kraynov, 2016-08-03 14:57:04]

PTAL

[Primiano Tucci (use gerrit), 2016-08-03 15:36:28]

Can we have fwd declarations instad of generating #imports?

https://codereview.chromium.org/2201393002/diff/1/components/tracing/test/exa...
File components/tracing/test/example_proto/various_messages.proto (right):

https://codereview.chromium.org/2201393002/diff/1/components/tracing/test/exa...
components/tracing/test/example_proto/various_messages.proto:13: // Tests
importing message definition frm another proto file.
s/frm/from/

https://codereview.chromium.org/2201393002/diff/1/components/tracing/tools/pr...
File components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc (right):

https://codereview.chromium.org/2201393002/diff/1/components/tracing/tools/pr...
components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:172:
Abort("Imported proto must be in the same package.");
out of curiosity, why?

https://codereview.chromium.org/2201393002/diff/1/components/tracing/tools/pr...
components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:180: //
Laconiacally said:
nit: s/Laconiacally/Laconically/

[kraynov, 2016-08-04 12:03:55]

https://codereview.chromium.org/2201393002/diff/1/components/tracing/tools/pr...
File components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc (right):

https://codereview.chromium.org/2201393002/diff/1/components/tracing/tools/pr...
components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:172:
Abort("Imported proto must be in the same package.");
On 2016/08/03 15:36:28, Primiano Tucci wrote:
> out of curiosity, why?

It would be complex to deal with namespaces in this case.

[kraynov, 2016-08-04 12:06:22]

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

[commit-bot: I haz the power, 2016-08-04 12:06:38]

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-04 13:17:06]

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

[commit-bot: I haz the power, 2016-08-04 13:17:06]

Dry run: This issue passed the CQ dry run.

[Primiano Tucci (use gerrit), 2016-08-04 13:32:05]

LGTM thanks

https://codereview.chromium.org/2201393002/diff/20001/components/tracing/BUIL...
File components/tracing/BUILD.gn (right):

https://codereview.chromium.org/2201393002/diff/20001/components/tracing/BUIL...
components/tracing/BUILD.gn:68: "test/example_proto/various_messages.proto",
maybe give it a more meaningful name than "various_messages". Maybe
outermost_message? encapsulated_messages?

https://codereview.chromium.org/2201393002/diff/20001/components/tracing/tool...
File components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc (right):

https://codereview.chromium.org/2201393002/diff/20001/components/tracing/tool...
components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:149:
std::vector<const FileDescriptor*> stack;
in general you want to name variables according to what they contain, not their
implementation.
Maybe this is a "deps_stack"? dependent_files?

https://codereview.chromium.org/2201393002/diff/20001/components/tracing/tool...
components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:179: if
(public_imports_.find(field->message_type()->file()) ==
.count(...) == 0

https://codereview.chromium.org/2201393002/diff/20001/components/tracing/tool...
components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:181: // Avoid
multiple forward declarations.
not sure this comment adds anything useful. same below.

[kraynov, 2016-08-04 13:42:48]

https://codereview.chromium.org/2201393002/diff/20001/components/tracing/BUIL...
File components/tracing/BUILD.gn (right):

https://codereview.chromium.org/2201393002/diff/20001/components/tracing/BUIL...
components/tracing/BUILD.gn:68: "test/example_proto/various_messages.proto",
On 2016/08/04 13:32:04, Primiano Tucci wrote:
> maybe give it a more meaningful name than "various_messages". Maybe
> outermost_message? encapsulated_messages?

test_messages.proto

https://codereview.chromium.org/2201393002/diff/20001/components/tracing/tool...
File components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc (right):

https://codereview.chromium.org/2201393002/diff/20001/components/tracing/tool...
components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:149:
std::vector<const FileDescriptor*> stack;
On 2016/08/04 13:32:04, Primiano Tucci wrote:
> in general you want to name variables according to what they contain, not
their
> implementation.
> Maybe this is a "deps_stack"? dependent_files?

It think it's okay since it's part of standard DFS algorithm implementation.
Stack is not used anywhere outside this scope.

https://codereview.chromium.org/2201393002/diff/20001/components/tracing/tool...
components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:179: if
(public_imports_.find(field->message_type()->file()) ==
On 2016/08/04 13:32:04, Primiano Tucci wrote:
> .count(...) == 0

Acknowledged.

https://codereview.chromium.org/2201393002/diff/20001/components/tracing/tool...
components/tracing/tools/proto_zero_plugin/proto_zero_generator.cc:181: // Avoid
multiple forward declarations.
On 2016/08/04 13:32:04, Primiano Tucci wrote:
> not sure this comment adds anything useful. same below.

Ok, will change the wording. I meant that definitions from public imports have
been already defined on expansion of #include statement.

[kraynov, 2016-08-04 14:41:10]

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

[commit-bot: I haz the power, 2016-08-04 14:41:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kraynov, 2016-08-04 14:42:49]

The CQ bit was unchecked by kraynov@chromium.org

[kraynov, 2016-08-04 15:21:48]

The CQ bit was checked by kraynov@chromium.org

[kraynov, 2016-08-04 15:21:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/2201393002/#ps40001
(title: "style")

[commit-bot: I haz the power, 2016-08-04 15:22:18]

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-04 16:21:36]

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

[commit-bot: I haz the power, 2016-08-04 16:21:37]

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

[kraynov, 2016-08-04 18:48:25]

The CQ bit was checked by kraynov@chromium.org

[commit-bot: I haz the power, 2016-08-04 18:49:31]

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-04 19:46:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-04 19:48:38]

Description was changed from

==========
Tracing V2: Import support for protobuf plugin.

Introduce processing of import references in proto files.

BUG=608721
==========

to

==========
Tracing V2: Import support for protobuf plugin.

Introduce processing of import references in proto files.

BUG=608721

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

[commit-bot: I haz the power, 2016-08-04 19:48:39]

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