Bug 91957 – [EFL] Add File Chooser API

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Kihong Kwon]

Currently on run_open_panel to open file dialog, there is no way to get attributes of FileChooserSettings except allowsMultipleFiles and acceptMIMETypes.
But we need to other attributes to open well designed file dialog, in addition we need to get the capture attribute to support HTML media capture.
Therefore I would like to add File Chooser Settings API to the EFL.

[Kihong Kwon]

Created attachment 153743 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 153743 [details]
Patch

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

I would like to know how many test cases can be covered by this patch.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:52
> +Eina_Bool ewk_file_chooser_settings_allows_directory_upload_get(const Ewk_File_Chooser_Settings *settings)

Nit : Move '*' to data type side.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:68
> +    for (unsigned i = 0; i < acceptMIMETypes.size(); i++) {

WebKit prefers to use ++i.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:82
> +    for (unsigned i = 0; i < acceptFileExtensions.size(); i++) {

ditto.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.h:32
> + * We can use these attributes by file chooser settings APIs.

I think "We can use these" is not proper API description.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.h:33
> + *

Nit : Unneeded line.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.h:114
> +EAPI const char *ewk_file_chooser_settings_capture_get(const Ewk_File_Chooser_Settings* s);

Nit : Move '*' to variable side.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings_private.h:29
> +void ewk_file_chooser_settings_free(Ewk_File_Chooser_Settings *o);

Nit : Do not abbreviation in internal functions.

[Kihong Kwon]

(In reply to comment #2)
> (From update of attachment 153743 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153743&action=review

Thank you. I will update patch. :)

[Kihong Kwon]

Created attachment 153747 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 153747 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:64
> +    EINA_SAFETY_ON_NULL_RETURN_VAL(settings, 0);

I think you need to mention this API can return NULL in API description.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:68
> +    for (unsigned i = 0; i < acceptMIMETypes.size(); ++i) {

It looks below code is more clear than this.

http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_intent.cpp#L107

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:78
> +    EINA_SAFETY_ON_NULL_RETURN_VAL(settings, 0);

ditto.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:92
> +    EINA_SAFETY_ON_NULL_RETURN_VAL(settings, 0);

ditto.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:118
> +        , eina_stringshare_add(settings->fileChooserSettings->capture.utf8().data()

Please adhere indentation.

See also below,
http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_intent.cpp#L93

[Kihong Kwon]

Created attachment 153765 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 153765 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:67
> +        mimetypes = eina_list_append(mimetypes, eina_stringshare_add((*it).utf8().data()));

I think it is good to use strdup(). Then, update API description.

See also : http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_intent.h#L50

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:79
> +        fileExtentions = eina_list_append(fileExtentions, eina_stringshare_add((*it).utf8().data()));

ditto.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:91
> +        files = eina_list_append(files, eina_stringshare_add((*it).utf8().data()));

ditto.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.h:110
> + * @return One of these string "camera", "camcorder", "microphone" or "filesystem" or @c NULL if there is not a proper file chooser settings scheme

Use lower case. s/One/one/g

[Kihong Kwon]

Created attachment 153770 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 153770 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.h:74
> + * @return list of accepted mimetypes or @c NULL if there is not a proper file chooser settings scheme

I think you need to add more description for Eina_List. Please refer below description.

http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_intent.h#L80

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.h:85
> + * @return list of accepted file extentions or @c NULL if there is not a proper file chooser settings scheme

ditto.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.h:99
> + * @return list of selected file names or @c NULL if there is not a proper file chooser settings scheme

ditto.

[Kihong Kwon]

Created attachment 153784 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 153784 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:108
> +

Remove unneeded line.

[Kihong Kwon]

Created attachment 153943 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 153943 [details]
Patch

Almost looks good to me now. But, It seems to me ewk_file_chooser_settings.h | .cpp are not proper file name. Because this files support to get file extension, mimetypes and selected file as well as APIs related to settings. So, I think ewk_file_chooser.h | .cpp is more better or ewk_file_chooser_request.h | cpp as GTK port.

[Kihong Kwon]

(In reply to comment #13)
> (From update of attachment 153943 [details])
> Almost looks good to me now. But, It seems to me ewk_file_chooser_settings.h | .cpp are not proper file name. Because this files support to get file extension, mimetypes and selected file as well as APIs related to settings. So, I think ewk_file_chooser.h | .cpp is more better or ewk_file_chooser_request.h | cpp as GTK port.

I agree.
I will change to ewk_file_chooser.
It could be used more general for the file chooser.

[Chris Dumez]

Comment on attachment 153943 [details]
Patch

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

Stupid question: How is the client support to use this API? Both the constructor and destructor are private and the patch does not introduce any where to retrieve the Ewk_File_Choose_Settings from somewhere else. In itself, the patch seems to add code but no real functionality. Am I missing something?

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:28
> +    const WebCore::FileChooserSettings& fileChooserSettings;

I'm not sure why we use a const reference here.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:30
> +    const char* capture;

Missing destructor with eina_stringshare_del() for capture.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:34
> +    _Ewk_File_Chooser_Settings(const WebCore::FileChooserSettings& settings , const char* captureString)

Extra space before comma.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:37
> +#else

I don't like duplicating the constructor with a #ifdef here.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:65
> +    Vector<WTF::String>::const_iterator it = settings->fileChooserSettings.acceptMIMETypes.begin();

As per coding style, you should avoid using iterators and use indexes instead.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:67
> +        mimetypes = eina_list_append(mimetypes, strdup((*it).utf8().data()));

I thought we prefered to use eina_stringshare in EFL port.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:78
> +    for (; it != settings->fileChooserSettings.acceptFileExtensions.end(); ++it)

Ditto (iterators vs indexes)

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:79
> +        fileExtentions = eina_list_append(fileExtentions, strdup((*it).utf8().data()));

Ditto (stringsharing)

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:90
> +    for (; it != settings->fileChooserSettings.selectedFiles.end(); ++it)

Ditto.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:91
> +        files = eina_list_append(files, strdup((*it).utf8().data()));

Ditto.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:109
> +    return new Ewk_File_Chooser_Settings(coreSettings, eina_stringshare_add(settings->fileChooserSettings->capture.utf8().data()));

"settings" -> "coreSettings" ?

Also since you get capture from coreSettings. I suggest having only 1 argument to the constructor and do the eina_stringshare_add() from inside the constructor implementation. This will also avoid duplicating the constructor with a #ifdef.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:118
> +    eina_stringshare_del(settings->capture);

Please move this to the destructor.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.h:43
> +typedef struct _Ewk_File_Chooser_Settings Ewk_File_Chooser_Settings;

Missing documentation.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.h:52
> + * @return @c EINA_TRUE on support multiple files or @c EINA_FALSE on not support

"on not support" -> "otherwise".

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.h:63
> + * @return @c EINA_TRUE on support directory upload or @c EINA_FALSE on not support

Ditto.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.h:107
> + * @return one of these string "camera", "camcorder", "microphone" 

If the values are predetermined, why not use an enumeration instead? That would be much better for the clients.

> Source/WebKit/efl/ewk/ewk_file_chooser_settings_private.h:23
> +// forward declarations

This comment is not really useful.

[Gyuyoung Kim]

Comment on attachment 153943 [details]
Patch

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

>> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:37
>> +#else
> 
> I don't like duplicating the constructor with a #ifdef here.

Yes, it is good to make new constructor.

>> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:67
>> +        mimetypes = eina_list_append(mimetypes, strdup((*it).utf8().data()));
> 
> I thought we prefered to use eina_stringshare in EFL port.

IIRC, ewk_intext.cpp was landed by you. If so, let's change this one in new bug.

http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_intent.cpp#L109

> Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:106
> +Ewk_File_Chooser_Settings* ewk_file_chooser_settings_new(const WebCore::FileChooserSettings& coreSettings)

I think this API doesn't need to have coreSettings. It is just enough to have *setting*.

[Gyuyoung Kim]

Comment on attachment 153943 [details]
Patch

Set r- because of many comments.

[Kihong Kwon]

(In reply to comment #15)
> (From update of attachment 153943 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153943&action=review
> 
> Stupid question: How is the client support to use this API? Both the constructor and destructor are private and the patch does not introduce any where to retrieve the Ewk_File_Choose_Settings from somewhere else. In itself, the patch seems to add code but no real functionality. Am I missing something?

Client can access this by run_open_panel to get attribute of panel.

> > Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:65
> > +    Vector<WTF::String>::const_iterator it = settings->fileChooserSettings.acceptMIMETypes.begin();
> 
> As per coding style, you should avoid using iterators and use indexes instead.
> 

I didn't know about that, could you show me the url of guide?

> > Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:67
> > +        mimetypes = eina_list_append(mimetypes, strdup((*it).utf8().data()));
> 
> I thought we prefered to use eina_stringshare in EFL port.
> 

I agree. but we need to change strdup to eina_string_share in the ewk_intent like gyuyoung said.

> > Source/WebKit/efl/ewk/ewk_file_chooser_settings.h:107
> > + * @return one of these string "camera", "camcorder", "microphone" 
> 
> If the values are predetermined, why not use an enumeration instead? That would be much better for the clients.

I agree.

Thanks :)

[Chris Dumez]

(In reply to comment #18)
> (In reply to comment #15)
> > (From update of attachment 153943 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=153943&action=review
> > 
> > Stupid question: How is the client support to use this API? Both the constructor and destructor are private and the patch does not introduce any where to retrieve the Ewk_File_Choose_Settings from somewhere else. In itself, the patch seems to add code but no real functionality. Am I missing something?
> 
> Client can access this by run_open_panel to get attribute of panel.

ewk_view_run_open_panel() is private as well. If you mean the run_open_panel callback on the Ewk_View, then I still don't get it because the definition looks like this:

Eina_Bool (*run_open_panel)(Ewk_View_Smart_Data *sd, Evas_Object *frame, Eina_Bool allows_multiple_files, Eina_List *accept_types, Eina_List **selected_filenames);

It does not use Ewk_File_Choose_Settings and your patch does not seem to make it use it either.

> 
> > > Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:65
> > > +    Vector<WTF::String>::const_iterator it = settings->fileChooserSettings.acceptMIMETypes.begin();
> > 
> > As per coding style, you should avoid using iterators and use indexes instead.
> > 
> 
> I didn't know about that, could you show me the url of guide?

http://www.webkit.org/coding/coding-style.html

[Kihong Kwon]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #15)
> > > (From update of attachment 153943 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=153943&action=review
> > > 
> > > Stupid question: How is the client support to use this API? Both the constructor and destructor are private and the patch does not introduce any where to retrieve the Ewk_File_Choose_Settings from somewhere else. In itself, the patch seems to add code but no real functionality. Am I missing something?
> > 
> > Client can access this by run_open_panel to get attribute of panel.
> 
> ewk_view_run_open_panel() is private as well. If you mean the run_open_panel callback on the Ewk_View, then I still don't get it because the definition looks like this:
> 
> Eina_Bool (*run_open_panel)(Ewk_View_Smart_Data *sd, Evas_Object *frame, Eina_Bool allows_multiple_files, Eina_List *accept_types, Eina_List **selected_filenames);
> 
> It does not use Ewk_File_Choose_Settings and your patch does not seem to make it use it either.

I would like to change prototype of run_open_panel. That is not enough to use file open dialog.(bug 91956)

> 
> > 
> > > > Source/WebKit/efl/ewk/ewk_file_chooser_settings.cpp:65
> > > > +    Vector<WTF::String>::const_iterator it = settings->fileChooserSettings.acceptMIMETypes.begin();
> > > 
> > > As per coding style, you should avoid using iterators and use indexes instead.
> > > 
> > 
> > I didn't know about that, could you show me the url of guide?
> 
> http://www.webkit.org/coding/coding-style.html

Thanks. I forgot them.

[Kihong Kwon]

Created attachment 154009 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 154009 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_file_chooser.h:86
> + *         the Eina_List and its items should be freed after use (use free() to free the items)

I would recommend to mention eina_stringshare_del().

For example,
the Eina_List and its items should be freed after use. Use eina_stringshare_del()
to free the items.

> Source/WebKit/efl/ewk/ewk_file_chooser.h:96
> + *         the Eina_List and its items should be freed after use (use free() to free the items)

ditto.

> Source/WebKit/efl/ewk/ewk_file_chooser.h:109
> + *         the Eina_List and its items should be freed after use (use free() to free the items)

ditto.

[Kihong Kwon]

Created attachment 154220 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 154220 [details]
Patch

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

When you submit modified patch, please take a look this patch further. In addition, please run layout test as well. I don't like too long bug review thread.

> Source/WebKit/efl/ewk/ewk_file_chooser.cpp:90
> +    if (capture == "camera")

If possible, follow enum type definition order.

> Source/WebKit/efl/ewk/ewk_file_chooser.cpp:99
> +    return EWK_FILE_CHOOSER_CAPTURE_TYPE_FILESYSTEM;

I wonder if this patch can guarantee that capture only has camera, camcorder, microphone and file system.

If not so, you have to modify this as below,

if (capture == "filesystem")
    return EWK_FILE_CHOOSER_CAPTURE_TYPE_FILESYSTEM;

return EWK_FILE_CHOOSER_CAPTURE_TYPE_INVALID;

> Source/WebKit/efl/ewk/ewk_file_chooser.cpp:112
> +void ewk_file_chooser_free(Ewk_File_Chooser* ewkFileChooser)

Other functions are using just *chooser*. Keep to be consistent with other functions.

> Source/WebKit/efl/ewk/ewk_file_chooser.h:112
> + *         (use eina_stringshare_del() to free the items)

I think you don't need to use ( ). See also below comment.

http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_intent.h#L80

I will not use (..) in my next patch.

[Kihong Kwon]

(In reply to comment #24)
> (From update of attachment 154220 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154220&action=review
> 
> When you submit modified patch, please take a look this patch further. In addition, please run layout test as well. I don't like too long bug review thread.
> 
> > Source/WebKit/efl/ewk/ewk_file_chooser.cpp:90
> > +    if (capture == "camera")
> 
> If possible, follow enum type definition order.

I think ordering is right.
filesystem is a basic type of capture attribute.
That's why, I let filesystem in the first of enum.
And use that for defalut return value.

http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLInputElement.cpp#L1723

> 
> > Source/WebKit/efl/ewk/ewk_file_chooser.cpp:99
> > +    return EWK_FILE_CHOOSER_CAPTURE_TYPE_FILESYSTEM;
> 
> I wonder if this patch can guarantee that capture only has camera, camcorder, microphone and file system.
> 
> If not so, you have to modify this as below,

As you can see in the WebCore, http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLInputElement.cpp#L1723
If there is not a valid value, input element works with "filesystem".

So, EWK_FILE_CHOOSER_CAPTURE_TYPE_INVALID is only used when Ewk_File_Chooser pointer or capture attribute is invalid.
Therefore I think current implementation is right.

> 
> if (capture == "filesystem")
>     return EWK_FILE_CHOOSER_CAPTURE_TYPE_FILESYSTEM;
> 
> return EWK_FILE_CHOOSER_CAPTURE_TYPE_INVALID;
> 
> > Source/WebKit/efl/ewk/ewk_file_chooser.cpp:112
> > +void ewk_file_chooser_free(Ewk_File_Chooser* ewkFileChooser)
> 
> Other functions are using just *chooser*. Keep to be consistent with other functions.

OK.

> 
> > Source/WebKit/efl/ewk/ewk_file_chooser.h:112
> > + *         (use eina_stringshare_del() to free the items)
> 
> I think you don't need to use ( ). See also below comment.
> 
> http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_intent.h#L80
> 
> I will not use (..) in my next patch.

No problem. 
but I wonder which one is right? "Use..." like a ewk_intest.h or "use...".
I thought we don't use capital letter with @return.
But, you didn't mention about that.

Thanks gyuyoung.

[Gyuyoung Kim]

Comment on attachment 154220 [details]
Patch

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

>>> Source/WebKit/efl/ewk/ewk_file_chooser.cpp:99
>>> +    return EWK_FILE_CHOOSER_CAPTURE_TYPE_FILESYSTEM;
>> 
>> I wonder if this patch can guarantee that capture only has camera, camcorder, microphone and file system.
>> 
>> If not so, you have to modify this as below,
>> 
>> if (capture == "filesystem")
>>     return EWK_FILE_CHOOSER_CAPTURE_TYPE_FILESYSTEM;
>> 
>> return EWK_FILE_CHOOSER_CAPTURE_TYPE_INVALID;
> 
> As you can see in the WebCore, http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLInputElement.cpp#L1723
> If there is not a valid value, input element works with "filesystem".
> 
> So, EWK_FILE_CHOOSER_CAPTURE_TYPE_INVALID is only used when Ewk_File_Chooser pointer or capture attribute is invalid.
> Therefore I think current implementation is right.

Ok, I understand.

>>> Source/WebKit/efl/ewk/ewk_file_chooser.h:112
>>> + *         (use eina_stringshare_del() to free the items)
>> 
>> I think you don't need to use ( ). See also below comment.
>> 
>> http://trac.webkit.org/browser/trunk/Source/WebKit/efl/ewk/ewk_intent.h#L80
>> 
>> I will not use (..) in my next patch.
> 
> No problem. 
> but I wonder which one is right? "Use..." like a ewk_intest.h or "use...".
> I thought we don't use capital letter with @return.
> But, you didn't mention about that.
> 
> Thanks gyuyoung.

Please use lower case. I'm going to change uppercase in ewk_intent.h

> Source/WebKit/efl/ewk/ewk_file_chooser.h:119
> + * The capture attributes is to support HTML Media Capture.

Why don't you mention more information as chromium port ?

    // See http://www.w3.org/TR/html-media-capture/ for the semantics of the
    // capture attribute. This string will either be empty (meaning the feature
    // is disabled) or one of the following values:
    //  - filesystem (default)
    //  - camera
    //  - camcorder
    //  - microphone

> Source/WebKit/efl/ewk/ewk_file_chooser_private.h:28
> +void ewk_file_chooser_free(Ewk_File_Chooser* ewkFileChooser);

ditto.

[Kihong Kwon]

Created attachment 154281 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 154281 [details]
Patch

LGTM now, thanks.

[Hajime Morrita]

Comment on attachment 154281 [details]
Patch

rs=me.

[WebKit Review Bot]

Comment on attachment 154281 [details]
Patch

Clearing flags on attachment: 154281

Committed r123597: <http://trac.webkit.org/changeset/123597>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.