[webkit-reviews] review denied: [Bug 136372] [GTK] allow overwriting destination of download : [Attachment 237353] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Michael Catanzaro
<mcatanzaro at gnome.org>'s request for review:
Bug 136372: [GTK] allow overwriting destination of download
https://bugs.webkit.org/show_bug.cgi?id=136372

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

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=237353&action=review


We need a test case in the unit tests for this new API, and new symbols in the
docs, see http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

> Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:205
> +	*/

Since: 2.6

> Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:213
> +    g_object_class_install_property(objectClass,
> +				       PROP_ALLOW_OVERWRITE,
> +				       g_param_spec_boolean("allow-overwrite",
> +							    _("Allow
Overwrite"),
> +							    _("Whether the
destination may be overwritten"),
> +							    FALSE,
> +							   
WEBKIT_PARAM_READWRITE));
> +

You need to follow the coding style here regarding indentation.

> Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:406
> -CString webkitDownloadDecideDestinationWithSuggestedFilename(WebKitDownload*
download, const CString& suggestedFilename)
> +CString webkitDownloadDecideDestinationWithSuggestedFilename(WebKitDownload*
download, const CString& suggestedFilename, bool* allowOverwrite)

The public C API uses a pointer for the out parameter but here we can use a
reference instead. We should move this to use API::DownloadClient at some point
instead of the C API.

> Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:413
> +    if (allowOverwrite)
> +	   *allowOverwrite = download->priv->allowOverwrite;

We should always receive a reference.

> Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:611
> + * Returns the current value of the #WebKitDownload::allow-overwrite
property,

:: is for signals, for properties use a single :

> Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:615
> + * Returns: the current value of the #WebKitDownload::allow-overwrite
property

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:629
> + * @allowed: the new value for the #WebKitDownload::allow-overwrite property


Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:631
> + * Sets the #WebKitDownload::allow-overwrite property, which determines
whether

Ditto.