Bug 42612

Summary: DRT should allow test scripts to access WebCore internal states for setup/verification
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: abarth, ap, aroben, darin, dglazkov, gns, hamaji, japhet, mjs, mrobinson, ojan, steveblock, tonikitoo, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
rebased
none
build fix
none
build fix for gtk
none
made the patch smaller to contain only mac port
none
Patch
none
Patch
none
Patch
none
Patch
none
rebaselined
none
Patch
none
Rebaselined. Could anyone take a look?
none
Patch
none
an attempt to fix GTK+ build none

Description Hajime Morrita 2010-07-19 19:23:14 PDT
The initial idea is here: https://lists.webkit.org/pipermail/webkit-dev/2010-July/013559.html.
Comment 1 Hajime Morrita 2010-07-21 04:55:21 PDT
Created attachment 62165 [details]
Patch
Comment 2 Hajime Morrita 2010-07-21 05:11:33 PDT
Created attachment 62166 [details]
rebased
Comment 3 Early Warning System Bot 2010-07-21 05:20:17 PDT
Attachment 62166 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3591310
Comment 4 WebKit Review Bot 2010-07-21 05:35:38 PDT
Attachment 62166 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3589309
Comment 5 Hajime Morrita 2010-07-21 05:39:51 PDT
Created attachment 62168 [details]
build fix
Comment 6 Hajime Morrita 2010-07-21 05:44:21 PDT
Created attachment 62169 [details]
build fix for gtk
Comment 7 Hajime Morrita 2010-07-21 21:32:22 PDT
Created attachment 62260 [details]
made the patch smaller to contain only mac port
Comment 8 Ojan Vafai 2010-07-22 18:26:26 PDT
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?
Comment 9 Hajime Morrita 2010-07-25 22:53:25 PDT
Created attachment 62545 [details]
Patch
Comment 10 Hajime Morrita 2010-07-25 23:18:11 PDT
Created attachment 62547 [details]
Patch
Comment 11 Hajime Morrita 2010-07-25 23:19:06 PDT
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 12 Ojan Vafai 2010-07-26 17:15:40 PDT
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);
Comment 13 Hajime Morrita 2010-07-26 21:22:03 PDT
Created attachment 62639 [details]
Patch
Comment 14 Hajime Morrita 2010-07-26 21:24:00 PDT
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 15 Adam Barth 2010-07-27 07:56:35 PDT
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 16 Adam Barth 2010-07-27 07:58:33 PDT
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.
Comment 17 Ojan Vafai 2010-07-27 08:09:21 PDT
> 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.
Comment 18 Adam Roben (:aroben) 2010-07-27 08:16:01 PDT
It does seem important for this object not to be exposed in production builds. Maybe not in nightly builds, either.
Comment 19 Adam Barth 2010-07-27 08:17:50 PDT
> 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.
Comment 20 Hajime Morrita 2010-07-28 01:35:50 PDT
Created attachment 62801 [details]
Patch
Comment 21 Hajime Morrita 2010-07-28 02:03:33 PDT
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.
Comment 22 Hajime Morrita 2010-07-28 02:21:50 PDT
Created attachment 62805 [details]
rebaselined
Comment 23 Adam Barth 2010-07-29 08:03:36 PDT
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.
Comment 24 Adam Barth 2010-07-29 08:18:59 PDT
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.
Comment 25 Darin Adler 2010-07-29 11:16:22 PDT
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.
Comment 26 Hajime Morrita 2010-08-03 19:09:21 PDT
Created attachment 63403 [details]
Patch
Comment 27 Hajime Morrita 2010-08-03 19:19:27 PDT
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!
Comment 28 Hajime Morrita 2010-08-11 21:37:23 PDT
Created attachment 64186 [details]
Rebaselined. Could anyone take a look?
Comment 29 Hajime Morrita 2010-09-09 01:57:04 PDT
Created attachment 67009 [details]
Patch
Comment 30 Hajime Morrita 2010-09-09 02:09:22 PDT
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!
Comment 31 WebKit Review Bot 2010-09-09 04:50:42 PDT
Attachment 67009 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3966325
Comment 32 Martin Robinson 2010-09-09 07:34:01 PDT
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.
Comment 33 Hajime Morrita 2010-09-09 18:25:47 PDT
Created attachment 67131 [details]
an attempt to fix GTK+ build
Comment 34 Hajime Morrita 2011-01-25 21:29:16 PST
Comment on attachment 67131 [details]
an attempt to fix GTK+ build

Got obsolete, giving up at now.
Comment 35 Dimitri Glazkov (Google) 2011-05-05 09:47:48 PDT
Folding into new pretty bug.

*** This bug has been marked as a duplicate of bug 60277 ***