Bug 118431 – [GTK] Migrate WebKitDOMDOMWindow unit tests to WebKit2

• Status: ASSIGNED
• Component: Tools / Tests

[Carlos Garcia Campos]

ssia

[Enrique Ocaña]

I'm working on this.

[Enrique Ocaña]

Created attachment 218981 [details]
Patch

[WebKit Commit 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

[Carlos Garcia Campos]

Comment on attachment 218981 [details]
Patch

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

Thanks for the patch. I think you have focused too much on migrating the wk1 test, but we can adapt it to the constraints of the webkit2. See my comments below.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDOMWindowTest.cpp:27
> +class WebKitDOMDOMWindowTest;

Do you really need this forward declaration here?

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDOMWindowTest.cpp:29
> +static gboolean loadedCallback(WebKitDOMDOMWindow*, WebKitDOMEvent*, WebKitDOMDOMWindowTest*);
> +static gboolean clickedCallback(WebKitDOMDOMWindow*, WebKitDOMEvent*, WebKitDOMDOMWindowTest*);

Do not do this, add the callbacks as static members of the WebKitDOMDOMWindowTest class.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDOMWindowTest.cpp:54
> +        notify("ready", "");

The idea of these web process tests is that, ideally, the run on their own, without needing any interaction back to the ui process.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDOMWindowTest.cpp:62
> +        g_assert(domWindow);

g_assert(WEBKIT_DOM_IS_DOM_WINDOW(domWindow));

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDOMWindowTest.cpp:74
> +        // The "load" WebKitDOMDOMWindow signal is issued before this test is
> +        // called. There's no way to capture it here. We simply assume that
> +        // the document is loaded and notify the uiprocess accordingly
> +        // notify("loaded", "");
> +
> +        webkit_dom_event_target_add_event_listener(
> +            WEBKIT_DOM_EVENT_TARGET(domWindow),
> +            "load",
> +            G_CALLBACK(loadedCallback),
> +            false,
> +            this);

This test is about testing signals, not about testing the load signal specifically. If testing the load signal is a problem because of the way these tests are designed, choose any other signal to test.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDOMWindowTest.cpp:83
> +        g_assert(element);

g_assert(WEBKIT_DOM_IS_ELEMENT(element));

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDOMWindowTest.cpp:95
> +        webkit_dom_event_target_add_event_listener(
> +            WEBKIT_DOM_EVENT_TARGET(element),
> +            "click",
> +            G_CALLBACK(clickedCallback),
> +            false,
> +            this);
> +
> +        // The "click" action will be issued in the uiprocess and that will
> +        // trigger the dom event here.
> +        // clickedCallback() will stop this loop
> +        RunLoop::run();

I think we can merge the first too tests, so that we dispatch the event here and we check the callbacks set with webkit_dom_event_target_add_event_listener are called. This way we don't need interaction with the UI process.

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDOMWindowTest.cpp:110
> +        g_assert(domWindow);

Use always GObject macros

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDOMWindowTest.cpp:125
> +        // The "load" WebKitDOMDOMWindow signal is issued before this test is
> +        // called. There's no way to capture it here. We simply assume that
> +        // the document is loaded and notify the uiprocess accordingly
> +        // notify("loaded", "");
> +
> +        webkit_dom_event_target_add_event_listener(
> +            WEBKIT_DOM_EVENT_TARGET(domWindow),
> +            "load",
> +            G_CALLBACK(loadedCallback),
> +            false,
> +            this);
> +
> +        // loadedCallback() will stop this loop
> +        RunLoop::run();

Why do you need to connect to load again if the page has already been loaded at this point?

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDOMWindowTest.cpp:135
> +        g_assert(event);
> +        g_assert(WEBKIT_DOM_IS_EVENT(event));

GObject macros will fail when the pointer is NULL so the first assert is redundant

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDOMWindowTest.cpp:136
> +        g_assert(WEBKIT_DOM_IS_MOUSE_EVENT(event));

I think this one is enough, since a DOMMourseEVent is always a DOMEvent

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDOMWindowTest.cpp:170
> +        g_assert(domWindow);

Use GObject macros

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDOMWindowTest.cpp:173
> +        g_assert(element);
> +        g_assert(WEBKIT_DOM_IS_ELEMENT(element));

Same comment here, the first assert is redundant

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDOMWindowTest.cpp:206
> +    // Stop the loop and let testSignals() or testDispatchEvent() continue its course

Nit: Comments should finish with a period. Comment is probably redundant , though

> Source/WebKit2/UIProcess/API/gtk/tests/DOMDOMWindowTest.cpp:217
> +    // Stop the loop and let testSignals() or testDispatchEvent() continue its course

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/TestDOMDOMWindow.cpp:27
> +#define HTML_DOCUMENT "<html><head><title></title></head><style type='text/css'>#test { font-size: 16px; }</style><body><p id='test'>test</p></body></html>"

Use a static global var instead, or not global if it's only used in one place.

> Source/WebKit2/UIProcess/API/gtk/tests/TestDOMDOMWindow.cpp:34
> +typedef struct {
> +    gboolean loaded;
> +    gboolean clicked;
> +    WebProcessTestRunner* testRunner;
> +    WebViewTest* test;
> +} DomDomWindowTestStatus;

I hope we don't need all thse, this file should just launch the web proces tests like TestFrame does, for instance.

> Source/WebKit2/UIProcess/API/gtk/tests/WebProcessTest.cpp:46
> +void WebProcessTest::notify(const char* key, const char* value)

And we won't need this signal either. In any case, this is specific to a test, so I think it should be done there, instead of here in case it's still needed.

[Enrique Ocaña]

Created attachment 219751 [details]
Patch

[Michael Catanzaro]

Comment on attachment 219751 [details]
Patch

This patch includes changes to old Autotools files. Removing from request queue.