Bug 110330 – [GTK] Move GTK-specific platform files from WebCore to Platform

• Component: New Bugs
• Status: NEW

[Zan Dobersek]

[GTK] Move GTK-specific platform files from WebCore to Platform

[Zan Dobersek]

Created attachment 189303 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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.

[Martin Robinson]

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?

[Zan Dobersek]

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

[Zan Dobersek]

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.

[Zan Dobersek]

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.

[Zan Dobersek]

Created attachment 189734 [details]
Patch

[Zan Dobersek]

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.

[Zan Dobersek]

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.

[Martin Robinson]

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.

[Zan Dobersek]

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.

[Martin Robinson]

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

[Zan Dobersek]

Created attachment 189979 [details]
Patch

[Zan Dobersek]

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.

[Zan Dobersek]

Created attachment 189982 [details]
Patch

[Martin Robinson]

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.

[Zan Dobersek]

Comment on attachment 189982 [details]
Patch

Removing from the review queue.

[Martin Robinson]

Perhaps this bug can be closed now?