Summary: | [EFL]: Move from FileChooserSettings deprecatedAcceptType to acceptMIMETypes | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||
Component: | WebKit EFL | Assignee: | Raphael Kubo da Costa (:rakuco) <rakuco> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | gyuyoung.kim, joepeck, leandro, lucas.de.marchi, rakuco, ryuan.choi, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Joseph Pecoraro
2011-10-12 23:36:48 PDT
Created attachment 110884 [details]
Proposed patch
Hi, Joseph. There are no EFL reviewers, so it'd be nice if you could take a look at the patch yourself. Comment on attachment 110884 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=110884&action=review > Source/WebKit/efl/ewk/ewk_view.cpp:3265 > + * @param accept_types @c Eina_List of accepted MIME types as char pointers. The list and the items will be freed automatically. > * @selected_filenames List of files selected. > * > * @return @EINA_FALSE if user canceled file selection; @EINA_TRUE if confirmed. > */ > -Eina_Bool ewk_view_run_open_panel(Evas_Object* ewkView, Evas_Object* frame, Eina_Bool allowsMultipleFiles, const char* acceptTypes, Eina_List** selectedFilenames) > +Eina_Bool ewk_view_run_open_panel(Evas_Object* ewkView, Evas_Object* frame, Eina_Bool allowsMultipleFiles, const Vector<String>& acceptMIMETypes, Eina_List** selectedFilenames) I have a bunch of questions! Still r? until they are answered. The patch itself looks good though! The documentation and function seem at odds. I can't tell if the documentation is for the API's run_open_panel or for this ewk_view_run_open_panel. The parameter names "mostly" match the API function and not this function. Could you clarify that. I also found it interesting that you passed the Vector<String> into this function, instead of an Eina_List, and do the conversion here instead of in ChromeClientEfl. It looks like most of the ewk functions in this file take Eina* types and not WTF/WebCore types. I suppose it doesn't matter as long as the API calls have the right types but it would be nice to be consistent. One more consistency question. The rest of the file uses namespace prefixed types for WTF::Vector and WTF::String. You should probably follow suit there. (In reply to comment #2) > Hi, Joseph. There are no EFL reviewers, so it'd be nice if you could take a look > at the patch yourself. Sure! Thanks for the quick patches, the quick response is much appreciated! (In reply to comment #3) > The documentation and function seem at odds. I can't tell if the documentation is for the > API's run_open_panel or for this ewk_view_run_open_panel. The parameter names "mostly" > match the API function and not this function. Could you clarify that. Oops, I ended up confusing both too :) > I also found it interesting that you passed the Vector<String> into this function, instead of > an Eina_List, and do the conversion here instead of in ChromeClientEfl. It looks like most > of the ewk functions in this file take Eina* types and not WTF/WebCore types. I suppose > it doesn't matter as long as the API calls have the right types but it would be nice to be > consistent. ewk_view_run_open_panel is an internal function only visible to ewk and WebCoreSupport, so we can use WTF and other data types if we need to. As it is only the child object API that is C and written by third-parties, I chose to keep the vector as long as possible and defer the ugly WTF->EFL glue. > One more consistency question. The rest of the file uses namespace prefixed types for > WTF::Vector and WTF::String. You should probably follow suit there. Will do. Patch following in a second. Created attachment 110898 [details]
Proposed patch, v2
Comment on attachment 110898 [details]
Proposed patch, v2
Excellent, this looks much better. Thanks!
(In reply to comment #7) > (From update of attachment 110898 [details]) > Excellent, this looks much better. Thanks! I don't see you in committers.py, do you need me to cq+? Yes, I was going to ask for a cq+ but you were faster ;) I haven't been nominated for a commit account yet. (In reply to comment #9) > Yes, I was going to ask for a cq+ but you were faster ;) I haven't been nominated for a commit account yet. With ~50 patches landed you should be eligible at least for commit access! http://trac.webkit.org/search?q=kubo%40profusion.mobi&noquickjump=1&changeset=on Do you know offhand who typically reviews your patches? (In reply to comment #10) > Do you know offhand who typically reviews your patches? In general, tonikitoo, kenneth and rniwa. tkent and mrobinson have also reviewed some patches over time. Comment on attachment 110898 [details] Proposed patch, v2 Clearing flags on attachment: 110898 Committed r97421: <http://trac.webkit.org/changeset/97421> All reviewed patches have been landed. Closing bug. |