Summary: | [GTK] Use a consistent coding style | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Christian Dywan <christian> | ||||||||
Component: | WebKitGTK | Assignee: | Christian Dywan <christian> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | alp | ||||||||
Priority: | P2 | Keywords: | Gtk | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
Attachments: |
|
Description
Christian Dywan
2008-01-29 07:18:52 PST
Created attachment 18762 [details]
Apply consistent coding style
Incidentally this should *not* be confused with bug 16676. WebKit coding style is kept. - 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. 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. 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. 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? 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. 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). Comment on attachment 18879 [details]
Updated again
r=me
ChangeLog entry next time please.
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. |