REGRESSION(r224179): [GTK] Several WebViewEditor tests are failing since r224179

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Unexpected failures (9)
    /WebKit2Gtk/TestWebsiteData
        /webkit2/WebKitWebsiteData/cache
        /webkit2/WebKitWebsiteData/databases
        /webkit2/WebKitWebsiteData/resource-load-statistics
        /webkit2/WebKitWebsiteData/configuration
    /WebKit2Gtk/TestInspector
        /webkit2/WebKitWebInspector/default
        /webkit2/WebKitWebInspector/manual-attach-detach
    /WebKit2Gtk/TestWebViewEditor
        /webkit2/WebKitWebView/cut-copy-paste/non-editable
        /webkit2/WebKitWebView/editable/editable
        /webkit2/WebKitWebView/cut-copy-paste/editable

Unexpected timeouts (3)
    /WebCore/TestWebCore
        FileMonitorTest.DetectMultipleChanges
        FileMonitorTest.DetectChangeAndThenDelete
    /WebKit2Gtk/TestWebViewEditor
        /webkit2/WebKitWebView/editor-state/typing-attributes

The TestWebViewEditor ones were introduced in r224179

[Carlos Garcia Campos]

Created attachment 326200 [details]
Patch

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

[Michael Catanzaro]

Comment on attachment 326200 [details]
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.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #3)
> Comment on attachment 326200 [details]
> 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.

I pasted the current bot results, but this patch only fixes TestWebViewEditor, not the other ones, as I said in bug description.

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

It worked with native xorg, wayland and weston, but always failed with xvfb, with this patch it passes in all the cases.

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

No, it's TestWebViewEditor what is failing under xvfb.

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

[Carlos Garcia Campos]

Committed r224567: <https://trac.webkit.org/changeset/224567>