Bug 28842 – [GTK] Do not emit extra FINISHED load-status signals

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Xan Lopez]

We are emitting extra finished load-status signals when loading pages that will error. Example of the current flow:

- Try to load bogus page

** (epiphany:30316): DEBUG: NOTIFY STATUS PROVISIONAL about:blank
** (epiphany:30316): DEBUG: dispatchDidFailProvisionalLoad

dispatchDidFailProvisionalLoad calls dispatchDidFailLoad

** (epiphany:30316): DEBUG: dispatchDidFailLoad

- The load fails, we start loading the error page

** (epiphany:30316): DEBUG: NOTIFY STATUS PROVISIONAL about:blank

- dispatchDidFailLoad emits FINISHED

** (epiphany:30316): DEBUG: NOTIFY STATUS FINISHED about:blank

- dispatchDidFailProvisionalLoad *also* emits FINISHED

** (epiphany:30316): DEBUG: NOTIFY STATUS FINISHED about:blank

- The error page loads succesfully

** (epiphany:30316): DEBUG: NOTIFY STATUS COMMITTED http://suaoehuoeuh.com/
** (epiphany:30316): DEBUG: NOTIFY STATUS FIRST_VISUALLY_NON_EMPTY_LAYOUT http://suaoehuoeuh.com/
** (epiphany:30316): DEBUG: NOTIFY STATUS FINISHED http://suaoehuoeuh.com/

Basically, I don't think there's a reason we should emit the FINISHED status ourselves, we receive those we need to emit from WebCore. Removing that we get:

** (epiphany:27885): DEBUG: NOTIFY STATUS PROVISIONAL about:blank
** (epiphany:27885): DEBUG: dispatchDidFailProvisionalLoad
** (epiphany:27885): DEBUG: dispatchDidFailLoad
** (epiphany:27885): DEBUG: NOTIFY STATUS PROVISIONAL about:blank
** (epiphany:27885): DEBUG: NOTIFY STATUS COMMITTED http://usoethuoeau.com/
** (epiphany:27885): DEBUG: NOTIFY STATUS FIRST_VISUALLY_NON_EMPTY_LAYOUT http://usoethuoeau.com/
** (epiphany:27885): DEBUG: NOTIFY STATUS FINISHED http://usoethuoeau.com/

Which goes from PROVISIONAL to FINISHED, with one extra PROVISIONAL in the middle (which is ok) because the original page failed loading.

[Xan Lopez]

Created attachment 38805 [details]
noextrafinished.patch

Do not emit extra FINISHED load-status.

[Eric Seidel (no email)]

Comment on attachment 38805 [details]
noextrafinished.patch

Can't we test this?  I know we have loader delegate callback tests for Mac.  It seems it should be easy enough to do something similar for Gtk.  Seems silly to check in a change like this w/o automated testing. :(

[Eric Seidel (no email)]

Comment on attachment 38805 [details]
noextrafinished.patch

r- for lack of tests, and lack of explanation of why testing is impossible.  Feel free to mark r=? again with explanation.

[Xan Lopez]

(In reply to comment #3)
> (From update of attachment 38805 [details])
> r- for lack of tests, and lack of explanation of why testing is impossible. 
> Feel free to mark r=? again with explanation.

Fair enough. I'm not totally sure the fix is correct (although IMHO it improves things), so in a way I simply wanted to get comments on it by uploading the patch, but you are right we should be adding tests for these things.

[Xan Lopez]

Created attachment 38814 [details]
onlyonefinished.patch

Emit only one FINISHED load-status per cycle. Now with tests!

[Jan Alonzo]

Comment on attachment 38814 [details]
onlyonefinished.patch

r=me.

[Xan Lopez]

Landed as r47924, closing.