[webkit-reviews] review denied: [Bug 98935] [GTK] Add support for color input : [Attachment 203207] updated patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied arno.
<arno at renevier.net>'s request for review:
Bug 98935: [GTK] Add support for color input
https://bugs.webkit.org/show_bug.cgi?id=98935

Attachment 203207: updated patch
https://bugs.webkit.org/attachment.cgi?id=203207&action=review

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


This looks much better, thanks! there are just a few minor issues. We still
need another GTK+ reviewer to check the new API and a WebKit2 owner to approve
also the changes in the WebColorPickerResultListenerProxy.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:88
> +#if ENABLE(INPUT_TYPE_COLOR)
> +#include "ColorChooserClient.h"
> +#endif

ColorChooserClient.h already has the #if ENABLE(INPUT_TYPE_COLOR) so we don't
need to add the #if here too.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.h:125
> +	   virtual PassOwnPtr<ColorChooser>
createColorChooser(ColorChooserClient*, const Color& initialColor);

You can omit the initialColor parameter name here.

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:28
> +#include "config.h"
> +
> +#include "WebKitColorChooserRequest.h"

Remove that empty line.

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:31
> +#include <WebCore/Color.h>

This is already included in WebKitColorChooserRequestPrivate.h

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:145
> + * Returns: the current color of request. 

Now that it returns a pointer you should add a introspection annotation to
indicate that the returned pointer is owned by the request. 

Returns: (transfer none): 

I wonder if it would be better to return the color as an out parameter, though.


> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:151
> +    g_return_val_if_fail(WEBKIT_IS_COLOR_CHOOSER_REQUEST(request), NULL);

Use 0 instead of NULL here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:27
> +#include "WebColorPickerResultListenerProxy.h"

This is already included by WebKitColorChooserRequestPrivate.h

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:459
> +    if (webView->priv->colorChooserRequest)
> +	   webView->priv->colorChooserRequest = 0;

You don't need to check, GRefPtr already checks the internal pointer and it's
safe to set 0 multiple times.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:483
> +    GtkWidget* dialog =  gtk_color_chooser_dialog_new(_("Select Color"),
toplevel ? GTK_WINDOW(toplevel) : 0);

Extra space after the =

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1296
> +	 *

Since: 2.2

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1313
> +	 * Emitted when color picker must be closed. This happens for example
if the
> +	 * page triggering the request unloads before the request is completed.


Isn't this also called when a color has been chosen?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1316
> +	 * Returns: %TRUE to stop other handlers from being invoked for the
event.
> +	 *   %FALSE to propagate the event further.

This signals returns G_TYPE_NONE now, remove the Returns: tag.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1317
> +	 *

Since: 2.2

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1768
> +    gboolean returnValue;
> +    g_signal_emit(webView, signals[CLOSE_COLOR_CHOOSER], 0, &returnValue);

This signal is not true handled.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:821
> +    static gboolean closeColorChooserCallback(WebKitWebView*,
ColorChooserTest* test)

This should be void

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:836
> +	   hadClosedSignal = TRUE;

signal name is close not closed, maybe hadCloseSignal sounds a bit better.
Maybe m_colorChooserClosed?

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:853
> +    gboolean hadClosedSignal;

Use bool instead of gboolean, and m_ prefix for class memebers.

bool m_hadClosedSignal;