Bug 118431

I'm working on this.

Created attachment 218981 [details] Patch

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

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.

Created attachment 219751 [details] Patch

Comment on attachment 219751 [details] Patch This patch includes changes to old Autotools files. Removing from request queue.