Bug 99772 – [chromium] include WebTestingSupport in the webkit target

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[chromium] include webcore_test_support in the webkit target

Created attachment 169500 [details]
Patch

[Adam Barth]

Comment on attachment 169500 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169500&action=review

> Source/WebKit/chromium/WebKit.gyp:72
> +                '../../WebCore/WebCore.gyp/WebCore.gyp:webcore_test_support',

I don't understand.  We don't want to link this code in production.  That's why we have it as a separate dependency.

[James Robinson]

(In reply to comment #2)
> (From update of attachment 169500 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169500&action=review
> 
> > Source/WebKit/chromium/WebKit.gyp:72
> > +                '../../WebCore/WebCore.gyp/WebCore.gyp:webcore_test_support',
> 
> I don't understand.  We don't want to link this code in production.  That's why we have it as a separate dependency.

I suggested jochen move this.  The idea is to expose this functionality to the content_shell-based DRT implementation.  Is there a reason this is guarded in particular vs other testing APIs we expose through WebKit API?

[James Robinson]

I don't think we have a way to distinguish "WebKit.dll being used for production" from "WebKit.dll being used for content_shell based DRT".  If layout tests rely on this functionality (which they do) and we insist on not making this available on WebKit.dll then there's not going to be any way to run layout tests with a content_shell based executable.

Comment on attachment 169500 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169500&action=review

>> Source/WebKit/chromium/WebKit.gyp:72
>> +                '../../WebCore/WebCore.gyp/WebCore.gyp:webcore_test_support',
> 
> I don't understand.  We don't want to link this code in production.  That's why we have it as a separate dependency.

The problem with the separate dependency is that it's not always compiled.

Also, why is it so bad to ship this code? It has to be explicitly enabled to do something

[Adam Barth]

WebKit.dll is never used in production.  We do link webcore_test_support into WebKit.dll today, but not into the targets that depend on WebKit in the non-component build.

Does that mean we're going to link TestRunner.a into production builds as well?  We might need to create a new top-level target that is ContentShell + TestRunner.  Maybe we should call it WebKitTestRunner since it is the moral equivalent of http://trac.webkit.org/browser/trunk/Tools/WebKitTestRunner ?

> Also, why is it so bad to ship this code? It has to be explicitly enabled to do something

There's no reason to ship test-only code to production.  It just bloats the binary and introduces security risks.

Created attachment 169517 [details]
Patch

[Adam Barth]

Comment on attachment 169517 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169517&action=review

We could alos make this a dependency of TestRunner once we build that as a separate lib.

> Source/WebKit/chromium/WebKit.gyp:876
> +                # WARNING: Do not view this particular case as a precedent for
> +                # including WebCore headers in DumpRenderTree project.

This warning seems unneeded now.

Created attachment 169519 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 169519 [details]
Patch for landing

Clearing flags on attachment: 169519

Committed r131835: <http://trac.webkit.org/changeset/131835>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.