Bug 48512 – [GTK] Implement sample browser app for WebKit2

• Component: WebKit2
• Status: RESOLVED FIXED

[Amruth Raj]

Implement sample MiniBrowser app for WebKit2

[Amruth Raj]

Created attachment 72327 [details]
Implements sample GTK based Browser app on WebKit2

[Ravi Phaneendra Kasibhatla]

Adding myself to the CC list for this bug.

[Ravi Phaneendra Kasibhatla]

Created attachment 72687 [details]
MiniBrowser implementation for WebKit2 GTK port

MiniBrowser implementation for WebKit2 GTK port

[WebKit Review Bot]

Attachment 72687 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/MiniBrowser/gtk/main.cpp:31:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKitTools/MiniBrowser/gtk/BrowserView.cpp:31:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKitTools/MiniBrowser/gtk/BrowserWindow.cpp:31:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKitTools/MiniBrowser/gtk/MiniBrowser.cpp:31:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 4 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 72687 [details]
MiniBrowser implementation for WebKit2 GTK port

View in context: https://bugs.webkit.org/attachment.cgi?id=72687&action=review

> WebKitTools/MiniBrowser/gtk/BrowserView.cpp:38
> +BrowserView::BrowserView()
> +    : m_wkview(0)
> +{
> +}

Shouldn't you implement this using the GTK C API ? For testing the API as well??

[Kenneth Rohde Christiansen]

GTK reviewers?

[Ravi Phaneendra Kasibhatla]

Created attachment 78238 [details]
MiniBrowser implementation for WebKit2 GTK port

[WebKit Review Bot]

Attachment 78238 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Tools/ChangeLog', u'Tools/MiniBrowser/gtk/BrowserView.cpp', u'Tools/MiniBrowser/gtk/BrowserView.h', u'Tools/MiniBrowser/gtk/BrowserWindow.cpp', u'Tools/MiniBrowser/gtk/BrowserWindow.h', u'Tools/MiniBrowser/gtk/GNUmakefile.am', u'Tools/MiniBrowser/gtk/MiniBrowser.cpp', u'Tools/MiniBrowser/gtk/MiniBrowser.h', u'Tools/MiniBrowser/gtk/main.cpp']" exit_code: 1
Tools/MiniBrowser/gtk/MiniBrowser.cpp:31:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Tools/MiniBrowser/gtk/main.cpp:31:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Tools/MiniBrowser/gtk/BrowserView.cpp:31:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Tools/MiniBrowser/gtk/BrowserWindow.cpp:31:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 4 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alejandro G. Castro]

Created attachment 85050 [details]
Updated patch

This is the current patch I use for testing, I think we should do a C application like the GTKLauncher. Use it if you want to test current code.

[Alejandro G. Castro]

Created attachment 86211 [details]
Proposed patch

C application based on the GtkLauncher, it basically loads and goes backward and forward. There are some style false positives because this is an application using the API a webkit internal file.

Added also bug 56668 to check the use of bool in the C API. Added also bug 56680 to make the generated forwarding headers work for c files.

[WebKit Review Bot]

Attachment 86211 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Tools/Cha..." exit_code: 1

Tools/MiniBrowser/gtk/main.c:30:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Tools/MiniBrowser/gtk/main.c:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 86211 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86211&action=review

Looks good!. Just needs a little bit of cleanup.

> Tools/MiniBrowser/gtk/main.c:37
> +static void activateUriEntryCb(GtkWidget *entry, gpointer data)

Why not pass in the WebView as the data parameter?

> Tools/MiniBrowser/gtk/main.c:41
> +    g_assert(uri);

Is it possible for get_text to return NULL here? If so it should be handled. If not, I think we can remove the assertion.

> Tools/MiniBrowser/gtk/main.c:65
> +static GtkWidget *createBrowser(GtkWidget *window, GtkWidget *uriEntry, WKViewRef *webView)
> +{
> +    GtkWidget *webViewWidget = WKViewGetWindow(*webView);
> +
> +    return webViewWidget;
> +}

Just remove this entirely and call  WKViewGetWindow(...) at the callsite.

> Tools/MiniBrowser/gtk/main.c:111
> +    GtkWidget *vbox;
> +    GtkWidget *window;
> +    GtkWidget *uriEntry;
> +    GtkWidget *webViewWidget;

My preference is to declare these where you first use them. We should check to see if this is the way that the Mac C code is written. If so, we should follow the same style.

> Tools/MiniBrowser/gtk/main.c:119
> +    webViewWidget = WKViewGetWindow(*webView);

