[webkit-reviews] review requested: [Bug 78491] [GTK] Implement WebUIClient's runOpenPanel in WebKit2GTK+ : [Attachment 127596] Patch Proposal + Unit Tests

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <msanchez at igalia.com> has asked  for review:
Bug 78491: [GTK] Implement WebUIClient's runOpenPanel in WebKit2GTK+
https://bugs.webkit.org/show_bug.cgi?id=78491

Attachment 127596: Patch Proposal + Unit Tests
https://bugs.webkit.org/attachment.cgi?id=127596&action=review

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
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!