Bug 15691 – [GTK] Public API does not follow GTK+ conventions

• Component: New Bugs
• Status: RESOLVED FIXED

[Alp Toker]

There is evidence that the WebKitPage class should instead be named *View to match GTK+ convention of widgets to be added to scrolled containers eg. TreeView and TextView.

It may also be prudent to follow GTK+ coding style in classes under WebKit/gtk/Api/ and perhaps WebKitSupport since its very much the convention when you are defining GObject classes in boilerplate-heavy C code.

We may also want to introduce much of the new API in one go to avoid the situation where people add bits and pieces to scratch an itch, making it inconsistent. To this end, we may want to borrow from/adopt:

http://gtk-webcore.svn.sourceforge.net/viewvc/gtk-webcore/trunk/WebkitGtk/

I'm not entirely convinced by that API either. The delegate classes would need to go. Some of the entry point names are also rather lengthy.

We need to discuss these issues so we can move forward with a public API for WebKit/Gtk+ that can be supported in the long term, or even recommend for use in applications at all.

[Holger Freyther]

The main reason I didn't base my work on this API where the 1:1 copy of the Mac API where specially the delegates didn't feel native.

The current API is (almost) good enough for the OpenMoko Browser, Feedreader and pidgin.im so I would like to see an evolutionary API design driven by actual demand instead of blindly throwing things away. But these are just my two cents.

[Xan Lopez]

(In reply to comment #0)
> There is evidence that the WebKitPage class should instead be named *View to
> match GTK+ convention of widgets to be added to scrolled containers eg.
> TreeView and TextView.
> 
> It may also be prudent to follow GTK+ coding style in classes under
> WebKit/gtk/Api/ and perhaps WebKitSupport since its very much the convention
> when you are defining GObject classes in boilerplate-heavy C code.

+1 for this if there's no problem in doing it. The people more likely to contribute to that code (I guess) are used to that style.

> 
> We may also want to introduce much of the new API in one go to avoid the
> situation where people add bits and pieces to scratch an itch, making it
> inconsistent. To this end, we may want to borrow from/adopt:
> 
> http://gtk-webcore.svn.sourceforge.net/viewvc/gtk-webcore/trunk/WebkitGtk/
> 
> I'm not entirely convinced by that API either. The delegate classes would need
> to go. Some of the entry point names are also rather lengthy.
> 
> We need to discuss these issues so we can move forward with a public API for
> WebKit/Gtk+ that can be supported in the long term, or even recommend for use
> in applications at all.
> 

Now that the ground work in Epiphany is done and we can start implementing some big missing chunks in the webkit port I hope I'll be able to provide some feedback there.

[Alp Toker]

A couple of things to add:

(1) http://bugs.webkit.org/show_bug.cgi?id=16042
[GTK] Eliminate webkit_init()

(2) Next time we make a breaking change, we should add padding to the structures for future expansion.

[Alp Toker]

In the WebKit GTK+ port's public API today, we have types and functions named like this:

  WebKitPage
  WebKitFrame
  WebKitNetworkRequest

  WebKitFrame *
  webkit_page_get_main_frame (WebKitPage *page);

We want to move away from this because:

  (1) "WebKitPage" doesn't follow GTK+ conventions. Widgets that are usually packed in a GtkScrolledWindow and represent a view are called GtkTextView, GtkTreeView, GtkIconView etc.

  (2) In some language bindings, "WebKit" would be considered the namespace and you'd be left with generic top-level type names like "Frame" which would annoyingly conflict with "Frame" from, say, the Gtk namespace.

  (3) What is a "Page"? Is it data? Is it a widget? It's too ambiguous.

So, we want to move to a naming convention similar to that of the Win and Mac ports. This means providing top-level types named *WebView and *WebFrame.

However, if we keep the current namespace "WebKit", things start to get verbose and repetitive:

  WebKitWebView
  WebKitWebFrame
  WebKitNetworkRequest

  WebKitWebFrame *
  webkit_web_view_get_main_frame (WebKitWebView *view);

One alternative proposed is simply to prepend "G" (though this looks like namespace pollution to me):

  GWebView
  GWebFrame
  GNetworkRequest

  GWebFrame *
  g_web_view_get_main_frame (GWebView *view);

Another proposal is to follow the example of GTK+. GIMP ToolKit shortens to "Gtk" in the API, so we'd follow suit and use the namespace "Wk":

  WkWebView
  WkWebFrame
  WkNetworkRequest

  WkWebFrame *
  wk_web_view_get_main_frame (WkWebView *view);

Whether we go for one of these schemes or a better proposal comes along, the API chosen now will probably be around for a few years to come. If you have a preference or disagree with my analysis, please speak up!

[Xan Lopez]

My opinion is that GObject C is by its very own nature verbose, so using artificially short/confusing APIs to save typing is just bad. If you don't want to type use bindings or some kind of autocompletion, there's plenty of solutions around :)