This seems to be unused?

> Tools/MiniBrowser/gtk/main.c:125
> +    gtk_box_pack_start(GTK_BOX(vbox), createBrowser(window, uriEntry, webView), TRUE, TRUE, 0);

As stated above, simply use WKViewGetWindow above.

> Tools/MiniBrowser/gtk/main.c:149
> +    GtkWidget *main_window;

main_window ==> mainWindow.

> Tools/MiniBrowser/gtk/main.c:157
> +    gchar *uri =(gchar*)(argc > 1 ? argv[1] : "http://www.google.com/");

Missing a space here after =. Should we use "http://webkitgtk.org " as the default page? :)

[Alejandro G. Castro]

(In reply to comment #12)
> (From update of attachment 86211 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86211&action=review
> 
> Looks good!. Just needs a little bit of cleanup.
> 
> > Tools/MiniBrowser/gtk/main.c:37
> > +static void activateUriEntryCb(GtkWidget *entry, gpointer data)
> 
> Why not pass in the WebView as the data parameter?
> 

I tried this one before :), and discarded it because the callback is used for the uri entry and for the stock button, which requires a g_signal_connect_swapped with the uriEntry using the data parameter.

> > Tools/MiniBrowser/gtk/main.c:41
> > +    g_assert(uri);
> 
> Is it possible for get_text to return NULL here? If so it should be handled. If not, I think we can remove the assertion.
> 

Apparently it can't, documentation is not clear but I've checked the code and it returns "" if the buffer is NULL. 

> > Tools/MiniBrowser/gtk/main.c:65
> > +static GtkWidget *createBrowser(GtkWidget *window, GtkWidget *uriEntry, WKViewRef *webView)
> > +{
> > +    GtkWidget *webViewWidget = WKViewGetWindow(*webView);
> > +
> > +    return webViewWidget;
> > +}
> 
> Just remove this entirely and call  WKViewGetWindow(...) at the callsite.
>

Ok, I had left it because in the future we will need it to create multiple windows, but yeah, not sure what we will do and we can add it later.

> > Tools/MiniBrowser/gtk/main.c:111
> > +    GtkWidget *vbox;
> > +    GtkWidget *window;
> > +    GtkWidget *uriEntry;
> > +    GtkWidget *webViewWidget;
> 
> My preference is to declare these where you first use them. We should check to see if this is the way that the Mac C code is written. If so, we should follow the same style.
>

This comes from the old GtkLauncher, initially I thought it was using some style more similar to a gnome application, that is why I left this like that, but yeah, we an keep the webkit style.

> > Tools/MiniBrowser/gtk/main.c:119
> > +    webViewWidget = WKViewGetWindow(*webView);
> 
> This seems to be unused?
> 

Yep, leftover, sorry about that.

> 
> > Tools/MiniBrowser/gtk/main.c:149
> > +    GtkWidget *main_window;
> 
> main_window ==> mainWindow.
> 

Ok.

> > Tools/MiniBrowser/gtk/main.c:157
> > +    gchar *uri =(gchar*)(argc > 1 ? argv[1] : "http://www.google.com/");
> 
> Missing a space here after =. Should we use "http://webkitgtk.org " as the default page? :)

Your are right! :)

Thanks for the comments.

[Alejandro G. Castro]

Created attachment 86627 [details]
Proposed patch

[WebKit Review Bot]

Attachment 86627 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Tools/Cha..." exit_code: 1

Tools/MiniBrowser/gtk/main.c:30:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Tools/MiniBrowser/gtk/main.c:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 86627 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86627&action=review

Sorry. I missed a few things. :/ My only issues here are minor style issues, but I feel like the change to createWindow will need another round.

> Tools/MiniBrowser/gtk/main.c:29
> +// FIXME: replace bool type in the C API
> +// https://bugs.webkit.org/show_bug.cgi?id=56668

I guess we may eventually need to add the stdbool.h to the headers ourselvses. :/

> Tools/MiniBrowser/gtk/main.c:33
> +#include <WebKit2/WebKit2.h>
> +#include <gtk/gtk.h>

You shoud reorder these includes.

> Tools/MiniBrowser/gtk/main.c:44
> +static void destroyCb(GtkWidget *widget, GtkWidget *window)

Please change Cb to Callback everywhere.

> Tools/MiniBrowser/gtk/main.c:49
> +static void goBackCb(GtkWidget *widget, WKViewRef *webView)

Ditto.

> Tools/MiniBrowser/gtk/main.c:54
> +static void goForwardCb(GtkWidget *widget, WKViewRef *webView)

