Compiler for the JSON Traversal Language.

Compiler for the JSON Traversal Language.

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

[engedy, 2013-09-27 14:04:21]

Please take a look.

[battre, 2013-09-27 15:00:01]

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
File chrome/tools/profile_reset/jtl_compiler.cc (right):

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
chrome/tools/profile_reset/jtl_compiler.cc:157: bool end_of_sentence;
nit: initialize to false

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
File chrome/tools/profile_reset/jtl_compiler.h (right):

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
chrome/tools/profile_reset/jtl_compiler.h:12: // Compiles text-based JTL source
code into JTL bytecode.
Please put a description of the source code syntax here.

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
File chrome/tools/profile_reset/jtl_compiler_unittest.cc (right):

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
chrome/tools/profile_reset/jtl_compiler_unittest.cc:36: {"any/",
OP_NAVIGATE_ANY}, {"back/", OP_NAVIGATE_BACK},
nit: new line after ,

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
File chrome/tools/profile_reset/jtl_parser.cc (right):

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
chrome/tools/profile_reset/jtl_parser.cc:19:
"((?:\"[^\"]*\"|.)*?)(?://.*)?(?:\n|$)";
How about splitting this into something like
    "((?:\"[^\"]*\"|.)*?)"  // Eat and return everything in quotes.
    "(?://.*)?"  // Eat optional comment
    "(?:\n|$)"  // Eat newline

I am not sure I understand what this regex does.

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
File chrome/tools/profile_reset/jtl_parser.h (right):

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
chrome/tools/profile_reset/jtl_parser.h:19: const std::vector<size_t>&
newline_indices);
nit: no explicit needed.

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
chrome/tools/profile_reset/jtl_parser.h:29: // index 'i'. Elements in
|newline_indices| will be monotonically increasing.
I don't understand this comment what goes into newline_indices.

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
chrome/tools/profile_reset/jtl_parser.h:38: bool* ends_sentence);
Can you add a comment to this function? In particular the last parameter is
unclear to me.

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
File chrome/tools/profile_reset/jtl_parser_unittest.cc (right):

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
chrome/tools/profile_reset/jtl_parser_unittest.cc:114: {"\"//not a comment//\"",
"\"//notacomment//\""},
This looks incorrect. You strip whitespaces in literals? That would break the
current JitInterpreter.

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
chrome/tools/profile_reset/jtl_parser_unittest.cc:118: {"\"literal // \"not a
literal\"", "\"literal//\"notaliteral\""}, };
Please add
"\"//literal\" \"//literal\""

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
chrome/tools/profile_reset/jtl_parser_unittest.cc:218: TEST(JtlParser,
FirstOperationIsIllFormed) {
Add a test for parsing program ";;"?

[engedy, 2013-10-01 10:48:09]

Please take another look.

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
File chrome/tools/profile_reset/jtl_compiler.cc (right):

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
chrome/tools/profile_reset/jtl_compiler.cc:157: bool end_of_sentence;
On 2013/09/27 15:00:01, battre wrote:
> nit: initialize to false

Done.

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
File chrome/tools/profile_reset/jtl_compiler.h (right):

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
chrome/tools/profile_reset/jtl_compiler.h:12: // Compiles text-based JTL source
code into JTL bytecode.
On 2013/09/27 15:00:01, battre wrote:
> Please put a description of the source code syntax here.

Done.

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
File chrome/tools/profile_reset/jtl_compiler_unittest.cc (right):

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
chrome/tools/profile_reset/jtl_compiler_unittest.cc:36: {"any/",
OP_NAVIGATE_ANY}, {"back/", OP_NAVIGATE_BACK},
On 2013/09/27 15:00:01, battre wrote:
> nit: new line after ,

Done.

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
File chrome/tools/profile_reset/jtl_parser.cc (right):

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
chrome/tools/profile_reset/jtl_parser.cc:19:
"((?:\"[^\"]*\"|.)*?)(?://.*)?(?:\n|$)";
On 2013/09/27 15:00:01, battre wrote:
> How about splitting this into something like
>     "((?:\"[^\"]*\"|.)*?)"  // Eat and return everything in quotes.
>     "(?://.*)?"  // Eat optional comment
>     "(?:\n|$)"  // Eat newline
> 
> I am not sure I understand what this regex does.

Done.

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
File chrome/tools/profile_reset/jtl_parser.h (right):

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
chrome/tools/profile_reset/jtl_parser.h:19: const std::vector<size_t>&
newline_indices);
On 2013/09/27 15:00:01, battre wrote:
> nit: no explicit needed.

Done.

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
chrome/tools/profile_reset/jtl_parser.h:29: // index 'i'. Elements in
|newline_indices| will be monotonically increasing.
On 2013/09/27 15:00:01, battre wrote:
> I don't understand this comment what goes into newline_indices.

Rephrased, please take another look.

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
chrome/tools/profile_reset/jtl_parser.h:38: bool* ends_sentence);
On 2013/09/27 15:00:01, battre wrote:
> Can you add a comment to this function? In particular the last parameter is
> unclear to me.

