Because we are in 2019.
Created attachment 372549 [details] Patch
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 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.
> > > 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 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
(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.
Committed r246632: <https://trac.webkit.org/changeset/246632>
(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 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.