[Webkit-unassigned] [Bug 48512] [GTK] Implement sample browser app for WebKit2

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=48512


Alejandro G. Castro <alex at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |alex at igalia.com




--- Comment #13 from Alejandro G. Castro <alex at igalia.com>  2011-03-23 09:08:30 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.