[webkit-gtk] Implement runOpenPanel in WebKit2GTK+
Gustavo Noronha Silva
gns at gnome.org
Mon Feb 13 05:12:26 PST 2012
+1 on Carlos' API suggestions
On Sat, 2012-02-11 at 19:20 +0100, Carlos Garcia Campos wrote:
> El sáb, 11-02-2012 a las 00:10 +0100, Mario Sanchez Prada escribió:
> > Hi all,
> >
> > I started yesterday working on an implementation for WebKitUIClient's
> > runOpenPanel() in WebKit2GTK+, and would like to share here the ideas
> > behind it and the current status of it (aka the patch I have so far).
> >
> > What I did so far is basically the following:
> >
> > - Implement runOpenPanel() in WebKitUIClient.cpp, where I create
> > an object of a new class wrapping WKOpenPanelResultListenerRef and
> > WKOpenPanelParametersRef, which I named WebKitOpenPanelDecision atm.
> >
> > - The API for that new WebKitOpenPanelDecision is simple: it provides
> > a
> > allows_multiple_files(), choose_files() and cancel() methods,
> > implemented in terms of the wrapped WKOpenPanelResultListenerRef and
> > WKOpenPanelParametersRef attributes.
>
> Decision is not probably the best name, even more if the object wraps
> both the listener and the parameters. I think something like Operation
> or Request could sound better. But I like the idea of using a single
> object to wrap both.
>
> You are ignoring the accepted MIME types, you could add a method that
> returns a GtkFileFilter with the mime types. Also the selected files,
> even though there isn't C API for that, they are in
> WebOpenPanelParameters object, so you can get them with
> toImpl(wkParameters)->selectedFileNames().
>
> > - Once that object is created, pass it to the webView to emit a new
> > signal from there, including that new object as parameter. At the
> > moment, I named that new signal 'decide-open-panel', inspired in the
> > 'decide-policy' signal. It uses g_signal_accumulator_true_handled.
>
> In this case it's not a decision, but a request to open the file chooser
> to select files to upload. It's similar to WebKitWebView::script-* or
> WebKitWebView::create signals. And "open-panel" sounds too mac,
> run-filechooser sounds more gnome.
>
> > - Provide a default handler in the webview for this new signal so, if
> > not intercepted from the app, a GtkFileChooserDialog will be run and
> > a handler for that dialog's 'response' signal will be installed, to
> > asynchronously handle the result after selecting files/cancelling:
> > it
> > will call webkit_open_panel_decision_cancel() if the dialog was
> > cancelled it and webkit_open_panel_decision_choose_files()
> > otherwise.
> >
> > I'm not quite sure about some parts of this at the moment (e.g. the
> > names for signals and the new class), but I think this is a good point
> > to start discussing about it, and see if we can agree on something
> > before keeping working along this line.
>
> For the signal I would use something like run-filechooser, and for the
> helper object, WebKitFileChooserOperation or WebKitFileChooserRequest.
>
> > So, I'm attaching the patch I've at the moment for reference and in
> > order to help with the discussion. Please notice it's not a final
> > patch,
> > but more kind of a draft :-)
>
> Ok, even though it's not a final patch I'll add some initial comments
> that I hope will help you to write the final patch.
>
> > Mario
> >
> >
>
> > +#include "config.h"
> > +#include "WebKitOpenPanelDecision.h"
> > +
> > +#include "APIObject.h"
> > +#include "FileSystem.h"
> > +#include "ImmutableArray.h"
> > +#include "WKOpenPanelResultListener.h"
> > +#include "WKOpenPanelParameters.h"
>
> For C API headers, use #include <WebKit2/Foo.h> and move them to
> WebKitPrivate.h
>
> > +#include "WebKitOpenPanelDecisionPrivate.h"
> > +#include "WebKitPrivate.h"
> > +
> > +using namespace WebKit;
> > +
> > +/**
> > + * SECTION: WebKitOpenPanelDecision
> > + * @Short_description: An open panel decision
> > + * @Title: WebKitOpenPanelDecision
> > + * @See_also: #WebKitWebView
> > + */
> > +G_DEFINE_TYPE(WebKitOpenPanelDecision, webkit_open_panel_decision, G_TYPE_OBJECT)
> > +
> > +struct _WebKitOpenPanelDecisionPrivate {
> > + WKRetainPtr<WKOpenPanelParametersRef> parameters;
> > + WKRetainPtr<WKOpenPanelResultListenerRef> listener;
>
> Use wk prefix for variable names that are C API objects, wkParameters
> and wkListener in this case.
>
>
> > +static void webkitOpenPanelDecisionFinalize(GObject* object)
> > +{
> > + WebKitOpenPanelDecisionPrivate* priv = WEBKIT_OPEN_PANEL_DECISION(object)->priv;
> > + priv->~WebKitOpenPanelDecisionPrivate();
> > + G_OBJECT_CLASS(webkit_open_panel_decision_parent_class)->finalize(object);
> > +}
>
> If choose_files or cancel is not called when the object finalizes, we
> could call cancel() to make sure the operation finishes. We do something
> similar with the policy decision. It can happen if the user handles the
> signal and just returns TRUE, to avoid any filechooser to be shown.
>
> > +gboolean webkit_open_panel_decision_allows_multiple_files(WebKitOpenPanelDecision* decision)
> > +{
>
> Use g_return macros in public methods.
>
> > + return WKOpenPanelParametersGetAllowsMultipleFiles(decision->priv->parameters.get());
> > +}
> > +
> > +void webkit_open_panel_decision_choose_files(WebKitOpenPanelDecision* decision, GSList* fileUris)
> > +{
>
> Ditto.
>
> > + Vector<RefPtr<APIObject> > selectedUris;
> > + int i = 0;
> > + for (GSList* item = fileUris ; item ; item = item->next) {
> > + if (!item->data)
> > + continue;
> > + selectedUris.append(WebURL::create(WebCore::filenameToString(static_cast<char*>(item->data))));
>
> List items must be uris, not filenames.
>
> > + }
> > +
> > + WKOpenPanelResultListenerChooseFiles(decision->priv->listener.get(), toAPI(ImmutableArray::adopt(selectedUris).get()));
>
> This can be implemented using the C API:
>
> WKRetainPtr<WKMutableArrayRef> wkSelectedURIs(AdoptWK, WKMutableArrayCreate());
>
> for (GSList* item = fileURIs ; item ; item = g_slist_next(item)) {
> if (!item->data)
> continue;
> WKRetainPtr<WKURLRef> wkURL(AdoptWK, WKURLCreateWithUTF8CString(static_cast<char*>(item->data)));
> WKArrayAppendItem(wkSelectedURIs.get(), wkURL.get());
> }
>
> WKOpenPanelResultListenerChooseFiles(decision->priv->listener.get(), wkSelectedURIs.get());
>
> It looks a bit easier to read to me, but your code is fine too.
>
> > +}
> > +
> > +void webkit_open_panel_decision_cancel (WebKitOpenPanelDecision* decision)
> > +{
>
> g_return.
>
> > + WKOpenPanelResultListenerCancel(decision->priv->listener.get());
> > +}
>
> > diff --git a/Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp b/Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp
> > index a659ad3..a051eaa 100644
> > --- a/Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp
> > +++ b/Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp
> > @@ -20,12 +20,17 @@
> > #include "config.h"
> > #include "WebKitUIClient.h"
> >
> > +#include "WKOpenPanelParameters.h"
>
> This should already be included by WebKitPrivate.h.
>
> > +#include "WebKitOpenPanelDecision.h"
> > +#include "WebKitOpenPanelDecisionPrivate.h"
> > #include "WebKitPrivate.h"
> > #include "WebKitWebViewBasePrivate.h"
> > #include "WebKitWebViewPrivate.h"
> > #include "WebKitWindowPropertiesPrivate.h"
> > #include "WebPageProxy.h"
> > #include <WebCore/GtkUtilities.h>
> > +#include <glib/gi18n-lib.h>
> > +#include <wtf/gobject/GRefPtr.h>
> >
> > using namespace WebKit;
> >
> > @@ -126,6 +131,12 @@ static void setWindowFrame(WKPageRef page, WKRect frame, const void* clientInfo)
> > webkitWindowPropertiesSetGeometry(windowProperties, &geometry);
> > }
> >
> > +static void runOpenPanel(WKPageRef page, WKFrameRef frame, WKOpenPanelParametersRef parameters, WKOpenPanelResultListenerRef listener, const void *clientInfo)
> > +{
> > + GRefPtr<WebKitOpenPanelDecision> decision = adoptGRef(webkitOpenPanelDecisionCreate(parameters, listener));
> > + webkitWebViewMakeOpenPanelDecision(WEBKIT_WEB_VIEW(clientInfo), WEBKIT_OPEN_PANEL_DECISION(decision.get()));
>
> No need to cast, decision is already a WebKitOpenPanelDecision.
>
> > +}
> > +
>
> > diff --git a/Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp b/Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp
> > index c3acccc..21a7a16 100644
> > --- a/Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp
> > +++ b/Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp
> > @@ -21,6 +21,8 @@
> > #include "config.h"
> > #include "WebKitWebView.h"
> >
> > +#include "FileSystem.h"
> > +#include "GOwnPtr.h"
>
> Use the angle-bracket form for header files from other WebCore and
> JavaScriptCore, see
> http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
>
> >
> > +static void fileChooserDialogResponseCallback(GtkDialog* dialog, gint responseID, WebKitOpenPanelDecision* decision)
> > +{
>
> You can adopt the ref here and use an early return.
>
> GRefPtr<WebKitOpenPanelDecision> decision = adoptGRef(decision);
> if (responseID != GTK_RESPONSE_ACCEPT) {
> webkit_open_panel_decision_cancel(decision);
> gtk_widget_destroy(GTK_WIDGET(dialog));
> return;
> }
>
> > + if (responseID == GTK_RESPONSE_ACCEPT) {
> > + GSList* fileUris = NULL;
> > + if (gtk_file_chooser_get_select_multiple(GTK_FILE_CHOOSER(dialog))) {
> > + fileUris = gtk_file_chooser_get_uris(GTK_FILE_CHOOSER(dialog));
> > + } else {
> > + gchar* fileUri = gtk_file_chooser_get_uri(GTK_FILE_CHOOSER(dialog));
> > + if (!fileUri)
> > + fileUris = g_slist_append(fileUris, fileUri);
> > + }
>
> I think gtk_file_chooser_get_uris works also if select multiple is not
> allowed, have you tried it?
>
> > + webkit_open_panel_decision_choose_files(decision, fileUris);
> > + g_slist_foreach(fileUris, reinterpret_cast<GFunc>(g_free), 0);
> > + g_slist_free(fileUris);
>
> g_list_free_full() does both, but you should use GOwnPtr anyway.
>
> > +
> > + } else
> > + webkit_open_panel_decision_cancel(decision);
> > +
> > + g_object_unref(decision);
> > +
> > + gtk_widget_destroy(GTK_WIDGET(dialog));
> > +}
> > +
> > +static gboolean webkitWebViewDecideOpenPanel(WebKitWebView* webView, WebKitOpenPanelDecision* decision)
> > +{
> > + GtkWidget* toplevel = gtk_widget_get_toplevel(GTK_WIDGET(webView));
> > + if (!WebCore::widgetIsOnscreenToplevelWindow(toplevel))
> > + toplevel = 0;
>
> WebKitWebView has 'using namespace WebCore;' so no need for WebCore::
>
> > + GtkWidget* dialog = gtk_file_chooser_dialog_new(_("Upload File"),
>
> I would use something lile Select Files or Select Files to upload, maybe
> using File or Files depending on the allow_multiple parameter. Upload
> File makes me think the dialog will upload a file, while it actually
> select a file that might be uploaded later.
>
> > + toplevel ? GTK_WINDOW(toplevel) : 0,
> > + GTK_FILE_CHOOSER_ACTION_OPEN,
> > + GTK_STOCK_CANCEL, GTK_RESPONSE_CANCEL,
> > + GTK_STOCK_OPEN, GTK_RESPONSE_ACCEPT,
> > + NULL);
> > +
> > + gtk_file_chooser_set_select_multiple(GTK_FILE_CHOOSER(dialog), webkit_open_panel_decision_allows_multiple_files(decision));
> > + g_signal_connect(G_OBJECT(dialog), "response", G_CALLBACK(fileChooserDialogResponseCallback), g_object_ref(decision));
>
> g_signal_connect receives a gpointer, you don't need the G_OBJECT cast.
>
> > + gtk_widget_show(dialog);
> > +
> > + return TRUE;
> > +}
> > +
>
> Don't forget to include the new header in the main header webkit2.h
>
> _______________________________________________
> webkit-gtk mailing list
> webkit-gtk at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-gtk
--
Gustavo Noronha Silva <gns at gnome.org>
GNOME Project
More information about the webkit-gtk
mailing list