So we can participate better in the design/architecture of the new API layer that we will very likely want to use, we should get something building as soon as possible. This bug tracks our efforts on that front.
Created attachment 53003 [details] non-working work in progress patch I started playing around with this, but I have to go away for a while, so I thought I'd upload what I already did. I haven't been too careful about correctness yet, just poked, and massaged stuff. I am trying to get a convenience library called libwebkit2 built from the Platform/ and Shared/ directories, essentially. Notes: * It looks like the plan is to avoid using config.h in WebKit2, but I have added it in some places in this first try; * I am using OS(LINUX) in places that need a non-toolkit-specific, platform-specific typedef, for now, without really looking at its correctness in detail; * Some files use "WebCore/<File>.h" paths for inclusion, that may or may not be related to changes that were done to WebCore's directory structure and will be landed in the future. Some examples: #include <WebCore/FloatRect.h> #include <WebCore/PlatformString.h>
(In reply to comment #1) > Created an attachment (id=53003) [details] > non-working work in progress patch > > I started playing around with this, but I have to go away for a while, so I > thought I'd upload what I already did. I haven't been too careful about > correctness yet, just poked, and massaged stuff. I am trying to get a > convenience library called libwebkit2 built from the Platform/ and Shared/ > directories, essentially. > > Notes: > > * It looks like the plan is to avoid using config.h in WebKit2, but I have > added it in some places in this first try; The platforms that are currently supported all support prefix headers, so there’s no need to explicitly do prefix includes in this manner. We may decide that it is a good idea to match the explicit style we use in JavaScriptCore and WebCore, or we may find that it is necessary because some platforms do not support prefix headers. > * I am using OS(LINUX) in places that need a non-toolkit-specific, > platform-specific typedef, for now, without really looking at its correctness > in detail; > * Some files use "WebCore/<File>.h" paths for inclusion, that may or may not be > related to changes that were done to WebCore's directory structure and will be > landed in the future. Some examples: > > #include <WebCore/FloatRect.h> > #include <WebCore/PlatformString.h> This is a Mac OS X-style framework include. We’ll need to work out which approach we want to use to handle including WebCore headers in a cross-platform manner within WebKit.
Hey Mark, thanks for the comments! (In reply to comment #2) > The platforms that are currently supported all support prefix headers, so Cool. Is the idea using one prefix header for all platforms, or having each platform define its own? WebKit2Prefix.h seems to be windows-specific, and WebKit2_Prefix.pch mac-specific. Should I make a new one for Linux, or add preprocessor checks to one of the existing? > This is a Mac OS X-style framework include. We’ll need to work out which > approach we want to use to handle including WebCore headers in a cross-platform > manner within WebKit. My current approach has been removing all the path, and including only the filename but that may not be the best approach, given that many files have similar names in both WebKit2, and Webcore, and depending only on include path precedence to fix that may not be the best thing. So, what do you suggest?
Created attachment 53043 [details] First try! So, this gets us a libwebkit2.la library built with a small part of what is already available of WebKit2. It has a few fixes to include paths, and such that we need to discuss better how to handle to avoid ambiguity. We also need to discuss our approach to the prefix headers.
Attachment 53043 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKit2/Shared/WebPreferencesStore.h:32: Alphabetical sorting problem. [build/include_order] [4] WebKit2/Shared/WebEventConversion.cpp:29: Alphabetical sorting problem. [build/include_order] [4] WebKit2/LinuxWebKit2Prefix.h:21: Alphabetical sorting problem. [build/include_order] [4] WebKit2/Shared/WebEvent.h:35: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 4 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
> filename but that may not be the best approach, given that many files have > similar names in both WebKit2, and Webcore, and depending only on include path s/similar/equal/ I had that problem with Arguments.h, which exists in JavaScriptCore/runtime and WebKit2/Platform/CoreIPC, while playing around.
Actually, regarding the Identifier for the connection, I am considering using a named unix socket. We could use dbus, but given the fact that CoreIPC has its own IPC model, and its own serialization of data types, and messages, I'm not sure what the advantage of using dbus would be. We would only be sending opaque messages containing a string with the serialized data. The alternative is to replace CoreIPC with dbus on our ports, but I think we'd lose a whole lot of the work that is already in place, and would have to reimplement a lot of the IPC code. Anyone knows better?
My (vague) understanding of dbus is that all messages get routed through the message daemon, which is overhead you will not want for low-latency high-throughput IPC. Chrome uses named pipes on Windows and anonymous socketpair() on Unix. The nice thing about anonymous file descriptors is that you don't end up with junk on your disk after crashes -- they clean themselves up. http://src.chromium.org/cgi-bin/gitweb.cgi?p=chromium.git;a=blob;f=ipc/ipc_channel_posix.cc;h=f6b19f7e0aa69ac6ce9870c84bc52f8c17f53bac;hb=HEAD discusses it
It is possible to use dbus peer-to-peer, with no server routing, yes. I also thought of socketpair, but CoreIPC in its current state looks simpler to implement if you have a named resource. Haven't looked into too much detail though.
(In reply to comment #9) > It is possible to use dbus peer-to-peer, with no server routing, yes. I also > thought of socketpair, but CoreIPC in its current state looks simpler to > implement if you have a named resource. Haven't looked into too much detail > though. I'm not up to speed on DBus, but it's definitely useful to have a low-level transport as much as possible. Ideally you will want to be able to pass shared memory and file descriptors over this channel (FDs are not needed right now, but will probably useful once you get to proxying.)
Created attachment 53818 [details] some more WIP - IPC work I don't even claim this builds - I'm just feeling bad about sitting on top of this work without being able to work full-throttle on it =).
Useful information, from andersca on the mailing list: "Right now WebCore needs to be built with some different flags (ENABLE_EXPERIMENTAL_SINGLE_VIEW_MODE=1 and WTF_USE_WEB_THREAD=1), but the goal is that the same WebCore should be able to support both WebKit and WebKit2 (and on the mac, both WebKit frameworks will link against the same WebCore framework)."
We (Sam Weinig) have started work on getting rid of the ENABLE_WEB_THREAD define.
Comment on attachment 53043 [details] First try! Looks like we need to work out how we want to do cross-framework includes before deciding what to do about this patch. Please post an updated patch once we've come to consensus on that topic.
Created attachment 63386 [details] Patch for this issue using framework-style includes and precompiled/prefix headers
Comment on attachment 63386 [details] Patch for this issue using framework-style includes and precompiled/prefix headers > distclean-local: >- -rm -rf $(GENSOURCES) $(GENPROGRAMS) >+ -rm -rf $(GENSOURCES) $(GENPROGRAMS) include Does the include dir need to be in the toplevel actually? I also wonder if there's some way to move that into the WebKit2 makefile. I suppose defining the rule multiple times won't do the right thing ;) >+libwebkit2_la_CXXFLAGS = \ >+ $(global_cxxflags) \ >+ $(corekit_cflags) >+ >+libwebkit2_la_CFLAGS = \ >+ $(global_cflags) \ >+ $(corekit_cflags) I believe corekit_cflags does not exist anymore? >+ >+# Ensure that the prefix header is built before >+BUILT_SOURCES += \ >+ $(webkit2_copied_headers) \ >+ DerivedSources/WebKit2/WebKit2Prefix.h.gch >+ >+# We compile the prefix header into a precompiled header in the DerivedSources >+# directory, so we can force it to be at the beginning of the include list. >+DerivedSources/WebKit2/WebKit2Prefix.h.gch: $(srcdir)/WebKit2/WebKit2Prefix.h >+ mkdir -p DerivedSources/WebKit2 >+ cp -u $< DerivedSources/WebKit2/WebKit2Prefix.h >+ $(CXXCOMPILE) $(libwebkit2_la_CPPFLAGS) -c DerivedSources/WebKit2/WebKit2Prefix.h Should use AM_V_GEN there. >+ >+javascriptcore_headers := $(filter %.h, $(javascriptcore_sources)) >+copy-javascriptcore-headers: $(javascriptcore_headers) >+ mkdir -p include/JavaScriptCore >+ cp -u $(foreach include,$(javascriptcore_headers),$(srcdir)/$(include)) include/JavaScriptCore And there. >+phony_targets += copy-javascriptcore-headers >+ >+webcore_headers := $(filter %.h, $(webcore_sources)) >+copy-webcore-headers: $(webcore_headers) >+ mkdir -p include/WebCore >+ cp -u $(foreach include,$(webcore_headers),$(srcdir)/$(include)) include/WebCore Same. >+phony_targets += copy-webcore-headers >+
Comment on attachment 63386 [details] Patch for this issue using framework-style includes and precompiled/prefix headers Marking as r- since this was reviewed some time ago already.
Thanks. I'll try to have a new patch up soon.
Created attachment 71620 [details] Initial Implementation of WebKit2 Gtk Port Basic implementation of WebKit2 Gtk Port along with a sample MiniBrowser app
Attachment 71620 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 Last 3072 characters of output: al.cpp:27: 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] WebKit2/UIProcess/gtk/WebViewInternal.cpp:33: webkit_widget_view_realize is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebKit2/UIProcess/gtk/WebViewInternal.cpp:58: webkit_widget_view_container_add is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebKit2/UIProcess/gtk/WebViewInternal.cpp:85: webkit_widget_view_class_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebKit2/UIProcess/gtk/WebViewInternal.cpp:100: gobject_class is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebKit2/UIProcess/gtk/WebViewInternal.cpp:110: webkit_widget_view_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebKit2/UIProcess/Plugins/gtk/PluginInfoStoreGtk.cpp:28: 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] WebKit2/Shared/gtk/NativeWebKeyboardEventGtk.cpp:27: 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] WebKit2/Shared/gtk/WebEventFactory.cpp:26: 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: "WebCore/platform/network/soup/cache/webkit/soup-cache.h" WebKitTools/MiniBrowser/gtk/main.cpp:31: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebKit2/UIProcess/Launcher/gtk/ThreadLauncherGtk.cpp:27: 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] WebKit2/UIProcess/gtk/WebContextGtk.cpp:27: 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] WebKitTools/MiniBrowser/gtk/BrowserWindow.cpp:31: 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] WebKit2/gtk/MainGtk.cpp:27: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:27: 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] Total errors found: 38 in 72 files If any of these errors are false positives, please file a bug against check-webkit-style.
config.h need not be the first header inclusion because we follow framework style includes. As for the other errors, GTK naming convention is followed. Raised a bug https://bugs.webkit.org/show_bug.cgi?id=48172 for the same. (In reply to comment #20) > Attachment 71620 [details] did not pass style-queue: > > Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 > Last 3072 characters of output: > al.cpp:27: 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] > WebKit2/UIProcess/gtk/WebViewInternal.cpp:33: webkit_widget_view_realize is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] > WebKit2/UIProcess/gtk/WebViewInternal.cpp:58: webkit_widget_view_container_add is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] > WebKit2/UIProcess/gtk/WebViewInternal.cpp:85: webkit_widget_view_class_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] > WebKit2/UIProcess/gtk/WebViewInternal.cpp:100: gobject_class is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] > WebKit2/UIProcess/gtk/WebViewInternal.cpp:110: webkit_widget_view_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] > WebKit2/UIProcess/Plugins/gtk/PluginInfoStoreGtk.cpp:28: 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] > WebKit2/Shared/gtk/NativeWebKeyboardEventGtk.cpp:27: 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] > WebKit2/Shared/gtk/WebEventFactory.cpp:26: 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: "WebCore/platform/network/soup/cache/webkit/soup-cache.h" > WebKitTools/MiniBrowser/gtk/main.cpp:31: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] > WebKit2/UIProcess/Launcher/gtk/ThreadLauncherGtk.cpp:27: 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] > WebKit2/UIProcess/gtk/WebContextGtk.cpp:27: 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] > WebKitTools/MiniBrowser/gtk/BrowserWindow.cpp:31: 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] > WebKit2/gtk/MainGtk.cpp:27: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] > WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:27: 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] > Total errors found: 38 in 72 files > > > If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #21) > config.h need not be the first header inclusion because we follow framework style includes. > As for the other errors, GTK naming convention is followed. > Raised a bug https://bugs.webkit.org/show_bug.cgi?id=48172 for the same. Great patch! I haven't had time to go through it in detail, but I believe we want all new code to follow WebKit coding style. The only exception are names that are exposed to the public API.
Comment on attachment 71620 [details] Initial Implementation of WebKit2 Gtk Port View in context: https://bugs.webkit.org/attachment.cgi?id=71620&action=review Generally the patch looks very promising for me, however I do not know much about GTK and I am not a reviewer. I think the MiniBrowser part should be separated to another patch, this is quite big even without that. The main problem I see is that there are too many ifdefs in common code. Those should be avoided somehow. > WebCore/platform/network/soup/cache/webkit/soup-cache.h:7 > * I have never saw the expressions "Portions Copyright" or any prefix before "all rights reserved" in WebKit before, so maybe you should omit those for convenience. > WebKit2/Platform/Module.h:69 > I do not think you should add copyright for that amount of changes, but of course it is your decision. > WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:97 > + return true; this should be "return m_socket > 0" as I see > WebKit2/UIProcess/ChunkedUpdateDrawingAreaProxy.cpp:139 > +#if PLATFORM(GTK) > + page->process()->send(DrawingAreaMessage::DidSetSizeDone, page->pageID(), CoreIPC::In(info().id, updateChunk->xID())); > +#endif We should avoid ifdefs in common code. You can send this message from invalidateBackingStore which is a platform method. > WebKit2/UIProcess/ChunkedUpdateDrawingAreaProxy.cpp:160 > +#if PLATFORM(GTK) > + page->process()->send(DrawingAreaMessage::DidUpdate, page->pageID(), CoreIPC::In(info().id, updateChunk->xID())); > +#else > page->process()->send(DrawingAreaMessage::DidUpdate, page->pageID(), CoreIPC::In(info().id)); > +#endif We should get rid of the ifdefs somehow. Is there a way to avoid the gtk specialization of didUpdate? I see the reason of that but maybe you can do it in another way. One think that had been came to my mind is to use info().id for the key in xIDMap and introduce a platform method that would be called from ChunkedUpdateArea::didUpdate, for example 'void platformDidUpdate(const DrawingAreaInfo&)' > WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:67 > +// FIXME: Including "PluginView.h" at current location was re-defining the enum CodePath { Complex } to 0 in WebCore/platform/graphics/Font.h. > +// The re-define is happening because of a macro COMPLEX in Xlib.h. Hence, moved the inclusion to the end of list for compilation on GTK platform. > +#include <WebKit2/PluginView.h> > Please try to fix this. I had similar problems that could be solved by including npruntime_internal.h instead of npruntime.h in some files. You should precompile it and find where the macro is coming from. > WebKit2/WebProcess/WebPage/ChunkedUpdateDrawingArea.cpp:102 > +#if PLATFORM(GTK) > + xIDMap.set(updateChunk.xID(), updateChunk.memory()).second; > +#endif why '.second'? I am assuming this has been left here accidentally :) > WebKit2/WebProcess/WebPage/ChunkedUpdateDrawingArea.cpp:152 > +#if PLATFORM(GTK) > + xIDMap.set(updateChunk.xID(), updateChunk.memory()).second; > +#endif As I sad above, this should be avoided. > WebKit2/WebProcess/WebPage/ChunkedUpdateDrawingArea.cpp:195 > +#if PLATFORM(GTK) > +void ChunkedUpdateDrawingArea::didUpdate(uint32_t xID) > +{ > + m_isWaitingForUpdate = false; > + freeBitMapMemory(xID); > + // Display if needed. > + display(); > +} > + > +void ChunkedUpdateDrawingArea::didSetSizeDone(uint32_t xID) > +{ > + freeBitMapMemory(xID); > +} > +#endif Ditto.
(In reply to comment #22) > (In reply to comment #21) > > config.h need not be the first header inclusion because we follow framework style includes. > > As for the other errors, GTK naming convention is followed. > > Raised a bug https://bugs.webkit.org/show_bug.cgi?id=48172 for the same. > > Great patch! I haven't had time to go through it in detail, but I believe we want all new code to follow WebKit coding style. The only exception are names that are exposed to the public API. Ok. Since these files do not expose any public API, we will change the naming convention to match the WebKit style and re-submit the patch.
(In reply to comment #23) > (From update of attachment 71620 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=71620&action=review > > Generally the patch looks very promising for me, however I do not know much about GTK and I am not a reviewer. > I think the MiniBrowser part should be separated to another patch, this is quite big even without that. Yes. Will split the patch and re-submit. > The main problem I see is that there are too many ifdefs in common code. Those should be avoided somehow. > > > WebCore/platform/network/soup/cache/webkit/soup-cache.h:7 > > * > > I have never saw the expressions "Portions Copyright" or any prefix before "all rights reserved" in WebKit before, so maybe > you should omit those for convenience. > > > WebKit2/Platform/Module.h:69 > > > > I do not think you should add copyright for that amount of changes, but of course it is your decision. > > > WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:97 > > + return true; > > this should be "return m_socket > 0" as I see Yes. Will incorporate this change. > > > WebKit2/UIProcess/ChunkedUpdateDrawingAreaProxy.cpp:139 > > +#if PLATFORM(GTK) > > + page->process()->send(DrawingAreaMessage::DidSetSizeDone, page->pageID(), CoreIPC::In(info().id, updateChunk->xID())); > > +#endif > > We should avoid ifdefs in common code. You can send this message from invalidateBackingStore which is a platform method. invalidate would be too early to send a confirmation as the handler in the WebProcess would delete the GdkPixmap and the next function in the UI Process, drawUpdateChunkIntoBackingStore would use this GdkPixmap. > > > WebKit2/UIProcess/ChunkedUpdateDrawingAreaProxy.cpp:160 > > +#if PLATFORM(GTK) > > + page->process()->send(DrawingAreaMessage::DidUpdate, page->pageID(), CoreIPC::In(info().id, updateChunk->xID())); > > +#else > > page->process()->send(DrawingAreaMessage::DidUpdate, page->pageID(), CoreIPC::In(info().id)); > > +#endif > > We should get rid of the ifdefs somehow. Is there a way to avoid the gtk specialization of didUpdate? I see the reason of > that but maybe you can do it in another way. One think that had been came to my mind is to use info().id for the key in > xIDMap and introduce a platform method that would be called from ChunkedUpdateArea::didUpdate, for example > 'void platformDidUpdate(const DrawingAreaInfo&)' info().id refers to the DrawingArea id. Each DrawingArea may result in multiple UpdateChunk's. So, this id cannot be the key in this case. I could think of another alternative(i'm not sure if that is a good idea) ie to define ID() in UpdateChunk() for all the platforms and use it wherever required. > > > WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:67 > > +// FIXME: Including "PluginView.h" at current location was re-defining the enum CodePath { Complex } to 0 in WebCore/platform/graphics/Font.h. > > +// The re-define is happening because of a macro COMPLEX in Xlib.h. Hence, moved the inclusion to the end of list for compilation on GTK platform. > > +#include <WebKit2/PluginView.h> > > > > Please try to fix this. I had similar problems that could be solved by including npruntime_internal.h instead of npruntime.h > in some files. You should precompile it and find where the macro is coming from. This is an incorrect FIXME comment that we left. Sorry for that. We just wanted to prevent WebCore/PluginView.h inclusion while compiling this file. > > > WebKit2/WebProcess/WebPage/ChunkedUpdateDrawingArea.cpp:102 > > +#if PLATFORM(GTK) > > + xIDMap.set(updateChunk.xID(), updateChunk.memory()).second; > > +#endif > > why '.second'? I am assuming this has been left here accidentally :) Yes. Thanks for pointing this out. Will correct this. > > > WebKit2/WebProcess/WebPage/ChunkedUpdateDrawingArea.cpp:152 > > +#if PLATFORM(GTK) > > + xIDMap.set(updateChunk.xID(), updateChunk.memory()).second; > > +#endif > > As I sad above, this should be avoided. > > > WebKit2/WebProcess/WebPage/ChunkedUpdateDrawingArea.cpp:195 > > +#if PLATFORM(GTK) > > +void ChunkedUpdateDrawingArea::didUpdate(uint32_t xID) > > +{ > > + m_isWaitingForUpdate = false; > > + freeBitMapMemory(xID); > > + // Display if needed. > > + display(); > > +} > > + > > +void ChunkedUpdateDrawingArea::didSetSizeDone(uint32_t xID) > > +{ > > + freeBitMapMemory(xID); > > +} > > +#endif > > Ditto. This can be moved to ChunkedUpdateDrawingAreaGtk.cpp, however, the declarations in the .h file need to be there.
Created attachment 71887 [details] WebKit2 GTK Patch after rework Corrected the style errors pointed by check-webkit-style. Like Martin Robinson suggested, we followed WebKit style for GTK specific files as well. Addressed a few review comments from Balazs Kelemen Separated MiniBrowser app patch from this
Created attachment 71888 [details] MiniBrowser App Patch MiniBrowser GTK App Patch separated
Attachment 71887 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 Last 3072 characters of output: file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebKit2/Shared/gtk/BackingStoreGtk.cpp:27: 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] WebKit2/WebProcess/WebCoreSupport/gtk/WebErrorsGtk.cpp:27: 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] WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:27: 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] WebKit2/Platform/gtk/WorkQueueGtk.cpp:27: 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] WebKit2/Platform/gtk/ModuleGtk.cpp:27: 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] WebKit2/UIProcess/gtk/WebViewInternal.cpp:27: 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] WebKit2/UIProcess/Plugins/gtk/PluginInfoStoreGtk.cpp:28: 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] WebKit2/Shared/gtk/NativeWebKeyboardEventGtk.cpp:27: 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] WebKit2/Shared/gtk/WebEventFactory.cpp:26: 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: "WebCore/platform/network/soup/cache/webkit/soup-cache.h" WebKit2/UIProcess/Launcher/gtk/ThreadLauncherGtk.cpp:27: 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] WebKit2/UIProcess/gtk/WebContextGtk.cpp:27: 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] WebKit2/gtk/MainGtk.cpp:27: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:27: 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] Total errors found: 26 in 63 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 71888 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKitTools/MiniBrowser/gtk/main.cpp:31: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebKitTools/MiniBrowser/gtk/BrowserView.cpp:31: 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] WebKitTools/MiniBrowser/gtk/BrowserWindow.cpp:31: 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] WebKitTools/MiniBrowser/gtk/MiniBrowser.cpp:31: 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] Total errors found: 4 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 71887 [details] WebKit2 GTK Patch after rework View in context: https://bugs.webkit.org/attachment.cgi?id=71887&action=review Phwew! This is a big patch. Thanks for contributing it. I've reviewed the build changes so far. Hopefully I'll find some time later to look over the rest of the patch. > GNUmakefile.am:163 > +# WebKit2 > +if ENABLE_WEBKIT2 > + > +libJavaScriptCore_ladir = $(prefix)/include/webkit2-@WEBKITGTK_API_VERSION@/JavaScriptCore > +libJavaScriptCore_la_HEADERS = $(javascriptcore_h_api) > + > +lib_LTLIBRARIES += \ > + libwebkit2gtk-@WEBKITGTK_API_MAJOR_VERSION@.@WEBKITGTK_API_MINOR_VERSION@.la > + > +nodist_EXTRA_libwebkit2gtk_@WEBKITGTK_API_MAJOR_VERSION@_@WEBKITGTK_API_MINOR_VERSION@_la_SOURCES = \ > + $(webcore_built_nosources) > + > +nodist_libwebkit2gtk_@WEBKITGTK_API_MAJOR_VERSION@_@WEBKITGTK_API_MINOR_VERSION@_la_SOURCES = \ > + DerivedSources/WebKit2/WebKit2Prefix.h \ > + $(webcore_built_sources) \ > + $(webkit2_built_sources) > + > +libwebkit2gtk_@WEBKITGTK_API_MAJOR_VERSION@_@WEBKITGTK_API_MINOR_VERSION@_ladir = $(prefix)/include/webkit2-@WEBKITGTK_API_VERSION@/webkit2 Instead of repeating all of these source and flag lists, is it possible to create a variable in configure.ac and use it much like WEBKITGTK_API_MAJOR_VERSION? > JavaScriptCore/GNUmakefile.am:-54 > - JavaScriptCore/API/APICast.h \ Why have you removed these headers from the source list? > JavaScriptCore/GNUmakefile.am:-71 > - JavaScriptCore/API/JSRetainPtr.h \ Same question. > JavaScriptCore/GNUmakefile.am:-76 > - JavaScriptCore/API/OpaqueJSString.h \ And here. > WebCore/ChangeLog:8 > + No new tests. (OOPS!) This line should be replaced with an explaination of what sort of testing is possible, if at all. > WebCore/ChangeLog:14 > + * GNUmakefile.am: Changes to enable disable building of WebCore/bindings/gobject when building WebKit2. > + This is required since WebCore/bindings/gobject access WebKit/gtk/webkit files, which are skipped for WebKit2. > + * platform/network/soup/cache/webkit/soup-cache.h: Changes to stop including <webkit/webkitdefines.h> > + when building WebKit2. Instead the macro WEBKIT_API is defined directly in soup-cache.h. <webkit/webkitdefines.h> > + is including only when building WebKit1. This is done as entire dir WebKit/gtk/webkit is skipped when building Talking to Xan today, it seems that the bindings/gobject files only include webkitdefines.h for the WEBKIT_API visibility markers. Can you try to simply eliminate the webkitdefines.h include and reproduce WEBKIT_API in bindings/gobject. That would eliminate a lot of the conditional stuff below. > WebCore/GNUmakefile.am:-2916 > - WebCore/storage/IDBIndexBackendInterface.h \ Why remove this? > WebCore/GNUmakefile.am:-2927 > - WebCore/storage/IDBObjectStoreBackendInterface.h \ And this? > WebCore/platform/network/soup/cache/webkit/soup-cache.h:6 > + * Portions Copyright (c) 2010 Motorola Mobility, Inc. Except as provided below, all rights reserved. It's fine for your copyright message to omit "Portions" and "Except as provided below,". If I understand correctly, these are implied. > WebKit2/GNUmakefile.am:481 > +# Copy header files to DerivedSources I guess I feel that 'include' makes sense as the destination instead of DerivedSources/WebKit2/include. These files aren't generated, they are just copied. > WebKit2/GNUmakefile.am:483 > +copy_wc_headers := $(filter %.h, $(webcore_sources)) > +webcore-headers: $(copy_wc_headers) This is sort of the reverse of what I would expect. I think the rule should have the verb. Something like this: webcore_headers := $(filter %.h, $(webcore_sources)) copy-webcore-headers: $(webcore_headers) I have this preference for the rest of these copying rules too. > WebKit2/GNUmakefile.am:538 > + $(WebKit2)/UIProcess/Plugins \ > + $(WebKit2)/WebProcess/Plugins \ > + $(WebKit2)/WebProcess/WebPage \ The indentation is a little funky here. > WebKit2/GNUmakefile.am:541 > +vpath %.messages.in = $(MESSAGE_INPUT_PATH) Is it possible to list these directly in the vpath directive instead of spinning them off into a separate variable? > WebKit2/GNUmakefile.am:543 > +MESSAGE_GEN_SCRIPT = \ I'd rather this be in lower-case since it's not global. Something like message_gen_scripts. > WebKit2/GNUmakefile.am:592 > +# .PHONY: include-headers I think you need to add all of the header copying targets to the .PHONY rule.
>> WebKit2/GNUmakefile.am:481 >> +# Copy header files to DerivedSources > I guess I feel that 'include' makes sense as the destination instead of > DerivedSources/WebKit2/include. These files aren't generated, they are just > copied. I recommend you to generate forwarding headers instead of copying. One of the benefits is dependency tracking. WebKitTools/Scripts/generate-forwarding-headers.pl should fit your needs, if it is not than you can hack on that.
(In reply to comment #31) > > I guess I feel that 'include' makes sense as the destination instead of > > DerivedSources/WebKit2/include. These files aren't generated, they are just > > copied. > I recommend you to generate forwarding headers instead of copying. > One of the benefits is dependency tracking. > WebKitTools/Scripts/generate-forwarding-headers.pl should fit your needs, > if it is not than you can hack on that. Great tip, Balazs! I definitely agree with this.
Attachment 71887 [details] did not build on gtk: Build output: http://queues.webkit.org/results/4824043
Created attachment 72181 [details] Bare minimum changes for building GTK port of WebKit2 The attached patch includes Makefile changes for building either WebKit1 or WebKit2. As part of the WebKit2 build, it builds all platform independent files, and stubbed out platform dependent files. Rest of the code will be attached as independent compilation units to each of the specific bugs raised. These bugs are marked as dependent on 37369.
The build failure is because MiniBrowser files are separated into a different patch and these files are included in the main Makefile. The patch is marked obsolete and a new patch is attached. Please ignore this error. (In reply to comment #33) > Attachment 71887 [details] did not build on gtk: > Build output: http://queues.webkit.org/results/4824043
Attachment 72181 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit2/WebProcess/InjectedBundle/gtk/InjectedBundleGtk.cpp:27: 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] WebKit2/Shared/gtk/WebCoreArgumentCodersGtk.cpp:27: 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] WebKit2/Platform/gtk/SharedMemoryGtk.cpp:27: 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] WebKit2/WebKit2Prefix.h:118: "WebCore/config.h" already included at WebKit2/WebKit2Prefix.h:107 [build/include] [4] WebKit2/Shared/gtk/BackingStoreGtk.cpp:27: 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] WebKit2/UIProcess/Plugins/gtk/PluginInfoStoreGtk.cpp:28: 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] WebKit2/WebProcess/Plugins/Netscape/gtk/NetscapePluginGtk.cpp:27: 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: "WebCore/platform/network/soup/cache/webkit/soup-cache.h" Total errors found: 7 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #30) > (From update of attachment 71887 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=71887&action=review > > Phwew! This is a big patch. Thanks for contributing it. I've reviewed the build changes so far. Hopefully I'll find some time later to look over the rest of the patch. > > > GNUmakefile.am:163 > > +# WebKit2 > > +if ENABLE_WEBKIT2 > > + > > +libJavaScriptCore_ladir = $(prefix)/include/webkit2-@WEBKITGTK_API_VERSION@/JavaScriptCore > > +libJavaScriptCore_la_HEADERS = $(javascriptcore_h_api) > > + > > +lib_LTLIBRARIES += \ > > + libwebkit2gtk-@WEBKITGTK_API_MAJOR_VERSION@.@WEBKITGTK_API_MINOR_VERSION@.la > > + > > +nodist_EXTRA_libwebkit2gtk_@WEBKITGTK_API_MAJOR_VERSION@_@WEBKITGTK_API_MINOR_VERSION@_la_SOURCES = \ > > + $(webcore_built_nosources) > > + > > +nodist_libwebkit2gtk_@WEBKITGTK_API_MAJOR_VERSION@_@WEBKITGTK_API_MINOR_VERSION@_la_SOURCES = \ > > + DerivedSources/WebKit2/WebKit2Prefix.h \ > > + $(webcore_built_sources) \ > > + $(webkit2_built_sources) > > + > > +libwebkit2gtk_@WEBKITGTK_API_MAJOR_VERSION@_@WEBKITGTK_API_MINOR_VERSION@_ladir = $(prefix)/include/webkit2-@WEBKITGTK_API_VERSION@/webkit2 > > Instead of repeating all of these source and flag lists, is it possible to create a variable in configure.ac and use it much like WEBKITGTK_API_MAJOR_VERSION? Addressed in the newly attached patch > > > JavaScriptCore/GNUmakefile.am:-54 > > - JavaScriptCore/API/APICast.h \ > > Why have you removed these headers from the source list? Originally done to remove the multiple copy error while creating the forward headers. With the suggestion by Balazs Keleman, these are no longer required. Reverted the changes now. > > > JavaScriptCore/GNUmakefile.am:-71 > > - JavaScriptCore/API/JSRetainPtr.h \ > > Same question. Reverted the changes. > > > JavaScriptCore/GNUmakefile.am:-76 > > - JavaScriptCore/API/OpaqueJSString.h \ > > And here. Reverted the changes. > > > WebCore/ChangeLog:8 > > + No new tests. (OOPS!) > > This line should be replaced with an explaination of what sort of testing is possible, if at all. > > > WebCore/ChangeLog:14 > > + * GNUmakefile.am: Changes to enable disable building of WebCore/bindings/gobject when building WebKit2. > > + This is required since WebCore/bindings/gobject access WebKit/gtk/webkit files, which are skipped for WebKit2. > > + * platform/network/soup/cache/webkit/soup-cache.h: Changes to stop including <webkit/webkitdefines.h> > > + when building WebKit2. Instead the macro WEBKIT_API is defined directly in soup-cache.h. <webkit/webkitdefines.h> > > + is including only when building WebKit1. This is done as entire dir WebKit/gtk/webkit is skipped when building > > Talking to Xan today, it seems that the bindings/gobject files only include webkitdefines.h for the WEBKIT_API visibility markers. Can you try to simply eliminate the webkitdefines.h include and reproduce WEBKIT_API in bindings/gobject. That would eliminate a lot of the conditional stuff below. GObject separation is still required for WebKit2 build. > > > WebCore/GNUmakefile.am:-2916 > > - WebCore/storage/IDBIndexBackendInterface.h \ > > Why remove this? Reverted > > > WebCore/GNUmakefile.am:-2927 > > - WebCore/storage/IDBObjectStoreBackendInterface.h \ > > And this? Reverted > > > WebCore/platform/network/soup/cache/webkit/soup-cache.h:6 > > + * Portions Copyright (c) 2010 Motorola Mobility, Inc. Except as provided below, all rights reserved. > > It's fine for your copyright message to omit "Portions" and "Except as provided below,". If I understand correctly, these are implied. We will get back to you after discussing this with our Legal Department. > > > WebKit2/GNUmakefile.am:481 > > +# Copy header files to DerivedSources > > I guess I feel that 'include' makes sense as the destination instead of DerivedSources/WebKit2/include. These files aren't generated, they are just copied. Now that we are generating the forward headers, I hope the location should be fine :) > > > WebKit2/GNUmakefile.am:483 > > +copy_wc_headers := $(filter %.h, $(webcore_sources)) > > +webcore-headers: $(copy_wc_headers) > > This is sort of the reverse of what I would expect. I think the rule should have the verb. Something like this: Removed this way of copying the headers now. > > webcore_headers := $(filter %.h, $(webcore_sources)) > copy-webcore-headers: $(webcore_headers) > > I have this preference for the rest of these copying rules too. Same as above. > > > WebKit2/GNUmakefile.am:538 > > + $(WebKit2)/UIProcess/Plugins \ > > + $(WebKit2)/WebProcess/Plugins \ > > + $(WebKit2)/WebProcess/WebPage \ > > The indentation is a little funky here. Done. > > > WebKit2/GNUmakefile.am:541 > > +vpath %.messages.in = $(MESSAGE_INPUT_PATH) > > Is it possible to list these directly in the vpath directive instead of spinning them off into a separate variable? Done. > > > WebKit2/GNUmakefile.am:543 > > +MESSAGE_GEN_SCRIPT = \ > > I'd rather this be in lower-case since it's not global. Something like message_gen_scripts. Done. > > > WebKit2/GNUmakefile.am:592 > > +# .PHONY: include-headers > > I think you need to add all of the header copying targets to the .PHONY rule. Removed PHONY rule.
Thanks for the comment. We've incorporated this change in our new patch. (In reply to comment #31) > >> WebKit2/GNUmakefile.am:481 > >> +# Copy header files to DerivedSources > > > I guess I feel that 'include' makes sense as the destination instead of > > DerivedSources/WebKit2/include. These files aren't generated, they are just > > copied. > > I recommend you to generate forwarding headers instead of copying. > One of the benefits is dependency tracking. > WebKitTools/Scripts/generate-forwarding-headers.pl should fit your needs, > if it is not than you can hack on that.
Created attachment 72201 [details] Bare minimum changes for building GTK port of WebKit2 Corrects a typo error in WebKit2/GNUmakefile.am Adds WebEventFactory.h to base implementation to avoid conflicts in further patches
Attachment 72201 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebKit2/WebProcess/InjectedBundle/gtk/InjectedBundleGtk.cpp:27: 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] WebKit2/Shared/gtk/WebCoreArgumentCodersGtk.cpp:27: 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] WebKit2/Platform/gtk/SharedMemoryGtk.cpp:27: 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] WebKit2/WebKit2Prefix.h:118: "WebCore/config.h" already included at WebKit2/WebKit2Prefix.h:107 [build/include] [4] WebKit2/Shared/gtk/BackingStoreGtk.cpp:27: 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] WebKit2/UIProcess/Plugins/gtk/PluginInfoStoreGtk.cpp:28: 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] WebKit2/WebProcess/Plugins/Netscape/gtk/NetscapePluginGtk.cpp:27: 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: "WebCore/platform/network/soup/cache/webkit/soup-cache.h" Total errors found: 7 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 72201 [details] Bare minimum changes for building GTK port of WebKit2 This patch should be split up into multiple pieces. Maybe first one patch per new file added and then one with the build changes?
(In reply to comment #41) > (From update of attachment 72201 [details]) > This patch should be split up into multiple pieces. Maybe first one patch per new file added and then one with the build changes? Just to clarify, this patch is simply build changes plus GTK+-specific WebKit2 files stubbed out.
(In reply to comment #42) > (In reply to comment #41) > > (From update of attachment 72201 [details] [details]) > > This patch should be split up into multiple pieces. Maybe first one patch per new file added and then one with the build changes? > > Just to clarify, this patch is simply build changes plus GTK+-specific WebKit2 files stubbed out. I know that. Still, an almost 80k patch is pretty unwieldy regardless of what it contains.
Comment on attachment 72201 [details] Bare minimum changes for building GTK port of WebKit2 View in context: https://bugs.webkit.org/attachment.cgi?id=72201&action=review Looks good, but I think the build file changes can be simplified even more. > GNUmakefile.am:74 > +webcoregtk_gdom_sources := > +webcoregtk_gdom_cppflags := I don't think these are necessary. > GNUmakefile.am:82 > +webkit2gtk_static_h_api := > +webkit2_cppflags := > +webkit2gtk_cppflags := GTK+ is the only autotools user and probably will be for the future. I think we can get rid of webkit2gtk_sources and webkit2gtk_cppflags. > GNUmakefile.am:84 > +webkit2_built_nosources := I don't think this variable needs to exist. See below. > GNUmakefile.am:827 > +if !ENABLE_WEBKIT2 > # Older automake versions (1.7) place Plo files in a different place so we need > # to create the output directory manually. > all-local: stamp-po > $(mkdir_p) $(top_builddir)/$(DEPDIR)/DerivedSources > +endif #!ENABLE_WEBKIT2 Why is this work-around for older versions of automake conditional on the WebKit2 build? > WebCore/GNUmakefile.am:88 > +webcoregtk_gdom_cppflags += \ I don't understand the need for webcoregtk_gdom_cppflags here. Why not just add it directly to webcoregtk_cppflags? > WebCore/GNUmakefile.am:3651 > +webcoregtk_gdom_sources += \ Please just add these directly to webcoregtk_sources and scrap the webcoregtk_gdom_sources variable. I think keeping the number of makefile variables low is key to keeping it understandable. > WebCore/platform/network/soup/cache/webkit/soup-cache.h:30 > +#if !defined(ENABLE_WEBKIT2_BUILD) > #if BUILDING_GTK__ > #include <webkit/webkitdefines.h> > #else Instead of a workaround for WebKit2, why not just define WEBKIT_API unconditionally to simplify the #ifdefs? > WebKit2/GNUmakefile.am:3 > +ForwardingHeaders := $(GENSOURCES_WEBKIT2)/include > +HttpStack := @HTTP_STACK@ If these need to be in a make variable, please move them down near where you use them. > WebKit2/GNUmakefile.am:9 > + $(srcdir)/WebKit2/Shared/API/c/WKArray.h \ > + $(srcdir)/WebKit2/Shared/API/c/WKBase.h \ > + $(srcdir)/WebKit2/Shared/API/c/WKCertificateInfo.h \ > + $(srcdir)/WebKit2/Shared/API/c/WKData.h \ Why not use the $(WebKit2) variable defined above for this source list? > WebKit2/GNUmakefile.am:401 > + $(webkit2_built_nosources) I think I'd rather see generate-forwarding-headers here explicitly rather than having a webkit2_built_nosources variable. > WebKit2/GNUmakefile.am:471 > +vpath %.messages.in = $(WebKit2)/PluginProcess $(WebKit2)/UIProcess $(WebKit2)/UIProcess/Plugins $(WebKit2)/WebProcess/Plugins $(WebKit2)/WebProcess/WebPage $(WebKit2)/WebProcess Please split this across multiple lines, if possible.
(In reply to comment #43) > I know that. Still, an almost 80k patch is pretty unwieldy regardless of what it contains. Very well. The build file changes clock in around 48K alone (likely they will be a bit smaller once the stubbed out source files are removed). In your opinion, should Ravi and Amruth split the build file changes into even smaller patches? We would prefer not to check in a broken WebKit2 build, if possible.
(In reply to comment #45) > (In reply to comment #43) > > > I know that. Still, an almost 80k patch is pretty unwieldy regardless of what it contains. > > Very well. The build file changes clock in around 48K alone (likely they will be a bit smaller once the stubbed out source files are removed). In your opinion, should Ravi and Amruth split the build file changes into even smaller patches? We would prefer not to check in a broken WebKit2 build, if possible. You can easily land the stubbed out source files before landing the build file changes. Besides, the WebKit2 GTK+ build is already broken (for some definition of broken).
(In reply to comment #46) > You can easily land the stubbed out source files before landing the build file changes. Besides, the WebKit2 GTK+ build is already broken (for some definition of broken). That dips a bit too far into philosophy this early in the afternoon for my tastes (can something that doesn't exist be broken), but I think your solution makes a lot of sense.
Created attachment 72677 [details] Makefile changes to building the WebKit2 GTK port The makefile changes and couple of WebCore file changes for compiling the WebKit2 GTK port. Note: The patch in itself doesn't compile.
Created attachment 72678 [details] Dummy implementation for few of the WebKit2 GTK port classes Patch contains the GTK port dummy implementation for classes like SharedMemory, BackingStore, PluginInfoStore, NetscapePlugin etc.
(In reply to comment #44) > (From update of attachment 72201 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=72201&action=review > > Looks good, but I think the build file changes can be simplified even more. > > > GNUmakefile.am:74 > > +webcoregtk_gdom_sources := > > +webcoregtk_gdom_cppflags := > > I don't think these are necessary. Done. > > > GNUmakefile.am:82 > > +webkit2gtk_static_h_api := > > +webkit2_cppflags := > > +webkit2gtk_cppflags := > > GTK+ is the only autotools user and probably will be for the future. I think we can get rid of webkit2gtk_sources and webkit2gtk_cppflags. Done. > > > GNUmakefile.am:84 > > +webkit2_built_nosources := > > I don't think this variable needs to exist. See below. Done. > > > GNUmakefile.am:827 > > +if !ENABLE_WEBKIT2 > > # Older automake versions (1.7) place Plo files in a different place so we need > > # to create the output directory manually. > > all-local: stamp-po > > $(mkdir_p) $(top_builddir)/$(DEPDIR)/DerivedSources > > +endif #!ENABLE_WEBKIT2 > > Why is this work-around for older versions of automake conditional on the WebKit2 build? This is not possible. The older automake targets points to targets stamp-po etc which are defined in WebKit/gtk/po/GNUmakefile.am folder. Since, we are excluding the entire WebKit/ folder itself for WebKit2, the inclusion of this rule is not possible. > > > WebCore/GNUmakefile.am:88 > > +webcoregtk_gdom_cppflags += \ > > I don't understand the need for webcoregtk_gdom_cppflags here. Why not just add it directly to webcoregtk_cppflags? Done. > > > WebCore/GNUmakefile.am:3651 > > +webcoregtk_gdom_sources += \ > > Please just add these directly to webcoregtk_sources and scrap the webcoregtk_gdom_sources variable. I think keeping the number of makefile variables low is key to keeping it understandable. Done. > > > WebCore/platform/network/soup/cache/webkit/soup-cache.h:30 > > +#if !defined(ENABLE_WEBKIT2_BUILD) > > #if BUILDING_GTK__ > > #include <webkit/webkitdefines.h> > > #else > > Instead of a workaround for WebKit2, why not just define WEBKIT_API unconditionally to simplify the #ifdefs? Done. > > > WebKit2/GNUmakefile.am:3 > > +ForwardingHeaders := $(GENSOURCES_WEBKIT2)/include > > +HttpStack := @HTTP_STACK@ > > If these need to be in a make variable, please move them down near where you use them. Done. > > > WebKit2/GNUmakefile.am:9 > > + $(srcdir)/WebKit2/Shared/API/c/WKArray.h \ > > + $(srcdir)/WebKit2/Shared/API/c/WKBase.h \ > > + $(srcdir)/WebKit2/Shared/API/c/WKCertificateInfo.h \ > > + $(srcdir)/WebKit2/Shared/API/c/WKData.h \ > > Why not use the $(WebKit2) variable defined above for this source list? Done. > > > WebKit2/GNUmakefile.am:401 > > + $(webkit2_built_nosources) > > I think I'd rather see generate-forwarding-headers here explicitly rather than having a webkit2_built_nosources variable. Done. > > > WebKit2/GNUmakefile.am:471 > > +vpath %.messages.in = $(WebKit2)/PluginProcess $(WebKit2)/UIProcess $(WebKit2)/UIProcess/Plugins $(WebKit2)/WebProcess/Plugins $(WebKit2)/WebProcess/WebPage $(WebKit2)/WebProcess > > Please split this across multiple lines, if possible. Done.
Comment on attachment 72678 [details] Dummy implementation for few of the WebKit2 GTK port classes Great.
Comment on attachment 72678 [details] Dummy implementation for few of the WebKit2 GTK port classes Rejecting patch 72678 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', 72678]" exit_code: 2 Last 500 characters of output: o.h patching file WebKit2/UIProcess/Plugins/gtk/PluginInfoStoreGtk.cpp patching file WebKit2/WebProcess/InjectedBundle/gtk/InjectedBundleGtk.cpp patching file WebKit2/WebProcess/InjectedBundle/InjectedBundle.h patching file WebKit2/WebProcess/Plugins/Netscape/gtk/NetscapePluginGtk.cpp patching file WebKit2/WebProcess/WebCoreSupport/gtk/WebFrameNetworkingContext.h Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Martin Robinson', u'--force']" exit_code: 2 Full output: http://queues.webkit.org/results/5000029
Committed r71214: <http://trac.webkit.org/changeset/71214>
Comment on attachment 72678 [details] Dummy implementation for few of the WebKit2 GTK port classes Clearing flags, since this has landed.
Created attachment 74419 [details] Makefile changes to building the WebKit2 GTK port WebKit2 makefile changes based on current makefile in trunk where entire WebKit related stuff have been moved to WebKit/gtk/GNUmakefile.am from root GNUmakefile.am
The new patch addresses the WebKit2 makefile rework and is based on the GNUmakefile.am changes done as part of bug 49400 (https://bugs.webkit.org/show_bug.cgi?id=49400).
Comment on attachment 74419 [details] Makefile changes to building the WebKit2 GTK port View in context: https://bugs.webkit.org/attachment.cgi?id=74419&action=review Very nice patch. Thanks for updating it. Just a couple more lingering issues. I think it's very close. > WebCore/ChangeLog:8 > + No new tests. (OOPS!) This line should be removed. > WebCore/platform/network/soup/cache/webkit/soup-cache.h:27 > +#define WEBKIT_API __attribute__((visibility("default"))) > +#define WEBKIT_OBSOLETE_API WEBKIT_API __attribute__((deprecated)) This will actually break the GTK+ Windows build. You should probably include the MSVC support here too. > WebKit2/GNUmakefile.am:580 > +# Ensure that the prefix header is built before You are no longer using using a precompiled header, so I think this comment is out of date. :) > WebKit2/WebKit2Prefix.h:33 > +#if !defined (BUILDING_GTK__) > #include <wtf/Platform.h> > #include <wtf/DisallowCType.h> > #ifdef __cplusplus > #include <wtf/FastMalloc.h> > #endif > +#endif /* !defined (BUILDING_GTK__) */ Why do you need to conditionally include these? > WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:-33 > -#include "PluginView.h" I think the right way to solve this problem is to only include the big list of -I directories in WebCore if we are building WebKit 1. Otherwise, we'll probably keep running into this issue. Do you mind trying that? > configure.ac:903 > +HTTP_STACK="soup" > +WEBKITGTK_PC_NAME=${WEBKITGTK_PC_NAME}2 > +AC_SUBST([HTTP_STACK]) > +AC_SUBST([WEBKITGTK_PC_NAME]) If HTTP_STACK is hardcoded, why not just remove it? Generally we have standardized on soup. If you want to re-add support for cURL, maybe we can address it in another patch? I think it's much more feasible for WebKit2, if you really need it for something. > configure.ac:989 > +AC_CONFIG_FILES([ If posible. please indent the inside of this conditional. > configure.ac:1008 > +AC_CONFIG_FILES([ > +WebKit2/gtk/${WEBKITGTK_PC_NAME}-${WEBKITGTK_API_VERSION}.pc:WebKit2/gtk/webkit2.pc.in > +] > +,[WEBKITGTK_API_VERSION=$WEBKITGTK_API_VERSION,WEBKITGTK_PC_NAME=$WEBKITGTK_PC_NAME] > +) Same here.
Created attachment 74925 [details] Makefile changes to building the WebKit2 GTK port Makefile and script changes for building GTK WebKit2 port.
(In reply to comment #57) > (From update of attachment 74419 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=74419&action=review > > Very nice patch. Thanks for updating it. Just a couple more lingering issues. I think it's very close. > > > WebCore/ChangeLog:8 > > + No new tests. (OOPS!) > > This line should be removed. Done. > > > WebCore/platform/network/soup/cache/webkit/soup-cache.h:27 > > +#define WEBKIT_API __attribute__((visibility("default"))) > > +#define WEBKIT_OBSOLETE_API WEBKIT_API __attribute__((deprecated)) > > This will actually break the GTK+ Windows build. You should probably include the MSVC support here too. Done. Defined in same manner as defined in <webkit/webkitdefines.h> > > > WebKit2/GNUmakefile.am:580 > > +# Ensure that the prefix header is built before > > You are no longer using using a precompiled header, so I think this comment is out of date. :) Done. > > > WebKit2/WebKit2Prefix.h:33 > > +#if !defined (BUILDING_GTK__) > > #include <wtf/Platform.h> > > #include <wtf/DisallowCType.h> > > #ifdef __cplusplus > > #include <wtf/FastMalloc.h> > > #endif > > +#endif /* !defined (BUILDING_GTK__) */ > > Why do you need to conditionally include these? Changed to include <WebCore/config.h> at top of file for GTK port as per our discussion. > > > WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:-33 > > -#include "PluginView.h" > > I think the right way to solve this problem is to only include the big list of -I directories in WebCore if we are building WebKit 1. Otherwise, we'll probably keep running into this issue. Do you mind trying that? Reverted this file. Solved the issue by building WebKit2 as a convenience target and then linking with actual webkit target as per our discussion yesterday. > > > configure.ac:903 > > +HTTP_STACK="soup" > > +WEBKITGTK_PC_NAME=${WEBKITGTK_PC_NAME}2 > > +AC_SUBST([HTTP_STACK]) > > +AC_SUBST([WEBKITGTK_PC_NAME]) > > If HTTP_STACK is hardcoded, why not just remove it? Generally we have standardized on soup. If you want to re-add support for cURL, maybe we can address it in another patch? I think it's much more feasible for WebKit2, if you really need it for something. Done. > > > configure.ac:989 > > +AC_CONFIG_FILES([ > > If posible. please indent the inside of this conditional. > > > configure.ac:1008 > > +AC_CONFIG_FILES([ > > +WebKit2/gtk/${WEBKITGTK_PC_NAME}-${WEBKITGTK_API_VERSION}.pc:WebKit2/gtk/webkit2.pc.in > > +] > > +,[WEBKITGTK_API_VERSION=$WEBKITGTK_API_VERSION,WEBKITGTK_PC_NAME=$WEBKITGTK_PC_NAME] > > +) > > Same here. Done.
Created attachment 76169 [details] Makefile changes to building the WebKit2 GTK port Makefile changes to building the WebKit2 GTK port based on comments given by Martin Robinson on 9th December.
Hi Martin Robinson, Based on our discussion yesterday, I have attached the new patch with 2 changes. 1) Removed unwanted variables from GNUmakefile.am - just kept the variables for built_sources 2) Removed the WebCore/bindings/gobject files from WebCore/GNUmakefile.am and added them to WebKit/gtk/GNUmakefile.am. Now there is no if !ENABLE_WEBKIT2 inside WebCore/GNUmakefile.am
Comment on attachment 76169 [details] Makefile changes to building the WebKit2 GTK port This looks good to me. I'd love it if a WebKit2 developer could confirm that the change to WebKit2/WebKit2Prefix.h is sane. Perhaps we should leave a comment there explaining the situation before landing this.
Comment on attachment 76169 [details] Makefile changes to building the WebKit2 GTK port Rejecting attachment 76169 [details] from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-sf', 'apply-attachment', '--non-interactive', 76169]" exit_code: 2 Last 500 characters of output: generate-forwarding-headers.pl -------------------------- No file to patch. Skipping patch. 1 out of 1 hunk ignored patching file WebKitTools/Scripts/webkitdirs.pm Hunk #1 succeeded at 1352 (offset 2 lines). patching file configure.ac Hunk #1 succeeded at 902 (offset 7 lines). Hunk #2 succeeded at 993 (offset 8 lines). Hunk #3 succeeded at 1082 (offset 9 lines). Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Martin Robinson', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/7008103
Comment on attachment 76169 [details] Makefile changes to building the WebKit2 GTK port Patch no logner applies. We'll need a new patch.
Created attachment 76630 [details] WebKit2 GTK makefile Updated the reviewed (r+) WebKit2 GTK Makefile patch to latest trunk revision (74097)
The commit-queue encountered the following flaky tests while processing attachment 76630 [details]: inspector/elements-panel-styles.html bug 50987 (author: pfeldman@chromium.org) http/tests/navigation/post-frames.html bug 51111 (authors: jamesr@chromium.org and mihaip@chromium.org) The commit-queue is continuing to process your patch.
Comment on attachment 76630 [details] WebKit2 GTK makefile Clearing flags on attachment: 76630 Committed r74117: <http://trac.webkit.org/changeset/74117>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/74117 might have broken GTK Linux 64-bit Debug
Created attachment 76766 [details] Makefile for WebKit2 GTK port WebKit2 GTK makefile with couple of typo errors fixed which were breaking GTK1 build.
(In reply to comment #69) > http://trac.webkit.org/changeset/74117 might have broken GTK Linux 64-bit Debug Attached the new patch which corrects the GTK1 breakage. Tested GTK1 with the current patch on both 32-bit & 64-bit debug builds. Layout Test output with 32-bit Debug build : 16129 test cases (96%) succeeded 482 test cases (2%) had incorrect layout 47 test cases (<1%) were new 1 test case (<1%) timed out 5 test cases (<1%) crashed 102 test cases (<1%) had stderr output
(In reply to comment #71) > (In reply to comment #69) > > http://trac.webkit.org/changeset/74117 might have broken GTK Linux 64-bit Debug > Attached the new patch which corrects the GTK1 breakage. > Tested GTK1 with the current patch on both 32-bit & 64-bit debug builds. > > Layout Test output with 32-bit Debug build : > 16129 test cases (96%) succeeded > 482 test cases (2%) had incorrect layout > 47 test cases (<1%) were new > 1 test case (<1%) timed out > 5 test cases (<1%) crashed > 102 test cases (<1%) had stderr output Layout Test output with 64-bit Debug build : 16140 test cases (96%) succeeded 460 test cases (2%) had incorrect layout 47 test cases (<1%) were new 17 test cases (<1%) crashed 165 test cases (<1%) had stderr output
Comment on attachment 76766 [details] Makefile for WebKit2 GTK port View in context: https://bugs.webkit.org/attachment.cgi?id=76766&action=review This patch is very close, but there are a couple issues to fix before it can land. I've commented below. > WebCore/ChangeLog:-645 > -2010-12-15 Amruth Raj <amruthraj@motorola.com> and Ravi Kasibhatla <ravi.kasibhatla@motorola.com> > - > - Reviewed by Martin Robinson. > - > - Changes to enable building WebKit2 for Gtk port > - (https://bugs.webkit.org/show_bug.cgi?id=37369) > - > - No new functionality added or deleted. Only makefile change. Hence, no tests added. > - > - * GNUmakefile.am: Removed bindings/gobject from webcore_sources & webcore_cppflags and added them to WebKit/gtk/GNUmakefile.am > - * platform/network/soup/cache/webkit/soup-cache.h: Remove include <webkit/webkitdefines.h> and declare WEBKIT_API directly > - You shouldn't remove lines from the ChangeLog here. I'll fix this when landing. > WebKit2/ChangeLog:-278 > -2010-12-15 Amruth Raj <amruthraj@motorola.com> and Ravi Kasibhatla <ravi.kasibhatla@motorola.com> > - > - Reviewed by Martin Robinson. > - > - Changes to enable building WebKit2 for Gtk port. > - (https://bugs.webkit.org/show_bug.cgi?id=37369) > - > - * GNUmakefile.am: Added. > - * Scripts/generate-forwarding-headers.pl: For GTK port, taking 1 extra argument for copying network headers. > - * WebKit2Prefix.h: Included WebCore/config.h for GTK port as the first header file for WebKit2 sources files. > - * gtk: Added. > - * gtk/webkit2.pc.in: Added. > - Same issue. > WebKitTools/ChangeLog:-195 > -2010-12-15 Amruth Raj <amruthraj@motorola.com> and Ravi Kasibhatla <ravi.kasibhatla@motorola.com> > - > - Reviewed by Martin Robinson. > - > - Change generate-forwarding-headers.pl for GTK port usage > - (https://bugs.webkit.org/show_bug.cgi?id=37369) > - > - * Scripts/webkitdirs.pm: Added changes to build webkit2 for GTK port using build-webkit script. > - Ditto. > WebKitTools/Scripts/webkitdirs.pm:1356 > + } elsif ($opt =~ /^--webkit2/i ) { > + push @buildArgs, "--enable-webkit2"; This change seems unrelated. Are they necessary, or is it possible to just remove this before landing?
(In reply to comment #73) > (From update of attachment 76766 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76766&action=review > > This patch is very close, but there are a couple issues to fix before it can land. I've commented below. > > > WebCore/ChangeLog:-645 > > -2010-12-15 Amruth Raj <amruthraj@motorola.com> and Ravi Kasibhatla <ravi.kasibhatla@motorola.com> > > - > > - Reviewed by Martin Robinson. > > - > > - Changes to enable building WebKit2 for Gtk port > > - (https://bugs.webkit.org/show_bug.cgi?id=37369) > > - > > - No new functionality added or deleted. Only makefile change. Hence, no tests added. > > - > > - * GNUmakefile.am: Removed bindings/gobject from webcore_sources & webcore_cppflags and added them to WebKit/gtk/GNUmakefile.am > > - * platform/network/soup/cache/webkit/soup-cache.h: Remove include <webkit/webkitdefines.h> and declare WEBKIT_API directly > > - > > You shouldn't remove lines from the ChangeLog here. I'll fix this when landing. > > > WebKit2/ChangeLog:-278 > > -2010-12-15 Amruth Raj <amruthraj@motorola.com> and Ravi Kasibhatla <ravi.kasibhatla@motorola.com> > > - > > - Reviewed by Martin Robinson. > > - > > - Changes to enable building WebKit2 for Gtk port. > > - (https://bugs.webkit.org/show_bug.cgi?id=37369) > > - > > - * GNUmakefile.am: Added. > > - * Scripts/generate-forwarding-headers.pl: For GTK port, taking 1 extra argument for copying network headers. > > - * WebKit2Prefix.h: Included WebCore/config.h for GTK port as the first header file for WebKit2 sources files. > > - * gtk: Added. > > - * gtk/webkit2.pc.in: Added. > > - > > Same issue. > > > WebKitTools/ChangeLog:-195 > > -2010-12-15 Amruth Raj <amruthraj@motorola.com> and Ravi Kasibhatla <ravi.kasibhatla@motorola.com> > > - > > - Reviewed by Martin Robinson. > > - > > - Change generate-forwarding-headers.pl for GTK port usage > > - (https://bugs.webkit.org/show_bug.cgi?id=37369) > > - > > - * Scripts/webkitdirs.pm: Added changes to build webkit2 for GTK port using build-webkit script. > > - > > Ditto. > > > WebKitTools/Scripts/webkitdirs.pm:1356 > > + } elsif ($opt =~ /^--webkit2/i ) { > > + push @buildArgs, "--enable-webkit2"; > > This change seems unrelated. Are they necessary, or is it possible to just remove this before landing? It was added so that we can build WebKit2 through the "build-webkit" script. We could land this separately also (once complete WebKit2 patch is in).
(In reply to comment #72) > (In reply to comment #71) > > (In reply to comment #69) > > > http://trac.webkit.org/changeset/74117 might have broken GTK Linux 64-bit Debug > > Attached the new patch which corrects the GTK1 breakage. > > Tested GTK1 with the current patch on both 32-bit & 64-bit debug builds. > > > > Layout Test output with 32-bit Debug build : > > 16129 test cases (96%) succeeded > > 482 test cases (2%) had incorrect layout > > 47 test cases (<1%) were new > > 1 test case (<1%) timed out > > 5 test cases (<1%) crashed > > 102 test cases (<1%) had stderr output > > Layout Test output with 64-bit Debug build : > 16140 test cases (96%) succeeded > 460 test cases (2%) had incorrect layout > 47 test cases (<1%) were new > 17 test cases (<1%) crashed > 165 test cases (<1%) had stderr output Layout Test results with xvfb: 32-bit build: 16376 test cases (98%) succeeded 238 test cases (1%) had incorrect layout 47 test cases (<1%) were new 1 test case (<1%) timed out 2 test cases (<1%) crashed 171 test cases (1%) had stderr output 64-bit build: 16483 test cases (98%) succeeded 127 test cases (<1%) had incorrect layout 47 test cases (<1%) were new 2 test cases (<1%) timed out 5 test cases (<1%) crashed 235 test cases (1%) had stderr output
Comment on attachment 76766 [details] Makefile for WebKit2 GTK port I Will fix the remaining issues while landing this one.
Reopening this bug until the patch lands.
Created attachment 77527 [details] Patch with suggested changes
I've uploaded a modified version of the patch fixing the issues I mentioned as well with a few other changes: 1. Remove the need to modify generate-forwarding-headers. The appropriate thing here is to call it twice, I think. 2. Some small cleanups. Amruth and Ravi, can you take a look and ensure that these changes fit with your work?
(In reply to comment #79) > I've uploaded a modified version of the patch fixing the issues I mentioned as well with a few other changes: > > 1. Remove the need to modify generate-forwarding-headers. The appropriate thing here is to call it twice, I think. > 2. Some small cleanups. > > Amruth and Ravi, can you take a look and ensure that these changes fit with your work? The patch looks fine and works well.
I had Xan Lopez double-check my modifications to Amruth and Ravi's patch.
Committed r74766: <http://trac.webkit.org/changeset/74766>
http://trac.webkit.org/changeset/74766 might have broken GTK Linux 64-bit Debug The following tests are not passing: editing/selection/extend-by-character-005.html