Bug 112339 – [WK2] Add C API to copy selected files from WebOpenPanelParameters.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Michael Brüning]

[WK2] Add C API to copy selected files from WebOpenPanelParameters.

[Michael Brüning]

Created attachment 193103 [details]
Patch

[Michael Brüning]

Anders, I cc'ed you as you have reviewed the patch that added the API for copying the accepted mime types. Could you please have a look?

[Jocelyn Turcotte]

Comment on attachment 193103 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/WKOpenPanelParameters.cpp:60
> +WKArrayRef WKOpenPanelParametersCopySelectedFileNames(WKOpenPanelParametersRef parametersRef)

This needs to be implemented in WebOpenPanelParameters, the API should just forward the call.
This will however ask us to adjust the GTK code to use the C API as well.

> Source/WebKit2/UIProcess/qt/QtWebPageUIClient.cpp:29
>  #include <WKAPICast.h>

I think that you removed the last toImpl(...) call using this header :)

[Michael Brüning]

Created attachment 193999 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 193999 [details]
Patch

LGTM

[Jocelyn Turcotte]

Comment on attachment 193999 [details]
Patch

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

> Source/WebKit2/Shared/WebOpenPanelParameters.h:52
> +    PassRefPtr<ImmutableArray> copySelectedFileNames() const;

I can't find any Web* classes with "Copy" in their method names, only in the WK* API layer.
I also think that this should replace the existing selectedFileNames() method. The GTK port is the only port left using selectedFileNames(), so we could adjust it to use the RefPtr<ImmutableArray> version, as it already does for acceptMIMETypes.

[Michael Brüning]

Created attachment 194016 [details]
Patch

[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

[Michael Brüning]

Created attachment 194026 [details]
Patch

Avoiding deep copy of the String vector.

[Carlos Garcia Campos]

Comment on attachment 194026 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:349
> -    const Vector<String> selectedFileNames = request->priv->parameters->selectedFileNames();
> -    size_t numOfFiles = selectedFileNames.size();
> +    RefPtr<ImmutableArray> selectedFileNames = request->priv->parameters->selectedFileNames();
> +    size_t numOfFiles = selectedFileNames->size();

Why duplicating the vector here? Can we just leave two methods, once that duplicates the array returning an Immutable array and another returning a const reference? The duplication is not needed here since we are already duplicating the array to create a GPtrArray

[Michael Brüning]

(In reply to comment #10)
> (From update of attachment 194026 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194026&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:349
> > -    const Vector<String> selectedFileNames = request->priv->parameters->selectedFileNames();
> > -    size_t numOfFiles = selectedFileNames.size();
> > +    RefPtr<ImmutableArray> selectedFileNames = request->priv->parameters->selectedFileNames();
> > +    size_t numOfFiles = selectedFileNames->size();
> 
> Why duplicating the vector here? Can we just leave two methods, once that duplicates the array returning an Immutable array and another returning a const reference? The duplication is not needed here since we are already duplicating the array to create a GPtrArray

I understand your concern here, but actually, the amount of duplication with the patch is the same as the one in the current implementation: 
Currently, selectedFileNames is in fact not returning a const reference, but rather creates a copy of the WebOpenPanelParameters::m_settings.selectedFiles Vector and the Strings in it.

The new version returns a reference to a Vector of WebStrings instead of Strings (just as the implementation of acceptMIMETypes does).

So with that in mind, I would propose to change the implementation for the sake of consistency with acceptMIMETypes and better integration with the C APIs. 

I would be very interested hearing in your opinion on this or if you see a better way of solving this and avoiding redundancy in the code.

[Carlos Garcia Campos]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 194026 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=194026&action=review
> > 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:349
> > > -    const Vector<String> selectedFileNames = request->priv->parameters->selectedFileNames();
> > > -    size_t numOfFiles = selectedFileNames.size();
> > > +    RefPtr<ImmutableArray> selectedFileNames = request->priv->parameters->selectedFileNames();
> > > +    size_t numOfFiles = selectedFileNames->size();
> > 
> > Why duplicating the vector here? Can we just leave two methods, once that duplicates the array returning an Immutable array and another returning a const reference? The duplication is not needed here since we are already duplicating the array to create a GPtrArray
> 
> I understand your concern here, but actually, the amount of duplication with the patch is the same as the one in the current implementation: 
> Currently, selectedFileNames is in fact not returning a const reference, but rather creates a copy of the WebOpenPanelParameters::m_settings.selectedFiles Vector and the Strings in it.

Ah, you are right, it returns a copy of the vector.

> The new version returns a reference to a Vector of WebStrings instead of Strings (just as the implementation of acceptMIMETypes does).
> 
> So with that in mind, I would propose to change the implementation for the sake of consistency with acceptMIMETypes and better integration with the C APIs. 
> 
> I would be very interested hearing in your opinion on this or if you see a better way of solving this and avoiding redundancy in the code.

Well, the only way I see to avoid the duplication would be to add a getter to return the WebCore::FileChooserSettings, similar to WebURLRequest/Response do. 
But that would actually be a different issue, so I'm fine with your patch, sorry for the noise.

[Jocelyn Turcotte]

Comment on attachment 194026 [details]
Patch

The Qt part looks good to me.

Anders, may I ask you to look at the WK2 part?

[Michael Brüning]

Benjamin, would you like to have a look?

[Michael Brüning]

Benjamin, Anders, any estimate on when you might be able to look at this?

[Andreas Kling]

Comment on attachment 194026 [details]
Patch

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

r=me

> Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:358
> +        String fileNameString = webFileName->string();
> +        if (fileNameString.isEmpty())
>              continue;

Do you need the String temporary here?

if (webFileName->isEmpty())
    continue;

[Michael Brüning]

(In reply to comment #16)
> (From update of attachment 194026 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194026&action=review
> 
> r=me
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp:358
> > +        String fileNameString = webFileName->string();
> > +        if (fileNameString.isEmpty())
> >              continue;
> 
> Do you need the String temporary here?
> 
> if (webFileName->isEmpty())
>     continue;
Good point, fileNameString is indeed not needed. Will land with this change.

[Michael Brüning]

Committed r148005: <http://trac.webkit.org/changeset/148005>