Bug 99772 - [chromium] include WebTestingSupport in the webkit target
Summary: [chromium] include WebTestingSupport in the webkit target
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: jochen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-18 16:20 PDT by jochen
Modified: 2012-10-18 18:13 PDT (History)
4 users (show)

See Also:


Attachments
Patch (6.28 KB, patch)
2012-10-18 16:22 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (4.60 KB, patch)
2012-10-18 17:45 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch for landing (4.44 KB, patch)
2012-10-18 17:53 PDT, jochen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jochen 2012-10-18 16:20:30 PDT
[chromium] include webcore_test_support in the webkit target
Comment 1 jochen 2012-10-18 16:22:48 PDT
Created attachment 169500 [details]
Patch
Comment 2 Adam Barth 2012-10-18 16:30:59 PDT
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.
Comment 3 James Robinson 2012-10-18 16:47:07 PDT
(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?
Comment 4 James Robinson 2012-10-18 16:48:28 PDT
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 5 jochen 2012-10-18 16:50:19 PDT
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
Comment 6 Adam Barth 2012-10-18 16:53:15 PDT
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.
Comment 7 jochen 2012-10-18 17:45:18 PDT
Created attachment 169517 [details]
Patch
Comment 8 Adam Barth 2012-10-18 17:47:29 PDT
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.
Comment 9 jochen 2012-10-18 17:53:27 PDT
Created attachment 169519 [details]
Patch for landing
Comment 10 WebKit Review Bot 2012-10-18 18:13:47 PDT
Comment on attachment 169519 [details]
Patch for landing

Clearing flags on attachment: 169519

Committed r131835: <http://trac.webkit.org/changeset/131835>
Comment 11 WebKit Review Bot 2012-10-18 18:13:51 PDT
All reviewed patches have been landed.  Closing bug.