Bug 124126 - [GTK] Support right-side attachment of the inspector
: [GTK] Support right-side attachment of the inspector
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Gustavo Noronha (kov)
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-11-10 12:15 PST by Gustavo Noronha (kov)
Modified: 2013-12-11 08:37 PST (History)
6 users (show)

See Also:


Attachments
Patch (13.54 KB, patch)
2013-11-10 12:22 PST, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
Patch (25.06 KB, patch)
2013-12-08 07:53 PST, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
Patch (14.14 KB, patch)
2013-12-11 03:26 PST, Gustavo Noronha (kov)
cgarcia: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gustavo Noronha (kov) 2013-11-10 12:15:07 PST
[GTK] Support right-side attachment of the inspector
Comment 1 Gustavo Noronha (kov) 2013-11-10 12:22:54 PST
Created attachment 216533 [details]
Patch
Comment 2 WebKit Commit Bot 2013-11-10 12:25:28 PST
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
Comment 3 Carlos Garcia Campos 2013-11-11 00:56:19 PST
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?
Comment 4 Gustavo Noronha (kov) 2013-12-08 07:53:26 PST
Created attachment 218688 [details]
Patch
Comment 5 Carlos Garcia Campos 2013-12-10 03:49:10 PST
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.
Comment 6 Gustavo Noronha (kov) 2013-12-11 03:26:45 PST
Created attachment 218948 [details]
Patch
Comment 7 Gustavo Noronha (kov) 2013-12-11 08:37:53 PST
Committed r160435: <http://trac.webkit.org/changeset/160435>