Summary: | [new-run-webkit-tests][Chromium] split http_server into a new top level script plus a module | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dirk Pranke <dpranke> | ||||
Component: | Tools / Tests | Assignee: | Dirk Pranke <dpranke> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | cjerdonek, eric, ojan, tony | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | PC | ||||||
OS: | OS X 10.5 | ||||||
Attachments: |
|
Description
Dirk Pranke
2010-03-05 14:27:17 PST
Created attachment 50134 [details]
patch to split running http_server as a standalone script vs. an embedded library
LGTM, but I thought that Ojan had already made up a python version of run-webkit-httpd which knew how to talk to apache2? I'm ready to r+ this as soon as we hear from Ojan. Comment on attachment 50134 [details]
patch to split running http_server as a standalone script vs. an embedded library
Or rather, I'll r+ it, but I'd like Ojan to bless the patch before landing. :)
(In reply to comment #2) > LGTM, but I thought that Ojan had already made up a python version of > run-webkit-httpd which knew how to talk to apache2? > > I'm ready to r+ this as soon as we hear from Ojan. If he does, I haven't seen it. However, the chromium test code actually has switches hard-coded into it for the LigHTTPd version (unfortunately), so I would like to address the whole apache/lighttpd thing separately from this patch. Comment on attachment 50134 [details] patch to split running http_server as a standalone script vs. an embedded library A couple nits on comments. The code looks fine. No need for another review before committing though. > +++ b/WebKitTools/ChangeLog > + the Chromium tree. At some point this script should be made to > + work with Apache-based implementations and on other ports. Could you file a bug for this and link to it in the ChangeLog description? > +++ b/WebKitTools/Scripts/new-run-webkit-httpd > +# TODO: currently this code only works with the Chromium ports and LigHTTPd. s/TODO/FIXME > +# It should be made to work on all ports. > +# > +# This script is also used by Chromium's ui_tests to run http layout tests > +# in a browser. I don't think this needs to be documented. It's not an explicit goal of the WebKit project to support Chromium's ui_test infrastructure. I don't think it's an issue in practice, but if WebKit needed this to change in a way that's incompatible with Chromium's needs, then it should be able to. (In reply to comment #5) > (From update of attachment 50134 [details]) > A couple nits on comments. The code looks fine. No need for another review > before committing though. > > > +++ b/WebKitTools/ChangeLog > > + the Chromium tree. At some point this script should be made to > > + work with Apache-based implementations and on other ports. > > Could you file a bug for this and link to it in the ChangeLog description? > Done. See bug 35820. > > +++ b/WebKitTools/Scripts/new-run-webkit-httpd > > +# TODO: currently this code only works with the Chromium ports and LigHTTPd. > s/TODO/FIXME > Grr. I thought FIXME was Chromium and TODO was WebKit. Guess I got it backwards. > > +# It should be made to work on all ports. > > +# > > +# This script is also used by Chromium's ui_tests to run http layout tests > > +# in a browser. > I don't think this needs to be documented. It's not an explicit goal of the > WebKit project to support Chromium's ui_test infrastructure. I don't think it's > an issue in practice, but if WebKit needed this to change in a way that's > incompatible with Chromium's needs, then it should be able to. I agree that WebKit should be able to change things, but it doesn't hurt to have the dependency documented. Patch landed as r55602: http://trac.webkit.org/changeset/55602 |