Bug 14678 – [gdk] API Drafting

• Component: New Bugs
• Status: RESOLVED FIXED

[Holger Freyther]

I have taken a look at WebView, gtk-webcore, WebKitQt and my chosen rolemodel is the Qt API. Both 'languages' have the concept of signal and slots/closures which feel more native than delegates.

IMHO we should have a Page/Frame split as well, signals and signals with default handlers to implement policy and notification. None of the internal WebCore names should be in the header files, this can be achieved by using GObject and PIMPL paradigma. The secret  would still be shared among several classes which should be okay.
For the API I'm going to follow the Qt route and will search for some test rabbits.

[Holger Freyther]

Created attachment 15587 [details]
Headers of the API for review.

First draft (only headers) of an API following WebKitQt. Known violation of Coding-Style include placing of braces and new lines in the header file (to some how match glib/gtk+).

Please spare comments, e.g. if this route would be acceptable, source-code location is the right one, etc...

[Christian Dywan]

1. There are no signals which indicate that a file (be it a webpage, an image or anything else) is about to be loaded. This is essential for being able to block uris, allow for custom protocols, handling downloads and custom behavior, for example loading images in a special image viewing application. It should be possible to retrieve the mime type of the file.

2. There is no indication of loading progress including loading of resources (filenames).

3. There is no signal when the loading has finished.

4. There are no signals for creating, modifying or deleting cookies. These three events might fit in one 'cookie' signal.

5. I suggest the following signals to be changed to match existing gtk naming:

From
 mouse_move_event, mouse_press_event, mouse_release_event
 , mouse_double_click_event, mouse_wheel_event

To
 motion-notify-event, button-press-event, button-release-event

The double_click and wheel_event would come along with button_press_event.

5. We need a documentation of the api, especially for the not so abvious parts.

[Maciej Stachowiak]

I think using signals as the notification mechanism makes sense for a Gtk+ API. However, I think for other things it's worth looking at the WebKit mac API (and the win API modelled on it but with some simplifications). The mac API has been proven in use in multiple production-quality browsers, a mail client, many chat programs, a number of newsreaders, and many other applications. Developers usually say it is pretty good.

Another possible advantage is that being closer to the mac and win API will make it easier to write cross-platform apps that still want a native version of webkit on each platform.

Even when it comes to signals, looking at the WebKit delegate methods will probably give a good idea of which things should have signals (don't see any signals so far in these headers, I guess those are not defined in the header for Gtk?). We've been careful to let the app override many key decisions and collect full information for reporting on the page load. At the same time we give sensible defaults so the API is easy to use.

[Holger Freyther]

(In reply to comment #3)
> I think using signals as the notification mechanism makes sense for a Gtk+ API.
> However, I think for other things it's worth looking at the WebKit mac API (and
> the win API modelled on it but with some simplifications). The mac API has been
> proven in use in multiple production-quality browsers, a mail client, many chat
> programs, a number of newsreaders, and many other applications. Developers
> usually say it is pretty good.

I have been looking at it a lot, and will continue to do so. The vast amount of applications being able to use it is prove enough.



> 
> Another possible advantage is that being closer to the mac and win API will
> make it easier to write cross-platform apps that still want a native version of
> webkit on each platform.

Conceptually WebView, WebKitQt and WebKitGtk are close enough to hopefully to be able to do so.


> 
> Even when it comes to signals, looking at the WebKit delegate methods will
> probably give a good idea of which things should have signals (don't see any
> signals so far in these headers, I guess those are not defined in the header
> for Gtk?). We've been careful to let the app override many key decisions and
> collect full information for reporting on the page load. At the same time we
> give sensible defaults so the API is easy to use.
> 

Yes, g_signal_new is not used in the header, you will see function pointers in the *Class struct's but these are either virtual methods or just default handlers for signals (where I think they make sense). E.g. there is no use of having a default handler for load progress so you won't see a trace in the header file.

I have been reading the WebView and WebKitQt API that much I really wonder what to put in the copyright line of the API :}

[Holger Freyther]

Created attachment 15603 [details]
Partial implementation of the API

This is a partial implementation of the API:
   -it violates the CodingStyle in the header file but I vote for it to stay
   -it is only partial (if you use some other functions you will get link errors)
   -Implementation of it will be changed when multiple frame support will be implemented (API should stay the same)
   -No signals are created, none emitted
   -Switch over GdkLauncher to it

