Support automatic javascript test registry in gtest when creating WebUI tests.

Support automatic javascript test registry in gtest when creating WebUI tests.

The goal was to support something as simple as the following, where the tests in the
javascript files would be 1st class GTESTs, supporting FLAKY_, DISABLED_, etc, and
registered at linker_initialization time.

WEB_UI_BROWSER_TEST_JS(WebUIBrowserTest, TestJSPass,
                       FILE_PATH_LITERAL("sample_passing.js")) {
  ui_test_utils::NavigateToURL(browser(), GURL(chrome::kChromeUIDownloadsURL));
}

This solution ended up being fairly fragile, and I ended up with a script to parse javascript (with the help of v8_shell) and generate an includable ...-inl.h file with the following for every test in the .js file specified in the |rules| section of tools/js2webui.py

IN_PROC_BROWSER_TEST_F(WebUIBrowserTestPass, testHelper) {
  AddLibrary(FilePath(FILE_PATH_LITERAL("sample_pass.js")));
  ASSERT_TRUE(RunJavascriptTest("testHelper"));
}

http://www.chromium.org/Home/domui-testing

BUG=82437
R=estade@chromium.org,jhawkins@chromium.org
TEST=browser_tests --gtest_filter=WebUIBrowserTest*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=89453

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=89605

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=89626

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91358

[Sheridan Rawlins, 2011-05-28 01:40:29]

Please review this framework patch, which should make writing domui tests much
easier.

Sample run showing format:

