Bug 50698 - [GTK] Split webkitprivate.{cpp,h} in more manageable chunks
Summary: [GTK] Split webkitprivate.{cpp,h} in more manageable chunks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Gustavo Noronha (kov)
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2010-12-08 10:47 PST by Gustavo Noronha (kov)
Modified: 2011-01-03 14:07 PST (History)
5 users (show)

See Also:


Attachments
split webkitwebviewprivate.h out of webkitprivate.h (29.58 KB, patch)
2010-12-08 10:53 PST, Gustavo Noronha (kov)
mrobinson: review-
Details | Formatted Diff | Diff
split webkitwebviewprivate.h (29.64 KB, patch)
2010-12-08 11:20 PST, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
split WebKitWebFrame (12.88 KB, patch)
2010-12-09 10:39 PST, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
split WebKitNetworkRequest (16.97 KB, patch)
2010-12-09 11:43 PST, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
split web inspector (9.32 KB, patch)
2010-12-09 12:11 PST, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
Patch (8.40 KB, patch)
2010-12-10 07:21 PST, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
Patch (51.77 KB, patch)
2010-12-10 10:32 PST, Gustavo Noronha (kov)
mrobinson: review+
Details | Formatted Diff | Diff
final patch (50.34 KB, patch)
2010-12-31 07:56 PST, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
final split, not with new files actually being added (63.78 KB, patch)
2011-01-03 04:55 PST, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gustavo Noronha (kov) 2010-12-08 10:47:14 PST
We currently have this huge collection of definitions, and code called webkitprivate. Splitting it into per-class files will make it more manageable and simpler.
Comment 1 Gustavo Noronha (kov) 2010-12-08 10:53:05 PST
Created attachment 75930 [details]
split webkitwebviewprivate.h out of webkitprivate.h
Comment 2 Martin Robinson 2010-12-08 11:06:42 PST
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.
Comment 3 Gustavo Noronha (kov) 2010-12-08 11:20:27 PST
Created attachment 75932 [details]
split webkitwebviewprivate.h

This should fix all the issues you raised =)
Comment 4 Martin Robinson 2010-12-08 11:30:29 PST
Comment on attachment 75932 [details]
split webkitwebviewprivate.h

Okay!
Comment 5 Gustavo Noronha (kov) 2010-12-09 10:38:17 PST
Comment on attachment 75932 [details]
split webkitwebviewprivate.h

Landed in r73539.
Comment 6 Gustavo Noronha (kov) 2010-12-09 10:39:45 PST
Created attachment 76091 [details]
split WebKitWebFrame
Comment 7 Gustavo Noronha (kov) 2010-12-09 11:43:31 PST
Created attachment 76100 [details]
split WebKitNetworkRequest
Comment 8 Gustavo Noronha (kov) 2010-12-09 12:11:41 PST
Created attachment 76103 [details]
split web inspector
Comment 9 Martin Robinson 2010-12-09 17:02:56 PST
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 10 Martin Robinson 2010-12-09 17:16:00 PST
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 11 Gustavo Noronha (kov) 2010-12-10 02:08:18 PST
Comment on attachment 76091 [details]
split WebKitWebFrame

Landed as r73696.
Comment 12 Gustavo Noronha (kov) 2010-12-10 02:26:31 PST
Comment on attachment 76100 [details]
split WebKitNetworkRequest

Landed as r73701.
Comment 13 Gustavo Noronha (kov) 2010-12-10 02:27:27 PST
Comment on attachment 76103 [details]
split web inspector

Landed as r73702.
Comment 14 Gustavo Noronha (kov) 2010-12-10 07:21:55 PST
Created attachment 76192 [details]
Patch
Comment 15 Gustavo Noronha (kov) 2010-12-10 10:32:15 PST
Created attachment 76218 [details]
Patch
Comment 16 WebKit Review Bot 2010-12-10 11:45:17 PST
http://trac.webkit.org/changeset/73701 might have broken GTK Linux 32-bit Debug
Comment 17 WebKit Review Bot 2010-12-10 11:45:23 PST
http://trac.webkit.org/changeset/73702 might have broken GTK Linux 32-bit Debug
Comment 18 Martin Robinson 2010-12-10 15:32:25 PST
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.
Comment 19 Gustavo Noronha (kov) 2010-12-11 03:29:33 PST
(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.
Comment 20 Eric Seidel (no email) 2010-12-14 15:14:14 PST
Attachment 76218 [details] was posted by a committer and has review+, assigning to Gustavo Noronha Silva for commit.
Comment 21 Gustavo Noronha (kov) 2010-12-31 07:56:10 PST
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;
Comment 22 WebKit Review Bot 2010-12-31 21:40:50 PST
Attachment 77720 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7224331
Comment 23 Gustavo Noronha (kov) 2011-01-03 04:55:42 PST
Created attachment 77803 [details]
final split, not with new files actually being added

Forgot to git add the new webkitgeneral* files.
Comment 24 Martin Robinson 2011-01-03 09:05:04 PST
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 25 Gustavo Noronha (kov) 2011-01-03 13:20:52 PST
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.
Comment 26 Gustavo Noronha (kov) 2011-01-03 13:37:08 PST
Why do I always forget to git add a few files?
Comment 27 WebKit Review Bot 2011-01-03 14:07:48 PST
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