Bug 68085 – [GTK] Loader client implementation for WebKit2 GTK+ API

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

WebKitWebView doesn't implement loader client.

[Carlos Garcia Campos]

Created attachment 107339 [details]
Patch

[Carlos Garcia Campos]

Created attachment 107985 [details]
New patch

- It still uses the glib/gio async pattern, but it removes the load progress callback
- It adds load-progress-changed and load-status-changed signals to notify about the load progress. I'm using signals instead of properties in this case, because progress and load-status only make sense while the view is loading, so they are passed as parameters to the signal callbacks, but they are not saved and there's no API to get them.
- It adds is-loading property.
- It adds all API docs missing in previous patch.

[Martin Robinson]

(In reply to comment #2)
> Created an attachment (id=107985) [details]
> New patch
> 
> - It still uses the glib/gio async pattern, but it removes the load progress callback
> - It adds load-progress-changed and load-status-changed signals to notify about the load progress. I'm using signals instead of properties in this case, because progress and load-status only make sense while the view is loading, so they are passed as parameters to the signal callbacks, but they are not saved and there's no API to get them.
> - It adds is-loading property.
> - It adds all API docs missing in previous patch.

I think the progress signal is a very friendly API and I'm not too bothered by the fact that network traffic may have ceased at any given time. I think it's sufficient to say something like "If the value of progress is 1.00, all network traffic for initial load of the page has ceased. Progress does not track the status of any asynchronous loading that happens on the page."

I don't really like the idea of the is-loading property, because the page may still be "loading" in a sense -- constructing the DOM, loading XHR, etc even after progress is 1. Progress really only tracks network traffic of the initial load. I think the API should be clear about that distinction.

[Carlos Garcia Campos]

(In reply to comment #3)

> I don't really like the idea of the is-loading property, because the page may still be "loading" in a sense -- constructing the DOM, loading XHR, etc even after progress is 1. Progress really only tracks network traffic of the initial load. I think the API should be clear about that distinction.

is-loading property has nothing to do with the progress, it's set to TRUE when a load operation starts and to FALSe en it finishes. It's useful to to start/stop a spinner, for example. I think epiphany has this property in its EphyWebView object.

[Martin Robinson]

Carlos and I talked recently about the badness of the WK1 API https://lists.webkit.org/pipermail/webkit-gtk/2010-May/000241.html. We've decided to make each callback a separate signal. Carlos agreed to split this patch up.

[Carlos Garcia Campos]

Created attachment 108136 [details]
Another approach using an interface

This is another approach using an interface and a default implementation for the interface that emit signals. It's undocumented for now, since we still don't know which approach we'll finally use. I'm still not sure about the default implementation, though. It's not clear when to use the default impl or implement the interface. You can do the same with both, using the default impl you don't need to create a GObject, but I think it's the only benefict. So, I wonder whether we should provide only one way, an interface for the user to implement, or just emit the signals from the view object.

[Martin Robinson]

Comment on attachment 108136 [details]
Another approach using an interface

I like this approach a lot. We don't have to expose an interface at first, if we are unsure about it. One suggestion I have for the patch is to make the loader client responsible for creating the WK2 page client. The callback methods can call the interface methods directly -- they will receive it as their private data. Ths will reduce the amount of null checks and the code in WebKitWebView.cpp.

[Carlos Garcia Campos]

(In reply to comment #7)
> (From update of attachment 108136 [details])
> I like this approach a lot. We don't have to expose an interface at first, if we are unsure about it. 

In that case I don't see the advantage of using this instead of adding the signals to WebKitWebView. If I had to choose I would probably leave the interfac and not the default impl.

> One suggestion I have for the patch is to make the loader client responsible for creating the WK2 page client. The callback methods can call the interface methods directly -- they will receive it as their private data. Ths will reduce the amount of null checks and the code in WebKitWebView.cpp.

The view will need to do things in those callbacks too, for example update the uri, load error pages, update the title, etc. even if there isn't a loader client. That's why I create the loader client on demand. 

I'm also not sure whether we should expose all the C API callbacks in the interface, some of them doesn't seem to have anything to do with loading like processDidCrash or didChangeBackForwardList. So, for now I would only add the callbacks to replace the old load-status.

[Martin Robinson]

(In reply to comment #8)

> The view will need to do things in those callbacks too, for example update the uri, load error pages, update the title, etc. even if there isn't a loader client. That's why I create the loader client on demand.

I think it makes sense to make private methods of the WebView available to the loader rather than making the entire entire loader interface available to the WebView. There will just be fewer methods and less boilerplate.

We could even make some of these tasks the responsibility of the loader. There's no reason the WebView should be the one keeping track of the URL. I can just ask the loader for it. 

> I'm also not sure whether we should expose all the C API callbacks in the interface, some of them doesn't seem to have anything to do with loading like processDidCrash or didChangeBackForwardList. So, for now I would only add the callbacks to replace the old load-status.

Let's keep them associated with the loader client unless they move to some other client in the future. It's confusing having the logic of the loader client handled by different classes, IMO.

[Alejandro G. Castro]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 108136 [details] [details])
> > I like this approach a lot. We don't have to expose an interface at first, if we are unsure about it. 
> 
> In that case I don't see the advantage of using this instead of adding the signals to WebKitWebView. If I had to choose I would probably leave the interfac and not the default impl.
>

I also have some doubts about the approach, adding two options is usually a way to confuse developers, because they have to decide if we do not say in which cases you should use one or the other. If we can explain that then we are ok with it.

> 
> I'm also not sure whether we should expose all the C API callbacks in the interface, some of them doesn't seem to have anything to do with loading like processDidCrash or didChangeBackForwardList. So, for now I would only add the callbacks to replace the old load-status.

I would say that if we think this should be located in other API callbacks maybe we should propose a change in the C API, and get information about why they added it here, because I agree with martin that it is confusing just to fix it for ourselves adding the logic in other class.

Hope it helps.

[Carlos Garcia Campos]

Created attachment 108456 [details]
New patch

This patch is similar to the interface approach, but using an object, like the previous default client one, that has virtual methods for the signals. This way you can use the object directly connecting to the signals, or use your own loader client inheriting from this one, and overriding the signal handlers, the same way it could be done with the interface. All the code related to the loader client has been moved from webview to webloaderclient.

[Philippe Normand]

Comment on attachment 108456 [details]
New patch

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

I like this new approach. It seems simple enough for the developer and extensible if the developer needs more.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebLoaderClient.cpp:294
> +     * The necessary transport requirements are stabilished, and the

s/stabilished/established ? or simply, set?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:169
> + * Returns: (trasnfer-none): the #WebKitWebLoaderClient of @web_view.

typo: trasnfer

[Carlos Garcia Campos]

*** Bug 57835 has been marked as a duplicate of this bug. ***

[Gustavo Noronha (kov)]

Comment on attachment 107985 [details]
New patch

I assume this one is obsolete.

[Carlos Garcia Campos]

(In reply to comment #14)
> (From update of attachment 107985 [details])
> I assume this one is obsolete.

Not exactly, because it's a different approach, but it seems nobody liked the gio aync pattern, so I guess we'll go with the second approach, anyway :-)

[Gustavo Noronha (kov)]

Comment on attachment 108456 [details]
New patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebLoaderClient.cpp:272
> +     * This signal is emitted when an error occurs when starting to
> +     * load data for a page. By default, if the signal is not handled,
> +     * a stock error page will be displayed. You need to handle the signal
> +     * if you want to provide your own error page.

I assume you can handle this signal to avoid the currently loaded page to be replaced by the error page, for instance? Might be good mentioning it here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebLoaderClient.cpp:318
> +     * This signal is emitted when a page load complete, that is, when all
> +     * the resources are done loading.
> +     *
> +     * This signal is emitted after #WebKitWebLoaderClient::load-committed.

What about that idea of also emiting a signal when the load stops either by finishing properly or by failing, giving us a single signal to know that the load try has finished?

[Carlos Garcia Campos]

(In reply to comment #16)
> (From update of attachment 108456 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108456&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebLoaderClient.cpp:272
> > +     * This signal is emitted when an error occurs when starting to
> > +     * load data for a page. By default, if the signal is not handled,
> > +     * a stock error page will be displayed. You need to handle the signal
> > +     * if you want to provide your own error page.
> 
> I assume you can handle this signal to avoid the currently loaded page to be replaced by the error page, for instance? Might be good mentioning it here.

I'm not sure I understand what you mean, it says that you need to handle the signal to provide your own error page. Do you mean it can be used to not show any error page at all?

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebLoaderClient.cpp:318
> > +     * This signal is emitted when a page load complete, that is, when all
> > +     * the resources are done loading.
> > +     *
> > +     * This signal is emitted after #WebKitWebLoaderClient::load-committed.
> 
> What about that idea of also emiting a signal when the load stops either by finishing properly or by failing, giving us a single signal to know that the load try has finished?

Do you mean adding a new signal in addition to the current ones or replace finished and failed signals by a single finished signal?

[Gustavo Noronha (kov)]

Sorry, I didn't realize they were two different options, but yeah, I guess this one looks simpler and better overall.

(In reply to comment #17)
> > I assume you can handle this signal to avoid the currently loaded page to be replaced by the error page, for instance? Might be good mentioning it here.
> 
> I'm not sure I understand what you mean, it says that you need to handle the signal to provide your own error page. Do you mean it can be used to not show any error page at all?

Yes, I mean can it be used to just not touch the page that is being shown? Since this is a provisional failure, you haven't committed yet, so you can remain at the already loaded page, right?
 
> Do you mean adding a new signal in addition to the current ones or replace finished and failed signals by a single finished signal?

Not sure what's the best way, but I think the idea of simplifying detecting that the load has finished one way or another would be welcome. Maybe we should finished at the end regardless of whether it failed or not, then you can query the failure and final state from the loader client, what do you think? Adding a new signal sounds OK too.

[Carlos Garcia Campos]

(In reply to comment #18)
> Sorry, I didn't realize they were two different options, but yeah, I guess this one looks simpler and better overall.

No problem.

> (In reply to comment #17)

> > I'm not sure I understand what you mean, it says that you need to handle the signal to provide your own error page. Do you mean it can be used to not show any error page at all?
> 
> Yes, I mean can it be used to just not touch the page that is being shown? Since this is a provisional failure, you haven't committed yet, so you can remain at the already loaded page, right?

Yes, ou can just connect to the signal and simply return TRUE. Note that default behaviour is not implemented yet.
 
> > Do you mean adding a new signal in addition to the current ones or replace finished and failed signals by a single finished signal?
> 
> Not sure what's the best way, but I think the idea of simplifying detecting that the load has finished one way or another would be welcome. Maybe we should finished at the end regardless of whether it failed or not, then you can query the failure and final state from the loader client, what do you think? Adding a new signal sounds OK too.

I'm not sure, I think Martin wanted to map all callbacks to signals to give more control to the user over the loading process. With the previous patch it was more natural to have a single finish point, because it's how gio async pattern works.

[Gustavo Noronha (kov)]

Oh, sure, I'm not arguing against having all the signals, I think they should be there, just saying we could have a signal that is always emitted in the end regardless of what happened - would make it simpler for those who do not need the finer grained control.

[Carlos Garcia Campos]

(In reply to comment #20)
> Oh, sure, I'm not arguing against having all the signals, I think they should be there, just saying we could have a signal that is always emitted in the end regardless of what happened - would make it simpler for those who do not need the finer grained control.

For that case I added is-loading property to web view, so that users who only want to start/stop a spinner or something like that can monitor the property and don't need to use the loader client at all. I didn't add it in the last patch just to make it smaller.

[Martin Robinson]

Comment on attachment 108456 [details]
New patch

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

This looks good to me. I do not think we should add any sort of convenience API until we definitely understand how all these callbacks fit together. For example does a failure during provisional load also call didFailLoadWithErrorForFrame as well as didStartProvisionalLoadForFrame? Carlos, do you mind taking a peak at the loader  code in WebCore and writing the answer to that question somewhere in the gtkdoc?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebLoaderClient.cpp:59
> +static void didStartProvisionalLoadForFrame(WKPageRef page, WKFrameRef frame, WKTypeRef userData, const void* clientInfo)

userData is unused, so the parameter name should be commented out.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebLoaderClient.cpp:66
> +    WebKitWebLoaderClient* client = WEBKIT_WEB_LOADER_CLIENT(clientInfo);
> +    gboolean returnValue;
> +    g_signal_emit(client, signals[PROVISIONAL_LOAD_STARTED], 0, &returnValue);

g_signal_emit takes a gpointer. Is it really necessary to cast clientInfo here?

[Carlos Garcia Campos]

(In reply to comment #22)
> (From update of attachment 108456 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108456&action=review
> 
> This looks good to me. I do not think we should add any sort of convenience API until we definitely understand how all these callbacks fit together. For example does a failure during provisional load also call didFailLoadWithErrorForFrame as well as didStartProvisionalLoadForFrame? Carlos, do you mind taking a peak at the loader  code in WebCore and writing the answer to that question somewhere in the gtkdoc?

Sure.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebLoaderClient.cpp:59
> > +static void didStartProvisionalLoadForFrame(WKPageRef page, WKFrameRef frame, WKTypeRef userData, const void* clientInfo)
> 
> userData is unused, so the parameter name should be commented out.

Ok.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebLoaderClient.cpp:66
> > +    WebKitWebLoaderClient* client = WEBKIT_WEB_LOADER_CLIENT(clientInfo);
> > +    gboolean returnValue;
> > +    g_signal_emit(client, signals[PROVISIONAL_LOAD_STARTED], 0, &returnValue);
> 
> g_signal_emit takes a gpointer. Is it really necessary to cast clientInfo here?

Yes, clientInfo is a const void * while g_signal_emit() expects a void *. We can cast directly instead of using a local variable, though.

[Carlos Garcia Campos]

Created attachment 109008 [details]
Updated patch

Patch rebased to current git master and updated according to comments.

[Martin Robinson]

Comment on attachment 109008 [details]
Updated patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebLoaderClient.cpp:314
> +     * #WebKitWebLoaderClient::provisional-load-failed or #WebKitWebLoaderClient::load-failed
> +     * are emitted.

So I guess only one is fired?

[Carlos Garcia Campos]

(In reply to comment #25)
> (From update of attachment 109008 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109008&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebLoaderClient.cpp:314
> > +     * #WebKitWebLoaderClient::provisional-load-failed or #WebKitWebLoaderClient::load-failed
> > +     * are emitted.
> 
> So I guess only one is fired?

Yes, privisional-failed, load-failed or load-finished, these 3 finish the load.

[Carlos Garcia Campos]

Committed r96226: <http://trac.webkit.org/changeset/96226>