Introduce WPTServe for running W3C Blink Layout tests

Basic support for running WPTServe locally through run-webkit-tests.

At this stage supporting only local linux runs with some additional setup.
Note that it also requires additional setup steps for web-platform-tests, such as SSL configuration.

DESIGN DOC:
https://goo.gl/iXUaZd (DRAFT)


INSTRUCTIONS

Assuming you are in the src/third_party/WebKit directory:

Run some tests with enabling WPT serve
$ ./Tools/Scripts/run-webkit-tests --debug --enable-wptserve --no-new-test-results imported/web-platform-tests

BUG=360762, 347864, 486860
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197732

[burnik, 2015-06-03 18:46:20]

burnik@chromium.org changed reviewers:
+ dpranke@chromium.org, joelo@chromium.org, jsbell@chromium.org

[jsbell, 2015-06-04 17:43:25]

just some initial nits, I'll go through it again and try it out when I have
cycles.

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right):

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:103: """Wheter this
test is part of a web-platform-tests which require wptserve servers."""
typo: "whether"

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:366:
self._printer.write_update('Stopping WPTServe servers ...')
Nit: make string symmetric with 'Starting WPTserve ...' (i.e. drop 'servers')

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
File Tools/Scripts/webkitpy/layout_tests/port/base.py (right):

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
Tools/Scripts/webkitpy/layout_tests/port/base.py:67: from
webkitpy.layout_tests.servers.third_party import wptserve_http
nit: sort the import lines

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
Tools/Scripts/webkitpy/layout_tests/port/base.py:1667: if self.is_wpt_enabled()
and "web-platform-tests" in test_input.test_name:
Can you factor out is_wpt_test() or just use one from another class (if an
instance is handy)?

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
File Tools/Scripts/webkitpy/layout_tests/port/driver.py (right):

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
Tools/Scripts/webkitpy/layout_tests/port/driver.py:232: return
self._port.is_wpt_enabled() and "web-platform-tests" in test_name
Use WPT_DIR here?

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
Tools/Scripts/webkitpy/layout_tests/port/driver.py:254: if
self.is_wpt_test(test_name):
Use is_wpt_test var from above?

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
File Tools/Scripts/webkitpy/layout_tests/servers/third_party/wptserve_http.py
(right):

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
Tools/Scripts/webkitpy/layout_tests/servers/third_party/wptserve_http.py:1: #
Copyright (C) 2015 Google Inc. All rights reserved.
Don't copy/paste the header, use the latest from:

https://www.chromium.org/developers/coding-style#TOC-File-headers

(It's also much shorter!)

[Dirk Pranke, 2015-06-04 20:51:53]

This looks like a pretty good start, but I do have a number of things you should
change. I don't think any of them are drastic.

You should probably restructure the patch so that all of the dependent
(third-party) code is checked in, rather than going through this checkout/pull
step you seem to be doing.

It's not immediately obvious to me if the code should be checked in directly or
pulled via DEPS, but we should definitely try to avoid having to do a 'git
clone' of the full w-p-t repo at any point just to get wptserve.

Can you give me an estimate of how much code we need to pull for this, and
whether any of it is compiled (native, platform-specific) code in python
extensions? I'm hoping the latter answer is zero.

Also, if we did need to pull subrepos, we need to make sure they are mirrored
onto Git-on-Borg (servers under our control); we cannot depend on third-party
repos directly when running the tests.

You should also get rid of the WPT_ENABLED files. This patch should either just
assume that wpt will be enabled, or control this via a command line flag.

Do we have sense yet of the performance of wptserve and how long it takes to run
the files through the server versus locally as files?

Lastly, I suspect we'll want to change a number of things in the way we import
files if we can assume that they'll all be run through the web server, but we
probably shouldn't try to do that as part of this patch.

All of this suggests that this is a big enough project that it might be good to
have a short design doc or writeup somewhere over how this will all work,
separate from just the code changes.

This could be some text in bug 360762, or a short Google Doc (the latter is
probably better since it gives us a way to add comments more easily).

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
File Tools/Scripts/webkitpy/layout_tests/port/base.py (right):

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
Tools/Scripts/webkitpy/layout_tests/port/base.py:1668: return False
why do you need this block at all?

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
File Tools/Scripts/webkitpy/layout_tests/port/driver.py (right):

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
Tools/Scripts/webkitpy/layout_tests/port/driver.py:232: return
self._port.is_wpt_enabled() and "web-platform-tests" in test_name
On 2015/06/04 17:43:25, jsbell wrote:
> Use WPT_DIR here?

I would add an is_wpt_test() method on the port, and call that from all of the
places you need this, rather than adding this three times.

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
Tools/Scripts/webkitpy/layout_tests/port/driver.py:236: # TODO(burnik): Read
from config or args.
Do you imagine needing multiple entries here? Why isn't hard-coded good enough?

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
File Tools/Scripts/webkitpy/layout_tests/servers/third_party/wptserve_http.py
(right):

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
Tools/Scripts/webkitpy/layout_tests/servers/third_party/wptserve_http.py:2: #
Put this file in layout_tests/servers; you do not need the "third_party" part;
after all wptserve is no more third-party than pywebsocket or apache :), and
this is all blink code, not third-party code.

[jsbell, 2015-06-04 21:02:47]

On 2015/06/04 20:51:53, Dirk Pranke wrote:
> All of this suggests that this is a big enough project that it might be good
to
> have a short design doc or writeup somewhere over how this will all work,
> separate from just the code changes.
> 
> This could be some text in bug 360762, or a short Google Doc (the latter is
> probably better since it gives us a way to add comments more easily).

+1

I am assuming this is an early stage in a roadmap something like this:

0. This CL - proof of concept, linux only, manual work required, but helps a
blink dev get running locally
1. Refine the tool to eliminate manual work
2. Pull in the dependencies via DEPS rather than 
3. Get it working on OSX
4. Get it working on Windows
5. Try this out on the bots
6. Enable for everyone/try jobs/waterfall

... where ordering for 1/2/3/4 is flexible. And maybe we should just jump
straight to #2? Not sure, hence doc of the roadmap to comment on would be great.

[burnik, 2015-06-05 12:58:20]

On 2015/06/04 21:02:47, jsbell wrote:
> On 2015/06/04 20:51:53, Dirk Pranke wrote:
> > All of this suggests that this is a big enough project that it might be good
> to
> > have a short design doc or writeup somewhere over how this will all work,
> > separate from just the code changes.
> > 
> > This could be some text in bug 360762, or a short Google Doc (the latter is
> > probably better since it gives us a way to add comments more easily).
> 
> +1
> 
> I am assuming this is an early stage in a roadmap something like this:
> 
> 0. This CL - proof of concept, linux only, manual work required, but helps a
> blink dev get running locally
> 1. Refine the tool to eliminate manual work
> 2. Pull in the dependencies via DEPS rather than 
> 3. Get it working on OSX
> 4. Get it working on Windows
> 5. Try this out on the bots
> 6. Enable for everyone/try jobs/waterfall
> 
> ... where ordering for 1/2/3/4 is flexible. And maybe we should just jump
> straight to #2? Not sure, hence doc of the roadmap to comment on would be
great.

I think the roadmap you suggested is almost exactly the same as what I had in
mind. :-)

I've added the design doc link so we can further discuss details there.

[burnik, 2015-06-05 12:59:29]

Thank for all the nitpicking, I believe I managed to factor our or fix most of
them. :-)

Also, updated description and added link to design doc.

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right):

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:103: """Wheter this
test is part of a web-platform-tests which require wptserve servers."""
On 2015/06/04 17:43:25, jsbell wrote:
> typo: "whether"

Done.

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:366:
self._printer.write_update('Stopping WPTServe servers ...')
On 2015/06/04 17:43:25, jsbell wrote:
> Nit: make string symmetric with 'Starting WPTserve ...' (i.e. drop 'servers')

Done.

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
File Tools/Scripts/webkitpy/layout_tests/port/base.py (right):

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
Tools/Scripts/webkitpy/layout_tests/port/base.py:67: from
webkitpy.layout_tests.servers.third_party import wptserve_http
On 2015/06/04 17:43:25, jsbell wrote:
> nit: sort the import lines

Done.

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
Tools/Scripts/webkitpy/layout_tests/port/base.py:1667: if self.is_wpt_enabled()
and "web-platform-tests" in test_input.test_name:
On 2015/06/04 17:43:25, jsbell wrote:
> Can you factor out is_wpt_test() or just use one from another class (if an
> instance is handy)?

Done.

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
Tools/Scripts/webkitpy/layout_tests/port/base.py:1668: return False
On 2015/06/04 20:51:53, Dirk Pranke wrote:
> why do you need this block at all?

When this was regarded as a pixel test, I had issues with the expected output
type (image diff rather than text diff). I will look into it in more detail
before we land any of this.

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
File Tools/Scripts/webkitpy/layout_tests/port/driver.py (right):

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
Tools/Scripts/webkitpy/layout_tests/port/driver.py:232: return
self._port.is_wpt_enabled() and "web-platform-tests" in test_name
On 2015/06/04 17:43:25, jsbell wrote:
> Use WPT_DIR here?

Factored out.

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
Tools/Scripts/webkitpy/layout_tests/port/driver.py:232: return
self._port.is_wpt_enabled() and "web-platform-tests" in test_name
On 2015/06/04 20:51:53, Dirk Pranke wrote:
> On 2015/06/04 17:43:25, jsbell wrote:
> > Use WPT_DIR here?
> 
> I would add an is_wpt_test() method on the port, and call that from all of the
> places you need this, rather than adding this three times.

Done.

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
Tools/Scripts/webkitpy/layout_tests/port/driver.py:236: # TODO(burnik): Read
from config or args.
On 2015/06/04 20:51:53, Dirk Pranke wrote:
> Do you imagine needing multiple entries here? Why isn't hard-coded good
enough?

Hardcoded is good enough for now. I believe it would be easier to have this in
one place so we can adjust the config rather than maintaining the values in at
least 2 places.

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
Tools/Scripts/webkitpy/layout_tests/port/driver.py:254: if
self.is_wpt_test(test_name):
On 2015/06/04 17:43:25, jsbell wrote:
> Use is_wpt_test var from above?

