Bug 199065 - [GTK] Remove support for GTK2 plugins
Summary: [GTK] Remove support for GTK2 plugins
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2019-06-20 02:43 PDT by Carlos Garcia Campos
Modified: 2019-06-20 06:39 PDT (History)
4 users (show)

See Also:


Attachments
Patch (80.81 KB, patch)
2019-06-20 02:46 PDT, Carlos Garcia Campos
svillar: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2019-06-20 02:43:06 PDT
Because we are in 2019.
Comment 1 Carlos Garcia Campos 2019-06-20 02:46:17 PDT
Created attachment 372549 [details]
Patch
Comment 2 Adrian Perez 2019-06-20 03:35:50 PDT
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!
Comment 3 Carlos Garcia Campos 2019-06-20 03:39:37 PDT
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.
Comment 4 Carlos Garcia Campos 2019-06-20 03:55:01 PDT
> > > 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
Comment 5 Sergio Villar Senin 2019-06-20 04:01:17 PDT
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
Comment 6 Carlos Garcia Campos 2019-06-20 04:06:56 PDT
(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.
Comment 7 Carlos Garcia Campos 2019-06-20 04:08:46 PDT
Committed r246632: <https://trac.webkit.org/changeset/246632>
Comment 8 Adrian Perez 2019-06-20 04:24:12 PDT
(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.)
Comment 9 Michael Catanzaro 2019-06-20 06:39:52 PDT
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.