Bug 35812 - [new-run-webkit-tests][Chromium] split http_server into a new top level script plus a module
Summary: [new-run-webkit-tests][Chromium] split http_server into a new top level scrip...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-05 14:27 PST by Dirk Pranke
Modified: 2010-03-05 16:40 PST (History)
4 users (show)

See Also:


Attachments
patch to split running http_server as a standalone script vs. an embedded library (8.49 KB, patch)
2010-03-05 15:38 PST, Dirk Pranke
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2010-03-05 14:27:17 PST
Chromium's ui_test infrastructure uses the layout tests http_server infrastructure for some HTTP testing. At the moment it calls into webkitpy/layout_tests/port/http_server directly (as a __main__), but this is awkward, buggy on a mac, and has several layering violations.

I'm going to resolve this by creating a new top-level new-run-webkit-httpd script and call that instead, and make http_server just be a module.

The new script only works w/ Chromium's LigHTTP implementation, but should be made generic.
Comment 1 Dirk Pranke 2010-03-05 15:38:19 PST
Created attachment 50134 [details]
patch to split running http_server as a standalone script vs. an embedded library
Comment 2 Eric Seidel (no email) 2010-03-05 15:41:13 PST
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 3 Eric Seidel (no email) 2010-03-05 15:42:09 PST
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. :)
Comment 4 Dirk Pranke 2010-03-05 16:12:59 PST
(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 5 Ojan Vafai 2010-03-05 16:27:03 PST
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.
Comment 6 Dirk Pranke 2010-03-05 16:33:01 PST
(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.
Comment 7 Dirk Pranke 2010-03-05 16:40:06 PST
Patch landed as r55602: http://trac.webkit.org/changeset/55602