Bug 17065 – [GTK] Use a consistent coding style

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Christian Dywan]

The way sources are formatted is still very inconsistent, regarding naming, structure and doing the same task in different ways.

This patch attempts to introduce a throughout convention that we can agree upon for future contributions. Further more by avoiding superfluous typechecks code becomes more efficient.

Some of the changes are:
1. Defining all gobject types in webkitdefines.h.
2. Using 'priv' as the name of the private structure and using ->priv throughout the source without a FOO_GET_PRIVATE macro.
3. Renaming WebKitWebFramePrivate->frame to 'coreFrame'.
4. Preferring private members over public api.

There are no api or functional changes.

[Christian Dywan]

Created attachment 18762 [details]
Apply consistent coding style

[Christian Dywan]

Incidentally this should *not* be confused with bug 16676. WebKit coding style is kept.

[Alp Toker]

-        g_value_set_string(value, webkit_web_frame_get_name(frame));
+        g_value_set_string(value, priv->name);

I'd like to keep the property implementations as abstracted/simple as possible, always wrapping a function, so that we don't have to keep the implementations up to date in two places. So I think this particular change is is a step backwards.


The core()/kit() removal is an interesting proposal. core()/kit() are quite convenient and they give us the opportunity to refactor the implementation without lots of search and replace. You could argue that force an unnatural one-to-one mapping of WebKitWebView -> WebCore::Page though. Unless there is a good case for the change, I'm not sure it's worth doing right now, especially core()/kit() for WebKitWebFrame which is a one-to-one with WebCore.

Renaming all private locals to "priv" seems a good move.


-    Frame* frame = core(webkit_web_view_get_main_frame(WEBKIT_WEB_VIEW(widget)));
+    WebKitWebView* webView = WEBKIT_WEB_VIEW(widget);
+    WebKitWebViewPrivate* priv = webView->priv;
+
+    Frame* frame = priv->corePage->mainFrame();


^ In cases like this, there's a question of whether we want to dogfood our own public API or use WebCore in the porting layer. We need to weigh this up.


Changes like this look OK:

 struct _WebKitWebFrameClass {
-    GObjectClass parent;
+    GObjectClass parent_class;


The doc changes look fine.

This comment isn't worthwhile:

+    // FIXME: DeprecatedString is deprecated


I'd like to get a little bit of discussion about these cleanups going. If you want you can propose a smaller patch the most obvious changes in the meantime.

[Alp Toker]

This change may also break a few feature patches various people are working on that haven't landed yet. This shouldn't blocker code cleanup patches like this, but we need to be sure the cleanups are genuinely an improvement.

[Christian Dywan]

Created attachment 18851 [details]
Updated patch

Properties using public api again.

Reintroducing core() and kit(), to be used consistently. Their implementations now uses ->priv directly for efficiency.

Keeping changes to prefer WebCore internals over dogfood for efficiency.
> Frame* frame = priv->corePage->mainFrame();
has become
> Frame* frame = core(webView)->mainFrame();
due to the above change. I think this is a nice compromise in terms of readability. If we use this consistently it should be pretty intuitive even to newcomers.

[Alp Toker]

This comment is wrong:

  * Checks whether the view can go back to the previous page
  *
+ * @Deprecated: Use webkit_web_view_go_back() instead.
+ *
  * Return value: %TRUE if the page can go back, otherwise returns %FALSE
  */
 gboolean webkit_web_view_can_go_backward(WebKitWebView* webView)


Is it conventional to use ->priv instead of WEBKIT_WEB_VIEW_GET_PRIVATE()/G_TYPE_INSTANCE_GET_PRIVATE() in GObject code? What are your reasons for accessing the struct directly?



I was trying to explain earlier that I'm not convinced we should encourage a one-to-one mapping of WebCore::Page to/from WebKitWebView, as changes like this do:

-    WebKitWebViewPrivate* webViewData = WEBKIT_WEB_VIEW_GET_PRIVATE(webView);
-    return !webViewData->corePage->selection().isNone();
+    return !core(webView)->selection().isNone();

So the question is whether core(webView) returning a corePage is sensible or bogus. I guess if other ports do it this way (I think they do) there may be sense in it, so it may be OK. Thoughts?

[Christian Dywan]

Created attachment 18879 [details]
Updated again

> Is it conventional to use ->priv instead of
> WEBKIT_WEB_VIEW_GET_PRIVATE()/G_TYPE_INSTANCE_GET_PRIVATE() in GObject
> code? What are your reasons for accessing the struct directly?

Yes, it is conventional in 'modern' code (you can't change old gtk code for the sake of ABI). The reason is simply efficiency: choose directly using pointers versus retrieving data dynamically through type safety checks. We *know* what that pointer is, we defined it. So type checks that tend to be slow are superfluous.
Incidentally that's the reason for not using "g_return_if_fail(FOO_IS_BAR(bar))" in static functions.

> So the question is whether core(webView) returning a corePage is sensible
> or bogus. I guess if other ports do it this way (I think they do) there may
> be sense in it, so it may be OK. Thoughts?

The core/ kit mapping is somewhat unusual 'magic'. In fact I would rather tend to remove it for the slightest doubts, since there is *no* one-to-one mapping in every case and keeping it only for part of the classes would be confusing. *If* we use them we really need to do it throughout.
In case of doubt, the most import thing is to either use it or not. If nobody speaks up about this, we really need to use it consistently instead of keeping the current mixture even if there are doubts.

[Alp Toker]

I spoke with mjs and he's convinced me that the kit/core mapping is good (apart from the naming, but we needn't change that now).

[Alp Toker]

Comment on attachment 18879 [details]
Updated again

r=me

ChangeLog entry next time please.

[Alp Toker]

Landed in r29985. I had to merge the patch manually in a few places, please take a look over the commit to make sure everything is in order.