test_runner: Add TestInfoExtractor to get the list of test-urls.

test_runner: Add TestInfoExtractor to get the list of test-urls.

Move the code to read the list of layout-tests to run into the test_runner
component. Use this from layout-test runner in both content_shell and web_view.

BUG=478250
R=jochen@chromium.org, rsleevi@chromium.org

Committed: https://crrev.com/bb75fb72829076bc8fd1942d8fe0a9bc7a307d59
Cr-Commit-Position: refs/heads/master@{#348307}

[sadrul, 2015-09-04 18:20:35]

sadrul@chromium.org changed reviewers:
+ jam@chromium.org

[sadrul, 2015-09-04 18:20:35]

This is a follow-up of https://codereview.chromium.org/1308973005/ to refactor
the code to read the list of tests from stdin into //components/test_runner, and
use that for both content and web_view layout tests.

[jam, 2015-09-08 15:31:36]

jam@chromium.org changed reviewers:
+ jochen@chromium.org

[jam, 2015-09-08 15:31:37]

switching reviewers to jochen

[jochen (gone - plz use gerrit), 2015-09-09 14:36:55]

lgtm with comment

https://codereview.chromium.org/1312693007/diff/1/components/test_runner/test...
File components/test_runner/test_info_extractor.h (right):

https://codereview.chromium.org/1312693007/diff/1/components/test_runner/test...
components/test_runner/test_info_extractor.h:18: TestInfo();
can you add parameters for all the members to the ctor, so the struct is always
initialized?

[sadrul, 2015-09-10 03:28:14]

Patchset #2 (id:20001) has been deleted

[sadrul, 2015-09-10 06:58:27]

sadrul@chromium.org changed reviewers:
+ rsesek@chromium.org

[sadrul, 2015-09-10 06:58:28]

+rsesek@ for moving +net from  components/web_view/test_runner/DEPS to 
components/test_runner/DEPS

[Robert Sesek, 2015-09-10 17:28:15]

On 2015/09/10 06:58:28, sadrul wrote:
> +rsesek@ for moving +net from  components/web_view/test_runner/DEPS to 
> components/test_runner/DEPS

I don't think I'm an OWNER?

[sadrul, 2015-09-10 17:29:47]

sadrul@chromium.org changed reviewers:
+ rsleevi@chromium.org
- rsesek@chromium.org

[sadrul, 2015-09-10 17:29:47]

Oh, wrong rs... sorry :)

+rsleevi@

[Ryan Sleevi, 2015-09-10 17:48:04]

I realized you only asked for the DEPS perspective, but I try to give a complete
review whenever a new DEPS is added to look for any gotchas or dragons. A few
(unrelated to DEPS) comments

https://codereview.chromium.org/1312693007/diff/40001/components/test_runner/...
File components/test_runner/DEPS (right):

https://codereview.chromium.org/1312693007/diff/40001/components/test_runner/...
components/test_runner/DEPS:4: "+net",
Do you need a blanket //net DEP here? Would it suffice for //net/base?

The context is I think if you're messing around with sockets or URLs, we'd
ideally like to find someone on the //net team to partner to understands needs,
use cases, API shapes, etc. If you're just doing this for the file APIs (as it
seems), it may help to make a more narrow inclusion via DEPS to help ensure
that.

https://codereview.chromium.org/1312693007/diff/40001/components/test_runner/...
File components/test_runner/test_info_extractor.cc (right):

https://codereview.chromium.org/1312693007/diff/40001/components/test_runner/...
components/test_runner/test_info_extractor.cc:30: const char
kAndroidLayoutTestBase[] = "http://127.0.0.1:8000/all-tests/";
BUG: I don't know if this is something Blink is already doing, but hardcoding
the URL like this is something we strongly discourage in //net.

For example, as written, this could potentially prevent test concurrency and
potentially cause issues with port forwarding. When we hardcoded URLs, we had
all sorts of issues.

Is there a better way to do this? Either IPC or via the command-line to indicate
where the test harness should talk to for running the tests?

This also suggests there may be platform test divergences - using a file:// URL
on one platform, but an http:// URL on another, which may lead to any number of
surprising things (especially as we move towards requiring secure contexts)

https://codereview.chromium.org/1312693007/diff/40001/components/web_view/tes...
File components/web_view/test_runner/test_runner_application_delegate.cc
(right):

