Bug 87283 – [GTK] run-file-chooser signal for WebKit1

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Daniel Drake]

Bug #78491 adds a run-file-chooser interface for WebKit2. OLPC/Sugar will move to WebKit2 shortly, but a temporary solution is needed for WebKit1.

I have backported the basics to WebKit1 and tested, Mario Sanchez Prada has added some missing bits. The patch (attached) just needs unit tests.

Adding unit tests looks quite time consuming, and this is my first time in the webkit code. I'll try to find some time to work on it soon, but help/direction would be much appreciated.

[Daniel Drake]

Created attachment 143594 [details]
Work in progress

A combination of my initial backport and Mario's additions.

[Mario Sanchez Prada]

(In reply to comment #0)
> [...]
> Adding unit tests looks quite time consuming, and this is my first time
> in the webkit code. I'll try to find some time to work on it soon, but 
> help/direction would be much appreciated.

You basically need to implement a new unit test in Source/WebKit/gtk/tests/testwebview.c, so you actually check that the run-file-chooser gets emitted when clicking on the button and that the new API work as expected in different cases.

For a baseline to start writing a new unit test in that file, I think test_webkit_web_view_fullscreen() could be a good thing to check, as it already connects to some signals in from the WebView and check stuff there.

To get ideas on how to actually implement this specific unit test, which HTML code you could use for testing this new API and so on, you can check testWebViewFileChooserRequest() inside Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp. Implementation of unit tests is slightly different there (as we use a C++ wrapper on top of GTest, instead of using plain C), but the underlying idea is basically the same: connect to the run-file-chooser signal and check different things depending on the HTML used.

Hope this helps, and thanks for your effort on this front.

[Daniel Drake]

Created attachment 143909 [details]
new patch

Here's the latest patch with unit tests ported from WebKit2.
One test fails :(

Mario, would you be able to spend a few mins looking at this?

[Daniel Drake]

Created attachment 143953 [details]
Attempting to solve test failure

Tricky, I think.
webkit_file_chooser_request_get_selected_files() calls into request->priv->chooser->settings().selectedFiles. However, this doesn't seem to be updated when a new selection is made.

So I moved to caching the selected files in webkit_file_chooser_request_select_files() in the attached incremental patch. This fixes the test failure but then fails on a later test, where we select some files then click the button again and check that the selection was retained. This creates a new ChooserRequest object which has forgotten the selection of the first one.

I then tried caching the initially selected files by reading the settings from webkit_file_chooser_request_create() but this segfaults upon access to chooser->settings(), not sure why.

[Mario Sanchez Prada]

(In reply to comment #4)
> Created an attachment (id=143953) [details]
> Attempting to solve test failure
> 
> Tricky, I think.

Yes, it's indeed tricky :-)

> webkit_file_chooser_request_get_selected_files() calls into request->priv-
> chooser->settings().selectedFiles. However, this doesn't seem to be updated 
> when a new selection is made.

This is the expected behavior, although I agree with you that it's confusing to say the least, as I had the same doubts when working on this.

The reason why this happens is that the list of selected files in the filechooser is generated when the user initiates the logic, by pressing in a 'upload' button, and won't change during the whole lifecycle of the 'file chooser request', I think.

To make it clear, this is a list of the steps that happen in a typical workflow like this:

  1. User clicks on a 'upload' <input type='file'> button
  2. The runOpenPanel stuff is triggered from WebCore, resulting on calling to wkgtk's implementation of ChromeClient::runOpenPanel
  3. The signal 'run-file-chooser' signal is emitted from the WebView, with an instance of WebKitFileChooserRequest with the details of the event (selected files that might be already present _from previous times the user opened the dialog_, mime type filters to be applied)
  4. The application handles the signal, showing a dialog to the user so (s)he can select local files. The application might take into account, when creating that dialog the following info:
         - selected files in the request (they could pre-select them in the dialog)
         - accepted mime types (they could decide to filter other mime types out)
         - allows multiple selection (so that more than one file could be selected)
  5. The user makes a choice and closes the dialog, which would result on a call to webkit_file_chooser_request_choose_files(), or just cancels it (webkit_file_chooser_request_cancel()).
  6. The whole process finishes at this point, and nobody should care anymore about that WebKitFileChooserRequest object, that should get disposed from this point on.

So, the thing is that if you open the file chooser request for the first time, selectedFiles in WebCore will be an empty list (since it's the first time) and it will keep being an empty list even after selecting some files (let's say "A" and "B") in the dialog, since it's associated to the current request the user is dealing with.

Then, if you open the dialog for the second time, selectedFiles will be filled now with the list of files previously selected ("A" and "B"), so the application will see them with the request in case it wants to do something with them (like pre-selecting them in the dialog). If now the user selects file "C" selectedFiles will internally keep containing "A" and "B", as won't be different until the next request, if that ever happens.

So this is precisely one of the reasons why we added the caching of selectedFiles at the level of the API (I mean, inside of WebKitFileChooserRequest, not in WebCore::FileChooser), so we could update the list of selected files at that level for the hypothetical case of the application calling to _get_selected_files() right after _select_files(), so we would return the list of files _currently_ selected even before disposing the current request, which would still be the old list in WebCore at that point.

Not sure if I managed to explain it properly. It's messy, I know... so feel free to ask again if you want some further clarification.

> So I moved to caching the selected files in 
> webkit_file_chooser_request_select_files() in the attached incremental
> patch. This fixes the test failure but then fails on a later test,
> where we select some files then click the button again and check that
> the selection was retained. This creates a new ChooserRequest object
> which has forgotten the selection of the first one.

It's ok to cache in the _select_files() method (so we can return the new list if the app calls _get_selected() right after making a new selection _and_ before disposing the current WebKitFileChooserRequest  object), but you should keep the cache in _get_selected_files() too, since you typically call it before opening the file chooser, so you know if you need to preselect some files in case there was a previous selection made.

> I then tried caching the initially selected files by reading the settings from
> webkit_file_chooser_request_create() but this segfaults upon access to
> chooser->settings(), not sure why.

I'm not sure either why this fails, nor why the previous tests you did fail too, but I'll take a look if possible today to it.

Btw, sorry for the late reply, but yesterday and today I'm attending to long meetings in the afternoon, so that's why I can't check this as promptly as I would like to.

[Daniel Drake]

Created attachment 144053 [details]
run-file-chooser ready for review

Thanks for the clear explanation, now I understand it better. The test case failure can be fixed by moving to the same logic used in WebKit2, which is as you describe. Here's an updated patch with all test cases working. Whats next?

[Mario Sanchez Prada]

(In reply to comment #6)
> Created an attachment (id=144053) [details]
> run-file-chooser ready for review
> 
> Thanks for the clear explanation, now I understand it better.

Glad to hear that.

> The test case failure can be fixed by moving to the same logic used in WebKit2, 
> which is as you describe. Here's an updated patch with all test cases working.

Thanks! Good to see you figured out the problem.

> Whats next?

I haven't time to checked the patch (last meeting this afternoon) and have to run now out of the office, but off the top of my head the next step would be to ask a  WebKit reviewer[1] (which I'm not yet) to review it by setting the r? flag in the "Details" link for the patch [2].

Still, I could make an informally review but I'm afraid that will have to wait until Monday, since I'm now already in a hurry. Hope it's fine

[1] http://www.webkit.org/coding/commit-review-policy.html
[2] http://www.webkit.org/coding/contributing.html

[Mario Sanchez Prada]

I will do an informal review myself now, but as I'm not a reviewer I'm adding Carlos to CC so he is aware of this bug and can make an official review later.

[Mario Sanchez Prada]

Comment on attachment 144053 [details]
run-file-chooser ready for review

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

Informally reviewing the patch, which overall looks good to me. I've mostly commented on he need of keeping the webkit_file_chooser_request_cancel() function in the API and some coding style issues. Hope this helps.

See my comments below...

> WebKit-r118336/Source/WebKit/gtk/webkit/webkitfilechooserrequest.cpp:365
> +     */

I'm not sure whether it's useful to keep this webkit_file_chooser_request_cancel() function in WebKit1 if we don't need it after all. For WebKit2 is needed in order to tell the Web process that the request have been cancelled so it does not keep waiting forever, but that's not an issue in WK1 (and the reason, I think, why is not needed).

So, I think I would remove this function from the API in WK1, even if that's not consistent with WK2 API. After all, both APIs are similar but not identical anyway.

> WebKit-r118336/Source/WebKit/gtk/webkit/webkitfilechooserrequest.h:70
> +webkit_file_chooser_request_cancel                (WebKitFileChooserRequest *request);

Same here. I would remove this one, although I'd appreciate others' opinion on this topic, too.

Carlos, what do you think?

> WebKit-r118336/Source/WebKit/gtk/webkit/webkitwebview.cpp:1315
> +        webkit_file_chooser_request_cancel(adoptedRequest.get());

Make sure to remove this else branch if we finally agree on removing webkit_file_chooser_request_cancel() from the API

> WebKit-r118336/Source/WebKit/gtk/webkit/webkitwebview.cpp:1338
> +    gtk_file_chooser_set_select_multiple(GTK_FILE_CHOOSER(dialog), allowsMultipleSelection);

This line seems to be duplicated.

Also, I think this is a good place to ask the WebKitFileChooserRequest object whether it already has a non-empty list of files selected that we can use to pre-select them in the file chooser dialog when opening (actually we just can pre-select one file). This is what we do in WK2:

    if (const gchar* const* selectedFiles = webkit_file_chooser_request_get_selected_files(request))
        gtk_file_chooser_select_filename(GTK_FILE_CHOOSER(dialog), selectedFiles[0]);

Hence, I would do the same here.

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:509
> +static gboolean checkMimeTypeForFilter(GtkFileFilter* filter, const gchar* mimeType)

Coding style issue: in the diff for this file you seem to use sometimes thisKindOfNamingStyle for functions, local variables and parameters, and some times this_other_naming_style, and that is wrong as you should be consistent.

In theory the former (camel case) is the commonly agreed coding style for newly introduced code in WK *but*, as (1) unit tests are an exception in some ways to coding style, as (2) 99% of this file seems to be already written using this_other_naming_style and as (3) we don't expect much more changes in this file from now on... I'd say that these are good reasons to stick to this_other_naming_style here.

Which is a long way of saying that you should be consistent with the code introduced in this patch and that you should rename this function name and parameters to something like this:

   static gboolean check_mime_type_for_filter(GtkFileFilter* filter, const gchar* mime_type)

Others might disagree (and I would also if we were thinking of adding many more changes in the future to this file), but I think that it's pretty reasonable to do it this time according to reasons (1), (2) and (3).

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:511
> +    GtkFileFilterInfo filterInfo;

Don't use camel case style here: filterInfo -> filter_info

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:520
> +    const gchar* const* selectedFiles;

Don't use camel case style here

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:521
> +    GtkFileFilter *filter;

Coding style issue: Misplaced '*'. It should be 'GtkFileFilter* filter;'

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:531
> +    webkit_file_chooser_request_cancel(request);

Same than before: remove this line if we agree on removing the _cancel() function from the API

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:540
> +    const gchar* const* selectedFiles;

Don't use camel case style here

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:541
> +    GtkFileFilter *filter;

Misplaced '*'.

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:570
> +    const gchar* const* selectedFiles;

Don't use camel case style here

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:578
> +    webkit_file_chooser_request_cancel(request);

Remove this line if agreed

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:588
> +    GtkFileFilter *filter;

Misplaced '*'.

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:607
> +    webkit_file_chooser_request_cancel(request);

Put my "usual comment" here :)

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:613
> +void doMouseButtonEvent(GtkWidget *widget, GdkEventType event_type, int x, int y, unsigned int button, unsigned int modifiers)

Don't use camel case style here: doMouseButtonEvent -> do_mouse_button_event

Also, misplaced '*' for widget.

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:619
> +    GdkEvent *event = gdk_event_new(event_type);

Misplaced '*'.

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:638
> +static void clickMouseButton(GtkWidget *widget, int x, int y, unsigned int button, unsigned int modifiers)

Don't use camel case style here. Misplaced '*' for widget

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:644
> +static gboolean clickMouseButtonAndWaitForFileChooserRequest(WebKitWebView* webView)

Don't use camel case style here

> WebKit-r118336/Source/WebKit/gtk/tests/testwebview.c:655
> +    gchar *html_format;

Misplaced '*'.in these two variables: html_format_base and html_format.

[Daniel Drake]

Created attachment 144555 [details]
updated patch for review

Thanks! Here's a new patch addressing those comments.

If there is not much focus on making WebKit1 and WebKit2 gobject APIs similar, I agree with removing the cancel method (done in the patch).

The one remaining issue I have here is that it complains during build:

/home/dsd/projects/WebKit-r118336/Source/WebKit/gtk/webkit/webkitfilechooserrequest.cpp:56: warning: Section WebKitFileChooserRequest is not defined in the webkitgtk-section.txt file.

Does it mean webkitgtk-sections.txt (with an 's')? I do have an entry there, and I can't see what I'm doing differently compared to other classes. Either way it does not seem to affect the functionality.

[Mario Sanchez Prada]

The patch looks great. So great it looks that I can't do much more on my side other than making some final comments (see below). From now on an official reviewer should take the lead and give the final approval (or not).

In the meanwhile, I'd recommend you to prepare the ChangeLog entry. It's the only thing missing I can spot (and sorry not to have spotted it before).

(In reply to comment #10)
> Created an attachment (id=144555) [details]
> updated patch for review
> 
> Thanks! Here's a new patch addressing those comments.

Great. I think now you only need to run the prepareChangeLog script and provide a properly formatted entry in Source/WebKit/gtk/ChangeLog.

You can do it manually too, but if so make sure you format it properly, avoid tabs (only spaces) and that you include a line like the following one:

  Reviewed by NOBODY (OOPS!).

(This line will get changed with the name of the reviewe on it once it gets r+ and while pushing it to the repository)

> If there is not much focus on making WebKit1 and WebKit2 gobject APIs 
> similar, I agree with removing the cancel method (done in the patch).

The idea is to make them similar, but not necessarily identical. In this case I think it makes sense to leave that function out of the game.

> The one remaining issue I have here is that it complains during build:
> 
> /home/dsd/projects/WebKit-r118336/Source/WebKit/gtk/webkit/webkitfilechooserrequest.cpp:56:
> warning: Section WebKitFileChooserRequest is not defined in the webkitgtk-section.txt file.
> 
> Does it mean webkitgtk-sections.txt (with an 's')? I do have an entry there, and I can't
> see what I'm doing differently compared to other classes. Either way it does not seem to
> affect the functionality.

I've seen this problem going away after forcing a full clean rebuild. Not sure if it's the case here, though.

[Daniel Drake]

Would you mind running the prepare-ChangeLog script for me? I haven't succeeded in checking out the webkit repository (my internet connection isn't very good).

I've been developing on top of the nightly snapshots, however prepare-ChangeLog doesn't work there:

$ ./Tools/Scripts/prepare-ChangeLog 
Couldn't determine your version control system. at ./Tools/Scripts/prepare-ChangeLog line 166.

[Martin Robinson]

(In reply to comment #12)

> $ ./Tools/Scripts/prepare-ChangeLog 
> Couldn't determine your version control system. at ./Tools/Scripts/prepare-ChangeLog line 166.

The dump at http://nightly.webkit.org/files/WebKit-SVN-source.tar.bz2 should theoretically work with prepare-ChangeLog.  Is that the one you are using?

[Daniel Drake]

Created attachment 144635 [details]
Patch

[Daniel Drake]

Thanks Martin, I was using a snapshot without the .svn database. That helped a lot.

Above patch fixes the gtk-doc warning mentioned above. I think that addresses everything raised so far - hopefully ready for final review now.

[Daniel Drake]

I'm finding this is crashing when calling file_chooser_request.select_files() from Python. Will update when I know more.

[Daniel Drake]

That crash was something local/unrelated. The patch is fine. Please go ahead with review (and commit? :))

[Mario Sanchez Prada]

Comment on attachment 144635 [details]
Patch

(In reply to comment #17)
> That crash was something local/unrelated. The patch is fine.

Good

> Please go ahead with review (and commit? :))

I'm now setting the cq? flag so reviewers can set it to cq+ in case the patch got approved, in order to get it in automatically through the commit-queue.

Thanks

[Martin Robinson]

Comment on attachment 144635 [details]
Patch

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

Thanks for writing this patch. It actually looks very much like it's ready to go. There are a few stylistic things that I think should be changed before that though. I always feel a bit bad giving a good patch an r- for mostly stylistic reasons, but it's important that we follow the rules that the community agreed on (otherwise chaos would ensue, undoubtedly). 

> Source/WebKit/gtk/tests/testwebview.c:509
> +static gboolean check_mime_type_for_filter(GtkFileFilter* filter, const gchar* mime_type)

Please use WebKit coding style when writing C files. See the style rules here: http://www.webkit.org/coding/coding-style.html Many of the existing tests were written before we followed these rules as well as we do now. We try to ensure that all new code follows the style rules though.

Note that C files have the bizarre requirement of putting the asterisk next to the variable name, unlike C++ files. Variable names should be like mimeType instead of mime_type.

> Source/WebKit/gtk/tests/testwebview.c:517
> +static gboolean run_file_chooser_cb_1(WebKitWebView* webview, WebKitFileChooserRequest* request, gpointer data)

This names run_file_chooser_cb_1 and run_file_chooser_cb_2 don't really say much about what these callbacks actually do. Consider using names that better describe the what each callback does.

> Source/WebKit/gtk/tests/testwebview.c:522
> +    const gchar* const* mime_types;
> +    const gchar* const* selected_files;
> +    GtkFileFilter* filter;
> +

We don't typically declare variable before we use them like this in WebKit. Please make this change throughout your patch.

> Source/WebKit/gtk/tests/testwebview.c:670
> +    webkit_web_view_load_string(WEBKIT_WEB_VIEW(web_view), html_format, NULL, NULL, NULL);
> +    g_free(html_format);
> +
> +    g_timeout_add(100, (GSourceFunc) click_mouse_button_and_wait_for_file_chooser_request, WEBKIT_WEB_VIEW(web_view));

Instead of using a timeout here it would be better to run the main loop and wait until loading it complete.

> Source/WebKit/gtk/tests/testwebview.c:694
> +    // Multiple selections not allowed, only accept images, audio and video files..

Nit: I think you have an extra period here.

> Source/WebKit/gtk/webkit/webkitfilechooserrequest.cpp:45
> + * some cases, which could prefer to use their own file chooser

"cases , which could prefer" sounds a bit odd here. Perhaps cases, such as when an embedding applications prefers to use its own"

> Source/WebKit/gtk/webkit/webkitfilechooserrequest.cpp:144
> +                                                      WEBKIT_PARAM_READABLE));    /**

Your comment seems to joined to the previous line here. Are you a vim user? :)

> Source/WebKit/gtk/webkit/webkitwebview.cpp:1318
> +static gboolean webkit_web_view_real_run_file_chooser(WebKitWebView* webView, WebKitFileChooserRequest* request)

You should use camelCase for this method name since it isn't exposed in the public API.

[Daniel Drake]

Created attachment 146762 [details]
Patch

[Daniel Drake]

Thanks. I've addressed all comments in the new patch, with one exception: the g_timeout_add.

While I can make it listen for the load-status FINISHED signal, I can't get the mouse-click simulation working from such context. Nothing happens, as if no click has happened. I hope this will be acceptable anyway given that the same file uses this technique in various other places.

[Martin Robinson]

Comment on attachment 146762 [details]
Patch

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

Okay. This looks pretty reasonable to me. We need one more reviewer to approve the API. I'd also like to clean up a few tiny details before landing this one.

> Source/WebKit/gtk/ChangeLog:9
> +rada's

Looks like an accidental newline here.

[Daniel Drake]

Created attachment 147465 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 147465 [details]
Patch

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

Thanks for your work! Looks good to me, but have a few comments, probably needs a new patch uploaded since you're not a committer, I guess?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2605
> +     * This signal is emitted when the user interacts with a <input
> +     * type='file' /> HTML element, requesting from WebKit to show
> +     * a dialog to select one or more files to be uploaded. To let the
> +     * application know the details of the file chooser, as well as to
> +     * allow the client application to either cancel the request or
> +     * perform an actual selection of files, the signal will pass an
> +     * instance of the #WebKitFileChooserRequest in the @request
> +     * argument.

I miss a comment about the lifecycle of the FileChooserRequest object and how to handle this async. If I want to handle this signal asynchronously, do I ref the request, return TRUE and then call its select_files() method when I have the chosen files? If that's the case (and it seems to be from my quick inspection of the default handler), then I think the API looks good, but having this spelled out here would help =).

> Source/WebKit/gtk/webkit/webkitwebview.h:170
> +    gboolean                   (* run_file_chooser)       (WebKitWebView            *web_view,
> +                                                           WebKitFileChooserRequest *request);

Should go at the bottom and consume the padding.

> Source/WebKit/gtk/webkit/webkitwebviewprivate.h:140
> +void webkit_web_view_run_file_chooser_request(WebKitWebView*, WebKitFileChooserRequest*);

This function should use camelCase like the other internal non-public ones.

[Daniel Drake]

Thanks for the review. Yes, I'll post a new patch.

(In reply to comment #24)
> > Source/WebKit/gtk/webkit/webkitwebview.cpp:2605
> > +     * This signal is emitted when the user interacts with a <input
> > +     * type='file' /> HTML element, requesting from WebKit to show
> > +     * a dialog to select one or more files to be uploaded. To let the
> > +     * application know the details of the file chooser, as well as to
> > +     * allow the client application to either cancel the request or
> > +     * perform an actual selection of files, the signal will pass an
> > +     * instance of the #WebKitFileChooserRequest in the @request
> > +     * argument.
> 
> I miss a comment about the lifecycle of the FileChooserRequest object and how to handle this async. If I want to handle this signal asynchronously, do I ref the request, return TRUE and then call its select_files() method when I have the chosen files? If that's the case (and it seems to be from my quick inspection of the default handler), then I think the API looks good, but having this spelled out here would help =).

This was just a copy/paste from the WebKit2 gobject API. Mario, can you help me answer this? Is Gustavo's interpretation correct?

[Mario Sanchez Prada]

(In reply to comment #25)
> [...]
> > I miss a comment about the lifecycle of the FileChooserRequest object and how 
> > to handle this async. If I want to handle this signal asynchronously, do I 
> > ref the request, return TRUE and then call its select_files() method when I 
> > have the chosen files? If that's the case (and it seems to be from my quick 
> > inspection of the default handler), then I think the API looks good, but 
> > having this spelled out here would help =).
> 
> This was just a copy/paste from the WebKit2 gobject API. Mario, can you help
> me answer this? Is Gustavo's interpretation correct?

I think Gustavo means adding an extra comment to the run-file-chooser signal, in a similar fashion to what it's done for geolocation-policy-decision-requested:

    /**
     * WebKitWebView::geolocation-policy-decision-requested:
     * @web_view: the object on which the signal is emitted
     * @frame: the frame that requests permission
     * @policy_decision: a WebKitGeolocationPolicyDecision
     *
     * This signal is emitted when a @frame wants to obtain the user's
     * location. The decision can be made asynchronously, but you must
     * call g_object_ref() the @policy_decision, and return %TRUE if
     * you are going to handle the request. To actually make the
     * decision you need to call webkit_geolocation_policy_allow() or
     * webkit_geolocation_policy_deny() on @policy_decision.
     *
     * Since: 1.1.23
     */

Is this my interpretation correct, Gustavo?

[Daniel Drake]

Created attachment 147888 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 147888 [details]
Patch

Looks good, we forgot the Since tags though, will land when a new patch carrying them is uploaded.

[Daniel Drake]

Created attachment 148661 [details]
Patch

[WebKit Review Bot]

Comment on attachment 148661 [details]
Patch

Clearing flags on attachment: 148661

Committed r120918: <http://trac.webkit.org/changeset/120918>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.