Bug 19392 – [Gtk] Enable WebInspector in the Gtk port

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Jan Alonzo]

The Gtk port is currently missing a WebInspector. It'd be nice if it has one.

[Gustavo Noronha (kov)]

Created attachment 22170 [details]
initial implementation of enabling the inspector

I did not include a changelog entry yet, since this is still a bit initial, though I believe some review would be important anyway. From my initial tests, it seems like enabling the inspector will help us uncover a bunch of javascript-related bugs in webkit/gtk+ - I had a number of segfaults while toying around with it in GtkLauncher and Midori.

[Gustavo Noronha (kov)]

Created attachment 22171 [details]
simple test code for the web inspector in GtkLauncher

If you want to play with the inspector, here's a patch to GtkLauncher.

[Jan Alonzo]

Hi gustavo, just a few suggestions

* Please don't include the GIO file crasher patch 
* Kindly fix your ChangeLog.
* create-inspector-web-view doesn't seem right. How about inspect-web-view because isn't that what we're doing here?

Apart from that looks good. (Not sure if we should have a WebKitWebInspector class though. Mac and win ports have it so maybe we should mimic one as well) 

[arno.]

I tried your patch, and it's nice to have inspector with gtk :)

 But there is a problem with m_webView. When inspector window is closed, it's inner Page object is deleted, but m_webView is not reset. That causes multiple problems:

 go to a web page, open inspector, close inspector, open inspector, inspector won't open.

 That's because in InspectorClient::createPage
 +    if (m_webView)
 +      return core(m_webView);
 will return a reference to a deleted page.

Also, launching GtkLauncher, opening inspector, and closing GtkLauncher will result in a crash in InspectorClient::closeWindow at line 

+ g_signal_emit_by_name(GTK_WIDGET(m_webView), "delete-event");

[Gustavo Noronha (kov)]

(In reply to comment #4)
> I tried your patch, and it's nice to have inspector with gtk :)

hey! thanks, and thanks for the comments! that was a first go, and I'm currently refactoring the patch to add a WebKitWebInspector class, and I'll try to address the problems you point; expect it attached here tonight or tomorrow =)

[arno.]

Another small thing:
If I understand correctly, shouldn't 

 core(m_webView)->inspectorController()->setWindowVisible(true); 

be

 core(m_inspectedWebView)->inspectorController()->setWindowVisible(true); 

