Bug 50698 – [GTK] Split webkitprivate.{cpp,h} in more manageable chunks

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Gustavo Noronha (kov)]

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.

[Gustavo Noronha (kov)]

Created attachment 75930 [details]
split webkitwebviewprivate.h out of webkitprivate.h

[Martin Robinson]

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.

[Gustavo Noronha (kov)]

Created attachment 75932 [details]
split webkitwebviewprivate.h

This should fix all the issues you raised =)

[Martin Robinson]

Comment on attachment 75932 [details]
split webkitwebviewprivate.h

Okay!

[Gustavo Noronha (kov)]

Comment on attachment 75932 [details]
split webkitwebviewprivate.h

Landed in r73539.

[Gustavo Noronha (kov)]

Created attachment 76091 [details]
split WebKitWebFrame

[Gustavo Noronha (kov)]

Created attachment 76100 [details]
split WebKitNetworkRequest

[Gustavo Noronha (kov)]

Created attachment 76103 [details]
split web inspector

[Martin Robinson]

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.

[Martin Robinson]

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.

[Gustavo Noronha (kov)]

Comment on attachment 76091 [details]
split WebKitWebFrame

Landed as r73696.

[Gustavo Noronha (kov)]

Comment on attachment 76100 [details]
split WebKitNetworkRequest

Landed as r73701.

[Gustavo Noronha (kov)]

Comment on attachment 76103 [details]
split web inspector

Landed as r73702.

[Gustavo Noronha (kov)]

Created attachment 76192 [details]
Patch

[Gustavo Noronha (kov)]

Created attachment 76218 [details]
Patch

[WebKit Review Bot]

http://trac.webkit.org/changeset/73701 might have broken GTK Linux 32-bit Debug

[WebKit Review Bot]

http://trac.webkit.org/changeset/73702 might have broken GTK Linux 32-bit Debug

[Martin Robinson]

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.

[Gustavo Noronha (kov)]

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

[Eric Seidel (no email)]

Attachment 76218 [details] was posted by a committer and has review+, assigning to Gustavo Noronha Silva for commit.

[Gustavo Noronha (kov)]

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;

[WebKit Review Bot]

Attachment 77720 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7224331

[Gustavo Noronha (kov)]

Created attachment 77803 [details]
final split, not with new files actually being added

Forgot to git add the new webkitgeneral* files.

[Martin Robinson]

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.

[Gustavo Noronha (kov)]

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.

[Gustavo Noronha (kov)]

Why do I always forget to git add a few files?

[WebKit Review Bot]

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