So I'd go with something like WebKitWebView, webkit_web_view_new (), etc

Using wk as namespace in the API might be ok too, although my first impression is "ugh". We use a similar trick in Epiphany, using "ephy" as namespace for our API, but I'd say ephy sounds better than wk :)

[Alp Toker]

Yet another inspiration comes from gtkhtml (also namespace pollution):

  GtkWebView
  GtkWebFrame
  GtkNetworkRequest

  GtkWebFrame *
  gtk_web_view_get_main_frame (GtkWebView *view);


On another subject, GTK+/GLib use "URI" terminology instead of "URL", which WebKit uses. I'm thinking we should follow the former here.

[Xan Lopez]

Using the gtk_ prefix outside GTK+ is really a no-no IMHO.

[Sven Herzberg]

(In reply to comment #7)
> Using the gtk_ prefix outside GTK+ is really a no-no IMHO.

I totally agree here. Using the GTK prefix should happen if and only if this is being coordinated with the GTK people.

[Alp Toker]

Created attachment 17501 [details]
API and header improvements

This patch does not yet break ABI or API in any significant way.

[Jan Alonzo]

(In reply to comment #4)
> 
> However, if we keep the current namespace "WebKit", things start to get verbose
> and repetitive:
> 
>   WebKitWebView
>   WebKitWebFrame
>   WebKitNetworkRequest
> 
>   WebKitWebFrame *
>   webkit_web_view_get_main_frame (WebKitWebView *view);

In my opinion being verbose is fine because redundancy only happens for web-related components. I also agree that we should follow naming convention of Mac/Win ports because it's a big help (for reference purposes) for people who are learning the API, and webkit code in general.

[Christian Dywan]

>   WebKitWebView
>   WebKitWebFrame
>   WebKitNetworkRequest
> 
>   WebKitWebFrame *
>   webkit_web_view_get_main_frame (WebKitWebView *view);

This looks like a very nice scheme.

However shouldn't it be WebkitWebView to actually match webkit_web_view_new speaking of capitalization?

I would also like to see WebkitWebView* treated like a widget unlike WebKitPage* currently. For example:

 WebKitPage* webkit_page_new

should be

 GtkWidget* webkit_web_view_new

[Alp Toker]

Comment on attachment 17501 [details]
API and header improvements

Already reviewed by Mark and landed in r28013. Taking this out of the review queue.

[Alp Toker]

Comment on attachment 17501 [details]
API and header improvements

Patch already landed, but the bug report needs to remain open. Removing r+ flag to clear the commit queue.

[Christian Dywan]

Changing the term Url to Uri is desirable, possibly also ^^location^^uri.

Using existing function and signal schemes would also be nice at places where it makes sense. Afterall this is about integration.

[Alp Toker]

Created attachment 17613 [details]
Refactor as planned

Would prefer to add ChangeLog entries post-review due to the number of changed files.

[Alp Toker]

Created attachment 17614 [details]
With corrected documentation

Internal variables in WebCoreSupport still need to be renamed but that's a style issue that I'll deal with in a follow-up patch.

[Alp Toker]

Created attachment 17615 [details]
With ChangeLog entries

[Christian Dywan]

(In reply to comment #15)
> Created an attachment (id=17613) [edit]
> Refactor as planned

This is starting to look good. However allow me a few comments.

> WebKitWebFrame* webkit_web_frame_init_with_web_view(WebKitWebView* webView, HTMLFrameOwnerElement* element)

I believe this is meant to be static. And while you're at it you might give it a n appropriate name, such as webkit_web_frame_init_with_element.

> WebKitWebView* webkit_web_frame_get_web_view(WebKitWebFrame* frame)
This should return GtkWidget*.

> +void webkit_web_view_go_backward(WebKitWebView* webView)
This should be s/backward/back. 

> WebKitSettings* webkit_web_settings_copy
This doesn't match, it should return WebKitWebSettings*.

> WebKitWebView != webkit_web_view
This capitalization is errorneous, WebkitWebView would be correct for webkit_web_view.

[Xan Lopez]

(In reply to comment #18)
> > WebKitWebView != webkit_web_view
> This capitalization is errorneous, WebkitWebView would be correct for
> webkit_web_view.
> 

But it's called WebKit, and web_kit_* is just too awkward. I'd say it's a sensible compromise.

[Xan Lopez]

Actually if you are ultra-pedantic, the underscores separate words, not capitalization. So webkit_* is correct because WebKit is used as one word that happens to have funny capitalization.

[Christian Dywan]

(In reply to comment #20)
> Actually if you are ultra-pedantic, the underscores separate words, not
> capitalization. So webkit_* is correct because WebKit is used as one word that
> happens to have funny capitalization.

Little things like that can easily cause confusion. I am not just being pedantic here. We don't want users to compain and answer "oh, this just happens to be a bad idea", do we? ;)

I think Webkit* would be an option. If we intentionally want to keep errorneous capitalization it must be clearly documented.

[Alp Toker]

(In reply to comment #18)
> (In reply to comment #15)
> > Created an attachment (id=17613) [edit]
> > Refactor as planned
> 
> This is starting to look good. However allow me a few comments.
> 
> > WebKitWebFrame* webkit_web_frame_init_with_web_view(WebKitWebView* webView, HTMLFrameOwnerElement* element)
> 
> I believe this is meant to be static. And while you're at it you might give it
> a n appropriate name, such as webkit_web_frame_init_with_element.
> 
> > WebKitWebView* webkit_web_frame_get_web_view(WebKitWebFrame* frame)
> This should return GtkWidget*.
> 
> > +void webkit_web_view_go_backward(WebKitWebView* webView)
> This should be s/backward/back. 
> 
> > WebKitSettings* webkit_web_settings_copy
> This doesn't match, it should return WebKitWebSettings*.
> 
> > WebKitWebView != webkit_web_view
> This capitalization is errorneous, WebkitWebView would be correct for
> webkit_web_view.
> 

If you look at the ChangeLog entry you'll find this patch deals only with refactoring WebKitPage -> WebKitWebView and WebKitFrame -> WebKitWebFrame.

The capitalisation is actually more correct this way. All language binding generators I know of deal fine with this case.

The other issues sound valid and should be fixed in individual patches, or at least not in this one.

[Christian Dywan]

(In reply to comment #22)
> [snip]
> If you look at the ChangeLog entry you'll find this patch deals only with
> refactoring WebKitPage -> WebKitWebView and WebKitFrame -> WebKitWebFrame.
> 
> The capitalisation is actually more correct this way. All language binding
> generators I know of deal fine with this case.
> 
> The other issues sound valid and should be fixed in individual patches, or at
> least not in this one.

Okay, I accept that WebKit* is a special case. If you prefer to address the other issues in a separate patch that is of course just as fine as long as they are not lost.

[Xan Lopez]

Just tested the patch (with GtkLauncher), everything seems to work fine.

As Christian says, the API is much better now, so this is good to go IMHO.

[Adam Roben (:aroben)]

Comment on attachment 17615 [details]
With ChangeLog entries

Looks like there is some inconsistent placement of * in webkitwebview.h

It's a bit hard to review this patch because it looks like svn mv was not used for many of the files in WebKit/gtk/WebView. But I trust you to get the renames right :-)

r=me

[Alp Toker]

Patch landed in r28273.

We've been asked repeatedly to close bugs once the patch is landed, so I've tried to salvage as many issues as I could from the thread here before closing this bug:

http://bugs.webkit.org/show_bug.cgi?id=16219
[GTK] API: WebKitWebSettings is not usable

http://bugs.webkit.org/show_bug.cgi?id=16218
[GTK] API: Should this entry point be called go_back rather than go_backward?

I didn't have time to file bugs for these (reasonable) API issues. Please try to file new bugs for these issues and any others I missed so they don't get lost:

> WebKitWebFrame* webkit_web_frame_init_with_web_view(WebKitWebView* webView, HTMLFrameOwnerElement* element)

I believe this is meant to be static. And while you're at it you might give it
a n appropriate name, such as webkit_web_frame_init_with_element.

> WebKitWebView* webkit_web_frame_get_web_view(WebKitWebFrame* frame)
This should return GtkWidget*.