Bug 98935 – [GTK] Add API to allow overriding the default color chooser implementation

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Zan Dobersek]

The GTK port should add support for color input, using the handy GtkColorChooser widget.

[arno.]

I'm interested in working on it

[arno.]

Created attachment 179382 [details]
WIP patch

Here is a wip patch. I've tested it and I'm confident it works fine. But the build fails when linking DumpRenderTree.

error is:

  CXXLD  Programs/DumpRenderTree
./.libs/libWebCoreInternals.a(libWebCoreInternals_la-Internals.o): In function `WebCore::Internals::selectColorInColorChooser(WebCore::Element*, WTF::String const&)':
Internals.cpp:(.text+0x1106): undefined reference to `WebCore::Color::Color(WTF::String const&)'
Internals.cpp:(.text+0x1112): undefined reference to `WebCore::HTMLInputElement::selectColorInColorChooser(WebCore::Color const&)'

This is strange because libWebCorePlatform.la in passed on the command line, and WebCore::Color::Color(WTF::String const&) is correctly defined in .libs/libWebCorePlatform.a (ie: I checked with nm -g, that's it's in the text section of one of the its object files). It seems to be the only place in DumpRenderTree where a function from webcore platform is used.

When this is fixed, I can enable the tests, and write the ChangeLogs

[arno.]

CCing people who worked on #94435
So may be they have an idea about this linking problem.

[arno.]

Created attachment 179501 [details]
WIP patch

rebased against current trunk.

[Zan Dobersek]

(In reply to comment #2)
> When this is fixed, I can enable the tests, and write the ChangeLogs

You have to additionally export these symbols in Source/autotools/symbols.filter:
diff --git a/Source/autotools/symbols.filter b/Source/autotools/symbols.filter
index f87f834..2c6b00c 100644
--- a/Source/autotools/symbols.filter
+++ b/Source/autotools/symbols.filter
@@ -232,6 +232,8 @@ _ZN7WebCore18StyleSheetContentsD1Ev;
 _ZN7WebCore28DocumentStyleSheetCollection12addUserSheetEN3WTF10PassRefPtrINS_18StyleSheetContentsEEE;
 _ZN7WebCore28DocumentStyleSheetCollection14addAuthorSheetEN3WTF10PassRefPtrINS_18StyleSheetContentsEEE;
 _ZN7WebCore4KURL10invalidateEv;
+_ZN7WebCore5ColorC1ERKN3WTF6StringE;
+_ZN7WebCore16HTMLInputElement25selectColorInColorChooserERKNS_5ColorE;
 
 local:
 _Z*;


http://trac.webkit.org/wiki/ExportingSymbols has a bit more info.

[arno.]

Created attachment 180391 [details]
thanks a lot

patch proposal

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[WebKit Review Bot]

Attachment 180391 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1248:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1249:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1250:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1251:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1252:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1253:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1254:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.h"
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:104:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:105:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:106:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:107:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:119:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:120:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:121:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:122:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 15 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 180391 [details]
thanks a lot

Attachment 180391 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15456143

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[arno.]

Created attachment 180417 [details]
updated patch

follows guidelines for WebKit2 GTK+ API better

[WebKit Review Bot]

Attachment 180417 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1248:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1249:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1250:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1251:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1252:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1253:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1254:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.h"
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:103:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:104:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:105:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:106:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:118:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:119:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:120:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:121:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 15 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 180417 [details]
updated patch

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(colorChooserRequest), ==, "#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.

[arno.]

Created attachment 180569 [details]
new patch iteration

(In reply to comment #12)


> 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.

input.value may be changed via javascript when the picker is open. We may choose either to change selected value in the picker or no. I've tested in chromimum and in opera; they both update the new value in the picker.

> 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.
The problem is that there in no changeColor callback.
Another thing is WebColorPickerResultListenerProxy only has a setColor api which calls m_page->setColorChooserColor and m_page->endColorChooser. So it's not possible to simply end a chooser without selecting a color.
So imagine you have a input with blue color. User opens the picker. Meanwile, value changes to red. We won't be aware of that change. Then user cancels the picker: with listenerproxy api, we need to select a color. But we are only aware of the initial color (blue). So, we would use not be able to put input color value to red (new javascript set color). I could not find a way to handle this edge case with {show,hide}ColorPicker and WebColorPickerResultListenerProxy. That's why I implemented createColorChooserProxy.

Do you have any idea to do better ?

> Instead of a client this should have a WebColorPickerResultListenerProxy
see above: with this object, we cannot just endChooser (ie: cancel it)


> > 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.

May be my new patch handle that. But I'm not totally sure what you mean here.

Other comments have been addressed in this new patch iteration (except I did not split it yet).

[WebKit Review Bot]

Attachment 180569 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
Source/WebKit2/UIProcess/gtk/WebColorChooserProxyGtk.cpp:46:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebKit2/UIProcess/gtk/WebColorChooserProxyGtk.cpp:58:  One line control clauses should not use braces.  [whitespace/braces] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:852:  Missing space before {  [whitespace/braces] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1240:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1241:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1242:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1243:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1244:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1245:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1246:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1262:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1263:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1264:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1265:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1266:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1267:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1268:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.h"
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:103:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:104:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:105:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:106:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:161:  Missing space before {  [whitespace/braces] [5]
Total errors found: 23 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[arno.]

Created attachment 180981 [details]
WebKit2 only patch

[arno.]

Created attachment 181118 [details]
rebase against tot

[WebKit Review Bot]

Attachment 181118 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:851:  Missing space before {  [whitespace/braces] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1263:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1264:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1265:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1266:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1267:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1268:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1269:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1285:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1286:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1287:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1288:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1289:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1290:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1291:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.h"
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:103:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:104:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:105:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:106:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:161:  Missing space before {  [whitespace/braces] [5]
Total errors found: 21 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 181118 [details]
rebase against tot

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?

[Martin Robinson]

Comment on attachment 181118 [details]
rebase against tot

Is it really interesting to expose a color chooser API. Why not just show the default GTK color chooser?

[Carlos Garcia Campos]

(In reply to comment #19)
> (From update of attachment 181118 [details])
> Is it really interesting to expose a color chooser API. Why not just show the default GTK color chooser?

I'm not sure I understand the question, the default signal handler shows the default GTK color chooser when the signal is not handled.

[Martin Robinson]

(In reply to comment #20)

> I'm not sure I understand the question, the default signal handler shows the default GTK color chooser when the signal is not handled.

Theoretically if someone wanted to show a different color chooser, they would just choose to modify GTK+ for their operating system, right?

[Carlos Garcia Campos]

(In reply to comment #21)
> (In reply to comment #20)
> 
> > I'm not sure I understand the question, the default signal handler shows the default GTK color chooser when the signal is not handled.
> 
> Theoretically if someone wanted to show a different color chooser, they would just choose to modify GTK+ for their operating system, right?

No, they would connect to the signal, show their chooser and return TRUE. That's what sugar does for the file chooser for example.

[arno.]

Created attachment 181399 [details]
new patch iteration

New patch iteration:
add endColorChooser method to WebColorPickerResultListenerProxy, and use it instead of WebColorChooserProxy::client in chooser proxy.
the main behaviour difference is that I do not update the color in the picker if color is change via javascript
also, implement showColorPicker and hideColorPicker callbacks.
The request is "owned" by the webView which g_object_ref's in webkitWebViewRunColorChooser and g_object_unref's it in webkitWebViewCloseColorChooser
close_color_chooser is now called even when picker is closed by user (in previous patch, it was called only when closed from WebCore).
I think I address all the other issues,
except this one which I don't really understand (neither the problem nor the solution).
> 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.

[WebKit Review Bot]

Attachment 181399 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1253:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1254:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1255:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1256:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1257:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1258:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1259:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1274:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1275:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1276:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1277:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1278:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1279:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.h"
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:114:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:116:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 15 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[kov's GTK+ EWS bot]

Comment on attachment 181399 [details]
new patch iteration

Attachment 181399 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15705669

[arno.]

(In reply to comment #25)
> (From update of attachment 181399 [details])

> ./webkit2gtk-scan: symbol lookup error: /srv/ews/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0: undefined symbol:_ZN6WebKit20WebColorChooserProxy16setSelectedColorERKN7WebCore5ColorE
> Scan failed: 

I don't understand this error. I cannot reproduce it, and I don't even have a webkit2gtk-scan binary.

[Martin Robinson]

(In reply to comment #26)
> (In reply to comment #25)
> > (From update of attachment 181399 [details] [details])
> 
> > ./webkit2gtk-scan: symbol lookup error: /srv/ews/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0: undefined symbol:_ZN6WebKit20WebColorChooserProxy16setSelectedColorERKN7WebCore5ColorE
> > Scan failed: 
> 
> I don't understand this error. I cannot reproduce it, and I don't even have a webkit2gtk-scan binary.

It's created-automatically during the gtkdoc generation step. Typically this means that there's a missing symbol in the library. What happens when you run MiniBrowser?

[arno.]

(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > (From update of attachment 181399 [details] [details] [details])
> > 
> > > ./webkit2gtk-scan: symbol lookup error: /srv/ews/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0: undefined symbol:_ZN6WebKit20WebColorChooserProxy16setSelectedColorERKN7WebCore5ColorE
> > > Scan failed: 
> > 
> > I don't understand this error. I cannot reproduce it, and I don't even have a webkit2gtk-scan binary.
> 
> It's created-automatically during the gtkdoc generation step. Typically this means that there's a missing symbol in the library. What happens when you run MiniBrowser?

MiniBrowser launches fine, and input color works correctly.

[Martin Robinson]

(In reply to comment #28)
> MiniBrowser launches fine, and input color works correctly.

Perhaps you are exposing _ZN6WebKit20WebColorChooserProxy16setSelectedColorERKN7WebCore5ColorE to the gtkdoc when it should be hidden. Take a look at the list of skipped files in Tools/gtk/generate-gtkdoc.

[arno.]

(In reply to comment #29)
> (In reply to comment #28)
> > MiniBrowser launches fine, and input color works correctly.
> 
> Perhaps you are exposing _ZN6WebKit20WebColorChooserProxy16setSelectedColorERKN7WebCore5ColorE to the gtkdoc when it should be hidden. Take a look at the list of skipped files in Tools/gtk/generate-gtkdoc.

In previous patch, I implemented WebColorChooserProxyGtk::setSelectedColor.
In the current patch, I don't implement it anymore (it's implemented in base class WebColorChooserProxy). Do I need to implement it ? Alternatively, could that be a bug in the bot where something would not have been cleaned up correctly since previous patch ?

[Martin Robinson]

The method seems to be in the header, so you'll probably need to add an empty implementation.

[arno.]

Created attachment 182856 [details]
try to add an empty implementation of WebColorChooserProxyGtk::setSelectedColor

[WebKit Review Bot]

Attachment 182856 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1253:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1254:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1255:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1256:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1257:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1258:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1259:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1274:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1275:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1276:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1277:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1278:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1279:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.h"
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:114:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:116:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 15 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[arno.]

Created attachment 188129 [details]
updated patch

> 
> 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.

This new patch fixes this issue: color chooser proxy calls webkitWebViewBaseShowColorChooser and webkitWebViewBase has a client from webkitWebView which it uses to notify that a color picker must be shown. I wonder if it would be better to display the gtk default color picker from webkitWebViewBase, and  webkitWebViewShowColorChooser to just emit a signal and return a boolean to tell webkitWebViewBase wether or not it should display the gtk default picker.

[WebKit Review Bot]

Attachment 188129 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.features.am.in', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp', u'Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.h', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/UIProcess/API/C/gtk/WKColorChooserClientGtk.cpp', u'Source/WebKit2/UIProcess/API/C/gtk/WKColorChooserClientGtk.h', u'Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserClient.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserClient.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequestPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBasePrivate.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebViewPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-docs.sgml', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk.types', u'Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp', u'Source/WebKit2/UIProcess/API/gtk/webkit2.h', u'Source/WebKit2/UIProcess/WebColorPickerResultListenerProxy.cpp', u'Source/WebKit2/UIProcess/WebColorPickerResultListenerProxy.h', u'Source/WebKit2/UIProcess/gtk/WebColorChooserClientGtk.cpp', u'Source/WebKit2/UIProcess/gtk/WebColorChooserClientGtk.h', u'Source/WebKit2/UIProcess/gtk/WebColorChooserProxyGtk.cpp', u'Source/WebKit2/UIProcess/gtk/WebColorChooserProxyGtk.h', u'Source/autotools/symbols.filter', u'Tools/ChangeLog', u'Tools/Scripts/webkitperl/FeatureList.pm']" exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1256:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1257:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1258:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1259:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1260:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1261:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1262:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1277:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1278:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1279:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1280:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1281:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1282:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.h"
Source/WebKit2/UIProcess/API/gtk/WebKitWebViewPrivate.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:114:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:116:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserClient.h"
Total errors found: 16 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[arno.]

Created attachment 200854 [details]
updated patch: fixes style issues

[Carlos Garcia Campos]

Comment on attachment 200854 [details]
updated patch: fixes style issues

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).

[arno.]

(In reply to comment #37)

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

The separate client was to (try to) implement WebColorChooserProxyGtk fallback. If we leave it for another patch, that makes the current patch much simpler.



> > 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

We need to store the request in the view private struct, because we need to delete in when close-color-chooser signal is emitted.


> > 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).

Right now, the ui client callbacks (showColorPicker and hideColorPicker) do not allow being notified when color is changed from JavaScript. Should I add such an api ?

[arno.]

Created attachment 203207 [details]
updated patch

[Carlos Garcia Campos]

(In reply to comment #38)
> (In reply to comment #37)
> 
> > 
> > Why do we need a separate client for the color chooser instead of using the UIClient?
> 
> The separate client was to (try to) implement WebColorChooserProxyGtk fallback. If we leave it for another patch, that makes the current patch much simpler.

Yes, that's the idea :-)

> 
> 
> > > 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
> 
> We need to store the request in the view private struct, because we need to delete in when close-color-chooser signal is emitted.
> 
> 
> > > 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).
> 
> Right now, the ui client callbacks (showColorPicker and hideColorPicker) do not allow being notified when color is changed from JavaScript. Should I add such an api ?

Yes, file a new bug report to implement that in the C API.

[Carlos Garcia Campos]

Comment on attachment 203207 [details]
updated patch

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;

[arno.]

Created attachment 204458 [details]
updated patch to address reviewer comments

[arno.]

(In reply to comment #40)

> > Right now, the ui client callbacks (showColorPicker and hideColorPicker) do not allow being notified when color is changed from JavaScript. Should I add such an api ?
> 
> Yes, file a new bug report to implement that in the C API.

I opened bug #117565 about it.
In case input value is changed from javascript, should we call gtk_color_chooser_set_rgba on the dialog ?
Also, should we change the request current color ? (I suppose we shouldn't, but I'm not sure)

[Carlos Garcia Campos]

Gustavo, Martin, could you review this, please?

[Martin Robinson]

Comment on attachment 204458 [details]
updated patch to address reviewer comments

View in context: https://bugs.webkit.org/attachment.cgi?id=204458&action=review

Looks good to me. :)

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:81
> +static void webkitColorChooserRequestGetProperty(GObject* object, guint propId, GValue* value, GParamSpec* paramSpec)

Tiny nit: propId -> propertyId.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1287
> +      * This signal is emitted when the user interacts with a <input
> +      * type='color' /> HTML element, requesting from WebKit to show
> +      * a dialog to select a color. To let the application know the details of
> +      * the color chooser, as well as to allow the client application to either
> +      * cancel the request or perform an actual color selection, the signal will
> +      * pass an instance of the #WebKitColorChooserRequest in the @request
> +      * argument.

It seems possible to handle this request asynchronously by increasing the reference count of the request. It might be a good idea to mention that explicitly.

[arno.]

Created attachment 206558 [details]
patch to adress martin's comments

[arno.]

Created attachment 206569 [details]
oups. This patch actually applies on trunk

[Carlos Garcia Campos]

Anders, Sam, could you take a look at the cross platform bits in this patch, please?

[Carlos Garcia Campos]

Our API freeze is too close (it's actually today but I'm going to delay it one week), I would like to land patches adding new API during this week. Arnaud, I think the internal C API classes have been renamed/refactored recently, so the patch might need to be rebased (sorry). WebKit2 owners, please, it would be great if you could review the cross-platform changes.

[Carlos Alberto Lopez Perez]

I have discovered via bug 136257 that the GTK port still don't supports <input type=color>.

A year has passed since the last update on the bug. The patch (as expected) not longer applies.

[Carlos Alberto Lopez Perez]

(In reply to comment #47)
> A year has passed since the last update on the bug. The patch (as expected)
> not longer applies.

Arnaud, do you plan on updating the patch for current trunk?

[arno.]

I won't have time for that in the near future, sorry.

[Carlos Garcia Campos]

Created attachment 246277 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 246277 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=246277&action=review

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:143
> +     * color input element is dettached from the page.

Dettached as in removed from the DOM?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:655
> +    webkitWebViewBaseSetColorPickerFactoryFunction(webViewBase, [](WebPageProxy& page, const Color& color, const IntRect& rect) {
> +        return WebKitColorChooser::create(page, color, rect);
> +    });

The API looks good to me, I just couldn't grasp the goal of allowing to override the factory function and in what cases it would not be overriden, leading to the default ColorPickerGtk to be created. Why not use WebKitColorChooser?

[Carlos Garcia Campos]

(In reply to comment #54)
> Comment on attachment 246277 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246277&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:143
> > +     * color input element is dettached from the page.
> 
> Dettached as in removed from the DOM?

Yes.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:655
> > +    webkitWebViewBaseSetColorPickerFactoryFunction(webViewBase, [](WebPageProxy& page, const Color& color, const IntRect& rect) {
> > +        return WebKitColorChooser::create(page, color, rect);
> > +    });
> 
> The API looks good to me, I just couldn't grasp the goal of allowing to
> override the factory function and in what cases it would not be overriden,
> leading to the default ColorPickerGtk to be created. Why not use
> WebKitColorChooser?

When using the C API.

[Carlos Garcia Campos]

(In reply to comment #55)
> (In reply to comment #54)
> > Comment on attachment 246277 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=246277&action=review
> > 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:143
> > > +     * color input element is dettached from the page.
> > 
> > Dettached as in removed from the DOM?
> 
> Yes.
> 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:655
> > > +    webkitWebViewBaseSetColorPickerFactoryFunction(webViewBase, [](WebPageProxy& page, const Color& color, const IntRect& rect) {
> > > +        return WebKitColorChooser::create(page, color, rect);
> > > +    });
> > 
> > The API looks good to me, I just couldn't grasp the goal of allowing to
> > override the factory function and in what cases it would not be overriden,
> > leading to the default ColorPickerGtk to be created. Why not use
> > WebKitColorChooser?
> 
> When using the C API.

The problem is always the same, that WebKitWebViewBase (which is the only thing the C API knows about) knows nothing about the WebKitWebView and the hook between WebKit2 core and the API is PageClientImpl::createColorPicker(). PageClientImpl was not inside API dir initially, and I think that's why it only knew about WebKitWebViewBase, but there's currently no reason why it can't know about WebKitWebView as well. So, we could probably simply this case (and the download request one), by checking in PageClientImpl whether m_webView is a WebKitWebView or not (then it can only be a WebKitWebViewBase). I can do this cleanup and simplify the download request case in a follow up patch.

[Carlos Garcia Campos]

Created attachment 246382 [details]
Updated patch

Rebased, and updated to remove the factory function trick.

[Gustavo Noronha (kov)]

Comment on attachment 246382 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=246382&action=review

> Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:143
> +     * color input element is dettached from the page.

Ah, I would prefer this changed to removed from the DOM for clarity if you're not opposed.

[Carlos Garcia Campos]

(In reply to comment #58)
> Comment on attachment 246382 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246382&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitColorChooserRequest.cpp:143
> > +     * color input element is dettached from the page.
> 
> Ah, I would prefer this changed to removed from the DOM for clarity if
> you're not opposed.

Sure! I'll change it before landing.

[Carlos Garcia Campos]

Committed r179984: <http://trac.webkit.org/changeset/179984>