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.
Created attachment 396142 [details] WIP Patch
Created attachment 396143 [details] WIP Patch
Created attachment 396153 [details] WIP Patch
Created attachment 396154 [details] WIP Patch
Created attachment 396171 [details] Patch
Created attachment 396172 [details] WIP Patch
Created attachment 396180 [details] WIP Patch
Created attachment 396181 [details] WIP Patch
Created attachment 396182 [details] WIP Patch
Created attachment 396184 [details] WIP Patch
Created attachment 396193 [details] WIP Patch
Created attachment 396194 [details] WIP Patch
Created attachment 396314 [details] WIP Patch
Created attachment 396851 [details] WIP Patch
Created attachment 396853 [details] WIP Patch
Created attachment 396854 [details] WIP Patch
Created attachment 396928 [details] WIP Patch
Created attachment 396929 [details] WIP Patch
Created attachment 396931 [details] WIP Patch
Created attachment 397389 [details] WIP Patch
Created attachment 397473 [details] WIP Patch
Created attachment 397475 [details] WIP Patch
Created attachment 397487 [details] WIP Patch
Created attachment 397491 [details] WIP Patch
Created attachment 397498 [details] WIP Patch
Created attachment 397698 [details] WIP Patch
Created attachment 397701 [details] WIP Patch
Created attachment 397703 [details] WIP Patch
Created attachment 397717 [details] WIP Patch
Created attachment 397724 [details] WIP Patch
Created attachment 397726 [details] WIP Patch
Created attachment 397728 [details] WIP Patch
Created attachment 397735 [details] WIP Patch
Created attachment 397747 [details] WIP Patch
Created attachment 397787 [details] WIP Patch
Created attachment 397788 [details] WIP Patch
Created attachment 397790 [details] WIP Patch
Created attachment 397793 [details] WIP Patch
Created attachment 397798 [details] WIP Patch
Created attachment 397799 [details] WIP Patch
Created attachment 397844 [details] WIP Patch
Created attachment 397853 [details] WIP Patch
Created attachment 397874 [details] WIP Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Created attachment 397877 [details] WIP Patch
Created attachment 397881 [details] WIP Patch
Created attachment 398065 [details] WIP Patch
Created attachment 398075 [details] WIP Patch
Does this obsolete bug #198093?
(In reply to Michael Catanzaro from comment #49) > Does this obsolete bug #198093? Yes, it would obsolete it :]
It looks like most ports are green, except iOS and JSCOnly. Stalled on those?
Created attachment 415452 [details] WIP Patch
Created attachment 415454 [details] WIP Patch
Created attachment 415565 [details] WIP Patch
*** Bug 219457 has been marked as a duplicate of this bug. ***
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?
(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 🛬️
Created attachment 415701 [details] WIP Patch
Created attachment 415702 [details] WIP Patch
Created attachment 415703 [details] WIP Patch
Created attachment 415706 [details] WIP Patch
Created attachment 415708 [details] WIP Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Created attachment 415709 [details] WIP Patch
Created attachment 415713 [details] WIP Patch
Created attachment 415715 [details] WIP Patch
Created attachment 415720 [details] WIP Patch
Created attachment 415721 [details] Patch
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?
(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 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 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 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)
(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 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
Created attachment 415922 [details] WIP Patch
Created attachment 415930 [details] No GTK hidden visibility
Created attachment 415932 [details] No GTK hidden visibility
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.
Found https://bugs.webkit.org/show_bug.cgi?id=181916 so I'll just point things at that.
Created attachment 416001 [details] Patch
Committed r270690: <https://trac.webkit.org/changeset/270690> All reviewed patches have been landed. Closing bug and clearing flags on attachment 416001 [details].