Add autocleanups, for its own sake, and so derived classes (like EphyWebView) can use the G_DECLARE macros.
Created attachment 261993 [details] Patch
Comment on attachment 261993 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=261993&action=review How do the new cleanups header affect the gtk-doc? Should we filter them out in files_to_ignore() (generate-gtkdoc)? > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:904 > +#ifdef G_DEFINE_AUTOPTR_CLEANUP_FUNC > +G_DEFINE_AUTOPTR_CLEANUP_FUNC (${className}, g_object_unref) > +#endif Why not generate a WebKitDOMAutocleanups.h? it would be consistent with the other autocleanups headers. You could modify gobject-generate-headers.pl to receive an "autocleanup" header type. > Source/WebKit2/PlatformGTK.cmake:886 > +set(WebKit2GTK_GIR_HEADERS ${WebKit2GTK_INSTALLED_HEADERS}) > +list(REMOVE_ITEM WebKit2GTK_GIR_HEADERS ${WEBKIT2_DIR}/UIProcess/API/gtk/WebKitAutocleanups.h) Instead of this, we could add #ifndef __GI_SCANNER__ to the autocleanup headers. > Source/WebKit2/PlatformGTK.cmake:930 > +set(WebKit2WebExtension_GIR_HEADERS ${WebKit2WebExtension_INSTALLED_HEADERS}) > +list(REMOVE_ITEM WebKit2WebExtension_GIR_HEADERS > + ${WEBKIT2_DIR}/WebProcess/InjectedBundle/API/gtk/WebKitWebExtensionAutocleanups.h) Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitAutocleanups.h:78 > +G_DEFINE_AUTOPTR_CLEANUP_FUNC (WebKitUserStyleSheet, webkit_user_style_sheet_unref) hmm, it seems very easy to forget adding a new autoptr definition when adding a new type to the API. We should at least document it here http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API once this lands. > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestAutocleanups.cpp:25 > +static void testAutocleanups(Test* test, gconstpointer) testUIProcessAutocleanups > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestAutocleanups.cpp:45 > + Test::add("Autocleanup", "Autocleanups", testAutocleanups); Use lower case for the text case name. I would use "ui-procress" for example. > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestAutocleanups.cpp:47 > + // DOM autocleanups are tested as part of WebExtensionTest. Add them here better, create a web process test instead of using the web extensions one with a DBus message. See DOM*Test.cpp or FrameTest.cpp for an exmaple of how to create a web process test. > Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp:272 > + g_assert(page); WEBKIT_IS_WEB_PAGE(page) > Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp:274 > + g_object_add_weak_pointer(G_OBJECT(page), reinterpret_cast<void**>(&watchedPage)); In a web process test you can use assertObjectIsDeletedWhenTestFinishes() > Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp:278 > + g_assert(document); WEBKIT_DOM_IS_DOCUMENT(document) > Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp:284 > + g_assert(window); WEBKIT_DOM_IS_DOM_WINDOW(window) > Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp:290 > + g_assert(range); WEBKIT_DOM_IS_RANGE(range)
(In reply to comment #2) > How do the new cleanups header affect the gtk-doc? Should we filter them out > in files_to_ignore() (generate-gtkdoc)? It causes no problems for the gtk-doc, at least not that I have noticed. > Why not generate a WebKitDOMAutocleanups.h? it would be consistent with the > other autocleanups headers. You could modify gobject-generate-headers.pl to > receive an "autocleanup" header type. OK, I didn't realize how easy this would be. > Instead of this, we could add #ifndef __GI_SCANNER__ to the autocleanup > headers. I went looking for such a macro but didn't find it; thanks. > hmm, it seems very easy to forget adding a new autoptr definition when > adding a new type to the API. We should at least document it here > http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API once this lands. OK. We'll still forget, but it's not really a huge problem if we do; we'll just fix it once someone notices. > testUIProcessAutocleanups OK > Use lower case for the text case name. I would use "ui-procress" for example. OK > Add them here better, create a web process test instead of using the web > extensions one with a DBus message. See DOM*Test.cpp or FrameTest.cpp for an > exmaple of how to create a web process test. I'll try that. > WEBKIT_IS_WEB_PAGE(page) OK > In a web process test you can use assertObjectIsDeletedWhenTestFinishes() OK > > Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp:278 > > + g_assert(document); > > WEBKIT_DOM_IS_DOCUMENT(document) > > > Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp:284 > > + g_assert(window); > > WEBKIT_DOM_IS_DOM_WINDOW(window) > > > Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp:290 > > + g_assert(range); > > WEBKIT_DOM_IS_RANGE(range) OK
Created attachment 262281 [details] Patch
Comment on attachment 262281 [details] Patch Rubber stamping this. While I’m not an expert on GTK this looks fine.
Unless you want to wait for another review from Carlos.
Comment on attachment 262281 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262281&action=review Patch looks good to me, just fix my comments before landing. Sorry for the delay reviewing this. > Tools/TestWebKitAPI/Tests/WebKit2Gtk/AutocleanupsTest.cpp:24 > + We should guard this file too, because it uses g_autoptr that is only available since glib 2.44, but our minimim version supported is 2.36 > Tools/TestWebKitAPI/Tests/WebKit2Gtk/AutocleanupsTest.cpp:57 > + if (!strcmp(testName, "web-process-autocleanups")) web-process is redundant here. we can just use autocleanups for the test name. > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestAutocleanups.cpp:24 > + And this one too. > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestAutocleanups.cpp:46 > + test->loadHtml(testHTML, 0); 0 -> nullptr.
> We should guard this file too, because it uses g_autoptr that is only > available since glib 2.44, but our minimim version supported is 2.36 OK > web-process is redundant here. we can just use autocleanups for the test > name. OK > And this one too. OK > 0 -> nullptr. OK
I'm noticing these: /home/mcatanzaro/src/WebKit/Source/WebKit2/UIProcess/API/gtk/WebKitAutocleanups.h:33: the __GI_SCANNER__ constant should only be used with simple #ifdef or #endif: #if defined(G_DEFINE_AUTOPTR_CLEANUP_FUNC) && !defined(__GI_SCANNER__) So I've switched to this stupid construct: #ifdef G_DEFINE_AUTOPTR_CLEANUP_FUNC #ifndef __GI_SCANNER__ Just to avoid the warnings. The __GI_SCANNER__ is not added because it breaks anything, only because it silences a warning spew from the scanner. glib's autocleanups cause the same warning spew, so it's really not our problem, but it's always nice to avoid unnecessary warnings....
(In reply to comment #8) > > web-process is redundant here. we can just use autocleanups for the test > > name. > > OK > > > And this one too. > > OK Turns out it has to match the test name here: WebViewTest::add("Autocleanups", "ui-process-autocleanups", testUIProcessAutocleanups); WebViewTest::add("Autocleanups", "web-process-autocleanups", testWebProcessAutocleanups); Since there's an assert: ERROR:../../Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebProcessTest.cpp:53:static std::unique_ptr<WebProcessTest> WebProcessTest::create(const WTF::String&): assertion failed: (testsMap().contains(testName))
Created attachment 262574 [details] Patch
Committed r190660: <http://trac.webkit.org/changeset/190660>