Done. Forgot to commit and repush this the first time. :-/

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
File Tools/Scripts/webkitpy/layout_tests/servers/third_party/wptserve_http.py
(right):

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
Tools/Scripts/webkitpy/layout_tests/servers/third_party/wptserve_http.py:1: #
Copyright (C) 2015 Google Inc. All rights reserved.
On 2015/06/04 17:43:25, jsbell wrote:
> Don't copy/paste the header, use the latest from:
> 
> https://www.chromium.org/developers/coding-style#TOC-File-headers
> 
> (It's also much shorter!)

Done.

https://codereview.chromium.org/1154373005/diff/1/Tools/Scripts/webkitpy/layo...
Tools/Scripts/webkitpy/layout_tests/servers/third_party/wptserve_http.py:2: #
On 2015/06/04 20:51:53, Dirk Pranke wrote:
> Put this file in layout_tests/servers; you do not need the "third_party" part;
> after all wptserve is no more third-party than pywebsocket or apache :), and
> this is all blink code, not third-party code.

Done.

[Dirk Pranke, 2015-06-05 18:27:49]

looks pretty close. I think if you check in wptserve directly and clean up the
rest of that scaffolding, we'll be ready to go.

https://codereview.chromium.org/1154373005/diff/20001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right):

https://codereview.chromium.org/1154373005/diff/20001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:365: 
nit: you only need one blank line between class methods.

https://codereview.chromium.org/1154373005/diff/20001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/base.py (right):

https://codereview.chromium.org/1154373005/diff/20001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/base.py:1170: self._is_wpt_enabled =
is_wpt_enabled
It doesn't seem like you need this, since it is set in the constructor from the
options object?

https://codereview.chromium.org/1154373005/diff/20001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/servers/wpt_support/WPTSubmodules
(right):

https://codereview.chromium.org/1154373005/diff/20001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/servers/wpt_support/WPTSubmodules:6: ./tools
html5lib 7cce65bbaa78411f98b8b37eeefc9db03c580097
Where does this list come from?

We have pywebsocket already in src/third_party

I don't see any reference to six or html5lib in wptserve, and I don't know what
testdata is?

wptserve is tiny, so I would just check include it directly in this patch under
webkitpy/third_party, and I think you can skip this whole checkout step?

(apart from maybe still wanting the full wpt repo, of course).

[jsbell, 2015-06-05 19:07:45]

quick nits, still haven't tried running it - will do so after lunch

https://codereview.chromium.org/1154373005/diff/20001/LayoutTests/WPT_DISABLED
File LayoutTests/WPT_DISABLED (right):

https://codereview.chromium.org/1154373005/diff/20001/LayoutTests/WPT_DISABLE...
LayoutTests/WPT_DISABLED:1: Existance of this file (WPT_DISABLED) is turning off
the WPTServe feature for Blink Layout tests.
This file can be deleted from the CL now.

https://codereview.chromium.org/1154373005/diff/20001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right):

https://codereview.chromium.org/1154373005/diff/20001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:188: help="Enable
running web-plaform-tests using WPTserve instead of Apache."),
nit: indenting here is inconsistent

[jsbell, 2015-06-05 19:43:25]

If I ctrl+C during a run I am left with several python
/usr/local/google/home/jsbell/google/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/servers/wpt_support/wpt/serve
processes running which are holding onto port 8001; I need to manually kill them
to do another run.

[jsbell, 2015-06-05 19:47:25]

On 2015/06/05 19:43:25, jsbell wrote:
> ... I am left with several python ... processes running....

This is true even if I don't kill run-webkit-tests (i.e. I let it complete
normally).

[jsbell, 2015-06-05 20:34:25]

... and, of course, all of the testharness-based tests fail since the script
resources can't be found - at least for now the imported tests all have
resources references of the form src="../../../resources/XXX" which (for tests
up to 3 levels deep) turns into "/resources/XXX". I can get the test directories
I own (IndexedDB, encoding) passing by adding these under
imported/web-platform-tests:

resources/testharness.js
resources/testharnessreport.js
resources/vendor-prefix.js
resources/idlharness.js
resources/webidl2/lib/webidl2.js

(/resources/WebIDLParser.js is rewritten to /resources/webidl2/lib/webidl2.js by
wptserve's config)

Um... yay for more copies?

...

Also, if we leave any config nuggets or directories behind we need to ensure the
files are in .gitignore. (With this CL as-is, that'd be the wpt/ directory)

[jsbell, 2015-06-05 20:36:31]

And just so it's clear: this is an AWESOME start, thanks for taking it on
burnik!

[burnik, 2015-06-08 12:16:49]

On 2015/06/05 19:43:25, jsbell wrote:
> If I ctrl+C during a run I am left with several python
>
/usr/local/google/home/jsbell/google/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/servers/wpt_support/wpt/serve
> processes running which are holding onto port 8001; I need to manually kill
them
> to do another run.

Yes, I'm aware of this issue. It has to do with the host.popen and
host.kill_process,
The process should be started and killed along with spawned children
(inter-operable).
Any suggestion on how to do it with the "host" class?

[burnik, 2015-06-08 12:17:50]

On 2015/06/05 19:47:25, jsbell wrote:
> On 2015/06/05 19:43:25, jsbell wrote:
> > ... I am left with several python ... processes running....
> 
> This is true even if I don't kill run-webkit-tests (i.e. I let it complete
> normally).

Ditto. :-)

[burnik, 2015-06-08 12:28:12]

