Bug 90346 – [Qt] WebCoreTestSupport functions should be exported

• Component: Tools / Tests
• Status: RESOLVED DUPLICATE

[Csaba Osztrogonác]

obj/release/InjectedBundlePage.o: In function `WTR::InjectedBundlePage::resetAfterTest()':
InjectedBundlePage.cpp:(.text._ZN3WTR18InjectedBundlePage14resetAfterTestEv+0x14): undefined reference to `WebCoreTestSupport::resetInternalsObject(OpaqueJSContext const*)'
obj/release/InjectedBundlePage.o: In function `WTR::InjectedBundlePage::didClearWindowForFrame(OpaqueWKBundleFrame const*, OpaqueWKBundleScriptWorld const*)':
InjectedBundlePage.cpp:(.text._ZN3WTR18InjectedBundlePage22didClearWindowForFrameEPK19OpaqueWKBundleFramePK25OpaqueWKBundleScriptWorld+0x140): undefined reference to `WebCoreTestSupport::injectInternalsObject(OpaqueJSContext const*)'

[Balazs Kelemen]

I'm looking into it.

[Balazs Kelemen]

Seems like an export problem, and I have no clue why does it work on other platforms or even qt-x86 (if I recall correctly this is the second time we have such a strange export issue on ARM). I thing we have to export these functions to  be able to use them in WTR.

[Balazs Kelemen]

Here is the diff on Qt:

-viewport size 320x356 scale 1.000000 with limits [1.000000, 5.000000] and userScalable -1.000000
+viewport size 480x534 scale 1.000000 with limits [1.000000, 5.000000] and userScalable -1.000000

Relevant parts of the test:
<meta name="viewport" content="width=device-width">
layoutTestController.dumpConfigurationForViewport(240, 480, 564, 480, 534);

So we don't divide the width and height dimensions with 1.5 (240/160), which is surprising, since the patch keeps the division, just organize it to the call sites.

[Balazs Kelemen]

(In reply to comment #3)
> Here is the diff on Qt:
> 
> -viewport size 320x356 scale 1.000000 with limits [1.000000, 5.000000] and userScalable -1.000000
> +viewport size 480x534 scale 1.000000 with limits [1.000000, 5.000000] and userScalable -1.000000
> 
> Relevant parts of the test:
> <meta name="viewport" content="width=device-width">
> layoutTestController.dumpConfigurationForViewport(240, 480, 564, 480, 534);
> 
> So we don't divide the width and height dimensions with 1.5 (240/160), which is surprising, since the patch keeps the division, just organize it to the call sites.

Ups, wrong bug...

[Balazs Kelemen]

Created attachment 150432 [details]
Patch

[Simon Hausmann]

Comment on attachment 150432 [details]
Patch

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

I think this is the right solution, exporting those symbols via export macros. But a little bit of fine tuning is needed :)

> Source/WebCore/ChangeLog:8
> +        [Qt] REGRESSION(r121550): It broke the Qt5 ARM build
> +        https://bugs.webkit.org/show_bug.cgi?id=90346
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        No new tests, this is a build fix.

I suggest to correct the title and the description of the change log entry here.

The title makes it look like that this is an ARM specific problem, but that is incorrect. It is a general build problem of the WK2 part of the Qt port for platforms that require symbol exports. This is the line in default_post.prf that controls this:

    !linux-g++*:!linux-icc*:contains(QT_CONFIG, reduce_exports):CONFIG += hide_symbols

and as it so happens the arm mkspec doesn't match linux-g++*. But _that_ line itself is incorrect and
I should've removed the linux-g++* part as part of r106650. Then this problem would manifest itself also
on the x86 WK2 bot.

I think the bug description should briefly include how the patch accomplishes to fix the issue.

> Source/WebCore/testing/js/WebCoreTestSupport.h:35
> +#include <wtf/ExportMacros.h>
> +
> +#if PLATFORM(QT)
> +#define TEST_SUPPORT_EXPORT WTF_EXPORT_PRIVATE
> +#else
> +#define TEST_SUPPORT_EXPORT
> +#endif

I think those belong into WebCore/platform/PlatformExportMacros.h and maybe should be prefixed with WEBKIT,
i.e. WEBKIT_EXPORT_TEST_SUPPORT.

[Balazs Kelemen]

Created attachment 150627 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 150627 [details]
Patch

Attachment 150627 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13130514

[Gustavo Noronha (kov)]

Comment on attachment 150627 [details]
Patch

Attachment 150627 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13136453

[Balazs Kelemen]

Created attachment 150629 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 150629 [details]
Patch

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

> Source/WebCore/platform/PlatformExportMacros.h:76
> +// Special cases.
> +
> +#if PLATFORM(QT)
> +#if defined(BUILDING_WebCore) || defined(BUILDING_WebKit) || \
> +    defined(STATICALLY_LINKED_WITH_WebCore) || defined(STATICALLY_LINKED_WITH_WebKit)
> +#define WEBKIT_TEST_SUPPORT_EXPORT WTF_EXPORT
> +#else
> +#define WEBKIT_TEST_SUPPORT_EXPORT WTF_IMPORT
> +#endif
> +#else
> +#define WEBKIT_TEST_SUPPORT_EXPORT
> +#endif // PLATFORM(QT)
> +

I would prefer something like this just after "#endif // USE(EXPORT_MACROS)":
#if PLATFORM(QT)
#define WEBKIT_TEST_SUPPORT_EXPORT WEBKIT_EXPORTDATA
#else
#define WEBKIT_TEST_SUPPORT_EXPORT
#endif // PLATFORM(QT)

[Balazs Kelemen]

It turned out that other ports have a special lib for these things, for example on Gtk it is libWebCoreTestSupport. The better solution is to set this up for Qt as well.

[Build Bot]

Comment on attachment 150629 [details]
Patch

Attachment 150629 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13134487

[Balazs Kelemen]

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

[Simon Hausmann]

(In reply to comment #6)
[...]
> The title makes it look like that this is an ARM specific problem, but that is incorrect. It is a general build problem of the WK2 part of the Qt port for platforms that require symbol exports. This is the line in default_post.prf that controls this:
> 
>     !linux-g++*:!linux-icc*:contains(QT_CONFIG, reduce_exports):CONFIG += hide_symbols
> 
> and as it so happens the arm mkspec doesn't match linux-g++*. But _that_ line itself is incorrect and
> I should've removed the linux-g++* part as part of r106650. Then this problem would manifest itself also
> on the x86 WK2 bot.
> 
> I think the bug description should briefly include how the patch accomplishes to fix the issue.

I've filed bug #90981 to address this.

[Balazs Kelemen]

Let's start with this and do the separation later. It seems like a bit trickier than I expected (idl-s for example).

[Balazs Kelemen]

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