Done.

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
File chrome/tools/profile_reset/jtl_parser_unittest.cc (right):

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
chrome/tools/profile_reset/jtl_parser_unittest.cc:118: {"\"literal // \"not a
literal\"", "\"literal//\"notaliteral\""}, };
On 2013/09/27 15:00:01, battre wrote:
> Please add
> "\"//literal\" \"//literal\""

This is already done 3 lines above.

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
chrome/tools/profile_reset/jtl_parser_unittest.cc:218: TEST(JtlParser,
FirstOperationIsIllFormed) {
On 2013/09/27 15:00:01, battre wrote:
> Add a test for parsing program ";;"?

Done.

[engedy, 2013-10-01 10:48:41]

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
File chrome/tools/profile_reset/jtl_parser_unittest.cc (right):

https://codereview.chromium.org/24998003/diff/1/chrome/tools/profile_reset/jt...
chrome/tools/profile_reset/jtl_parser_unittest.cc:114: {"\"//not a comment//\"",
"\"//notacomment//\""},
On 2013/09/27 15:00:01, battre wrote:
> This looks incorrect. You strip whitespaces in literals? That would break the
> current JitInterpreter.

This is done as well.

[battre, 2013-10-01 12:18:26]

LGTM with nits

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
File chrome/tools/profile_reset/jtl_compiler.cc (right):

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
chrome/tools/profile_reset/jtl_compiler.cc:29: CHECK(hash.size() ==
jtl::kHashSizeInBytes);
nit: CHECK_EQ(jtl::kHashSizeInBytes, hash.size());

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
chrome/tools/profile_reset/jtl_compiler.cc:77: bool value;
nit: initialize to false.

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
File chrome/tools/profile_reset/jtl_compiler.h (right):

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
chrome/tools/profile_reset/jtl_compiler.h:30: //   // Store "x"=true path
"foo.bar" is found.
nit: "if path..."

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
File chrome/tools/profile_reset/jtl_compiler_unittest.cc (right):

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
chrome/tools/profile_reset/jtl_compiler_unittest.cc:40: {"any/",
OP_NAVIGATE_ANY}, {"back/", OP_NAVIGATE_BACK},
nit: {"back/" in new line?

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
File chrome/tools/profile_reset/jtl_parser.cc (right):

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
chrome/tools/profile_reset/jtl_parser.cc:32: // The substitution pattern to use
together with the above when replacing.
Please add: As white spaces are not captured, they get removed.

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
chrome/tools/profile_reset/jtl_parser.cc:150: if (!boolean_value.empty())
Please add {} for multiline arguments.

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
File chrome/tools/profile_reset/jtl_parser.h (right):

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
chrome/tools/profile_reset/jtl_parser.h:28: // and will refer to positions in
|compacted_text| such that a new line has
such that |newline_indices[i]| indicates the number of new lines that have been
removed from verbose_text before |compacted_text[i]|.?

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
chrome/tools/profile_reset/jtl_parser.h:59: size_t last_line_number() const;
If this is hacker_style it should be inlined. If this is not possible, it should
be GetLastLineNumber().

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
File chrome/tools/profile_reset/jtl_parser_unittest.cc (right):

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
chrome/tools/profile_reset/jtl_parser_unittest.cc:178: "a/ /b//Comment \n//\n//
Full line comment\n cd //";
Nit: change format?
    "a/ /b//Comment \n"
    "//\n"
    "// Full line comment\n"
    " cd //";

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
chrome/tools/profile_reset/jtl_parser_unittest.cc:210: {"foo5(\"bar\")/",
"foo5", "[\"bar\"]", false},
\"foo bar\" to add whitespaces?

[engedy, 2013-10-01 15:09:32]

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
File chrome/tools/profile_reset/jtl_compiler.cc (right):

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
chrome/tools/profile_reset/jtl_compiler.cc:29: CHECK(hash.size() ==
jtl::kHashSizeInBytes);
On 2013/10/01 12:18:26, battre wrote:
> nit: CHECK_EQ(jtl::kHashSizeInBytes, hash.size());

Done.

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
chrome/tools/profile_reset/jtl_compiler.cc:77: bool value;
On 2013/10/01 12:18:26, battre wrote:
> nit: initialize to false.

Done.

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
File chrome/tools/profile_reset/jtl_compiler.h (right):

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
chrome/tools/profile_reset/jtl_compiler.h:30: //   // Store "x"=true path
"foo.bar" is found.
On 2013/10/01 12:18:26, battre wrote:
> nit: "if path..."

Done.

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
File chrome/tools/profile_reset/jtl_compiler_unittest.cc (right):

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
chrome/tools/profile_reset/jtl_compiler_unittest.cc:40: {"any/",
OP_NAVIGATE_ANY}, {"back/", OP_NAVIGATE_BACK},
On 2013/10/01 12:18:26, battre wrote:
> nit: {"back/" in new line?

Sorry, clang-format again... Done.

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
File chrome/tools/profile_reset/jtl_parser.cc (right):

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
chrome/tools/profile_reset/jtl_parser.cc:32: // The substitution pattern to use
together with the above when replacing.
On 2013/10/01 12:18:26, battre wrote:
> Please add: As white spaces are not captured, they get removed.

Done.

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
chrome/tools/profile_reset/jtl_parser.cc:150: if (!boolean_value.empty())
On 2013/10/01 12:18:26, battre wrote:
> Please add {} for multiline arguments.

Done.

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
File chrome/tools/profile_reset/jtl_parser.h (right):

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
chrome/tools/profile_reset/jtl_parser.h:28: // and will refer to positions in
|compacted_text| such that a new line has
On 2013/10/01 12:18:26, battre wrote:
> such that |newline_indices[i]| indicates the number of new lines that have
been
> removed from verbose_text before |compacted_text[i]|.?

Added example to clarify.

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
chrome/tools/profile_reset/jtl_parser.h:59: size_t last_line_number() const;
On 2013/10/01 12:18:26, battre wrote:
> If this is hacker_style it should be inlined. If this is not possible, it
should
> be GetLastLineNumber().

Changed to CamelCase, as this cannot be inlined due to heavy include
requirements.

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
File chrome/tools/profile_reset/jtl_parser_unittest.cc (right):

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
chrome/tools/profile_reset/jtl_parser_unittest.cc:178: "a/ /b//Comment \n//\n//
Full line comment\n cd //";
On 2013/10/01 12:18:26, battre wrote:
> Nit: change format?
>     "a/ /b//Comment \n"
>     "//\n"
>     "// Full line comment\n"
>     " cd //";

Done.

https://codereview.chromium.org/24998003/diff/8001/chrome/tools/profile_reset...
chrome/tools/profile_reset/jtl_parser_unittest.cc:210: {"foo5(\"bar\")/",
"foo5", "[\"bar\"]", false},
On 2013/10/01 12:18:26, battre wrote:
> \"foo bar\" to add whitespaces?

Done.

[engedy, 2013-10-01 22:03:05]

@Nico: please review for OWNERS' approval.

Furthermore, I am not sure how thing works in chrome/tools/*, only one directory
seems to have owners. Should we add an OWNERS file for ourselves, or what shall
we do for subsequent changes in the future?

[Nico, 2013-10-01 22:24:43]

Folders in tools/ can have OWNERS files.

This is code that runs during a build, not code that's linked into chrome,
right?

Why are you adding these targets to chrome.gyp? Are there problems with giving
this its own gyp file?

[engedy, 2013-10-01 22:36:36]

On 2013/10/01 22:24:43, Nico wrote:
> Folders in tools/ can have OWNERS files.

Great! Added one.

> This is code that runs during a build, not code that's linked into chrome,
> right?

The tool is completely manual, does not even run during a build.

> Why are you adding these targets to chrome.gyp? Are there problems with giving
> this its own gyp file?

Are you suggesting refactoring to a .gypi file, or a completely separate .gyp
file? In any case, I just followed suit with the other tools already here, happy
to do it either way.

[engedy, 2013-10-02 01:03:36]

> Are you suggesting refactoring to a .gypi file, or a completely separate .gyp
> file? In any case, I just followed suit with the other tools already here,
happy
> to do it either way.

As discussed offline, I have added a separate GYP file. Could you please take
another look?

I am somewhat concerned about regressions, though. With this solution, what is
the preferred method of running continuous build/tests on this?

[Nico, 2013-10-02 01:07:42]

On 2013/10/02 01:03:36, engedy wrote:
> > Are you suggesting refactoring to a .gypi file, or a completely separate
.gyp
> > file? In any case, I just followed suit with the other tools already here,
> happy
> > to do it either way.
> 
> As discussed offline, I have added a separate GYP file. Could you please take
> another look?
> 
> I am somewhat concerned about regressions, though. With this solution, what is
> the preferred method of running continuous build/tests on this?

Oh, sorry, I meant to say "Baking the tests into an existing target is fine"
above, looks like I didn't do that. You can restore the
chrome/chrome_tests_unit.gypi change if you want. lgtm with that (or as is, too)

[commit-bot: I haz the power, 2013-10-02 12:20:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/engedy@chromium.org/24998003/119001

[commit-bot: I haz the power, 2013-10-02 12:40:15]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[engedy, 2013-10-02 13:08:25]

@Carlos: we would like to add "+third_party/re2/" to "chrome/tools/DEPS". Could
you please review this for OWNER's approval on the "third_party/" side? Thanks!

[engedy, 2013-10-02 13:08:25]

@Carlos: we would like to add "+third_party/re2/" to "chrome/tools/DEPS". Could
you please review this for OWNER's approval on the "third_party/" side? Thanks!

[cpu_(ooo_6.6-7.5), 2013-10-02 19:13:22]

lgtm

[commit-bot: I haz the power, 2013-10-03 11:34:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/engedy@chromium.org/24998003/169001

[commit-bot: I haz the power, 2013-10-03 15:13:07]

Change committed as 226750