Note: Google Test filter = WebUIBrowserTest.testInitialFocus/TestJSPass
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from WebUIBrowserTest, where TypeParam = 
[ RUN      ] WebUIBrowserTest.testInitialFocus/TestJSPass
[6470:6470:0527/183440:18631207566:INFO:CONSOLE(1)] "Uncaught ReferenceError:
downloadsList is not defined", source:  (1)
[6470:6470:0527/183440:18631477685:INFO:CONSOLE(564)] "results", source:
chrome://downloads/downloads.js (564)
[6470:6470:0527/183440:18631486839:INFO:CONSOLE(150)] "Running test
testInitialFocus", source:  (150)
[       OK ] WebUIBrowserTest.testInitialFocus/TestJSPass (672 ms)
[----------] 1 test from WebUIBrowserTest (672 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (673 ms total)
[  PASSED  ] 1 test.
  YOU HAVE 25 FLAKY TESTS

[Paweł Hajdan Jr., 2011-05-28 08:03:55]

Drive-by with a testing comment (chrome/test/OWNERS). Please let me do another
review before this gets committed.

http://codereview.chromium.org/7087014/diff/1/chrome/browser/ui/webui/web_ui_...
File chrome/browser/ui/webui/web_ui_browsertest.h (right):

http://codereview.chromium.org/7087014/diff/1/chrome/browser/ui/webui/web_ui_...
chrome/browser/ui/webui/web_ui_browsertest.h:26: #define
WEB_UI_BROWSER_TEST_JS_(test_case_name, test_name, parent_class, \
Wait, are you trying to do parametrized browser tests or something like that? I
think we now have BROWSER_TEST_P or something like that. I'd like to avoid
increasing number of complicated macros like this, so you should have a _very_
good justification if this is needed.

[Sheridan Rawlins, 2011-05-28 14:26:30]

http://codereview.chromium.org/7087014/diff/1/chrome/browser/ui/webui/web_ui_...
File chrome/browser/ui/webui/web_ui_browsertest.h (right):

http://codereview.chromium.org/7087014/diff/1/chrome/browser/ui/webui/web_ui_...
chrome/browser/ui/webui/web_ui_browsertest.h:26: #define
WEB_UI_BROWSER_TEST_JS_(test_case_name, test_name, parent_class, \
Thanks for the drive by, Pawel.  So here's the justification for this macro -
When I showed the output of the parameterized tests to Evan, he thought it
wasn't great that the gtest system added /0, /1, /2 instead of using the
javascript test names. Upon further investigation, "not great" turned into "not
able to make a javascript FLAKY_ or DISABLED_ because its name wasn't
used/registered to each test; only a number.  This macro uses the parameterizing
system to allow our static method AddJSToRegistry to add all javascript tests as
parameters, and use the parameter in the name when registering (see sample
output in my first message.

If you don't want multiple macros, I will consider removing the one I added the
other day (IN_PROC_BROWSER_TEST_P), though others _may_ find it useful for some
other reason (like creating a cartesian product with gtest's Combine value
generator).

On 2011/05/28 08:03:55, Paweł Hajdan Jr. wrote:
> Wait, are you trying to do parametrized browser tests or something like that?
I
> think we now have BROWSER_TEST_P or something like that. I'd like to avoid
> increasing number of complicated macros like this, so you should have a _very_
> good justification if this is needed.

[Paweł Hajdan Jr., 2011-05-30 17:17:58]

http://codereview.chromium.org/7087014/diff/1/chrome/browser/ui/webui/web_ui_...
File chrome/browser/ui/webui/web_ui_browsertest.h (right):

http://codereview.chromium.org/7087014/diff/1/chrome/browser/ui/webui/web_ui_...
chrome/browser/ui/webui/web_ui_browsertest.h:26: #define
WEB_UI_BROWSER_TEST_JS_(test_case_name, test_name, parent_class, \
On 2011/05/28 14:26:30, Sheridan Rawlins wrote:
> Thanks for the drive by, Pawel.  So here's the justification for this macro -
> When I showed the output of the parameterized tests to Evan, he thought it
> wasn't great that the gtest system added /0, /1, /2 instead of using the
> javascript test names. Upon further investigation, "not great" turned into
"not
> able to make a javascript FLAKY_ or DISABLED_ because its name wasn't
> used/registered to each test; only a number.  This macro uses the
parameterizing
> system to allow our static method AddJSToRegistry to add all javascript tests
as
> parameters, and use the parameter in the name when registering (see sample
> output in my first message.
> 
> If you don't want multiple macros, I will consider removing the one I added
the
> other day (IN_PROC_BROWSER_TEST_P), though others _may_ find it useful for
some
> other reason (like creating a cartesian product with gtest's Combine value
> generator).

Well, it wasn't really about multiple macros, but while I can understand the
need for IN_PROC_BROWSER_TEST_P (which also builds on a known concept from
gtest), this very specific macro is going to be a debugging nightmare.

Could you find a different solution?

http://codereview.chromium.org/7087014/diff/5001/chrome/browser/ui/webui/web_...
File chrome/browser/ui/webui/web_ui_browsertest.cc (right):

http://codereview.chromium.org/7087014/diff/5001/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_browsertest.cc:66: if (library.IsAbsolute()) {
I think this may be error prone. Could you add two variants - one for relative,
and one for absolute paths if really needed?

http://codereview.chromium.org/7087014/diff/5001/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_browsertest.cc:104: NOTREACHED() << "Exception: "
<<  *exception_str;
Why should a JS exception crash the entire test (that has then to become
disabled)?

[David Tseng, 2011-05-30 20:44:49]

Awesome!  Thanks so much for doing this.  A few comments.

http://codereview.chromium.org/7087014/diff/5001/chrome/browser/ui/webui/web_...
File chrome/browser/ui/webui/web_ui_browsertest.cc (right):

http://codereview.chromium.org/7087014/diff/5001/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_browsertest.cc:83: "(function () {"
Is there some way we can ask V8 to give us back the functions within the file
without in lining JS to get the functions?

Also, are we checking to only append functions with "test" prefix?

http://codereview.chromium.org/7087014/diff/5001/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_browsertest.cc:206: std::string test_name = *it +
"/" + name;
Should
    std::string test_name = *it + "/" + name; 
just be
    std::string test_name = *it; 
(see other comment).

or
    std::string test_name = name + "/" + *it; 

since *it is the more specific identifier?

http://codereview.chromium.org/7087014/diff/5001/chrome/browser/ui/webui/web_...
File chrome/browser/ui/webui/web_ui_browsertest.h (right):

http://codereview.chromium.org/7087014/diff/5001/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_browsertest.h:55: #test_case_name, #test_name,
"", "",\
Do we need the |test_name| parameter?  It is already parsed from the js file and
the |test_case_name| serves to be the general category of test.

[Sheridan Rawlins, 2011-05-31 23:15:00]

http://codereview.chromium.org/7087014/diff/1/chrome/browser/ui/webui/web_ui_...
File chrome/browser/ui/webui/web_ui_browsertest.h (right):

http://codereview.chromium.org/7087014/diff/1/chrome/browser/ui/webui/web_ui_...
chrome/browser/ui/webui/web_ui_browsertest.h:26: #define
WEB_UI_BROWSER_TEST_JS_(test_case_name, test_name, parent_class, \
Short answer: This macro seemed to be the best solution, providing the best
engineering experience and usability.

Long answer: I thought about this quite a bit.  The "other solutions" didn't
stack up.  Here's what I considered for alternatives:

1- Use the IN_PROC_BROWSER_TEST_P: Falls short because the goal was to have the
javascript test name in the GTEST test name for proper DISABLED/FLAKY handling,
and this adds the number of the test; not its name (/1, /2, /3, etc)

2- Modify GTEST to support this. Since GTEST already "supports this" I didn't
see this as necessary, and thought it would take longer to get in.

3- Turn the v8 code into a separate executable and write gyp rules to generate
C++ using IN_PROC_BROWSER_TEST_F, and use its test_info_ to get its name(). 
This seemed fairly cumbersome and removed from the authors by comparison.  It
also would not allow the ease of extension by writing the prep code.  The sample
(below) is fairly simple, but I've got some PrintPreview tests which set up a
mocked PrintPreviewHandler and put the ACTIONS in the block.

WEB_UI_BROWSER_TEST_JS(WebUIBrowserTest, TestJSPass,
                       FILE_PATH_LITERAL("sample_passing.js")) {
  ui_test_utils::NavigateToURL(browser(), GURL(chrome::kChromeUIDownloadsURL));
}

On 2011/05/30 17:17:58, Paweł Hajdan Jr. wrote:
> On 2011/05/28 14:26:30, Sheridan Rawlins wrote:
> > Thanks for the drive by, Pawel.  So here's the justification for this macro
-
> > When I showed the output of the parameterized tests to Evan, he thought it
> > wasn't great that the gtest system added /0, /1, /2 instead of using the
> > javascript test names. Upon further investigation, "not great" turned into
> "not
> > able to make a javascript FLAKY_ or DISABLED_ because its name wasn't
> > used/registered to each test; only a number.  This macro uses the
> parameterizing
> > system to allow our static method AddJSToRegistry to add all javascript
tests
> as
> > parameters, and use the parameter in the name when registering (see sample
> > output in my first message.
> > 
> > If you don't want multiple macros, I will consider removing the one I added
> the
> > other day (IN_PROC_BROWSER_TEST_P), though others _may_ find it useful for
> some
> > other reason (like creating a cartesian product with gtest's Combine value
> > generator).
> 
> Well, it wasn't really about multiple macros, but while I can understand the
> need for IN_PROC_BROWSER_TEST_P (which also builds on a known concept from
> gtest), this very specific macro is going to be a debugging nightmare.
> 
> Could you find a different solution?

http://codereview.chromium.org/7087014/diff/5001/chrome/browser/ui/webui/web_...
File chrome/browser/ui/webui/web_ui_browsertest.cc (right):

http://codereview.chromium.org/7087014/diff/5001/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_browsertest.cc:66: if (library.IsAbsolute()) {
David says we don't need the absolute case.  I'll put an ASSERT and remove the
handling.

(Done.)

On 2011/05/30 17:17:58, Paweł Hajdan Jr. wrote:
> I think this may be error prone. Could you add two variants - one for
relative,
> and one for absolute paths if really needed?

http://codereview.chromium.org/7087014/diff/5001/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_browsertest.cc:83: "(function () {"
The only way to get a variable from javascript is to compile/run a script. 
However, once I get a variable, I can iterate across the property names quite
easily/efficiently in v8.  I'll simplify the inlined javascript code.

We are including all functions.  See the sample_passing.js for how to add helper
functions.

On 2011/05/30 20:44:49, David Tseng wrote:
> Is there some way we can ask V8 to give us back the functions within the file
> without in lining JS to get the functions?
> 
> Also, are we checking to only append functions with "test" prefix?

http://codereview.chromium.org/7087014/diff/5001/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_browsertest.cc:104: NOTREACHED() << "Exception: "
<<  *exception_str;
Keep in mind this is a JS exception in examining the js file for functions.  If
you can't get the list of functions, you can't register the tests, so it has to
fail.

On 2011/05/30 17:17:58, Paweł Hajdan Jr. wrote:
> Why should a JS exception crash the entire test (that has then to become
> disabled)?

http://codereview.chromium.org/7087014/diff/5001/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_browsertest.cc:206: std::string test_name = *it +
"/" + name;
I prefer to have the C++ Testname available (as mentioned in my comment on
web_ui_browsertest.h, for the case where you want to run the same .js file
against different locations, or, more probably, with different mock side-effects
(ACTIONs).

The javascript test name (*it) must come first in order for DISABLED_ & FLAKY_
to work properly.

On 2011/05/30 20:44:49, David Tseng wrote:
> Should
>     std::string test_name = *it + "/" + name; 
> just be
>     std::string test_name = *it; 
> (see other comment).
> 
> or
>     std::string test_name = name + "/" + *it; 
> 
> since *it is the more specific identifier?
>

http://codereview.chromium.org/7087014/diff/5001/chrome/browser/ui/webui/web_...
File chrome/browser/ui/webui/web_ui_browsertest.h (right):

http://codereview.chromium.org/7087014/diff/5001/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_browsertest.h:55: #test_case_name, #test_name,
"", "",\
I include the test_name after the slash so that it is visible, and useful in
--gtest_filter.

Consider the case where you have a js file with several tests and want to run
them on several pages, you could do

WEB_UI_BROWSER_TEST_JS(MyCase, MyTest1, FILE_PATH_LITERAL("myjs.js")) {
  ui_test_utils::NavigateToURL(browser(), GURL("mypage1"));
}

WEB_UI_BROWSER_TEST_JS(MyCase, MyTest2, FILE_PATH_LITERAL("myjs.js")) {
  ui_test_utils::NavigateToURL(browser(), GURL("mypage2"));
}

You could run .../browser_tests --gtest_filter=MyCase.*/MyTest2

On 2011/05/30 20:44:49, David Tseng wrote:
> Do we need the |test_name| parameter?  It is already parsed from the js file
and
> the |test_case_name| serves to be the general category of test.

[David Tseng, 2011-06-01 00:11:23]

LGTM

wrt my comments.

On 5/31/11, scr@chromium.org <scr@chromium.org> wrote:
>
>
http://codereview.chromium.org/7087014/diff/1/chrome/browser/ui/webui/web_ui_...
> File chrome/browser/ui/webui/web_ui_browsertest.h (right):
>
>
http://codereview.chromium.org/7087014/diff/1/chrome/browser/ui/webui/web_ui_...
> chrome/browser/ui/webui/web_ui_browsertest.h:26: #define
> WEB_UI_BROWSER_TEST_JS_(test_case_name, test_name, parent_class, \
> Short answer: This macro seemed to be the best solution, providing the
> best engineering experience and usability.
>
> Long answer: I thought about this quite a bit.  The "other solutions"
> didn't stack up.  Here's what I considered for alternatives:
>
> 1- Use the IN_PROC_BROWSER_TEST_P: Falls short because the goal was to
> have the javascript test name in the GTEST test name for proper
> DISABLED/FLAKY handling, and this adds the number of the test; not its
> name (/1, /2, /3, etc)
>
> 2- Modify GTEST to support this. Since GTEST already "supports this" I
> didn't see this as necessary, and thought it would take longer to get
> in.
>
> 3- Turn the v8 code into a separate executable and write gyp rules to
> generate C++ using IN_PROC_BROWSER_TEST_F, and use its test_info_ to get
> its name().  This seemed fairly cumbersome and removed from the authors
> by comparison.  It also would not allow the ease of extension by writing
> the prep code.  The sample (below) is fairly simple, but I've got some
> PrintPreview tests which set up a mocked PrintPreviewHandler and put the
> ACTIONS in the block.
>
> WEB_UI_BROWSER_TEST_JS(WebUIBrowserTest, TestJSPass,
>                         FILE_PATH_LITERAL("sample_passing.js")) {
>    ui_test_utils::NavigateToURL(browser(),
> GURL(chrome::kChromeUIDownloadsURL));
> }
>
> On 2011/05/30 17:17:58, Paweł Hajdan Jr. wrote:
>> On 2011/05/28 14:26:30, Sheridan Rawlins wrote:
>> > Thanks for the drive by, Pawel.  So here's the justification for
> this macro -
>> > When I showed the output of the parameterized tests to Evan, he
> thought it
>> > wasn't great that the gtest system added /0, /1, /2 instead of using
> the
>> > javascript test names. Upon further investigation, "not great"
> turned into
>> "not
>> > able to make a javascript FLAKY_ or DISABLED_ because its name
> wasn't
>> > used/registered to each test; only a number.  This macro uses the
>> parameterizing
>> > system to allow our static method AddJSToRegistry to add all
> javascript tests
>> as
>> > parameters, and use the parameter in the name when registering (see
> sample
>> > output in my first message.
>> >
>> > If you don't want multiple macros, I will consider removing the one
> I added
>> the
>> > other day (IN_PROC_BROWSER_TEST_P), though others _may_ find it
> useful for
>> some
>> > other reason (like creating a cartesian product with gtest's Combine
> value
>> > generator).
>
>> Well, it wasn't really about multiple macros, but while I can
> understand the
>> need for IN_PROC_BROWSER_TEST_P (which also builds on a known concept
> from
>> gtest), this very specific macro is going to be a debugging nightmare.
>
>> Could you find a different solution?
>
>
http://codereview.chromium.org/7087014/diff/5001/chrome/browser/ui/webui/web_...
> File chrome/browser/ui/webui/web_ui_browsertest.cc (right):
>
>
http://codereview.chromium.org/7087014/diff/5001/chrome/browser/ui/webui/web_...
> chrome/browser/ui/webui/web_ui_browsertest.cc:66: if
> (library.IsAbsolute()) {
> David says we don't need the absolute case.  I'll put an ASSERT and
> remove the handling.
>
> (Done.)
>
> On 2011/05/30 17:17:58, Paweł Hajdan Jr. wrote:
>> I think this may be error prone. Could you add two variants - one for
> relative,
>> and one for absolute paths if really needed?
>
>
http://codereview.chromium.org/7087014/diff/5001/chrome/browser/ui/webui/web_...
> chrome/browser/ui/webui/web_ui_browsertest.cc:83: "(function () {"
> The only way to get a variable from javascript is to compile/run a
> script.  However, once I get a variable, I can iterate across the
> property names quite easily/efficiently in v8.  I'll simplify the
> inlined javascript code.
>
> We are including all functions.  See the sample_passing.js for how to
> add helper functions.
>
> On 2011/05/30 20:44:49, David Tseng wrote:
>> Is there some way we can ask V8 to give us back the functions within
> the file
>> without in lining JS to get the functions?
>
>> Also, are we checking to only append functions with "test" prefix?
>
>
http://codereview.chromium.org/7087014/diff/5001/chrome/browser/ui/webui/web_...
> chrome/browser/ui/webui/web_ui_browsertest.cc:104: NOTREACHED() <<
> "Exception: " <<  *exception_str;
> Keep in mind this is a JS exception in examining the js file for
> functions.  If you can't get the list of functions, you can't register
> the tests, so it has to fail.
>
> On 2011/05/30 17:17:58, Paweł Hajdan Jr. wrote:
>> Why should a JS exception crash the entire test (that has then to
> become
>> disabled)?
>
>
http://codereview.chromium.org/7087014/diff/5001/chrome/browser/ui/webui/web_...
> chrome/browser/ui/webui/web_ui_browsertest.cc:206: std::string test_name
> = *it + "/" + name;
> I prefer to have the C++ Testname available (as mentioned in my comment
> on web_ui_browsertest.h, for the case where you want to run the same .js
> file against different locations, or, more probably, with different mock
> side-effects (ACTIONs).
>
> The javascript test name (*it) must come first in order for DISABLED_ &
> FLAKY_ to work properly.
>
> On 2011/05/30 20:44:49, David Tseng wrote:
>> Should
>>      std::string test_name = *it + "/" + name;
>> just be
>>      std::string test_name = *it;
>> (see other comment).
>
>> or
>>      std::string test_name = name + "/" + *it;
>
>> since *it is the more specific identifier?
>
>
>
http://codereview.chromium.org/7087014/diff/5001/chrome/browser/ui/webui/web_...
> File chrome/browser/ui/webui/web_ui_browsertest.h (right):
>
>
http://codereview.chromium.org/7087014/diff/5001/chrome/browser/ui/webui/web_...
> chrome/browser/ui/webui/web_ui_browsertest.h:55: #test_case_name,
> #test_name, "", "",\
> I include the test_name after the slash so that it is visible, and
> useful in --gtest_filter.
>
> Consider the case where you have a js file with several tests and want
> to run them on several pages, you could do
>
> WEB_UI_BROWSER_TEST_JS(MyCase, MyTest1, FILE_PATH_LITERAL("myjs.js")) {
>    ui_test_utils::NavigateToURL(browser(), GURL("mypage1"));
> }
>
> WEB_UI_BROWSER_TEST_JS(MyCase, MyTest2, FILE_PATH_LITERAL("myjs.js")) {
>    ui_test_utils::NavigateToURL(browser(), GURL("mypage2"));
> }
>
> You could run .../browser_tests --gtest_filter=MyCase.*/MyTest2
>
> On 2011/05/30 20:44:49, David Tseng wrote:
>> Do we need the |test_name| parameter?  It is already parsed from the
> js file and
>> the |test_case_name| serves to be the general category of test.
>
> http://codereview.chromium.org/7087014/
>

[Sheridan Rawlins, 2011-06-08 14:24:45]

Got the green trybots now for Mac & Windows.

Changes from original design:
- Because Mac & Windows don't have a thread at linker-initialization time, V8
can't run until main(); instead of running v8 at linker-initialization time,
register callbacks, which are called by out_of_proc_test_runner's main(), and
before the test initialization.
- Don't use PathService for determining the test data dir: on mac, it is far too
troublesome to find the right combination of initialization or path hackery to
get it working without breaking assumptions made by other initialization code,
test framework (chrome_test_suite, in_process_browser_test) or tests themselves.

Pawel, the macro is the best way to cause registry at linker-initialization time
with most benefit to authors.  I haven't spent any time in this process
debugging the macros, only the win/macintosh initialization sequence and
assumptions, which are now resolved.

Thanks,

-SCR

http://codereview.chromium.org/7087014/diff/5001/chrome/browser/ui/webui/web_...
File chrome/browser/ui/webui/web_ui_browsertest.cc (right):

http://codereview.chromium.org/7087014/diff/5001/chrome/browser/ui/webui/web_...
chrome/browser/ui/webui/web_ui_browsertest.cc:66: if (library.IsAbsolute()) {
The BIDI checker uses third party test script.  Putting back.

http://build.chromium.org/p/tryserver.chromium/builders/win/builds/34811/step...

On 2011/05/31 23:15:00, Sheridan Rawlins wrote:
> David says we don't need the absolute case.  I'll put an ASSERT and remove the
> handling.
> 
> (Done.)
> 
> On 2011/05/30 17:17:58, Paweł Hajdan Jr. wrote:
> > I think this may be error prone. Could you add two variants - one for
> relative,
> > and one for absolute paths if really needed?
>

[Paweł Hajdan Jr., 2011-06-08 14:48:43]

http://codereview.chromium.org/7087014/diff/31001/chrome/browser/ui/webui/web...
File chrome/browser/ui/webui/web_ui_browsertest.h (right):

http://codereview.chromium.org/7087014/diff/31001/chrome/browser/ui/webui/web...
chrome/browser/ui/webui/web_ui_browsertest.h:26: #define
WEB_UI_BROWSER_TEST_JS_(test_case_name, test_name, parent_class, \
Sorry, this is still too complicated and is just going to be a debugging
nightmare if someone needs to dig through this. Have you considered using a
lightweight code generator to write all TEST_F instantiations? I guess that
should really simplify the code, and avoid us reaching into gtest internals
(InProcessBrowserTest does a similar, only slightly simpler thing, but I also
consider that non-optimal; let's not proliferate that practice).

[Sheridan Rawlins, 2011-06-10 06:16:43]

Ok, how's this?

http://codereview.chromium.org/7087014/diff/31001/chrome/browser/ui/webui/web...
File chrome/browser/ui/webui/web_ui_browsertest.h (right):

http://codereview.chromium.org/7087014/diff/31001/chrome/browser/ui/webui/web...
chrome/browser/ui/webui/web_ui_browsertest.h:26: #define
WEB_UI_BROWSER_TEST_JS_(test_case_name, test_name, parent_class, \
How do you like this radical change?  it uses the v8_shell and a few python
helper scripts, as well as some gyp actions on browser_tests to generate the
macros from the javascript file.

To maintain, edit the tools/gypv8sh.py "rules" list and add more rules.

On 2011/06/08 14:48:44, Paweł Hajdan Jr. wrote:
> Sorry, this is still too complicated and is just going to be a debugging
> nightmare if someone needs to dig through this. Have you considered using a
> lightweight code generator to write all TEST_F instantiations? I guess that
> should really simplify the code, and avoid us reaching into gtest internals
> (InProcessBrowserTest does a similar, only slightly simpler thing, but I also
> consider that non-optimal; let's not proliferate that practice).

[Paweł Hajdan Jr., 2011-06-10 07:38:29]

Mostly minor comments, the approach looks fine.

http://codereview.chromium.org/7087014/diff/33001/chrome/browser/ui/webui/jav...
File chrome/browser/ui/webui/javascript2webui.js (right):

http://codereview.chromium.org/7087014/diff/33001/chrome/browser/ui/webui/jav...
chrome/browser/ui/webui/javascript2webui.js:1: #!tools/v8sh.py
I'm not sure that this shebang with a relative path works.

http://codereview.chromium.org/7087014/diff/33001/chrome/browser/ui/webui/jav...
chrome/browser/ui/webui/javascript2webui.js:17: print('// GENERATED FILE
(from');
Please add an info what tool generated this file.

http://codereview.chromium.org/7087014/diff/33001/chrome/browser/ui/webui/jav...
chrome/browser/ui/webui/javascript2webui.js:22: print('WEB_UI_UNITTEST_F(' +
test_fixture + ', ' + func + ');');
Could you remove the WEB_UI_UNITTEST_F macro and just make the generator print
its expanded form?

http://codereview.chromium.org/7087014/diff/33001/tools/gypv8sh.py
File tools/gypv8sh.py (right):

http://codereview.chromium.org/7087014/diff/33001/tools/gypv8sh.py#newcode1
tools/gypv8sh.py:1: #!/usr/bin/python
Please make sure someone working more frequently with Python files reviews this
(some suggestions: Pam Greene, Nicolas Sylvain, Marc-Antoine Ruel).

http://codereview.chromium.org/7087014/diff/33001/tools/v8sh.py
File tools/v8sh.py (right):

http://codereview.chromium.org/7087014/diff/33001/tools/v8sh.py#newcode29
tools/v8sh.py:29:
os.path.dirname(os.path.dirname(sys.argv[0])),'out/Debug/v8_shell')
What with Release mode and non-Linux platforms?

[Sheridan Rawlins, 2011-06-11 18:07:19]

Nicholas, would you be able to review the python script gypv8sh.py?  It needs to
populate the arguments for a javascript script and invoke it with v8_shell.  The
<(PRODUCT_DIR) will be passed by gyp in -p, and input/output will be returned to
gyp via -i and -o.

http://codereview.chromium.org/7087014/diff/33001/chrome/browser/ui/webui/jav...
File chrome/browser/ui/webui/javascript2webui.js (right):

http://codereview.chromium.org/7087014/diff/33001/chrome/browser/ui/webui/jav...
chrome/browser/ui/webui/javascript2webui.js:1: #!tools/v8sh.py
Removed in favor of passing paths in (from gyp/python script).

On 2011/06/10 07:38:29, Paweł Hajdan Jr. wrote:
> I'm not sure that this shebang with a relative path works.

http://codereview.chromium.org/7087014/diff/33001/chrome/browser/ui/webui/jav...
chrome/browser/ui/webui/javascript2webui.js:17: print('// GENERATED FILE
(from');
On 2011/06/10 07:38:29, Paweł Hajdan Jr. wrote:
> Please add an info what tool generated this file.

Done.

http://codereview.chromium.org/7087014/diff/33001/chrome/browser/ui/webui/jav...
chrome/browser/ui/webui/javascript2webui.js:22: print('WEB_UI_UNITTEST_F(' +
test_fixture + ', ' + func + ');');
On 2011/06/10 07:38:29, Paweł Hajdan Jr. wrote:
> Could you remove the WEB_UI_UNITTEST_F macro and just make the generator print
> its expanded form?

Done.

http://codereview.chromium.org/7087014/diff/33001/tools/v8sh.py
File tools/v8sh.py (right):

http://codereview.chromium.org/7087014/diff/33001/tools/v8sh.py#newcode29
tools/v8sh.py:29:
os.path.dirname(os.path.dirname(sys.argv[0])),'out/Debug/v8_shell')
On 2011/06/10 07:38:29, Paweł Hajdan Jr. wrote:
> What with Release mode and non-Linux platforms?

Done.

(Pass the <(PRODUCT_DIR) to the script).

[nsylvain, 2011-06-12 21:13:18]

Looked only at the .py file.

If possible, you should use optparse for the arguments. See
http://docs.python.org/library/optparse.html

That would make half the code a lot less error-prone.

http://codereview.chromium.org/7087014/diff/28010/tools/gypv8sh.py
File tools/gypv8sh.py (right):

http://codereview.chromium.org/7087014/diff/28010/tools/gypv8sh.py#newcode18
tools/gypv8sh.py:18: import getopt
sort the imports

http://codereview.chromium.org/7087014/diff/28010/tools/gypv8sh.py#newcode69
tools/gypv8sh.py:69: subprocess.call(cmd, stdout=open(rule[2],'w+'))
I guess you should sys.exit(x) the return value of this command.

[Sheridan Rawlins, 2011-06-13 15:40:44]

Also updated to use optparse.

http://codereview.chromium.org/7087014/diff/28010/tools/gypv8sh.py
File tools/gypv8sh.py (right):

http://codereview.chromium.org/7087014/diff/28010/tools/gypv8sh.py#newcode18
tools/gypv8sh.py:18: import getopt
On 2011/06/12 21:13:19, nsylvain wrote:
> sort the imports

Done.

http://codereview.chromium.org/7087014/diff/28010/tools/gypv8sh.py#newcode69
tools/gypv8sh.py:69: subprocess.call(cmd, stdout=open(rule[2],'w+'))
On 2011/06/12 21:13:19, nsylvain wrote:
> I guess you should sys.exit(x) the return value of this command.

Done.

[Paweł Hajdan Jr., 2011-06-13 19:38:40]

http://codereview.chromium.org/7087014/diff/33004/chrome/browser/ui/webui/web...
File chrome/browser/ui/webui/web_ui_browsertest.cc (right):

http://codereview.chromium.org/7087014/diff/33004/chrome/browser/ui/webui/web...
chrome/browser/ui/webui/web_ui_browsertest.cc:209: virtual void
SetUpOnMainThread() {
Shouldn't you call parent's SetUpOnMainThread or rather use TEST_F? What is this
test testing?

http://codereview.chromium.org/7087014/diff/33004/tools/gypv8sh.py
File tools/gypv8sh.py (right):

http://codereview.chromium.org/7087014/diff/33004/tools/gypv8sh.py#newcode60
tools/gypv8sh.py:60: if not opts.product_dir:
I'd prefer to remove this guessing logic and just make the param mandatory.

[Sheridan Rawlins, 2011-06-13 22:18:53]

http://codereview.chromium.org/7087014/diff/33004/chrome/browser/ui/webui/web...
File chrome/browser/ui/webui/web_ui_browsertest.cc (right):

http://codereview.chromium.org/7087014/diff/33004/chrome/browser/ui/webui/web...
chrome/browser/ui/webui/web_ui_browsertest.cc:209: virtual void
SetUpOnMainThread() {
This is not a test.  It is the "framework" class for the tests created in the
inlined file below.  Because the web test framework written by David needs a
domui page, navigating to that page must be done by the C++.  For Print Preview,
there is far more setup to register mock objects, navigate to a test page, cause
it to print, and wait for the newly created print tab to open.

FWIW, the generated content at the time of writing is:
// GENERATED FILE
// browser/ui/webui/javascript2webui.js WebUIBrowserTestPass
test/data/webui/sample_pass.js sample_pass.js
// PLEASE DO NOT HAND EDIT!

IN_PROC_BROWSER_TEST_F(WebUIBrowserTestPass, testAssertFalse) {
  AddLibrary(FilePath(FILE_PATH_LITERAL("sample_pass.js")));
  ASSERT_TRUE(RunJavascriptTest("testAssertFalse"));
}

IN_PROC_BROWSER_TEST_F(WebUIBrowserTestPass, testAssertTrue) {
  AddLibrary(FilePath(FILE_PATH_LITERAL("sample_pass.js")));
  ASSERT_TRUE(RunJavascriptTest("testAssertTrue"));
}

IN_PROC_BROWSER_TEST_F(WebUIBrowserTestPass, testAssertEquals) {
  AddLibrary(FilePath(FILE_PATH_LITERAL("sample_pass.js")));
  ASSERT_TRUE(RunJavascriptTest("testAssertEquals"));
}

On 2011/06/13 19:38:40, Paweł Hajdan Jr. wrote:
> Shouldn't you call parent's SetUpOnMainThread or rather use TEST_F? What is
this
> test testing?

http://codereview.chromium.org/7087014/diff/33004/tools/gypv8sh.py
File tools/gypv8sh.py (right):

http://codereview.chromium.org/7087014/diff/33004/tools/gypv8sh.py#newcode60
tools/gypv8sh.py:60: if not opts.product_dir:
On 2011/06/13 19:38:40, Paweł Hajdan Jr. wrote:
> I'd prefer to remove this guessing logic and just make the param mandatory.

Done.

[nsylvain, 2011-06-14 16:55:26]

python lgtm

[Paweł Hajdan Jr., 2011-06-14 20:23:17]

http://codereview.chromium.org/7087014/diff/33004/chrome/browser/ui/webui/web...
File chrome/browser/ui/webui/web_ui_browsertest.cc (right):

http://codereview.chromium.org/7087014/diff/33004/chrome/browser/ui/webui/web...
chrome/browser/ui/webui/web_ui_browsertest.cc:209: virtual void
SetUpOnMainThread() {
On 2011/06/13 22:18:54, Sheridan Rawlins wrote:
> This is not a test.  It is the "framework" class for the tests created in the
> inlined file below.  Because the web test framework written by David needs a
> domui page, navigating to that page must be done by the C++.  For Print
Preview,
> there is far more setup to register mock objects, navigate to a test page,
cause
> it to print, and wait for the newly created print tab to open.

I see. That still doesn't explain not calling parent's virtual method, and also
please just put in comment (a bit shorter) the above explanation.

http://codereview.chromium.org/7087014/diff/34001/tools/gypv8sh.py
File tools/gypv8sh.py (right):

http://codereview.chromium.org/7087014/diff/34001/tools/gypv8sh.py#newcode63
tools/gypv8sh.py:63: exit(-1);
I think there is some call in optparse to exit, could you use it?

[Sheridan Rawlins, 2011-06-14 22:39:26]

I added a comment for you, which should explain that WebUIBrowserTest doesn't
provide SetUpOnMainThread and that it doesn't need to be called by superclass.

http://codereview.chromium.org/7087014/diff/33004/chrome/browser/ui/webui/web...
File chrome/browser/ui/webui/web_ui_browsertest.cc (right):

http://codereview.chromium.org/7087014/diff/33004/chrome/browser/ui/webui/web...
chrome/browser/ui/webui/web_ui_browsertest.cc:209: virtual void
SetUpOnMainThread() {
  // Override this to add any custom setup code that needs to be done on the
  // main thread after the browser is created and just before calling
  // RunTestOnMainThread().
  virtual void SetUpOnMainThread() {}

WebUIBrowserTest does not override this.  Sometimes virtual methods need to call
the parent's method, sometimes they don't and you must rely on comments & code
to guide you. This particular function doesn't appear to need to call the
parent's impl.

On 2011/06/14 20:23:17, Paweł Hajdan Jr. wrote:
> On 2011/06/13 22:18:54, Sheridan Rawlins wrote:
> > This is not a test.  It is the "framework" class for the tests created in
the
> > inlined file below.  Because the web test framework written by David needs a
> > domui page, navigating to that page must be done by the C++.  For Print
> Preview,
> > there is far more setup to register mock objects, navigate to a test page,
> cause
> > it to print, and wait for the newly created print tab to open.
> 
> I see. That still doesn't explain not calling parent's virtual method, and
also
> please just put in comment (a bit shorter) the above explanation.

http://codereview.chromium.org/7087014/diff/34001/tools/gypv8sh.py
File tools/gypv8sh.py (right):

http://codereview.chromium.org/7087014/diff/34001/tools/gypv8sh.py#newcode63
tools/gypv8sh.py:63: exit(-1);
So it does; done.

On 2011/06/14 20:23:17, Paweł Hajdan Jr. wrote:
> I think there is some call in optparse to exit, could you use it?

[Paweł Hajdan Jr., 2011-06-15 08:12:31]

http://codereview.chromium.org/7087014/diff/33004/chrome/browser/ui/webui/web...
File chrome/browser/ui/webui/web_ui_browsertest.cc (right):

http://codereview.chromium.org/7087014/diff/33004/chrome/browser/ui/webui/web...
chrome/browser/ui/webui/web_ui_browsertest.cc:209: virtual void
SetUpOnMainThread() {
On 2011/06/14 22:39:26, Sheridan Rawlins wrote:
>   // Override this to add any custom setup code that needs to be done on the
>   // main thread after the browser is created and just before calling
>   // RunTestOnMainThread().
>   virtual void SetUpOnMainThread() {}
> 
> WebUIBrowserTest does not override this.  Sometimes virtual methods need to
call
> the parent's method, sometimes they don't and you must rely on comments & code
> to guide you. This particular function doesn't appear to need to call the
> parent's impl.

... and when WebUIBrowserTest _does_ override SetUpOnMainThread good luck
maintaining the mess and comments lying to you.

Please just call the parent's method; not doing so and relying on comments that
can get out of sync with reality is asking for trouble (and yes, I remember
things like that happening in the past and it was bad).

http://codereview.chromium.org/7087014/diff/41001/chrome/browser/ui/webui/web...
chrome/browser/ui/webui/web_ui_browsertest.cc:217: 
The comments don't explain why we're navigating to the downloads page.

http://codereview.chromium.org/7087014/diff/41001/tools/gypv8sh.py
File tools/gypv8sh.py (right):

http://codereview.chromium.org/7087014/diff/41001/tools/gypv8sh.py#newcode63
tools/gypv8sh.py:63: parser.exit(status=-1);
Sorry, I was thinking about parser.error (just replace entire body of this "if"
with parser.error("--product_dir option is required")).

[Sheridan Rawlins, 2011-06-15 23:54:32]

Addressed 2 out of 3 of your concerns & expressed my stance on the third.

http://codereview.chromium.org/7087014/diff/33004/chrome/browser/ui/webui/web...
File chrome/browser/ui/webui/web_ui_browsertest.cc (right):

http://codereview.chromium.org/7087014/diff/33004/chrome/browser/ui/webui/web...
chrome/browser/ui/webui/web_ui_browsertest.cc:209: virtual void
SetUpOnMainThread() {
On 2011/06/15 08:12:31, Paweł Hajdan Jr. wrote:
> On 2011/06/14 22:39:26, Sheridan Rawlins wrote:
> >   // Override this to add any custom setup code that needs to be done on the
> >   // main thread after the browser is created and just before calling
> >   // RunTestOnMainThread().
> >   virtual void SetUpOnMainThread() {}
> > 
> > WebUIBrowserTest does not override this.  Sometimes virtual methods need to
> call
> > the parent's method, sometimes they don't and you must rely on comments &
code
> > to guide you. This particular function doesn't appear to need to call the
> > parent's impl.
> 
> ... and when WebUIBrowserTest _does_ override SetUpOnMainThread good luck
> maintaining the mess and comments lying to you.
> 
> Please just call the parent's method; not doing so and relying on comments
that
> can get out of sync with reality is asking for trouble (and yes, I remember
> things like that happening in the past and it was bad).

Done.

http://codereview.chromium.org/7087014/diff/41001/chrome/browser/ui/webui/web...
chrome/browser/ui/webui/web_ui_browsertest.cc:217: 
Ok, I made a dummy page and navigate to there instead of using downloads page.

On 2011/06/15 08:12:31, Paweł Hajdan Jr. wrote:
> The comments don't explain why we're navigating to the downloads page.

http://codereview.chromium.org/7087014/diff/41001/tools/gypv8sh.py
File tools/gypv8sh.py (right):

http://codereview.chromium.org/7087014/diff/41001/tools/gypv8sh.py#newcode63
tools/gypv8sh.py:63: parser.exit(status=-1);
I really don't like the result as much.  With the current code, you get:

Need to supply the -p PRODUCT_DIR.
Usage: gypv8sh.py [options]

Options:
  -h, --help            show this help message and exit
  -v, --verbose         
  -n, --impotent        don't execute; just print (as if verbose)
  -p PRODUCT_DIR, --product_dir=PRODUCT_DIR
                        for gyp to set the <(PRODUCT_DIR) for running v8_shell
  -t, --test_fixture    print test_fixtures
  -i, --in              print inputs
  -o, --out             print outputs

With your recommendation, you get

Usage: gypv8sh.py [options]

gypv8sh.py: error: --product_dir option is required.

The help is much more useful than the usage as it actually describes the
--product_dir option.

On 2011/06/15 08:12:31, Paweł Hajdan Jr. wrote:
> Sorry, I was thinking about parser.error (just replace entire body of this
"if"
> with parser.error("--product_dir option is required")).

[Paweł Hajdan Jr., 2011-06-16 17:50:10]

LGTM with comments.

http://codereview.chromium.org/7087014/diff/41001/tools/gypv8sh.py
File tools/gypv8sh.py (right):

http://codereview.chromium.org/7087014/diff/41001/tools/gypv8sh.py#newcode63
tools/gypv8sh.py:63: parser.exit(status=-1);
On 2011/06/15 23:54:32, Sheridan Rawlins wrote:
> The help is much more useful than the usage as it actually describes the
> --product_dir option.

This is a good point, but I think consistency is better.

However, you'll be working much more frequently with this tool than I'll be, so
if it's important for you, feel free to leave the current version as is. In that
case, I'd have a small nit (also up to you): change "status=-1" to just "-1".

http://codereview.chromium.org/7087014/diff/45003/chrome/browser/ui/webui/web...
File chrome/browser/ui/webui/web_ui_browsertest.cc (right):

http://codereview.chromium.org/7087014/diff/45003/chrome/browser/ui/webui/web...
chrome/browser/ui/webui/web_ui_browsertest.cc:268: }
nit: Please add blank lines between methods.

[Sheridan Rawlins, 2011-06-17 00:44:55]

Trying trybots, then I'll commit.

http://codereview.chromium.org/7087014/diff/41001/tools/gypv8sh.py
File tools/gypv8sh.py (right):

http://codereview.chromium.org/7087014/diff/41001/tools/gypv8sh.py#newcode63
tools/gypv8sh.py:63: parser.exit(status=-1);
As it turns out you can adjust the usage with parser.set_usage().  It's a little
redundant for the programmer to maintain (I wish optparse would do it for me),
but I did that and changed to the error() call.

On 2011/06/16 17:50:10, Paweł Hajdan Jr. wrote:
> On 2011/06/15 23:54:32, Sheridan Rawlins wrote:
> > The help is much more useful than the usage as it actually describes the
> > --product_dir option.
> 
> This is a good point, but I think consistency is better.
> 
> However, you'll be working much more frequently with this tool than I'll be,
so
> if it's important for you, feel free to leave the current version as is. In
that
> case, I'd have a small nit (also up to you): change "status=-1" to just "-1".

http://codereview.chromium.org/7087014/diff/45003/chrome/browser/ui/webui/web...
File chrome/browser/ui/webui/web_ui_browsertest.cc (right):

http://codereview.chromium.org/7087014/diff/45003/chrome/browser/ui/webui/web...
chrome/browser/ui/webui/web_ui_browsertest.cc:268: }
On 2011/06/16 17:50:10, Paweł Hajdan Jr. wrote:
> nit: Please add blank lines between methods.

Done.

[commit-bot: I haz the power, 2011-06-17 08:00:07]

Change committed as 89453

[joth, 2011-06-17 08:13:34]

On 2011/06/17 08:00:07, I haz the power (commit-bot) wrote:
> Change committed as 89453

Looks like  chrome/browser/ui/webui/javascript2webui.js was committed with the
execute bit set, and check_deps is not happy
http://build.chromium.org/p/chromium/builders/Linux/builds/7993/steps/check_p...

Can you put in a fix?

[Timur Iskhodzhanov, 2011-06-17 08:30:14]

Looks like this has broken the build on Mac.
Mind reverting?

[joth, 2011-06-17 08:33:52]

I can't contact scr, so I'm just reverting it now.
(alas I just fixed the aforementioned permissions issue, which may make the
revert more tricky)


On 17 June 2011 09:30, <timurrrr@chromium.org> wrote:

> Looks like this has broken the build on Mac.
> Mind reverting?
>
>
>
http://codereview.chromium.**org/7087014/<http://codereview.chromium.org/7087...
>

[joth, 2011-06-17 08:35:11]

On 2011/06/17 08:33:52, joth wrote:
> I can't contact scr, so I'm just reverting it now.
> (alas I just fixed the aforementioned permissions issue, which may make the
> revert more tricky)
> 
> 

Revert: http://codereview.chromium.org/7193030/

[joth, 2011-06-17 08:47:25]

FYI looks like this patch was the probable cause of chromeos compile failure
(haven't investigated deeply)


http://build.chromium.org/p/chromium/builders/Arm/builds/12743/steps/compile/...

/lib/ld-linux.so.3: No such file or directory
make: *** [chrome/browser/ui/webui/web_ui_browsertest-inl.h] Error 255




On 17 June 2011 09:35, <joth@chromium.org> wrote:

> On 2011/06/17 08:33:52, joth wrote:
>
>> I can't contact scr, so I'm just reverting it now.
>> (alas I just fixed the aforementioned permissions issue, which may make
>> the
>> revert more tricky)
>>
>
>
>
> Revert:
http://codereview.chromium.**org/7193030/<http://codereview.chromium.org/7193...
>
>
>
>
http://codereview.chromium.**org/7087014/<http://codereview.chromium.org/7087...
>

[M-A Ruel, 2011-06-17 18:15:36]

Drive-by

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py
File tools/gypv8sh.py (right):

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode1
tools/gypv8sh.py:1: #!/usr/bin/python
#!/usr/bin/env python

Don't forget your BSD friends.

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode6
tools/gypv8sh.py:6: # This script is used by chrome_tests.gypi's js2webui action
to maintain the
Use a docstring instead.

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode13
tools/gypv8sh.py:13: #   python tools/gypv8sh.py -o # print outputs
Add empty line

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode26
tools/gypv8sh.py:26: ],
Align ] on [

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode32
tools/gypv8sh.py:32: # For options -t, -i, & -o, we print the "column" of the
|rules|.  We keep a set
Put that in a main() function and only keep this at the end:

if __name__ == '__main__':
  sys.exit(main())

otherwise it's easy to get variable aliasing issues.

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode39
tools/gypv8sh.py:39: "%prog [-v][-n] (-i|-t|-o)");
comma everywhere

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode40
tools/gypv8sh.py:40: parser.add_option('-v', '--verbose', action='store_true',
dest='verbose')
Don't use dest='' if it is the same name than the long argument name.

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode43
tools/gypv8sh.py:43: parser.add_option('-p', '--product_dir', action='store',
type='string',
action='store', type='string' is superfluous, it's already the default.

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode44
tools/gypv8sh.py:44: dest='product_dir',
like this dest=''

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode47
tools/gypv8sh.py:47: callback_args=tuple([print_rule_indices, 0]),
callback_args=([print_rule_indices, 0],)

But this code looks like it's really opaque for no good reason.

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode55
tools/gypv8sh.py:55: (opts, args) = parser.parse_args();
Remove extra comma

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode57
tools/gypv8sh.py:57: if len(print_rule_indices):
if print_rule_indices:

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode60
tools/gypv8sh.py:60: print rule[index],
Did you intent?
print '\n'.join(rule[index] for index in sorted(print_rule_indices))

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode69
tools/gypv8sh.py:69: arguments = "arguments=" + json.dumps(arguments)
This variable aliasing is weird since you use 2 different types for the same
variable.

[Sheridan Rawlins, 2011-06-18 00:03:11]

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py
File tools/gypv8sh.py (right):

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode1
tools/gypv8sh.py:1: #!/usr/bin/python
On 2011/06/17 18:15:36, Marc-Antoine Ruel wrote:
> #!/usr/bin/env python
> 
> Don't forget your BSD friends.

Done.

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode6
tools/gypv8sh.py:6: # This script is used by chrome_tests.gypi's js2webui action
to maintain the
On 2011/06/17 18:15:36, Marc-Antoine Ruel wrote:
> Use a docstring instead.

Done.

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode13
tools/gypv8sh.py:13: #   python tools/gypv8sh.py -o # print outputs
On 2011/06/17 18:15:36, Marc-Antoine Ruel wrote:
> Add empty line

Done.

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode26
tools/gypv8sh.py:26: ],
On 2011/06/17 18:15:36, Marc-Antoine Ruel wrote:
> Align ] on [

Done.

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode32
tools/gypv8sh.py:32: # For options -t, -i, & -o, we print the "column" of the
|rules|.  We keep a set
On 2011/06/17 18:15:36, Marc-Antoine Ruel wrote:
> Put that in a main() function and only keep this at the end:
> 
> if __name__ == '__main__':
>   sys.exit(main())
> 
> otherwise it's easy to get variable aliasing issues.

Done.

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode39
tools/gypv8sh.py:39: "%prog [-v][-n] (-i|-t|-o)");
On 2011/06/17 18:15:36, Marc-Antoine Ruel wrote:
> comma everywhere

As per IM session, you mean remove all semi-colons.  Done.

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode40
tools/gypv8sh.py:40: parser.add_option('-v', '--verbose', action='store_true',
dest='verbose')
On 2011/06/17 18:15:36, Marc-Antoine Ruel wrote:
> Don't use dest='' if it is the same name than the long argument name.

Done.

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode43
tools/gypv8sh.py:43: parser.add_option('-p', '--product_dir', action='store',
type='string',
On 2011/06/17 18:15:36, Marc-Antoine Ruel wrote:
> action='store', type='string' is superfluous, it's already the default.

Done.

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode44
tools/gypv8sh.py:44: dest='product_dir',
On 2011/06/17 18:15:36, Marc-Antoine Ruel wrote:
> like this dest=''

Done.

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode47
tools/gypv8sh.py:47: callback_args=tuple([print_rule_indices, 0]),
As we only need to print one "column" at a time, used store_const instead.

On 2011/06/17 18:15:36, Marc-Antoine Ruel wrote:
> callback_args=([print_rule_indices, 0],)
> 
> But this code looks like it's really opaque for no good reason.

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode55
tools/gypv8sh.py:55: (opts, args) = parser.parse_args();
On 2011/06/17 18:15:36, Marc-Antoine Ruel wrote:
> Remove extra comma

Done.

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode57
tools/gypv8sh.py:57: if len(print_rule_indices):
On 2011/06/17 18:15:36, Marc-Antoine Ruel wrote:
> if print_rule_indices:

Done.

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode60
tools/gypv8sh.py:60: print rule[index],
I don't think so, but it's moot with rewrite.

On 2011/06/17 18:15:36, Marc-Antoine Ruel wrote:
> Did you intent?
> print '\n'.join(rule[index] for index in sorted(print_rule_indices))

http://codereview.chromium.org/7087014/diff/49002/tools/gypv8sh.py#newcode69
tools/gypv8sh.py:69: arguments = "arguments=" + json.dumps(arguments)
On 2011/06/17 18:15:36, Marc-Antoine Ruel wrote:
> This variable aliasing is weird since you use 2 different types for the same
> variable.

Rolled 69 into the next line - Done.

[M-A Ruel, 2011-06-20 16:38:45]

python lgtm.

http://codereview.chromium.org/7087014/diff/59001/tools/js2webui.py
File tools/js2webui.py (right):

http://codereview.chromium.org/7087014/diff/59001/tools/js2webui.py#newcode35
tools/js2webui.py:35: """Run the program"""
Please remove.

http://codereview.chromium.org/7087014/diff/59001/tools/js2webui.py#newcode57
tools/js2webui.py:57: if (opts.print_rule_index != None):
if opts.print_rule_index is not None:

http://codereview.chromium.org/7087014/diff/59001/tools/js2webui.py#newcode59
tools/js2webui.py:59: print rule[opts.print_rule_index]
optioanl: You can add a return 0, so you don't need to indent the remaining on
the function.

[Sheridan Rawlins, 2011-06-22 06:49:22]

Ok, based on comments from v8 team, reverted patchset 29 to use the v8_shell to
run pure javascript instead of using experimental flags (--print_json_ast).

This is still waiting for v8's http://code.google.com/p/v8/source/detail?r=8356
to merge to trunk before making the appropriate DEPS changes.

http://codereview.chromium.org/7087014/diff/59001/tools/js2webui.py
File tools/js2webui.py (right):

http://codereview.chromium.org/7087014/diff/59001/tools/js2webui.py#newcode35
tools/js2webui.py:35: """Run the program"""
On 2011/06/20 16:38:46, Marc-Antoine Ruel wrote:
> Please remove.

Done.

http://codereview.chromium.org/7087014/diff/59001/tools/js2webui.py#newcode57
tools/js2webui.py:57: if (opts.print_rule_index != None):
On 2011/06/20 16:38:46, Marc-Antoine Ruel wrote:
> if opts.print_rule_index is not None:

Done.

http://codereview.chromium.org/7087014/diff/59001/tools/js2webui.py#newcode59
tools/js2webui.py:59: print rule[opts.print_rule_index]
On 2011/06/20 16:38:46, Marc-Antoine Ruel wrote:
> optioanl: You can add a return 0, so you don't need to indent the remaining on
> the function.

Done.

[Sheridan Rawlins, 2011-06-29 22:44:42]

After a great conversation with gyp guru Brad Nelson, I was able to convert this
from actions to rules and also output into the intermediate dir rather than into
the source (which removed the need for .gitignore). Besides being correct, this
also makes the timestamp check as granular as it can be, and makes the
maintenance of the javascript test files in one place in one gyp
(chrome_tests.gyp), rather than in the python script, which is now just a
processing script to convert arguments (jsonify) and execute the v8_shell,
piping output to the output file & removing it on errors.

[cmp, 2011-07-01 00:33:22]

lgtm with ifdef-out-on-arm changes with nit (assuming previous reviews' lgtms
carry over)

http://codereview.chromium.org/7087014/diff/71001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/7087014/diff/71001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:566: '../ui/ui.gyp:ui_resources',
Is this change from app -> ui a part of the "don't build on Arm strategy".

[Sheridan Rawlins, 2011-07-01 05:24:36]

Thanks.

Waiting for manual build on Arm from Jeff Bailey, before commit, hopefully
before lunch Friday.

http://codereview.chromium.org/7087014/diff/71001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/7087014/diff/71001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:566: '../ui/ui.gyp:ui_resources',
No, I also did a git pull before this change, so it includes changes from ToT. 
If you look at the diff from origin..., it doesn't include this line as a diff.

On 2011/07/01 00:33:22, cmp wrote:
> Is this change from app -> ui a part of the "don't build on Arm strategy".

[cmp, 2011-07-01 17:56:56]

lgtm, thanks for the update