Bug 222860 – [WPE] Reenable -fvisibility=hidden and -fvisibility-inlines-hidden

• Status: RESOLVED
• Resolution: FIXED
• Component: WPE WebKit

[Michael Catanzaro]

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.

[Philippe Normand]

WDYT Adrian?

[Michael Catanzaro]

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.

[Michael Catanzaro]

(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.

[Michael Catanzaro]

Created attachment 440506 [details]
Patch

[Michael Catanzaro]

Comment on attachment 440506 [details]
Patch

Looks like the QtWPE API does indeed need more work.

[Philippe Normand]

Naive build fix: http://sprunge.us/pYbFZg

[Michael Catanzaro]

(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.

[Michael Catanzaro]

(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. ?????????

[Philippe Normand]

(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.

[Philippe Normand]

Created attachment 440603 [details]
Patch

[Michael Catanzaro]

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).

[Philippe Normand]

Alright, I'll fork in a separate bug!

[Philippe Normand]

(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

[Michael Catanzaro]

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.

[Michael Catanzaro]

(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

[EWS]

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].

[WebKit Commit Bot]

Re-opened since this is blocked by bug 231735

[Carlos Garcia Campos]

Created attachment 441358 [details]
Patch

[Carlos Garcia Campos]

I ran speedometer after and before this patch:

score before: 118
score after: 139

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

(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?

[Don Olmstead]

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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Created attachment 441862 [details]
Patch for landing

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Created attachment 441871 [details]
Patch for landing

[Carlos Garcia Campos]

Committed r284527 (243269@main): <https://commits.webkit.org/243269@main>