I would vote for landing this API as draft and start to implement it. It will take some time before we can think of API stability.

[Holger Freyther]

Created attachment 15624 [details]
Partial implementation of the API

Start implementing signals. The only controversial change is in FrameGdk to return the associated WebKitGtkFrame*.

[Adam Roben (:aroben)]

Comment on attachment 15624 [details]
Partial implementation of the API

Index: WebCore/loader/gdk/FrameLoaderClientGdk.cpp

Now that a WebKit/gtk is starting to exist, all these client implementations should move out of WebCore and into WebKit/gtk. ChromeClientGdk.cpp is missing from your patch but it looks like you put it in platform/gdk, which is definitely not the right place (even through that's where some of the other clients are at the moment).

+    WebKitGtkFrame* frame = static_cast<FrameGdk*>(m_frame)->gtkFrame();

In Mac/Windows WebKit we have a set of overloaded kit()/core() functions to do this kind of translation between WebKit and WebCore types.

+typedef struct _WebKitGtkPage WebKitGtkPage;

You can just say `struct WebKitGtkPage` since this is C++ code.

+typedef struct _WebKitGtkFrame WebKitGtkFrame;

Ditto.

+    WebKitGtkFrame* gtkFrame() const { return m_gtkFrame; }

You shouldn't be holding onto WebKit types inside WebCore.  On Windows we get back to the WebFrame* through the FrameLoaderClient* pointer (a little ugly, but it keeps the separation).

I'm not going to look at the code in Api/ since I'm unfamiliar with Gtk+. It looks like this patch is a big step forward, but r- based on ChromeClientGdk.cpp being in platform/gdk.

[Adam Roben (:aroben)]

(In reply to comment #7)
> (From update of attachment 15624 [details] [edit])
> +    WebKitGtkFrame* gtkFrame() const { return m_gtkFrame; }
> 
> You shouldn't be holding onto WebKit types inside WebCore.  On Windows we get
> back to the WebFrame* through the FrameLoaderClient* pointer (a little ugly,
> but it keeps the separation).

I was a little unclear here. On Windows, WebFrame is the class that implements WebCore::FrameLoaderClient, so we just static_cast the FrameLoaderClient* to a WebFrame*, because we know within WebKit that that's what it is. Since you have a separate FrameLoaderClientGdk class, I guess that's where the WebKitGtkFrame* pointer should go.

[Holger Freyther]

(In reply to comment #7)
> (From update of attachment 15624 [details] [edit])

> 
> +    WebKitGtkFrame* frame = static_cast<FrameGdk*>(m_frame)->gtkFrame();
> 
> In Mac/Windows WebKit we have a set of overloaded kit()/core() functions to do
> this kind of translation between WebKit and WebCore types.
> 
> +typedef struct _WebKitGtkPage WebKitGtkPage;
> 
> You can just say `struct WebKitGtkPage` since this is C++ code.
> 
> +typedef struct _WebKitGtkFrame WebKitGtkFrame;
> 
> Ditto.
> 

I gave it a try and ended with this (the API is C).

../../../WebCore/platform/gdk/FrameGdk.h:36: error: conflicting declaration 'typedef struct _WebKitGtkFrame WebKitGtkFrame'
../../../WebCore/loader/gdk/FrameLoaderClientGdk.h:36: error: 'struct WebKitGtkFrame' has a previous declaration as 'struct WebKitGtkFrame'

[Holger Freyther]

Created attachment 15629 [details]
Partial implementation of the API

-Make the WebKitGtkpage a GtkLayout (for now) which needs to be embedded into a GtkScrolledWindow
-Rename the namespace from WebKitGtk to WebCore and add core/kit methods converting from WebKitGtkFrame (WebFrame) to WebCore::Frame and the same for page.
-Start reducing the implementation of FrameGdk, Move the WebKitGtkFrame (WebFrame) instance pointer into the FrameLoaderClientGdk which will be moved to WebKit/gtk/WebCoreSupport
-Move ChromeClientGdk to WebKit/gtk/WebCoreSupport

[Adam Roben (:aroben)]

Comment on attachment 15629 [details]
Partial implementation of the API

+    EditorClientGdk* editorClient = new EditorClientGdk;
+    private_data->page = new Page(new ChromeClientGdk(page), new ContextMenuClientGdk, editorClient, 0, new InspectorClientGdk);
+    editorClient->setPage(private_data->page);

Seems like EditorClientGdk should take a WebKitGtkPage* argument in its constructor. Just a FIXME is good enough for now.

+     * FIXME: Will be different with multiple frames

It would be good to say what/how/why this will be different so that you don't forget it and other people understand.

+    PassRefPtr<SharedBuffer> sharedBuffer = new SharedBuffer(strdup(content), strlen(content));    
+    SubstituteData substituteData(sharedBuffer, String(content_mime_type), String(content_encoding), KURL("about:blank"), url);

I believe the correct idiom here is to make sharedBuffer a RefPtr (not PassRefPtr), then pass `sharedBuffer.release()` into the SubstituteData constructor.

Nothing in WebKit/gtk should be in the WebCore namespace (though I realize that right now many things are in WebCore that should be in WebKit/gtk). I think you can move everything in webkitgtkprivate.cpp out of the WebCore namespace. Would be good to move ChromeClientGdk, too, though that could be a separate patch.

+FloatRect ChromeClientGdk::windowRect() {

+bool ChromeClientGdk::scrollbarsVisible() {

Brace should be on the next line.

+float ChromeClientGdk::scaleFactor()
+{
+    notImplemented();
+    return 1.0;
+}

Should be `1.0f` to avoid double -> float conversion.

+    if (!cresult)
+        return false;
+    else {
+        result = UTF8Encoding().decode(cresult, strlen(cresult));
+        g_free(cresult);
+        return true;
+    }

Don't need to say `else` after the return.

+    for my $i (0 .. $#ARGV) {
+        my $opt = $ARGV[$i];

Since you don't use $i anywhere else, you should just do:

foreach my $opt (@ARGV) {
}

r=me, though I think it would be nice to get someone with GTK+ experience to have a look at it (maybe Alp Toker?). You should also keep in mind what Maciej said about the benefits of keeping the API relatively close to the Mac/Windows API.

It's great to see all those WebCore headers gone from GdkLauncher!

[Holger Freyther]

Created attachment 15644 [details]
Partial implementation of the API

-Change webkitdirs.pm as proposed by Adam
-Move ChromeClientGdk and webkitgtkprivates.h to the WebKitGtk namespace and adapt to the changes
-Make EditorClientGdk take the WebKitGtkPage and use core() to access the WebCore::Page
-Clarify on the FIXME. GtkLayout will certainly go away once we implement multiple frames. I don't know the details on the replacement.
-Fixed the CodingStyle in the two ChromeClientGdk methods to put the opening curly braces on the newline.
-ChangeLog was not updated

[Holger Freyther]

Comment on attachment 15644 [details]
Partial implementation of the API

And ask for review

[Holger Freyther]

(In reply to comment #11)
> Nothing in WebKit/gtk should be in the WebCore namespace (though I realize that
> right now many things are in WebCore that should be in WebKit/gtk). I think you
> can move everything in webkitgtkprivate.cpp out of the WebCore namespace. Would
> be good to move ChromeClientGdk, too, though that could be a separate patch.

I have started with moving ChromeClientGdk into WebKitGtk namespace.

> +    if (!cresult)
> +        return false;
> +    else {
> +        result = UTF8Encoding().decode(cresult, strlen(cresult));
> +        g_free(cresult);
> +        return true;
> +    }

I forgot that, the final patch will have that changed as well.

 
> r=me, though I think it would be nice to get someone with GTK+ experience to
> have a look at it (maybe Alp Toker?). You should also keep in mind what Maciej
> said about the benefits of keeping the API relatively close to the Mac/Windows
> API.

Maciej did some handwaving that there is something in the WebKitQt API he didn't like and I might have already followed that. IMHO WebView <-> WebKitGtkPage. I have decided against the name View as IMHO this will trigger people to search for a Model as well (The toolkits using some flavor of Model-View-Controler have people trained to search for a model when they see the name View).

The good thing about WebView and its delegates is the grouping. You group policy, ui, form handling,... nicely. But with C++ and C/GObject I think using plain signals for informational events/notification/changes feels native. For the WebKitPolicyDelegate, after a small discussion with my GSoC mentor, we will go with signals with a default handler. This allows to connect to the signal or subclass them and will give a native feeling.
So the plan is to have methods found in WebView in WebKitGtkPage, most of WebFrame in WebKitGtkFrame. The delegates will be implementes as signals emitted mostly from WebKitGtkPage. It will be enough to just connect to WebKitGtkPage to get everything needed.

For some methods where you do [[webView _mainFrame] xyz:...] I'm tempted to place them in WebKitGtkPage directly and already did so.

I have not used the WebView myself, just had a look at the Examples installed in /Developers/Examples, but I think the APIs will be rouhgly similiar.


I have integrated WebKitGtkPage into my OpenMoko RSSReader, and there is a first version of midori using WebKitGtkPage as well. So the API is to some degree already usable and will slowly grow.

 
> It's great to see all those WebCore headers gone from GdkLauncher!

:)

[Adam Roben (:aroben)]

(In reply to comment #14)
> (In reply to comment #11)
> > Nothing in WebKit/gtk should be in the WebCore namespace (though I realize that
> > right now many things are in WebCore that should be in WebKit/gtk). I think you
> > can move everything in webkitgtkprivate.cpp out of the WebCore namespace. Would
> > be good to move ChromeClientGdk, too, though that could be a separate patch.
> 
> I have started with moving ChromeClientGdk into WebKitGtk namespace.

Great.

> > r=me, though I think it would be nice to get someone with GTK+ experience to
> > have a look at it (maybe Alp Toker?). You should also keep in mind what Maciej
> > said about the benefits of keeping the API relatively close to the Mac/Windows
> > API.
> 
> Maciej did some handwaving that there is something in the WebKitQt API he
> didn't like and I might have already followed that. IMHO WebView <->
> WebKitGtkPage. I have decided against the name View as IMHO this will trigger
> people to search for a Model as well (The toolkits using some flavor of
> Model-View-Controler have people trained to search for a model when they see
> the name View).

That seems fine to me.

> The good thing about WebView and its delegates is the grouping. You group
> policy, ui, form handling,... nicely. But with C++ and C/GObject I think using
> plain signals for informational events/notification/changes feels native. For
> the WebKitPolicyDelegate, after a small discussion with my GSoC mentor, we will
> go with signals with a default handler. This allows to connect to the signal or
> subclass them and will give a native feeling.
> So the plan is to have methods found in WebView in WebKitGtkPage, most of
> WebFrame in WebKitGtkFrame. The delegates will be implementes as signals
> emitted mostly from WebKitGtkPage. It will be enough to just connect to
> WebKitGtkPage to get everything needed.

I think using signals is a good idea. I just meant that you should consider keeping the names of the signals close to the names of the delegate methods, and the names/responsibilities of the WebKitGtk{Frame,Page} methods similar to their equivalents on Mac/Windows.

> For some methods where you do [[webView _mainFrame] xyz:...] I'm tempted to
> place them in WebKitGtkPage directly and already did so.

Presumably the methods are on WebFrame because they make sense to be called on any frame -- putting them on Page is not equivalent.

> I have integrated WebKitGtkPage into my OpenMoko RSSReader, and there is a
> first version of midori using WebKitGtkPage as well. So the API is to some
> degree already usable and will slowly grow.

That's great. It's good not to develop an API in a bubble.

[Xan Lopez]

Some comments on the implementation:

+    webkit_gtk_frame_signals[CLEARED] = g_signal_new("cleared",
+            G_TYPE_FROM_CLASS(frame_class),
+            (GSignalFlags)(G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION),
+            NULL,
+            NULL,
+            NULL,
+            g_cclosure_marshal_VOID__VOID,
+            G_TYPE_NONE, 0);

The WebKitGtkFrame signals have no class method associated, so not even subclasses will be able to assign one. Is this the desired behavior?

+static void webkit_gtk_page_register_rendering_area_events(GtkWidget* win, WebKitGtkPage* page)
+{
+    gdk_window_set_events(GTK_IS_LAYOUT(win) ? GTK_LAYOUT(win)->bin_window : win->window,
+                          (GdkEventMask)(GDK_EXPOSURE_MASK
+                            | GDK_BUTTON_PRESS_MASK
+                            | GDK_BUTTON_RELEASE_MASK
+                            | GDK_POINTER_MOTION_MASK
+                            | GDK_KEY_PRESS_MASK
+                            | GDK_KEY_RELEASE_MASK
+                            | GDK_BUTTON_MOTION_MASK
+                            | GDK_BUTTON1_MOTION_MASK
+                            | GDK_BUTTON2_MOTION_MASK
+                            | GDK_BUTTON3_MOTION_MASK));

Set the realize handler and do this there. 

+
+    g_signal_connect(GTK_OBJECT(win), "expose-event", G_CALLBACK(webkit_gtk_page_rendering_area_handle_gdk_event), page);
+    g_signal_connect(GTK_OBJECT(win), "key-press-event", G_CALLBACK(webkit_gtk_page_rendering_area_handle_gdk_event), page);
+    g_signal_connect(GTK_OBJECT(win), "key-release-event", G_CALLBACK(webkit_gtk_page_rendering_area_handle_gdk_event), page);
+    g_signal_connect(GTK_OBJECT(win), "button-press-event", G_CALLBACK(webkit_gtk_page_rendering_area_handle_gdk_event), page);
+    g_signal_connect(GTK_OBJECT(win), "button-release-event", G_CALLBACK(webkit_gtk_page_rendering_area_handle_gdk_event), page);
+    g_signal_connect(GTK_OBJECT(win), "motion-notify-event", G_CALLBACK(webkit_gtk_page_rendering_area_handle_gdk_event), page);
+    g_signal_connect(GTK_OBJECT(win), "scroll-event", G_CALLBACK(webkit_gtk_page_rendering_area_handle_gdk_event), page);
+}

Same here. You could set the "event" handler and call the function for all those events. In general connecting to your own signals makes little sense as far as I know.

+    webkit_gtk_page_signals[LOAD_PROGRESS_CHANGED] = g_signal_new("load_progress_changed",
+            G_TYPE_FROM_CLASS(page_class),
+            (GSignalFlags)(G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION),
+            NULL,
+            NULL,
+            NULL,
+            g_cclosure_marshal_VOID__INT,
+            G_TYPE_NONE, 1,
+            G_TYPE_INT);

Same than in frame.

+    g_signal_connect_after(G_OBJECT(page), "realize", G_CALLBACK(webkit_gtk_page_register_rendering_area_events), page);
+    g_signal_connect_after(G_OBJECT(page), "size-allocate", G_CALLBACK(webkit_gtk_page_rendering_area_resize_callback), page);
+

Set the handlers. You can call your parent's method before doing your thing if needed.

Index: WebKit/gtk/Api/webkitgtksettings.h
===================================================================
--- WebKit/gtk/Api/webkitgtksettings.h	(revision 0)
+++ WebKit/gtk/Api/webkitgtksettings.h	(revision 0)

(...)

In general settings are implemented as GObject properties, any reason to not do that here (as properties on other class)?

[Xan Lopez]

> +static void webkit_gtk_page_register_rendering_area_events(GtkWidget* win,
> WebKitGtkPage* page)
> +{
> +    gdk_window_set_events(GTK_IS_LAYOUT(win) ? GTK_LAYOUT(win)->bin_window :
> win->window,
> +                          (GdkEventMask)(GDK_EXPOSURE_MASK
> +                            | GDK_BUTTON_PRESS_MASK
> +                            | GDK_BUTTON_RELEASE_MASK
> +                            | GDK_POINTER_MOTION_MASK
> +                            | GDK_KEY_PRESS_MASK
> +                            | GDK_KEY_RELEASE_MASK
> +                            | GDK_BUTTON_MOTION_MASK
> +                            | GDK_BUTTON1_MOTION_MASK
> +                            | GDK_BUTTON2_MOTION_MASK
> +                            | GDK_BUTTON3_MOTION_MASK));
> 
> Set the realize handler and do this there. 
> 

This was not very clear, you shouldn't do it that way. You could call the parent's implementation and then modify the event mask (get the mask from parent, add yours, set).

[Adam Roben (:aroben)]

Comment on attachment 15644 [details]
Partial implementation of the API

This seems fine to me for now. Let's get this checked in and file bugs for the remaining work. A few things I can think of off-hand:

* Move the rest of the clients to WebKit/gtk and the WebKitGtk namespace
* Get rid of FrameGdk

r=me

[Holger Freyther]

Landed in r24537. Further discussion on the API and Implementation will be done in a different bug.

[Holger Freyther]

Landed in r24537. Further discussion on the API and Implementation will be done in a different bug.