Bug 149588 - [GTK] Add autocleanups
Summary: [GTK] Add autocleanups
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Enhancement
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-09-27 16:05 PDT by Michael Catanzaro
Modified: 2015-10-06 21:08 PDT (History)
3 users (show)

See Also:


Attachments
Patch (23.21 KB, patch)
2015-09-27 17:18 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (24.31 KB, patch)
2015-10-01 13:17 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (24.43 KB, patch)
2015-10-06 21:07 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2015-09-27 16:05:21 PDT
Add autocleanups, for its own sake, and so derived classes (like EphyWebView) can use the G_DECLARE macros.
Comment 1 Michael Catanzaro 2015-09-27 17:18:49 PDT
Created attachment 261993 [details]
Patch
Comment 2 Carlos Garcia Campos 2015-09-28 00:10:24 PDT
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)
Comment 3 Michael Catanzaro 2015-09-29 18:43:45 PDT
(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
Comment 4 Michael Catanzaro 2015-10-01 13:17:53 PDT
Created attachment 262281 [details]
Patch
Comment 5 Darin Adler 2015-10-06 09:13:36 PDT
Comment on attachment 262281 [details]
Patch

Rubber stamping this. While Iā€™m not an expert on GTK this looks fine.
Comment 6 Darin Adler 2015-10-06 09:14:15 PDT
Unless you want to wait for another review from Carlos.
Comment 7 Carlos Garcia Campos 2015-10-06 10:20:25 PDT
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.
Comment 8 Michael Catanzaro 2015-10-06 11:11:32 PDT
> 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
Comment 9 Michael Catanzaro 2015-10-06 20:57:25 PDT
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....
Comment 10 Michael Catanzaro 2015-10-06 21:05:29 PDT
(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))
Comment 11 Michael Catanzaro 2015-10-06 21:07:44 PDT
Created attachment 262574 [details]
Patch
Comment 12 Michael Catanzaro 2015-10-06 21:08:56 PDT
Committed r190660: <http://trac.webkit.org/changeset/190660>