Bug 124126 – [GTK] Support right-side attachment of the inspector

• Component: New Bugs
• Status: RESOLVED FIXED

[Gustavo Noronha (kov)]

[GTK] Support right-side attachment of the inspector

[Gustavo Noronha (kov)]

Created attachment 216533 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

Comment on attachment 216533 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216533&action=review

> Source/WebKit2/UIProcess/API/C/gtk/WKInspectorClientGtk.h:51
> +    WKInspectorClientGtkDidChangeAttachedWidthCallback  didChangeAttachedWidth;

We should use this new callback in WebKitWebInspector to expose an attached-width property. And I guess we also need to expose somehow the AttachmentSide in the API. We can do it in a follow up patch, though, but in such case, add a NULL for the new callback in  webkitWebInspectorCreate

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:110
>      unsigned inspectorViewHeight;
> +    unsigned inspectorViewWidth;

These are mutually exclusive in the end, I wonder if we can use a single member inspectorViewSize and use it as width or height depending on inspectorAttachmentSide

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:311
> +    webViewBase->priv->inspectorAttachmentSide = attachmentSide;
> +
> +    if (webViewBase->priv->inspectorView == inspector) {
> +        gtk_widget_queue_resize(GTK_WIDGET(webViewBase));
> +        return;
> +    }

Should we check if the attachmentSide has actually changed in this case?

[Gustavo Noronha (kov)]

Created attachment 218688 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 218688 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=218688&action=review

We need also unit tests for the new API. Maybe we can split the patch and implement right side attachment for the default embedded web inspector view first, and then add the new api to allow users to do it as well.

> Source/WebKit2/UIProcess/API/C/gtk/WKInspectorClientGtk.h:62
> +typedef struct WKInspectorClientGtk WKInspectorClientGtk;
> +
> +enum { kWKInspectorClientGtkCurrentVersion = 0 };

I think we don't need these anymore with the versioned structs.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebInspector.cpp:150
> +     * The width that the inspector view should have when it is attached.
> +     */

Since: 2.4

> Source/WebKit2/UIProcess/API/gtk/WebKitWebInspector.cpp:163
> +     * The side the inspector view should be attached to.
> +     */

Since: 2.4

> Source/WebKit2/UIProcess/API/gtk/WebKitWebInspector.cpp:360
> +    WebKitWebInspector* inspector = WEBKIT_WEB_INSPECTOR(clientInfo);
> +    g_object_notify(G_OBJECT(inspector), "attachment-side");

Shouldn't we check it actually changed before emitting the notify signal? or this callback is only called when it has actually changed?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebInspector.cpp:506
> + */

Since: 2.4

> Source/WebKit2/UIProcess/API/gtk/WebKitWebInspector.cpp:512
> +    if (!inspector->priv->webInspector->isAttached())
> +        return WEBKIT_WEB_INSPECTOR_ATTACHMENT_SIDE_NONE;

I think we could return the actual value even if not attached, to know where to attach it no? or am I misunderstanding the API? Because this is not the attached side , but the attachment side

> Source/WebKit2/UIProcess/API/gtk/WebKitWebInspector.cpp:548
> + */

Since: 2.4

> Source/WebKit2/UIProcess/API/gtk/WebKitWebInspector.h:46
> + * @WEBKIT_WEB_INSPECTOR_ATTACHMENT_SIDE_NONE: The inspector should be detached.

I think I don't understand the NONE case. If it's only to be used as default, I would use bottom as default value, which I guess it's the default in webkit2

> Source/WebKit2/UIProcess/API/gtk/WebKitWebInspector.h:52
> + */

Since: 2.4

> Source/WebKit2/UIProcess/WebInspectorProxy.h:118
> +    AttachmentSide attachmentSide() const { return m_attachmentSide; }
> +

This is cross-platform change in wk2, I guess we need an owner.

> Source/WebKit2/UIProcess/gtk/WebInspectorClientGtk.h:56
> +    void didChangeAttachedWidth(WebInspectorProxy*, unsigned width);
>  };

What about didChangeAttachmentSide? it seems the C API callback is not called anywhere.

[Gustavo Noronha (kov)]

Created attachment 218948 [details]
Patch

[Gustavo Noronha (kov)]

Committed r160435: <http://trac.webkit.org/changeset/160435>