[webkit-reviews] review granted: [Bug 178655] [WPE] webkit_web_view_new() should enable specifying wpe_view_backend object : [Attachment 327157] Fix GTK build

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 178655: [WPE] webkit_web_view_new() should enable specifying
wpe_view_backend object
https://bugs.webkit.org/show_bug.cgi?id=178655

Attachment 327157: Fix GTK build

https://bugs.webkit.org/attachment.cgi?id=327157&action=review




--- Comment #12 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 327157
  --> https://bugs.webkit.org/attachment.cgi?id=327157
Fix GTK build

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

> Source/WebKit/ChangeLog:14
> +	   Update all WebKitWebView constructors to receive a
WebKitWebViewBackend as argument. It's now required to
> +	   provide a backend to create a web view, but it can be NULL to use
the default one. WebKitWebViewBackend is a
> +	   boxed type wrapping a struct wpe_view_backend* used as construct
only property of WebKitWebView. The view always
> +	   takes the ownership of the WebKitWebViewBackend which owns the
struct wpe_view_backend*. An optional
> +	   GDestroyNotify and user data pointer can be passed to the
WebKitWebViewBackend constructor to provide a custom
> +	   deleter for the backend. In the C API the struct wpe_view_backend*
is also mandatory now, but it can't be NULL
> +	   and it's owned by the caller, not the view.

Excellent plan.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:854
> +	* The #WebKoitWebViewBackend of the view.

Koit!

> Source/WebKit/UIProcess/API/wpe/WebKitWebView.h:232
> -webkit_web_view_new					(void);
> +webkit_web_view_new					(WebKitWebViewBackend  
   *backend);

I'm OK with doing this.

But I think I'd prefer to add a new webkit_web_view_new_with_backend()
constructor, and not add WebKitWebViewBackend parameters to all the other
constructors. As long as the backend is optional, I don't see the point in
making it special. And these constructors are pretty much never going to be
sufficient for constructing a WebKitWebView, because you're almost always going
to need to use g_object_new() anyway.

But this is fine too... either way, it does not matter very much.

> Source/WebKit/UIProcess/API/wpe/WebKitWebViewBackend.cpp:54
> +    _WebKitWebViewBackend()
> +	   : backend(wpe_view_backend_create())
> +	   ,
notifyCallback(reinterpret_cast<GDestroyNotify>(wpe_view_backend_destroy))
> +	   , notifyCallbackData(backend)
> +    {
> +    }

I think this constructor is unused, right? You should delete it.

> Source/WebKit/UIProcess/API/wpe/WebKitWebViewBackend.cpp:96
> + * @user_data: user data to pass to @notify

Does user_data not need to be marked (nullable) as well? I guess that's
implied?

> Source/WebKit/UIProcess/API/wpe/WebKitWebViewBackend.cpp:101
> + * The returned #WebKitWebViewBackend should never be freed by the user, it
must be passed to a

It's a comma splice. Change the comma to a semicolon, or split it into two
sentences.

> Source/WebKit/UIProcess/API/wpe/WebKitWebViewBackend.h:38
> +webkit_web_view_backend_get_type  (void);

(void) not aligned with the other function calls' parameters.

> Source/WebKit/UIProcess/API/wpe/WebKitWebViewBackend.h:44
> +WEBKIT_API struct wpe_view_backend*

Missing space:

wpe_view_backend *

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebView.cpp:131
> +    g_assert(!wpeBackend);

Maybe it was obvious to you, but I think this check was clever!