[webkit-reviews] review denied: [Bug 124126] [GTK] Support right-side attachment of the inspector : [Attachment 218688] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Gustavo Noronha (kov)
<gns at gnome.org>'s request for review:
Bug 124126: [GTK] Support right-side attachment of the inspector
https://bugs.webkit.org/show_bug.cgi?id=124126

Attachment 218688: Patch
https://bugs.webkit.org/attachment.cgi?id=218688&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
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.