This is a follow-up from bug #181916. We should just need to add this to OptionsWPE.cmake: set(CMAKE_C_VISIBILITY_PRESET hidden) set(CMAKE_CXX_VISIBILITY_PRESET hidden) set(CMAKE_VISIBILITY_INLINES_HIDDEN ON) But first we need to add more exports until it builds. E.g. the entire QtWPE API is not exported.
WDYT Adrian?
In https://bugs.webkit.org/show_bug.cgi?id=181916#c58 Carlos Lopez found that -fvisibility=hidden resulted in a major performance improvement for GTK. I would expect similar improvement for WPE. WPE used to use -fvisibility=hidden until I removed it stupidly in r224642. I assumed it was redundant with using a linker version script, but in fact we really want both. Sorry about that.
(In reply to Michael Catanzaro from comment #0) > But first we need to add more exports until it builds. E.g. the entire QtWPE > API is not exported. I'm not sure that's true anymore, I see some classes are exported now. Anyway, I failed to build QtWPE due to bug #231369. The rest of WPE build successfully. Let's see if the EWS is happy.
Created attachment 440506 [details] Patch
Comment on attachment 440506 [details] Patch Looks like the QtWPE API does indeed need more work.
Naive build fix: http://sprunge.us/pYbFZg
(In reply to Philippe Normand from comment #6) > Naive build fix: http://sprunge.us/pYbFZg Cool, thanks. Except WPEQtViewLoadRequest.h is public API, so we can't use WebKit internals there. That will break the public headers since WKDeclarationSpecifiers.h is not installed. Looks like we should use Q_DECL_EXPORT there.
(In reply to Michael Catanzaro from comment #7) > Except WPEQtViewLoadRequest.h is public API, so we can't use WebKit > internals there. That will break the public headers since > WKDeclarationSpecifiers.h is not installed. Looks like we should use > Q_DECL_EXPORT there. Wait, WPEQtView.h is public too, of course, so we should use Q_DECL_EXPORT there too. But that header is actually already using GRefPtr.h. How is that possible? GRefPtr.h is not a public header and should not be installed, but WPEQtView.h is installed. ?????????
(In reply to Michael Catanzaro from comment #7) > (In reply to Philippe Normand from comment #6) > > Naive build fix: http://sprunge.us/pYbFZg > > Cool, thanks. > > Except WPEQtViewLoadRequest.h is public API, so we can't use WebKit > internals there. That will break the public headers since > WKDeclarationSpecifiers.h is not installed. Looks like we should use > Q_DECL_EXPORT there. Good point! (In reply to Michael Catanzaro from comment #8) > (In reply to Michael Catanzaro from comment #7) > > Except WPEQtViewLoadRequest.h is public API, so we can't use WebKit > > internals there. That will break the public headers since > > WKDeclarationSpecifiers.h is not installed. Looks like we should use > > Q_DECL_EXPORT there. > > Wait, WPEQtView.h is public too, of course, so we should use Q_DECL_EXPORT > there too. But that header is actually already using GRefPtr.h. How is that > possible? GRefPtr.h is not a public header and should not be installed, but > WPEQtView.h is installed. ????????? I think that means no one tried to use this API. The couple folks I heard from were using the QML modules, not the C++ API.
Created attachment 440603 [details] Patch
Comment on attachment 440603 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440603&action=review > Source/cmake/OptionsWPE.cmake:151 > +set(CMAKE_C_VISIBILITY_PRESET hidden) > +set(CMAKE_CXX_VISIBILITY_PRESET hidden) > +set(CMAKE_VISIBILITY_INLINES_HIDDEN ON) Let's land this in two steps: (1) Everything except this portion of the diff, r=me (2) This part, in a second commit. That way, we don't have to revert (1) if it turns out there is some problem caused by (2).
Alright, I'll fork in a separate bug!
(In reply to Philippe Normand from comment #12) > Alright, I'll fork in a separate bug! https://bugs.webkit.org/show_bug.cgi?id=231430
So only remaining problem is we need to export WebKit::TextChecker::setContinuousSpellCheckingEnabled for WKTR, but for some reason this breaks the iOS build. It might be doing some sort of check to ensure that all exported symbols correspond to something in a public header, which is pretty strict. Proposal: let's create and export WKTextCheckerSetContinuousSpellCheckingEnabled instead. It already exists on GTK, and we can easily add a WPE version.
(In reply to Michael Catanzaro from comment #14) > Proposal: let's create and export > WKTextCheckerSetContinuousSpellCheckingEnabled instead. It already exists on > GTK, and we can easily add a WPE version. Bug #231446
Committed r283878 (242755@main): <https://commits.webkit.org/242755@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 440506 [details].
Re-opened since this is blocked by bug 231735
Created attachment 441358 [details] Patch
I ran speedometer after and before this patch: score before: 118 score after: 139
Comment on attachment 441358 [details] Patch This needs to be reviewed by Don. I suspect he's not going to like this because WEBKIT_FRAMEWORKS ought to be handling this.
(In reply to Michael Catanzaro from comment #20) > Comment on attachment 441358 [details] > Patch > > This needs to be reviewed by Don. I suspect he's not going to like this > because WEBKIT_FRAMEWORKS ought to be handling this. I did my best, but I don't understand CMake, I tried to use frameworks everywhere, but for some reason WebKitTestRunner_FRAMEWORKS didn't work, so I added JavaScriptCore directly to LIBRARIES since for WPE it's always built as object in this patch.
(In reply to Michael Catanzaro from comment #20) > Comment on attachment 441358 [details] > Patch > > This needs to be reviewed by Don. I suspect he's not going to like this > because WEBKIT_FRAMEWORKS ought to be handling this. Don? Could you review this, please?
Comment on attachment 441358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=441358&action=review LGTM with a couple potential nits. > Source/WebKit/PlatformWPE.cmake:451 > + $<TARGET_PROPERTY:JavaScriptCore,INCLUDE_DIRECTORIES> I feel like you'd want something more specific for this. This means its getting ALL include directories from JavaScriptCore. > Tools/WebKitTestRunner/PlatformWPE.cmake:30 > + $<TARGET_OBJECTS:JavaScriptCore> > + $<TARGET_OBJECTS:bmalloc> I'm surprised these are needed since WebKitTestRunner uses _FRAMEWORKS can you maybe double check this? > Tools/WebKitTestRunner/PlatformWPE.cmake:40 > + $<TARGET_OBJECTS:JavaScriptCore> > + $<TARGET_OBJECTS:bmalloc> This one isn't surprising as the InjectedBundle stuff didn't get the _FRAMEWORKS treatment.
(In reply to Don Olmstead from comment #23) > Comment on attachment 441358 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=441358&action=review > > LGTM with a couple potential nits. > > > Source/WebKit/PlatformWPE.cmake:451 > > + $<TARGET_PROPERTY:JavaScriptCore,INCLUDE_DIRECTORIES> > > I feel like you'd want something more specific for this. This means its > getting ALL include directories from JavaScriptCore. I copy-pasted :-P I'll try to use something more specific. > > Tools/WebKitTestRunner/PlatformWPE.cmake:30 > > + $<TARGET_OBJECTS:JavaScriptCore> > > + $<TARGET_OBJECTS:bmalloc> > > I'm surprised these are needed since WebKitTestRunner uses _FRAMEWORKS can > you maybe double check this? Yes, I tried everything and the only thing that worked was manually adding the libraries. I don't understand why. > > Tools/WebKitTestRunner/PlatformWPE.cmake:40 > > + $<TARGET_OBJECTS:JavaScriptCore> > > + $<TARGET_OBJECTS:bmalloc> > > This one isn't surprising as the InjectedBundle stuff didn't get the > _FRAMEWORKS treatment.
Created attachment 441862 [details] Patch for landing
I managed to use FRAMEWORKS for WTR, it was indeed already used, but for some reason I had to remove WebKit and WebCoreTestSupport, I guess they aren't actually needed because they are already in TestRunnerShared lib
Created attachment 441871 [details] Patch for landing
Committed r284527 (243269@main): <https://commits.webkit.org/243269@main>