? If I make that change, I get for example focus on selected node (which I don't otherwise)


> hey! thanks, and thanks for the comments! that was a first go, and I'm
> currently refactoring the patch to add a WebKitWebInspector class

Great, I'll test your next patch :)

[Gustavo Noronha (kov)]

(In reply to comment #6)
> Another small thing:
> If I understand correctly, shouldn't 
> 
>  core(m_webView)->inspectorController()->setWindowVisible(true); 
> 
> be
> 
>  core(m_inspectedWebView)->inspectorController()->setWindowVisible(true); 

OMGWTF! yeah, you're quite right! thanks =)

> ? If I make that change, I get for example focus on selected node (which I
> don't otherwise)
> 
> 
> > hey! thanks, and thanks for the comments! that was a first go, and I'm
> > currently refactoring the patch to add a WebKitWebInspector class
> 
> Great, I'll test your next patch :)
> 

[Christian Dywan]

Comment on attachment 22170 [details]
initial implementation of enabling the inspector

(Clearing review flag, the patch is being revised anyway.)

>+    webkit_web_view_open(m_webView, "file://"DATA_DIR"/webkit-1.0/webinspector/inspector.html");
>+
>+    return core(m_webView);

Please use g_get_user_data_dir and g_get_system_data_dirs to find the right location of the data files. And I don't think the folder should be versioned.

> void InspectorClient::attachWindow()
> {
>-    notImplemented();
>+    /* I don't think we should let a widget inside the inspector deal
>+       with this for WebKit/GTK+ */
> }

I wonder if the comment should actually say what "this" means.

>+    /**
>+    * WebKitWebSettings:enable-developer-extras:
>+    *
>+    * Whether developer extensions should be enabled.
>+    *
>+    * Since: 1.0.2
>+    */
>+    g_object_class_install_property(gobject_class,
>+                                    PROP_ENABLE_DEVELOPER_EXTRAS,
>+                                    g_param_spec_boolean(
>+                                    "enable-developer-extras",
>+                                    "Enable Developer Extras",
>+                                    "Enables special extensions that help developers",
>+                                    FALSE,
>+                                    flags));

That property name feels a little awkward. Would enable-debug do it as well?

>+    /**
>+     * WebKitWebView::create-inspector-web-view:
>+     * @web_view: the object on which the signal is emitted
>+     * @return: a newly allocated #WebKitWebView or %NULL
>+     *
>+     * Emitted when the creation of a new window for the Web Inspector
>+     * is requested.  If this signal is handled the signal handler
>+     * should return the newly created #WebKitWebView.
>+     *
>+     * The signal handlers should not try to deal with the reference
>+     * count for the new #WebKitWebView. The widget to which the
>+     * widget is added will handle that.
>+     */

> * create-inspector-web-view doesn't seem right. How about inspect-web-view
> because isn't that what we're doing here?

I tend to disagree. There is a web view being created, and the name should reflect that. 'inspect-web-view' looks like something that operates on an existing inspector. However I wonder if the name can be a little shorter, for instance would create-inspector be a good name? On the other hand, it might be less clear, not sure about that.

> Apart from that looks good. (Not sure if we should have a WebKitWebInspector
> class though. Mac and win ports have it so maybe we should mimic one as well) 

What is the purpose of a WebKitWebInspector class here? Right now you can load the inspector into an arbitrary web view, including a sub class. But that won't work if the inspector is a different class.
I'd like to see an explanation of why we need another class.

[Jan Alonzo]

Hi Christian,

(In reply to comment #8)
> >+    /**
> >+    * WebKitWebSettings:enable-developer-extras:
> >+    *
> >+    * Whether developer extensions should be enabled.
> >+    *
> >+    * Since: 1.0.2
> >+    */
> >+    g_object_class_install_property(gobject_class,
> >+                                    PROP_ENABLE_DEVELOPER_EXTRAS,
> >+                                    g_param_spec_boolean(
> >+                                    "enable-developer-extras",
> >+                                    "Enable Developer Extras",
> >+                                    "Enables special extensions that help developers",
> >+                                    FALSE,
> >+                                    flags));
> 
> That property name feels a little awkward. Would enable-debug do it as well?

Mac and Win call this "developer extras". Calling this enable-debug or diverging for that matter would be confusing especially if we are going to use the Mac and Win ports as our reference implementation.

> >+    /**
> >+     * WebKitWebView::create-inspector-web-view:
> >+     * @web_view: the object on which the signal is emitted
> >+     * @return: a newly allocated #WebKitWebView or %NULL
> >+     *
> >+     * Emitted when the creation of a new window for the Web Inspector
> >+     * is requested.  If this signal is handled the signal handler
> >+     * should return the newly created #WebKitWebView.
> >+     *
> >+     * The signal handlers should not try to deal with the reference
> >+     * count for the new #WebKitWebView. The widget to which the
> >+     * widget is added will handle that.
> >+     */
> 
> > * create-inspector-web-view doesn't seem right. How about inspect-web-view
> > because isn't that what we're doing here?
> 
> I tend to disagree. There is a web view being created, and the name should
> reflect that. 'inspect-web-view' looks like something that operates on an
> existing inspector. However I wonder if the name can be a little shorter, for
> instance would create-inspector be a good name? On the other hand, it might be
> less clear, not sure about that.

To me "inspect-web-view" means we are inspecting a/the web-view. I agree that there is a web view being created, but that web view is "special" because rather than displaying normal content like most web views, it displays information about a web view's content. 

Gtk events, for example, means something happened (a widget was clicked, toggled, about to be destroyed, etc..). We should do the same thing here (e.g. a web view is about to be inspected, whatever). Creating an inspector is less clear as by itself, an inspector is really not stand-alone but is associated with/inspecting a web view.

Conceptually, there should be difference between a WebView and a WebInspector (which is a web view + other things that developers care about).

> > Apart from that looks good. (Not sure if we should have a WebKitWebInspector
> > class though. Mac and win ports have it so maybe we should mimic one as well) 
> 
> What is the purpose of a WebKitWebInspector class here? Right now you can load
> the inspector into an arbitrary web view, including a sub class. But that won't
> work if the inspector is a different class.
> I'd like to see an explanation of why we need another class.
> 

I'm not sure about this either but one thing that comes to mind is testability of the inspector. Perhaps when there's a use case we can add it or when we have actual tests.

Cheers.

[Gustavo Noronha (kov)]

Created attachment 22282 [details]
My refactored patch

Here's the code. I have seen the latest comments, but I'd like to know which of them still stand by looking at the new approach, if you guys have the time =).

I'll take a look at using g_get_system_data_dirs. The directory must be versioned, IMO, so that we can have multiple versions of WebKit installed at once, to ease transitions when we decide upon API/ABI breakage.

[Gustavo Noronha (kov)]

Created attachment 22283 [details]
updated patch for GtkLauncher, just for testing

[Holger Freyther]

Please take a look at the coding style. Do not use NULL in C++, have early exits, place the curly braces according to the rules. Otherwise it mostly looks fine.

[Christian Dywan]

Comment on attachment 22282 [details]
My refactored patch

>+     * WebKitWebInspector::inspect-web-view:
>+     * @web_inspector: the object on which the signal is emitted
>+     * @web_view: the #WebKitWeb which will be inspected
>+     * @return: a newly allocated #WebKitWebView or %NULL
>+     *
>+     * Emitted when the user activates the 'inspect' context menu item
>+     * to inspect a web view. The application which is interested in
>+     * the inspector should create a window, or otherwise add the
>+     * #WebKitWebView it creates to an existing window.
>+     *
>+     * The #WebKitWebView must never be destroyed; that means that you
>+     * probably want to handle the delete-event and destory signals of
>+     * windows containing the inspector #WebKitWebView, so that you
>+     * can hide them when they are closed, and show them again when
>+     * ::show-window is emitted.

This is slightly wrong, when "destroy" is emitted it's already too late for saving the window, the only remaining option is to reparent the widget.

Would it be too complicated to work around the issue of a destroyed web view on the WebKit side? It should be possible, to unset the view via gtk_widget_destroyed and request a new web view as needed, shouldn't it?

>+     * WebKitWebInspector::attach-window:
>+     * @web_inspector: the object on which the signal is emitted
>+     * @return: %TRUE if the signal has been handled
>+     *
>+     * Emitted when the inspector window should be attached to the
>+     * window in which the #WebKitWebView it is inspecting is located.
>+     */
>+    webkit_web_inspector_signals[ATTACH_WINDOW] = g_signal_new("attach-window",
>+            G_TYPE_FROM_CLASS(klass),
>+            (GSignalFlags)(G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION),
>+            G_STRUCT_OFFSET (WebKitWebInspectorClass, attach_window),
>+            g_signal_accumulator_true_handled,
>+            NULL,
>+            webkit_marshal_BOOLEAN__VOID,
>+            G_TYPE_BOOLEAN , 0);

Could the documentation be a little clearer? To me "attaching to a window" doesn't intuitively make much sense.

>+     * WebKitWebInspector::uri-changed:
>+     * @web_inspector: the object on which the signal is emitted
>+     * @return: %TRUE if the signal has been handled
>+     *
>+     * Emitted when the URI being inspected changes.
>+     */
>+    webkit_web_inspector_signals[URI_CHANGED] = g_signal_new("uri-changed",
>+            G_TYPE_FROM_CLASS(klass),
>+            (GSignalFlags)(G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION),
>+            G_STRUCT_OFFSET (WebKitWebInspectorClass, uri_changed),
>+            g_signal_accumulator_true_handled,
>+            NULL,
>+            webkit_marshal_BOOLEAN__STRING,
>+            G_TYPE_BOOLEAN , 1,
>+            G_TYPE_STRING);

I suggest an URI property instead of a signal. The current patch doesn't seem to do anything with the boolean return value anyway.

>+    /**
>+     * WebKitWebInspector::inspector-destroyed:
>+     * @web_inspector: the object on which the signal is emitted
>+     * @return: %TRUE if the signal has been handled
>+     *
>+     * Emitted when the inspector is destroyed.
>+     */
>+    webkit_web_inspector_signals[INSPECTOR_DESTROYED] = g_signal_new("inspector-destroyed",
>+            G_TYPE_FROM_CLASS(klass),
>+            (GSignalFlags)(G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION),
>+            G_STRUCT_OFFSET (WebKitWebInspectorClass, inspector_destroyed),
>+            g_signal_accumulator_true_handled,
>+            NULL,
>+            webkit_marshal_BOOLEAN__VOID,
>+            G_TYPE_BOOLEAN , 0);

The boolean return value isn't used at all. Would it make sense to use GtkObject, gtk_object_destroy and "destroy" here? If not, what about s/inspector-destroyed/destroy, which is obvious enough as I see it.

>+    /*
>+     * properties
>+     */
>+    g_object_class_install_property(gobject_class, PROP_INSPECTOR_WEB_VIEW,
>+                                    g_param_spec_object("inspector-web-view",
>+                                                        "Inspector Web View",
>+                                                        "The Web View that renders the Web Inspector itself",
>+                                                        WEBKIT_TYPE_WEB_VIEW,
>+                                                        WEBKIT_PARAM_READWRITE));

What about s/inspector-web-view/web-view, since it's probably obvious enough.

>+     */
>+    klass->inspect_web_view = webkit_web_inspector_real_inspect_web_view;
>+    klass->show_window = webkit_web_inspector_real_show_window;
>+    klass->attach_window = webkit_web_inspector_real_attach_window;
>+    klass->dettach_window = webkit_web_inspector_real_dettach_window;
>+    klass->close_window = webkit_web_inspector_real_close_window;
>+    klass->uri_changed = webkit_web_inspector_real_uri_changed;
>+    klass->inspector_destroyed = webkit_web_inspector_real_inspector_destroyed;

Do we need all those defaults, most of which are empty placeholders?

>@@ -1206,6 +1212,13 @@ static void webkit_web_view_class_init(WebKitWebViewClass* webViewClass)
>                                                         WEBKIT_TYPE_WEB_SETTINGS,
>                                                         WEBKIT_PARAM_READWRITE));
> 
>+    g_object_class_install_property(objectClass, PROP_INSPECTOR,
>+                                    g_param_spec_object("inspector",
>+                                                        "Inspector",
>+                                                        "The associated WebKitWebInspector instance",
>+                                                        WEBKIT_TYPE_WEB_INSPECTOR,
>+                                                        WEBKIT_PARAM_READABLE));
>+

The property ought to be "web-inspector", right?

Looking at the code I see the use case of the WebInspector class and I think it's actually a good idea. I imagine you can keep one application wide instance around like you can with the WebSettings.

[Gustavo Noronha (kov)]

(In reply to comment #13)
> (From update of attachment 22282 [details] [edit])
> This is slightly wrong, when "destroy" is emitted it's already too late for
> saving the window, the only remaining option is to reparent the widget.
> 
> Would it be too complicated to work around the issue of a destroyed web view on
> the WebKit side? It should be possible, to unset the view via
> gtk_widget_destroyed and request a new web view as needed, shouldn't it?

I thought about doing it that way, but I can't remember why I decided going with the already existing WebKit 'workflow' would be better. I can implement that; I believe it would feel more natural indeed.

> Could the documentation be a little clearer? To me "attaching to a window"
> doesn't intuitively make much sense.

I decided to actually implement this signal because the 'attach' and 'dettach' commands are in the Inspector's HTML, and I'm not sure we will want to deviate from that UI.

I'm not sure how to improve that documentation, though. Perhaps something like 'Emitted when the inspector should appear at the same window as the #WebKitWebView being inspected.'?
 
> I suggest an URI property instead of a signal. The current patch doesn't seem
> to do anything with the boolean return value anyway.

That works for me. I added the boolean return values on everything to let the user create a number of signals, and decide on one of them to be the last.

> The boolean return value isn't used at all. Would it make sense to use
> GtkObject, gtk_object_destroy and "destroy" here? If not, what about
> s/inspector-destroyed/destroy, which is obvious enough as I see it.

I think the name change is in order, but I'm not sure about how I'd use GtkObject.
 
> What about s/inspector-web-view/web-view, since it's probably obvious enough.

oki!
 
> Do we need all those defaults, most of which are empty placeholders?

I am mostly mimicing WebKitWebView, which does have such empty placeholders, but I was on the brink of removing them. I was just worried that then we'de not have the members in the class structure, and may have problems if we want to add them later. I'm all for removing, though.
 
> The property ought to be "web-inspector", right?

Yeah, my bad.
 
> Looking at the code I see the use case of the WebInspector class and I think
> it's actually a good idea. I imagine you can keep one application wide instance
> around like you can with the WebSettings.

Right now you can't, since I intentionaly bound the creation and relation of the WebInspector instance to the creation of WebView, and provide no setter, only a getter, but that does look like a good thing to have. Coupled with the first modification (handling the Inspector WebView destroying) this would make the WebInspector very natural to use. I'll be quite busy at work and other real life issues today, but I'll try to have a new patch with most comments addressed by tomorrow night.

Thanks!

[Gustavo Noronha (kov)]

(In reply to comment #12)
> Please take a look at the coding style. Do not use NULL in C++, have early
> exits, place the curly braces according to the rules. Otherwise it mostly looks
> fine.

Hey! Shame on me for not having checked that. I'll have those changes in while addressing the comments from Christian! Thanks!

[Gustavo Noronha (kov)]

(In reply to comment #14)
> > the WebKit side? It should be possible, to unset the view via
> > gtk_widget_destroyed and request a new web view as needed, shouldn't it?
> 
> I thought about doing it that way, but I can't remember why I decided going
> with the already existing WebKit 'workflow' would be better. I can implement
> that; I believe it would feel more natural indeed.

While preparing to come to work today I was thinking about how to make this work, and remembered what I found to be the problem earlier: the InspectorController which interacts with the InspectorClient didn't seem to have an (at least obvious) way of redefining the page. I took a quick look at the code again right now, and think I may have found the Right Way of doing this.

I'm not sure WebCore will enjoy having multiple InspectorControllers pointing to the same inspector page, though, which will be needed to enable sharing WebKitWebInspector instances.

[Gustavo Noronha (kov)]

Created attachment 22362 [details]
proposed patch

This patch addresses most issues, I hope. It also depends on the fix for #19370, for the handling of the webview destruction to work (you will get a segfault otherwise). I am not sure still how to allow for the use of a single webinspector instance, so I haven't hacked that.

[Gustavo Noronha (kov)]

Created attachment 22363 [details]
updated GtkLauncher test patch

[Gustavo Noronha (kov)]

Created attachment 22364 [details]
proposed patch (forgot a commit on the other)

(copied from the old entry)

This patch addresses most issues, I hope. It also depends on the fix for
#19370, for the handling of the webview destruction to work (you will get a
segfault otherwise). I am not sure still how to allow for the use of a single
webinspector instance, so I haven't hacked that.

[Gustavo Noronha (kov)]

Created attachment 22365 [details]
correct launcher patch

[Christian Dywan]

Comment on attachment 22364 [details]
proposed patch (forgot a commit on the other)

>+    /**
>+     * WebKitWebInspector::destroy:
>+     * @web_inspector: the object on which the signal is emitted
>+     *
>+     * Emitted when the inspector is destroyed.
>+     */
>+    webkit_web_inspector_signals[DESTROY] = g_signal_new("destroy",
>+            G_TYPE_FROM_CLASS(klass),
>+            (GSignalFlags)(G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION),
>+            0,
>+            NULL,
>+            NULL,
>+            g_cclosure_marshal_VOID__VOID,
>+            G_TYPE_NONE , 0);

GtkObject, and therefore every GtkWidget, has a "destroy" signal with that same signature, that means "the object is disposed, with no way to stop that". But this signal merely means "finished" and it's still possible to keep a reference of the instance.
I suggest the signal should have a different name to avoid confusion about how this signal works.

>+static void webkit_web_inspector_finalize(GObject* object)
>+{
>+    WebKitWebInspector* web_inspector = WEBKIT_WEB_INSPECTOR(object);
>+    WebKitWebInspectorPrivate* priv = web_inspector->priv;
>+
>+    if (priv->inspector_view)
>+        g_object_unref(priv->inspector_view);
>+
>+    if (priv->inspected_uri)
>+        g_free(priv->inspected_uri);

No need to check whether a string array is non-NULL, g_free does that implicitly.

>+void webkit_web_inspector_set_web_view(WebKitWebInspector *web_inspector, WebKitWebView *web_view)
>+{
>+    g_return_if_fail(WEBKIT_IS_WEB_INSPECTOR(web_inspector));
>+    g_return_if_fail(WEBKIT_IS_WEB_VIEW(web_view));
>+
>+    WebKitWebInspectorPrivate* priv = web_inspector->priv;
>+
>+    if (priv->inspector_view)
>+        g_object_unref(priv->inspector_view);
>+    
>+    g_object_ref(web_view);
>+    priv->inspector_view = web_view;
>+}

No documentation at all here... Please add that.

And, would it be a good idea to allow _set_web_view with a NULL web_view to unset the web view?

>+void webkit_web_inspector_set_inspected_uri(WebKitWebInspector* web_inspector, const gchar* inspected_uri)
>+{
>+    g_return_if_fail(WEBKIT_IS_WEB_INSPECTOR(web_inspector));
>+
>+    WebKitWebInspectorPrivate* priv = web_inspector->priv;
>+
>+    if (priv->inspected_uri)
>+        g_free(priv->inspected_uri);
>+    
>+    priv->inspected_uri = g_strdup(inspected_uri);
>+}

Again, no need for the if (priv->inspected_uri).

>+    /**
>+    * WebKitWebSettings:enable-developer-extras:
>+    *
>+    * Whether developer extensions should be enabled.
>+    *
>+    * Since: 1.0.2
>+    */

I suggest you should mention the Web Inspector expressly here. Otherwise "special extensions" is rather meaningless to newcomers.

>@@ -1206,6 +1212,13 @@ static void webkit_web_view_class_init(WebKitWebViewClass* webViewClass)
>                                                         WEBKIT_TYPE_WEB_SETTINGS,
>                                                         WEBKIT_PARAM_READWRITE));
> 
>+    g_object_class_install_property(objectClass, PROP_WEB_INSPECTOR,
>+                                    g_param_spec_object("web-inspector",
>+                                                        "Web Inspector",
>+                                                        "The associated WebKitWebInspector instance",
>+                                                        WEBKIT_TYPE_WEB_INSPECTOR,
>+                                                        WEBKIT_PARAM_READABLE));

This needs documentation, particularly a Since tag.

>+WebKitWebInspector* webkit_web_view_get_inspector(WebKitWebView* webView)
>+{
>+    g_return_val_if_fail(WEBKIT_IS_WEB_VIEW(webView), NULL);
>+
>+    WebKitWebViewPrivate* priv = webView->priv;
>+    return priv->webInspector;
>+}

Documentation missing. Particularly it should be documented if this can be NULL.

[Gustavo Noronha (kov)]

(In reply to comment #21)
> GtkObject, and therefore every GtkWidget, has a "destroy" signal with that same
> signature, that means "the object is disposed, with no way to stop that". But
> this signal merely means "finished" and it's still possible to keep a reference
> of the instance.
> I suggest the signal should have a different name to avoid confusion about how
> this signal works.

I renamed it "finished". I hope this works better =).

> No documentation at all here... Please add that.

I added documentation, with Since tags.
 
> And, would it be a good idea to allow _set_web_view with a NULL web_view to
> unset the web view?

Not really. The web view creation process is related to the InspectorClient::createPage delegate, which is translated to the inspect-web-view signal; The view can only be sanely set there, as far as I know.

Just finishing up the patch.

[Gustavo Noronha (kov)]

Created attachment 23014 [details]
proposed patch

I believe all the latest comments have been addressed.

[Eric Seidel (no email)]

Comment on attachment 23014 [details]
proposed patch

Assigning to Alp for review or re-assignment to some other Gtk reviewer.  Pretty much all of the rest of us know nothing about gtk, so silly for this to sit in the general review queue.

[Timothy Hatcher]

Is it possible to remove the dist_webinspector_DATA and dist_webinspectorimages_DATA lists in the build file? We add new images and files a lot and it would be nice to not have to modify the GTK files each time. The Mac project just recursive copies the whole inspector folder.

[Holger Freyther]

Comment on attachment 23014 [details]
proposed patch

Pretty good. Some minor comments for the future:

Please use WebKitTools/Scripts/prepare-ChangeLog to generate the ChangeLog.

Your commenting style was really mixed, sadly we favor c++ comments (//), please try to be consistent.

In InspectorClient::inspectorDestroyed, it might be wise to initialize m_webInspector and then check for if (m_webInspector)..

And I also get this from the gobject runtime: (GtkLauncher:12425): GLib-GObject-WARNING **: /build/buildd/glib2.0-2.18.2/gobject/gsignal.c:2267: signal `destroy' is invalid for instance `0x8842b60'

[Holger Freyther]

(In reply to comment #26)

> And I also get this from the gobject runtime: (GtkLauncher:12425):
> GLib-GObject-WARNING **: /build/buildd/glib2.0-2.18.2/gobject/gsignal.c:2267:
> signal `destroy' is invalid for instance `0x8842b60'

Okay, this is likely to come from the GtkLauncher patch to connect to the non exisiting destroy signal.

[Holger Freyther]

Created attachment 24756 [details]
Rediffed patch, style changes, Makefile changes

I would be happy to land this patch. It contains some style changes, one variable initialisation and a GNUmakefile change to get every file from the inspector directory.

[Holger Freyther]

Comment on attachment 23014 [details]
proposed patch

Okay

[Holger Freyther]

Mark was so kind to review my additional changes so I could land this patch in r37982. Thanks a lot.