Bug 90094 – [chromium] use WEBKIT_IMPLEMENTATION == 1 for webkit_unit_tests

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Shawn Singh]

It was originally intended that WEBKIT_IMPLEMENTATION would be defined to value 1 for webkit_unit_tests build target.   It turns out that was not the case.  At this point, adding this define will make some code migration much easier, because WEBKIT_IMPLEMENTATION == 1 will enable several WebCore-to-chromium datatype conversions in the code.

This patch adds the WEBKIT_IMPLEMENTATION preprocessor define.   It turns out that some code -- string and URL stuff -- required that WEBKIT_IMPLEMENTATION == 0, and that needed to be refactored/fixed for this patch.  For now, the solution we opted for is to err towards chromium/public/Web* and WebCore types, such as KURL instead of GURL.

Patch coming shortly.

[Shawn Singh]

Created attachment 149809 [details]
Patch

The URL logic had a lot of redundancy that was just slightly different in each place --> a haven for frustrating bugs. So I went ahead and refactored it and cleaned it up, along with this 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.

[Darin Fisher (:fishd, Google)]

It may make sense to have two separate unit test targets:  1) tests that build against the implementation and 2) tests that build only against the API.

I'd probably call the first target webkit_unit_tests and the second webkit_api_tests.

[James Robinson]

Comment on attachment 149809 [details]
Patch

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

Looks good to me.

Adam and I talked about having two test targets a bit yesterday - it sounds like a good idea, but it might get really confusing for someone writing a new test to figure out which sort of thing they were writing and where it should go.  Would they live in different directories?

> Source/WebKit/chromium/public/WebDOMMessageEvent.h:49
> +    WebDOMMessageEvent() { };

no ;

[Shawn Singh]

Darin - is it OK with you to land this for now, and we can continue discussiong the two-target need if you want?

[Darin Fisher (:fishd, Google)]

(In reply to comment #5)
> Darin - is it OK with you to land this for now, and we can continue discussiong the two-target need if you want?

Yes.

[Darin Fisher (:fishd, Google)]

(In reply to comment #4)
...
> Adam and I talked about having two test targets a bit yesterday - it sounds like a good idea, but it might get really confusing for someone writing a new test to figure out which sort of thing they were writing and where it should go.  Would they live in different directories?

Different directories seems like a good idea to me.  The "API tests" would obviously fail to build if you attempted to include WebCore headers.  Those just wouldn't be on the include path.  The "unit tests" would just be what we have today.  We would probably also try to make all integration tests become API tests, and try to encourage all WebCore-using tests to be small unit tests.

[Adam Barth]

Maybe having two test suites is the better approach.  We already have both the TestWebKitAPI (43 tests) and the webkit_unit_tests targets (959 tests).  However, both of them grab at symbols inside WebCore...

[Adam Barth]

Even if we end up with two (I guess three) targets, many of the webkit_unit_tests tests are in the "can see WebCore" category, and so defining WEBKIT_IMPLEMENTATION == 1 makes sense.  This patch might be creating work for us down the road if/when we create the second target, but hopefully not too much work.

[WebKit Review Bot]

Comment on attachment 149809 [details]
Patch

Clearing flags on attachment: 149809

Committed r121405: <http://trac.webkit.org/changeset/121405>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Shawn Singh]

Actually I forgot to address James's comment - I'm really sorry about that.

I don't think there's any need to revert, I'll just re-open this bug to remind me to fix it in a quick follow-up patch tomorrow.

[Shawn Singh]

Created attachment 149974 [details]
Patch

Sorry for the churn; I had to revert anyway because of a windows-only build error.  This patch addresses reviewer's previous comments, and fixes windows build errors on PopupMenuTest.cpp.  Otherwise it is the same.

[WebKit Review Bot]

Comment on attachment 149974 [details]
Patch

Rejecting attachment 149974 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
e/WebKit/chromium/tests/WebFrameTest.cpp: In member function 'virtual void<unnamed>::WebFrameTest_ReloadWithOverrideURLPreservesState_Test::TestBody()':
Source/WebKit/chromium/tests/WebFrameTest.cpp:446: error: 'GURL' was not declared in this scope
  CXX(target) out/Debug/obj.target/webkit_unit_tests/Source/WebKit/chromium/tests/WebPageSerializerTest.o
make: *** [out/Debug/obj.target/webkit_unit_tests/Source/WebKit/chromium/tests/WebFrameTest.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/13121058

[Shawn Singh]

This is just innocent bad timing, another patch landed just now that uses the old way.  I'll just land directly instead, so that this has some bake time before the 5pm outage.

[Shawn Singh]

Committed r121463: <http://trac.webkit.org/changeset/121463>

[Hin-Chung Lam]

Reverted r121463 for reason:

Broke Windows build

Committed r121469: <http://trac.webkit.org/changeset/121469>

[WebKit Review Bot]

Comment on attachment 149974 [details]
Patch

Attachment 149974 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13110174

[Shawn Singh]

update: I intentionally waited because branch was coming this week.   I'm getting a windows machine set up and I'll test this on all platforms before submitting again, probably next week after dust settles from branch.

[Shawn Singh]

(In reply to comment #19)
> update: I intentionally waited because branch was coming this week.   I'm getting a windows machine set up and I'll test this on all platforms before submitting again, probably next week after dust settles from branch.

The patch (updated to tip of tree) seems to work fine for me on VS 2008.  The EWS bots didn't catch the build error, either.  So I have asked the gardener and got permission to land it, expecting that it may break the windows chromium canary bots - if it does, at least we can see the build errors again, and I will revert it.  If we're lucky, perhaps it was a fluke last time and may pass the bots this time, and the patch will stick.  I'll be trying to land it again shortly.

[Shawn Singh]

Created attachment 151725 [details]
Fixed component build setup

Tested on windows debug static/shared builds, mac release/debug static/shared builds, linux release/debug static/shared builds.

[Shawn Singh]

(In reply to comment #21)
> Created an attachment (id=151725) [details]
> Fixed component build setup
> 
> Tested on windows debug static/shared builds, mac release/debug static/shared builds, linux release/debug static/shared builds.

This patch is essentially the same as the ones that jamesr and abarth already gave an r+.   But it was different enough, I felt it needs one more 5-minute review.

Hopefully the build errors are solved now... the frustration was that component build was set up differently than static build and it was not obvious when originally modifying the gyp files.  In particular, webkit_support caused cyclic dependencies with other WebCore stuff when RunAllTests was dependent on both.

The differences between this patch and the previous one:

(1) (minor) a few additional locations where GURL was added since last time the patch landed, changed those instances to toKURL again.

(2) (of interest for reviewers)  changed the WEBKIT_IMPLEMENTATION define so that it is used in webkitUnitTests.gyp for static build, and webkit.gyp for shared library build.  This way, RunAllTests.cpp can actually compile in shared build with its dependencies on webkit_support. This is more consistent with how the existing build setup was arranged.  The Changelog describes what changes are made to each gyp.

(3) added an #if guard to config.h in RunAllTests.cpp, so that it only includes that stuff when statically linking.  when dynamically linking, it doesn't need that include, anyway.

Let's see if this patch sticks!

[Adrienne Walker]

Comment on attachment 151725 [details]
Fixed component build setup

R=me.  Thanks for all the explanation.  Those changes all make sense to me.  Let's give this a try on the bots.  :)

[Shawn Singh]

Looks like this version has passed the webkit chromium canary bots.  It should stick and we can close this bug now.  Thanks for everyone's patience and help =)

The patch landed in two separate CLs:
http://trac.webkit.org/changeset/122344
http://trac.webkit.org/changeset/122348