[webkit-reviews] review denied: [Bug 91957] [EFL] Add File Chooser API : [Attachment 154220] Patch

[bugzilla-daemon at webkit.org]

Gyuyoung Kim <gyuyoung.kim at samsung.com> has denied Kihong Kwon
<kihong.kwon at samsung.com>'s request for review:
Bug 91957: [EFL] Add File Chooser API
https://bugs.webkit.org/show_bug.cgi?id=91957

Attachment 154220: Patch
https://bugs.webkit.org/attachment.cgi?id=154220&action=review

------- Additional Comments from Gyuyoung Kim <gyuyoung.kim at samsung.com>
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.