[webkit-reviews] review denied: [Bug 90346] [Qt] REGRESSION(r121550): It broke the Qt5 ARM build : [Attachment 150432] Patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Balazs Kelemen
<kbalazs at webkit.org>'s request for review:
Bug 90346: [Qt] REGRESSION(r121550): It broke the Qt5 ARM build
https://bugs.webkit.org/show_bug.cgi?id=90346

Attachment 150432: Patch
https://bugs.webkit.org/attachment.cgi?id=150432&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
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.