Bug 109369 – [chromium] Enable more of webkit_unit_tests in component builds

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

[James Robinson]

[chromium] Enable more of webkit_unit_tests in component builds

[James Robinson]

Created attachment 187460 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

This adds a number of APIs to Platform that don't make sense for normal operation, but I think the benefits of running more webkit_unit_tests in component builds is worth it.

This will compile right now, but without https://codereview.chromium.org/12220091/ the tests won't work.

[Darin Fisher (:fishd, Google)]

Comment on attachment 187460 [details]
Patch

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

> Source/Platform/chromium/public/Platform.h:366
> +    virtual void registerMockedURL(const WebURL&, const WebURLResponse&, const WebString& filePath) { }

Would it make sense to lump all these methods together behind a tear-off interface?  That way, in Chrome we could just make the method to get the tear-off return NULL.

> Source/Platform/chromium/public/Platform.h:382
>      // Tracing -------------------------------------------------------------

nit: two new lines above section comment

[James Robinson]

LevelDBTest.cpp is the one holdout.  It uses webkit_support::ScopedTempDirectory / webkit_support::CreateScopedTempDirectory which are thin wrappers around base::ScopedTempDir.  This would be possible to plumb this through as well and remove the component/non-component distinction for webkit_unit_tests completely, but that's probably for another patch.

[James Robinson]

(In reply to comment #4)
> (From update of attachment 187460 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187460&action=review
> 
> > Source/Platform/chromium/public/Platform.h:366
> > +    virtual void registerMockedURL(const WebURL&, const WebURLResponse&, const WebString& filePath) { }
> 
> Would it make sense to lump all these methods together behind a tear-off interface?  That way, in Chrome we could just make the method to get the tear-off return NULL.

Sure.  WebTesting?  WebTestingSupport?

[Darin Fisher (:fishd, Google)]

(In reply to comment #6)
> (In reply to comment #4)
> > (From update of attachment 187460 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=187460&action=review
> > 
> > > Source/Platform/chromium/public/Platform.h:366
> > > +    virtual void registerMockedURL(const WebURL&, const WebURLResponse&, const WebString& filePath) { }
> > 
> > Would it make sense to lump all these methods together behind a tear-off interface?  That way, in Chrome we could just make the method to get the tear-off return NULL.
> 
> Sure.  WebTesting?  WebTestingSupport?

I'd probably go with WebTestingSupport.  We have other Web*Support tear-offs.

[James Robinson]

Created attachment 187462 [details]
with tearoff WebUnitTestSupport interface

[James Robinson]

To keep things running, I'll have to:

1.) Check in the Platform/...h changes without the unit test changes so the header is available in chromium
2.) Roll (1) into chromium
3.) Check in chromium implementation of test functions
4.) Roll (3) into WebKit
5.) Check in WebKit/chromium/tests/... changes

[James Robinson]

(In reply to comment #9)
> To keep things running, I'll have to:
> 

Slight refinement:

1.) Check in the Platform/...h changes without the unit test changes so the header is available in chromium with #define HAVE_WEBUNITTESTSUPPORT
2.) Check in chromium implementation with #include/etc guarded by #if HAVE_WEBUNITTESTSUPPORT
3.) Wait for (1) to roll into (2), roll (2) into WebKit
4.) Check in WebKit/chromium/tests/... changes
5.) Remove #define and #ifs at leisure

[James Robinson]

Committed r142388: <http://trac.webkit.org/changeset/142388>

[James Robinson]

(In reply to comment #11)
> Committed r142388: <http://trac.webkit.org/changeset/142388>

This is the Platform API, still need to land the chromium side + roll it in + land the actual unit test changes.

[James Robinson]

The chromium side change has rolled in, but in local testing I'm finding many of the tests that will now start running in debug+component builds are flaky or buggy.  It seems that pretty much none of the tests in WebFrameTest.cpp clean up properly.  I guess this is somewhat expected given that they haven't run on any bots in debug mode in forever :/

[James Robinson]

Created attachment 187500 [details]
Patch for landing

[James Robinson]

(In reply to comment #14)
> Created an attachment (id=187500) [details]
> Patch for landing

http://www.youtube.com/watch?v=FONN-0uoTHI

[WebKit Review Bot]

Comment on attachment 187500 [details]
Patch for landing

Clearing flags on attachment: 187500

Committed r142422: <http://trac.webkit.org/changeset/142422>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

This is great.  Thanks for doing this.