Bug 42612 – DRT should allow test scripts to access WebCore internal states for setup/verification

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Tools / Tests

[Hajime Morrita]

The initial idea is here: https://lists.webkit.org/pipermail/webkit-dev/2010-July/013559.html.

[Hajime Morrita]

Created attachment 62165 [details]
Patch

[Hajime Morrita]

Created attachment 62166 [details]
rebased

[Early Warning System Bot]

Attachment 62166 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3591310

[WebKit Review Bot]

Attachment 62166 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3589309

[Hajime Morrita]

Created attachment 62168 [details]
build fix

[Hajime Morrita]

Created attachment 62169 [details]
build fix for gtk

[Hajime Morrita]

Created attachment 62260 [details]
made the patch smaller to contain only mac port

[Ojan Vafai]

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?

[Hajime Morrita]

Created attachment 62545 [details]
Patch

[Hajime Morrita]

Created attachment 62547 [details]
Patch

[Hajime Morrita]

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.

[Ojan Vafai]

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);

[Hajime Morrita]

Created attachment 62639 [details]
Patch

[Hajime Morrita]

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.

[Adam Barth]

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.

[Adam Barth]

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.

[Ojan Vafai]

> 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.

[Adam Roben (:aroben)]

It does seem important for this object not to be exposed in production builds. Maybe not in nightly builds, either.

[Adam Barth]

> 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.

[Hajime Morrita]

Created attachment 62801 [details]
Patch

[Hajime Morrita]

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.

[Hajime Morrita]

Created attachment 62805 [details]
rebaselined

[Adam Barth]

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.

[Adam Barth]

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.

[Darin Adler]

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.

[Hajime Morrita]

Created attachment 63403 [details]
Patch

[Hajime Morrita]

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!

[Hajime Morrita]

Created attachment 64186 [details]
Rebaselined. Could anyone take a look?

[Hajime Morrita]

Created attachment 67009 [details]
Patch

[Hajime Morrita]

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!

[WebKit Review Bot]

Attachment 67009 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3966325

[Martin Robinson]

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.

[Hajime Morrita]

Created attachment 67131 [details]
an attempt to fix GTK+ build

[Hajime Morrita]

Comment on attachment 67131 [details]
an attempt to fix GTK+ build

Got obsolete, giving up at now.

[Dimitri Glazkov (Google)]

Folding into new pretty bug.

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