Bug 149588 – [GTK] Add autocleanups

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

Add autocleanups, for its own sake, and so derived classes (like EphyWebView) can use the G_DECLARE macros.

[Michael Catanzaro]

Created attachment 261993 [details]
Patch

[Carlos Garcia Campos]

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)

[Michael Catanzaro]

(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

[Michael Catanzaro]

Created attachment 262281 [details]
Patch

[Darin Adler]

Comment on attachment 262281 [details]
Patch

Rubber stamping this. While I’m not an expert on GTK this looks fine.

[Darin Adler]

Unless you want to wait for another review from Carlos.

[Carlos Garcia Campos]

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.

[Michael Catanzaro]

> 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

[Michael Catanzaro]

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

[Michael Catanzaro]

(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))

[Michael Catanzaro]

Created attachment 262574 [details]
Patch

[Michael Catanzaro]

Committed r190660: <http://trac.webkit.org/changeset/190660>