Ditto.

> Tools/MiniBrowser/gtk/main.c:120
> +    GtkWidget *window;
> +    window = gtk_window_new(GTK_WINDOW_TOPLEVEL);
> +    gtk_window_set_default_size(GTK_WINDOW(window), 800, 600);
> +    gtk_widget_set_name(window, "MiniBrowser");
> +
> +    WKContextRef context = WKContextGetSharedProcessContext();
> +    *webView = WKViewCreate(context, 0);
> +
> +    GtkWidget *uriEntry;
> +    uriEntry = gtk_entry_new();
> +
> +    GtkWidget *vbox;
> +    vbox = gtk_vbox_new(FALSE, 0);
> +    gtk_box_pack_start(GTK_BOX(vbox), createToolbar(uriEntry, webView), FALSE, FALSE, 0);
> +    gtk_box_pack_start(GTK_BOX(vbox), WKViewGetWindow(*webView), TRUE, TRUE, 0);
> +
> +    gtk_container_add(GTK_CONTAINER(window), vbox);
> +
> +    g_signal_connect(window, "destroy", G_CALLBACK(destroyCb), NULL);
> +
> +    return window;

Oh, sorry. I guess I wasn't totally clear. You should do:

GtkWindow* window = gtk_window_new(GTK_WINDOW_TOPLEVEL); 

and the same with context, uriEntry, vbox, etc.

> Tools/MiniBrowser/gtk/main.c:144
> +    WKViewRef webView;
> +    GtkWidget *mainWindow;
> +    mainWindow = createWindow(&webView);
> +

I think I just realized what's confusing me about this bit. Here's my suggested change:

1. Create the WKWebView externally with a helper: createWebView.
2. Pass the WebView widget to createWindow.

In the end it will look like this:

WkViewRef webView = createWebView();
GtkWidget *mainWindow = createWindow(&webView);

[Martin Robinson]

