Bug 199065 – [GTK] Remove support for GTK2 plugins

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

Because we are in 2019.

[Carlos Garcia Campos]

Created attachment 372549 [details]
Patch

[Adrian Perez]

Comment on attachment 372549 [details]
Patch

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

Informally reviewing... Looks good to me, with a small question.

Indeed it's 2019 — it makes me smile to see this go away.

> Source/WTF/wtf/glib/GTypedefs.h:-82
> -

Kinda amazing that this slipped in and stayed around so long.
Good catch.

> Source/WebKit/PlatformGTK.cmake:-469
> -    PRIVATE

Is there any reason to remove the “PRIVATE” here? IIUC the GTK
Unix printing library would be still fine linked as such.

> Source/WebKit/SourcesGTK.txt:-27
> -

Great to see more sources built unified, yay!

[Carlos Garcia Campos]

Comment on attachment 372549 [details]
Patch

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

>> Source/WebKit/PlatformGTK.cmake:-469
>> -    PRIVATE
> 
> Is there any reason to remove the “PRIVATE” here? IIUC the GTK
> Unix printing library would be still fine linked as such.

Michael told me this is useless in another patch.

[Carlos Garcia Campos]

> > > Source/WebKit/PlatformGTK.cmake:479
> > > +if (USE_WPE_RENDERER)
> > > +    list(APPEND WebKit_LIBRARIES
> > > +        PRIVATE
> > 
> > Oops! (PRIVATE doesn't work here.)
> 
> What doesn't work? What should I do then, just remove it?

Yeah, just remove it. PRIVATE is not a library and cannot be appended to WebKit_LIBRARIES.
[tag] [reply] [−] Comment 8

[Sergio Villar Senin]

Comment on attachment 372549 [details]
Patch

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

> Source/WebKit/Shared/gtk/WebEventFactory.cpp:46
> +    return keyval >= GDK_KEY_KP_Space && keyval <= GDK_KEY_KP_9;

A bit unrelated bug OK

[Carlos Garcia Campos]

(In reply to Sergio Villar Senin from comment #5)
> Comment on attachment 372549 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=372549&action=review
> 
> > Source/WebKit/Shared/gtk/WebEventFactory.cpp:46
> > +    return keyval >= GDK_KEY_KP_Space && keyval <= GDK_KEY_KP_9;
> 
> A bit unrelated bug OK

Not unrelated, it doesn't build because I have removed the compatibility header too.

[Carlos Garcia Campos]

Committed r246632: <https://trac.webkit.org/changeset/246632>

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #3)
> Comment on attachment 372549 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=372549&action=review
> 
> >> Source/WebKit/PlatformGTK.cmake:-469
> >> -    PRIVATE
> > 
> > Is there any reason to remove the “PRIVATE” here? IIUC the GTK
> > Unix printing library would be still fine linked as such.
> 
> Michael told me this is useless in another patch.

As far as I understand the documentation, the following:

  target_link_libraries(a PUBLIC b PRIVATE c)

Will cause code that links against library “a” will be able to use
*also* symbols from “b”, but not symbols from “c” (unless it explicitly
links to “c” as well).

If the above is the case (I am not 100% sure, because the CMake
documentation is not particularly good), then it looks useful to
me, because it allows for containment of symbols from other libraries
used internally without exposing the transitively as part of the ABI
of the ABI of the “a” library. It also avoids overlinking… I suppose
this needs some support from the linker and that CMake can figure out
by itself how to put it to use.

So I wouldn't consider “PRIVATE” useless ;-)

(Of couse, “PRIVATE” should not be part of a list-variable, only
passed directly to “target_link_libraries”, so it's of course a
good thing that you removed it here.)

[Michael Catanzaro]

Comment on attachment 372549 [details]
Patch

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

Nice! Especially good job removing @no-unify from SourcesGTK.txt; it looks like that was easier than I expected.

And now Adrian, you see why I didn't want you to remove the GDK_API_VERSION_2 guards yesterday.

>>>> Source/WebKit/PlatformGTK.cmake:-469
>>>> -    PRIVATE
>>> 
>>> Is there any reason to remove the “PRIVATE” here? IIUC the GTK
>>> Unix printing library would be still fine linked as such.
>> 
>> Michael told me this is useless in another patch.
> 
> As far as I understand the documentation, the following:
> 
>   target_link_libraries(a PUBLIC b PRIVATE c)
> 
> Will cause code that links against library “a” will be able to use
> *also* symbols from “b”, but not symbols from “c” (unless it explicitly
> links to “c” as well).
> 
> If the above is the case (I am not 100% sure, because the CMake
> documentation is not particularly good), then it looks useful to
> me, because it allows for containment of symbols from other libraries
> used internally without exposing the transitively as part of the ABI
> of the ABI of the “a” library. It also avoids overlinking… I suppose
> this needs some support from the linker and that CMake can figure out
> by itself how to put it to use.
> 
> So I wouldn't consider “PRIVATE” useless ;-)
> 
> (Of couse, “PRIVATE” should not be part of a list-variable, only
> passed directly to “target_link_libraries”, so it's of course a
> good thing that you removed it here.)

Yeah, it has to be used directly in target_link_libraries. What we had here was just a bug.

Don has been working on replacing PUBLIC include directories with PRIVATE to simplify our build. But for now, we're still using PUBLIC for libraries everywhere. To change this, we'd need to add a *_PRIVATE_LIBRARIES variable to the _WEBKIT_TARGET macros. This would be of limited value and cleaning up include directories seems a lot more important.