[Webkit-unassigned] [Bug 19392] [Gtk] Enable WebInspector in the Gtk port

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jul 14 14:27:16 PDT 2008


christian at imendio.com changed:

           What    |Removed                     |Added
  Attachment #22170|review?                     |
               Flag|                            |

------- Comment #8 from christian at imendio.com  2008-07-14 14:27 PDT -------
(From update of attachment 22170)
(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.

Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

More information about the webkit-unassigned mailing list