Bug 70002 – [EFL]: Move from FileChooserSettings deprecatedAcceptType to acceptMIMETypes

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Joseph Pecoraro]

In r97336 and r97338 WebCore::FileChooserSettings deprecated its "acceptType" string
in favor of a Vector<String> of MIME types "acceptMIMETypes".

EFL should transition from the deprecated value to the new value. It looks to me as though
the string was exposed to EFL API "ewk_view_run_open_panel". It sounds possible to update
that from a char* to an Eina_List of strings.

WebCore::FileChooserSettings is a struct of parameters for a <input type="file">
File Upload Dialog. Its passed to ports via ChomeClient::runOpenPanel. The deprecated
string value was the unparsed "accept" attribute from the <input>. The new Vector
value is the parsed MIME types from that "accept" attribute. This way all ports
share the same parsing code for the different mime types listed in the "accept" attribute.

The changes:
<http://trac.webkit.org/changeset/97336>
<http://trac.webkit.org/changeset/97338>

Introduced in the following:
<http://webkit.org/b/69598> Pass Parsed Accept Attribute MIME Types to WebKit Clients

[Raphael Kubo da Costa (:rakuco)]

Created attachment 110884 [details]
Proposed patch

[Raphael Kubo da Costa (:rakuco)]

Hi, Joseph. There are no EFL reviewers, so it'd be nice if you could take a look at the patch yourself.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

(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!

[Raphael Kubo da Costa (:rakuco)]

(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.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 110898 [details]
Proposed patch, v2

[Joseph Pecoraro]

Comment on attachment 110898 [details]
Proposed patch, v2

Excellent, this looks much better. Thanks!

[Joseph Pecoraro]

(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+?

[Raphael Kubo da Costa (:rakuco)]

Yes, I was going to ask for a cq+ but you were faster ;) I haven't been nominated for a commit account yet.

[Joseph Pecoraro]

(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?

[Raphael Kubo da Costa (:rakuco)]

(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.

[WebKit Review Bot]

Comment on attachment 110898 [details]
Proposed patch, v2

Clearing flags on attachment: 110898

Committed r97421: <http://trac.webkit.org/changeset/97421>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.