[webkit-reviews] review cancelled: [Bug 19392] [Gtk] Enable WebInspector in the Gtk port : [Attachment 22170] initial implementation of enabling the inspector

[bugzilla-daemon at webkit.org]

Christian Dywan <christian at imendio.com> has cancelled Gustavo Noronha Silva
<gns at gnome.org>'s request for review:
Bug 19392: [Gtk] Enable WebInspector in the Gtk port
https://bugs.webkit.org/show_bug.cgi?id=19392

Attachment 22170: initial implementation of enabling the inspector
https://bugs.webkit.org/attachment.cgi?id=22170&action=edit

------- Additional Comments from Christian Dywan <christian at imendio.com>
(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.