[webkit-reviews] review granted: [Bug 179366] REGRESSION(r224179): [GTK] Several WebViewEditor tests are failing since r224179 : [Attachment 326200] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 179366: REGRESSION(r224179): [GTK] Several WebViewEditor tests are failing
since r224179
https://bugs.webkit.org/show_bug.cgi?id=179366

Attachment 326200: Patch

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




--- Comment #3 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 326200
  --> https://bugs.webkit.org/attachment.cgi?id=326200
Patch

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

Hm. Problem #1 is I failed to run all the API tests. I only ran the
TestWebViewEditor tests. But I knew this change would break other API tests
unless I added calls to flush the editor state. I just forgot. Sorry.

Problem #2 is I did not test under X11. It looks like the TestWebViewEditor
tests are all currently passing on the Wayland bot. That's a bit surprising,
but I guess the editor state flush works differently in Wayland as it is
tightly coupled with the drawing code. My original flushEditorState() worked
quite reliably for me, but I had tons of failures when I tried to use
showInWindowAndWaitUntilMapped, so let's check the Wayland bot after this lands
to make sure it's OK.

Thanks for cleaning up after me.

> Source/WebKit/ChangeLog:11
> +	   In r224179, webkit_web_view_can_execute_editing_command() was
optimized to use the sync path for commands
> +	   supported by the WebViewEditorState, but the state requires a redraw
to be up to date. We can't know if
> +	   WebViewEditorState is in sync, when
webkit_web_view_can_execute_editing_command() is called, so we always need
> +	   to ask the web process.

It should work if we add more calls to flushEditorState() in all the API tests
that require it. So it really should not be necessary to remove this.

But I'm more comfortable with removing it, so OK. I only added it in the first
place because you requested it.

> Tools/TestWebKitAPI/Tests/WebKitGtk/TestWebViewEditor.cpp:44
> -	   return G_SOURCE_REMOVE;
> +	   return FALSE;

Oops.

> Tools/TestWebKitAPI/Tests/WebKitGtk/TestWebViewEditor.cpp:52
> +	   g_signal_handler_disconnect(m_webView, signalID);

Ooooooooops.