Tidy up LoadEvent mess

Bug #1398941 reported by Chris Coulson on 2014-12-03
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Oxide
Medium
Chris Coulson
1.3
Medium
Chris Coulson
1.4
Medium
Chris Coulson

Bug Description

Currently if a load fails, the renderer loads an error page. When this happens, we get a sequence of load events that look like this:

type=started, url="http://invalid/"
type=failed, url="http://invalid/"
type=started, url="data:text/html,chromewebdata"
type=committed, url="http://invalid/"
type=succeeded, url="data:text/html,chromewebdata"

It's easy to filter out those events with url=="data:text/html,chromewebdata" as being an error page (this URL doesn't change), but the committed event in the middle is confusing and difficult for an application to filter out.

There's a few potential solutions for cleaning this up:

- We could add an "isErrorPage" property to LoadEvent, which would be true for all of the last 3 events. We can detect that the committed event is an error page by checking the page type on the currently committed navigation entry.
- Alternatively, we could disable loading of the (currently non-existent / empty, but I'd like to provide a default page eventually) error page on the renderer side. This would get rid of these events entirely. Do we know whether QtWebkit or WebkitGTK provide a default error page?
- Another option would be to keep the error page but filter out the events. I'm less keen on this though, as it means we do commit a navigation entry but without emitting the appropriate committed notification.

Changed in oxide:
importance: Undecided → Medium
status: New → Triaged
milestone: none → branch-1.5
assignee: nobody → Chris Coulson (chrisccoulson)
Olivier Tilloy (osomon) wrote :

I guess either of the first two options would work.

I don’t know about WebKitGTK, but I just tested QtWebKit and it doesn’t provide a default error page (nor does it provide a way for applications to set one). When loading e.g. "http://invalid/" with QtWebKit, we get two load events: load started, and then load failed.

Whenever we implement a default error page, it would be good to expose an API to allow applications to override it.

Chris Coulson (chrisccoulson) wrote :

Ok, I'll probably just go with disabling it entirely for now then, so we'll behave more like QtWebkit

Changed in oxide:
status: Triaged → In Progress
Chris Coulson (chrisccoulson) wrote :

It turns out that disabling the built-in error page has unintended consequences.

When the initial load fails, the pending navigation entry is removed. When the error page load starts, a new pending entry is created and this gets committed with the URL that caused the error. This means that if we remove the error page then WebView.url reverts to the previously committed URL when a load fails, which is not the expected behaviour (it also explains why the committed event has a different URL).

I'm going to fix this by removing the extra started and succeeded events, and adding LoadEvent.isErrorPage. This means that the new sequence will be:

type=started, url="http://invalid/", isErrorPage=false
type=failed, url="http://invalid/", isErrorPage=false
type=committed, url="http://invalid/", isErrorPage=true

... with the committed event indicating that the webview has been navigated and the old document no longer exists. There are already other cases where we get a committed event without any corresponding started or succeeded events - eg, in-document navigations (where only the URL fragment changes) will do this.

Changed in oxide:
status: In Progress → Fix Released
Chris Coulson (chrisccoulson) wrote :

The fix for this does change the behaviour of WebView.loadEvent in a way which would break applications that depend on the broken behaviour. However, as this API was only introduced in 1.3 and the browser isn't using it, I don't think there will be any consequences for doing this. I wonder whether we should backport it to both 1.4 and 1.3 so that we don't end up with people depending on this bug (which would mean that we would have to fix it by introducing yet another new signal, whilst we keep the existing signal broken)?

Olivier Tilloy (osomon) wrote :

Yes, I think it would be best to backport the fix to 1.3 (and as a bonus it would allow us to start using it in the browser right away), as well as to 1.4.

The change looks good to me. Shouldn’t we add a Q_REVISION marker to the new isError property?

Chris Coulson (chrisccoulson) wrote :

I did think about that, but as LoadEvent can't be instantiated in QML, it doesn't really suffer the same scoping issues as other API's

Olivier Tilloy (osomon) wrote :

Right. However if the fix is backported to 1.3, client applications using the new property should be able to express that they want to use it by importing com.canonical.Oxide 1.3, and know that they won’t get it when importing an earlier version.

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers