Bug 48512

Summary: [GTK] Implement sample browser app for WebKit2
Product: WebKit Reporter: Amruth Raj <amruthraj>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, andersca, cgarcia, gustavo, kbalazs, kenneth, mrobinson, ravi.kasibhatla, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 52805    
Attachments:
Description Flags
Implements sample GTK based Browser app on WebKit2
none
MiniBrowser implementation for WebKit2 GTK port
none
MiniBrowser implementation for WebKit2 GTK port
none
Updated patch
none
Proposed patch
mrobinson: review-
Proposed patch
mrobinson: review-
Proposed patch
mrobinson: review+
Proposed patch mrobinson: review+

Description Amruth Raj 2010-10-28 04:56:10 PDT
Implement sample MiniBrowser app for WebKit2
Comment 1 Amruth Raj 2010-10-29 06:29:50 PDT
Created attachment 72327 [details]
Implements sample GTK based Browser app on WebKit2
Comment 2 Ravi Phaneendra Kasibhatla 2010-10-29 07:15:55 PDT
Adding myself to the CC list for this bug.
Comment 3 Ravi Phaneendra Kasibhatla 2010-11-02 09:47:15 PDT
Created attachment 72687 [details]
MiniBrowser implementation for WebKit2 GTK port

MiniBrowser implementation for WebKit2 GTK port
Comment 4 WebKit Review Bot 2010-11-02 09:54:27 PDT
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.
Comment 5 Kenneth Rohde Christiansen 2010-11-16 08:55:59 PST
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??
Comment 6 Kenneth Rohde Christiansen 2010-12-24 02:23:43 PST
GTK reviewers?
Comment 7 Ravi Phaneendra Kasibhatla 2011-01-07 07:24:22 PST
Created attachment 78238 [details]
MiniBrowser implementation for WebKit2 GTK port
Comment 8 WebKit Review Bot 2011-01-07 13:41:21 PST
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.
Comment 9 Alejandro G. Castro 2011-03-08 08:26:02 PST
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.
Comment 10 Alejandro G. Castro 2011-03-18 14:19:37 PDT
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.
Comment 11 WebKit Review Bot 2011-03-18 14:20:56 PDT
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.
Comment 12 Martin Robinson 2011-03-18 15:27:14 PDT
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? :)
Comment 13 Alejandro G. Castro 2011-03-23 09:08:30 PDT
(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.
Comment 14 Alejandro G. Castro 2011-03-23 09:31:21 PDT
Created attachment 86627 [details]
Proposed patch
Comment 15 WebKit Review Bot 2011-03-23 09:34:17 PDT
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.
Comment 16 Martin Robinson 2011-03-23 09:57:27 PDT
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);
Comment 17 Martin Robinson 2011-03-23 09:58:28 PDT
(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);
Comment 18 Alejandro G. Castro 2011-03-25 08:31:40 PDT
(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.
Comment 19 Alejandro G. Castro 2011-03-25 11:19:10 PDT
Created attachment 86961 [details]
Proposed patch
Comment 20 WebKit Review Bot 2011-03-25 11:20:40 PDT
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.
Comment 21 Martin Robinson 2011-03-25 11:29:45 PDT
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?
Comment 22 Carlos Garcia Campos 2011-03-28 03:54:07 PDT
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?
Comment 23 Alejandro G. Castro 2011-03-30 05:46:55 PDT
Thanks very much for the comments, adding them and committing.
Comment 24 Alejandro G. Castro 2011-03-30 05:48:24 PDT
(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.
Comment 25 Alejandro G. Castro 2011-03-30 05:58:39 PDT
(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.
Comment 26 Alejandro G. Castro 2011-03-30 06:04:39 PDT
Created attachment 87511 [details]
Proposed patch
Comment 27 WebKit Review Bot 2011-03-30 06:06:33 PDT
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.
Comment 28 Martin Robinson 2011-03-30 15:28:06 PDT
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.
Comment 29 Alejandro G. Castro 2011-03-31 05:54:26 PDT
Thanks for the comments, landed: http://trac.webkit.org/changeset/82570