(In reply to comment #16)
> Oh, sorry. I guess I wasn't totally clear. You should do:
> GtkWindow* window = gtk_window_new(GTK_WINDOW_TOPLEVEL); 

Here I should have said:

GtkWindow *window = gtk_window_new(GTK_WINDOW_TOPLEVEL);

[Alejandro G. Castro]

(In reply to comment #16)
> (From update of attachment 86627 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86627&action=review
> 
> Sorry. I missed a few things. :/ My only issues here are minor style issues, but I feel like the change to createWindow will need another round.
> 
> > Tools/MiniBrowser/gtk/main.c:29
> > +// FIXME: replace bool type in the C API
> > +// https://bugs.webkit.org/show_bug.cgi?id=56668
> 
> I guess we may eventually need to add the stdbool.h to the headers ourselvses. :/
> 

Yep, I'll add it to the  WKBaseGtk.h.

> > Tools/MiniBrowser/gtk/main.c:33
> > +#include <WebKit2/WebKit2.h>
> > +#include <gtk/gtk.h>
> 
> You shoud reorder these includes.
> 

I think it assumes gtk is external and will complain if we reorder, like it does for the stdbool.h, but in that case we need it before the webkit2.h.

> > Tools/MiniBrowser/gtk/main.c:144
> > +    WKViewRef webView;
> > +    GtkWidget *mainWindow;
> > +    mainWindow = createWindow(&webView);
> > +
> 
> I think I just realized what's confusing me about this bit. Here's my suggested change:
> 
> 1. Create the WKWebView externally with a helper: createWebView.
> 2. Pass the WebView widget to createWindow.
> 
> In the end it will look like this:
> 
> WkViewRef webView = createWebView();
> GtkWidget *mainWindow = createWindow(&webView);

I understand, I like it.

[Alejandro G. Castro]

Created attachment 86961 [details]
Proposed patch

[WebKit Review Bot]

Attachment 86961 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Source/We..." exit_code: 1

Tools/MiniBrowser/gtk/main.c:28:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 86961 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86961&action=review

Great! Please change all WKViewRef* to just be WKViewRef since WKViewRef is already a pointer type.

> Tools/MiniBrowser/gtk/GNUmakefile.am:2
> +#MiniBrowser

I think you can omit this.

> Tools/MiniBrowser/gtk/main.c:33
> +    WKViewRef *webView = g_object_get_data(G_OBJECT(entry), "web-view");

data should now just be a WKViewRef. See below.

> Tools/MiniBrowser/gtk/main.c:53
> +static GtkWidget *createToolbar(GtkWidget *uriEntry, WKViewRef *webView)

Here just use WKViewRef  instead of WKViewRef*.

> Tools/MiniBrowser/gtk/main.c:64
> +    /* the back button */

Please either make these comments into full sentences or just remove them. You can probalby just remove them.

> Tools/MiniBrowser/gtk/main.c:69
> +    /* The forward button */

Ditto.

> Tools/MiniBrowser/gtk/main.c:74
> +    /* The URL entry */

Ditto.

> Tools/MiniBrowser/gtk/main.c:81
> +    /* The go button */

Ditto.

> Tools/MiniBrowser/gtk/main.c:94
> +    WKContextRef context = WKContextGetSharedProcessContext();
> +
> +    return WKViewCreate(context, 0);

Please just change this to: WKViewCreate(WKContextGetSharedProcessContext(), 0);

> Tools/MiniBrowser/gtk/main.c:97
> +static GtkWidget *createWindow(WKViewRef* webView)

Here just use WKViewRef instead of WKViewRef*. It's already a pointer.

> Tools/MiniBrowser/gtk/main.c:135
> +    GtkWidget *mainWindow = createWindow(&webView);

Here you should just pass the WKViewRef itself, since it's already a pointer.

> Tools/MiniBrowser/gtk/main.c:137
> +    gchar *uri = (gchar*)(argc > 1 ? argv[1] : "http://www.webkitgtk.org/");

Is this cast necessary?

[Carlos Garcia Campos]

Comment on attachment 86961 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86961&action=review

> Tools/MiniBrowser/gtk/main.c:66
> +    g_signal_connect(G_OBJECT(item), "clicked", G_CALLBACK(goBackCallback), webView);

g_signal_connect macros receive a gpointer, so you can avoid casts and simply use g_signal_connect(item, ...);

> Tools/MiniBrowser/gtk/main.c:138
> +    gchar *fileURL = filenameToURL(uri);

I think we could/should use g_file_new_for_commandline_arg() when argc > 1 and then simply g_file_get_uri() instead of filenameToURL. Do we really need to check whether the file exists when it's a local path?

[Alejandro G. Castro]

Thanks very much for the comments, adding them and committing.

[Alejandro G. Castro]

(In reply to comment #22)
> > Tools/MiniBrowser/gtk/main.c:138
> > +    gchar *fileURL = filenameToURL(uri);
> 
> I think we could/should use g_file_new_for_commandline_arg() when argc > 1 and then simply g_file_get_uri() instead of filenameToURL. Do we really need to check whether the file exists when it's a local path?

Just for the record, I'm not adding this change because it chages the behavior of what the gtklauncher is already doing, it is a fair request, we can discuss if it is what we want in other bug. Thanks for the comment.

[Alejandro G. Castro]

(In reply to comment #24)
> (In reply to comment #22)
> > > Tools/MiniBrowser/gtk/main.c:138
> > > +    gchar *fileURL = filenameToURL(uri);
> > 
> > I think we could/should use g_file_new_for_commandline_arg() when argc > 1 and then simply g_file_get_uri() instead of filenameToURL. Do we really need to check whether the file exists when it's a local path?
> 
> Just for the record, I'm not adding this change because it chages the behavior of what the gtklauncher is already doing, it is a fair request, we can discuss if it is what we want in other bug. Thanks for the comment.

After checking in more detail the implementation I realized the code is not doing exactly what I was thinking in the laucher, so I'm proposing a solution with carlos suggestion.

[Alejandro G. Castro]

Created attachment 87511 [details]
Proposed patch

[WebKit Review Bot]

Attachment 87511 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Source/We..." exit_code: 1

Tools/MiniBrowser/gtk/main.c:28:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 87511 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87511&action=review

> Tools/MiniBrowser/gtk/main.c:133
> +    const gchar *uri = argc > 1 ? argv[1] : "http://www.webkitgtk.org/";
> +    gchar *fileURL = filenameToURL(uri);
> +
> +    WKPageLoadURL(WKViewGetPage(webView), WKURLCreateWithURL(fileURL));
> +    g_free(fileURL);

Please change this to be (including changing the name of filenameToURL to argumentToURL):

gchar* url = argumentToURL(argc > 1 ? argv[1] : "http://www.webkitgtk.org/");
WKPageLoadURL(WKViewGetPage(webView), WKURLCreateWithURL(url));
g_free(url);

We do not necessarily know if the argument is a path or a URL or whether the result is a file or web URL.

[Alejandro G. Castro]

Thanks for the comments, landed: http://trac.webkit.org/changeset/82570