The initial idea is here: https://lists.webkit.org/pipermail/webkit-dev/2010-July/013559.html.
Created attachment 62165 [details] Patch
Created attachment 62166 [details] rebased
Attachment 62166 [details] did not build on qt: Build output: http://queues.webkit.org/results/3591310
Attachment 62166 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3589309
Created attachment 62168 [details] build fix
Created attachment 62169 [details] build fix for gtk
Created attachment 62260 [details] made the patch smaller to contain only mac port
Comment on attachment 62260 [details] made the patch smaller to contain only mac port Thanks for doing this. This is a big improvement over needing to implement the same API many times over. I don't really know JSC APIs well enough to know if there is a better way to do the inheritance. Someone else will need to review the code properly. I just have a few naming issues. I think we should call the new class LayoutTestControllerWebCore though. Just saying "Core" is too vague (e.g. that could also be JavaScriptCore). > + int documentMakerSizeForNode(Node*, ExceptionCode&) const; > + String documentMakerStringForNodeAt(Node*, int, ExceptionCode&) const; Did you mean documentMarker* here?
Created attachment 62545 [details] Patch
Created attachment 62547 [details] Patch
HI ojan, thank you for reviewing! (In reply to comment #8) > I think we should call the new class LayoutTestControllerWebCore though. Just saying "Core" is too vague (e.g. that could also be JavaScriptCore). I agree that the name "WebCore::LayoutTestControllerCore" is strange. but "WebCore::LayoutTestControllerWebCore" looks redundant. So I just renamed it to "WebCore::LayoutTestController". (In our convention, original LayoutTestController shoud be named WebLayoutTestController so something. But it's too late.) > > + int documentMakerSizeForNode(Node*, ExceptionCode&) const; > > + String documentMakerStringForNodeAt(Node*, int, ExceptionCode&) const; > > Did you mean documentMarker* here? Oops. You're right. Thank you pointing this out. Fixed.
Comment on attachment 62547 [details] Patch Except for the naming nits below, this patch looks good to me, so I'm not setting r-. But I know Darin and Maciej had some reservations about extending layoutTestController at all. So I'd like one of them to at least look at this before setting r+. > +++ b/LayoutTests/fast/dom/Window/script-tests/window-layout-test-controller-core.js Please rename to window-layout-test-controller.js or window-layout-test-controller-webcore.js. In all of the below, please rename core to webCoreLayoutTestController and coreObject to webCoreLayoutTestControllerObject. It's more verbose, but much less confusing. > +++ b/WebKitTools/DumpRenderTree/LayoutTestController.cpp > +static const char setupLayoutTestControllerScript[] = > + "function delegate(dst, src, name) { dst[name] = function() { return src[name].apply(src, arguments); } }\n" > + "for (var name in this.core) {\n" > + " delegate(this, this.core, name);\n" > + "}\n"; > + > +void LayoutTestController::makeDelegationMethods(JSContextRef context, JSObjectRef thisObject, JSValueRef coreObject, JSValueRef* exception) > +{ > + JSRetainPtr<JSStringRef> coreStr(Adopt, JSStringCreateWithUTF8CString("core")); > + JSObjectSetProperty(context, thisObject, coreStr.get(), coreObject, kJSPropertyAttributeNone, exception); > + ASSERT(!*exception); > -void LayoutTestController::makeWindowObject(JSContextRef context, JSObjectRef windowObject, JSValueRef* exception) > +void LayoutTestController::makeWindowObject(JSContextRef context, JSObjectRef windowObject, JSValueRef* exception, JSValueRef core) > @@ -1706,6 +1725,9 @@ void LayoutTestController::makeWindowObject(JSContextRef context, JSObjectRef wi > > + if (core) > + makeDelegationMethods(context, JSValueToObject(context, layoutTestContollerObject, exception), core, exception); > + > +++ b/WebKitTools/DumpRenderTree/LayoutTestController.h > - void makeWindowObject(JSContextRef context, JSObjectRef windowObject, JSValueRef* exception); > + void makeDelegationMethods(JSContextRef context, JSObjectRef thisObject, JSValueRef coreObject, JSValueRef* exception); > + void makeWindowObject(JSContextRef context, JSObjectRef windowObject, JSValueRef* exception, JSValueRef core = 0); > +++ b/WebKitTools/DumpRenderTree/mac/FrameLoadDelegate.mm > + JSValueRef coreLayoutTestController = [frame layoutTestController]; > + gLayoutTestController->makeWindowObject(context, globalObject, &exception, coreLayoutTestController); > ASSERT(!exception);
Created attachment 62639 [details] Patch
Hi Ojan, thank you for reviewing again! Suggested renames are done. > Except for the naming nits below, this patch looks good to me, so I'm not setting r-. But I know Darin and Maciej had some reservations about extending layoutTestController at all. So I'd like one of them to at least look at this before setting r+. I see. We should wait for their comments.
Comment on attachment 62639 [details] Patch I'm super confused. Is this code turned on in production??? Why is layoutTestController in WebCore? The ChangeLog doesn't answer any of these questions for me... In the past, layoutTestController has been completely external to WebCore and WebKit. LayoutTests/fast/dom/Window/window-layout-test-controller-webcore.html:1 + <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> Can we just change this to <!DOCTYPE html> ? Feel free to fix them all in a followup patch.
Comment on attachment 62639 [details] Patch I looked at the webkit-dev thread, and I don't see consensus that this is a good idea. It's scary to me.
> I looked at the webkit-dev thread, and I don't see consensus that this is a good idea. It's scary to me. What exactly is scary? The only unaddressed concern from the webkit-dev thread is whether we really should make it easier to add things to layoutTestController or find a better way of doing testing. I certainly share that concern, but unless there is a concrete suggestion for doing a better way of testing, I think we should move forward with this. It seems a considerable improvement over the current world where, when trying to test something new, you have a choice between adding many layers of plumbing across many ports or adding a manual test. This doesn't block coming up with better ways of testing. It just makes the current way less painful.
It does seem important for this object not to be exposed in production builds. Maybe not in nightly builds, either.
> What exactly is scary? We don't want this to be part of the web platform. That means we want it turned off in non-testing builds. Also, we want to be sure we're not doing to screw up and expose it by accident. Having the layoutTestController in a separate project makes that much harder to screw up.
Created attachment 62801 [details] Patch
Hi everyone, thank you for your feedback! Updated patch disables WebCore::LTC on production build. > > LayoutTests/fast/dom/Window/window-layout-test-controller-webcore.html:1 > + <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> > Can we just change this to <!DOCTYPE html> ? Feel free to fix them all in a followup patch. It looks OK because this file is generated by make-script-test-wrappers like other script tests. (In reply to comment #16) > (From update of attachment 62639 [details]) > I looked at the webkit-dev thread, and I don't see consensus that this is a good idea. It's scary to me. Yes, a series of patches is for a starting point of discussion. I'd like to continue to discuss here because most of interest party is already in CC list. In my understanding, what bad is current trend of DRT testing which disallow manual testing. This patch might not solve the problem though I hope this helps, but it would approaches other problem, as discussed on webkit-dev thread. (In reply to comment #18) > It does seem important for this object not to be exposed in production builds. Maybe not in nightly builds, either. Agreed. I disabled it on production build. But I have no idea which configuration is used for nightly build. I think WebCore::LTC should be enabled on Release build because we use Release build of DRT. (In reply to comment #19) > > What exactly is scary? > > We don't want this to be part of the web platform. That means we want it turned off in non-testing builds. Also, we want to be sure we're not doing to screw up and expose it by accident. Having the layoutTestController in a separate project makes that much harder to screw up. Creating a separate project doesn't help an original goal of the patch. By making LTC different framework, we - need to export more symbols from WebCore framework for accessing from LTC. - need to provide the way to access WebCore internal object (like WebCore::Frame). Such are possibly worse than just to allow LTC staying inside the WebCore. On the other hand, I agree that making LTC available in accident could be a tragedy. I hope the design have such case in mind. On availability for JS world, It's simply hard to access it because applications need to - explicitly instantiate it, an - explicitly put it into the context. WebCore::LTC isn't accessible existing DOM object graph. So it wouldn't be possible unless intentional. I'll be happy to hear your opinions more.
Created attachment 62805 [details] rebaselined
Comment on attachment 62805 [details] rebaselined WebCore/Configurations/FeatureDefines.xcconfig:47 + ENABLE_CORE_LAYOUT_TEST_CONTROLLER_Production = ; I'm not sure this is a good approach. Differences between Production and Release are bad because it means we're not testing the same thing we're shipping. Also, it seems very likely that some port or other will get confused and ship WebCore::LTC to users. I'm not sure what the consequences are today, but as it grows, I'm sure it will eventually be disaster. Also, as a consequence, we won't be able to run these tests on production builds. That means, we'll lose out on test coverage of the actual bits we ship. There's got to be a better approach. Here's a strawman proposal: 1) WebCore exports a C API for testing that consumed by a cross-platform layout test controller object that's build in a different project (DRT). In that approach, we save the per-port hassle of adding features to LTC and we don't risk shipping those API to the web.
Another thought: we could make this object conditional on a WebCore::Setting. We could name the setting something like "dangerousTestingAPIEnabled" or some such. I think this is easy to do in V8. I don't remember if a similar mechanism exists in JSC.
On all our platforms I believe it’s easy to export an ability to create the object, but then have the WebKit client responsible for injecting it into each frame. So we don’t necessarily need a preference.
Created attachment 63403 [details] Patch
Adam, Darin, thank you for your feedback! - and sorry for slow reply. The last patch split test-specific code out from WebCore.framework into WebCoreTesting.framework. An idea is: - WebCore providing hook interface and JS stub for that. - WebCoreTesting use that interface to inject a test-specific code - WebKit provide a API for WebCoreTesting, while linking WebCoreTesting.framework with "weak" linkage, that allow us to ship WebKit.framework without WebCoreTesting.framework. The source code for WebKitTesting still stays WebCore/testing/ and built inside WebCore.xcodeproj So it can access WebCore APIs. A tradeoff is that we need to expose WebCore APIs that are required by WebCoreTesting. But I think it need less hassle than wrapping and exposing it via WebKit. I'm not sure if weak linkage is available for other port but I believe there are some workaround for that. (i.e. just built WebCore/testing/* into DRT.) Any ideas and feedbacks are welcome!
Created attachment 64186 [details] Rebaselined. Could anyone take a look?
Created attachment 67009 [details] Patch
Hi everyone! I'd like to restart this effort. I'm sorry that old patches were too large and did a big change. An latest patch is get further simple, thus small. I'm happy if you took a look at this one. The approach is based on Darin's advice, with Adam's concern in mind: - The WebCore::LayoutTestController is under testing/ directory. And the rest of WebCore NEVER depend on the code under testing/ So its isolation is clear and hard to accidentally available from browsers, even though it lives in WebCore.framework and can take its advantage. - testing/ module expose just one API (per JS engine) that instantiates WebCore::LTC and injects its method into given JS object using a delegation. This approach doesn't use JSC-specifi mechanism and relatively interpreter neutral. - DumpRenderTree just invokes that API to make WebCore::LTC available. So integration to each DRT implementation will be easy. Any feedback is welcome!
Attachment 67009 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3966325
One change necessary for this to build on GTK+ is to include the WebCore/testing directory on the list of IDL directories specified with the vpath feature.
Created attachment 67131 [details] an attempt to fix GTK+ build
Comment on attachment 67131 [details] an attempt to fix GTK+ build Got obsolete, giving up at now.
Folding into new pretty bug. *** This bug has been marked as a duplicate of bug 60277 ***