Bug 78491 – [GTK] Implement WebUIClient's runOpenPanel in WebKit2GTK+

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Mario Sanchez Prada]

We need to implement this functionality for the <input type="file" /> HTML5 tag[1] to work in WebKit2GTK+ based browsers.

[1] http://dev.w3.org/html5/markup/input.file.html

[Mario Sanchez Prada]

Created attachment 127102 [details]
Patch Proposal + Unit tests

Attaching a provisional patch for this bug, which in theory is lacking documentation only. Let's see what reviewers say :-)

The attachment is a follow-up patch for the discussion started in the webkit-gtk mailing list (see thread at [1]), and the main issues that changed since the last patch sent to the list (see [2]) would be the following:

  - Implemented needed unit tests for checking the whole new API, in TestWebKitWebView.cpp.
  - Implemented new function mouseButtonClick() in WebViewTest, to simulate button clicks through GdkEvent's (required for writing the unit test)
  - Consider the MIME handlers in the default handler for 'run-file-chooser' provided by the WebView (to set a GtkFileFilter)
  - Migrated all the occurrences in the API of gchar**  to GSList*, for consistency. Also, to easy usage from apps, those functions returning GSList* will work on a [transfer none] basis, so the caller won't have to bother neither about freeing the GSList nor about its elements. WebKitFileChooserRequest will internally cache those lists and their values.
  - Make MiniBrowser use this API too, for easily testing it (it will just connect to the 'run-file-chooser' signal and provide its own GtkFileChooserDialog)
  - Some fixes all around the patch

So, if the API implemented in this patch sounds more or less OK, my next step would be to document everything and polish the patch, in order to ask for a proper and formal review. Informal reviews are appreciated in the meantime, though :-)

[1] https://lists.webkit.org/pipermail/webkit-gtk/2012-February/000965.html
[2] https://lists.webkit.org/pipermail/webkit-gtk/2012-February/000972.html

[Mario Sanchez Prada]

Adding some additional WebKitGTK+ reviewers

[Philippe Normand]

Comment on attachment 127102 [details]
Patch Proposal + Unit tests

Attachment 127102 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11518549

[Carlos Garcia Campos]

Comment on attachment 127102 [details]
Patch Proposal + Unit tests

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

Great work. All the patches including new WebKit2 GTK+ API should be fully documented. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:42
> +    GSList* acceptedMimeTypes;
> +    GSList* selectedFiles;

I don't think we want to cache these

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:43
> +    gboolean handledRequest;

Use bool, instead of gboolean here.

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:52
> +    request->priv->acceptedMimeTypes = 0;
> +    request->priv->selectedFiles = 0;
> +    request->priv->handledRequest = false;

Struct instances are always 0 initialized, so you don't need these initializations.

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:67
> +    if (request->priv->acceptedMimeTypes)
> +        g_slist_free_full(request->priv->acceptedMimeTypes, g_free);
> +
> +    if (request->priv->selectedFiles)
> +        g_slist_free_full(request->priv->selectedFiles, g_free);

Don't cache these, the api should just return a transfer-full GSList in both cases that user will free. It's very unlikely that these are requested twice for the same filechooser request.

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:88
> +gboolean webkit_file_chooser_request_allows_multiple_files(WebKitFileChooserRequest* request)

Documentation is missing for this function

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:94
> +GSList* webkit_file_chooser_request_accepted_mime_types(WebKitFileChooserRequest* request)

Documentation missing. webkit_file_chooser_get_request_accepted_mime_types, maybe?

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:97
> +    WKRetainPtr<WKArrayRef> wkMimeTypes(WKOpenPanelParametersCopyAcceptedMIMETypes(request->priv->wkParameters.get()));

I think you should adopt the reference to avoid leaking the array.

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:101
> +    if (numOfMimeTypes) {

If numOfMimeTypes is 0 the for loop won't iterate anyway, so you don't need to check it here.

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:102
> +        for (size_t i = 0; i < numOfMimeTypes; ++i) {

we can simply use WKArrayGetSize(wkMimeTypes.get()), we don't need to cache the size in a local var.

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:119
> +GtkFileFilter* webkit_file_chooser_request_get_file_filter(WebKitFileChooserRequest* request)

Documentation missing

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:125
> +    GSList* acceptedMimeTypes(webkit_file_chooser_request_accepted_mime_types(request));
> +    if (!acceptedMimeTypes)
> +        return 0;

This is convenient, but we end up iterating the mime types twice and allocating every mime-type twice too. I would use the same code than webkit_file_chooser_request_accepted_mime_types but adding the mimetypes to the filter instead of to the list.

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:134
> +void webkit_file_chooser_request_choose_files(WebKitFileChooserRequest* request, GSList* fileURIs)

Docs

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:150
> +GSList* webkit_file_chooser_request_selected_files(WebKitFileChooserRequest* request)

webkit_file_chooser_request_get_selected_files? You already know this should be documented too :-)

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:159
> +    size_t numOfFiles = selectedFileNames.size();
> +    if (numOfFiles) {
> +        for (size_t i = 0; i < numOfFiles; ++i)

Same here, we don't need to check numOfFiles is not 0, nor to cache the size, just use selectedFileNames.size()

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.h:58
> +webkit_file_chooser_request_allows_multiple_files (WebKitFileChooserRequest*);

Include the parameter names in public headers and place the * adjacent to the parameter identifier.

webkit_file_chooser_request_allows_multiple_files (WebKitFileChooserRequest *request);

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.h:60
> +WEBKIT_API GSList*

Add a space between type name and *

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.h:73
> +webkit_file_chooser_request_accepted_mime_types   (WebKitFileChooserRequest*);
> +
> +WEBKIT_API GtkFileFilter*
> +webkit_file_chooser_request_get_file_filter       (WebKitFileChooserRequest*);
> +
> +WEBKIT_API void
> +webkit_file_chooser_request_choose_files          (WebKitFileChooserRequest*, GSList*);
> +
> +WEBKIT_API GSList*
> +webkit_file_chooser_request_selected_files        (WebKitFileChooserRequest*);
> +
> +WEBKIT_API void
> +webkit_file_chooser_request_cancel                (WebKitFileChooserRequest*);

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:24
> +#include "WebKitFileChooserRequest.h"
> +#include "WebKitFileChooserRequestPrivate.h"

WebKitFileChooserRequestPrivate.h already includes WebKitFileChooserRequest.h

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:183
> +    GtkWidget* dialog = gtk_file_chooser_dialog_new(_("Upload File"),

I would use something like 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.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:194
> +

GtkFileChooser doesn't allow to select multiple files, so just pick the first one and call gtk_file_chooser_select_uri()

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:664
> +    /* TODO: Describe the signal. */

Nothing more to add :-P

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:434
> +    GSList* mimeType = mimeTypes;
> +    g_assert_cmpstr(static_cast<gchar*>(mimeType->data), ==, "audio/*");
> +    mimeType = g_slist_next(mimeType);
> +    g_assert_cmpstr(static_cast<gchar*>(mimeType->data), ==, "video/*");
> +    mimeType = g_slist_next(mimeType);
> +    g_assert_cmpstr(static_cast<gchar*>(mimeType->data), ==, "image/*");
> +    mimeType = g_slist_next(mimeType);
> +    g_assert(!mimeType);

You could also check that any other mime types is not in the list

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:442
> +    filter = webkit_file_chooser_request_get_file_filter(fileChooserRequest);
> +    g_assert(GTK_IS_FILE_FILTER(filter.get()));
> +    g_assert(checkMimeTypeForFilter(filter.get(), "audio/*"));
> +    g_assert(checkMimeTypeForFilter(filter.get(), "video/*"));
> +    g_assert(checkMimeTypeForFilter(filter.get(), "image/*"));
> +    selectedFiles = webkit_file_chooser_request_selected_files(fileChooserRequest);
> +    g_assert(!selectedFiles);

Ditto.

> Tools/MiniBrowser/gtk/BrowserWindow.c:287
> +

You are duplicating the code of the default handler here, just let the minibrowser use the default handler

[WebKit Review Bot]

Attachment 127102 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/GNUmakefile.am', u'Source/W..." exit_code: 1

WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.h"
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:188:  Use 0 instead of NULL.  [readability/null] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Mario Sanchez Prada]

Thanks for the review, Carlos.

Just some comments to advance on the topic, even if I won't be able to code anything at all today...

(In reply to comment #4)
> (From update of attachment 127102 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127102&action=review
> 
> Great work. All the patches including new WebKit2 GTK+ API should be fully documented. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Yeah, as I said before, I haven't documented anything yet, but I plan to do it next, after addressing the issues that might come up from the review process.

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:42
> > +    GSList* acceptedMimeTypes;
> > +    GSList* selectedFiles;
> 
> I don't think we want to cache these

This has been the result of a discussion in Jabber with Martin where we talked about maybe returning GSList* with [transfer none] in thos methods that were previously returning gchar**... and so I needed to cache those lists to ensure that memory will be retained by the API as needed.

However, I have to say that I'm not that convinced about that now and that I think perhaps it would be better, easier and simpler to return a GSList* with [transfer full], so the caller takes all the responsibility on freeing both the GSList and its elements, which is what you were proposing in the first place.

So, in a nutshell, I agree with you on removing these two things from here, and also in using [transfer full] for those methods returning GSList*. I just didnt change it before attaching this patch because (1) I already had it implemented that way, (2) I wanted to trigger this discussion and (3) it was quite late already when I uploaded  the patch (4:30am!) :-).

Will change it in a follow up patch then.

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:43
> > +    gboolean handledRequest;
> 
> Use bool, instead of gboolean here.

Oops! Will do.

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:52
> > +    request->priv->acceptedMimeTypes = 0;
> > +    request->priv->selectedFiles = 0;
> > +    request->priv->handledRequest = false;
> 
> Struct instances are always 0 initialized, so you don't need these initializations.

I'd swear it was not working that way for acceptedMimeTypes and selectedFiles in this case... but that doesn't matter much anyway since I will be removing them anyway.

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:67
> > +    if (request->priv->acceptedMimeTypes)
> > +        g_slist_free_full(request->priv->acceptedMimeTypes, g_free);
> > +
> > +    if (request->priv->selectedFiles)
> > +        g_slist_free_full(request->priv->selectedFiles, g_free);
> 
> Don't cache these, the api should just return a transfer-full GSList in both cases that user will free.

As I said before, I will change this and the caller will have the responsibility of freeing everything.

> It's very unlikely that these are requested twice for the same filechooser request.

Agree.

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:88
> > +gboolean webkit_file_chooser_request_allows_multiple_files(WebKitFileChooserRequest* request)
> 
> Documentation is missing for this function

Yes, Will add it later (will work on that tomorrow).

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:94
> > +GSList* webkit_file_chooser_request_accepted_mime_types(WebKitFileChooserRequest* request)
> 
> Documentation missing. webkit_file_chooser_get_request_accepted_mime_types, maybe?

You probably mean webkit_file_chooser_request_get_accepted_mime_types :-)

In any case, I agree with the change. I intentionally ommitted the _get_ part thinking of what we normally do in WebKit, but it's true that in GNOME libraries this _get_ part is quite common, so I think it makes sense. Will change.

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:97
> > +    WKRetainPtr<WKArrayRef> wkMimeTypes(WKOpenPanelParametersCopyAcceptedMIMETypes(request->priv->wkParameters.get()));
> 
> I think you should adopt the reference to avoid leaking the array.

You're probably right. Will fix it.

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:101
> > +    if (numOfMimeTypes) {
> 
> If numOfMimeTypes is 0 the for loop won't iterate anyway, so you don't need to check it here.

Argh! This comes from the times when I used a gchar**. Will fix it.

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:102
> > +        for (size_t i = 0; i < numOfMimeTypes; ++i) {
> 
> we can simply use WKArrayGetSize(wkMimeTypes.get()), we don't need to cache the size in a local var.

Sure. I cached it because I was using it in two places, but if I'm removing that 'if' condition this caching does not make any sense.

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:119
> > +GtkFileFilter* webkit_file_chooser_request_get_file_filter(WebKitFileChooserRequest* request)
> 
> Documentation missing

Yes, yes... :-)

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:125
> > +    GSList* acceptedMimeTypes(webkit_file_chooser_request_accepted_mime_types(request));
> > +    if (!acceptedMimeTypes)
> > +        return 0;
> 
> This is convenient, but we end up iterating the mime types twice and allocating every mime-type twice too. I would use the same code than webkit_file_chooser_request_accepted_mime_types but adding the mimetypes to the filter instead of to the list.

Side effects of worshipping the DRY principle too much I guess :-). Agree with you that, in this case it's better to just clone the code (after all, it's not that much).

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:134
> > +void webkit_file_chooser_request_choose_files(WebKitFileChooserRequest* request, GSList* fileURIs)
> 
> Docs

Will work on that tomorrow.

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:150
> > +GSList* webkit_file_chooser_request_selected_files(WebKitFileChooserRequest* request)
> 
> webkit_file_chooser_request_get_selected_files? 

Same than with accepted_mimetypes(): I agree with you and will change it

> You already know this should be documented too :-)

