Bug 17066 – [GTK] Improve frameloader signals

• Component: WebKitGTK
• Status: RESOLVED WONTFIX

[Christian Dywan]

The signals related to loading and loading progress need improvement. The following points should be addressed in this bug:

1. Signal naming is confusing.
2. Providing context about what is loaded, ie WebKitEventLoad.
3. There is no error reporting no failure.
4. "load-done" is missing from WebView for no obvious reason.

[Christian Dywan]

Created attachment 18763 [details]
Improve load signals

This is a suggestion for improving the above mentioned issues.

This patch makes names all signals consistently, emits 'load-done' on the WebView, passes a WebKitEventLoad* structure to each signal and reports errors with a GError*.

Note that this does not address missing error reporting in the http backend.

I am not fully decided myself on whether this specific patch is what we want, particularly I'm concerned with 'load-progress-changed' and critique is very welcome.

[Mark Rowe (bdash)]

I think the handling of provisional loads needs improved a bit here.

[Christian Dywan]

Comment on attachment 18763 [details]
Improve load signals

Clearing review for now, taking a closer look.

[Christian Dywan]

Created attachment 18817 [details]
Improve load signals Take 2

After a more in-depth look, updated patch.

[Alp Toker]

(In reply to comment #4)
> Created an attachment (id=18817) [edit]
> Improve load signals Take 2
> 
> After a more in-depth look, updated patch.
> 


Careful with the trailing backslash in the .pro:

         platform/network/curl/ResourceHandleManager.cpp \
+        platform/network/gtk/ResourceErrorGtk.cpp
         platform/graphics/cairo/AffineTransformCairo.cpp \

The operator overload looks like it could lead to leaks -- didn't look closely yet though.

Will try look over this soon. Feedback from others would be great too.

[Alp Toker]

(In reply to comment #5)
> The operator overload looks like it could lead to leaks -- didn't look closely
> yet though.

Furthermore, you probably want to use g_error_new_literal() rather than g_error_new().

> 
> Will try look over this soon. Feedback from others would be great too.
> 

[Alp Toker]

The "event" structures are pretty unconventional outside of GDK, but it's an interesting idea. I'd be interested to hear rationales for some of the signal name changes as well.

I think I like the WEBKIT_LOAD_STATE, it reminds me of how what Epiphany maps our current mess of signals to internally.

I think a cleanup of these signals is important, and I really want to pull anyone who is hacking a GTK+ browser into this discussion, since we only get one shot at designing this. Epiphany/Galeon hackers?

[Christian Dywan]

Created attachment 18937 [details]
Improve load signals Take 3

One part, terminology wise, was the distinction between "progress" and "load". The former is global in relation to the view and not part of the "load" as such. The original 'load-finished' was not the end of the load but in fact the end of the "progress". So I replaced it to match web frame's "load-done" in the proper meaning. In general it's now possible to use a web view or a web frame on its own since both receive all meaningful signals.

As for enhancements, I'm introducing "redirect-scheduled", "redirect-done" and "redirect-cancelled" and "document-load-done". Further more errors are reported in the form of gerrors.

I tried to explain all the signals in code, ie. gtkdoc. If that is not sufficient/ clear please say so.

A few slightly higher level changes:
. Renaming 'status-bar-text-changed' to "statusbar-text-changed"
. Removing 'selection-changed' and 'icon-loaded' since they are virtually dead code; better introduce them again properly or they just cause confusion.
. Adding all signals to the web view struct so they can be cleanly overridden in derived classes.
. A bit of related code/ docs cleanup.

Note that this patch is not 100% clean, I'm sure I overlooked one or two things. I am going to fix these soon while testing a bit more and depending on your feedback.

[Christian Dywan]

For reference, especially useful if you aren't particularly familiar with this part of WebCore, the docs for Mac's delegate that corresponds to our frame loading signals.

http://developer.apple.com/documentation/Cocoa/Reference/WebKit/Protocols/WebFrameLoadDelegate_Protocol/Reference/Reference.html#//apple_ref/occ/cat/WebFrameLoadDelegate

[Alp Toker]

Comment on attachment 18937 [details]
Improve load signals Take 3

Clearing the request flag so the issues discussed on IRC can be fixed (avoiding event structs, removing the dead enum, explicitly creating GErrors with a static func for now and anything else I mentioned).

[Christian Dywan]

Created attachment 18972 [details]
Improve load signals Take 4

[Christian Dywan]

Created attachment 18973 [details]
Improve load signals Take 5

[Alp Toker]

(In reply to comment #12)
> Created an attachment (id=18973) [edit]
> Improve load signals Take 5
> 

Can you propose these changes on the mailing list, explaining the changes and requesting review from the Obj-C API and loader hackers? I think mjs and andersca can help the most here (by luck they're also both familiar with GObject).

The old code limited the usefulness of WebView in anything but simple applications. I hope we get it right this time.

[Alp Toker]

Comment on attachment 18973 [details]
Improve load signals Take 5

Clearing review request. We'll need to take a fresh look at the loader API as discussed.

[Marco Barisione]

The patch emits the WebKitWebView signals only for the main frame but WebKit doesn't have a way to notify the application of the creation of new frames, so it's impossible to get those signals for sub frames.
IMO we have 3 possible ways to solve this:
1 - Emit the WebView signals for all the WebFrames.
2 - Emit something like new-frame when a new frame is created so the application can monitor the creation of new frames.
3 - Like 2 but still emit the WebView signals for all the WebFrames.

If we do 2 I suggest to remove the WebFrame argument from the signals, as it will always be the main frame.


I think that being able to use GError (like the mac port uses NSError) would be nice but it's not a real object so we cannot subclass it. This means that we cannot add new properties/methods if needed, so if we plan to extend it in the future (do we really need to extend it?) maybe it's better to add a WebKitWebError object.


It would be nice to provide a default error page if there is an error, but in this case the application should be able to suppress the default behaviour.
How to do this? My suggestions are:
1 - Split the load-done in load-done and load-error. Then callback for load-error returns a boolean saying if the default behaviour is wanted or not. This has also the advantage of not using load-done for errors as name load-done seems to imply success (at least to me).
2 - Add a "show-errors" property to WebView (or maybe WebSettings). If the property is set to TRUE (the default) then WebKit will show error pages in case of errors, else the application will have to handle errors.


Note that to make error handling work we have also to modify the soup back-end (now it uses internal error codes) and the curl one (it uses the CURL error codes and somewhere it creates a null ResourceError).


The patch doesn't implement dispatchDidFailLoading that is called for every resource that was not loaded (images, html pages, etc.). A signal for that could be useful but we can add it later.

[Christian Dywan]

(In reply to comment #15)
> The patch emits the WebKitWebView signals only for the main frame but WebKit
> doesn't have a way to notify the application of the creation of new frames, so
> it's impossible to get those signals for sub frames.
> IMO we have 3 possible ways to solve this:
> 1 - Emit the WebView signals for all the WebFrames.
> 2 - Emit something like new-frame when a new frame is created so the
> application can monitor the creation of new frames.
> 3 - Like 2 but still emit the WebView signals for all the WebFrames.
> 
> If we do 2 I suggest to remove the WebFrame argument from the signals, as it
> will always be the main frame.

I think we should actually discard the WebFrame* argument from all WebView signals and add new-frame here. Average applications won't care about frames at all and it makes no sense to send dozens of signals for each frame. This way, an application interested in frames can still connect to new-frame and put their callbacks on as many frames as needed.

> I think that being able to use GError (like the mac port uses NSError) would be
> nice but it's not a real object so we cannot subclass it. This means that we
> cannot add new properties/methods if needed, so if we plan to extend it in the
> future (do we really need to extend it?) maybe it's better to add a
> WebKitWebError object.

I think we should go for a WebKitWebError. The way GError is designed is hardly enough for our use cases. Usually functions fill in a nulled &GError reference. It's not suitable for our cases, for instance as an argument in signal callbacks.

> It would be nice to provide a default error page if there is an error, but in
> this case the application should be able to suppress the default behaviour.
> How to do this? My suggestions are:
> 1 - Split the load-done in load-done and load-error. Then callback for
> load-error returns a boolean saying if the default behaviour is wanted or not.
> This has also the advantage of not using load-done for errors as name load-done
> seems to imply success (at least to me).
> 2 - Add a "show-errors" property to WebView (or maybe WebSettings). If the
> property is set to TRUE (the default) then WebKit will show error pages in case
> of errors, else the application will have to handle errors.

I think having *only* load-done with a trailing error struct/ object is the most intuitive approach. For instance it seems easy to confuse if you need to connect to two signals only to stop your loading animation. A boolean return could indicate whether or not WebKit should take care of eventual error pages or not.

> The patch doesn't implement dispatchDidFailLoading that is called for every
> resource that was not loaded (images, html pages, etc.). A signal for that
> could be useful but we can add it later.

I think that's out of the scope of this bug actually, unless there is an aspect of it that conflicts/ interferes with the frame loader signals.

[Marco Barisione]

I added some debug messages to the frame loader to understand how it works, signals we already have are reported after "->".


EMPTY PAGE
==========
When an empty page (no images, scripts, etc.) or about:blank is loaded you get the following call sequence:

FrameLoaderClient::provisionalLoadStarted
FrameLoaderClient::postProgressStartedNotification -> load-started
FrameLoaderClient::dispatchDidStartProvisionalLoad
FrameLoaderClient::assignIdentifierToInitialRequest(identifier=2, request=http://localhost/public/empty.html)
FrameLoaderClient::dispatchWillSendRequest(identifier=2, request=http://localhost/public/empty.html, response=)
FrameLoaderClient::transitionToCommittedForNewPage
FrameLoaderClient::finishedLoading
FrameLoaderClient::dispatchDidCommitLoad -> load-committed
FrameLoaderClient::dispatchDidFirstLayout
FrameLoaderClient::dispatchDidFinishDocumentLoad
FrameLoaderClient::dispatchDidLoadMainResource
FrameLoaderClient::frameLoadCompleted
FrameLoaderClient::dispatchDidFinishLoad -> load-done(true)
FrameLoaderClient::postProgressEstimateChangedNotification -> load-progress-changed(100)
FrameLoaderClient::postProgressFinishedNotification -> load-finished
FrameLoaderClient::dispatchDidFinishLoading(identifier=2)

The provisional load stage is used when WebKit doesn't still now the mime type, etc. of the new page.

FrameLoaderClient::assignIdentifierToInitialRequest() is called when a new resource is going to be loaded, probably we are going to keep a hash map to associate identifiers with resources.

The name load-committed is not very clear but the signal is quite important, from a comment in the source code: This means the URI is valid and successfully identify the page that's going to be loaded.


PAGE WITH AN IMAGE
==================
FrameLoaderClient::provisionalLoadStarted
FrameLoaderClient::postProgressStartedNotification -> load-started
FrameLoaderClient::dispatchDidStartProvisionalLoad
FrameLoaderClient::assignIdentifierToInitialRequest(identifier=5, request=http://localhost/public/test.html)
FrameLoaderClient::dispatchWillSendRequest(identifier=5, request=http://localhost/public/test.html, response=)
FrameLoaderClient::transitionToCommittedForNewPage
FrameLoaderClient::dispatchDidCommitLoad -> load-committed
FrameLoaderClient::postProgressEstimateChangedNotification -> load-progress-changed(50)
FrameLoaderClient::finishedLoading
FrameLoaderClient::dispatchDidFinishDocumentLoad
FrameLoaderClient::dispatchDidLoadMainResource
FrameLoaderClient::dispatchDidFinishLoading(identifier=5)
FrameLoaderClient::assignIdentifierToInitialRequest(identifier=6, request=http://localhost/public/test.jpg)
FrameLoaderClient::dispatchWillSendRequest(identifier=6, request=http://localhost/public/test.jpg, response=)
FrameLoaderClient::dispatchDidFirstLayout
FrameLoaderClient::postProgressEstimateChangedNotification -> load-progress-changed(57)
FrameLoaderClient::postProgressEstimateChangedNotification -> load-progress-changed(65)
FrameLoaderClient::postProgressEstimateChangedNotification -> load-progress-changed(72)
FrameLoaderClient::postProgressEstimateChangedNotification -> load-progress-changed(79)
FrameLoaderClient::postProgressEstimateChangedNotification -> load-progress-changed(83)
FrameLoaderClient::frameLoadCompleted
FrameLoaderClient::dispatchDidFinishLoad -> load-done(true)
FrameLoaderClient::postProgressEstimateChangedNotification -> load-progress-changed(100)
FrameLoaderClient::postProgressFinishedNotification -> load-finished
FrameLoaderClient::dispatchDidFinishLoading(identifier=6)

Same as before but the frame loader is also informed of the loading of the image.


PAGE WITH A NON-EXISTING IMAGE
==============================
FrameLoaderClient::provisionalLoadStarted
FrameLoaderClient::postProgressStartedNotification -> load-started
FrameLoaderClient::dispatchDidStartProvisionalLoad
FrameLoaderClient::assignIdentifierToInitialRequest(identifier=9, request=http://localhost/public/test.html)
FrameLoaderClient::dispatchWillSendRequest(identifier=9, request=http://localhost/public/test.html, response=)
FrameLoaderClient::transitionToCommittedForNewPage
FrameLoaderClient::dispatchDidCommitLoad -> load-committed
FrameLoaderClient::postProgressEstimateChangedNotification -> load-progress-changed(50)
FrameLoaderClient::finishedLoading
FrameLoaderClient::dispatchDidFinishDocumentLoad
FrameLoaderClient::dispatchDidLoadMainResource
FrameLoaderClient::dispatchDidFinishLoading(identifier=9)
FrameLoaderClient::assignIdentifierToInitialRequest(identifier=10, request=http://example.invalid/no.jpg)
FrameLoaderClient::dispatchWillSendRequest(identifier=10, request=http://example.invalid/no.jpg, response=)
FrameLoaderClient::dispatchDidFirstLayout
FrameLoaderClient::frameLoadCompleted
FrameLoaderClient::dispatchDidFinishLoad -> load-done(true)
FrameLoaderClient::postProgressEstimateChangedNotification -> load-progress-changed(100)
FrameLoaderClient::postProgressFinishedNotification -> load-finished
FrameLoaderClient::dispatchDidFailLoading(identifier=10)

FrameLoaderClient::dispatchDidFailLoading() is called for the not existing image.


PAGE WITH AN INVALID IMAGE
==========================
FrameLoaderClient::provisionalLoadStarted
FrameLoaderClient::postProgressStartedNotification -> load-started
FrameLoaderClient::dispatchDidStartProvisionalLoad
FrameLoaderClient::assignIdentifierToInitialRequest(identifier=7, request=http://localhost/public/test.html)
FrameLoaderClient::dispatchWillSendRequest(identifier=7, request=http://localhost/public/test.html, response=)
FrameLoaderClient::transitionToCommittedForNewPage
FrameLoaderClient::dispatchDidCommitLoad -> load-committed
FrameLoaderClient::postProgressEstimateChangedNotification -> load-progress-changed(50)
FrameLoaderClient::finishedLoading
FrameLoaderClient::dispatchDidFinishDocumentLoad
FrameLoaderClient::dispatchDidLoadMainResource
FrameLoaderClient::dispatchDidFinishLoading(identifier=7)
FrameLoaderClient::assignIdentifierToInitialRequest(identifier=8, request=http://localhost/public/no.jpg)
FrameLoaderClient::dispatchWillSendRequest(identifier=8, request=http://localhost/public/no.jpg, response=)
FrameLoaderClient::dispatchDidFirstLayout
FrameLoaderClient::postProgressEstimateChangedNotification -> load-progress-changed(59)
FrameLoaderClient::frameLoadCompleted
FrameLoaderClient::dispatchDidFinishLoad -> load-done(true)
FrameLoaderClient::postProgressEstimateChangedNotification -> load-progress-changed(100)
FrameLoaderClient::postProgressFinishedNotification -> load-finished
FrameLoaderClient::dispatchDidFinishLoading(identifier=8)

The image does not exist but the server returns a 404 html page for it. This cannot be showed but FrameLoaderClient::dispatchDidFinishLoading() is called because the resource was loaded.


NON-EXISTING DOMAIN
===================
FrameLoaderClient::provisionalLoadStarted
FrameLoaderClient::postProgressStartedNotification -> load-started
FrameLoaderClient::dispatchDidStartProvisionalLoad
FrameLoaderClient::assignIdentifierToInitialRequest(identifier=12, request=http://example.invalid/)
FrameLoaderClient::dispatchWillSendRequest(identifier=12, request=http://example.invalid/, response=)
FrameLoaderClient::dispatchDidLoadMainResource
FrameLoaderClient::dispatchDidFailProvisionalLoad -> load-done(false)
FrameLoaderClient::postProgressEstimateChangedNotification -> load-progress-changed(100)
FrameLoaderClient::postProgressFinishedNotification -> load-finished
FrameLoaderClient::frameLoadCompleted
FrameLoaderClient::dispatchDidFailLoading(identifier=12)

Trying to load a page from a non-existing domain.


HTTP REDIRECT
=============
www.google.com redirects to www.google.co.uk:

FrameLoaderClient::provisionalLoadStarted
FrameLoaderClient::postProgressStartedNotification -> load-started
FrameLoaderClient::dispatchDidStartProvisionalLoad
FrameLoaderClient::assignIdentifierToInitialRequest(identifier=4, request=http://www.google.com/)
FrameLoaderClient::dispatchWillSendRequest(identifier=4, request=http://www.google.com/, response=)
FrameLoaderClient::dispatchWillSendRequest(identifier=4, request=http://www.google.co.uk/, response=http://www.google.com/)
FrameLoaderClient::dispatchDidReceiveServerRedirectForProvisionalLoad
FrameLoaderClient::transitionToCommittedForNewPage
FrameLoaderClient::dispatchDidCommitLoad -> load-committed
FrameLoaderClient::willChangeTitle
FrameLoaderClient::didChangeTitle
FrameLoaderClient::postProgressEstimateChangedNotification -> load-progress-changed(44)
FrameLoaderClient::postProgressEstimateChangedNotification -> load-progress-changed(45)
FrameLoaderClient::finishedLoading
FrameLoaderClient::dispatchDidFinishDocumentLoad
FrameLoaderClient::dispatchDidLoadMainResource
FrameLoaderClient::dispatchDidFinishLoading(identifier=4)
FrameLoaderClient::assignIdentifierToInitialRequest(identifier=5, request=http://www.google.co.uk/intl/en_uk/images/logo.gif)
FrameLoaderClient::dispatchWillSendRequest(identifier=5, request=http://www.google.co.uk/intl/en_uk/images/logo.gif, response=)
FrameLoaderClient::postProgressEstimateChangedNotification -> load-progress-changed(46)
FrameLoaderClient::dispatchDidFirstLayout
FrameLoaderClient::postProgressEstimateChangedNotification -> load-progress-changed(56)
FrameLoaderClient::assignIdentifierToInitialRequest(identifier=6, request=http://www.google.co.uk/images/nav_logo3.png)
FrameLoaderClient::dispatchWillSendRequest(identifier=6, request=http://www.google.co.uk/images/nav_logo3.png, response=)
FrameLoaderClient::dispatchDidFinishLoading(identifier=5)
FrameLoaderClient::postProgressEstimateChangedNotification -> load-progress-changed(62)
FrameLoaderClient::postProgressEstimateChangedNotification -> load-progress-changed(65)
FrameLoaderClient::frameLoadCompleted
FrameLoaderClient::dispatchDidFinishLoad -> load-done(true)
FrameLoaderClient::postProgressEstimateChangedNotification -> load-progress-changed(100)
FrameLoaderClient::postProgressFinishedNotification -> load-finished
FrameLoaderClient::dispatchDidFinishLoading(identifier=6)

FrameLoaderClient::dispatchWillSendRequest() is called twice because of the redirect, the first time the response argument contains the new destination.

The response object (we still don't have it in the GTK port) knows about the HTTP status code, so we can distinguish between permanent or temporary redirects.


HTML redirect
=============
Redirect using HTML and a timeout:

FrameLoaderClient::provisionalLoadStarted
FrameLoaderClient::postProgressStartedNotification -> load-started
FrameLoaderClient::dispatchDidStartProvisionalLoad
FrameLoaderClient::assignIdentifierToInitialRequest(identifier=11, request=http://localhost/public/test.html)
FrameLoaderClient::dispatchWillSendRequest(identifier=11, request=http://localhost/public/test.html, response=)
FrameLoaderClient::transitionToCommittedForNewPage
FrameLoaderClient::dispatchDidCommitLoad -> load-committed
FrameLoaderClient::postProgressEstimateChangedNotification -> load-progress-changed(50)
FrameLoaderClient::finishedLoading
FrameLoaderClient::dispatchDidFirstLayout
FrameLoaderClient::dispatchWillPerformClientRedirect(url=about:blank, delay=10,000000, fireDate=1216223024,062564)
FrameLoaderClient::dispatchDidFinishDocumentLoad
FrameLoaderClient::dispatchDidLoadMainResource
FrameLoaderClient::frameLoadCompleted
FrameLoaderClient::dispatchDidFinishLoad -> load-done(true)
FrameLoaderClient::postProgressEstimateChangedNotification -> load-progress-changed(100)
FrameLoaderClient::postProgressFinishedNotification -> load-finished
FrameLoaderClient::dispatchDidFinishLoading(identifier=11)
[after 10 seconds the normal loading of the new page starts]

In case of a HTML redirect the page is normally loaded but FrameLoaderClient::dispatchWillPerformClientRedirect() is called. After the specified delay a new page load happens.

If the redirect is cancelled (for instance moving to a new page) then FrameLoaderClient::dispatchDidCancelClientRedirect is called. I don't know how to cancel a pending redirect but it should be possible.


CONCLUSIONS
===========
Do we need both a load-done and load-finished signals? IMO just a progress going to 100% is ok but it's strange that load-done is emitted before notify::progress for 100%. Ideally I would prefer to be able to emit something like load-done after the progress goes to 100%.

We don't map dispatchDidFinishLoading, is it useful for tracking sub resources? maybe it's better to add it later.

It seems that there aren't so many progress change notifications so emitting a signal for each of them seems ok.

FrameLoaderClient::dispatchWillSendRequest is quite useful because it allows to handle HTTP redirects and also to change the request from the programs using webKit. The main problem for this is that we don't have a response and a data source object that should be passed as arguments to the signal.

I don't know when FrameLoaderClient::revertToProvisionalState() can be called.

Note that now some signals are emitted jsut on the frame and some others just on the WebView. We should emit every frame-specific signal on the frame and also on the web view if the frame is the main one. In this case we should not pass the frame as argument as the frame can be easily get from the WebView with webkit_web_view_get_main_frame().

[Marco Barisione]

Created attachment 22311 [details]
Patch to get some debugging info from the frame loader

[Christian Dywan]

(In reply to comment #17)
> I added some debug messages to the frame loader to understand how it works,
> signals we already have are reported after "->".
>
> [snip]
>
> CONCLUSIONS
> ===========
> Do we need both a load-done and load-finished signals? IMO just a progress
> going to 100% is ok but it's strange that load-done is emitted before
> notify::progress for 100%. Ideally I would prefer to be able to emit something
> like load-done after the progress goes to 100%.

If we have something similar to -started, -confirmed, -done and -finished, to me it makes perfect sense the way it is. -started and -finished are basically indicator for a hypothetical progress widget, so they tell us *before* anything is loaded (-started) and tell us *after* everything is really, eventually, done (-finished).

> It seems that there aren't so many progress change notifications so emitting a
> signal for each of them seems ok.

WebCore is very smart with regard to that, as far as I can observe. Be that not sufficient, we can still improve that later internally. The API will be the same anyway.

> Note that now some signals are emitted jsut on the frame and some others just
> on the WebView. We should emit every frame-specific signal on the frame and
> also on the web view if the frame is the main one. In this case we should not
> pass the frame as argument as the frame can be easily get from the WebView with
> webkit_web_view_get_main_frame().

That sounds reasonable, I second that.

[Christian Dywan]

I'd like to point out that we talked about having a single "load-state" signal that for example knows the states Started, Confirmed, Done and Finished. On top of that we would have a "progress" property.
It might be a good idea to refer to this concept from now on, to avoid confusion especially for people who did not attend the meeting.

[Marco Barisione]

This is my proposal for the API cleanup, I would really like to know what people that are going to use this API think.

The progress change is only reported on the FrameLoaderClient for the main frame, so the relative signals and properties should be just on the web view.

The other signals are reported on every frame, so the signals/properties should be per frame but if the frame is the main one they should also be propagated to the web view.


PROGRESS
========
FrameLoaderClient::postProgressStartNotification:
- Deprecate load-started (the name is confusing)
- Add progress-started
- Emit notify::progress with progress=0 (it's not emitted by FrameLoaderClient::postProgressEstimateChanged)

FrameLoaderClient::postProgressFinishedNotification:
- Deprecate load-finished (the name is confusing)
- Add progress-finished

FrameLoaderClient::postProgressEstimateChanged:
- Deprecate load-progress-changed (the name is confusing and the argument is an int in [0,100])
- Emit notify::progress using a double in [0.0, 1.0] (for uniformity with GtkProgressBar::fraction)

do we need progress-changed and progress-finished? It's possible just to use the value of the progress property to show/hide the progress bar. Note that we can guarantee the emission of notify::progress for both 0.0 and 1.0. what do epiphany/midori devs prefer?


STATUS
======
FrameLoaderClient::didStartProvisionalLoad:
- Emit notify::load-status with load-status=PROVISIONAL

FrameLoaderClient::dispatchDidCommitLoad:
- Emit notify::load-status with load-status=COMMITTED

FrameLoaderClient::dispatchDidFinishLoad:
- Emit notify::load-status with load-status=FINISHED
- Deprecate load-done (it just uses a bool for error reporting, the name seems to always imply success and it doesn't allow to have a default behaviour for loading errors)

FrameLoaderClient::dispatchDidFailLoad:
FrameLoaderClient::dispatchDidFailProvisionalLoad:
- Emit error(error_object), if this signal is emitted before setting the status then the provisional/non-provisional status doesn't need to be propagated as we have the load-status property. The return value of the handler is used to decide if the program wants the default error page or not. Most programs will probably just ignore this signal and use the default behaviour
- Emit notify::load-status with load-status=FINISHED (or ERROR?)
- Deprecate load-done

FrameLoaderClient::revertToProvisionalState
- Emit notify::load-status with load-status=PROVISIONAL

Status? State? Do you have better suggestions for the names?


REDIRECTS
=========
FrameLoaderClient::dispatchWillSendRequest:
- Emit sending-request(object, request, response, data_source). Better name for this? We don't have objects to represent single resources, responses and data sources, so I would add this in another patch

FrameLoaderClient::dispatchWillPerformClientRedirect:
- Emit client-redirect-scheduled(const gchar *url, guint delay_ms, time_t when)
- When we will have a data source object it will be possible to cancel a scheduled redirect calling the public C function for documentLoader->stopLoading()

FrameLoaderClient::dispatchDidCancelRedirect:
- Emit client-redirect-cancelled (canceled?)

FrameLoaderClient::dispatchDidServerRedirectForProvisionalLoad:
- This method is not very useful because it's impossible to know the destination of the redirect and the HTTP code sent by the server, so I would prefer to not emit anything. If a program needs to know about HTTP redirects it can use the signal emitted by dispatchWillSendRequest

[Marco Barisione]

I forgot to say what happens when you call webkit_web_view_load_html_string() from the error handler. In this case a new page load starts, so you receive all the normal loading signals.

[Christian Dywan]

(In reply to comment #21)
> The progress change is only reported on the FrameLoaderClient for the main
> frame, so the relative signals and properties should be just on the web view.

*nod*

> The other signals are reported on every frame, so the signals/properties
> should be per frame but if the frame is the main one they should also be
> propagated to the web view.

*nod* Progress aside, we want to be able to work only with WebView or with a WebFrame, unlike currently, where we need a bit of each to do anything useful.

> PROGRESS
> ========
> FrameLoaderClient::postProgressStartNotification:
> - Deprecate load-started (the name is confusing)
> - Add progress-started

> - Emit notify::progress with progress=0 (it's not emitted by
> FrameLoaderClient::postProgressEstimateChanged)

Agreed, we totally need "progress" to start with 0, even if WebCore doesn't do that.

> FrameLoaderClient::postProgressFinishedNotification:
> - Deprecate load-finished (the name is confusing)
> - Add progress-finished
> 
> FrameLoaderClient::postProgressEstimateChanged:
> - Deprecate load-progress-changed (the name is confusing and the argument is an
> int in [0,100])

Agreed, the name is bogus.

> - Emit notify::progress using a double in [0.0, 1.0] (for uniformity with
> GtkProgressBar::fraction)

That's a nice idea I hadn't thought about.

> do we need progress-changed and progress-finished? It's possible just to use
> the value of the progress property to show/hide the progress bar. Note that we
> can guarantee the emission of notify::progress for both 0.0 and 1.0. what do
> epiphany/midori devs prefer?

We don't need progress-changed when we have "progress", it's superfluous. And I actually tend towards having no progress signals at all, since "progress" does it all. The application callback code should actually become much simpler.

[Christian Dywan]

(In reply to comment #21)
> STATUS
> ======
> FrameLoaderClient::didStartProvisionalLoad:
> - Emit notify::load-status with load-status=PROVISIONAL
> FrameLoaderClient::dispatchDidCommitLoad:
> - Emit notify::load-status with load-status=COMMITTED
> FrameLoaderClient::dispatchDidFinishLoad:
> - Emit notify::load-status with load-status=FINISHED
> FrameLoaderClient::dispatchDidFailLoad:
> FrameLoaderClient::dispatchDidFailProvisionalLoad:
> - Emit error(error_object), if this signal is emitted before setting the
> status then the provisional/non-provisional status doesn't need to be
> propagated as we have the load-status property. The return value of the
> handler is used to decide if the program wants the default error page
> or not. Most programs will probably just ignore this signal and use the
> default behaviour

That's looks like a nice solution and it's a nifty trick to emit error with a boolean before updating status for the last time.

I am a little of shaky with regard to semantics/ terminology here, but functionally that looks very good.

> - Emit notify::load-status with load-status=FINISHED (or ERROR?)

We can't do load-status=ERROR because the final state must be reliably set after everything else. We need a state that applications can check, that means "done, not loading".

> - Deprecate load-done
> 
> FrameLoaderClient::revertToProvisionalState
> - Emit notify::load-status with load-status=PROVISIONAL
> 
> Status? State? Do you have better suggestions for the names?

Maybe s/State/Status and s/Finished/Complete? But I'm really not sure about this either.

[Marco Barisione]

(In reply to comment #23)
> > The other signals are reported on every frame, so the signals/properties
> > should be per frame but if the frame is the main one they should also be
> > propagated to the web view.
>
> *nod* Progress aside, we want to be able to work only with WebView or with a
> WebFrame, unlike currently, where we need a bit of each to do anything useful.

For errors we want to have the signal on the web view for every frame. And for redirects?

(In reply to comment #24)
> We can't do load-status=ERROR because the final state must be reliably set
> after everything else. We need a state that applications can check, that means
> "done, not loading".

Yes, I agree.

> Maybe s/State/Status and s/Finished/Complete? But I'm really not sure about
> this either.

Don't know about state/status (any native english speaker?), note that gtk uses both for similar things without any apparent logic. Ok for complete.

[Wouter Bolsterlee]

(In reply to comment #25)
> For errors we want to have the signal on the web view for every frame. And for
> redirects?

Note that "timeout redirects" (refresh page every now and then) can also be used for frames inside a page, e.g. update a "status frame" every few seconds. It would be useful to know from which frame the redirect originated. Perhaps as an argument in the callback when connecting to the signal on the web view?

[Christian Dywan]

(In reply to comment #21)
> REDIRECTS
> =========
> FrameLoaderClient::dispatchWillSendRequest:
> - Emit sending-request(object, request, response, data_source). Better
> name for this? We don't have objects to represent single resources,
> responses and data sources, so I would add this in another patch

Since we don't have the required classes I agree it makes more sense in a patch on top of this. Discussing these new classes may need some detailed discussion that shouldn't make this bug even more complicated. :)

> FrameLoaderClient::dispatchWillPerformClientRedirect:
> - Emit client-redirect-scheduled(const gchar *url, guint delay_ms,
> time_t when)
> - When we will have a data source object it will be possible to cancel a
> scheduled redirect calling the public C function for
> documentLoader->stopLoading()
> 
> FrameLoaderClient::dispatchDidCancelRedirect:
> - Emit client-redirect-cancelled (canceled?)

Being able to simply cancel a redirect-scheduled with webkit_web_view_stop_loading sounds totally nice.

I suggest s/client/ because I don't really see any meaning to this term here, outside of WebCore.

(In reply to comment #26)
> (In reply to comment #25)
> > For errors we want to have the signal on the web view for every frame.
> > And for redirects?
> 
> Note that "timeout redirects" (refresh page every now and then) can also be
> used for frames inside a page, e.g. update a "status frame" every few seconds.
> It would be useful to know from which frame the redirect originated.
> Perhaps as an argument in the callback when connecting to the signal on
> the web view?

So it seems most useful useful to have "error" and the signals for redirection on the web view, with a frame argument. Since you usually are interested in either all or none.

[Xan Lopez]

(In reply to comment #21)
> PROGRESS
> ========
> FrameLoaderClient::postProgressStartNotification:
> - Deprecate load-started (the name is confusing)
> - Add progress-started
> - Emit notify::progress with progress=0 (it's not emitted by
> FrameLoaderClient::postProgressEstimateChanged)
> 
> FrameLoaderClient::postProgressFinishedNotification:
> - Deprecate load-finished (the name is confusing)
> - Add progress-finished
> 
> FrameLoaderClient::postProgressEstimateChanged:
> - Deprecate load-progress-changed (the name is confusing and the argument is an
> int in [0,100])
> - Emit notify::progress using a double in [0.0, 1.0] (for uniformity with
> GtkProgressBar::fraction)
> 
> do we need progress-changed and progress-finished? It's possible just to use
> the value of the progress property to show/hide the progress bar. Note that we
> can guarantee the emission of notify::progress for both 0.0 and 1.0. what do
> epiphany/midori devs prefer?

Connecting to notify::progress and understanding 0.0 as started and 1.0 as finished seems to be functionally equivalent to having the extra signals, so I think it's just a matter of what seems clearer to people. No strong opinion here, although to me at least having the two extra signals makes reading code a bit clearer maybe.

> 
> 
> STATUS
> ======
> FrameLoaderClient::didStartProvisionalLoad:
> - Emit notify::load-status with load-status=PROVISIONAL
> 
> FrameLoaderClient::dispatchDidCommitLoad:
> - Emit notify::load-status with load-status=COMMITTED
> 
> FrameLoaderClient::dispatchDidFinishLoad:
> - Emit notify::load-status with load-status=FINISHED
> - Deprecate load-done (it just uses a bool for error reporting, the name seems
> to always imply success and it doesn't allow to have a default behaviour for
> loading errors)

Looks good to me.

> 
> FrameLoaderClient::dispatchDidFailLoad:
> FrameLoaderClient::dispatchDidFailProvisionalLoad:
> - Emit error(error_object), if this signal is emitted before setting the status
> then the provisional/non-provisional status doesn't need to be propagated as we
> have the load-status property. The return value of the handler is used to
> decide if the program wants the default error page or not. Most programs will
> probably just ignore this signal and use the default behaviour

The signal will be just "error"? Probably for another bug, but there has to be a way of customizing the error pages.

> - Emit notify::load-status with load-status=FINISHED (or ERROR?)
> - Deprecate load-done
> 
> FrameLoaderClient::revertToProvisionalState
> - Emit notify::load-status with load-status=PROVISIONAL
> 
> Status? State? Do you have better suggestions for the names?

Indifferent. Status sounds a bit maybe better.

> 
> 
> REDIRECTS
> =========
> FrameLoaderClient::dispatchWillSendRequest:
> - Emit sending-request(object, request, response, data_source). Better name for
> this? We don't have objects to represent single resources, responses and data
> sources, so I would add this in another patch
> 
> FrameLoaderClient::dispatchWillPerformClientRedirect:
> - Emit client-redirect-scheduled(const gchar *url, guint delay_ms, time_t when)
> - When we will have a data source object it will be possible to cancel a
> scheduled redirect calling the public C function for
> documentLoader->stopLoading()
> 
> FrameLoaderClient::dispatchDidCancelRedirect:
> - Emit client-redirect-cancelled (canceled?)
> 
> FrameLoaderClient::dispatchDidServerRedirectForProvisionalLoad:
> - This method is not very useful because it's impossible to know the
> destination of the redirect and the HTTP code sent by the server, so I would
> prefer to not emit anything. If a program needs to know about HTTP redirects it
> can use the signal emitted by dispatchWillSendRequest
> 

Agreed this should go in next patches.

[Christian Dywan]

This patch implements load-status and progress properties with according properties on the view, as well as load-status on the frame. It also includes porting GtkLauncher to the new interface.

The patch includes none of the other discussed changes and no deprecations, I figure to ease review we should do these bit by bit.

[Christian Dywan]

Created attachment 27001 [details]
Implement load-status and progress properties

Oh well, this time I'm actually attaching a patch ^_~.

[Gustavo Noronha (kov)]

Comment on attachment 27001 [details]
Implement load-status and progress properties

>  #include "webkitwebframe.h"
>  #include "webkitwebview.h"
> +#include "webkitenumtypes.h"
>  #include "webkitmarshal.h"
>  #include "webkitprivate.h"

This header addition is not in the correct order.

> +    * Since: 1.1.1

We now need to use 1.1.7 here =).

> --- a/WebKitTools/GtkLauncher/main.c
> +++ b/WebKitTools/GtkLauncher/main.c

All changes to the launcher look good to me, but please put them in another commit.

>      gtk_init (&argc, &argv);
> +    if (!g_thread_supported ())
> +        g_thread_init (NULL);
>  

And this is no longer needed, but you know this, of course =).

The patch looks good, and since we have already had lots of discussion on this already, so I'll r+ and you can address my comments while landing.

[Christian Dywan]

2009-05-06  Christian Dywan  <[e-mail]>

        Reviewed by Gustavo Noronha.

        http://bugs.webkit.org/show_bug.cgi?id=17066
        [GTK] Improve frameloader signals

        Implement load-status and progress properties on the view, as well as
        load-status on the frame. This supersedes the different load signals
        load-progress-changed, load-committed, load-done, load-started and
        load-finished which are not only misnamed but broken by design.

        * WebCoreSupport/FrameLoaderClientGtk.cpp:
        (WebKit::notifyStatus):
        (WebKit::FrameLoaderClient::postProgressStartedNotification):
        (WebKit::FrameLoaderClient::postProgressEstimateChangedNotification):
        (WebKit::FrameLoaderClient::dispatchDidFinishLoad):
        (WebKit::FrameLoaderClient::dispatchDidStartProvisionalLoad):
        * webkit/webkitprivate.h:
        * webkit/webkitwebframe.cpp:
        * webkit/webkitwebframe.h:
        * webkit/webkitwebview.cpp:
        * webkit/webkitwebview.h:

[Christian Dywan]

2009-05-06  Christian Dywan  <[e-mail]>

        Reviewed by Gustavo Noronha.

        http://bugs.webkit.org/show_bug.cgi?id=17066
        [GTK] Improve frameloader signals

        Update GtkLauncher to use the new load-status and progress properties
        instead of the previous loading signals.

        * GtkLauncher/main.c:
        (update_title):
        (notify_load_status_cb):
        (notify_progress_cb):
        (create_browser):
        (create_window):

[Gustavo Noronha (kov)]

Guess this is now done? =)

[Christian Dywan]

We don't have redirection yet. FrameLoader, as referred to in the title and as discussed here, also features redirecting pages.

[Gustavo Noronha (kov)]

Comment on attachment 27001 [details]
Implement load-status and progress properties

You are right! Let me clear this review flag so that my crude search will not show this bug, then =).

[Brian Holt]

Is this bug still an issue? What still needs to be done or can it be closed?

[Martin Robinson]

WebKit1GTK+ has been removed.