[webkit-reviews] review denied: [Bug 168219] [GTK] Expose editing command availability in WebKitHitTestResult : [Attachment 319080] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Michael Catanzaro
<mcatanzaro at igalia.com>'s request for review:
Bug 168219: [GTK] Expose editing command availability in WebKitHitTestResult
https://bugs.webkit.org/show_bug.cgi?id=168219

Attachment 319080: Patch

https://bugs.webkit.org/attachment.cgi?id=319080&action=review




--- Comment #21 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 319080
  --> https://bugs.webkit.org/attachment.cgi?id=319080
Patch

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

> Source/WebCore/rendering/HitTestResult.cpp:599
> -bool HitTestResult::allowsCopy() const
> +auto HitTestResult::allowedEditCommands() const -> EditCommands

You could use OptionSet instead of simple enum + static_cast

> Source/WebCore/rendering/HitTestResult.h:135
> +    enum EditCommands {

This should be an enum class AllowedEditCommands

> Source/WebKit/Shared/API/glib/WebKitHitTestResult.cpp:510
> + * webkit_hit_test_result_get_available_commands:

available commands sounds too generic, we are referring to editing commands.
I'm not sure about exposing an enum for this API here. I would rather expose
methods to query them. It's more boilerplate, but the API is less confusing.

> Source/WebKit/Shared/WebHitTestResultData.h:63
> +    unsigned allowedEditCommands;

This should also be an OptionSet

> Source/WebKit/UIProcess/API/gtk/WebKitHitTestResult.h:67
> + * WebKitHitTestResultCommands:

So the name of the enum sounds like it's going to list commands, but it lists
availability to run them

> Source/WebKit/UIProcess/API/gtk/WebKitHitTestResult.h:75
> + * Enum values with flags representing commands that can be performed at
> + * hit test locations.

This is only about editing commands

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:567
> +    // Copied from TestWebViewEditor. There's no way to know when the
selection
> +    // has been copied to the clipboard, so use a timeout source to query
the
> +    // clipboard. Note this function could quit early if some other text is
> +    // already present in the clipboard, so this function can't be relied on
to
> +    // actually copy the desired text before returning, but our test only
cares
> +    // that *something* is copied to the clipboard.
> +    webkit_web_view_execute_editing_command(test->m_webView,
WEBKIT_EDITING_COMMAND_COPY);

We could move this to WebViewTest and then do
test->waitUntilTextIsAvailableInClipboard().

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:584
> +    webkit_web_view_run_javascript(test->m_webView,
"document.getElementById('editable').select();", nullptr, [](GObject*
sourceObject, GAsyncResult* result, gpointer userData) {

Use test->runJavaScriptAndWaitUntilFinished()

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:593
> +	   auto mainLoop = static_cast<GMainLoop*>(userData);
> +	   auto webView = WEBKIT_WEB_VIEW(sourceObject);
> +	   GUniqueOutPtr<GError> error;
> +	   WebKitJavascriptResult* jsResult =
webkit_web_view_run_javascript_finish(webView, result, &error.outPtr());
> +	   g_assert_no_error(error.get());
> +	   webkit_javascript_result_unref(jsResult);
> +	   g_main_loop_quit(mainLoop);
> +    }, test->m_mainLoop);
> +    g_main_loop_run(test->m_mainLoop);

And you don't need to do all this.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:605
> +	   webkit_web_view_run_javascript(test->m_webView,
"document.getElementById('editable').value;", nullptr, [](GObject*
sourceObject, GAsyncResult* result, gpointer userData) {

test->runJavaScriptAndWaitUntilFinished()

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestContextMenu.cpp:621
> +	       JSGlobalContextRef context =
webkit_javascript_result_get_global_context(jsResult);
> +	       JSValueRef value = webkit_javascript_result_get_value(jsResult);
> +	       g_assert(JSValueIsString(context, value));
> +
> +	       JSStringRef jsStringValue = JSValueToStringCopy(context, value,
nullptr);
> +	       gsize stringLength =
JSStringGetMaximumUTF8CStringSize(jsStringValue);
> +	       GUniquePtr<char>
stringValue(static_cast<char*>(g_malloc(stringLength)));
> +	       JSStringGetUTF8CString(jsStringValue, stringValue.get(),
stringLength);
> +	       JSStringRelease(jsStringValue);
> +	       webkit_javascript_result_unref(jsResult);

test->javascriptResultToCString