On 2015/06/05 20:34:25, jsbell wrote:
> ... and, of course, all of the testharness-based tests fail since the script
> resources can't be found - at least for now the imported tests all have
> resources references of the form src="../../../resources/XXX" which (for tests
> up to 3 levels deep) turns into "/resources/XXX". I can get the test
directories
> I own (IndexedDB, encoding) passing by adding these under
> imported/web-platform-tests:
> 
> resources/testharness.js
> resources/testharnessreport.js
> resources/vendor-prefix.js
> resources/idlharness.js
> resources/webidl2/lib/webidl2.js
> 
> (/resources/WebIDLParser.js is rewritten to /resources/webidl2/lib/webidl2.js
by
> wptserve's config)
> 
> Um... yay for more copies?
> 
> ...
> 
> Also, if we leave any config nuggets or directories behind we need to ensure
the
> files are in .gitignore. (With this CL as-is, that'd be the wpt/ directory)

Regarding resources, this can all be fixed, ideally (in the future) in the
following manner:
- do not convert tests when importing to imported/web-platform/tests (optional)
- keep ./resources directory as is for the most part in
$WEBKIT/LayoutTests/imported/web-platform-tests/

Or we can do some rewriting in the config.json of WPT. In any case, 
I will tend not to have duplicates lying everywhere around before we decide to
land this.

Agreed for the .gitignore part, however I still haven't decided if we should
be generating the config file on-the-fly. I would be more comfortable to have it
checked in as part of the CL or expand ./serve to accept command line flags for 
the entire config granularly (or by reading JSON from stdin).
As I have already landed the support to define doc_root and ws_doc_root through
command line,
https://github.com/w3c/wpt-tools/pull/9

it would be a plausible approach.

What do you think?

[burnik, 2015-06-08 12:34:20]

On 2015/06/05 20:36:31, jsbell wrote:
> And just so it's clear: this is an AWESOME start, thanks for taking it on
> burnik!

My pleasure! 

I know, this is only the tip of the iceberg. :-)
A lot of stuff to consider and discuss, so it's good I started
writing the design doc.

I'm sorry I cannot move any faster with this, 
since it's only a P2 for me at this point.

In any case, it's my goal to land this and make it useful some time
this month if it proves possible.

[burnik, 2015-06-08 12:37:02]

Most of nits addressed. There are some (new) open questions,
we can continue with comments in the design doc.

https://codereview.chromium.org/1154373005/diff/20001/LayoutTests/WPT_DISABLED
File LayoutTests/WPT_DISABLED (right):

https://codereview.chromium.org/1154373005/diff/20001/LayoutTests/WPT_DISABLE...
LayoutTests/WPT_DISABLED:1: Existance of this file (WPT_DISABLED) is turning off
the WPTServe feature for Blink Layout tests.
On 2015/06/05 19:07:45, jsbell wrote:
> This file can be deleted from the CL now.

Done.

https://codereview.chromium.org/1154373005/diff/20001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right):

https://codereview.chromium.org/1154373005/diff/20001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:365: 
On 2015/06/05 18:27:48, Dirk Pranke wrote:
> nit: you only need one blank line between class methods.

Done.

https://codereview.chromium.org/1154373005/diff/20001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/port/base.py (right):

https://codereview.chromium.org/1154373005/diff/20001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/port/base.py:1170: self._is_wpt_enabled =
is_wpt_enabled
On 2015/06/05 18:27:48, Dirk Pranke wrote:
> It doesn't seem like you need this, since it is set in the constructor from
the
> options object?

That's right. I wasn't sure if I would be able to pass through options at the
point when I was adding the modifier. So it remained here since I didn't
properly clean up. :-/

Removed.

https://codereview.chromium.org/1154373005/diff/20001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right):

https://codereview.chromium.org/1154373005/diff/20001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:188: help="Enable
running web-plaform-tests using WPTserve instead of Apache."),
On 2015/06/05 19:07:45, jsbell wrote:
> nit: indenting here is inconsistent

Pre-submit checks gave me a really hard time with this one. 
I'll ignore them then.

Done.

https://codereview.chromium.org/1154373005/diff/20001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/servers/wpt_support/WPTSubmodules
(right):

https://codereview.chromium.org/1154373005/diff/20001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/servers/wpt_support/WPTSubmodules:6: ./tools
html5lib 7cce65bbaa78411f98b8b37eeefc9db03c580097
On 2015/06/05 18:27:49, Dirk Pranke wrote:
> Where does this list come from?
> 
> We have pywebsocket already in src/third_party
> 
> I don't see any reference to six or html5lib in wptserve, and I don't know
what
> testdata is?
> 
> wptserve is tiny, so I would just check include it directly in this patch
under
> webkitpy/third_party, and I think you can skip this whole checkout step?
> 
> (apart from maybe still wanting the full wpt repo, of course).

I am actually checking out web-platform-tests and removing unneeded submodules
and other files (tests). I do this because wptserve is very basic, however
./serve is already starting up all servers (HTTP/S + WS/S).

The list is coming from my inspection of the repo HEADs which I assume to be
good. My intention was to allow gradual rolling of new stuff in WPT to prevent
breaking changes with this list, however this would have to be coordinated with
our mirror repos as well. I'll dive in more into DEPS approach as I progress.

Yes, I am aware that the pywebsocket module is redundant (as noted in the design
doc).

I'll have to recheck why I included testdata, I think it's not required to
remain here.

[jsbell, 2015-06-08 21:06:11]

> The process should be started and killed along with spawned children
> (inter-operable).
> Any suggestion on how to do it with the "host" class?

executive.py's kill_process() ?

* win32 - we appear to send /t ("tree kill") already
* posix - negating the pid appears to be the convention to kill the process
group; at least for me; changing the line to os.kill(-pid, signal.SIGKILL)
appears to do the right thing; the pid passed to waitpid() should be negated to.

The above is my naive first reaction reading docs/man pages; don't assume I know
what I'm talking about, though.

[jsbell, 2015-06-08 21:11:29]

On 2015/06/08 12:28:12, burnik wrote:
> I would be more comfortable to have it
> checked in as part of the CL or expand ./serve to accept command line flags
for 
> the entire config granularly (or by reading JSON from stdin).
> As I have already landed the support to define doc_root and ws_doc_root
through
> command line,
> https://github.com/w3c/wpt-tools/pull/9
> 
> it would be a plausible approach.
> 
> What do you think?

Specifying config on the command line (rather than a generated file) sgtm.

Agreed that no generated temp files and thus no new .gitignore entries is ideal.

[jsbell, 2015-06-08 21:29:04]

On 2015/06/08 12:28:12, burnik wrote:
> On 2015/06/05 20:34:25, jsbell wrote:
> > Um... yay for more copies?
> 
> Regarding resources, this can all be fixed, ideally (in the future) in the
> following manner:
> - do not convert tests when importing to imported/web-platform/tests
(optional)

^^ Not an option until we enable wptserve by default, is it?

> - keep ./resources directory as is for the most part in
> $WEBKIT/LayoutTests/imported/web-platform-tests/

^^ I'm not sure what this means here?

> Or we can do some rewriting in the config.json of WPT. 

^^ That sounds ideal... but can the current code reach out of the root dir via
"rewrites"? (i.e. is it just a URL rewrite, or can it do local path mapping)

> In any case, 
> I will tend not to have duplicates lying everywhere around before we decide to
> land this.

If we get blocked on e.g. upstreaming fixes to wpt, I would not object to
duplicates. (We already have 3 copies of most of these - see crbug.com/362788 -
a fourth will be annoying but enabling wptserve is worth it and we'll solve it
eventually)

[Dirk Pranke, 2015-06-09 01:21:33]

The python code looks good to me to land-and-iterate, assuming the name change I
suggest below.

I still want to understand exactly which submodules we really need in order to
run things, and why, and whether we can just check wptserve in directly before
I'm happy w/ the patch as a whole.

https://codereview.chromium.org/1154373005/diff/40001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/servers/wptserve_http.py (right):

https://codereview.chromium.org/1154373005/diff/40001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/servers/wptserve_http.py:1: # Copyright 2015
The Chromium Authors. All rights reserved.
We should probably just call this 'wptserve.py'. The reason apache_http.py is
apache_http.py is because the actual name of the product is the "apache http
server".

[burnik, 2015-06-09 18:31:12]

On 2015/06/08 21:06:11, jsbell wrote:
> > The process should be started and killed along with spawned children
> > (inter-operable).
> > Any suggestion on how to do it with the "host" class?
> 
> executive.py's kill_process() ?
> 
> * win32 - we appear to send /t ("tree kill") already
> * posix - negating the pid appears to be the convention to kill the process
> group; at least for me; changing the line to os.kill(-pid, signal.SIGKILL)
> appears to do the right thing; the pid passed to waitpid() should be negated
to.
> 
> The above is my naive first reaction reading docs/man pages; don't assume I
know
> what I'm talking about, though.

Ok. This does seem to work well. At least we're good on posix. :-)

[burnik, 2015-06-09 18:32:31]

On 2015/06/08 21:11:29, jsbell wrote:
> On 2015/06/08 12:28:12, burnik wrote:
> > I would be more comfortable to have it
> > checked in as part of the CL or expand ./serve to accept command line flags
> for 
> > the entire config granularly (or by reading JSON from stdin).
> > As I have already landed the support to define doc_root and ws_doc_root
> through
> > command line,
> > https://github.com/w3c/wpt-tools/pull/9
> > 
> > it would be a plausible approach.
> > 
> > What do you think?
> 
> Specifying config on the command line (rather than a generated file) sgtm.
> 
> Agreed that no generated temp files and thus no new .gitignore entries is
ideal.

I've factored out the config to a file I'm checking in to this CL.
We can reconsider updating wptserve to support my suggested alternatives.

[burnik, 2015-06-09 18:37:29]

On 2015/06/08 21:29:04, jsbell wrote:
> On 2015/06/08 12:28:12, burnik wrote:
> > On 2015/06/05 20:34:25, jsbell wrote:
> > > Um... yay for more copies?
> > 
> > Regarding resources, this can all be fixed, ideally (in the future) in the
> > following manner:
> > - do not convert tests when importing to imported/web-platform/tests
> (optional)
> 
> ^^ Not an option until we enable wptserve by default, is it?
> 
> > - keep ./resources directory as is for the most part in
> > $WEBKIT/LayoutTests/imported/web-platform-tests/
> 
> ^^ I'm not sure what this means here?

I mean keeping all of the resources in
$WEBKIT/LayoutTests/imported/web-platform-tests/
as well as $WEBKIT/LayoutTests/http/tests/w3c/resources/

We could also probably fix this by adding a patch into wptserve. I'll think
about it more.

> 
> > Or we can do some rewriting in the config.json of WPT. 
> 
> ^^ That sounds ideal... but can the current code reach out of the root dir via
> "rewrites"? (i.e. is it just a URL rewrite, or can it do local path mapping)
> 
> > In any case, 
> > I will tend not to have duplicates lying everywhere around before we decide
to
> > land this.
> 
> If we get blocked on e.g. upstreaming fixes to wpt, I would not object to
> duplicates. (We already have 3 copies of most of these - see crbug.com/362788
-
> a fourth will be annoying but enabling wptserve is worth it and we'll solve it
> eventually)

Haven't tried to play around with rewrites yet, in any case, this duplication
should be documented.

[burnik, 2015-06-09 18:39:34]

On 2015/06/09 01:21:33, Dirk Pranke wrote:
> The python code looks good to me to land-and-iterate, assuming the name change
I
> suggest below.
> 
> I still want to understand exactly which submodules we really need in order to
> run things, and why, and whether we can just check wptserve in directly before
> I'm happy w/ the patch as a whole.
> 
>
https://codereview.chromium.org/1154373005/diff/40001/Tools/Scripts/webkitpy/...
> File Tools/Scripts/webkitpy/layout_tests/servers/wptserve_http.py (right):
> 
>
https://codereview.chromium.org/1154373005/diff/40001/Tools/Scripts/webkitpy/...
> Tools/Scripts/webkitpy/layout_tests/servers/wptserve_http.py:1: # Copyright
2015
> The Chromium Authors. All rights reserved.
> We should probably just call this 'wptserve.py'. The reason apache_http.py is
> apache_http.py is because the actual name of the product is the "apache http
> server".

I will pick out the wpt submodules very soon and make sure to reduce it to the
minimum, 
that's why I'm uncomfortable checking in the whole wpt code into this CL.

[burnik, 2015-06-09 18:54:44]

A bit of clean up and comments addressed. 

Also, updated design doc. 
https://goo.gl/iXUaZd

https://codereview.chromium.org/1154373005/diff/40001/Tools/Scripts/webkitpy/...
File Tools/Scripts/webkitpy/layout_tests/servers/wptserve_http.py (right):

https://codereview.chromium.org/1154373005/diff/40001/Tools/Scripts/webkitpy/...
Tools/Scripts/webkitpy/layout_tests/servers/wptserve_http.py:1: # Copyright 2015
The Chromium Authors. All rights reserved.
On 2015/06/09 01:21:33, Dirk Pranke wrote:
> We should probably just call this 'wptserve.py'. The reason apache_http.py is
> apache_http.py is because the actual name of the product is the "apache http
> server".
> 

Done.

[Dirk Pranke, 2015-06-09 19:06:06]

On 2015/06/09 18:31:12, burnik wrote:
> On 2015/06/08 21:06:11, jsbell wrote:
> > > The process should be started and killed along with spawned children
> > > (inter-operable).
> > > Any suggestion on how to do it with the "host" class?
> > 
> > executive.py's kill_process() ?
> > 
> > * win32 - we appear to send /t ("tree kill") already
> > * posix - negating the pid appears to be the convention to kill the process
> > group; at least for me; changing the line to os.kill(-pid, signal.SIGKILL)
> > appears to do the right thing; the pid passed to waitpid() should be negated
> to.
> > 
> > The above is my naive first reaction reading docs/man pages; don't assume I
> know
> > what I'm talking about, though.
> 
> Ok. This does seem to work well. At least we're good on posix. :-)

I'm not entirely comfortable with this. kill_process() != kill_process_group(),
though in most cases
in webkitpy it's probably safe to use these interchangably.

More importantly, though, kill_process() is kind of a mechanism of last resort,
and should not be
first choice for an orderly shutdown of a process. It sounds like wptserve is
expecting a SIGINT,
so we should probably modify the wptserve.py's stop() call to send that,
instead, and then wait
before giving up and aborting the process (or process group).

[burnik, 2015-06-09 19:37:09]

On 2015/06/09 19:06:06, Dirk Pranke wrote:
> On 2015/06/09 18:31:12, burnik wrote:
> > On 2015/06/08 21:06:11, jsbell wrote:
> > > > The process should be started and killed along with spawned children
> > > > (inter-operable).
> > > > Any suggestion on how to do it with the "host" class?
> > > 
> > > executive.py's kill_process() ?
> > > 
> > > * win32 - we appear to send /t ("tree kill") already
> > > * posix - negating the pid appears to be the convention to kill the
process
> > > group; at least for me; changing the line to os.kill(-pid, signal.SIGKILL)
> > > appears to do the right thing; the pid passed to waitpid() should be
negated
> > to.
> > > 
> > > The above is my naive first reaction reading docs/man pages; don't assume
I
> > know
> > > what I'm talking about, though.
> > 
> > Ok. This does seem to work well. At least we're good on posix. :-)
> 
> I'm not entirely comfortable with this. kill_process() !=
kill_process_group(),
> though in most cases
> in webkitpy it's probably safe to use these interchangably.
> 
> More importantly, though, kill_process() is kind of a mechanism of last
resort,
> and should not be
> first choice for an orderly shutdown of a process. It sounds like wptserve is
> expecting a SIGINT,
> so we should probably modify the wptserve.py's stop() call to send that,
> instead, and then wait
> before giving up and aborting the process (or process group).

Do you suggest I use:

self._executive.interrupt(self._pid)

instead?

This does work, but the process is defunct until run-webkit-tests finishes.
Is that a desired behavior?

And about waiting, is this what you had in mind:

if not self._wait_for_action(lambda: not
self._executive.check_running_pid(self._pid)):
  self._executive.kill_process(-self._pid)

Since it's defunct, this will always time out after 20 secs and then resort to
killing.

[burnik, 2015-06-09 19:41:45]

On 2015/06/09 19:37:09, burnik wrote:
> On 2015/06/09 19:06:06, Dirk Pranke wrote:
> > On 2015/06/09 18:31:12, burnik wrote:
> > > On 2015/06/08 21:06:11, jsbell wrote:
> > > > > The process should be started and killed along with spawned children
> > > > > (inter-operable).
> > > > > Any suggestion on how to do it with the "host" class?
> > > > 
> > > > executive.py's kill_process() ?
> > > > 
> > > > * win32 - we appear to send /t ("tree kill") already
> > > > * posix - negating the pid appears to be the convention to kill the
> process
> > > > group; at least for me; changing the line to os.kill(-pid,
signal.SIGKILL)
> > > > appears to do the right thing; the pid passed to waitpid() should be
> negated
> > > to.
> > > > 
> > > > The above is my naive first reaction reading docs/man pages; don't
assume
> I
> > > know
> > > > what I'm talking about, though.
> > > 
> > > Ok. This does seem to work well. At least we're good on posix. :-)
> > 
> > I'm not entirely comfortable with this. kill_process() !=
> kill_process_group(),
> > though in most cases
> > in webkitpy it's probably safe to use these interchangably.
> > 
> > More importantly, though, kill_process() is kind of a mechanism of last
> resort,
> > and should not be
> > first choice for an orderly shutdown of a process. It sounds like wptserve
is
> > expecting a SIGINT,
> > so we should probably modify the wptserve.py's stop() call to send that,
> > instead, and then wait
> > before giving up and aborting the process (or process group).
> 
> Do you suggest I use:
> 
> self._executive.interrupt(self._pid)
> 
> instead?
> 
> This does work, but the process is defunct until run-webkit-tests finishes.
> Is that a desired behavior?
> 
> And about waiting, is this what you had in mind:
> 
> if not self._wait_for_action(lambda: not
> self._executive.check_running_pid(self._pid)):
>   self._executive.kill_process(-self._pid)
> 
> Since it's defunct, this will always time out after 20 secs and then resort to
> killing.

Or did you mean just:

self._process.wait() 

?

[Dirk Pranke, 2015-06-09 19:48:53]

On 2015/06/09 19:37:09, burnik wrote:
> On 2015/06/09 19:06:06, Dirk Pranke wrote:
> > On 2015/06/09 18:31:12, burnik wrote:
> > > On 2015/06/08 21:06:11, jsbell wrote:
> > > > > The process should be started and killed along with spawned children
> > > > > (inter-operable).
> > > > > Any suggestion on how to do it with the "host" class?
> > > > 
> > > > executive.py's kill_process() ?
> > > > 
> > > > * win32 - we appear to send /t ("tree kill") already
> > > > * posix - negating the pid appears to be the convention to kill the
> process
> > > > group; at least for me; changing the line to os.kill(-pid,
signal.SIGKILL)
> > > > appears to do the right thing; the pid passed to waitpid() should be
> negated
> > > to.
> > > > 
> > > > The above is my naive first reaction reading docs/man pages; don't
assume
> I
> > > know
> > > > what I'm talking about, though.
> > > 
> > > Ok. This does seem to work well. At least we're good on posix. :-)
> > 
> > I'm not entirely comfortable with this. kill_process() !=
> kill_process_group(),
> > though in most cases
> > in webkitpy it's probably safe to use these interchangably.
> > 
> > More importantly, though, kill_process() is kind of a mechanism of last
> resort,
> > and should not be
> > first choice for an orderly shutdown of a process. It sounds like wptserve
is
> > expecting a SIGINT,
> > so we should probably modify the wptserve.py's stop() call to send that,
> > instead, and then wait
> > before giving up and aborting the process (or process group).
> 
> Do you suggest I use:
> 
> self._executive.interrupt(self._pid)
> 
> instead?

Hm. I guess I forgot that method existed :). 

