Summary: | [GTK] Split webkitprivate.{cpp,h} in more manageable chunks | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gustavo Noronha (kov) <gustavo> | ||||||||||||||||||||
Component: | WebKitGTK | Assignee: | Gustavo Noronha (kov) <gustavo> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | abarth, eric, gustavo, webkit.review.bot, xan.lopez | ||||||||||||||||||||
Priority: | P2 | Keywords: | Gtk | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||||
Attachments: |
|
Description
Gustavo Noronha (kov)
2010-12-08 10:47:14 PST
Created attachment 75930 [details]
split webkitwebviewprivate.h out of webkitprivate.h
Comment on attachment 75930 [details] split webkitwebviewprivate.h out of webkitprivate.h View in context: https://bugs.webkit.org/attachment.cgi?id=75930&action=review I love this change. > WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:60 > #include <wtf/text/CString.h> > - > #include <glib.h> > #include <glib/gi18n-lib.h> > #include <gtk/gtk.h> These should be in alphabetical order. > WebKit/gtk/WebCoreSupport/ContextMenuClientGtk.cpp:22 > No newline here. > WebKit/gtk/WebCoreSupport/ContextMenuClientGtk.cpp:-32 > #include <wtf/text/CString.h> > - > #include <glib/gi18n-lib.h> > #include <glib-object.h> > #include <gtk/gtk.h> > -#include "webkitprivate.h" These should be in alphabetical order. > WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:74 > #include <wtf/text/CString.h> > #include <wtf/text/StringConcatenate.h> > - > #include <JavaScriptCore/APICast.h> > #include <gio/gio.h> > #include <glib.h> Run these through sort too. > WebKit/gtk/webkit/webkitwebview.cpp:57 > +#include "GOwnPtr.h" > +#include "GOwnPtrGtk.h" I think should be like <wtf/gobject/*> since they are WTF includes. > WebKit/gtk/webkit/webkitwebview.cpp:93 > #include <wtf/text/CString.h> > - > #include <gdk/gdkkeysyms.h> > +#include <glib/gi18n-lib.h> Alpha ordering here. > WebKit/gtk/webkit/webkitwebviewprivate.h:50 > +extern "C" { > + #define WEBKIT_WEB_VIEW_GET_PRIVATE(obj) (G_TYPE_INSTANCE_GET_PRIVATE((obj), WEBKIT_TYPE_WEB_VIEW, WebKitWebViewPrivate)) No indentation for this block. > WebKit/gtk/webkit/webkitwebviewprivate.h:124 > + void > + webkit_web_view_notify_ready(WebKitWebView* web_view); > + > + void > + webkit_web_view_request_download(WebKitWebView* web_view, WebKitNetworkRequest* request, const WebCore::ResourceResponse& response = WebCore::ResourceResponse(), WebCore::ResourceHandle* handle = 0); > + > + void > + webkit_web_view_add_resource(WebKitWebView*, const char*, WebKitWebResource*); > + > + void > + webkit_web_view_add_main_resource(WebKitWebView*, const char*, WebKitWebResource*); > + > + void > + webkit_web_view_remove_resource(WebKitWebView*, const char*); We should switch all these declarations to WebKit style since they aren't public API. Remove the newline after the type and between them and remove all undeeded variable names. Created attachment 75932 [details]
split webkitwebviewprivate.h
This should fix all the issues you raised =)
Comment on attachment 75932 [details]
split webkitwebviewprivate.h
Okay!
Comment on attachment 75932 [details] split webkitwebviewprivate.h Landed in r73539. Created attachment 76091 [details]
split WebKitWebFrame
Created attachment 76100 [details]
split WebKitNetworkRequest
Created attachment 76103 [details]
split web inspector
Comment on attachment 76100 [details] split WebKitNetworkRequest View in context: https://bugs.webkit.org/attachment.cgi?id=76100&action=review > WebKit/gtk/webkit/webkitnetworkrequest.cpp:264 > +WebKitNetworkRequest* kitNew(const WebCore::ResourceRequest& resourceRequest) > +{ > + PlatformRefPtr<SoupMessage> soupMessage(adoptPlatformRef(resourceRequest.toSoupMessage())); > + if (soupMessage) > + return WEBKIT_NETWORK_REQUEST(g_object_new(WEBKIT_TYPE_NETWORK_REQUEST, "message", soupMessage.get(), NULL)); > + > + return WEBKIT_NETWORK_REQUEST(g_object_new(WEBKIT_TYPE_NETWORK_REQUEST, "uri", resourceRequest.url().string().utf8().data(), NULL)); > +} > + It's a shame that the WebKitNetworkRequest doesn't store a copy of the original resource request. If it's possible perhaps it would make sense to do that in another patch. The kitNew name seems a little funky here, but makes sense. > WebKit/gtk/webkit/webkitnetworkrequestprivate.h:28 > +WebCore::ResourceRequest core(WebKitNetworkRequest* request); Please remove this variable name before landing. Comment on attachment 76103 [details] split web inspector View in context: https://bugs.webkit.org/attachment.cgi?id=76103&action=review > WebKit/gtk/webkit/webkitwebinspectorprivate.h:38 > +WEBKIT_API void webkit_web_inspector_execute_script(WebKitWebInspector*, long, const gchar*); I think here it makes sense to leave the variable names. Please add them back before landing. This should eventually move to DumpRenderTreeSupport, but that can be in another patch. Comment on attachment 76091 [details] split WebKitWebFrame Landed as r73696. Comment on attachment 76100 [details] split WebKitNetworkRequest Landed as r73701. Comment on attachment 76103 [details] split web inspector Landed as r73702. Created attachment 76192 [details]
Patch
Created attachment 76218 [details]
Patch
http://trac.webkit.org/changeset/73701 might have broken GTK Linux 32-bit Debug http://trac.webkit.org/changeset/73702 might have broken GTK Linux 32-bit Debug Comment on attachment 76218 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76218&action=review Yay! Please fix the small nits before landing. > WebKit/gtk/webkit/webkitdownloadprivate.h:33 > +void > +webkit_download_set_suggested_filename(WebKitDownload*, const gchar* suggestedFilename); This should probably be one line here. > WebKit/gtk/webkit/webkitwebnavigationactionprivate.h:31 > +WebKitWebNavigationReason kit(WebCore::NavigationType type); > +WebCore::NavigationType core(WebKitWebNavigationReason reason); I think you can delete these variable names. > WebKit/gtk/webkit/webkitwebresourceprivate.h:46 > +WebKitWebResource* > +webkit_web_resource_new_with_core_resource(PassRefPtr<WebCore::ArchiveResource>); > + > +void > +webkit_web_resource_init_with_core_resource(WebKitWebResource*, PassRefPtr<WebCore::ArchiveResource>); I think maybe no extra newlines here. (In reply to comment #18) > > WebKit/gtk/webkit/webkitwebresourceprivate.h:46 > > +WebKitWebResource* > > +webkit_web_resource_new_with_core_resource(PassRefPtr<WebCore::ArchiveResource>); > > + > > +void > > +webkit_web_resource_init_with_core_resource(WebKitWebResource*, PassRefPtr<WebCore::ArchiveResource>); > > I think maybe no extra newlines here. These should probably become kitNew() and kit(), I guess. Attachment 76218 [details] was posted by a committer and has review+, assigning to Gustavo Noronha Silva for commit.
Created attachment 77720 [details]
final patch
ok, this is a rather big patch, but I think it makes sense to go in one go; it finally removes webkitprivate, and adds a new webkitgeneral module that holds the non-class-specific stuff, both public and private;
Attachment 77720 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7224331 Created attachment 77803 [details]
final split, not with new files actually being added
Forgot to git add the new webkitgeneral* files.
Comment on attachment 77803 [details] final split, not with new files actually being added View in context: https://bugs.webkit.org/attachment.cgi?id=77803&action=review Looks great! There are a couple issues below, which should be fixed before landing (where appropriate). In general, I think that webkitglobals makes more sense than webkitgeneral, but that's a matter of opinion. > WebKit/gtk/webkit/webkitgeneral.cpp:75 > +SoupSession* webkit_get_default_session () There's an extra space here. > WebKit/gtk/webkit/webkitgeneral.cpp:220 > +void webkit_init() Do mind renaming this with camelCase now, to match WebKit style? > WebKit/gtk/webkit/webkitgeneralprivate.h:38 > +#define WEBKIT_PARAM_READABLE ((GParamFlags)(G_PARAM_READABLE|G_PARAM_STATIC_NAME|G_PARAM_STATIC_NICK|G_PARAM_STATIC_BLURB)) > +#define WEBKIT_PARAM_READWRITE ((GParamFlags)(G_PARAM_READWRITE|G_PARAM_STATIC_NAME|G_PARAM_STATIC_NICK|G_PARAM_STATIC_BLURB)) These are just defines, so they should probably go at the top of the file. I don't think they need to be in an extern "C" block. > WebKit/gtk/webkit/webkitgeneralprivate.h:40 > +void webkit_init(); Is this ever called from a C file so that it needs to be in an extern "C"? I think it is probably unnecessary now and can be in the WebKit block above. > WebKit/gtk/webkit/webkitwebsettings.cpp:1449 > + return (WebCore::EditingBehaviorType)type; This function just does a cast. I'd much rather it be removed completely and replaced with static_cast<WebCore::EditingBehaviorType(...) at all callsites. That avoids a function call. > WebKit/gtk/webkit/webkitwebsettingsprivate.h:41 > GSList* webkit_web_settings_get_enchant_dicts(WebKitWebView*); > > +WTF::String webkitUserAgent(); > + If these aren't exported, so I think they belong outside the extern "C" block and in a WebKit block, just for clarity. Comment on attachment 77803 [details] final split, not with new files actually being added Landed as r74933. I ended up not moving some functions out of extern C as requested because they ended up causing undefined references. Why do I always forget to git add a few files? http://trac.webkit.org/changeset/74933 might have broken SnowLeopard Intel Release (Tests) The following tests are not passing: http/tests/xmlhttprequest/remember-bad-password.html |