Bug 178655

Summary: [WPE] webkit_web_view_new() should enable specifying wpe_view_backend object
Product: WebKit Reporter: Zan Dobersek <zan>
Component: WPE WebKitAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Normal CC: aperez, bugs-noreply, cgarcia, ews-watchlist, mcatanzaro, rniwa
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 173770, 178894    
Attachments:
Description Flags
Patch
none
Fix GTK build
mcatanzaro: review+
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 none

Description Zan Dobersek 2017-10-23 01:29:18 PDT
The webkit_web_view_new() entrypoint, or any related webkit_web_view_new_with_*() entrypoint, doesn't make it possible to specify the wpe_view_backend object that should be used internally. This was perfectly possible with WKViewCreateWithViewBackend(), and that functionality should stay available with the new GLib API.
Comment 1 Carlos Garcia Campos 2017-10-23 23:39:48 PDT
Right. We just need to figure out how to expose WPE lib objects in the API, because libwpe is not introspectable and it doesn't have documentation either. How does the user know which bakends are available? how to create them? who owns it once passed to yhe new function? should it be manually freed after being passed? or when the web view is destroyed? etc.
Comment 2 Zan Dobersek 2017-10-24 02:25:49 PDT
(In reply to Carlos Garcia Campos from comment #1)
> Right. We just need to figure out how to expose WPE lib objects in the API,
> because libwpe is not introspectable and it doesn't have documentation
> either.

I don't think we should treat introspection support as a priority.

As far as C headers go, forward-declaring struct wpe_view_backend type is enough.

> How does the user know which bakends are available? how to create them?

That depends on what platform-specific WPEBackend implementation (which provides its own headers) the user is leveraging.

> who owns it once passed to yhe new function? should it be manually
> freed after being passed? or when the web view is destroyed? etc.

If it's passed to the constructor, ownership is not passed. If WPE::View has to load the interface-implementing object manually, then it's owned by that WPE::View object. Note this isn't currently implemented in such a manner, and should be fixed.
Comment 3 Carlos Garcia Campos 2017-10-24 02:49:54 PDT
(In reply to Zan Dobersek from comment #2)
> (In reply to Carlos Garcia Campos from comment #1)
> > Right. We just need to figure out how to expose WPE lib objects in the API,
> > because libwpe is not introspectable and it doesn't have documentation
> > either.
> 
> I don't think we should treat introspection support as a priority.

It's not a priority, but we should take it into account when designing the API.

> As far as C headers go, forward-declaring struct wpe_view_backend type is
> enough.

I think public headers should include the wpe headers, except the platform-specific ones, of course.

> > How does the user know which bakends are available? how to create them?
> 
> That depends on what platform-specific WPEBackend implementation (which
> provides its own headers) the user is leveraging.
>
> > who owns it once passed to yhe new function? should it be manually
> > freed after being passed? or when the web view is destroyed? etc.
> 
> If it's passed to the constructor, ownership is not passed. If WPE::View has
> to load the interface-implementing object manually, then it's owned by that
> WPE::View object. Note this isn't currently implemented in such a manner,
> and should be fixed.

We need to clarify that. But my point in general is that if we expose wpe objects in our public API, wpe libraries should be documented too.
Comment 4 Michael Catanzaro 2017-10-24 18:38:28 PDT
(In reply to Carlos Garcia Campos from comment #3)
> We need to clarify that. But my point in general is that if we expose wpe
> objects in our public API, wpe libraries should be documented too.

gtk-doc and introspection support for WPEBackends are both high on my short-term TODO list. They go hand in hand, in fact: if the docs are done correctly, then adding introspection should be very little additional effort. It's good marketing for us, and it's also the right time to validate that our API is not going to require changes to make introspectable.
Comment 5 Zan Dobersek 2017-10-25 00:54:00 PDT
(In reply to Zan Dobersek from comment #2)
> If it's passed to the constructor, ownership is not passed. If WPE::View has
> to load the interface-implementing object manually, then it's owned by that
> WPE::View object. Note this isn't currently implemented in such a manner,
> and should be fixed.

Bug #178773.
Comment 6 Michael Catanzaro 2017-11-07 19:33:37 PST
I see two options here. The simple option is to add a new new function:

WebKitWebView *webkit_web_view_new_with_backend (wpe_view_backend *backend);

and add a corresponding construct-only property, gpointer view-backend.

The alternative is to wrap wpe_view_backend in a GBoxed WebKitViewBackend. Then the new new function take a WebKitViewBackend* instead of a wpe_view_backend*, and the property would be a WebKitViewBackend* instead of a gpointer.

Either way, a new property is surely required, and that implies the WebKitWebView must own its view backend (to be discussed in bug #178773).
Comment 7 Carlos Garcia Campos 2017-11-17 01:32:54 PST
Created attachment 327156 [details]
Patch
Comment 8 Carlos Garcia Campos 2017-11-17 01:57:50 PST
Created attachment 327157 [details]
Fix GTK build
Comment 9 Zan Dobersek 2017-11-17 02:38:15 PST
LGTM.
Comment 10 EWS Watchlist 2017-11-17 03:11:49 PST
Comment on attachment 327157 [details]
Fix GTK build

Attachment 327157 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5271803

New failing tests:
http/tests/appcache/offline-access.html
Comment 11 EWS Watchlist 2017-11-17 03:11:50 PST
Created attachment 327161 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 12 Michael Catanzaro 2017-11-17 08:03:47 PST
Comment on attachment 327157 [details]
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!
Comment 13 Carlos Garcia Campos 2017-11-17 08:25:30 PST
Comment on attachment 327157 [details]
Fix GTK build

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

>> Source/WebKit/UIProcess/API/wpe/WebKitWebView.h:232
>> +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.

The thing is that the backend is not optional, but you can pass null. I know, in practice it's the same, but that's why I added it explicitly, to make it clear that it's not optional. I also considered to add a webkit_web_view_backend_new_default() and not allow null in constructors, but it not as convenient as allowing null.

>> Source/WebKit/UIProcess/API/wpe/WebKitWebViewBackend.cpp:54
>> +    }
> 
> I think this constructor is unused, right? You should delete it.

See webkitWebViewBackendCreateDefault() below.

>> 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?

I guess it's implied because I've never seen nullable in a user data parameter.

>> 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.

Ok.
Comment 14 Michael Catanzaro 2017-11-17 10:08:27 PST
(In reply to Carlos Garcia Campos from comment #13)
> The thing is that the backend is not optional, but you can pass null. I
> know, in practice it's the same, but that's why I added it explicitly, to
> make it clear that it's not optional. I also considered to add a
> webkit_web_view_backend_new_default() and not allow null in constructors,
> but it not as convenient as allowing null.

I don't understand... allowing NULL means it is most definitely optional.

Anyway, it's fine.

> >> Source/WebKit/UIProcess/API/wpe/WebKitWebViewBackend.cpp:54
> >> +    }
> > 
> > I think this constructor is unused, right? You should delete it.
> 
> See webkitWebViewBackendCreateDefault() below.

Ah, OK.
Comment 15 Carlos Garcia Campos 2017-11-20 00:16:34 PST
Committed r225044: <https://trac.webkit.org/changeset/225044>