[webkit-reviews] review denied: [Bug 98935] [GTK] Add support for color input : [Attachment 180417] 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 180417: updated patch
https://bugs.webkit.org/attachment.cgi?id=180417&action=review

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


Thanks for the patch! I think it would be easier to review if you split the
patch in two, one for webkit1 and the other for webkit2. I'll review the
webkit2 part for now.

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:238
> -    notImplemented();
> -    return 0;
> +    return WebColorChooserProxyGtk::create(page, m_viewWidget, initialColor,
elementRect);

Instead of implementing this, it would be better to implement showColorPicker
and hideColorPicker callbacks of the UI client. That way the implementation
would be in the API. This could be implemented to also provide a color chooser
for C API users if they don't implement the UI client callbacks themselves. For
now, I would only focus on GTK+ API.

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

Use angle-bracket form for header files from WebCore. #include
<WebCore/Color.h>

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:62
> +#if ENABLE(INPUT_TYPE_COLOR)
> +    WebColorChooserProxy::Client* client;
> +#endif

Instead of a client this should have a WebColorPickerResultListenerProxy

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:101
> +    /**
> +	* WebKitColorChooserRequest:end-chooser:
> +	* @request: the #WebKitColorChooserRequest on which the signal is
emitted
> +	*
> +	* Emitted when request must be terminated. This happens for example if
the
> +	* page triggering the request unloads before the request is completed.
> +	*/
> +    signals[END_CHOOSER] =

Maybe this should be a signal in the view hide-color-chooser, for example.

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:116
> +    /**
> +	* WebKitColorChooserRequest:change-color:
> +	* @request: the #WebKitColorChooserRequest on which the signal is
emitted
> +	* @color: the new color
> +	*
> +	* Emitted when default value of request is changed.
> +	*/
> +    signals[CHANGE_COLOR] =

This shouldn't happen, no? the request is created with an initial color, and
the user finish the request setting a new color

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:140
> +#if ENABLE(INPUT_TYPE_COLOR)
> +    request->priv->client->didEndColorChooser();
> +    request->priv->handledRequest = TRUE;
> +#endif

Return early if the request has been completed to make sure we don't handle
this twice.

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:146
> + * @color: (array zero-terminated=1) (transfer none): a

Why is color a zero-terminated array? Doc is missing there, cut and paste? :-)

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:150
> +void webkit_color_chooser_request_select_color(WebKitColorChooserRequest*
request, const gchar* color)

Could we pass a GdkRGBA instead? and convert it to a WebCore::Color

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:159
> +    request->priv->color = Color(color);
> +#if ENABLE(INPUT_TYPE_COLOR)
> +    request->priv->client->didChooseColor(request->priv->color);
> +    request->priv->client->didEndColorChooser();
> +#endif
> +    request->priv->handledRequest = TRUE;

REturn early if the request has been completed here too.

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:170
> +const gchar*
webkit_color_chooser_request_get_current_color(WebKitColorChooserRequest*
request)

Could we return a GdkRGBA instead?

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.h:61
> +WEBKIT_API GType
> +webkit_color_chooser_request_get_type		  (void);

(void) shuld be lined up with parameters of the other methods.

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.h:64
> +webkit_color_chooser_request_get_current_color  (WebKitColorChooserRequest*
request);

In public headers we use GNOME coding style, this should be
WebKitColorChooserRequest *request

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.h:68
> +webkit_color_chooser_request_select_color	   (WebKitColorChooserRequest*
request, 
> +						    const gchar * color);

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:110
> +    RUN_COLOR_CHOOSER,

This signal is a bit different that run-file-chooser, because in this case the
dialog should be closed/hidden when the page changes, so maybe it should split
in show-color-chooser and close-color-chooser, for example. Or we could keep
run-file-chooser and add a cancelled signal to the request object and simply
cancel the request.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:421
> +#if GTK_CHECK_VERSION(3, 4, 0)

GTK+ 3.4 is the minimum required version, so we don't need this checks

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:449
> +	   GOwnPtr<char> str_value;

This should be stringValue

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:453
> +	   str_value.set(g_strdup_printf("#%.2X%.2X%.2X", (int)(color.red *
255), (int)(color.green * 255), (int)(color.blue * 255)));

I could pass a GdkRGBA directly to the request and do the conversions needed
there.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:471
> +    if (!gtk_widget_is_toplevel(parent) || !GTK_IS_WINDOW(parent))

Use widgetIsOnscreenToplevelWindow() instead.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:479
> +    const gchar* initialColor =
webkit_color_chooser_request_get_current_color(request);
> +    updateColorChooserColorValue(dialog, initialColor);

I think the request should be created with the initial color.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:485
> +    g_object_ref(request);

Move this to the signal_connect to make it clear that we are passing the
ownership to the callback.

g_signal_connect(dialog, "response",
G_CALLBACK(colorChooserDialogResponseCallback), g_object_ref(request));

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:842
> +   
test->runJavaScriptAndWaitUntilFinished("document.querySelector('input').value
= '#00ff00';", 0);
> +   
g_assert_cmpstr(webkit_color_chooser_request_get_current_color(colorChooserRequ
est), ==, "#00ff00");

Oh, I understand now, the current color might change while the color chooser is
running. It would be a bit weird, that you are selecting a color from the
dialog and a javascript changes it.