Bug 48512

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

Adding myself to the CC list for this bug.

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

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 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??

GTK reviewers?

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

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.

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.

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.

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 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? :)

(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.

Created attachment 86627 [details] Proposed patch

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 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);

(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);

(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.

Created attachment 86961 [details] Proposed patch

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 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 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?

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

(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.

(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.

Created attachment 87511 [details] Proposed patch

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 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.

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