https://codereview.chromium.org/1312693007/diff/40001/components/web_view/tes...
components/web_view/test_runner/test_runner_application_delegate.cc:105: if
(auto test_info = test_extractor_->GetNextTest())
This does not seem like a style-guide approved use of auto, nor does line 102
(in particular, the clause "Continue to use manifest type declarations when it
helps readability, ")

The thread from
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/5-Bt3BJzAo0/EWcU2...
explains how this usage can hide dangerous subtleties

https://codereview.chromium.org/1312693007/diff/40001/components/web_view/tes...
components/web_view/test_runner/test_runner_application_delegate.cc:137: if
(auto test_info = test_extractor_->GetNextTest())
Same

https://codereview.chromium.org/1312693007/diff/40001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_browser_main.cc (right):

https://codereview.chromium.org/1312693007/diff/40001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_browser_main.cc:85: while (auto
test_info = test_extractor.GetNextTest()) {
More auto danger here

[sadrul, 2015-09-10 18:09:09]

On 2015/09/10 17:48:04, Ryan Sleevi wrote:
> I realized you only asked for the DEPS perspective, but I try to give a
complete
> review whenever a new DEPS is added to look for any gotchas or dragons. A few
> (unrelated to DEPS) comments

Appreciate the thorough review. Thank you!

https://codereview.chromium.org/1312693007/diff/40001/components/test_runner/...
File components/test_runner/DEPS (right):

https://codereview.chromium.org/1312693007/diff/40001/components/test_runner/...
components/test_runner/DEPS:4: "+net",
On 2015/09/10 17:48:03, Ryan Sleevi wrote:
> Do you need a blanket //net DEP here? Would it suffice for //net/base?
> 
> The context is I think if you're messing around with sockets or URLs, we'd
> ideally like to find someone on the //net team to partner to understands
needs,
> use cases, API shapes, etc. If you're just doing this for the file APIs (as it
> seems), it may help to make a more narrow inclusion via DEPS to help ensure
> that.

Yep, makes sense. Narrowed to net/base/.

https://codereview.chromium.org/1312693007/diff/40001/components/test_runner/...
File components/test_runner/test_info_extractor.cc (right):

https://codereview.chromium.org/1312693007/diff/40001/components/test_runner/...
components/test_runner/test_info_extractor.cc:30: const char
kAndroidLayoutTestBase[] = "http://127.0.0.1:8000/all-tests/";
On 2015/09/10 17:48:03, Ryan Sleevi wrote:
> BUG: I don't know if this is something Blink is already doing, but hardcoding
> the URL like this is something we strongly discourage in //net.
> 
> For example, as written, this could potentially prevent test concurrency and
> potentially cause issues with port forwarding. When we hardcoded URLs, we had
> all sorts of issues.
> 
> Is there a better way to do this? Either IPC or via the command-line to
indicate
> where the test harness should talk to for running the tests?
> 
> This also suggests there may be platform test divergences - using a file://
URL
> on one platform, but an http:// URL on another, which may lead to any number
of
> surprising things (especially as we move towards requiring secure contexts)

This is something we have been doing for a while on android
(https://chromiumcodereview.appspot.com/17265005). I have filed a bug to address
the concerns (http://crbug.com/530257)

https://codereview.chromium.org/1312693007/diff/40001/components/web_view/tes...
File components/web_view/test_runner/test_runner_application_delegate.cc
(right):

https://codereview.chromium.org/1312693007/diff/40001/components/web_view/tes...
components/web_view/test_runner/test_runner_application_delegate.cc:105: if
(auto test_info = test_extractor_->GetNextTest())
On 2015/09/10 17:48:04, Ryan Sleevi wrote:
> This does not seem like a style-guide approved use of auto, nor does line 102
> (in particular, the clause "Continue to use manifest type declarations when it
> helps readability, ")
> 
> The thread from
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/5-Bt3BJzAo0/EWcU2...
> explains how this usage can hide dangerous subtleties

Done.

https://codereview.chromium.org/1312693007/diff/40001/components/web_view/tes...
components/web_view/test_runner/test_runner_application_delegate.cc:137: if
(auto test_info = test_extractor_->GetNextTest())
On 2015/09/10 17:48:03, Ryan Sleevi wrote:
> Same

Done.

https://codereview.chromium.org/1312693007/diff/40001/content/shell/browser/l...
File content/shell/browser/layout_test/layout_test_browser_main.cc (right):

https://codereview.chromium.org/1312693007/diff/40001/content/shell/browser/l...
content/shell/browser/layout_test/layout_test_browser_main.cc:85: while (auto
test_info = test_extractor.GetNextTest()) {
On 2015/09/10 17:48:04, Ryan Sleevi wrote:
> More auto danger here

Done.

[Ryan Sleevi, 2015-09-10 23:04:48]

lgtm

[commit-bot: I haz the power, 2015-09-11 01:28:12]

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

[sadrul, 2015-09-11 01:28:33]

Committed patchset #3 (id:60001) manually as
bb75fb72829076bc8fd1942d8fe0a9bc7a307d59.

[commit-bot: I haz the power, 2015-09-23 12:17:28]

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