[Webkit-unassigned] [Bug 91957] [EFL] Add File Chooser Settings API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=91957





--- Comment #15 from Christophe Dumez <christophe.dumez at intel.com>  2012-07-23 23:54:58 PST ---
(From update of attachment 153943)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.