Right now, layoutTestController directly adds itself to window object using JSC/V8 code. It'll be nice if it were done via IDL files like internals object.
The challenge here is that layoutTestController is defined in DumpRenderTree instead of WebCore so we can't use Source/WebCore/DerivedSources.make. One thing we can do is to add layoutTestController as an abstract class in WebCore/testing/ along with an IDL file, and let DumpRenderTree implement it with a concrete class. That might be cleaner than introducing a derived source in DumpRenderTree since the latter involves modifying all build systems we have.
That sounds like a reasonable solution. Adding a method would then involve editing the IDL file, the ABC, and the concrete LayoutTestController object. That still seems like an improvement over the current situation.
(In reply to comment #2) > That sounds like a reasonable solution. Adding a method would then involve editing the IDL file, the ABC, and the concrete LayoutTestController object. That still seems like an improvement over the current situation. Yeah, we'll at least eliminate the need to modify both JSC and V8 versions of layoutTestController.cpp files.
I haven't looked at the details in a while, but I seem to remember that DRT and LayoutTestController don't share that much code across ports. Having an ABC might be a good way to share more code. For example, you could have an intermediate class that's shared by all the ports and then a further port-specific leaf class. We'll have to look at the details to see what makes the most sense.
Created attachment 145480 [details] work in progress The test passes on Mac WebKit1. Will finish it up tomorrow.
Comment on attachment 145480 [details] work in progress View in context: https://bugs.webkit.org/attachment.cgi?id=145480&action=review > Tools/DumpRenderTree/LayoutTestController.h:445 > +class TestRunner : public WebCore::TestRunnerBase { I was expecting LayoutTestController to inherit from TestRunnerBase. As written, you're going to need to forward every call to m_controller...
(In reply to comment #6) > (From update of attachment 145480 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145480&action=review > > > Tools/DumpRenderTree/LayoutTestController.h:445 > > +class TestRunner : public WebCore::TestRunnerBase { > > I was expecting LayoutTestController to inherit from TestRunnerBase. As written, you're going to need to forward every call to m_controller... It turns out LayoutTestController doesn't implement methods like dumpAsText or waitUntilDone. Instead, static callback functions implement those. So my current plan is to implement TestRunner class as a proxy for LayoutTestController, transition to use IDL, and then merge the existing LayoutTestController into TestRunner. I mean... I could attempt to do all of them at once but the patch is going to be huge then.
Ah, ok. That makes sense.
(In reply to comment #8) > Ah, ok. That makes sense. But now I realize that WebKit2 already has layoutTestController.idl and CodeGeneratorTestRunner.pm. And it's a bit tricky to share the code there because of extensions like "PassContext".
I guess doing the rename in this patch is too aggressive given the separate IDL & code generator in WebKit2. We probably need to add IDL support in DumpRenderTree first, and somehow share IDL files between WebKitTestRunner and DumpRenderTree.
+Sam, +Maciej since they implemented WebKitTestRunner's IDL & code generator.
You're being drawn into the bigger problem, which is to unify DumpRenderTree and WebKitTestRunner. Sharing the IDL file seems valuable given that we expect tests to run both in WebKit1 and WebKit2.
(In reply to comment #12) > You're being drawn into the bigger problem, which is to unify DumpRenderTree and WebKitTestRunner. Sharing the IDL file seems valuable given that we expect tests to run both in WebKit1 and WebKit2. Yeah, we should probably do it in a separate patch though. This patch has already gotten quite massive.
Doing it in a separate patch is fine. I'd just recommending having a plan in mind for how to share the IDL file.
Created attachment 145622 [details] work in progress 2