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.
Created attachment 38805 [details] noextrafinished.patch Do not emit extra FINISHED load-status.
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. :(
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.
(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.
Created attachment 38814 [details] onlyonefinished.patch Emit only one FINISHED load-status per cycle. Now with tests!
Comment on attachment 38814 [details] onlyonefinished.patch r=me.
Landed as r47924, closing.