Bug 210366 - [CMake] Determine correct visibility for linked frameworks
Summary: [CMake] Determine correct visibility for linked frameworks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CMake (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Don Olmstead
URL:
Keywords:
: 219457 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-04-10 17:58 PDT by Don Olmstead
Modified: 2020-12-11 11:16 PST (History)
29 users (show)

See Also:


Attachments
WIP Patch (7.75 KB, patch)
2020-04-10 18:19 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (8.26 KB, patch)
2020-04-10 19:15 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (8.48 KB, patch)
2020-04-10 22:13 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (8.48 KB, patch)
2020-04-10 22:19 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (8.87 KB, patch)
2020-04-11 10:26 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (9.36 KB, patch)
2020-04-11 10:34 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (11.16 KB, patch)
2020-04-11 11:50 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (11.25 KB, patch)
2020-04-11 11:55 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (11.25 KB, patch)
2020-04-11 12:01 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (12.09 KB, patch)
2020-04-11 12:06 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (12.60 KB, patch)
2020-04-11 15:35 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (12.77 KB, patch)
2020-04-11 15:51 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (16.22 KB, patch)
2020-04-13 12:07 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (14.78 KB, patch)
2020-04-18 09:39 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (15.25 KB, patch)
2020-04-18 09:48 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (16.68 KB, patch)
2020-04-18 10:01 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (15.07 KB, patch)
2020-04-19 16:23 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (16.27 KB, patch)
2020-04-19 17:09 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (16.29 KB, patch)
2020-04-19 18:02 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (17.38 KB, patch)
2020-04-23 15:05 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (17.47 KB, patch)
2020-04-24 11:16 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (18.15 KB, patch)
2020-04-24 11:40 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (18.20 KB, patch)
2020-04-24 12:36 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (18.25 KB, patch)
2020-04-24 12:58 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (18.73 KB, patch)
2020-04-24 13:32 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (19.27 KB, patch)
2020-04-27 10:37 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (19.75 KB, patch)
2020-04-27 10:50 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (19.78 KB, patch)
2020-04-27 11:07 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (25.61 KB, patch)
2020-04-27 12:30 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (26.91 KB, patch)
2020-04-27 13:10 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (26.90 KB, patch)
2020-04-27 13:15 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (27.30 KB, patch)
2020-04-27 13:30 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (28.45 KB, patch)
2020-04-27 14:13 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (30.38 KB, patch)
2020-04-27 14:58 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (32.72 KB, patch)
2020-04-27 19:18 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (33.03 KB, patch)
2020-04-27 19:30 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (33.68 KB, patch)
2020-04-27 20:10 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (33.79 KB, patch)
2020-04-27 20:36 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (34.82 KB, patch)
2020-04-27 21:22 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (35.00 KB, patch)
2020-04-27 21:27 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (36.28 KB, patch)
2020-04-28 09:26 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (37.92 KB, patch)
2020-04-28 10:18 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (38.62 KB, patch)
2020-04-28 12:42 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (38.75 KB, patch)
2020-04-28 13:15 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (40.24 KB, patch)
2020-04-28 13:56 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (41.75 KB, patch)
2020-04-30 10:42 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (42.09 KB, patch)
2020-04-30 12:13 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (16.26 KB, patch)
2020-12-04 13:08 PST, Don Olmstead
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP Patch (17.87 KB, patch)
2020-12-04 13:40 PST, Don Olmstead
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP Patch (21.33 KB, patch)
2020-12-07 11:06 PST, Don Olmstead
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP Patch (28.62 KB, patch)
2020-12-08 19:40 PST, Don Olmstead
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP Patch (32.74 KB, patch)
2020-12-08 20:10 PST, Don Olmstead
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP Patch (32.91 KB, patch)
2020-12-08 20:22 PST, Don Olmstead
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP Patch (33.31 KB, patch)
2020-12-08 20:36 PST, Don Olmstead
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP Patch (34.90 KB, patch)
2020-12-08 21:23 PST, Don Olmstead
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP Patch (35.04 KB, patch)
2020-12-08 21:29 PST, Don Olmstead
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP Patch (37.44 KB, patch)
2020-12-08 22:03 PST, Don Olmstead
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP Patch (38.18 KB, patch)
2020-12-08 22:19 PST, Don Olmstead
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP Patch (38.94 KB, patch)
2020-12-08 22:55 PST, Don Olmstead
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (39.61 KB, patch)
2020-12-08 23:11 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
WIP Patch (26.91 KB, patch)
2020-12-10 13:36 PST, Don Olmstead
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
No GTK hidden visibility (25.80 KB, patch)
2020-12-10 14:17 PST, Don Olmstead
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
No GTK hidden visibility (26.35 KB, patch)
2020-12-10 14:37 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (27.82 KB, patch)
2020-12-11 09:32 PST, Don Olmstead
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Don Olmstead 2020-04-10 17:58:29 PDT
Each port can set ${framework}_LIBRARY_TYPE to whatever value it wants. This leads to issues with linking frameworks within the WebKit code. For example JavaScriptCore can be compiled as a shared library with the contents of WTF and bmalloc exposed through it.

The CMake source should be able to handle cases like this.
Comment 1 Don Olmstead 2020-04-10 18:19:45 PDT Comment hidden (obsolete)
Comment 2 Don Olmstead 2020-04-10 19:15:26 PDT Comment hidden (obsolete)
Comment 3 Don Olmstead 2020-04-10 22:13:54 PDT Comment hidden (obsolete)
Comment 4 Don Olmstead 2020-04-10 22:19:38 PDT Comment hidden (obsolete)
Comment 5 Don Olmstead 2020-04-11 10:26:10 PDT Comment hidden (obsolete)
Comment 6 Don Olmstead 2020-04-11 10:34:20 PDT Comment hidden (obsolete)
Comment 7 Don Olmstead 2020-04-11 11:50:57 PDT Comment hidden (obsolete)
Comment 8 Don Olmstead 2020-04-11 11:55:40 PDT Comment hidden (obsolete)
Comment 9 Don Olmstead 2020-04-11 12:01:05 PDT Comment hidden (obsolete)
Comment 10 Don Olmstead 2020-04-11 12:06:02 PDT Comment hidden (obsolete)
Comment 11 Don Olmstead 2020-04-11 15:35:08 PDT Comment hidden (obsolete)
Comment 12 Don Olmstead 2020-04-11 15:51:04 PDT Comment hidden (obsolete)
Comment 13 Don Olmstead 2020-04-13 12:07:25 PDT Comment hidden (obsolete)
Comment 14 Don Olmstead 2020-04-18 09:39:08 PDT Comment hidden (obsolete)
Comment 15 Don Olmstead 2020-04-18 09:48:38 PDT Comment hidden (obsolete)
Comment 16 Don Olmstead 2020-04-18 10:01:22 PDT Comment hidden (obsolete)
Comment 17 Don Olmstead 2020-04-19 16:23:53 PDT Comment hidden (obsolete)
Comment 18 Don Olmstead 2020-04-19 17:09:15 PDT Comment hidden (obsolete)
Comment 19 Don Olmstead 2020-04-19 18:02:30 PDT Comment hidden (obsolete)
Comment 20 Don Olmstead 2020-04-23 15:05:37 PDT Comment hidden (obsolete)
Comment 21 Don Olmstead 2020-04-24 11:16:47 PDT Comment hidden (obsolete)
Comment 22 Don Olmstead 2020-04-24 11:40:39 PDT Comment hidden (obsolete)
Comment 23 Don Olmstead 2020-04-24 12:36:50 PDT Comment hidden (obsolete)
Comment 24 Don Olmstead 2020-04-24 12:58:38 PDT Comment hidden (obsolete)
Comment 25 Don Olmstead 2020-04-24 13:32:45 PDT Comment hidden (obsolete)
Comment 26 Don Olmstead 2020-04-27 10:37:43 PDT Comment hidden (obsolete)
Comment 27 Don Olmstead 2020-04-27 10:50:35 PDT Comment hidden (obsolete)
Comment 28 Don Olmstead 2020-04-27 11:07:05 PDT Comment hidden (obsolete)
Comment 29 Don Olmstead 2020-04-27 12:30:14 PDT Comment hidden (obsolete)
Comment 30 Don Olmstead 2020-04-27 13:10:42 PDT Comment hidden (obsolete)
Comment 31 Don Olmstead 2020-04-27 13:15:29 PDT Comment hidden (obsolete)
Comment 32 Don Olmstead 2020-04-27 13:30:39 PDT Comment hidden (obsolete)
Comment 33 Don Olmstead 2020-04-27 14:13:23 PDT Comment hidden (obsolete)
Comment 34 Don Olmstead 2020-04-27 14:58:54 PDT Comment hidden (obsolete)
Comment 35 Don Olmstead 2020-04-27 19:18:11 PDT Comment hidden (obsolete)
Comment 36 Don Olmstead 2020-04-27 19:30:21 PDT Comment hidden (obsolete)
Comment 37 Don Olmstead 2020-04-27 20:10:57 PDT Comment hidden (obsolete)
Comment 38 Don Olmstead 2020-04-27 20:36:10 PDT Comment hidden (obsolete)
Comment 39 Don Olmstead 2020-04-27 21:22:58 PDT Comment hidden (obsolete)
Comment 40 Don Olmstead 2020-04-27 21:27:21 PDT Comment hidden (obsolete)
Comment 41 Don Olmstead 2020-04-28 09:26:46 PDT Comment hidden (obsolete)
Comment 42 Don Olmstead 2020-04-28 10:18:52 PDT Comment hidden (obsolete)
Comment 43 Don Olmstead 2020-04-28 12:42:26 PDT Comment hidden (obsolete)
Comment 44 EWS Watchlist 2020-04-28 12:43:00 PDT Comment hidden (obsolete)
Comment 45 Don Olmstead 2020-04-28 13:15:44 PDT Comment hidden (obsolete)
Comment 46 Don Olmstead 2020-04-28 13:56:02 PDT Comment hidden (obsolete)
Comment 47 Don Olmstead 2020-04-30 10:42:04 PDT Comment hidden (obsolete)
Comment 48 Don Olmstead 2020-04-30 12:13:17 PDT Comment hidden (obsolete)
Comment 49 Michael Catanzaro 2020-05-07 12:03:33 PDT
Does this obsolete bug #198093?
Comment 50 Adrian Perez 2020-05-11 03:17:41 PDT
(In reply to Michael Catanzaro from comment #49)
> Does this obsolete bug #198093?

Yes, it would obsolete it :]
Comment 51 Michael Catanzaro 2020-06-12 06:44:20 PDT
It looks like most ports are green, except iOS and JSCOnly. Stalled on those?
Comment 52 Don Olmstead 2020-12-04 13:08:02 PST Comment hidden (obsolete)
Comment 53 Don Olmstead 2020-12-04 13:40:35 PST Comment hidden (obsolete)
Comment 54 Don Olmstead 2020-12-07 11:06:33 PST Comment hidden (obsolete)
Comment 55 Adrian Perez 2020-12-08 05:02:31 PST
*** Bug 219457 has been marked as a duplicate of this bug. ***
Comment 56 Michael Catanzaro 2020-12-08 06:25:48 PST
Looking at the linked bugs, can we also get rid of ADD_WHOLE_ARCHIVE_TO_LIBRARIES? Or reduce its scope such that it's defined only right before it is used?
Comment 57 Adrian Perez 2020-12-08 07:55:11 PST
(In reply to Michael Catanzaro from comment #56)
> Looking at the linked bugs, can we also get rid of
> ADD_WHOLE_ARCHIVE_TO_LIBRARIES? Or reduce its scope such that it's defined
> only right before it is used?

Yes, I think we should (and could) get rid of it. I am trying to pinpoint
why the GTK bot does not seem happy, let's see if we manage to help Don
get this landed 🛬️
Comment 58 Don Olmstead 2020-12-08 19:40:06 PST Comment hidden (obsolete)
Comment 59 Don Olmstead 2020-12-08 20:10:03 PST Comment hidden (obsolete)
Comment 60 Don Olmstead 2020-12-08 20:22:01 PST Comment hidden (obsolete)
Comment 61 Don Olmstead 2020-12-08 20:36:22 PST Comment hidden (obsolete)
Comment 62 Don Olmstead 2020-12-08 21:23:38 PST Comment hidden (obsolete)
Comment 63 EWS Watchlist 2020-12-08 21:24:40 PST Comment hidden (obsolete)
Comment 64 Don Olmstead 2020-12-08 21:29:33 PST Comment hidden (obsolete)
Comment 65 Don Olmstead 2020-12-08 22:03:47 PST Comment hidden (obsolete)
Comment 66 Don Olmstead 2020-12-08 22:19:51 PST Comment hidden (obsolete)
Comment 67 Don Olmstead 2020-12-08 22:55:17 PST Comment hidden (obsolete)
Comment 68 Don Olmstead 2020-12-08 23:11:47 PST
Created attachment 415721 [details]
Patch
Comment 69 Don Olmstead 2020-12-08 23:28:24 PST
Comment on attachment 415721 [details]
Patch

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

So GTK and WPE are green! 🎊

The additional exports can probably be split into its own patch and as noted the TextChecker thing needs a resolution.

GTK folks should probably run the tests and everything though as is to make sure that nothing fun cropped up. Other than that though GTK builds with hidden visibility!

> Source/WebKit/NetworkProcess/soup/NetworkProcessMainSoup.cpp:37
> +#if USE(GCRYPT)
> +#include <pal/crypto/gcrypt/Initialization.h>
> +#endif
> +

Same reasoning as WebProcess

> Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp:53
> +#if USE(GCRYPT)
> +        PAL::GCrypt::initialize();
> +#endif
> +

I moved the `USE(GCRYPT)` in here since it makes more sense but also because of WebKit linking WebCore/PAL as PRIVATE libraries so the gcrypt lib and includes don't propagate to the WebProcess build.

> Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp:81
> +    // I think this needs to be in a WK API
> +    //WebKit::TextChecker::setContinuousSpellCheckingEnabled(enabled);

I'm remembering that doing a `WK_EXPORT` on that static method causes the Mac builds to fail. Looking at what the Apple stuff is doing you all might want to have some kind of internal API that you can do `WK_EXPORT` and call that instead. That something the GTK folks can handle here?
Comment 70 Adrian Perez 2020-12-09 07:22:06 PST
(In reply to Don Olmstead from comment #69)
> Comment on attachment 415721 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415721&action=review
> 
> So GTK and WPE are green! 🎊
> 
> The additional exports can probably be split into its own patch and as noted
> the TextChecker thing needs a resolution.
> 
> GTK folks should probably run the tests and everything though as is to make
> sure that nothing fun cropped up. Other than that though GTK builds with
> hidden visibility!

I'm making a build and will run a full run, to see where we are, but I do
not expect (much?) trouble.

> > Source/WebKit/NetworkProcess/soup/NetworkProcessMainSoup.cpp:37
> > +#if USE(GCRYPT)
> > +#include <pal/crypto/gcrypt/Initialization.h>
> > +#endif
> > +
> 
> Same reasoning as WebProcess
> 
> > Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp:53
> > +#if USE(GCRYPT)
> > +        PAL::GCrypt::initialize();
> > +#endif
> > +
> 
> I moved the `USE(GCRYPT)` in here since it makes more sense but also because
> of WebKit linking WebCore/PAL as PRIVATE libraries so the gcrypt lib and
> includes don't propagate to the WebProcess build.

This looks sensible to me.

It's actually kind of amusing that this was not done before; I wonder
if the GCrypt initialization code might predate the existence of an
overridable platformInitialize() method 🤔️

> > Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp:81
> > +    // I think this needs to be in a WK API
> > +    //WebKit::TextChecker::setContinuousSpellCheckingEnabled(enabled);
> 
> I'm remembering that doing a `WK_EXPORT` on that static method causes the
> Mac builds to fail. Looking at what the Apple stuff is doing you all might
> want to have some kind of internal API that you can do `WK_EXPORT` and call
> that instead. That something the GTK folks can handle here?

I'll take a look.
Comment 71 Michael Catanzaro 2020-12-09 08:29:35 PST
Comment on attachment 415721 [details]
Patch

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

My only concern is that we need to make sure this does not bring back bug #179914. We can check that by:

 (a) running $ Tools/Scripts/check-for-global-bss-symbols-in-webkigtk-libs _build/lib/libjavascriptcoregtk-4.0.so _build/lib/libwebkit2gtk-4.0.so. (Note: type fixed in bug #219686)
 (b) Sanity-check: add WTFLogAlways() to the PerProcess<> constructor to make sure these objects are still created exactly once and not twice.

Then I'm not sure if it makes sense to keep webkitglib-symbols.map. I guess it doesn't hurt anything, in that it's an extra layer of protection to ensure we don't export things in release builds by mistake. But it also prevents us from running API tests in release builds. So maybe we should delete it.

> Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp:75
> +    // Ignore the GTK_THEME environment variable, the theme is always set by the UI process now.
> +    unsetenv("GTK_THEME");

Nit: I would leave this where it was, since unsetenv is very dangerous, and it's more clear that it comes at the very top of main when there are not objects like WebProcessMain that have to be read and understood to see that.
Comment 72 Michael Catanzaro 2020-12-09 10:25:28 PST
Comment on attachment 415721 [details]
Patch

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

>> Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp:75
>> +    // Ignore the GTK_THEME environment variable, the theme is always set by the UI process now.
>> +    unsetenv("GTK_THEME");
> 
> Nit: I would leave this where it was, since unsetenv is very dangerous, and it's more clear that it comes at the very top of main when there are not objects like WebProcessMain that have to be read and understood to see that.

Actually, hi Carlos, is this obsolete? Since the web process doesn't use the GTK theme anymore?
Comment 73 Adrian Perez 2020-12-09 10:46:24 PST
Comment on attachment 415721 [details]
Patch

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

> Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp:76
> +

I *think* that we might be able to remove this now that we are not using
the GTK external rendering anymore for controls and we let WebKit paint
them.

At any rate, not a blocker for landing. I'll double check with Carlos
García, and we may was well zap this in a follow-up patch.

> Source/cmake/OptionsGTK.cmake:28
> +    set(CMAKE_CXX_VISIBILITY_PRESET hidden)

We should probably throw in also a

    set(CMAKE_C_VISIBILITY_PRESET hidden)

as we have a few plain C files which are part of the build as well.

>>> Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp:81
>>> +    //WebKit::TextChecker::setContinuousSpellCheckingEnabled(enabled);
>> 
>> I'm remembering that doing a `WK_EXPORT` on that static method causes the Mac builds to fail. Looking at what the Apple stuff is doing you all might want to have some kind of internal API that you can do `WK_EXPORT` and call that instead. That something the GTK folks can handle here?
> 
> I'll take a look.

I think we can get away with using the public API, because at this point
in the code we know for a fact that the web view being used is a GTK
WebKitWebView. I did this locally, and currently running tests, I'll let
you know how it goes:


--- a/Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp
+++ b/Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp
@@ -77,8 +77,9 @@ void UIScriptControllerGtk::doAsyncTask(JSValueRef callback)

 void UIScriptControllerGtk::setContinuousSpellCheckingEnabled(bool enabled)
 {
-    // I think this needs to be in a WK API
-    //WebKit::TextChecker::setContinuousSpellCheckingEnabled(enabled);
+    auto* webView = TestController::singleton().mainWebView();
+    auto* webContext = webkit_web_view_get_context(webView);
+    webkit_web_context_set_spell_checking_enabled(webContext, enabled ? TRUE : FALSE);
 }

 void UIScriptControllerGtk::copyText(JSStringRef text)
Comment 74 Michael Catanzaro 2020-12-09 11:35:42 PST
(In reply to Adrian Perez from comment #73)
> We should probably throw in also a
> 
>     set(CMAKE_C_VISIBILITY_PRESET hidden)
> 
> as we have a few plain C files which are part of the build as well.

Oops, yes please. :)
Comment 75 Carlos Garcia Campos 2020-12-10 00:49:24 PST
Comment on attachment 415721 [details]
Patch

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

>> Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp:76
>> +
> 
> I *think* that we might be able to remove this now that we are not using
> the GTK external rendering anymore for controls and we let WebKit paint
> them.
> 
> At any rate, not a blocker for landing. I'll double check with Carlos
> García, and we may was well zap this in a follow-up patch.

Note that we sill use GtkStyleContext in GTK3 to get the selection colors

>>>> Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp:81
>>>> +    //WebKit::TextChecker::setContinuousSpellCheckingEnabled(enabled);
>>> 
>>> I'm remembering that doing a `WK_EXPORT` on that static method causes the Mac builds to fail. Looking at what the Apple stuff is doing you all might want to have some kind of internal API that you can do `WK_EXPORT` and call that instead. That something the GTK folks can handle here?
>> 
>> I'll take a look.
> 
> I think we can get away with using the public API, because at this point
> in the code we know for a fact that the web view being used is a GTK
> WebKitWebView. I did this locally, and currently running tests, I'll let
> you know how it goes:
> 
> 
> --- a/Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp
> +++ b/Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp
> @@ -77,8 +77,9 @@ void UIScriptControllerGtk::doAsyncTask(JSValueRef callback)
> 
>  void UIScriptControllerGtk::setContinuousSpellCheckingEnabled(bool enabled)
>  {
> -    // I think this needs to be in a WK API
> -    //WebKit::TextChecker::setContinuousSpellCheckingEnabled(enabled);
> +    auto* webView = TestController::singleton().mainWebView();
> +    auto* webContext = webkit_web_view_get_context(webView);
> +    webkit_web_context_set_spell_checking_enabled(webContext, enabled ? TRUE : FALSE);
>  }
> 
>  void UIScriptControllerGtk::copyText(JSStringRef text)

I don't this that works, in WTR there's not a WebKitWebView, it's a WebKitWebViewBase
Comment 76 Don Olmstead 2020-12-10 13:36:32 PST
Created attachment 415922 [details]
WIP Patch
Comment 77 Don Olmstead 2020-12-10 14:17:06 PST Comment hidden (obsolete)
Comment 78 Don Olmstead 2020-12-10 14:37:10 PST
Created attachment 415932 [details]
No GTK hidden visibility
Comment 79 Don Olmstead 2020-12-10 19:05:30 PST
Comment on attachment 415932 [details]
No GTK hidden visibility

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

I have https://bugs.webkit.org/show_bug.cgi?id=219749 with the gcrypt changes ready for review. Other than that I feel this patch is about ready for review so feel free to leave comments on it.

> Source/WebCore/PlatformGTK.cmake:16
> +# Remove when turning on hidden visibility
> +list(APPEND WebCore_PRIVATE_LIBRARIES WebKit::WTF)
> +if (NOT USE_SYSTEM_MALLOC)
> +    list(APPEND WebCore_PRIVATE_LIBRARIES WebKit::bmalloc)
> +endif ()
> +

So at this time it looks like GTK isn't ready to turn on hidden visibility so I added this in.

Could we open a bug for turning on hidden visibility for GTK and potentially land this with a FIXME pointing at that bug.

I'm happy to work with Adrian or whoever on getting hidden visibility working.
Comment 80 Don Olmstead 2020-12-11 08:00:53 PST
Found https://bugs.webkit.org/show_bug.cgi?id=181916 so I'll just point things at that.
Comment 81 Don Olmstead 2020-12-11 09:32:59 PST
Created attachment 416001 [details]
Patch
Comment 82 EWS 2020-12-11 11:16:09 PST
Committed r270690: <https://trac.webkit.org/changeset/270690>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416001 [details].