Bug 26409 – [GTK] Glitches in new progress/load-status system

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Xan Lopez]

There's a couple of issues with the new progress and load-status signals of the WebView:

- A weird 10% progress is reported for pages before the status even reaches PROVISIONAL. Not sure where this comes from, but it should either go away or come after PROVISIONAL or COMMITTED I think.

- FINISHED comes before 100% progress is reported. I believe the natural expectation would be to receive 100% while the status is still not finished, and *then* receive FINISHED.

You can see both issues with the attached testcase.

[Xan Lopez]

Created attachment 31294 [details]
testprogress.c

A simple testcase.

Output is like this:

** Message: Progress is 0.100000
** Message: Status is WEBKIT_LOAD_PROVISIONAL
** Message: Status is WEBKIT_LOAD_COMMITTED
** Message: Progress is 0.500000
** Message: Progress is 0.669581
** Message: Progress is 0.698058
** Message: Progress is 0.718557
** Message: Progress is 0.746067
** Message: Progress is 0.767503
** Message: Progress is 0.791314
** Message: Progress is 0.814049
** Message: Progress is 0.816215
** Message: Status is WEBKIT_LOAD_FINISHED
** Message: Progress is 1.000000

[Gustavo Noronha (kov)]

Created attachment 31594 [details]
Initial test case for loading

 ChangeLog                      |    8 +++
 GNUmakefile.am                 |    6 ++
 WebKit/gtk/ChangeLog           |   12 ++++
 WebKit/gtk/tests/testloading.c |  117 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 143 insertions(+), 0 deletions(-)

[Gustavo Noronha (kov)]

Comment on attachment 31594 [details]
Initial test case for loading

An initial test case for the loading API. We will want to also cover the progress here, in a future patch.

[Jan Alonzo]

Comment on attachment 31594 [details]
Initial test case for loading

> index 802a56b..34d5ced 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,11 @@
> [phone]  Gustavo Noronha Silva  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Initial test case for loading.

Need to be more descriptive here.

