[webkit-reviews] review granted: [Bug 50744] [GTK] Insta-crash when closing browser with inspector window opened : [Attachment 125598] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted Gustavo Noronha (kov)
<gns at gnome.org>'s request for review:
Bug 50744: [GTK] Insta-crash when closing browser with inspector window opened
https://bugs.webkit.org/show_bug.cgi?id=50744

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

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=125598&action=review


Great stuff! Just have a few minor nits below.

> Source/WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp:236
> +    GRefPtr<WebKitWebInspector> webInspector =
adoptGRef(m_webInspector.leakRef());

I think eventually we should do away with GRefPtr. RefPtr/PassRefPtr have so
many good features. For instance this would just be:

RefPtr<WebKitWebInspector> webInspector = m_webInspector.release();

> Source/WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp:239
> +	   g_signal_handlers_disconnect_by_func(m_inspectorWebView,
(gpointer)notifyWebViewDestroyed, (gpointer)this);

You shouldn't have to cast to gpointer here, I don't think. If it complains
about notifyWebViewDestroyed, you should use reinterpret_cast.

> Source/WebKit/gtk/tests/testwebinspector.c:27
> +#include "test_utils.h"
> +
> +#include <errno.h>
> +#include <unistd.h>
> +#include <string.h>
> +

You have a few extra newlines here.

> Source/WebKit/gtk/tests/testwebinspector.c:68
> +						gpointer data)
> +{
> +    int *beenThere = (int*)data;

Since you are already casting these functions, I think you can just use an int*
in the signature. For instance:

static WebKitWebView* inspectElementCallback(WebKitWebInspector *inspector,
WebKitWebView *inspectedWebView, int *timesElementInspected)

The signature should all be one line too.

> Source/WebKit/gtk/tests/testwebinspector.c:87
> +static gboolean closeInspector (WebKitWebInspector *inspector,
> +				   gpointer data)

Ditto.

> Source/WebKit/gtk/tests/testwebinspector.c:110
> +    int beenThere = 0;
> +    int closed = 0;

I'd suggest timesClosed and timesElementInspected or something like that.

> Source/WebKit/gtk/tests/testwebinspector.c:174
> +    g_test_add_func("/webkit/webinspector/destroy-inspected-web-view",
test_webkit_web_inspector_destroy_inspected_web_view);
> +    g_test_add_func("/webkit/webinspector/close_and_inspect",
test_webkit_web_inspector_close_and_inspect);

Perhaps the second test should be called close-and-inspect to match the naming
style of the other tests?