Bug 181885

Created attachment 332054 [details] Patch

I'm curious what other contributors think about placing more logic like this in the port class. I am actually against a change like this. I think that we've placed too much in the port class, and am unhappy that a class like LayoutTestApacheHttpd even requires the port object at all. I would much rather LayoutTestApacheHttpd take a host object and preform and platform specific path manipulation itself. The advantage of this would be that it turns LayoutTestApacheHttpd into ApacheHttpd which is not wedded to layout tests in the first place (which will be helpful for API tests, for example). I would much rather we just add the WinCairo branch and keep this logic inside LayoutTestApacheHttpd rather than continuing to pollute the port class.

> I am actually against a change like this. I think that we've placed too much in the port class, and am unhappy that a class like LayoutTestApacheHttpd even requires the port object at all. The point is that launching httpd server is the duty of port object ( start_server() ). That means each port has different requirements for launching servers. Actually current implementation for win is for AppleWin, but WinCairo has different requirements. > I would much rather LayoutTestApacheHttpd take a host object and preform and platform specific path manipulation itself. With above reason, host object is not enough. > The advantage of this would be that it turns LayoutTestApacheHttpd into ApacheHttpd which is not wedded to layout tests in the first place (which will be helpful for API tests, for example). I agree with this. Httpd should be more generic. I also don't like too much dependencies. One solution may passing arguments for httpd by constructor. Things to configure are just paths to be passed to start command. They can be an simple dictionary. For now, we really need this change to achieve http tests on WinCairo. Of course we can keep patching onto current LayoutTestApacheHttpd without this refactoring, but that may bring another madness on it and ended up to hardly maintainable code. With taking this refactoring and those madness are gone from LayoutTestApacheHttpd and other ports. They appears only on WinCairoPort, but we can manage that.

(In reply to Basuke Suzuki from comment #3) > ... > > For now, we really need this change to achieve http tests on WinCairo. Of > course we can keep patching onto current LayoutTestApacheHttpd without this > refactoring, but that may bring another madness on it and ended up to hardly > maintainable code. With taking this refactoring and those madness are gone > from LayoutTestApacheHttpd and other ports. They appears only on > WinCairoPort, but we can manage that. I would prefer this code remain in LayoutTestApacheHttpd. Your statements about platforminfo being insufficient are correct. I would be be for using arguments to LayoutTestApacheHttpd. Another possible solution might be to override Port.start_http_server(...) for win-cairo and create a class which inherits from LayoutTestApacheHttpd to handle the win-cairo specific bits. I'm also curious how win-cairo is differentiated from the other ports. I wouldn't expect LayoutTestApacheHttpd to differ much.

Actually WinCairo tries to be as same as other ports. AppleWin did different thing than others. I don't have detail about that, but they do that change always on is_win() == true. See the patched win.py.