> +static void
> +load_finished_cb(WebKitWebView* web_view, WebKitWebFrame* web_frame, gpointer data)
> +{

Please put this in one line to match the other tests.

> +    return g_test_run ();

remove spaces.

r=me with these changes.

[Gustavo Noronha (kov)]

Landed in r44898.

[Gustavo Noronha (kov)]

Not really fixed.

[Jan Alonzo]

Comment on attachment 31594 [details]
Initial test case for loading

Clearing flag so it will not appear in the commit queue.

[Carlos Garcia Campos]

Created attachment 33975 [details]
Test case for FIRST_VISUALLY_NON_EMPTY_LAYOUT after FINISHED

This is a test case for another issue I've found in load_status system. When loading more than one document at the same time, sometimes, for some of the documents, WEBKIT_LOAD_FIRST_VISUALLY_NON_EMPTY_LAYOUT status is reported after WEBKIT_LOAD_FINISHED. The test program receives gnome bugs numbers.

$ ./test 102382 105129 106872 
** Message: Status for (null) is WEBKIT_LOAD_PROVISIONAL
** Message: Status for (null) is WEBKIT_LOAD_PROVISIONAL
** Message: Status for (null) is WEBKIT_LOAD_PROVISIONAL
** Message: Status for http://bugzilla.gnome.org/show_bug.cgi?id=102382 is WEBKIT_LOAD_COMMITTED
** Message: Status for http://bugzilla.gnome.org/show_bug.cgi?id=105129 is WEBKIT_LOAD_COMMITTED
** Message: Status for http://bugzilla.gnome.org/show_bug.cgi?id=105129 is WEBKIT_LOAD_FINISHED
** Message: Status for http://bugzilla.gnome.org/show_bug.cgi?id=106872 is WEBKIT_LOAD_COMMITTED
** Message: Status for http://bugzilla.gnome.org/show_bug.cgi?id=106872 is WEBKIT_LOAD_FIRST_VISUALLY_NON_EMPTY_LAYOUT
** Message: Status for http://bugzilla.gnome.org/show_bug.cgi?id=102382 is WEBKIT_LOAD_FIRST_VISUALLY_NON_EMPTY_LAYOUT
** Message: Status for http://bugzilla.gnome.org/show_bug.cgi?id=105129 is WEBKIT_LOAD_FIRST_VISUALLY_NON_EMPTY_LAYOUT
** Message: Status for http://bugzilla.gnome.org/show_bug.cgi?id=106872 is WEBKIT_LOAD_FINISHED
** Message: Status for http://bugzilla.gnome.org/show_bug.cgi?id=102382 is WEBKIT_LOAD_FINISHED

In this case for bug 105129, FINISHED is sent before FIRST_VISUALLY_NON_EMPTY_LAYOUT

[Benjamin Otte]

(In reply to comment #0)
> - A weird 10% progress is reported for pages before the status even reaches
> PROVISIONAL. Not sure where this comes from, but it should either go away or
> come after PROVISIONAL or COMMITTED I think.
> 
This is wanted behavior. See WebCore/loader/ProgressTracker.cpp. The current implementation clamps progress values to always be inside [0.1, 0.9] during an active load, so that you can actually see that something is happening.

[Christian Dywan]

> - A weird 10% progress is reported for pages before the status even reaches
> PROVISIONAL. Not sure where this comes from, but it should either go away or
> come after PROVISIONAL or COMMITTED I think.

Arguably a provisional load is already doing *something*, this comes down to what you perceive as progress. A user may be confused when he hits return and the progress bar is empty because it takes ages to obtain a response from the remote end. So I don't really think it's wrong.

> - FINISHED comes before 100% progress is reported. I believe the natural
> expectation would be to receive 100% while the status is still not finished,
> and *then* receive FINISHED.

The FrameLoadClientGtk relies on FrameLoader. And FrameLoader actually has guards to catch this case, so it looks like a bug in WebCore. I mostly hacked the Gtk bits so I can't easily say where to look.  I guess this needs some looking at debugging output.

[Xan Lopez]

(In reply to comment #10)
> > - FINISHED comes before 100% progress is reported. I believe the natural
> > expectation would be to receive 100% while the status is still not finished,
> > and *then* receive FINISHED.
> 
> The FrameLoadClientGtk relies on FrameLoader. And FrameLoader actually has
> guards to catch this case, so it looks like a bug in WebCore. I mostly hacked
> the Gtk bits so I can't easily say where to look.  I guess this needs some
> looking at debugging output.

It does not seem to be a bug but a deliberate decision. This patch fixes it:

diff --git a/WebCore/loader/FrameLoader.cpp b/WebCore/loader/FrameLoader.cpp
index 4530c11..f187b5d 100644
--- a/WebCore/loader/FrameLoader.cpp
+++ b/WebCore/loader/FrameLoader.cpp
@@ -3386,13 +3386,14 @@ void FrameLoader::checkLoadCompleteForThisFrame()
 #ifndef NDEBUG
             m_didDispatchDidCommitLoad = false;
 #endif
+            if (Page* page = m_frame->page())
+                page->progress()->progressCompleted(m_frame);
+
             if (!error.isNull())
                 m_client->dispatchDidFailLoad(error);
             else
                 m_client->dispatchDidFinishLoad();

-            if (Page* page = m_frame->page())
-                page->progress()->progressCompleted(m_frame);
             return;
         }

I guess I'll poke someone to see if it would be reasonable to change this.

[Xan Lopez]

(In reply to comment #11)
> I guess I'll poke someone to see if it would be reasonable to change this.

I opened a bug for this, https://bugs.webkit.org/show_bug.cgi?id=28851

[Xan Lopez]

(In reply to comment #8)
> Created an attachment (id=33975) [details]
> Test case for FIRST_VISUALLY_NON_EMPTY_LAYOUT after FINISHED
> 
> This is a test case for another issue I've found in load_status system. When
> loading more than one document at the same time, sometimes, for some of the
> documents, WEBKIT_LOAD_FIRST_VISUALLY_NON_EMPTY_LAYOUT status is reported after
> WEBKIT_LOAD_FINISHED. The test program receives gnome bugs numbers.
> 
> $ ./test 102382 105129 106872 
> ** Message: Status for (null) is WEBKIT_LOAD_PROVISIONAL
> ** Message: Status for (null) is WEBKIT_LOAD_PROVISIONAL
> ** Message: Status for (null) is WEBKIT_LOAD_PROVISIONAL
> ** Message: Status for http://bugzilla.gnome.org/show_bug.cgi?id=102382 is
> WEBKIT_LOAD_COMMITTED
> ** Message: Status for http://bugzilla.gnome.org/show_bug.cgi?id=105129 is
> WEBKIT_LOAD_COMMITTED
> ** Message: Status for http://bugzilla.gnome.org/show_bug.cgi?id=105129 is
> WEBKIT_LOAD_FINISHED
> ** Message: Status for http://bugzilla.gnome.org/show_bug.cgi?id=106872 is
> WEBKIT_LOAD_COMMITTED
> ** Message: Status for http://bugzilla.gnome.org/show_bug.cgi?id=106872 is
> WEBKIT_LOAD_FIRST_VISUALLY_NON_EMPTY_LAYOUT
> ** Message: Status for http://bugzilla.gnome.org/show_bug.cgi?id=102382 is
> WEBKIT_LOAD_FIRST_VISUALLY_NON_EMPTY_LAYOUT
> ** Message: Status for http://bugzilla.gnome.org/show_bug.cgi?id=105129 is
> WEBKIT_LOAD_FIRST_VISUALLY_NON_EMPTY_LAYOUT
> ** Message: Status for http://bugzilla.gnome.org/show_bug.cgi?id=106872 is
> WEBKIT_LOAD_FINISHED
> ** Message: Status for http://bugzilla.gnome.org/show_bug.cgi?id=102382 is
> WEBKIT_LOAD_FINISHED
> 
> In this case for bug 105129, FINISHED is sent before
> FIRST_VISUALLY_NON_EMPTY_LAYOUT

So, after looking a bit at the code in FrameView.cpp I'm not sure there's anything wrong with this. It simple means we received all the data before WebCore had a chance to do the first visually non empty layout, so the order of the notifications makes sense I'd say. We should probably add a note to the docs and adapt Epiphany to better take this into account.

[Xan Lopez]

(In reply to comment #13)
> So, after looking a bit at the code in FrameView.cpp I'm not sure there's
> anything wrong with this. It simple means we received all the data before
> WebCore had a chance to do the first visually non empty layout, so the order of
> the notifications makes sense I'd say. We should probably add a note to the
> docs and adapt Epiphany to better take this into account.

I asked on IRC and aroben confirmed that yes, this is perfectly OK. Since the 10% stuff is also intentional and I have opened a different bug for the DONE/100% ordering issue, I'll close this bug now.