Summary: | [WK2] Add C API to copy selected files from WebOpenPanelParameters. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Brüning <michael.bruning> | ||||||||||
Component: | New Bugs | Assignee: | Michael Brüning <michael.bruning> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abecsi, andersca, benjamin, cgarcia, cmarcelo, gustavo, hausmann, jturcotte, menard, mrobinson, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Michael Brüning
2013-03-14 03:42:47 PDT
Created attachment 193103 [details]
Patch
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? 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 :) Created attachment 193999 [details]
Patch
Comment on attachment 193999 [details]
Patch
LGTM
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. Created attachment 194016 [details]
Patch
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 Created attachment 194026 [details]
Patch
Avoiding deep copy of the String vector.
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 (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. (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. Comment on attachment 194026 [details]
Patch
The Qt part looks good to me.
Anders, may I ask you to look at the WK2 part?
Benjamin, would you like to have a look? Benjamin, Anders, any estimate on when you might be able to look at this? 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; (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. Committed r148005: <http://trac.webkit.org/changeset/148005> |