[webkit-reviews] review denied: [Bug 98935] [GTK] Add support for color input : [Attachment 200854] updated patch: fixes style issues

[bugzilla-daemon at webkit.org]

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

Attachment 200854: updated patch: fixes style issues
https://bugs.webkit.org/attachment.cgi?id=200854&action=review

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


Sorry for the delay reviewing this. Since we are enabling the color picker
feature unconditionally, I wonder if we could avoid all the ifdefs.

> Source/WebKit2/ChangeLog:11
> +	   Implement showColorPicker and hideColorPicker UI client callbacks.
As
> +	   a fallback, implement WebColorChooserProxyGtk.

I think this should be easier to review if we leave the fallback impl for a
follow up patch.

> Source/WebKit2/ChangeLog:17
> +	   Implement a WebColorChooserClientGtk object. A
> +	   WebColorChooser client will be attached to the WebKitWebView. Both
> +	   showColorPicker UI callback or WebColorChooserClientGtk will call
> +	   webkitWebViewBaseShowColorChooser which will then use client to
notify
> +	   the WebKitWebView that it should display the color chooser.

Why do you need this instead of implementing showColorPicker and
hideColorPicker from the WKPageUIClient?

> Source/WebKit2/UIProcess/API/C/gtk/WKColorChooserClientGtk.cpp:37
> +void WKViewSetColorChooserClientGtk(WKViewRef viewRef, const
WKColorChooserClientGtk* wkClient)
> +{
> +    webkitWebViewBaseInitializeColorChooserClient(toImpl(viewRef),
wkClient);
> +}

Why do we need a separate client for the color chooser instead of using the
UIClient?

> Source/WebKit2/UIProcess/API/C/gtk/WKColorChooserClientGtk.h:42
> +    WKColorChooserClientGtkShowColorChooserCallback	showColorChooser;
> +    WKColorChooserClientGtkHideColorChooserCallback	hideColorChooser;

These are already in WKPageUIClient.

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

As I said I would leave this unimplemented for now, to make the patch smaller.

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:112
> +	*/

All new API should have now Since: 2.2 tags.

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:129
> + * Ask WebKit to cancel the request. It's important to do this in case
> + * no selection has been made in the client, otherwise the request
> + * won't be properly completed and the browser will keep the request
> + * pending forever, which might cause the browser to hang.

To prevent this, you should implement dispose and cancel the request if it
hasn't been handled.

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:135
> +    request->priv->handledRequest = TRUE;

handledRequest is bool, use true instead of TRUE.

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:153
> +    request->priv->handledRequest = TRUE;

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:167
> + * Returns: the current color of request. While no color has been selected,
it
> + * is the initial value defined by WebKit. When a color has been selected,
this
> + * becomes the selected value.

Please, add the xplanation to the body and leave the return tag as Returns: the
current color of request

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

Please line up parameter names.

> Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:175
> +    webkitWebViewBaseShowColorChooser(WEBKIT_WEB_VIEW_BASE(clientInfo),
WebCore::Color(WebKit::toWTFString(initialColor)), toImpl(listener));

You can use toWTFString directly since there's a using namespace WebKit;

> Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:182
> +#endif

So you are implementing it here too? What's the other client for then?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:470
> +    if (webView->priv->colorChooserRequest) {
> +	   g_object_unref(webView->priv->colorChooserRequest.get());
> +	   webView->priv->colorChooserRequest.clear();
> +    }

I think you can simply do webView->priv->colorChooserRequest = 0;

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:472
> +    if (webView->priv->colorChooserDialog)
> +	   gtk_widget_destroy(webView->priv->colorChooserDialog);

You should set webView->priv->colorChooserDialog = 0 here.

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

webView->priv->colorChooserRequest is a RefPtr, so you are increasing the ref
counter twice and then leaking the request.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:489
> +    if (!!widgetIsOnscreenToplevelWindow(toplevel))

!!widgetIsOnscreenToplevelWindow(toplevel)?

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

toplevel can be NULL, you should use toplevel ? GTK_WINDOW(toplevel) : 0

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:496
> +    g_signal_connect(dialog, "response",
G_CALLBACK(colorChooserDialogResponseCallback), webView);

I think you can pass the request to the signal (instead of the view) and that
way you don't probably need to cache it in the private struct

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1322
> +	 * Emitted when color picker must be hidden. This happens for example
if the

I would say closed instead of hidden since the signals is called
close-color-chooser

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1340
> +    ColorChooserTest::add("WebKitWebView", "color-chooser-request",
testWebViewColorChooserRequest);

I would be nice to add also a tests for the color property to mke sure notify
is emitted when it changes (and only if it has changed).