What I was suggesting is that we should do whatever wptserve wants us to do 
in order to do an orderly shutdown (e.g., in apache's case, we do `apache2 -k
stop`).

If wptserve's definition of an orderly shutdown is SIGINT, then we should do
SIGINT.
If it's definition is "we don't have an orderly shutdown", we should file an 
enhancement request to fix that.

Does that make sense?

> 
> This does work, but the process is defunct until run-webkit-tests finishes.
> Is that a desired behavior?
>

To me, that sounds like wptserve won't do the right thing then.
 
> And about waiting, is this what you had in mind:
> 
> if not self._wait_for_action(lambda: not
> self._executive.check_running_pid(self._pid)):
>   self._executive.kill_process(-self._pid)
> 
> Since it's defunct, this will always time out after 20 secs and then resort to
> killing.

Yup, that's more-or-less what I had in mind, but if it doesn't work (i.e., we
have
to kill regardless), then we should just kill and add a FIXME: to do something
cleaner
when it is available.

If you need to kill the process group, I'm fine w/ changing kill_process() to do
that
(since, as you say, we already do that on win, and as I noted earlier, this is
probably
safe regardless), but please update the comments and docstring to be clear that
that's what the function does.

[burnik, 2015-06-09 20:05:31]

On 2015/06/09 19:48:53, Dirk Pranke wrote:
> On 2015/06/09 19:37:09, burnik wrote:
> > On 2015/06/09 19:06:06, Dirk Pranke wrote:
> > > On 2015/06/09 18:31:12, burnik wrote:
> > > > On 2015/06/08 21:06:11, jsbell wrote:
> > > > > > The process should be started and killed along with spawned children
> > > > > > (inter-operable).
> > > > > > Any suggestion on how to do it with the "host" class?
> > > > > 
> > > > > executive.py's kill_process() ?
> > > > > 
> > > > > * win32 - we appear to send /t ("tree kill") already
> > > > > * posix - negating the pid appears to be the convention to kill the
> > process
> > > > > group; at least for me; changing the line to os.kill(-pid,
> signal.SIGKILL)
> > > > > appears to do the right thing; the pid passed to waitpid() should be
> > negated
> > > > to.
> > > > > 
> > > > > The above is my naive first reaction reading docs/man pages; don't
> assume
> > I
> > > > know
> > > > > what I'm talking about, though.
> > > > 
> > > > Ok. This does seem to work well. At least we're good on posix. :-)
> > > 
> > > I'm not entirely comfortable with this. kill_process() !=
> > kill_process_group(),
> > > though in most cases
> > > in webkitpy it's probably safe to use these interchangably.
> > > 
> > > More importantly, though, kill_process() is kind of a mechanism of last
> > resort,
> > > and should not be
> > > first choice for an orderly shutdown of a process. It sounds like wptserve
> is
> > > expecting a SIGINT,
> > > so we should probably modify the wptserve.py's stop() call to send that,
> > > instead, and then wait
> > > before giving up and aborting the process (or process group).
> > 
> > Do you suggest I use:
> > 
> > self._executive.interrupt(self._pid)
> > 
> > instead?
> 
> Hm. I guess I forgot that method existed :). 
> 
> What I was suggesting is that we should do whatever wptserve wants us to do 
> in order to do an orderly shutdown (e.g., in apache's case, we do `apache2 -k
> stop`).
> 
> If wptserve's definition of an orderly shutdown is SIGINT, then we should do
> SIGINT.
> If it's definition is "we don't have an orderly shutdown", we should file an 
> enhancement request to fix that.
> 
> Does that make sense?
> 
> > 
> > This does work, but the process is defunct until run-webkit-tests finishes.
> > Is that a desired behavior?
> >
> 
> To me, that sounds like wptserve won't do the right thing then.
>  
> > And about waiting, is this what you had in mind:
> > 
> > if not self._wait_for_action(lambda: not
> > self._executive.check_running_pid(self._pid)):
> >   self._executive.kill_process(-self._pid)
> > 
> > Since it's defunct, this will always time out after 20 secs and then resort
to
> > killing.
> 
> Yup, that's more-or-less what I had in mind, but if it doesn't work (i.e., we
> have
> to kill regardless), then we should just kill and add a FIXME: to do something
> cleaner
> when it is available.
> 
> If you need to kill the process group, I'm fine w/ changing kill_process() to
do
> that
> (since, as you say, we already do that on win, and as I noted earlier, this is
> probably
> safe regardless), but please update the comments and docstring to be clear
that
> that's what the function does.

I have updated the CL to signal the process with SIGINT and wait.
As far as I've tested, this works as expected. 

I was also considering your suggestion to have a diffrent signaling
mechanism to gracefully stop wpt serve.

Added a TODO note.

[burnik, 2015-06-11 18:31:24]

I've checked in the WPTServe files. I belive this is the bare minimum at this
point (at least for the server to be able to run). 

Please take a look.

[Dirk Pranke, 2015-06-11 23:17:01]

Getting closer still ...

Any third party code should be checked in under
Tools/Scripts/webkitpy/thirdparty ; we need to keep the number of places in the
tree where there is third party code to a minimum, because any place we do this
needs legal approval (which is usually straightforward, and I will help take
care of this when we're ready to land things).

Also, update webkitpy/thirdparty/README.chromium to describe how your directory
was derived: where the code came from, what the license file is, whether you had
to locally modify things or not; it should be fairly obvious from the existing
file what I'm looking for. It needs to be easy for any developer in the future
to know how to recreate what you've done: doe we still need the checkout script,
what does the whitelist file do, etc.

I actually tried (briefly) to run this locally as-is, and it didn't work;
wptserve failed to start up because it couldn't import mod_pywebsocket. You'll
probably need to modify your wptserve.py start() code to adjust the PYTHONPATH
that is passed to the subprocess, and you should verify that when you are
testing locally that you don't have stuff in your PYTHONPATH that might be
causing issues.

https://codereview.chromium.org/1154373005/diff/120001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right):

https://codereview.chromium.org/1154373005/diff/120001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:100: return False
I'm confused, when would a test be both an http test and a wpt_test ?

[burnik, 2015-06-12 01:38:38]

On 2015/06/11 23:17:01, Dirk Pranke wrote:
> Getting closer still ...
> 
> Any third party code should be checked in under
> Tools/Scripts/webkitpy/thirdparty ; we need to keep the number of places in
the
> tree where there is third party code to a minimum, because any place we do
this
> needs legal approval (which is usually straightforward, and I will help take
> care of this when we're ready to land things).
> 
> Also, update webkitpy/thirdparty/README.chromium to describe how your
directory
> was derived: where the code came from, what the license file is, whether you
had
> to locally modify things or not; it should be fairly obvious from the existing
> file what I'm looking for. It needs to be easy for any developer in the future
> to know how to recreate what you've done: doe we still need the checkout
script,
> what does the whitelist file do, etc.
> 
> I actually tried (briefly) to run this locally as-is, and it didn't work;
> wptserve failed to start up because it couldn't import mod_pywebsocket. You'll
> probably need to modify your wptserve.py start() code to adjust the PYTHONPATH
> that is passed to the subprocess, and you should verify that when you are
> testing locally that you don't have stuff in your PYTHONPATH that might be
> causing issues.
> 
>
https://codereview.chromium.org/1154373005/diff/120001/Tools/Scripts/webkitpy...
> File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right):
> 
>
https://codereview.chromium.org/1154373005/diff/120001/Tools/Scripts/webkitpy...
> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:100: return False
> I'm confused, when would a test be both an http test and a wpt_test ?

Thanks for the guidelines. I'll try to patch this up today. :-)

It's interesting that you have the path issue preventing it to run, 
since I inject the path to mod_pywebsocket into the code where it is used.
In any case, I think it's a minor issue I'll address.

I hope there won't be legal issues over this, I'll recheck if all LICENSE files
were indeed included. So far some W3 contributors were very supportive of this
effort. :-)
I suppose reducing the amount of checked out code shouldn't be a problem in that
sense either?

I was considering transforming checkout.sh into checkout.py for interoperability
at some point.

[burnik, 2015-06-12 01:40:54]

Some details regarding comments on "is_http_test" and the path issue.

https://codereview.chromium.org/1154373005/diff/120001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right):

https://codereview.chromium.org/1154373005/diff/120001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:100: return False
On 2015/06/11 23:17:01, Dirk Pranke wrote:
> I'm confused, when would a test be both an http test and a wpt_test ? 

Whenever a test path matches *http/

e.g.
imported/web-platform-tests/referrer-policy/no-referrer/http-csp/cross-origin/http-http/script-tag/generic.no-redirect.http.html

This prevents from spinning up Apache alongside WPTServe when the list of tests
to run is only under imported/web-platform-tests.

https://codereview.chromium.org/1154373005/diff/140001/Tools/Scripts/webkitpy...
File
Tools/Scripts/webkitpy/layout_tests/servers/wpt_support/wpt/tools/serve/serve.py
(right):

https://codereview.chromium.org/1154373005/diff/140001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/layout_tests/servers/wpt_support/wpt/tools/serve/serve.py:21:
sys.path.insert(0, os.path.abspath(os.path.join('..', '..', '..', '..', '..',
'webkitpy', 'thirdparty')))
Hm, so this injected line didn't help fix the mod_pywebsocket?
Interesting...

It's either because of the relative path or something other in your env... I
guess...

[Dirk Pranke, 2015-06-12 02:47:40]

On 2015/06/12 01:40:54, burnik wrote:
> Some details regarding comments on "is_http_test" and the path issue.
> 
>
https://codereview.chromium.org/1154373005/diff/120001/Tools/Scripts/webkitpy...
> File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right):
> 
>
https://codereview.chromium.org/1154373005/diff/120001/Tools/Scripts/webkitpy...
> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:100: return False
> On 2015/06/11 23:17:01, Dirk Pranke wrote:
> > I'm confused, when would a test be both an http test and a wpt_test ? 
> 
> Whenever a test path matches *http/
> 
> e.g.
>
imported/web-platform-tests/referrer-policy/no-referrer/http-csp/cross-origin/http-http/script-tag/generic.no-redirect.http.html
>

Ah. In that case, the is_http_test() thing is wrong. We should only consider
something an http test
if the test name startswith 'http'. Please fix that in this CL so that we don't
get this weird collision.
 
> 
>
https://codereview.chromium.org/1154373005/diff/140001/Tools/Scripts/webkitpy...
>
Tools/Scripts/webkitpy/layout_tests/servers/wpt_support/wpt/tools/serve/serve.py:21:
> sys.path.insert(0, os.path.abspath(os.path.join('..', '..', '..', '..', '..',
> 'webkitpy', 'thirdparty')))
> Hm, so this injected line didn't help fix the mod_pywebsocket?
> Interesting...
> 
> It's either because of the relative path or something other in your env... I
> guess...

Dunno, I can dig into it further if you're stumped.

[Dirk Pranke, 2015-06-12 02:48:44]

On 2015/06/12 01:38:38, burnik wrote:
> I hope there won't be legal issues over this, I'll recheck if all LICENSE
files
> were indeed included. So far some W3 contributors were very supportive of this
> effort. :-)
> I suppose reducing the amount of checked out code shouldn't be a problem in
that
> sense either?

I really don't expect there to be any issues; the files are supposedly
dual-licensed but one
of the two licenses is 3-clause BSD which should be fine. But I'm not a lawyer
:).

[burnik, 2015-06-12 12:54:04]

Patchset #10 (id:180001) has been deleted

[burnik, 2015-06-12 12:57:20]

* Fixed is_http_test resolving
* Fixed PYTHONPATH issue
* Moved WPT to thirdparty
* Updated thirdparty/README.chromium
* Added thirdparty/wpt/README.chromium with detailed instructions

[burnik, 2015-06-12 13:01:27]

burnik@chromium.org changed reviewers:
+ jochen@chromium.org, mkwst@chromium.org

[burnik, 2015-06-12 13:01:28]

+ mkwst, +jochen

[jsbell, 2015-06-12 17:13:17]

doc nits

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/thirdparty/README.chromium (right):

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/README.chromium:74: Version: ?
Should we use the (short) hash here?

... or reference wpt/WPTHeads for the hashes, I suppose.

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium (right):

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium:5: This directory contains
checked out and reduced code from web-platform-tests
We should get jgraham (W3C) to review this doc at some point. He may be able to
point out assumptions we're making about wpt that won't hold long term. (not a
blocker for ship, though)

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium:23: and perform a fresh
checkout of the web-platform-tests code. When rolling in new
Can you rewrite this more imperatively, i.e. keep short descriptions of the
files here, then have a section of the doc: "Steps to update wpt"

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium:63: (which gets generated
on the first run) and Web Platform Tests Documentation.
Include URL of the WPT docs?

[burnik, 2015-06-12 17:23:55]

Thanks for nits. I'll make sure to update this.
I'll be iterating a bit faster as I'm eager to see this work with all of blink
build-test workflows. :-)

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium (right):

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium:5: This directory contains
checked out and reduced code from web-platform-tests
On 2015/06/12 17:13:17, jsbell wrote:
> We should get jgraham (W3C) to review this doc at some point. He may be able
to
> point out assumptions we're making about wpt that won't hold long term. (not a
> blocker for ship, though)

Agreed. That's why I explicitly state the HEADs at WPTHeads to make the point.
:-)
I'll ping him to give some more insight.

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium:63: (which gets generated
on the first run) and Web Platform Tests Documentation.
On 2015/06/12 17:13:17, jsbell wrote:
> Include URL of the WPT docs?

When I find them, I'll make sure to add. So far, number of docs referring to SSL
in WPT: 
http://www.quickmeme.com/img/a5/a5482d8548fd094a6968efb4659393dfea89edeadeed8...

[Dirk Pranke, 2015-06-12 22:25:02]

Okay, after applying the patch locally and hacking on it a bit, I was able to
run things mostly successfully.

At r196958 (a checkout from yesterday), most of the tests passed, but I got ~80
failures under html/ with exceptions about cross origin reads being blocked. 

Do you see that?

Also, in order to run anything, I had to add a bunch of dummy entries for
*web-platform.test domains to /etc/hosts.

Hopefully there are flags we can pass to wptserve that make this not necessary?
We can't require devs to have to modify /etc/hosts in order to run the tests,
and we don't want to have to do this on the bots either.

(We might need to work out some scheme for figuring out how to map domain names
back and forth to ports between wptserve and content_shell in order for this to
work).

Apart from that, we continue to get closer. I took a closer look at the
servers/wptserve.py code and we can simplify that a lot, I think. Comments on
specify parts of the patch follow.

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/common/system/executive.py (right):

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/common/system/executive.py:60: self._env[self._var] =
self._pathsep.join(self)
These changes are not necessary, and we should never modify os.environ directly
(which is what this does). More comments about this elsewhere ...

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/common/system/executive.py:106: pythonpath =
EnvPath(os.environ, "PYTHONPATH", os.pathsep)
the pythonpath change isn't necessary.

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py (right):

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py:37:
self._executive.pythonpath.update()
You don't need this.

If you set 

self._env = {'PYTHONPATH': self._path_to_thirdparty} then
you can pass env=self._env in the popen() call.

In addition, if you set self._stdout = self._stderr = self._executive.DEVNULL, 
you should be able to use ServerBases's implementation of _spawn_process()
directly
and not need to override it.

You might need to add a way to specify the cwd, but it seems like that's a bug
in wptserve if it needs to be started from a particular directory.

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py:39:
self._path_to_wpt_root = os.path.join(path_to_wpt_support, 'wpt')
Use self._filesystemm.join() instead of os.path.join(), here and elsewhere. 

Modules outside of webkitpy.common.system should generally not need to 
import sys or os.

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py:45: start_cmd =
[self._executable,
This should be 

    start_cmd = [self.port_obj.host.executable, self._executable, ...]

and you should rename self.executable to something else.

You don't want to run the python script directly; that won't work on Windows.

Also, if you make the change I suggest you don't need to call popen(...,
shell=True).

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py:55: def
_spawn_process(self):
As noted above, let's see if we can make the base class impl work for you.

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py:62:
preexec_fn=os.setsid)
AFAICT, you don't need (and don't want) os.setsid to be called.

Why did you think you needed this?

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py:70:
self._stop_running_server()
I don't think you need to override the base class here.

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py:82:
self._executive.interrupt(self._pid)
We might need to add another hook to ServerBase so that you can override it
to call executive.interrupt() + self._process.wait() instead of 
executive.kill_process().

I think you can get rid of the rest of the code in this method and 
just use the base class impl otherwise.

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium (right):

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium:18: changing the ports
(HTTP/S, WS/S), make sure to update the python code too.
This is actually a file you wrote, right? Can we move it out of thirdparty
somewhere?

Also, do we really need both config files? Where does the
wpt/config.default.json file
come from? I don't see it on github, but maybe I'm not looking in the right
place?

We should try to avoid putting files we write under thirdparty/ dirs.

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium:26: WPTWhiteList from the
checked out directory.
From what I can tell, we only actually need 

the serve/ and maybe sslutils directories from

https://github.com/w3c/wpt-tools

and then the wptserve directory from

https://github.com/w3c/wptserve

Can we check those into 

thirdparty/wpt_tools/{serve,sslutils}
thirdparty/wptserve/

and avoid a bunch of nested dirs?

Alternatively, we can consider just cloning the repos and pulling them via DEPS.

I'd like to be able to skip and/or simplify as many of these steps as possible
...

[jsbell, 2015-06-12 22:36:00]

On 2015/06/12 22:25:02, Dirk Pranke wrote:
> Also, in order to run anything, I had to add a bunch of dummy entries for
> *web-platform.test domains to /etc/hosts.

The ones documented at https://github.com/w3c/web-platform-tests ?

If you used a subset, that could explain the failures?

> We can't require devs to have to modify /etc/hosts in order to run the tests,
> and we don't want to have to do this on the bots either.

+1
 
> (We might need to work out some scheme for figuring out how to map domain
names
> back and forth to ports between wptserve and content_shell in order for this
to
> work).

I was assuming we'd do some modifications to make content_shell do the name
mapping; someone on blink-dev@ said we already do this for some domain names
e.g. .test ?

[Dirk Pranke, 2015-06-12 22:43:20]

On 2015/06/12 22:36:00, jsbell wrote:
> On 2015/06/12 22:25:02, Dirk Pranke wrote:
> > Also, in order to run anything, I had to add a bunch of dummy entries for
> > *web-platform.test domains to /etc/hosts.
> 
> The ones documented at https://github.com/w3c/web-platform-tests ?
> 
> If you used a subset, that could explain the failures?
> 

I believe I used the full list. As an aside, what's up w/ the random-looking
host
names? I'm guess that's a hash of something or other?

> > We can't require devs to have to modify /etc/hosts in order to run the
tests,
> > and we don't want to have to do this on the bots either.
> 
> +1
>  
> > (We might need to work out some scheme for figuring out how to map domain
> names
> > back and forth to ports between wptserve and content_shell in order for this
> to
> > work).
> 
> I was assuming we'd do some modifications to make content_shell do the name
> mapping; someone on blink-dev@ said we already do this for some domain names
> e.g. .test ?

Yes, we do.

[jsbell, 2015-06-13 00:08:57]

On 2015/06/12 22:43:20, Dirk Pranke wrote:
> As an aside, what's up w/ the random-looking host
> names? I'm guess that's a hash of something or other?

Non-ASCII domain names encoded with Punycode:

https://en.wikipedia.org/wiki/Internationalized_domain_name#ToASCII_and_ToUni...

[burnik, 2015-06-15 10:30:31]

On 2015/06/12 22:25:02, Dirk Pranke wrote:
> Okay, after applying the patch locally and hacking on it a bit, I was able to
> run things mostly successfully.
> 
> At r196958 (a checkout from yesterday), most of the tests passed, but I got
~80
> failures under html/ with exceptions about cross origin reads being blocked. 
> 
> Do you see that?
> 
> Also, in order to run anything, I had to add a bunch of dummy entries for
> *web-platform.test domains to /etc/hosts.
> 
> Hopefully there are flags we can pass to wptserve that make this not
necessary?
> We can't require devs to have to modify /etc/hosts in order to run the tests,
> and we don't want to have to do this on the bots either.
> 
> (We might need to work out some scheme for figuring out how to map domain
names
> back and forth to ports between wptserve and content_shell in order for this
to
> work).
> 
> Apart from that, we continue to get closer. I took a closer look at the
> servers/wptserve.py code and we can simplify that a lot, I think. Comments on
> specify parts of the patch follow.
> 
>
https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
> File Tools/Scripts/webkitpy/common/system/executive.py (right):
> 
>
https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
> Tools/Scripts/webkitpy/common/system/executive.py:60: self._env[self._var] =
> self._pathsep.join(self)
> These changes are not necessary, and we should never modify os.environ
directly
> (which is what this does). More comments about this elsewhere ...
> 
>
https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
> Tools/Scripts/webkitpy/common/system/executive.py:106: pythonpath =
> EnvPath(os.environ, "PYTHONPATH", os.pathsep)
> the pythonpath change isn't necessary.
> 
>
https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
> File Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py (right):
> 
>
https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
> Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py:37:
> self._executive.pythonpath.update()
> You don't need this.
> 
> If you set 
> 
> self._env = {'PYTHONPATH': self._path_to_thirdparty} then
> you can pass env=self._env in the popen() call.
> 
> In addition, if you set self._stdout = self._stderr = self._executive.DEVNULL,

> you should be able to use ServerBases's implementation of _spawn_process()
> directly
> and not need to override it.
> 
> You might need to add a way to specify the cwd, but it seems like that's a bug
> in wptserve if it needs to be started from a particular directory.
> 
>
https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
> Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py:39:
> self._path_to_wpt_root = os.path.join(path_to_wpt_support, 'wpt')
> Use self._filesystemm.join() instead of os.path.join(), here and elsewhere. 
> 
> Modules outside of webkitpy.common.system should generally not need to 
> import sys or os.
> 
>
https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
> Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py:45: start_cmd =
> [self._executable,
> This should be 
> 
>     start_cmd = [self.port_obj.host.executable, self._executable, ...]
> 
> and you should rename self.executable to something else.
> 
> You don't want to run the python script directly; that won't work on Windows.
> 
> Also, if you make the change I suggest you don't need to call popen(...,
> shell=True).
> 
>
https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
> Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py:55: def
> _spawn_process(self):
> As noted above, let's see if we can make the base class impl work for you.
> 
>
https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
> Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py:62:
> preexec_fn=os.setsid)
> AFAICT, you don't need (and don't want) os.setsid to be called.
> 
> Why did you think you needed this?
> 
>
https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
> Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py:70:
> self._stop_running_server()
> I don't think you need to override the base class here.
> 
>
https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
> Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py:82:
> self._executive.interrupt(self._pid)
> We might need to add another hook to ServerBase so that you can override it
> to call executive.interrupt() + self._process.wait() instead of 
> executive.kill_process().
> 
> I think you can get rid of the rest of the code in this method and 
> just use the base class impl otherwise.
> 
>
https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
> File Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium (right):
> 
>
https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
> Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium:18: changing the ports
> (HTTP/S, WS/S), make sure to update the python code too.
> This is actually a file you wrote, right? Can we move it out of thirdparty
> somewhere?
> 
> Also, do we really need both config files? Where does the
> wpt/config.default.json file
> come from? I don't see it on github, but maybe I'm not looking in the right
> place?
> 
> We should try to avoid putting files we write under thirdparty/ dirs.
> 
>
https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
> Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium:26: WPTWhiteList from
the
> checked out directory.
> From what I can tell, we only actually need 
> 
> the serve/ and maybe sslutils directories from
> 
> https://github.com/w3c/wpt-tools
> 
> and then the wptserve directory from
> 
> https://github.com/w3c/wptserve
> 
> Can we check those into 
> 
> thirdparty/wpt_tools/{serve,sslutils}
> thirdparty/wptserve/
> 
> and avoid a bunch of nested dirs?
> 
> Alternatively, we can consider just cloning the repos and pulling them via
DEPS.
> 
> I'd like to be able to skip and/or simplify as many of these steps as possible
> ...

I've refactored most of the stuff you pointed out. Also I left some comments
on points we should discuss further of defer to a followup CL.

I've also tried out running the tests. It's not straightforward yet as we have
multiple setup steps still required for this to run properly. So yes, I've seen
the failures you mention.

As for the hostnames, there are at least a few tests that are highly depending
on
acutal FQDNs or domain names. This is especially true for tests checking
cross-origin
behavior and security features.

I'll dig in deeper on this issue, maybe add some parameters to content_shell to
mock the
host names. However, I don't think we should do it as part of this CL as it's
already pretty big.

[burnik, 2015-06-15 10:35:10]

Refactored most of suggestions. Still a few open questions.

Please point out what is blocking this CL from landing.

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/common/system/executive.py (right):

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/common/system/executive.py:60: self._env[self._var] =
self._pathsep.join(self)
On 2015/06/12 22:25:01, Dirk Pranke wrote:
> These changes are not necessary, and we should never modify os.environ
directly
> (which is what this does). More comments about this elsewhere ...

Done.

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/common/system/executive.py:106: pythonpath =
EnvPath(os.environ, "PYTHONPATH", os.pathsep)
On 2015/06/12 22:25:01, Dirk Pranke wrote:
> the pythonpath change isn't necessary.

Done.

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py (right):

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py:37:
self._executive.pythonpath.update()
On 2015/06/12 22:25:02, Dirk Pranke wrote:
> You don't need this.
> 
> If you set 
> 
> self._env = {'PYTHONPATH': self._path_to_thirdparty} then
> you can pass env=self._env in the popen() call.
> 
> In addition, if you set self._stdout = self._stderr = self._executive.DEVNULL,

> you should be able to use ServerBases's implementation of _spawn_process()
> directly
> and not need to override it.
> 
> You might need to add a way to specify the cwd, but it seems like that's a bug
> in wptserve if it needs to be started from a particular directory.

Done.

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py:39:
self._path_to_wpt_root = os.path.join(path_to_wpt_support, 'wpt')
On 2015/06/12 22:25:01, Dirk Pranke wrote:
> Use self._filesystemm.join() instead of os.path.join(), here and elsewhere. 
> 
> Modules outside of webkitpy.common.system should generally not need to 
> import sys or os.

Done.

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py:45: start_cmd =
[self._executable,
On 2015/06/12 22:25:02, Dirk Pranke wrote:
> This should be 
> 
>     start_cmd = [self.port_obj.host.executable, self._executable, ...]
> 
> and you should rename self.executable to something else.
> 
> You don't want to run the python script directly; that won't work on Windows.
> 
> Also, if you make the change I suggest you don't need to call popen(...,
> shell=True).

Done.

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py:55: def
_spawn_process(self):
On 2015/06/12 22:25:02, Dirk Pranke wrote:
> As noted above, let's see if we can make the base class impl work for you.

Done.

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py:62:
preexec_fn=os.setsid)
On 2015/06/12 22:25:01, Dirk Pranke wrote:
> AFAICT, you don't need (and don't want) os.setsid to be called.
> 
> Why did you think you needed this?

It had to do with kill_process when killing a group. Removed.

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py:70:
self._stop_running_server()
On 2015/06/12 22:25:01, Dirk Pranke wrote:
> I don't think you need to override the base class here.

Done.

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py:82:
self._executive.interrupt(self._pid)
On 2015/06/12 22:25:01, Dirk Pranke wrote:
> We might need to add another hook to ServerBase so that you can override it
> to call executive.interrupt() + self._process.wait() instead of 
> executive.kill_process().
> 
> I think you can get rid of the rest of the code in this method and 
> just use the base class impl otherwise.

Actually we would need two hooks. 
One telling the base to keep the reference to the process (see _spawn_process)
so we can wait on it when stopping and the other one to signify we want to call
interrupt+wait. 

As I see it, the ServerBase is more tuned to the Apache approach than WPTServe.
I would therefore keep this _stop_running_server method and probably make
attempt to fix this start/stop mechanism on the WPTServe side.
There is already a TODO saying that I should find a cleaner way of doing it.

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/thirdparty/README.chromium (right):

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/README.chromium:74: Version: ?
On 2015/06/12 17:13:17, jsbell wrote:
> Should we use the (short) hash here?
> 
> ... or reference wpt/WPTHeads for the hashes, I suppose.

I was wondering the same thing...
Not sure if these files are being parsed so they might expect a particular
format... So "?" was the safest way to go.

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium (right):

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium:18: changing the ports
(HTTP/S, WS/S), make sure to update the python code too.
On 2015/06/12 22:25:02, Dirk Pranke wrote:
> This is actually a file you wrote, right? Can we move it out of thirdparty
> somewhere?
> 
> Also, do we really need both config files? Where does the
> wpt/config.default.json file
> come from? I don't see it on github, but maybe I'm not looking in the right
> place?
> 
> We should try to avoid putting files we write under thirdparty/ dirs.

I would actually like to keep it in thirdparty for the time being. The
README.chromium clearly states what's being stored in the directory. I would
then move it once I'm confident on the way we should maintain the third party
repos.

A bigger issue here is the way serve works. It loads the config.default.json
first and then the specific config.json provided and merges them together.
Until we fix that in WPT, I would follow the intended use pattern.

https://github.com/w3c/web-platform-tests/blob/master/config.default.json

I would definitely avoid rewriting the config.default.json or add the config to
the actual WPT directory, because it's easier to see what's been done with the
checked in code.

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium:23: and perform a fresh
checkout of the web-platform-tests code. When rolling in new
On 2015/06/12 17:13:17, jsbell wrote:
> Can you rewrite this more imperatively, i.e. keep short descriptions of the
> files here, then have a section of the doc: "Steps to update wpt"
> 

Done.

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium:26: WPTWhiteList from the
checked out directory.
On 2015/06/12 22:25:02, Dirk Pranke wrote:
> From what I can tell, we only actually need 
> 
> the serve/ and maybe sslutils directories from
> 
> https://github.com/w3c/wpt-tools
> 
> and then the wptserve directory from
> 
> https://github.com/w3c/wptserve
> 
> Can we check those into 
> 
> thirdparty/wpt_tools/{serve,sslutils}
> thirdparty/wptserve/
> 
> and avoid a bunch of nested dirs?
> 
> Alternatively, we can consider just cloning the repos and pulling them via
DEPS.
> 
> I'd like to be able to skip and/or simplify as many of these steps as possible
> ...

Actually, that's not all we need.

We need the wpt/serve script as it's the call into serve.main().
If we to decide to integrate the server into the same process as
run_webkit_tests then we can avoid it. Until then this script is a requirement.

I would also like to retain the directory structure imposed by WPT. Otherwise
this would require more wrapping of PYTHONPATH and other preconfiguration. As
I've stated in the DD, the behavior and structure should try to resemble what
one would experience in a local checkout of WPT.

I'm not sure if rolling into DEPS at this stage is really necessary, I believe
we should focus more on being able to run the existing tests on linux at this
point.

https://codereview.chromium.org/1154373005/diff/200001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium:63: (which gets generated
on the first run) and Web Platform Tests Documentation.
On 2015/06/12 17:23:54, burnik wrote:
> On 2015/06/12 17:13:17, jsbell wrote:
> > Include URL of the WPT docs?
> 
> When I find them, I'll make sure to add. So far, number of docs referring to
SSL
> in WPT: 
>
http://www.quickmeme.com/img/a5/a5482d8548fd094a6968efb4659393dfea89edeadeed8...
> 

I've rewritten this in the "Configuration" section.

[burnik, 2015-06-15 15:02:07]

I believe patch 11 should resolve the /etc/hosts issue. Since content_shell is
already mapping *.test to 127.0.0.1.

Big thanks to jochen@. :-)

The only thing that's now an issue is the /resources directory which should be
in LayoutTests/imported/web-platform-tests/. Since it's missing, the tests are
failing because the resource files can't be loaded from the doc_root. 

I assume I could safely copy the resources directory from LayoutTests/resources
(or just check them out from the github repo) and update
LayoutTests/W3CImportExpectations ?

This I believe would be the last step to be able to run this in a local dev env.
:-)
Still, there are some tests which are failing (26 as I've tested), but I believe
it's an easier fix.

We should then work on removing other copies of /resources once WPT serve is
adopted as a default
way to run the layout tests.

[burnik, 2015-06-15 15:42:47]

On 2015/06/15 15:02:07, burnik wrote:
> I believe patch 11 should resolve the /etc/hosts issue. Since content_shell is
> already mapping *.test to 127.0.0.1.

And by patch 11, I mean patch 12 (0-based indices FTW :-D).

> 
> Big thanks to jochen@. :-)
> 
> The only thing that's now an issue is the /resources directory which should be
> in LayoutTests/imported/web-platform-tests/. Since it's missing, the tests are
> failing because the resource files can't be loaded from the doc_root. 
> 
> I assume I could safely copy the resources directory from
LayoutTests/resources
> (or just check them out from the github repo) and update
> LayoutTests/W3CImportExpectations ?
> 
> This I believe would be the last step to be able to run this in a local dev
env.
> :-)
> Still, there are some tests which are failing (26 as I've tested), but I
believe
> it's an easier fix.
> 
> We should then work on removing other copies of /resources once WPT serve is
> adopted as a default
> way to run the layout tests.

[Dirk Pranke, 2015-06-15 23:41:49]

I think we're very close to being able to land this; I'm still not happy w/ the
directory structure and importing process, but I don't think we need to nail the
final version of this before we can land anything.

I have a few more nits, and will reach out to the lawyers as well, as the
licensing terms for this stuff is a little weird.

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/common/system/filesystem.py (right):

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/common/system/filesystem.py:49: pathsep = os.pathsep
You shouldn't need this (see other comments).

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/layout_tests/servers/server_base.py (right):

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/layout_tests/servers/server_base.py:79: self._process =
None
This seems a bit off; we actually define a self._process in the base class, but
only set it in apache_http.

Can we set it in the base class and then not need keep_process_reference?

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py (right):

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py:48:
self._env['PYTHONPATH'] = (path_to_thirdparty + fs.pathsep +
self._env.get('PYTHONPATH', ''))
We shouldn't need this (and hence not need fs.pathsep, either). It's a bug if we
need PYTHONPATH to be set to anything other than the directories we explicitly
set it to; we shouldn't pass through the user's setting.

(We actually do this in servers/pywebsocket.py as well, apparently, and it's a
bug there as well).

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium (right):

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium:6:
(https://github.com/w3c/web-platform-tests/) required to run WPT tests as part
nit: "run the WPT tests as part of Blink's test infrastructure".

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium:7: of Blink Layout Tests
as well as some maintenance and configuration code.
Please mention that this directory also contains code from the tools and
wptserve repos.

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/localpaths.py (right):

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/localpaths.py:1: import os
Can we drop this file from the import? I don't think we need it ...

[Dirk Pranke, 2015-06-15 23:42:25]

dpranke@chromium.org changed reviewers:
+ xam@google.com

[Dirk Pranke, 2015-06-15 23:42:59]

Max, can you take a look at this CL from a third-party OSPO perspective? I'll
send you a separate email as well.

[burnik, 2015-06-16 08:49:30]

Patchset #13 (id:260001) has been deleted

[burnik, 2015-06-16 09:24:10]

On 2015/06/15 23:41:49, Dirk Pranke wrote:
> I think we're very close to being able to land this; I'm still not happy w/
the
> directory structure and importing process, but I don't think we need to nail
the
> final version of this before we can land anything.
> 
> I have a few more nits, and will reach out to the lawyers as well, as the
> licensing terms for this stuff is a little weird.
> 
>
https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
> File Tools/Scripts/webkitpy/common/system/filesystem.py (right):
> 
>
https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
> Tools/Scripts/webkitpy/common/system/filesystem.py:49: pathsep = os.pathsep
> You shouldn't need this (see other comments).
> 
>
https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
> File Tools/Scripts/webkitpy/layout_tests/servers/server_base.py (right):
> 
>
https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
> Tools/Scripts/webkitpy/layout_tests/servers/server_base.py:79: self._process =
> None
> This seems a bit off; we actually define a self._process in the base class,
but
> only set it in apache_http.
> 
> Can we set it in the base class and then not need keep_process_reference?
> 
>
https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
> File Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py (right):
> 
>
https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
> Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py:48:
> self._env['PYTHONPATH'] = (path_to_thirdparty + fs.pathsep +
> self._env.get('PYTHONPATH', ''))
> We shouldn't need this (and hence not need fs.pathsep, either). It's a bug if
we
> need PYTHONPATH to be set to anything other than the directories we explicitly
> set it to; we shouldn't pass through the user's setting.
> 
> (We actually do this in servers/pywebsocket.py as well, apparently, and it's a
> bug there as well).
> 
>
https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
> File Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium (right):
> 
>
https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
> Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium:6:
> (https://github.com/w3c/web-platform-tests/) required to run WPT tests as part
> nit: "run the WPT tests as part of Blink's test infrastructure".
> 
>
https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
> Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium:7: of Blink Layout Tests
> as well as some maintenance and configuration code.
> Please mention that this directory also contains code from the tools and
> wptserve repos.
> 
>
https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
> File Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/localpaths.py (right):
> 
>
https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
> Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/localpaths.py:1: import os
> Can we drop this file from the import? I don't think we need it ...

Thanks for all the comments and the guidelines, Dirk. :-)

I've added a release plan step to fix up the directory structure 
and importing process. It will not go unaddressed.

jgraham also took a look at the CL and he sees no apparent issues 
(other than my confusing way of importing code).

So I guess we're counting on Max to give us more advice on the
legal aspect before we land the first bit. :-)

Thanks, Max!

[burnik, 2015-06-16 09:24:34]

Responding inline to code comments.

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/common/system/filesystem.py (right):

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/common/system/filesystem.py:49: pathsep = os.pathsep
On 2015/06/15 23:41:48, Dirk Pranke wrote:
> You shouldn't need this (see other comments).

Done.

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/layout_tests/servers/server_base.py (right):

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/layout_tests/servers/server_base.py:79: self._process =
None
On 2015/06/15 23:41:48, Dirk Pranke wrote:
> This seems a bit off; we actually define a self._process in the base class,
but
> only set it in apache_http.
> 
> Can we set it in the base class and then not need keep_process_reference?

Done. Hope it doesn't break anything...

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py (right):

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/layout_tests/servers/wptserve.py:48:
self._env['PYTHONPATH'] = (path_to_thirdparty + fs.pathsep +
self._env.get('PYTHONPATH', ''))
On 2015/06/15 23:41:48, Dirk Pranke wrote:
> We shouldn't need this (and hence not need fs.pathsep, either). It's a bug if
we
> need PYTHONPATH to be set to anything other than the directories we explicitly
> set it to; we shouldn't pass through the user's setting.
> 
> (We actually do this in servers/pywebsocket.py as well, apparently, and it's a
> bug there as well).

Done. Added a comment to check why PyWebSocket thinks it needs it...

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium (right):

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium:6:
(https://github.com/w3c/web-platform-tests/) required to run WPT tests as part
On 2015/06/15 23:41:48, Dirk Pranke wrote:
> nit: "run the WPT tests as part of Blink's test infrastructure".

Done.

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium:7: of Blink Layout Tests
as well as some maintenance and configuration code.
On 2015/06/15 23:41:48, Dirk Pranke wrote:
> Please mention that this directory also contains code from the tools and
> wptserve repos.

Done.

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/localpaths.py (right):

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/localpaths.py:1: import os
On 2015/06/15 23:41:48, Dirk Pranke wrote:
> Can we drop this file from the import? I don't think we need it ...

This is third party code (run as part of WPT serve).
If I start here, I'll change the whole thing, which will kill the effort
to use third party updates. :-)

[Dirk Pranke, 2015-06-17 04:19:39]

lgtm w/ one question which doesn't really block anything, and we still need to
wait while I clarify the legal aspect (which I will follow up on off-CL).

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/localpaths.py (right):

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/localpaths.py:1: import os
On 2015/06/16 09:24:34, burnik wrote:
> On 2015/06/15 23:41:48, Dirk Pranke wrote:
> > Can we drop this file from the import? I don't think we need it ...
> 
> This is third party code (run as part of WPT serve).
> If I start here, I'll change the whole thing, which will kill the effort
> to use third party updates. :-)

I don't understand; can't you just remove localpaths.py from the list 
in WPTWhitelist and re-run the checkout script so that we're not
adding this file?

Maybe I don't understand your concern?

[burnik, 2015-06-17 08:24:27]

I forgot to push the followup comment yesterday about localpaths.py

See comments below.

Thanks! :)

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/localpaths.py (right):

https://codereview.chromium.org/1154373005/diff/240001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/localpaths.py:1: import os
On 2015/06/16 09:24:34, burnik wrote:
> On 2015/06/15 23:41:48, Dirk Pranke wrote:
> > Can we drop this file from the import? I don't think we need it ...
> 
> This is third party code (run as part of WPT serve).
> If I start here, I'll change the whole thing, which will kill the effort
> to use third party updates. :-)

It's being imported in thirdparty/wpt/wpt/tools/serve.py 
from .. import localpaths

So I could possibly get rid of it, however, then the changes to the third party
code are no longer just pure "reduction" in terms of deleting files, it would
require
actual changes to the code itself (hence even more difficult to get updates).

Ideally we would be using a single file for the whole WPTServe and not care
about this at all.
It would be nice if WPT contributors had some API release plan with at least
some guarantees so we could transition to newer versions easier... 

I am thinking about writing some small integration tests to help this
transitioning process for the future. I am sure that wptserve and wpt-tools will
get a bunch of updates and some of them are bound to be a breaking change for
us.

We should discuss this further in the design doc.

[Dirk Pranke, 2015-06-17 15:51:59]

On 2015/06/17 08:24:27, burnik wrote:
> On 2015/06/16 09:24:34, burnik wrote:
> > On 2015/06/15 23:41:48, Dirk Pranke wrote:
> > > Can we drop this file from the import? I don't think we need it ...
> > 
> > This is third party code (run as part of WPT serve).
> > If I start here, I'll change the whole thing, which will kill the effort
> > to use third party updates. :-)
> 
> It's being imported in thirdparty/wpt/wpt/tools/serve.py 
> from .. import localpaths

Oh, okay. Somehow I missed that when searching to see if it was used anywhere.

I'm fine w/ leaving it alone for now, though I agree that we should clean things
up a bit more at some point.

[burnik, 2015-06-23 08:12:29]

Patchset #14 (id:300001) has been deleted

[Dirk Pranke, 2015-06-23 19:10:52]

lgtm w/ a few nits.

Thanks for working on this! I'm excited to try it out on the different platforms
and see how it goes.

https://codereview.chromium.org/1154373005/diff/320001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/thirdparty/README.chromium (right):

https://codereview.chromium.org/1154373005/diff/320001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/README.chromium:78: Description: Used to run
all supported web-platform-tests as part of blink layout tests.
Nit: can you add a bit about how stuff in this directory is different from what
is in LayoutTests/imported/web-platform-tests ? I.e., that this is just meant to
be the tooling ?

Also, add a mention of wpt/README.chormium.

https://codereview.chromium.org/1154373005/diff/320001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/README.chromium:79: Local Modifications:
Removed all files except listed in wpt/WPTWhiteList
Nit: "except for those listed in"

https://codereview.chromium.org/1154373005/diff/320001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/README.chromium:89: Local Modifications:
Removed all files except listed in wpt/WPTWhiteList
Nit: "except for those listed in"

https://codereview.chromium.org/1154373005/diff/320001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/README.chromium:99: Local Modifications:
Removed all files except listed in wpt/WPTWhiteList
same.

https://codereview.chromium.org/1154373005/diff/320001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium (right):

https://codereview.chromium.org/1154373005/diff/320001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium:40: List of git commit-ish
for the WPT repositories. File format is as follows:
Nit: does it make sense to list the HEADS in thirdparty/README.chromium, in the
versions ? Or have those versions point at WPTHeads?

[burnik, 2015-06-24 09:19:05]

Addressed all nits. :-)

https://codereview.chromium.org/1154373005/diff/320001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/thirdparty/README.chromium (right):

https://codereview.chromium.org/1154373005/diff/320001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/README.chromium:78: Description: Used to run
all supported web-platform-tests as part of blink layout tests.
On 2015/06/23 19:10:52, Dirk Pranke wrote:
> Nit: can you add a bit about how stuff in this directory is different from
what
> is in LayoutTests/imported/web-platform-tests ? I.e., that this is just meant
to
> be the tooling ?
> 
> Also, add a mention of wpt/README.chormium.

Done.

https://codereview.chromium.org/1154373005/diff/320001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/README.chromium:79: Local Modifications:
Removed all files except listed in wpt/WPTWhiteList
On 2015/06/23 19:10:52, Dirk Pranke wrote:
> Nit: "except for those listed in"

Done.

https://codereview.chromium.org/1154373005/diff/320001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/README.chromium:89: Local Modifications:
Removed all files except listed in wpt/WPTWhiteList
On 2015/06/23 19:10:52, Dirk Pranke wrote:
> Nit: "except for those listed in"

Done.

https://codereview.chromium.org/1154373005/diff/320001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/README.chromium:99: Local Modifications:
Removed all files except listed in wpt/WPTWhiteList
On 2015/06/23 19:10:52, Dirk Pranke wrote:
> same.

Done.

https://codereview.chromium.org/1154373005/diff/320001/Tools/Scripts/webkitpy...
File Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium (right):

https://codereview.chromium.org/1154373005/diff/320001/Tools/Scripts/webkitpy...
Tools/Scripts/webkitpy/thirdparty/wpt/README.chromium:40: List of git commit-ish
for the WPT repositories. File format is as follows:
On 2015/06/23 19:10:52, Dirk Pranke wrote:
> Nit: does it make sense to list the HEADS in thirdparty/README.chromium, in
the
> versions ? Or have those versions point at WPTHeads?

I've added the current SHA1s to thirdparty/README.chromium
Also added a note pointer to wpt/WPTHeads.

+: Added the instructions on updating the thirdparty/README.chromium.

[burnik, 2015-06-24 09:19:52]

The CQ bit was checked by burnik@chromium.org

[burnik, 2015-06-24 09:19:53]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1154373005/#ps340001
(title: "Update README.chromium files. Fix nits.")

[commit-bot: I haz the power, 2015-06-24 09:20:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1154373005/340001

[commit-bot: I haz the power, 2015-06-24 09:27:33]

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

[commit-bot: I haz the power, 2015-06-24 09:27:35]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

[burnik, 2015-06-24 10:06:53]

The CQ bit was checked by burnik@chromium.org

[burnik, 2015-06-24 10:06:54]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1154373005/#ps360001
(title: "Add executable bit to pass permchecks.")

[commit-bot: I haz the power, 2015-06-24 10:07:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1154373005/360001

[commit-bot: I haz the power, 2015-06-24 11:21:10]

Committed patchset #16 (id:360001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197732