[webkit-reviews] review denied: [Bug 118431] [GTK] Migrate WebKitDOMDOMWindow unit tests to WebKit2 : [Attachment 218981] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Enrique Ocaña
<eocanha at igalia.com>'s request for review:
Bug 118431: [GTK] Migrate WebKitDOMDOMWindow unit tests to WebKit2
https://bugs.webkit.org/show_bug.cgi?id=118431

Attachment 218981: Patch
https://bugs.webkit.org/attachment.cgi?id=218981&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
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.