[GTK] Move GTK-specific platform files from WebCore to Platform
Created attachment 189303 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Attachment 189303 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Source/Platform/ChangeLog', u'Source/Platform/GNUmakefile.am', u'Source/Platform/GNUmakefile.list.am', u'Source/Platform/gtk/AsyncFileSystemGtk.cpp', u'Source/Platform/gtk/AsyncFileSystemGtk.h', u'Source/Platform/gtk/ClipboardGtk.cpp', u'Source/Platform/gtk/ClipboardGtk.h', u'Source/Platform/gtk/ClipboardUtilitiesGtk.cpp', u'Source/Platform/gtk/ClipboardUtilitiesGtk.h', u'Source/Platform/gtk/CompositionResults.h', u'Source/Platform/gtk/ContextMenuGtk.cpp', u'Source/Platform/gtk/ContextMenuItemGtk.cpp', u'Source/Platform/gtk/CursorGtk.cpp', u'Source/Platform/gtk/CursorGtk.h', u'Source/Platform/gtk/DataObjectGtk.cpp', u'Source/Platform/gtk/DataObjectGtk.h', u'Source/Platform/gtk/DragDataGtk.cpp', u'Source/Platform/gtk/DragIcon.cpp', u'Source/Platform/gtk/DragIcon.h', u'Source/Platform/gtk/DragImageGtk.cpp', u'Source/Platform/gtk/ErrorsGtk.cpp', u'Source/Platform/gtk/ErrorsGtk.h', u'Source/Platform/gtk/EventLoopGtk.cpp', u'Source/Platform/gtk/FileSystemGtk.cpp', u'Source/Platform/gtk/GOwnPtrGtk.cpp', u'Source/Platform/gtk/GOwnPtrGtk.h', u'Source/Platform/gtk/GRefPtrGtk.cpp', u'Source/Platform/gtk/GRefPtrGtk.h', u'Source/Platform/gtk/GamepadsGtk.cpp', u'Source/Platform/gtk/GtkAuthenticationDialog.cpp', u'Source/Platform/gtk/GtkAuthenticationDialog.h', u'Source/Platform/gtk/GtkClickCounter.cpp', u'Source/Platform/gtk/GtkClickCounter.h', u'Source/Platform/gtk/GtkDragAndDropHelper.cpp', u'Source/Platform/gtk/GtkDragAndDropHelper.h', u'Source/Platform/gtk/GtkInputMethodFilter.cpp', u'Source/Platform/gtk/GtkInputMethodFilter.h', u'Source/Platform/gtk/GtkPluginWidget.cpp', u'Source/Platform/gtk/GtkPluginWidget.h', u'Source/Platform/gtk/GtkPopupMenu.cpp', u'Source/Platform/gtk/GtkPopupMenu.h', u'Source/Platform/gtk/GtkUtilities.cpp', u'Source/Platform/gtk/GtkUtilities.h', u'Source/Platform/gtk/GtkVersioning.c', u'Source/Platform/gtk/GtkVersioning.h', u'Source/Platform/gtk/GtkWidgetBackingStoreX11.cpp', u'Source/Platform/gtk/KURLGtk.cpp', u'Source/Platform/gtk/KeyBindingTranslator.cpp', u'Source/Platform/gtk/KeyBindingTranslator.h', u'Source/Platform/gtk/LanguageGtk.cpp', u'Source/Platform/gtk/LocalizedStringsGtk.cpp', u'Source/Platform/gtk/LoggingGtk.cpp', u'Source/Platform/gtk/MIMETypeRegistryGtk.cpp', u'Source/Platform/gtk/MainFrameScrollbarGtk.cpp', u'Source/Platform/gtk/MainFrameScrollbarGtk.h', u'Source/Platform/gtk/PasteboardGtk.cpp', u'Source/Platform/gtk/PasteboardHelper.cpp', u'Source/Platform/gtk/PasteboardHelper.h', u'Source/Platform/gtk/PlatformKeyboardEventGtk.cpp', u'Source/Platform/gtk/PlatformMouseEventGtk.cpp', u'Source/Platform/gtk/PlatformScreenGtk.cpp', u'Source/Platform/gtk/PlatformWheelEventGtk.cpp', u'Source/Platform/gtk/PopupMenuGtk.cpp', u'Source/Platform/gtk/PopupMenuGtk.h', u'Source/Platform/gtk/RedirectedXCompositeWindow.cpp', u'Source/Platform/gtk/RedirectedXCompositeWindow.h', u'Source/Platform/gtk/RenderThemeGtk.cpp', u'Source/Platform/gtk/RenderThemeGtk.h', u'Source/Platform/gtk/RenderThemeGtk2.cpp', u'Source/Platform/gtk/RenderThemeGtk3.cpp', u'Source/Platform/gtk/RunLoopGtk.cpp', u'Source/Platform/gtk/ScrollViewGtk.cpp', u'Source/Platform/gtk/ScrollbarThemeGtk.cpp', u'Source/Platform/gtk/ScrollbarThemeGtk.h', u'Source/Platform/gtk/ScrollbarThemeGtk2.cpp', u'Source/Platform/gtk/ScrollbarThemeGtk3.cpp', u'Source/Platform/gtk/SearchPopupMenuGtk.cpp', u'Source/Platform/gtk/SearchPopupMenuGtk.h', u'Source/Platform/gtk/SharedBufferGtk.cpp', u'Source/Platform/gtk/SharedTimerGtk.cpp', u'Source/Platform/gtk/SoundGtk.cpp', u'Source/Platform/gtk/TemporaryLinkStubs.cpp', u'Source/Platform/gtk/UserAgentGtk.cpp', u'Source/Platform/gtk/UserAgentGtk.h.in', u'Source/Platform/gtk/WidgetGtk.cpp', u'Source/Platform/gtk/WidgetRenderingContext.cpp', u'Source/Platform/gtk/WidgetRenderingContext.h', u'Source/Platform/gtk/audio/AudioBusGtk.cpp', u'Source/Platform/gtk/graphics/ColorGtk.cpp', u'Source/Platform/gtk/graphics/FullscreenVideoControllerGtk.cpp', u'Source/Platform/gtk/graphics/FullscreenVideoControllerGtk.h', u'Source/Platform/gtk/graphics/GdkCairoUtilities.cpp', u'Source/Platform/gtk/graphics/GdkCairoUtilities.h', u'Source/Platform/gtk/graphics/IconGtk.cpp', u'Source/Platform/gtk/graphics/ImageBufferGtk.cpp', u'Source/Platform/gtk/graphics/ImageGtk.cpp', u'Source/Platform/gtk/graphics/IntPointGtk.cpp', u'Source/Platform/gtk/graphics/IntRectGtk.cpp', u'Source/Platform/gtk/network/CredentialBackingStore.cpp', u'Source/Platform/gtk/network/CredentialBackingStore.h', u'Source/Platform/gtk/text/TextBreakIteratorInternalICUGtk.cpp', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.am', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/bindings/gobject/GNUmakefile.am', u'Source/WebCore/platform/audio/gtk/AudioBusGtk.cpp', u'Source/WebCore/platform/graphics/gtk/ColorGtk.cpp', u'Source/WebCore/platform/graphics/gtk/FullscreenVideoControllerGtk.cpp', u'Source/WebCore/platform/graphics/gtk/FullscreenVideoControllerGtk.h', u'Source/WebCore/platform/graphics/gtk/GdkCairoUtilities.cpp', u'Source/WebCore/platform/graphics/gtk/GdkCairoUtilities.h', u'Source/WebCore/platform/graphics/gtk/IconGtk.cpp', u'Source/WebCore/platform/graphics/gtk/ImageBufferGtk.cpp', u'Source/WebCore/platform/graphics/gtk/ImageGtk.cpp', u'Source/WebCore/platform/graphics/gtk/IntPointGtk.cpp', u'Source/WebCore/platform/graphics/gtk/IntRectGtk.cpp', u'Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp', u'Source/WebCore/platform/gtk/AsyncFileSystemGtk.h', u'Source/WebCore/platform/gtk/ClipboardGtk.cpp', u'Source/WebCore/platform/gtk/ClipboardGtk.h', u'Source/WebCore/platform/gtk/ClipboardUtilitiesGtk.cpp', u'Source/WebCore/platform/gtk/ClipboardUtilitiesGtk.h', u'Source/WebCore/platform/gtk/CompositionResults.h', u'Source/WebCore/platform/gtk/ContextMenuGtk.cpp', u'Source/WebCore/platform/gtk/ContextMenuItemGtk.cpp', u'Source/WebCore/platform/gtk/CursorGtk.cpp', u'Source/WebCore/platform/gtk/CursorGtk.h', u'Source/WebCore/platform/gtk/DataObjectGtk.cpp', u'Source/WebCore/platform/gtk/DataObjectGtk.h', u'Source/WebCore/platform/gtk/DragDataGtk.cpp', u'Source/WebCore/platform/gtk/DragIcon.cpp', u'Source/WebCore/platform/gtk/DragIcon.h', u'Source/WebCore/platform/gtk/DragImageGtk.cpp', u'Source/WebCore/platform/gtk/ErrorsGtk.cpp', u'Source/WebCore/platform/gtk/ErrorsGtk.h', u'Source/WebCore/platform/gtk/EventLoopGtk.cpp', u'Source/WebCore/platform/gtk/FileSystemGtk.cpp', u'Source/WebCore/platform/gtk/GOwnPtrGtk.cpp', u'Source/WebCore/platform/gtk/GOwnPtrGtk.h', u'Source/WebCore/platform/gtk/GRefPtrGtk.cpp', u'Source/WebCore/platform/gtk/GRefPtrGtk.h', u'Source/WebCore/platform/gtk/GamepadsGtk.cpp', u'Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp', u'Source/WebCore/platform/gtk/GtkAuthenticationDialog.h', u'Source/WebCore/platform/gtk/GtkClickCounter.cpp', u'Source/WebCore/platform/gtk/GtkClickCounter.h', u'Source/WebCore/platform/gtk/GtkDragAndDropHelper.cpp', u'Source/WebCore/platform/gtk/GtkDragAndDropHelper.h', u'Source/WebCore/platform/gtk/GtkInputMethodFilter.cpp', u'Source/WebCore/platform/gtk/GtkInputMethodFilter.h', u'Source/WebCore/platform/gtk/GtkPluginWidget.cpp', u'Source/WebCore/platform/gtk/GtkPluginWidget.h', u'Source/WebCore/platform/gtk/GtkPopupMenu.cpp', u'Source/WebCore/platform/gtk/GtkPopupMenu.h', u'Source/WebCore/platform/gtk/GtkUtilities.cpp', u'Source/WebCore/platform/gtk/GtkUtilities.h', u'Source/WebCore/platform/gtk/GtkVersioning.c', u'Source/WebCore/platform/gtk/GtkVersioning.h', u'Source/WebCore/platform/gtk/GtkWidgetBackingStoreX11.cpp', u'Source/WebCore/platform/gtk/KURLGtk.cpp', u'Source/WebCore/platform/gtk/KeyBindingTranslator.cpp', u'Source/WebCore/platform/gtk/KeyBindingTranslator.h', u'Source/WebCore/platform/gtk/LanguageGtk.cpp', u'Source/WebCore/platform/gtk/LocalizedStringsGtk.cpp', u'Source/WebCore/platform/gtk/LoggingGtk.cpp', u'Source/WebCore/platform/gtk/MIMETypeRegistryGtk.cpp', u'Source/WebCore/platform/gtk/MainFrameScrollbarGtk.cpp', u'Source/WebCore/platform/gtk/MainFrameScrollbarGtk.h', u'Source/WebCore/platform/gtk/PasteboardGtk.cpp', u'Source/WebCore/platform/gtk/PasteboardHelper.cpp', u'Source/WebCore/platform/gtk/PasteboardHelper.h', u'Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp', u'Source/WebCore/platform/gtk/PlatformMouseEventGtk.cpp', u'Source/WebCore/platform/gtk/PlatformScreenGtk.cpp', u'Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp', u'Source/WebCore/platform/gtk/PopupMenuGtk.cpp', u'Source/WebCore/platform/gtk/PopupMenuGtk.h', u'Source/WebCore/platform/gtk/RedirectedXCompositeWindow.cpp', u'Source/WebCore/platform/gtk/RedirectedXCompositeWindow.h', u'Source/WebCore/platform/gtk/RenderThemeGtk.cpp', u'Source/WebCore/platform/gtk/RenderThemeGtk.h', u'Source/WebCore/platform/gtk/RenderThemeGtk2.cpp', u'Source/WebCore/platform/gtk/RenderThemeGtk3.cpp', u'Source/WebCore/platform/gtk/RunLoopGtk.cpp', u'Source/WebCore/platform/gtk/ScrollViewGtk.cpp', u'Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp', u'Source/WebCore/platform/gtk/ScrollbarThemeGtk.h', u'Source/WebCore/platform/gtk/ScrollbarThemeGtk2.cpp', u'Source/WebCore/platform/gtk/ScrollbarThemeGtk3.cpp', u'Source/WebCore/platform/gtk/SearchPopupMenuGtk.cpp', u'Source/WebCore/platform/gtk/SearchPopupMenuGtk.h', u'Source/WebCore/platform/gtk/SharedBufferGtk.cpp', u'Source/WebCore/platform/gtk/SharedTimerGtk.cpp', u'Source/WebCore/platform/gtk/SoundGtk.cpp', u'Source/WebCore/platform/gtk/TemporaryLinkStubs.cpp', u'Source/WebCore/platform/gtk/UserAgentGtk.cpp', u'Source/WebCore/platform/gtk/UserAgentGtk.h.in', u'Source/WebCore/platform/gtk/WidgetGtk.cpp', u'Source/WebCore/platform/gtk/WidgetRenderingContext.cpp', u'Source/WebCore/platform/gtk/WidgetRenderingContext.h', u'Source/WebCore/platform/network/gtk/CredentialBackingStore.cpp', u'Source/WebCore/platform/network/gtk/CredentialBackingStore.h', u'Source/WebCore/platform/text/gtk/TextBreakIteratorInternalICUGtk.cpp', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/GNUmakefile.am', u'Source/WebKit/gtk/po/ChangeLog', u'Source/WebKit/gtk/po/POTFILES.in', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.am', u'Source/WebKit2/Scripts/generate-forwarding-headers.pl', u'Source/WebKit2/Shared/Downloads/gtk/DownloadSoupErrorsGtk.cpp', u'Source/WebKit2/Shared/NativeWebKeyboardEvent.h', u'Source/WebKit2/Shared/NativeWebMouseEvent.h', u'Source/WebKit2/Shared/NativeWebWheelEvent.h', u'Source/WebKit2/Shared/gtk/ArgumentCodersGtk.cpp', u'Source/WebKit2/Shared/gtk/WebEventFactory.h', u'Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKit2GtkAuthenticationDialog.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitError.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebViewBaseInputMethodFilter.cpp', u'Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.cpp', u'Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp', u'Source/WebKit2/UIProcess/gtk/WebInspectorProxyGtk.cpp', u'Source/WebKit2/UIProcess/gtk/WebPageProxyGtk.cpp', u'Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.cpp', u'Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.h', u'Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebDragClientGtk.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebEditorClientGtk.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebErrorsGtk.cpp', u'Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp', u'Source/WebKit2/WebProcess/WebPage/gtk/WebPrintOperationGtk.cpp', u'Tools/ChangeLog', u'Tools/GNUmakefile.am', u'Tools/TestWebKitAPI/GNUmakefile.am', u'Tools/TestWebKitAPI/gtk/PlatformWebViewGtk.cpp', u'configure.ac']" exit_code: 1 Source/Platform/gtk/GtkVersioning.c:21: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKit2GtkAuthenticationDialog.h" Total errors found: 1 in 147 files If any of these errors are false positives, please file a bug against check-webkit-style.
Couple questions: We can't we totally replace libWebCoreGTK+ with PlatformGTK? So this means that ClipboardGtk will be in platform, but the superclass, Clipboard, will be in WebCore? Isn't it a bit odd that a subclass is defined in the dependency?
(In reply to comment #4) > Couple questions: We can't we totally replace libWebCoreGTK+ with PlatformGTK? This patch only moves GTK-specific source files to Source/Platform. There are still other files that are built into libWebCoreGtk that are used by other ports as well, for instance geoclue geolocation provider, gstreamer-based media player etc. The libWebCoreGtk library could potentially be replaced completely by libPlatformGtk, also building those sources that are still located in Source/WebCore/platform. Given that there's intention to move everything under Source/WebCore/platform to Source/Platform perhaps sources that are currently built into libWebCorePlatform could also be built into libPlatformGtk (perhaps the name simplified into libPlatform). Sources that are still placed in Source/WebCore/platform could still be listed in Source/WebCore/GNUmakefile.list.am just so people adding new files wouldn't have to go edit the Source/Platform/GNUmakefile.list.am. So yeah, I'd support replacing libWebCoreGtk completely with libPlatformGtk (and possibly replacing libWebCorePlatform as well), if the outlined plan is OK with everyone. > So this means that ClipboardGtk will be in platform, but the superclass, Clipboard, will be in WebCore? Isn't it a bit odd that a subclass is defined in the dependency? I assume it is. Diminishing libWebCoreGtk in favor of a libPlatformGtk library would fix most of these issues. Note, however, that the Clipboard class is defined in Source/WebCore/dom/Clipboard.h and is as such currently built into libWebCoreDOM.
Comment on attachment 189303 [details] Patch The patch will build with a clean build. Unfortunately, with all the style fixes, a small error got into one of the theming source files, causing problems.
Right, I'd recommend doing this change in three parts: - renaming libWebCoreGtk to libPlatform(Gtk?), defining the build rules in Source/Platform/GNUmakefile.am, - rolling libWebCorePlatform into libPlatform, - moving GTK-specific source files into Source/Platform/gtk. The last part can be done under this bug, I'll upload patches for the other two into new ones.
Created attachment 189734 [details] Patch
Created attachment 189739 [details] Patch Commit #3 from the platform-gtk branch I've set up on GitHub. https://github.com/zdobersek/webkit/commits/platform-gtk Will most likely not apply. The branch builds and works as a whole.
I haven't included any style fixes to avoid any (additional? :>) regressions. Since the patch doesn't apply yet the style bot will not dump the complete list of all the 500+ style issues the files on the move contain.
Comment on attachment 189734 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189734&action=review Looks like this patch might include a few changelog entries. > Source/WebKit2/GNUmakefile.am:-582 > - libWebCorePlatform.la \ > - libWebCoreGtk2.la \ I'm not sure I understand how we can get rid of libWebCorePlatform here. I probably need to see a high-level overview of what's going on.
Comment on attachment 189734 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189734&action=review >> Source/WebKit2/GNUmakefile.am:-582 >> - libWebCoreGtk2.la \ > > I'm not sure I understand how we can get rid of libWebCorePlatform here. I probably need to see a high-level overview of what's going on. This patch[1] relies on the patch that removes libWebCorePlatform.la and builds its sources into libPlatform.la. libPlatformGtk2.la is built in the same way as libPlatform.la but uses GTK2. [1] I incorrectly diffed out this patch from the branch. I'll upload a new one.
(In reply to comment #12) > (From update of attachment 189734 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189734&action=review > > >> Source/WebKit2/GNUmakefile.am:-582 > >> - libWebCoreGtk2.la \ > > > > I'm not sure I understand how we can get rid of libWebCorePlatform here. I probably need to see a high-level overview of what's going on. > > This patch[1] relies on the patch that removes libWebCorePlatform.la and builds its sources into libPlatform.la. libPlatformGtk2.la is built in the same way as libPlatform.la but uses GTK2. > > [1] I incorrectly diffed out this patch from the branch. I'll upload a new one. Thanks. Once I saw the other patch it made a lot more sense. :)
Created attachment 189979 [details] Patch
Comment on attachment 189979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189979&action=review > configure.ac:40 > +AC_CONFIG_FILES([DerivedSources/WebCore/UserAgentGtk.h:Source/Platform/gtk/UserAgentGtk.h.in]) The patch in bug #110582 removes the UserAgentGtk.h header generation, so this doesn't really apply anymore. I can remove related changes to this file when landing if I don't close bug #110582 first.
Created attachment 189982 [details] Patch
Comment on attachment 189982 [details] Patch Maybe we should wait on this part until all the platform files move? It feels weird to only move some of them.
Comment on attachment 189982 [details] Patch Removing from the review queue.
Perhaps this bug can be closed now?