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

[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 138070: Patch Proposal + Unit Tests + Docs
https://bugs.webkit.org/attachment.cgi?id=138070&action=review

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