[webkit-reviews] review denied: [Bug 98935] [GTK] Add support for color input : [Attachment 181118] rebase against tot

Thu Jan 3 07:56:41 PST 2013

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 181118: rebase against tot
https://bugs.webkit.org/attachment.cgi?id=181118&action=review

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

Thanks for splitting the patch, it looks much better now, but we should
implement the UIClient callback from the GTK+ API.

> Source/WebKit2/ChangeLog:12
> +	   #WebKitColorChooserRequest object. If the signal is not handled by
the

Don't use # in the changelog

> Source/WebKit2/ChangeLog:14
> +	   application, we show a GtkGtkColorChooserDialog (or
> +	   GtkColorSelectionDialog for Gtk < 3.4.0).

We don't hace special case for Gtk < 3.4.0, right?

> Source/WebKit2/ChangeLog:16
> +	   WebKitColorChooserRequest api allows to read current color (either

API

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:240
> -PassRefPtr<WebColorChooserProxy>
PageClientImpl::createColorChooserProxy(WebPageProxy*, const WebCore::Color&,
const WebCore::IntRect&)
> +PassRefPtr<WebColorChooserProxy>
PageClientImpl::createColorChooserProxy(WebPageProxy* page, const
WebCore::Color& initialColor, const WebCore::IntRect& elementRect)
>  {
> -    notImplemented();
> -    return 0;
> +    return WebColorChooserProxyGtk::create(page, m_viewWidget, initialColor,
elementRect);

I still think this should be implemented only as a fallback when the client is
not implemented and implement the UI client callbacks. If the only problem is
that the listener doesn't allow to finish the request without selecting a
color, add WebColorPickerResultListenerProxy::endColorChooser()

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:63
> +    Color color;

I think it would be better to cache a GdkRGBA that we can return directly.

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:101
> +    signals[CHANGE_COLOR] =

Instead of a signal this could be a property "color" and users can connect to
"notify" signal.

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

Pass a pointer for the color const GdkRGBA* color

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:159
> +GdkRGBA
webkit_color_chooser_request_get_current_color(WebKitColorChooserRequest*
request)

Return a pointer

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:167
> +WebKitColorChooserRequest* webkitColorChooserRequestCreate(const Color
initialColor, WebColorChooserProxy::Client* client)

You could probably pass a reference for the color const Color& initialColor

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

This is false already, private struct is initialized to 0 when allocated.

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:181
> +    g_signal_emit(request, signals[END_CHOOSER], 0, NULL);

This signal doesn't exist.

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:187
> +    g_signal_emit(request, signals[CHANGE_COLOR], 0,
request->priv->color.serialized().ascii().data(), NULL);

I we use a property here we would emit notify. We should check that the color
has actually changed, and return early if the color hasn't changed.

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequestPrivate.h:29
> +#include "Color.h"

Use #include <WebCore/Color.h> instead.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:111
> +    HIDE_COLOR_CHOOSER,

hmm, I think I prefer close-color-chooser

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:185
> +    GRefPtr<GtkWidget> colorChooserDialog;

Don't use GRefPtr for GtkWidgets, they have a floating reference, and you are
going to delete it with gtk_widget_destroy

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:437
> +	   // noop if webkit_color_chooser_request_select_color has been called
before

In what case can this happen?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:438
> +	  
webkit_color_chooser_request_cancel(webView->priv->colorChooserRequest.get());

The request is cancelled in the dispose, so we could simply unref the request
object.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:450
> +    GRefPtr<WebKitColorChooserRequest> refedRequest =
webView->priv->colorChooserRequest;

Do we need to take another reference?

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

This seems unused.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:458
> +    webkitWebViewHideColorChooser(webView, refedRequest.get());

This is already called after webkit_color_chooser_request_select_color(), so
instead of calling it here again, we could call
webkit_color_chooser_request_cancel() when responseID != OK

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:464
> +    if (webView->priv->colorChooserRequest)
> +	   return FALSE;

Can this really happen?

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

!! -> !

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1244
> +	* WebKitWebView::run-color-chooser:
> +	* @web_view: the #WebKitWebView on which the signal is emitted
> +	* @request: a #WebKitColorChooserRequest

This is not correctly indented, the * should be lined up.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1274
> +	* @request: a #WebKitColorChooserRequest

Is this really important? I guess there can't be more than one request as the
same time for the same view.

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

This is also emitted when the request is finished correctly after selecting a
color, no?

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1288
>> +			 g_signal_accumulator_true_handled, 0 /* accumulator
data */,
> 
> Weird number of spaces at line-start.  Are you using a 4-space indent? 
[whitespace/indent] [3]

I'm not sure this needs to be true_handled. We could check if we have a dialog
in the default handler and destroy it.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:842
> +    g_assert_cmpfloat(color.red, ==, 1.0);
> +    g_assert_cmpfloat(color.green, ==, 0.0);
> +    g_assert_cmpfloat(color.blue, ==, 0.0);
> +    g_assert_cmpfloat(color.alpha, ==, 1.0);

Maybe we could add a helper method to check a color assertColorIsEqual() or
something like that.

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

We should also check that the notify signal is emitted when the color is
changed by javascript.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:860
> +    g_assert_cmpstr(valueString.get(), ==, "#0000ff");

We should also test that close-color-chooser is emitted when the request is
finished and cancelled.

> Source/WebKit2/UIProcess/gtk/WebColorChooserProxyGtk.cpp:45
> +    if
(!webkitWebViewRunColorChooserRequest(WEBKIT_WEB_VIEW(m_webView.get()),
WEBKIT_COLOR_CHOOSER_REQUEST(m_request.get())))
> +	  
webkit_color_chooser_request_cancel(WEBKIT_COLOR_CHOOSER_REQUEST(m_request.get(
)));

The problem of doing this here is that you can't assume the view widget is a
WebKitWebView, in case of C API users it will be a WebKitWebViewBase. Also,
WebKitColorChooserRequest is part of the GTK+ API not the C API.

> LayoutTests/ChangeLog:15
> +	   * fast/forms/color/input-color-onchange-event-expected.txt:
> +	   * platform/gtk/TestExpectations:
> +	   * platform/gtk/fast/forms/color/input-appearance-color-expected.txt:
Added.

I guess this would be part fo the wk1 patch, no?