You can be quite insistent when you want... ;-)

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:159
> > +    size_t numOfFiles = selectedFileNames.size();
> > +    if (numOfFiles) {
> > +        for (size_t i = 0; i < numOfFiles; ++i)
> 
> Same here, we don't need to check numOfFiles is not 0, nor to cache the size, just use selectedFileNames.size()

Sure. Will fix both.

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.h:58
> > +webkit_file_chooser_request_allows_multiple_files (WebKitFileChooserRequest*);
> 
> Include the parameter names in public headers and place the * adjacent to the parameter identifier.

Argh! This switching between coding styles kills me sometimes... will fix it.

> webkit_file_chooser_request_allows_multiple_files (WebKitFileChooserRequest *request);
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.h:60
> > +WEBKIT_API GSList*
> 
> Add a space between type name and *

Ok.

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.h:73
> > +webkit_file_chooser_request_accepted_mime_types   (WebKitFileChooserRequest*);
> > +
> > +WEBKIT_API GtkFileFilter*
> > +webkit_file_chooser_request_get_file_filter       (WebKitFileChooserRequest*);
> > +
> > +WEBKIT_API void
> > +webkit_file_chooser_request_choose_files          (WebKitFileChooserRequest*, GSList*);
> > +
> > +WEBKIT_API GSList*
> > +webkit_file_chooser_request_selected_files        (WebKitFileChooserRequest*);
> > +
> > +WEBKIT_API void
> > +webkit_file_chooser_request_cancel                (WebKitFileChooserRequest*);
> 
> Ditto.

Sure thing.

> > Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:24
> > +#include "WebKitFileChooserRequest.h"
> > +#include "WebKitFileChooserRequestPrivate.h"
> 
> WebKitFileChooserRequestPrivate.h already includes WebKitFileChooserRequest.h

Ok.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:183
> > +    GtkWidget* dialog = gtk_file_chooser_dialog_new(_("Upload File"),
> 
> I would use something like 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.

I know what you mean and I agree with it. I just used the same string I could find in WK1's ChromeClientGtk.cpp since I think it makes sense it's the same thing in both places. Maybe we should file a l10n bug about this specific thing?

I won't change this for now, for the sake of consistency among WK and WK2.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:194
> > +
> 
> GtkFileChooser doesn't allow to select multiple files, so just pick the first one and call gtk_file_chooser_select_uri()

Hmmm I think you're mixing two different things here:

  - gtk_file_chooser_select_uri(): pre-selects a file in a GtkFileChooser. Not using it yet.
  - gtk_file_chooser_set_select_multiple(): allows selecting multiple files in the file chooser with the help of Ctrl and Shift modifiers, but does _not_ pre-select anything in the file chooser (it just enables the possibility for the user to do it). This is what I'm using here.

So I think the code here is just correct.

Another thing is whether we should use gtk_file_chooser_select_uri() to pre-select a file in the dialog when available. And in such a case, as per the limitation you already pointed out, we can only pre-select one file from the list returned by webkit_file_chooser_request_get_selected() (probably the first one).

I will add that in my follow up patch then, although I must point out this step is not happening either in WK1's ChromeClientGtk (maybe we should update the code there too?).

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:664
> > +    /* TODO: Describe the signal. */
> 
> Nothing more to add :-P

Yes, pretty self-descriptive I'd say :P

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:434
> > +    GSList* mimeType = mimeTypes;
> > +    g_assert_cmpstr(static_cast<gchar*>(mimeType->data), ==, "audio/*");
> > +    mimeType = g_slist_next(mimeType);
> > +    g_assert_cmpstr(static_cast<gchar*>(mimeType->data), ==, "video/*");
> > +    mimeType = g_slist_next(mimeType);
> > +    g_assert_cmpstr(static_cast<gchar*>(mimeType->data), ==, "image/*");
> > +    mimeType = g_slist_next(mimeType);
> > +    g_assert(!mimeType);
> 
> You could also check that any other mime types is not in the list

Hmm... but that's precisely what I do by explicitly checking the mime tipe in each position _and_ that there are no more elements in the list after those, right?

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:442
> > +    filter = webkit_file_chooser_request_get_file_filter(fileChooserRequest);
> > +    g_assert(GTK_IS_FILE_FILTER(filter.get()));
> > +    g_assert(checkMimeTypeForFilter(filter.get(), "audio/*"));
> > +    g_assert(checkMimeTypeForFilter(filter.get(), "video/*"));
> > +    g_assert(checkMimeTypeForFilter(filter.get(), "image/*"));
> > +    selectedFiles = webkit_file_chooser_request_selected_files(fileChooserRequest);
> > +    g_assert(!selectedFiles);
> 
> Ditto.

Same doubt here. Maybe I'm not understanding you properly?

> > Tools/MiniBrowser/gtk/BrowserWindow.c:287
> > +
> 
> You are duplicating the code of the default handler here, just let the minibrowser use the default handler

Yes, I know... my point is to make sure we are using the new API in MiniBrowser to allow manually checking it works fine, even if the result is exactly the same than the default handler. I had some doubts about this part of the patch precisely of that... so maybe I should just drop it?

[Carlos Garcia Campos]

(In reply to comment #7)
> Thanks for the review, Carlos.
> 
> Just some comments to advance on the topic, even if I won't be able to code anything at all today...
> 
> (In reply to comment #4)
> > (From update of attachment 127102 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=127102&action=review
> > 
> > Great work. All the patches including new WebKit2 GTK+ API should be fully documented. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
> 
> Yeah, as I said before, I haven't documented anything yet, but I plan to do it next, after addressing the issues that might come up from the review process.

Sorry I didn't read your comments :-P
 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:42
> > > +    GSList* acceptedMimeTypes;
> > > +    GSList* selectedFiles;
> > 
> > I don't think we want to cache these
> 
> This has been the result of a discussion in Jabber with Martin where we talked about maybe returning GSList* with [transfer none] in thos methods that were previously returning gchar**... and so I needed to cache those lists to ensure that memory will be retained by the API as needed.

FileChooseRequest is a temporary value, to be used only during the async operation, so mime-types and selected files won't be asked more than once. I think it's perfectly fine to return a trasfer-full GSList in both cases.
 
> However, I have to say that I'm not that convinced about that now and that I think perhaps it would be better, easier and simpler to return a GSList* with [transfer full], so the caller takes all the responsibility on freeing both the GSList and its elements, which is what you were proposing in the first place.
> 
> So, in a nutshell, I agree with you on removing these two things from here, and also in using [transfer full] for those methods returning GSList*. I just didnt change it before attaching this patch because (1) I already had it implemented that way, (2) I wanted to trigger this discussion and (3) it was quite late already when I uploaded  the patch (4:30am!) :-).
> 
> Will change it in a follow up patch then.

Thanks

> > Struct instances are always 0 initialized, so you don't need these initializations.
> 
> I'd swear it was not working that way for acceptedMimeTypes and selectedFiles in this case... but that doesn't matter much anyway since I will be removing them anyway.

I'm quite sure it was.
 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:94
> > > +GSList* webkit_file_chooser_request_accepted_mime_types(WebKitFileChooserRequest* request)
> > 
> > Documentation missing. webkit_file_chooser_get_request_accepted_mime_types, maybe?
> 
> You probably mean webkit_file_chooser_request_get_accepted_mime_types :-)

indeed
 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:183
> > > +    GtkWidget* dialog = gtk_file_chooser_dialog_new(_("Upload File"),
> > 
> > I would use something like 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.
> 
> I know what you mean and I agree with it. I just used the same string I could find in WK1's ChromeClientGtk.cpp since I think it makes sense it's the same thing in both places. Maybe we should file a l10n bug about this specific thing?
> 
> I won't change this for now, for the sake of consistency among WK and WK2.

I agree with keeping consistency between wk1 and wk2 when there's no a good reason not to do it. In this case, I think there's one :-)
 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:194
> > > +
> > 
> > GtkFileChooser doesn't allow to select multiple files, so just pick the first one and call gtk_file_chooser_select_uri()
> 
> Hmmm I think you're mixing two different things here:

No, I don't

>   - gtk_file_chooser_select_uri(): pre-selects a file in a GtkFileChooser. Not using it yet.
>   - gtk_file_chooser_set_select_multiple(): allows selecting multiple files in the file chooser with the help of Ctrl and Shift modifiers, but does _not_ pre-select anything in the file chooser (it just enables the possibility for the user to do it). This is what I'm using here.
> 
> So I think the code here is just correct.

I haven't said the code is wrong, I was just suggesting to pre-select the first file in selectedFiles. I don't know much about html, but I think that file input element has an option to indicate preselected files, no?

> Another thing is whether we should use gtk_file_chooser_select_uri() to pre-select a file in the dialog when available. And in such a case, as per the limitation you already pointed out, we can only pre-select one file from the list returned by webkit_file_chooser_request_get_selected() (probably the first one).

This is exactly what I was proposing :-)

> I will add that in my follow up patch then, although I must point out this step is not happening either in WK1's ChromeClientGtk (maybe we should update the code there too?).

Yes, why not, the fact that it's not implemented in wk1 doesn't mean we shoulnd't do it win wk2.

> > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:434
> > > +    GSList* mimeType = mimeTypes;
> > > +    g_assert_cmpstr(static_cast<gchar*>(mimeType->data), ==, "audio/*");
> > > +    mimeType = g_slist_next(mimeType);
> > > +    g_assert_cmpstr(static_cast<gchar*>(mimeType->data), ==, "video/*");
> > > +    mimeType = g_slist_next(mimeType);
> > > +    g_assert_cmpstr(static_cast<gchar*>(mimeType->data), ==, "image/*");
> > > +    mimeType = g_slist_next(mimeType);
> > > +    g_assert(!mimeType);
> > 
> > You could also check that any other mime types is not in the list
> 
> Hmm... but that's precisely what I do by explicitly checking the mime tipe in each position _and_ that there are no more elements in the list after those, right?

Ah, I didn't notice you were using g_slit_next(), forget my comment then, sorry for the noise.

> > > Tools/MiniBrowser/gtk/BrowserWindow.c:287
> > > +
> > 
> > You are duplicating the code of the default handler here, just let the minibrowser use the default handler
> 
> Yes, I know... my point is to make sure we are using the new API in MiniBrowser to allow manually checking it works fine, even if the result is exactly the same than the default handler. I had some doubts about this part of the patch precisely of that... so maybe I should just drop it?

I think so

[Mario Sanchez Prada]

Hi again,

It looks like I finally got a 1,5h quantum of time to work today so I was working in a new patch considering the issues you pointed out (including those in this last review) that I'll be attaching in 5 min (no docs yet).

In the meantime, see some comments below...

(In reply to comment #8)
> [...]
> > Yeah, as I said before, I haven't documented anything yet, but I plan to do it next, after addressing the issues that might come up from the review process.
> 
> Sorry I didn't read your comments :-P

No problem.

> > > > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:42
> > > > +    GSList* acceptedMimeTypes;
> > > > +    GSList* selectedFiles;
> > > 
> > > I don't think we want to cache these
> > 
> > This has been the result of a discussion in Jabber with Martin where we talked about maybe returning GSList* with [transfer none] in thos methods that were previously returning gchar**... and so I needed to cache those lists to ensure that memory will be retained by the API as needed.
> 
> FileChooseRequest is a temporary value, to be used only during the async operation, so mime-types and selected files won't be asked more than once. I think it's perfectly fine to return a trasfer-full GSList in both cases.

Yes, I came to the same conclusion yesterday at night while working in the patch.

[...]
> > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:183
> > > > +    GtkWidget* dialog = gtk_file_chooser_dialog_new(_("Upload File"),
> > > 
> > > I would use something like 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.
> > 
> > I know what you mean and I agree with it. I just used the same string I could find in WK1's ChromeClientGtk.cpp since I think it makes sense it's the same thing in both places. Maybe we should file a l10n bug about this specific thing?
> > 
> > I won't change this for now, for the sake of consistency among WK and WK2.
> 
> I agree with keeping consistency between wk1 and wk2 when there's no a good reason not to do it. In this case, I think there's one :-)

You convinced me. I will change it.
 
> > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:194
> > > > +
> > > 
> > > GtkFileChooser doesn't allow to select multiple files, so just pick the first one and call gtk_file_chooser_select_uri()
> > 
> > Hmmm I think you're mixing two different things here:
> 
> No, I don't
> 
> >   - gtk_file_chooser_select_uri(): pre-selects a file in a GtkFileChooser. Not using it yet.
> >   - gtk_file_chooser_set_select_multiple(): allows selecting multiple files in the file chooser with the help of Ctrl and Shift modifiers, but does _not_ pre-select anything in the file chooser (it just enables the possibility for the user to do it). This is what I'm using here.
> > 
> > So I think the code here is just correct.
> 
> I haven't said the code is wrong, I was just suggesting to pre-select the first file in selectedFiles. I don't know much about html, but I think that file input element has an option to indicate preselected files, no?

My bad. I got you definitely wrong.
 
> > Another thing is whether we should use gtk_file_chooser_select_uri() to pre-select a file in the dialog when available. And in such a case, as per the limitation you already pointed out, we can only pre-select one file from the list returned by webkit_file_chooser_request_get_selected() (probably the first one).
> 
> This is exactly what I was proposing :-)

I'll do that then.

> > I will add that in my follow up patch then, although I must point out this step is not happening either in WK1's ChromeClientGtk (maybe we should update the code there too?).
> 
> Yes, why not, the fact that it's not implemented in wk1 doesn't mean we shoulnd't do it win wk2.

Fair enough :)

> > > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:434
> > > > +    GSList* mimeType = mimeTypes;
> > > > +    g_assert_cmpstr(static_cast<gchar*>(mimeType->data), ==, "audio/*");
> > > > +    mimeType = g_slist_next(mimeType);
> > > > +    g_assert_cmpstr(static_cast<gchar*>(mimeType->data), ==, "video/*");
> > > > +    mimeType = g_slist_next(mimeType);
> > > > +    g_assert_cmpstr(static_cast<gchar*>(mimeType->data), ==, "image/*");
> > > > +    mimeType = g_slist_next(mimeType);
> > > > +    g_assert(!mimeType);
> > > 
> > > You could also check that any other mime types is not in the list
> > 
> > Hmm... but that's precisely what I do by explicitly checking the mime tipe in each position _and_ that there are no more elements in the list after those, right?
> 
> Ah, I didn't notice you were using g_slit_next(), forget my comment then, sorry for the noise.

Ok.

> > > > Tools/MiniBrowser/gtk/BrowserWindow.c:287
> > > > +
> > > 
> > > You are duplicating the code of the default handler here, just let the minibrowser use the default handler
> > 
> > Yes, I know... my point is to make sure we are using the new API in MiniBrowser to allow manually checking it works fine, even if the result is exactly the same than the default handler. I had some doubts about this part of the patch precisely of that... so maybe I should just drop it?
> 
> I think so

I'll remove it then

[Mario Sanchez Prada]

Created attachment 127182 [details]
Patch proposal + Unit tests

New patch incorporating all the suggestions pointed out by Carlos BUT the documentation (will do tomorrow).

[WebKit Review Bot]

Attachment 127182 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/GNUmakefile.am', u'Source/W..." exit_code: 1

WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.h"
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:189:  Use 0 instead of NULL.  [readability/null] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 127182 [details]
Patch proposal + Unit tests

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

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:101
> +GtkFileFilter* webkit_file_chooser_request_get_file_filter(WebKitFileChooserRequest* request)

I know the GTK type is GtkFileFilter, but maybe it's easier to understand if this is called get_mime_type_filter? or get_accepted_mime_type_filter? to make it clear that this function returns the same than webkit_file_chooser_request_get_accepted_mime_types() but in a GtkFileFilter just to make it easier for GtkFileChooser users.

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:126
> +    for (GSList* item = fileURIs ; item ; item = g_slist_next(item)) {

extra space between fileURIs and ; and item and ;

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:137
> +GSList* webkit_file_chooser_request_get_selected_files(WebKitFileChooserRequest* request)

Since we are retuning uris, maybe it would be better to call this get_selected_uris. GtkFileChooser has select_file(), select_filename() and select_uri(), so it might be confusing, the same applies to choose_files(), what do you think?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:191
> +    GtkFileFilter* filter = webkit_file_chooser_request_get_file_filter(request);

Use GRefPtr, since GtkFileFilter should be freed by the caller now.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:374
> +    GRefPtr<GtkFileFilter> filter(webkit_file_chooser_request_get_file_filter(fileChooserRequest));

You should use adoptGRef here, or not use GRefPtr at all since you are expecting a NULL.

[Martin Robinson]

(In reply to comment #8)

> > > > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:42
> > > > +    GSList* acceptedMimeTypes;
> > > > +    GSList* selectedFiles;
> > > 
> > > I don't think we want to cache these
> > 
> > This has been the result of a discussion in Jabber with Martin where we talked about maybe returning GSList* with [transfer none] in thos methods that were previously returning gchar**... and so I needed to cache those lists to ensure that memory will be retained by the API as needed.
> 
> FileChooseRequest is a temporary value, to be used only during the async operation, so mime-types and selected files won't be asked more than once. I think it's perfectly fine to return a trasfer-full GSList in both cases.

I think it's much better to cache these lists within the FileChooserRequest. It isn't for performance reasons, but to make using the API simpler -- as the client shouldn't need to free things that are really owned by the FileChooserRequest. This is also quite important because these values may be allocated by WebKit's internal allocator (now or in the future). We should be using this allocator when we can. Thus, I feel *strongly* that these should be cached.

[Carlos Garcia Campos]

(In reply to comment #13)
> I think it's much better to cache these lists within the FileChooserRequest. It isn't for performance reasons, but to make using the API simpler -- as the client shouldn't need to free things that are really owned by the FileChooserRequest. This is also quite important because these values may be allocated by WebKit's internal allocator (now or in the future). We should be using this allocator when we can. Thus, I feel *strongly* that these should be cached.

Ok, it's fine with me

[Gustavo Noronha (kov)]

Comment on attachment 127182 [details]
Patch proposal + Unit tests

Attachment 127182 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11537105

[Mario Sanchez Prada]

Created attachment 127357 [details]
Patch Proposal + Unit Tests

New patch addressing the issues commented by Carlos and Martin in their last comments.

Btw, besides bringing the cache for those GSList's back to the WebKitFileChooserRequest object, I've also cached the GtkFileFilter in there, for similar reasons.

[Philippe Normand]

Comment on attachment 127357 [details]
Patch Proposal + Unit Tests

Attachment 127357 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11534803

[Carlos Garcia Campos]

Comment on attachment 127357 [details]
Patch Proposal + Unit Tests

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

Perfect!

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:87
> +gboolean webkit_file_chooser_request_allows_multiple_files(WebKitFileChooserRequest* request)

I'm thinking that maybe we could use something like webkit_file_chooser_request_allow_select_multiple() or get_select_multiple(). That way we avoid using files here (for the confusion with files, filenames and uris) and it's consistent with GtkFileChooser API.

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:139
> +void webkit_file_chooser_request_choose_files(WebKitFileChooserRequest* request, GSList* fileURIs)

what about using choose_uris as I suggested? to make it clear that the list must be a list of uris and not files nor filenames.

[Mario Sanchez Prada]

(In reply to comment #18)
> (From update of attachment 127357 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127357&action=review
> 
> Perfect!
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:87
> > +gboolean webkit_file_chooser_request_allows_multiple_files(WebKitFileChooserRequest* request)
> 
> I'm thinking that maybe we could use something like webkit_file_chooser_request_allow_select_multiple() or get_select_multiple(). That way we avoid using files here (for the confusion with files, filenames and uris) and it's consistent with GtkFileChooser API.

I think it makes sense. I hereby propose webkit_file_chooser_request_allow_select_multiple_selection()  :-)
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:139
> > +void webkit_file_chooser_request_choose_files(WebKitFileChooserRequest* request, GSList* fileURIs)
> 
> what about using choose_uris as I suggested? to make it clear that the list must be a list of uris and not files nor filenames

You're definitely right.

I will change those in the next patch, but first, I'd like to hear others' opinions (Martin?), in order not to flood this bug with maybe too many quick changes.

[Mario Sanchez Prada]

(In reply to comment #19)
>[...]
> I think it makes sense. I hereby propose webkit_file_chooser_request_allow_select_multiple_selection()  :-)

Oops! I meant webkit_file_chooser_request_allows_multiple_selection()

Reason: it's consistent with what's specified in the description for that optional attribute here: 
http://dev.w3.org/html5/markup/input.file.html

[Carlos Garcia Campos]

(In reply to comment #20)
> (In reply to comment #19)
> >[...]
> > I think it makes sense. I hereby propose webkit_file_chooser_request_allow_select_multiple_selection()  :-)
> 
> Oops! I meant webkit_file_chooser_request_allows_multiple_selection()
> 
> Reason: it's consistent with what's specified in the description for that optional attribute here: 
> http://dev.w3.org/html5/markup/input.file.html

Ok, sounds good too.

[Mario Sanchez Prada]

Today, I was thinking again quite deeply about the implications of representing the list of mime types and selected files for the request as either a GSList* or a gchar**, and surpsingly I reached the same conclusion than Carlos yesterday: contrary to what I thought, using gchar** everywhere seems like the way to go.

Reasons for this are several and of course arguable, but IMHO worth considering:

1. GSList is not a defined GType (not even a boxed one) so if we want to expose it in the future as a property we will have to do it as a pointer. If we use gchar** we have GStrv for properties, which is a GBoxed type representing a NULL-terminated array of gchar*.

2. I was reviewing different APIs from different GNOME libraries and I found out that, whenever they wanted to expose a list of strings for a given class, they also used gchar** for that (using GStrv when exposing them as properties). This makes me think that it's what people would expect from us too, and so the way to go, for consistency with what the rest of libraries do.

3. In the same way I looked for what other APIs did to expose this kind of attributes, I looked for APIs using GSList/GList to export things, and found out that whenever they used those, it was always in a [transfer container] or [transfer full] scenario, but never in a [transfer none] case, which is what we want.

So, after much thinking I now think that it's better to store (cache) and expose those attributes in WebKitFileChooserRequest as gchar** instead of using GSList*. The only drawback I've found so far is that, as we don't have a way to handle this kind of elements with GOwnPtr _and_ we want to cache the structures for allowing the [transfer none] thing, I had no choice but defining those attributes as raw gchar** pointers and freeing them on finalize with g_strfreev.

Perhaps there's another way to do it, or perhaps we could even consider creating a new kind of smart pointer for GBoxed types (GBoxedPtr ?), but in any case it doesn't look to me like a stopper or something, although I'm probably not the best suited one to judge such a thing :-)

In order to help making a decision, I'll be attaching right away two new different patches (both incorporating all the latest suggestions in this bug): one implemented with GSList* and the other implemented with gchar** (my preferred so far)

Thanks!

PS: Yes, yes.. no documentation yet :P

[Mario Sanchez Prada]

Created attachment 127456 [details]
Patch Proposal + Unit Tests (GSList* version)

[Mario Sanchez Prada]

Created attachment 127457 [details]
Patch Proposal + Unit Tests (gchar** version)

It also includes a couple of extra bool attributes to help when retrieving the cached gchar* arrays

[Martin Robinson]

Comment on attachment 127457 [details]
Patch Proposal + Unit Tests (gchar** version)

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

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:170
> +    if (request->priv->mimeTypesChecked)
> +        return request->priv->mimeTypes;
> +
> +    request->priv->mimeTypesChecked = true;
> +
> +    WKRetainPtr<WKArrayRef> wkMimeTypes(AdoptWK, WKOpenPanelParametersCopyAcceptedMIMETypes(request->priv->wkParameters.get()));
> +    size_t numOfMimeTypes = WKArrayGetSize(wkMimeTypes.get());
> +    if (!numOfMimeTypes)
> +        return 0;
> +

If you return an empty NULL terminated gchar** here it's both more consistent, avoids forcing null check in the client and you can get rid of request->priv->mimeTypesChecked. At the very least, there's no harm in it and it makes the code simpler.

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:190
> +    WKRetainPtr<WKArrayRef> wkMimeTypes(AdoptWK, WKOpenPanelParametersCopyAcceptedMIMETypes(request->priv->wkParameters.get()));
> +    size_t numOfMimeTypes = WKArrayGetSize(wkMimeTypes.get());
> +    if (!numOfMimeTypes)
> +        return 0;

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:227
> +    if (request->priv->selectedURIsChecked)
> +        return request->priv->selectedURIs;
> +
> +    request->priv->selectedURIsChecked = true;
> +
> +    Vector<String> selectedFileNames = toImpl(request->priv->wkParameters.get())->selectedFileNames();
> +    size_t numOfURIs = selectedFileNames.size();
> +    if (!numOfURIs)
> +        return 0;

Ditto.

[Carlos Garcia Campos]

(In reply to comment #25)
> (From update of attachment 127457 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127457&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:170
> > +    if (request->priv->mimeTypesChecked)
> > +        return request->priv->mimeTypes;
> > +
> > +    request->priv->mimeTypesChecked = true;
> > +
> > +    WKRetainPtr<WKArrayRef> wkMimeTypes(AdoptWK, WKOpenPanelParametersCopyAcceptedMIMETypes(request->priv->wkParameters.get()));
> > +    size_t numOfMimeTypes = WKArrayGetSize(wkMimeTypes.get());
> > +    if (!numOfMimeTypes)
> > +        return 0;
> > +
> 
> If you return an empty NULL terminated gchar** here it's both more consistent, avoids forcing null check in the client and you can get rid of request->priv->mimeTypesChecked. At the very least, there's no harm in it and it makes the code simpler.

Consistent with what? Not with GNOME APIs

[Carlos Garcia Campos]

(In reply to comment #22)
> Today, I was thinking again quite deeply about the implications of representing the list of mime types and selected files for the request as either a GSList* or a gchar**, and surpsingly I reached the same conclusion than Carlos yesterday: contrary to what I thought, using gchar** everywhere seems like the way to go.
> 
> Reasons for this are several and of course arguable, but IMHO worth considering:
> 
> 1. GSList is not a defined GType (not even a boxed one) so if we want to expose it in the future as a property we will have to do it as a pointer. If we use gchar** we have GStrv for properties, which is a GBoxed type representing a NULL-terminated array of gchar*.

Yes.

> 2. I was reviewing different APIs from different GNOME libraries and I found out that, whenever they wanted to expose a list of strings for a given class, they also used gchar** for that (using GStrv when exposing them as properties). This makes me think that it's what people would expect from us too, and so the way to go, for consistency with what the rest of libraries do.

Take a look at gtk_about_dialog_get_authors(), gtk_about_dialog_get_artists(), gtk_about_dialog_get_documenters(), etc. They are a good example of doing exactly what we want. They return a const gchar * const * [array zero-terminated=1][transfer none].

> 3. In the same way I looked for what other APIs did to expose this kind of attributes, I looked for APIs using GSList/GList to export things, and found out that whenever they used those, it was always in a [transfer container] or [transfer full] scenario, but never in a [transfer none] case, which is what we want.

Right

> So, after much thinking I now think that it's better to store (cache) and expose those attributes in WebKitFileChooserRequest as gchar** instead of using GSList*. The only drawback I've found so far is that, as we don't have a way to handle this kind of elements with GOwnPtr _and_ we want to cache the structures for allowing the [transfer none] thing, I had no choice but defining those attributes as raw gchar** pointers and freeing them on finalize with g_strfreev.
>
> Perhaps there's another way to do it, or perhaps we could even consider creating a new kind of smart pointer for GBoxed types (GBoxedPtr ?), but in any case it doesn't look to me like a stopper or something, although I'm probably not the best suited one to judge such a thing :-)

There's an easier way. You can use a GPtrArray internally and return its data pointer. GPtrArray is refcounted now, so it's easy to add a GRefPtr for it.

[Martin Robinson]

(In reply to comment #26)

> > If you return an empty NULL terminated gchar** here it's both more consistent, avoids forcing null check in the client and you can get rid of request->priv->mimeTypesChecked. At the very least, there's no harm in it and it makes the code simpler.
> 
> Consistent with what? Not with GNOME APIs

Self consistent. Instead of returning an array sometimes and NULL others, it would return an array always.

[Mario Sanchez Prada]

(In reply to comment #28)
> (In reply to comment #26)
> 
> > > If you return an empty NULL terminated gchar** here it's both more consistent, avoids forcing null check in the client and you can get rid of request->priv->mimeTypesChecked. At the very least, there's no harm in it and it makes the code simpler.
> > 
> > Consistent with what? Not with GNOME APIs
> 
> Self consistent. Instead of returning an array sometimes and NULL others, it would return an array always.

Hmm... not sure, I'm with Carlos here: I think the common agreement in GNOME APIs is returning an array only when there's something to return, otherwise return NULL is fine.

Also there's and advantage in the side of the client: if you get NULL, you don't need to do anything else (e.g. freeing the empty array), which is handy for instance for early returns.

[Carlos Garcia Campos]

(In reply to comment #28)
> (In reply to comment #26)
> 
> > > If you return an empty NULL terminated gchar** here it's both more consistent, avoids forcing null check in the client and you can get rid of request->priv->mimeTypesChecked. At the very least, there's no harm in it and it makes the code simpler.
> > 
> > Consistent with what? Not with GNOME APIs
> 
> Self consistent. Instead of returning an array sometimes and NULL others, it would return an array always.

Ok, then let's be self consistent and with GNOME APIs and return NULL instead of an empty array.

[Mario Sanchez Prada]

Created attachment 127596 [details]
Patch Proposal + Unit Tests

Hi again,

After struggling today with a couple of issues related with encodings, URIs and such... I'm attaching now a new version of the patch (based in the gchar** version, since I understand we agree on going down that road), incorporating the following changes:

* Expose new read-only property 'allows-multiple-selection', which I forgot to add yesterday.

*As per Carlos's suggestion, use GRefPtr<GPtrArray> for caching the list of strings in the WebKitFileChooserRequest object. As expected, I needed to add a new handler for GPtrArray* pointers in GRefPtr.[h|cpp] (contained in this patch), but it's definitely better than manually handling the memory cached in those former gchar** attributes.
  
* Fixed a bug related to the encoding of files: if a selected file included characters in it path needing escaping, the value returned by _get_selected_uris() was wrong (escaped twice!) and therefore the file chooser dialog could not pre-selecte them. The problem seemed to be that I wrongly assumed that WebCore's FileChooser understood URIs insted of filepaths, and that was not right: what it understands is paths escaped with filenameToString() (see WebCore/platform/FileSystem.h), which varies depending on the platform, prefixed with the 'file://' string, so that's why I misunderstood it accepted URIs (since it was matching our situation _when choosing files_).

However, whe retrieving the selected files from WebCore it turns out that it returns those escaped paths without the 'file://' prefix, which needs to be passed through fileSystemRepresentation() (see WebCore/platform/FileSystem.h) before being actually usable by the file chooser dialog, through gtk_file_chooser_set_filename(), as it's not an URI.

So, wrapping up this last issue, what I did to fix it is as follows:

   - Change the WebKitFileChooserRequest's API to handle 'files' instead of 'uris':
       - It will call internally to filenameToString() and filenameSystemRepresentation() as needed.
       - It will prepend the 'file://' prefix when choosing files, if not already set, so the selection actually happens in WebCore

   - In the WebView's default handler, change the URIs related functions in favor of those FILEs related ones:
       - gtk_file_chooser_get_filenames() instead of gtk_file_chooser_get_uris()
       - gtk_file_chooser_select_filename() instead of gtk_file_chooser_select_uri()

   - Of course, rename all variable names, parameters, properties.... from *uri* to *file* :-)

This shouldn't be an issue since, after all, WebCore's FileChooser understands local files only. If that changes in the future, it will be a matter of expanding the API to handle URIs as well (as in the case of GtkFileChooser).

Last, I haven't addressed yet the issues brought up by Martin since I understand it probably needs some additional discussion. Also, I haven't written documentation yet (promise I'll do next week!) due to having spent more time than what I thought today on fixing this issues, but setting the r? flag anyway since I understand the patch is already mature enough to ask for review over it :-)

Thanks!

[Carlos Garcia Campos]

Comment on attachment 127596 [details]
Patch Proposal + Unit Tests

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

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:178
> +    request->priv->mimeTypesChecked = true;

I'm not sure we need this check, if the array len > 0 then return  the array, otherwise just check WKOpenPanelParametersCopyAcceptedMIMETypes again, this method is very unlikely to be called more than once.

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:227
> +        String fileString = filenameToString(files[i]);
> +        if (!fileString.startsWith("file://", false))
> +            fileString.insert("file://", 0);

GRefPtr<GFile> filename = adoptGRef(g_file_new_for_path(fileString.utf8().data()));
GOwnPtr<char> uri(g_file_get_uri(filename.get());
WKRetainPtr<WKURLRef> wkURL(AdoptWK, WKURLCreateWithUTF8CString(uri.get()));

Have you tried this?

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:243
> +    request->priv->selectedFilesChecked = true;

Ditto

[Mario Sanchez Prada]

(In reply to comment #32)
> (From update of attachment 127596 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127596&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:178
> > +    request->priv->mimeTypesChecked = true;
> 
> I'm not sure we need this check, if the array len > 0 then return  the array, otherwise just check WKOpenPanelParametersCopyAcceptedMIMETypes again, this method is very unlikely to be called more than once.

Avoiding that extra check of WKOpenPanelParametersCopyAcceptedMIMETypes is the reason why I added that bool parameter. However, I agree with you it's unlikely to happen and so it might be better just to remove it. Will do in a follow-up patch.

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:227
> > +        String fileString = filenameToString(files[i]);
> > +        if (!fileString.startsWith("file://", false))
> > +            fileString.insert("file://", 0);
> 
> GRefPtr<GFile> filename = adoptGRef(g_file_new_for_path(fileString.utf8().data()));
> GOwnPtr<char> uri(g_file_get_uri(filename.get());
> WKRetainPtr<WKURLRef> wkURL(AdoptWK, WKURLCreateWithUTF8CString(uri.get()));
> 
> Have you tried this?

Yes, but at some point I thought it could perhaps be better to do the work explictly instead of using g_file_get_uri (or g_filename_to_uri), don't ask why... :-)

Anyway, all things considered, I think using your suggestion is good enough and probably cleaner, so let's go for it.

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:243
> > +    request->priv->selectedFilesChecked = true;
> 
> Ditto

Will change in a follow-up patch

[Mario Sanchez Prada]

One quick consideration before uploading my next patch...

I was now documenting all the code and realized of something looking at the API:

WEBKIT_API void
webkit_file_chooser_request_choose_files(WebKitFileChooserRequest *request, const gchar * const      *files);

WEBKIT_API const gchar * const *
webkit_file_chooser_request_get_selected_files(WebKitFileChooserRequest *request);


Shouldn't it be better to name the first function webkit_file_chooser_request_select_files()? It would be definitely more consistent with webkit_file_chooser_request_get_selected_files() and with the WebKitFileChooserRequest:selected-files properties indeed.

I will change that in a follow up patch unless someone says the contrary.

[Carlos Garcia Campos]

(In reply to comment #34)
> One quick consideration before uploading my next patch...
> 
> I was now documenting all the code and realized of something looking at the API:
> 
> WEBKIT_API void
> webkit_file_chooser_request_choose_files(WebKitFileChooserRequest *request, const gchar * const      *files);
> 
> WEBKIT_API const gchar * const *
> webkit_file_chooser_request_get_selected_files(WebKitFileChooserRequest *request);
> 
> 
> Shouldn't it be better to name the first function webkit_file_chooser_request_select_files()? It would be definitely more consistent with webkit_file_chooser_request_get_selected_files() and with the WebKitFileChooserRequest:selected-files properties indeed.

Does choose_files modify selected_files? In that case it makes sense.

> I will change that in a follow up patch unless someone says the contrary.

[Mario Sanchez Prada]

(In reply to comment #35)
> [...]
> > Shouldn't it be better to name the first function webkit_file_chooser_request_select_files()? It would be definitely more consistent with webkit_file_chooser_request_get_selected_files() and with the WebKitFileChooserRequest:selected-files properties indeed.
> 
> Does choose_files modify selected_files? In that case it makes sense.

Hmm... good point. The truth is that when you call webkit_file_chooser_request_choose_files() the selection changes _but only in WebCore_, in WebCore::FileChooser, but not in the request (in the API level), which is a read-only object that will be disposed when the whole process finishes.

So, that means that if we renamed the function as I proposed, and then called webkit_file_chooser_request_select_files() over a _non-NULL_ list of files, then webkit_file_chooser_request_get_selected_files() right afterwads, we would still be receiving NULL, as the selection happened in WebCore, not in the request.  And I think this might be confusing...

So, perhaps it's better to keep the 'choose' name in the API, to differentiate from what get_selected_files() returns: the list of files already selected in the _previous_ request, not in the current one.

> > I will change that in a follow up patch unless someone says the contrary.

Won't do at the end. Thanks for the feedback.

[Mario Sanchez Prada]

Created attachment 127815 [details]
Patch Proposal + Unit Tests + Docs

Attaching the first complete patch, featuring all the missing bits (e.g. gtk-doc documentation + gobject-introspection annotations) as well as addressing the last issues pointed out in this bug.

Hence, now officially asking for review over it.

[Carlos Garcia Campos]

Comment on attachment 127815 [details]
Patch Proposal + Unit Tests + Docs

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

Looks great!

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:200
> + * Returns: #TRUE if the file chooser should allow selecting multiple files or #FALSE otherwise.

Use %TRUE %FALSE instead of #TRUE #FALSE.

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:219
> + * Returns: (array zero-terminated=1) (transfer none): a
> + * NULL-terminated array of strings if a list of accepted MIME types
> + * is defined or NULL otherwise. This array and its contents are owned
> + * by WebKitGTK+ and should not be modified or freed.

%NULL

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:251
> + * MIME types is defined or NULL otherwise. The returned object is

%NULL

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:265
> +    request->priv->filter = gtk_file_filter_new();

adoptGRef(gtk_file_filter_new());

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:313
> + * associated with the request or NULL otherwise. This array and its

%NULL

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:323
> +    Vector<String> selectedFileNames = toImpl(request->priv->wkParameters.get())->selectedFileNames();

We are not going to modify the Vector, would it be possible to get a const reference?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:179
> +        GOwnPtr<GSList> filesList(gtk_file_chooser_get_filenames(GTK_FILE_CHOOSER(dialog)));
> +        guint numOfFiles = g_slist_length(filesList.get());
> +
> +        const gchar** filesArray = g_new0(const gchar*, numOfFiles + 1);
> +        GSList* file = filesList.get();
> +        for (guint i = 0; i < numOfFiles; i++) {
> +            filesArray[i] = static_cast<const gchar*>(file->data);
> +            file = g_slist_next(file);
> +        }
> +
> +        webkit_file_chooser_request_choose_files(adoptedRequest.get(), const_cast<const gchar* const*>(filesArray));
> +        g_free(filesArray);

I think this would be simpler using a GPtrArray

GRefPtr<GPtrArray> filesArray = adoptGRef(g_ptr_array_new());
for (GSList* l = filesList.get(); l; l = g_slist_next(l))
    g_ptr_array_add(filesArray.get(), l->data);
webkit_file_chooser_request_choose_files(adoptedRequest.get(), reinterpret_cast<gchar**>(filesArray->pdata));

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:200
> +    GRefPtr<GtkFileFilter> filter(webkit_file_chooser_request_get_mime_types_filter(request));

You don't need a GRefPtr in this case, since get_mime_types_filter returns a transfer none, you know the filter will be valid while the request is alive.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:206
> +    if (selectedFiles && selectedFiles[0] && *selectedFiles[0])

I think we should ignore empty files when adding items to the selected files array, so that the user doesn't need to check this.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:858
> +void webkitWebViewMakeFileChooserRequest(WebKitWebView* webView, WebKitFileChooserRequest* request)

webkitWebViewRunFileChooser()

[Mario Sanchez Prada]

Created attachment 127870 [details]
Patch Proposal + Unit Tests + Docs

(In reply to comment #38)
> [...]
> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:200
> > + * Returns: #TRUE if the file chooser should allow selecting multiple files or #FALSE otherwise.
> 
> Use %TRUE %FALSE instead of #TRUE #FALSE.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:219
> > + * Returns: (array zero-terminated=1) (transfer none): a
> > + * NULL-terminated array of strings if a list of accepted MIME types
> > + * is defined or NULL otherwise. This array and its contents are owned
> > + * by WebKitGTK+ and should not be modified or freed.
> 
> %NULL

Fixed.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:251
> > + * MIME types is defined or NULL otherwise. The returned object is
> 
> %NULL

Fixed.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:265
> > +    request->priv->filter = gtk_file_filter_new();
> 
> adoptGRef(gtk_file_filter_new());

Not possible. Since GtkFileFilter derives from GInitiallyUnowned, gtk_file_filter_new() returns a floating object, so we need to get g_object_ref_sink() not to fail when calling the derefPtr method (right after finishing the assignment). Calling adoptGRef avoids calling g_object_ref_sink and so it's not possible in this case.

I put a comment explaining the situation, right before that line.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:313
> > + * associated with the request or NULL otherwise. This array and its
> 
> %NULL

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:323
> > +    Vector<String> selectedFileNames = toImpl(request->priv->wkParameters.get())->selectedFileNames();
> 
> We are not going to modify the Vector, would it be possible to get a const reference?

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:179
> > +        GOwnPtr<GSList> filesList(gtk_file_chooser_get_filenames(GTK_FILE_CHOOSER(dialog)));
> > +        guint numOfFiles = g_slist_length(filesList.get());
> > +
> > +        const gchar** filesArray = g_new0(const gchar*, numOfFiles + 1);
> > +        GSList* file = filesList.get();
> > +        for (guint i = 0; i < numOfFiles; i++) {
> > +            filesArray[i] = static_cast<const gchar*>(file->data);
> > +            file = g_slist_next(file);
> > +        }
> > +
> > +        webkit_file_chooser_request_choose_files(adoptedRequest.get(), const_cast<const gchar* const*>(filesArray));
> > +        g_free(filesArray);
> 
> I think this would be simpler using a GPtrArray
> 
> GRefPtr<GPtrArray> filesArray = adoptGRef(g_ptr_array_new());
> for (GSList* l = filesList.get(); l; l = g_slist_next(l))
>     g_ptr_array_add(filesArray.get(), l->data);
> webkit_file_chooser_request_choose_files(adoptedRequest.get(), reinterpret_cast<gchar**>(filesArray->pdata));

You lack a g_ptr_array_add(filesArray.get(), 0); right after the loop (webkit_file_chooser_request_choose_files expects a NULL-terminated array of strings), but otherwise I agree with you.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:200
> > +    GRefPtr<GtkFileFilter> filter(webkit_file_chooser_request_get_mime_types_filter(request));
> 
> You don't need a GRefPtr in this case, since get_mime_types_filter returns a transfer none, you know the filter will be valid while the request is alive.

You're right. Fixed.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:206
> > +    if (selectedFiles && selectedFiles[0] && *selectedFiles[0])
> 
> I think we should ignore empty files when adding items to the selected files array, so that the user doesn't need to check this.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:858
> > +void webkitWebViewMakeFileChooserRequest(WebKitWebView* webView, WebKitFileChooserRequest* request)
> 
> webkitWebViewRunFileChooser()

I renamed it to webkitWebViewRunFileChooserRequest instead, since webkitWebViewRunFileChooser was a function name already in use for the default handler.

[Carlos Garcia Campos]

(In reply to comment #39)
> Created an attachment (id=127870) [details]
> Patch Proposal + Unit Tests + Docs
> > > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:265
> > > +    request->priv->filter = gtk_file_filter_new();
> > 
> > adoptGRef(gtk_file_filter_new());
> 
> Not possible. Since GtkFileFilter derives from GInitiallyUnowned, gtk_file_filter_new() returns a floating object, so we need to get g_object_ref_sink() not to fail when calling the derefPtr method (right after finishing the assignment). Calling adoptGRef avoids calling g_object_ref_sink and so it's not possible in this case.

Ah right, I did it again, sorry. I had already asked you the same thing and you had already explained this to me, sorry for the noise :-P

> I put a comment explaining the situation, right before that line.

Please, to make sure I won't ask you the same thing again :-D

> > I think this would be simpler using a GPtrArray
> > 
> > GRefPtr<GPtrArray> filesArray = adoptGRef(g_ptr_array_new());
> > for (GSList* l = filesList.get(); l; l = g_slist_next(l))
> >     g_ptr_array_add(filesArray.get(), l->data);
> > webkit_file_chooser_request_choose_files(adoptedRequest.get(), reinterpret_cast<gchar**>(filesArray->pdata));
> 
> You lack a g_ptr_array_add(filesArray.get(), 0); right after the loop (webkit_file_chooser_request_choose_files expects a NULL-terminated array of strings), but otherwise I agree with you.

Oops, right :-P

> > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:858
> > > +void webkitWebViewMakeFileChooserRequest(WebKitWebView* webView, WebKitFileChooserRequest* request)
> > 
> > webkitWebViewRunFileChooser()
> 
> I renamed it to webkitWebViewRunFileChooserRequest instead, since webkitWebViewRunFileChooser was a function name already in use for the default handler.

Perfect!

[Mario Sanchez Prada]

Thank you Carlos for the informal (yet thorough and extremely helpful) review. I understand from your last comment that the patch is already looking fine, so it looks like it's now the moment to ask for two different official reviews over it, according to [1].

So, any reviewer willing to take a look to this? :-)

Thank you!

[1] http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

(In reply to comment #41)
> Thank you Carlos for the informal (yet thorough and extremely helpful) review.

No problem, great work!

> I understand from your last comment that the patch is already looking fine, so it looks like it's now the moment to ask for two different official reviews over it, according to [1].

No, one review is enough for now until the first stable release of WebKit2. The patch looks good to me, but I haven't looked at the documentation in detail, Martin will do it when officially reviewing it.

[Martin Robinson]

Comment on attachment 127870 [details]
Patch Proposal + Unit Tests + Docs

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

Looks good! I have one real concern about the cached value of selectedFiles below. I won't r- this, because without that there are no blocking issues.

> Source/JavaScriptCore/ChangeLog:8
> +        Handle the GSList type in GOwnPtr, by calling g_ptr_array_ref and

This line seems inaccurate. I think what you mean here is:

Handle the GPtrArray type in GRefPtr...

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:54
> + * In case the client application do not wish to handle this signal,

Nit: do not -> does not

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:197
> + * which depends on the HTML Input element having a 'multiple'

Nit. Input -> input

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:214
> + * Get the list of "media types" the file chooser dialog should
> + * handle, in the format specified in RFC 2046. Its contents depend on
> + * the value of the 'accept' attribute for HTML Input elements.

Nice doc.

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:219
> + * Returns: (array zero-terminated=1) (transfer none): a
> + * %NULL-terminated array of strings if a list of accepted MIME types
> + * is defined or %NULL otherwise. This array and its contents are owned
> + * by WebKitGTK+ and should not be modified or freed.

Perhaps it would be good to mention specifically that if there are no mime-types listed, all of them should be accepted.

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:242
> +    return reinterpret_cast<gchar**>(request->priv->mimeTypes->pdata);

Are you sure you cannot use a static_cast here?

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:308
> +    WKOpenPanelResultListenerChooseFiles(request->priv->wkListener.get(), wkChosenFiles.get());
> +    request->priv->handledRequest = true;

Should you also clear priv->selectedFiles here so that if you call webkit_file_chooser_request_get_selected_files, it's up to date?

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:318
> + * if any, in further requests over the same HTML Input Element.

Nit: Input -> input

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:187
> +    GtkWidget* dialog = gtk_file_chooser_dialog_new(allowsMultipleSelection ? _("Select Files for Upload") : _("Select File for Upload"),

I think you should omit the phrase "for Upload" here as this file may not necessarily be uploaded.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:195
> +    GtkFileFilter* filter = webkit_file_chooser_request_get_mime_types_filter(request);
> +    if (filter)

This can be expressed as:
if (GtkFileFilter* filter = webkit_file_chooser_request_get_mime_types_filter(request))
        gtk_file_chooser_set_filter(GTK_FILE_CHOOSER(dialog), filter);

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:201
> +    const gchar* const* selectedFiles = webkit_file_chooser_request_get_selected_files(request);
> +    if (selectedFiles)
> +        gtk_file_chooser_select_filename(GTK_FILE_CHOOSER(dialog), selectedFiles[0]);

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.cpp:183
> +void WebViewTest::mouseButtonClick(int x, int y, unsigned int button, unsigned int mouseModifiers)

It's better to name these verb first: clickMouseButton

> Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.cpp:189
> +void WebViewTest::mouseButtonEvent(GdkEventType eventType, int x, int y, unsigned int button, unsigned int mouseModifiers)

executeMouseEvent

[Carlos Garcia Campos]

Comment on attachment 127870 [details]
Patch Proposal + Unit Tests + Docs

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

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:157
> +     * "media types" the file chooser dialog should handle. See

why media types?

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:212
> + * Get the list of "media types" the file chooser dialog should

I guess this is "mime types", not "media types"

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:250
> + * up with the list of media types as returned by

media types again?

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:251
> + * webkit_file_chooser_request_get_mime_types.

webkit_file_chooser_request_get_mime_types()

[Mario Sanchez Prada]

Comment on attachment 127870 [details]
Patch Proposal + Unit Tests + Docs

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

Thanks for the review, Carlos and Martin. I'm attaching a new patch soon addressing all the issues. In the meanwhile, see my comments below...

>> Source/JavaScriptCore/ChangeLog:8
>> +        Handle the GSList type in GOwnPtr, by calling g_ptr_array_ref and
> 
> This line seems inaccurate. I think what you mean here is:
> 
> Handle the GPtrArray type in GRefPtr...

You're obviously right :-)

>> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:54
>> + * In case the client application do not wish to handle this signal,
> 
> Nit: do not -> does not

Fixed

>> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:157
>> +     * "media types" the file chooser dialog should handle. See
> 
> why media types?

Becauses I'm using the format specified by RFC 2046, and they use the term "media types" there.

From http://www.ietf.org/rfc/rfc2046.txt:

   """
   Multipurpose Internet Mail Extensions
           (MIME) Part Two:
                 Media Types
                      [...]
   """

Anyway, in this specific part (documenting a GObject property to be used by a file chooser), and considering what I used in the @nick param, I guess it would be better to change it to 'mime types'.

>> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:197
>> + * which depends on the HTML Input element having a 'multiple'
> 
> Nit. Input -> input

Fixed

>> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:212
>> + * Get the list of "media types" the file chooser dialog should
> 
> I guess this is "mime types", not "media types"

See my comment above on the rationale behind using that term. Anyway, I agree it's better to change it for the API.

>> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:214
>> + * the value of the 'accept' attribute for HTML Input elements.
> 
> Nice doc.

Thank you!

However, as per the answers to Carlos's review, I will change this paragraph to say "MIME types" instead and  clarify it maps to "media types" in the RFC: 

  ...
    * Get the list of MIME types the file chooser dialog should handle,
    * in the format specified in RFC 2046 for "media types". Its contents
    * depend on the value of the 'accept' attribute for HTML input
    * elements.
   ...

>> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:219
>> + * by WebKitGTK+ and should not be modified or freed.
> 
> Perhaps it would be good to mention specifically that if there are no mime-types listed, all of them should be accepted.

Fixed.

>> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:242
>> +    return reinterpret_cast<gchar**>(request->priv->mimeTypes->pdata);
> 
> Are you sure you cannot use a static_cast here?

It seems I need reinterpret_cast. This is the error the compiler is spitting:
    error: invalid static_cast from type ‘void**’ to type ‘gchar** {aka char**}’

>> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:250
>> + * up with the list of media types as returned by
> 
> media types again?

Fixed.

>> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:251
>> + * webkit_file_chooser_request_get_mime_types.
> 
> webkit_file_chooser_request_get_mime_types()

Fixed.

>> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:308
>> +    request->priv->handledRequest = true;
> 
> Should you also clear priv->selectedFiles here so that if you call webkit_file_chooser_request_get_selected_files, it's up to date?

I'm not sure... I think we should not do anything in that regard, since the selected files for a given request is actually tied to the origin of the request (when it gets fired) not to the selection being done when handled by the application.

Thus, I would leave it this way, I think.

>> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:318
>> + * if any, in further requests over the same HTML Input Element.
> 
> Nit: Input -> input

Fixed.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:187
>> +    GtkWidget* dialog = gtk_file_chooser_dialog_new(allowsMultipleSelection ? _("Select Files for Upload") : _("Select File for Upload"),
> 
> I think you should omit the phrase "for Upload" here as this file may not necessarily be uploaded.

Fixed.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:195
>> +    if (filter)
> 
> This can be expressed as:
> if (GtkFileFilter* filter = webkit_file_chooser_request_get_mime_types_filter(request))
>         gtk_file_chooser_set_filter(GTK_FILE_CHOOSER(dialog), filter);

Fixed.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:201
>> +        gtk_file_chooser_select_filename(GTK_FILE_CHOOSER(dialog), selectedFiles[0]);
> 
> Ditto.

Fixed.

>> Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.cpp:183
>> +void WebViewTest::mouseButtonClick(int x, int y, unsigned int button, unsigned int mouseModifiers)
> 
> It's better to name these verb first: clickMouseButton

Fixed.

>> Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.cpp:189
>> +void WebViewTest::mouseButtonEvent(GdkEventType eventType, int x, int y, unsigned int button, unsigned int mouseModifiers)
> 
> executeMouseEvent

Fixed.

[Mario Sanchez Prada]

Created attachment 131037 [details]
Patch Proposal + Unit Tests + Docs

...and here is the new patch

[WebKit Review Bot]

Attachment 131037 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
Source/JavaScriptCore/wtf/gobject/GRefPtr.h:211:  The parameter name "ptr" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/gobject/GRefPtr.h:212:  The parameter name "ptr" adds no information, so it should be removed.  [readability/parameter_name] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.h"
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:220:  Use 0 instead of NULL.  [readability/null] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
Total errors found: 3 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mario Sanchez Prada]

(In reply to comment #47)
> Attachment 131037 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
> WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
> Source/JavaScriptCore/wtf/gobject/GRefPtr.h:211:  The parameter name "ptr" adds no information, so it should be removed.  [readability/parameter_name] [5]
> Source/JavaScriptCore/wtf/gobject/GRefPtr.h:212:  The parameter name "ptr" adds no information, so it should be removed.  [readability/parameter_name] [5]

I just did mimic the other declarations in that file. Should we change them all?

> WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.h"
> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:220:  Use 0 instead of NULL.  [readability/null] [5]

It's a sentinel. I'm not sure we can get rid of this NULL

[Martin Robinson]

(In reply to comment #48)
> (In reply to comment #47)
> > Attachment 131037 [details] [details] did not pass style-queue:
> > 
> > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
> > WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
> > Source/JavaScriptCore/wtf/gobject/GRefPtr.h:211:  The parameter name "ptr" adds no information, so it should be removed.  [readability/parameter_name] [5]
> > Source/JavaScriptCore/wtf/gobject/GRefPtr.h:212:  The parameter name "ptr" adds no information, so it should be removed.  [readability/parameter_name] [5]
> 
> I just did mimic the other declarations in that file. Should we change them all?

You can just change the new ones.

[Martin Robinson]

(In reply to comment #45)

> >> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:308
> >> +    request->priv->handledRequest = true;
> > 
> > Should you also clear priv->selectedFiles here so that if you call webkit_file_chooser_request_get_selected_files, it's up to date?
> 
> I'm not sure... I think we should not do anything in that regard, since the selected files for a given request is actually tied to the origin of the request (when it gets fired) not to the selection being done when handled by the application.

I took a closer look at that code and I see why it's like that. It's probably good to be super explicit in the documentation for webkit_file_chooser_request_get_selected_files that it returns the previously selected files. In fact, perhaps it's better to call it webkit_file_chooser_request_get_previously_selected_files. What do you think?

[Mario Sanchez Prada]

(In reply to comment #49)
> [...]
> > I just did mimic the other declarations in that file. Should we change them all?
> 
> You can just change the new ones.

Ok. I'll do.

(In reply to comment #50)
> (In reply to comment #45)
> 
> > >> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:308
> > >> +    request->priv->handledRequest = true;
> > > 
> > > Should you also clear priv->selectedFiles here so that if you call webkit_file_chooser_request_get_selected_files, it's up to date?
> > 
> > I'm not sure... I think we should not do anything in that regard, since the selected files for a given request 
> > is actually tied to the origin of the request (when it gets fired) not to the selection being done when 
> > handled by the application.
> 
> I took a closer look at that code and I see why it's like that. It's probably good to be super explicit in the 
> documentation for webkit_file_chooser_request_get_selected_files that it returns the previously selected 
> files. In fact, perhaps it's better to call it webkit_file_chooser_request_get_previously_selected_files. What 
> do you think?

I'm ok with being more explicit in the documentation about this, but I'm not sure about the renaming. After all, the webkit_file_chooser_request_*() functions are all about the current request being fired, and once the dialog is closed the request is already handled and so you should not care anymore about the details of the WebKitFileChooserRequest object.

In other words, a typical workflow here would be like this:

  1. User clicks on a 'upload' <input type='file'> button
  2. The runOpenPanel stuff is triggered from WebCore, resuñting on calling to WebUIClient::runOpenPanel() in the UI Process
  3. The signal 'run-file-chooser' signal is emitted from the WebKitWebView, with an instance of WebKitFileChooserRequest with the details of the event (selected files that might be already present, 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.

Hence, my point is that as nobody will ever should call any webkit_file_chooser_request_get_* after step [5], so I don't see the point on renaming the function that way. Also, I'm not 100% sure if there are other ways to the runOpenPanel function called with a list of 'selected files', other than having performed another selection previously (perhaps there's a way through JavaScript or some other mechanism), so calling it webkit_file_chooser_request_get_previously_selected_files() does not sound 'perfect' to me....

So, wrapping up, my vote goes for keeping the name as it is now (so it reflect that you want the list of selected files associated to the current request) _and_ make an extra effort to properly explain what is it about in the documentation.

What do you think?

[Mario Sanchez Prada]

Created attachment 131058 [details]
Patch Proposal + Unit Tests + Docs

Attaching a new patch fixing the coding style issue in GRefPtr.h and explaining better the meaning of 'selected files' in the documentation.

For ease of review, I paste here the diff between this patch related to that explanation:

@@ -168,7 +168,7 @@ static void webkit_file_chooser_request_class_init(WebKitFileChooserRequestClass
      * WebKitFileChooserRequest:selected-files:
      *
      * A %NULL-terminated array of strings containing the list of
-     * selected files currently associated to the request. See
+     * selected files associated to the current request. See
      * webkit_file_chooser_request_get_selected_files() for more details.
      */
     g_object_class_install_property(objectClass,
@@ -212,7 +212,9 @@ gboolean webkit_file_chooser_request_allows_multiple_selection(WebKitFileChooser
  * Get the list of MIME types the file chooser dialog should handle,
  * in the format specified in RFC 2046 for "media types". Its contents
  * depend on the value of the 'accept' attribute for HTML input
- * elements.
+ * elements. This function should normally be called before presenting
+ * the file chooser dialog to the user, to decide whether to allow the
+ * user to select multiple files at once or only one.
  *
  * Returns: (array zero-terminated=1) (transfer none): a
  * %NULL-terminated array of strings if a list of accepted MIME types
@@ -250,7 +252,10 @@ const gchar* const* webkit_file_chooser_request_get_mime_types(WebKitFileChooser
  *
  * Get the filter currently associated with the request, already set
  * up with the list of MIME types as returned by
- * webkit_file_chooser_request_get_mime_types().
+ * webkit_file_chooser_request_get_mime_types(). This function should
+ * normally be called before presenting the file chooser dialog to the
+ * user, to decide whether to apply a filter so the user would not be
+ * allowed to select files with other MIME types.
  *
  * Returns: (transfer none): a #GtkFileFilter if a list of accepted
  * MIME types is defined or %NULL otherwise. The returned object is
@@ -315,9 +320,16 @@ void webkit_file_chooser_request_choose_files(WebKitFileChooserRequest* request,
  * @request: a #WebKitFileChooserRequest
  *
  * Get the list of selected files currently associated to the
- * request. This list will normally be empty with the first
- * request and will contain the list of files previously selected,
- * if any, in further requests over the same HTML input Element.
+ * request. This list will normally be empty with the first request
+ * and will contain the list of files previously selected, if any, in
+ * further requests over the same HTML input Element. It's important
+ * to notice that this list won't change for the current request after
+ * making a new selection, since it does not represent the list of
+ * files being selected but those that might be associated to the
+ * request at the moment it gets triggered. Thus, this function should
+ * normally be called only before presenting the file chooser dialog
+ * to the user, probably to decide whether to perform some extra
+ * action, like getting those files in the list pre-selected.
  *
  * Returns: (array zero-terminated=1) (transfer none): a
  * %NULL-terminated array of strings if there are selected files

[WebKit Review Bot]

Attachment 131058 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.h"
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:220:  Use 0 instead of NULL.  [readability/null] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

(In reply to comment #52)

>   * Get the list of selected files currently associated to the
> - * request. This list will normally be empty with the first
> - * request and will contain the list of files previously selected,
> - * if any, in further requests over the same HTML input Element.
> + * request. This list will normally be empty with the first request
> + * and will contain the list of files previously selected, if any, in
> + * further requests over the same HTML input Element. It's important
> + * to notice that this list won't change for the current request after
> + * making a new selection, since it does not represent the list of
> + * files being selected but those that might be associated to the
> + * request at the moment it gets triggered. Thus, this function should
> + * normally be called only before presenting the file chooser dialog
> + * to the user, probably to decide whether to perform some extra
> + * action, like getting those files in the list pre-selected.

How about something a bit simpler. :)

Get the list of files previously selected when this request was
created. Typically for the first request, this list is empty. For instance, if files
are selected during one request and the user activates the input element
again, this method will return the files selected during the first round. Thus, calls
to webkit_file_chooser_request_choose_files() do not affect the return value
of this method.

Does it make sense to rename webkit_file_chooser_request_choose_files to webkit_file_chooser_request_select_files to make it consistent with webkit_file_chooser_request_get_selected_files?

[Mario Sanchez Prada]

(In reply to comment #54)
> [...]
> How about something a bit simpler. :)
> 
> Get the list of files previously selected when this request was
> created. Typically for the first request, this list is empty. For instance, if files
> are selected during one request and the user activates the input element
> again, this method will return the files selected during the first round. Thus, calls
> to webkit_file_chooser_request_choose_files() do not affect the return value
> of this method.

I can buy that :-)

> Does it make sense to rename webkit_file_chooser_request_choose_files to 
> webkit_file_chooser_request_select_files to make it consistent with 
> webkit_file_chooser_request_get_selected_files?

Hmm... I already thought of this in the past (see comment #34) but a comment from Carlos (comment #35) made me reconsider this option and finally opt for not renaming the function that way. See comment #36.

[Martin Robinson]

(In reply to comment #36)

> So, perhaps it's better to keep the 'choose' name in the API, to differentiate from what get_selected_files() returns: the list of files already selected in the _previous_ request, not in the current one.

Sorry, I missed this before. Consider that in one round you will "choose" some files and then in the next round they will have been "selected." I guess it's a minor point, but are you truly opposed to the following?

webkit_file_chooser_request_choose_files
webkit_file_chooser_request_get_previously_chosen_files

or

webkit_file_chooser_request_select_files
webkit_file_chooser_request_get_previously_selected_files

[Mario Sanchez Prada]

(In reply to comment #56)
> (In reply to comment #36)
> 
> > So, perhaps it's better to keep the 'choose' name in the API, to differentiate from what get_selected_files() returns: the list of files already selected in the _previous_ request, not in the current one.
> 
> Sorry, I missed this before. Consider that in one round you will "choose" some files and then in the next round they will have been "selected." I guess it's a minor point, but are you truly opposed to the following?
> 
> webkit_file_chooser_request_choose_files
> webkit_file_chooser_request_get_previously_chosen_files
> 
> or
> 
> webkit_file_chooser_request_select_files
> webkit_file_chooser_request_get_previously_selected_files

Hmm... No I think I can't say I'm "truly opposed" to any of those either, while at the same time I'm begininng to appreciate the consistency you're pursuing with those proposals :)

If I had to choose I think I kinda prefer the second proposal (select/previously_selected), but I don't have an strong opinion here either. I'd like to hear Carlos's opinion as well to see if he opposes to this or not (as he wrote comment #35 he might have a saying on this). If he agrees, I think we would have just reached consensus here, and I would propose a new patch with the result of the discussion.

[Carlos Garcia Campos]

(In reply to comment #57)
> (In reply to comment #56)
> > (In reply to comment #36)
> > 
> > > So, perhaps it's better to keep the 'choose' name in the API, to differentiate from what get_selected_files() returns: the list of files already selected in the _previous_ request, not in the current one.
> > 
> > Sorry, I missed this before. Consider that in one round you will "choose" some files and then in the next round they will have been "selected." I guess it's a minor point, but are you truly opposed to the following?
> > 
> > webkit_file_chooser_request_choose_files
> > webkit_file_chooser_request_get_previously_chosen_files
> > 
> > or
> > 
> > webkit_file_chooser_request_select_files
> > webkit_file_chooser_request_get_previously_selected_files
> 
> Hmm... No I think I can't say I'm "truly opposed" to any of those either, while at the same time I'm begininng to appreciate the consistency you're pursuing with those proposals :)
> 
> If I had to choose I think I kinda prefer the second proposal (select/previously_selected), but I don't have an strong opinion here either. I'd like to hear Carlos's opinion as well to see if he opposes to this or not (as he wrote comment #35 he might have a saying on this). If he agrees, I think we would have just reached consensus here, and I would propose a new patch with the result of the discussion.

selected files refers to files that appear selected in the file chooser, having the same meaning than gtk_file_chooser_select_*(), right? choosen files are the files finally choosen by the user, they can be different than selected files if the user changed the selection, or if the user cancelled the dialog. So they are different things, and that's why file chooser doesn't have a get_selected_* API, but get_*(). So, I still find more clear to use selected and choosen, they don't have to be consistent because they are not the same thing. Maybe we could use preselected to make it a bit clearer, but choosen files are not selected files.

[Mario Sanchez Prada]

(In reply to comment #58)
> [...]
> selected files refers to files that appear selected in the file chooser, having the same meaning than
> gtk_file_chooser_select_*(), right? choosen files are the files finally choosen by the user, they can
> be different than selected files if the user changed the selection, or if the user cancelled the dialog.
> So they are different things, and that's why file chooser doesn't have a get_selected_* API, but
> get_*(). So, I still find more clear to use selected and choosen, they don't have to be consistent
> because they are not the same thing. Maybe we could use preselected to make it a bit clearer, but
> choosen files are not selected files.

Good point. So that would leave us with leaving webkit_file_chooser_request_choose_files() and renaming the other one to something like webkit_file_chooser_request_get_previously_selected_files(), as Martin was suggesting in comment #50.

Up to this point I'm convinced that could be a good solution, so if Martin agrees I'll change the patch to reflect that.

[Carlos Garcia Campos]

Sorry, I had misunderstood what selected files were. Mario explained to me that selected files are the ones selected by the user on a previous request, so in that case selected files and choosen files are actually the same. As I said, if select_files modifies selected_files, then it makes sense to be consistent, and it does indeed. So, either choose_files()/get_chosen_files() or select_files()/get_selected_files() would be ok for me.

[Carlos Garcia Campos]

Comment on attachment 131058 [details]
Patch Proposal + Unit Tests + Docs

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

> Source/WebKit2/UIProcess/API/gtk/tests/WebViewTest.cpp:211
> +void WebViewTest::executeMouseButtonEvent(GdkEventType eventType, int x, int y, unsigned int button, unsigned int mouseModifiers)

I would call this doMouseButtonEvent, events are not executed.

[Mario Sanchez Prada]

Created attachment 134053 [details]
Patch Proposal + Unit Tests + Docs

After some time has passed, and in order to help moving things forward I'm now attaching a new patch with the following changes:

* Renamed functions to follow the second one of Martin's suggestions in comment #58, which I think Carlos is also in favor of right now (so do I):
     webkit_file_chooser_request_select_files
     webkit_file_chooser_request_get_previously_selected_files

* Renamed executeMouseButtonEvent to doMouseButtonEvent, as per Carlos's suggestion in comment #61 (it's a private function anyway)

* Patch rebased against the SVN trunk, which also meant that changes in good ol' JavascriptCore/wtf are now in WTF/wtf.

Hope this helps on easing the review :-)

[WebKit Review Bot]

Attachment 134053 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/go..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.h"
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:235:  Use 0 instead of NULL.  [readability/null] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gustavo Noronha (kov)]

Comment on attachment 134053 [details]
Patch Proposal + Unit Tests + Docs

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

80% through with the review, thought I'd submit it =P

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:40
> + * Whenever the user interacts with a <input type='file' /> HTML

a -> an for <input, I think

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:46
> + * specific platforms, which could prefer to use their own file
> + * chooser dialog), WebKit will fire the

I'd say just "in some cases" instead of talking about platforms and what they would prefer to do

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:50
> + * details from the request (e.g. if multiple selection should be

from the -> of the

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:52
> + * allowed) and to cancel the request, in case nothing was finally
> + * being selected.

s/was finally being selected/was selected/

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:202
> +gboolean webkit_file_chooser_request_allows_multiple_selection(WebKitFileChooserRequest* request)

I kinda like this name. I'll go ahead and say I didn't read any discussion on this other than the one on the bug, but why don't we use get_select_multiple(), like GtkFileChooser's? This would have a 'get' that's usually present in property getters (which this is), and would match GTK+'s counterpart, which would make it easier to find for API users.

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:255
> + * Get the filter currently associated with the request, already set
> + * up with the list of MIME types as returned by
> + * webkit_file_chooser_request_get_mime_types(). This function should

This is a bit confusing. It made me think you'd have to call get_mime_types(); Perhaps say just that this is a filter appropriate for handing directly to GtkFileChooser. The other method can still be mentioned, but like 'If you need the MIME types call blah()'

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:278
> +    // Do not use adoptGRef here, since we want to sink the floating
> +    // reference for the new instance of GtkFileFilter, so we make
> +    // sure we keep the ownership during the lifetime of the request.
> +    request->priv->filter = gtk_file_filter_new();

I may have missed some progress on GRefPtr, but how about having a specialization of GRefPtr like the one we added for ClutterActor, that sinks the ref?

http://trac.webkit.org/browser/trunk/Source/WebCore/platform/clutter/GRefPtrClutter.cpp#L27

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:308
> +        // Make sure the file path is presented as an URI (escaped
> +        // string, with the 'file://' prefix) to WebCore otherwise the
> +        // FileChooser won't actually choose it.

Would be great if we could use proper URIs - it should be possible to select files in non-local places, such as files in a different host through ssh.

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:319
> + * webkit_file_chooser_request_get_previously_selected_files:

previously is quite confusing I think, it sounds like it would allow you to get what files were selected in the past rather than what are set for the chooser currently, I would prefer it to just say get_selected_files(); I understand what is meant with previously, but I don't think it's important to have this distinction, it's pretty much implied for any property =)

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:329
> + * to notice that this list won't change for the current request after
> + * making a new selection, since it does not represent the list of
> + * files being selected but those that might be associated to the
> + * request at the moment it gets triggered. Thus, this function should

I don't understand this. Does it meant that if you call this will not return the current selection after webkit_file_chooser_request_select_files() is called? This explanation seems like it's causing more confusion than it's solving to me.

[Martin Robinson]

Comment on attachment 134053 [details]
Patch Proposal + Unit Tests + Docs

So after talking with Gustavo I think we agree that the API should be a little different here. Instead of select_files not updated selected_files, we think that it should simply replace request->priv->selectedFiles with the selected files. That would turn webkit_file_chooser_request_get_previously_selected_files into webkit_file_chooser_request_get_selected_files and make the documentation and API a lot simpler. 

Suggestion for the documentation for webkit_file_chooser_request_get_selected_files: Initially, the return value of this method contains any files selected in previous file chooser requests for this HTML input element. Once webkit_file_chooser_request_select_files, the value will reflect whatever file are given. 

Otherwise, the patch looks great. Feel free to commit with this change or reupload a new patch.

[Mario Sanchez Prada]

Created attachment 138070 [details]
Patch Proposal + Unit Tests + Docs

Thanks for the review to you all.

Now attaching a new patch addressing the issues you pointed out (or at least, most of them). Since I changed quite some things, I'd like to get a final review over the patch, even if Martin already gave it r+.

See some comments inline.

(In reply to comment #64)
> [...](From update of attachment 134053 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134053&action=review
> 
> 80% through with the review, thought I'd submit it =P
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:40
> > + * Whenever the user interacts with a <input type='file' /> HTML
> 
> a -> an for <input, I think

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:46
> > + * specific platforms, which could prefer to use their own file
> > + * chooser dialog), WebKit will fire the
> 
> I'd say just "in some cases" instead of talking about platforms and what they would prefer to do

Fixed.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:50
> > + * details from the request (e.g. if multiple selection should be
> 
> from the -> of the

Fixed.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:52
> > + * allowed) and to cancel the request, in case nothing was finally
> > + * being selected.
> 
> s/was finally being selected/was selected/

Fixed.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:202
> > +gboolean webkit_file_chooser_request_allows_multiple_selection(WebKitFileChooserRequest* request)
> 
> I kinda like this name. I'll go ahead and say I didn't read any discussion on this other than the one on the bug, but why don't we use get_select_multiple(), like GtkFileChooser's? This would have a 'get' that's usually present in property getters (which this is), and would match GTK+'s counterpart, which would make it easier to find for API users.

Fixed. I changed the method to *_get_select_multiple(), and also renamed the property to 'select-multiple', for consistency. I agree it's nicer to make it closer to GTK+'s counter part.

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:255
> > + * Get the filter currently associated with the request, already set
> > + * up with the list of MIME types as returned by
> > + * webkit_file_chooser_request_get_mime_types(). This function should
> 
> This is a bit confusing. It made me think you'd have to call get_mime_types(); Perhaps say just that this is a filter appropriate for handing directly to GtkFileChooser. The other method can still be mentioned, but like 'If you need the MIME types call blah()'

Fixed (hopefully is not confusing anymore).

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:278
> > +    // Do not use adoptGRef here, since we want to sink the floating
> > +    // reference for the new instance of GtkFileFilter, so we make
> > +    // sure we keep the ownership during the lifetime of the request.
> > +    request->priv->filter = gtk_file_filter_new();
> 
> I may have missed some progress on GRefPtr, but how about having a specialization of GRefPtr like the one we added for ClutterActor, that sinks the ref?
> 
> http://trac.webkit.org/browser/trunk/Source/WebCore/platform/clutter/GRefPtrClutter.cpp#L27

That's a good idea, I guess. But since it's an internal matter I would leave it for a different bug, if you don't mind, so we can now concentrate on landing this one.

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:308
> > +        // Make sure the file path is presented as an URI (escaped
> > +        // string, with the 'file://' prefix) to WebCore otherwise the
> > +        // FileChooser won't actually choose it.
> 
> Would be great if we could use proper URIs - it should be possible to select files in non-local places, such as files in a different host through ssh.

AFAICS, this limitation is not in the API we're defining but in WebCore, which doesn't accept any URI schema other than file:///. I agree it would be great to fix that too, but that would be also a different bug, IMHO.

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:319
> > + * webkit_file_chooser_request_get_previously_selected_files:
> 
> previously is quite confusing I think, it sounds like it would allow you to get what files were selected in the past rather than what are set for the chooser currently, I would prefer it to just say get_selected_files(); I understand what is meant with previously, but I don't think it's important to have this distinction, it's pretty much implied for any property =)

Then it's not confusing, since you're understanding it perfectly right, since that's how it works in that patch :-)

However, based on Martin's last comment, I changed things in this new patch to work as it seems it would be better: reflecting also the current selection right after choosing filenames in the dialog. Thus this is no longer an issue, I think.

Also, as per those changes, the method got renamed to webkit_file_chooser_request_get_selected_files().

> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:329
> > + * to notice that this list won't change for the current request after
> > + * making a new selection, since it does not represent the list of
> > + * files being selected but those that might be associated to the
> > + * request at the moment it gets triggered. Thus, this function should
> 
> I don't understand this. Does it meant that if you call this will not return the current selection after webkit_file_chooser_request_select_files() is called? This explanation seems like it's causing more confusion than it's solving to me.

You understood it perfectly right it seems :-)

Anyway, this is no longer like that in the current patch (see my previous comment).



(In reply to comment #65)
> (From update of attachment 134053 [details])
> So after talking with Gustavo I think we agree that the API should be a little different here. Instead of select_files not updated selected_files, we think that it should simply replace request->priv->selectedFiles with the selected files. That would turn webkit_file_chooser_request_get_previously_selected_files into webkit_file_chooser_request_get_selected_files and make the documentation and API a lot simpler. 

Fixed in this new patch.

> Suggestion for the documentation for webkit_file_chooser_request_get_selected_files: Initially, the return value of this method contains any files selected in previous file chooser requests for this HTML input element. Once webkit_file_chooser_request_select_files, the value will reflect whatever file are given. 

Sounds good. Fixed.

> Otherwise, the patch looks great. Feel free to commit with this change or reupload a new patch.

If you don't mind, I'd prefer to have a last review on the patch, to make sure both you and Gustavo (and anyone else) are ok with the new patch.

[Mario Sanchez Prada]

Created attachment 138072 [details]
Patch Proposal + Unit Tests + Docs

This is the good one

[WebKit Review Bot]

Attachment 138072 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/go..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.h"
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:239:  Use 0 instead of NULL.  [readability/null] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 138072 [details]
Patch Proposal + Unit Tests + Docs

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

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:336
> + * value will reflect whatever file are given.

whatever file are -> whatever files are

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:228
> +    static gboolean runFileChooser(WebKitWebView*, WebKitFileChooserRequest* request, UIClientTest* test)
> +    {
> +        test->assertObjectIsDeletedWhenTestFinishes(G_OBJECT(request));
> +        test->m_fileChooserRequest = request;
> +        g_main_loop_quit(test->m_mainLoop);
> +        return TRUE;
> +    }
> +

I think it's good to keep separate unrelated test infrastructure. The UIClientTest class is huge already and I think we should instead be trying to be making it smaller and more modular. When landing this, do you think you can create a subclass of UIClientTest called FileChooserTest? It will look something like this:

class FileChooserTest: public UIClientTest {
public:
    MAKE_GLIB_TEST_FIXTURE(UIClientTest);

    FileChooserTest()
    {
        g_signal_connect(m_webView, "run-file-chooser", G_CALLBACK(runFileChooser), this);
    }

     static gboolean runFileChooserCallback(WebKitWebView*, WebKitFileChooserRequest* request, UIClientTest* test)
    {
        test->runFileChooser(request);
        return TRUE;
    }

    void runFileChooser(WebKitFileChooserRequest* request)
    {
        assertObjectIsDeletedWhenTestFinishes(G_OBJECT(request));
        m_fileChooserRequest = request;
        g_main_loop_quit(test->m_mainLoop);
   }

     WebKitFileChooserRequest* clickMouseButtonAndWaitForFileChooserRequest(int x, int y)
    {
        clickMouseButton(x, y);
        g_main_loop_run(m_mainLoop);
        return m_fileChooserRequest.get();
    }

private:
    GRefPtr<WebKitFileChooserRequest> m_fileChooserRequest;
};

[Mario Sanchez Prada]

Committed r115627: <http://trac.webkit.org/changeset/115627>