Bug 28851 – Emit last progress notification before calling dispatchDidFinishLoad

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Xan Lopez]

When a frame finishes loading the last batch of progress reports and status signals goes like this:

... 80%, 90%, FINISHED, 100%.

This is because the code in FrameLoader::checkLoadCompleteForThisFrame will call first progressCompleted() and then dispatchDidFinishLoad(). IMHO this goes against the natural expectation of receiving all the progress reports before the FINISHED notification, so I propose to reverse the order of the function calls. The attached patch does that, and does not seem to regress any layout tests here.

This surely should get some tests with it, but I'm not sure of what should I be testing, so suggestions are welcome.

[Xan Lopez]

Created attachment 38827 [details]
progressbeforefinished.patch

[Eric Seidel (no email)]

I believe Brady is still our loader expert.

[Xan Lopez]

(In reply to comment #0)
> This is because the code in FrameLoader::checkLoadCompleteForThisFrame will
> call first progressCompleted() and then dispatchDidFinishLoad(). 

I guess it's obvious, but the current code does exactly the opposite and that is what I'm proposing to do now. Sorry for the confusion :)

[Xan Lopez]

Ping?

[Brady Eidson]

The concept is fine.
If all layout tests continue to pass on all platforms, then the patch is fine.

It makes me nervous that we've had this behavior forever and are changing it now.

But we need tests.  That we've had this bizarre behavior forever and no layout test covered it is something that needs to change.

DRT already listens to didFinishLoadForFrame, but can definitely be augmented to listen for the progress notifications also.

Sometimes the DRT changes to test a patch are 20 times bigger than the patch itself - This might be one of those times...  ;)

[Brady Eidson]

Comment on attachment 38827 [details]
progressbeforefinished.patch

Code change looks good, but we can't land this without layout tests.

[Xan Lopez]

(In reply to comment #6)
> (From update of attachment 38827 [details])
> Code change looks good, but we can't land this without layout tests.

Right, so my point is that I'm not really sure what should I be testing exactly. I suppose ideally you'd identify possible places in the code that *could* be relying on the old behavior and see that they don't break, but that's surely impossible to do correctly or thoroughly? Could you give me any indication about what kind of test would you like to see here?

[Caio Marcelo de Oliveira Filho]

Created attachment 103537 [details]
Patch

[Caio Marcelo de Oliveira Filho]

This patch still doesn't cover all the platforms, but it's already working on Qt port. I uploaded to get some feedback whether the test is correct, or if we need a different kind of testing.

The "unnatural" ordering also surfaces on QtWebKit API, I'm making tests to cover the API as well.

[Adam Barth]

Comment on attachment 103537 [details]
Patch

I think this is the right approach, but we'll need implementations on all the ports (sadly).  I agree with Comment #5.

[Caio Marcelo de Oliveira Filho]

Created attachment 104251 [details]
Patch

[Caio Marcelo de Oliveira Filho]

(Work in progress)

Implemented the necessary things in Chromium and GTK DRT. Chromium and GTK LayoutTests work exactly like without them in my machine. I don't get 100% pass in neither, but the number of failings don't change, some help in testing will be appreciated.

API tests for Chromium pass, but I think they don't exercise this change. GTK ones showed a problem, that I think I've fixed in bug 66243, that I'm marking as dependency for this bug.

[WebKit Review Bot]

Comment on attachment 104251 [details]
Patch

Attachment 104251 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9420263

[Caio Marcelo de Oliveira Filho]

Created attachment 104336 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Created attachment 104834 [details]
Patch

[Caio Marcelo de Oliveira Filho]

(In reply to comment #15)
> Created an attachment (id=104834) [details]
> Patch

Adding skips for Mac and Win ports with links to bug reports to implement them.

[Caio Marcelo de Oliveira Filho]

Martin and Xan, since the patch involves a bit of non-trivial changes in GTK implementation of DRT would be very nice to get some review/comments from GTK reviewers. Could one of you take a look at this patch?

[Martin Robinson]

Comment on attachment 104834 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104834&action=review

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:785
> +    // The deprecated "load-finished" signal is triggered by postProgressFinishedNotification(),
> +    // so we can use it here in the DRT to provide the correct dump.
>      if (frame != topLoadingFrame)
>          return;
> -
> -    topLoadingFrame = 0;
> -    WorkQueue::shared()->setFrozen(true); // first complete load freezes the queue for the rest of this test
> -    if (gLayoutTestController->waitToDump())
> -        return;
> -
> -    if (WorkQueue::shared()->count())
> -        g_timeout_add(0, processWork, 0);
> -    else
> -        dump();
> +    if (gLayoutTestController->dumpProgressFinishedCallback())
> +        printf("postProgressFinishedNotification\n");

I'm not sure I understand why you are doing this work at a different time now.

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1051
>          case WEBKIT_LOAD_FINISHED:
> -            if (frame != topLoadingFrame || !done)
> +            if (!done)
>                  printf("%s - didFinishLoadForFrame\n", frameName.get());
>              break;

Why do we now print this notification for non-main-frames?

[Caio Marcelo de Oliveira Filho]

Comment on attachment 104834 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104834&action=review

Martin, thanks for the review.

>> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:785
>> +        printf("postProgressFinishedNotification\n");
> 
> I'm not sure I understand why you are doing this work at a different time now.

This work was happening (in FrameLoaderClient terms) after the load has finished, during the progress finished. With the patch those two moments will happen in reverse order. Which means that is too soon for clearing "topLoadingFrame" as well as to call dump() (that will set "done = true"), because we still have an load-status callback to process later.

>> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1051
>>              break;
> 
> Why do we now print this notification for non-main-frames?

I didn't understand your question. Did you mean "for main-frames"?

If so, we should print the didFinishLoadForFrame for main frames as well, and this is done even before this patch. The topLoadingFrame was being printed because we were "!done" yet, and after we would get progress finished that would change set done to true and reset topLoadingFrame. My understanding is that in this code, the first half of the condition wasn't helping at all.

[Martin Robinson]

Comment on attachment 104834 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104834&action=review

The GTK+ parts look good to me.

>>> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:785
>>> +        printf("postProgressFinishedNotification\n");
>> 
>> I'm not sure I understand why you are doing this work at a different time now.
> 
> This work was happening (in FrameLoaderClient terms) after the load has finished, during the progress finished. With the patch those two moments will happen in reverse order. Which means that is too soon for clearing "topLoadingFrame" as well as to call dump() (that will set "done = true"), because we still have an load-status callback to process later.

Makes sense.

>>> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1051
>>>              break;
>> 
>> Why do we now print this notification for non-main-frames?
> 
> I didn't understand your question. Did you mean "for main-frames"?
> 
> If so, we should print the didFinishLoadForFrame for main frames as well, and this is done even before this patch. The topLoadingFrame was being printed because we were "!done" yet, and after we would get progress finished that would change set done to true and reset topLoadingFrame. My understanding is that in this code, the first half of the condition wasn't helping at all.

Thanks for clearing that up.

[Adam Barth]

Looks like everyone is happy with this patch.

[WebKit Review Bot]

Comment on attachment 104834 [details]
Patch

Rejecting attachment 104834 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
rTree/chromium/LayoutTestController.h
Hunk #2 succeeded at 415 (offset 2 lines).
Hunk #3 succeeded at 549 (offset 2 lines).
patching file Tools/DumpRenderTree/chromium/WebViewHost.cpp
patching file Tools/DumpRenderTree/gtk/DumpRenderTree.cpp
patching file Tools/DumpRenderTree/qt/LayoutTestControllerQt.cpp
patching file Tools/DumpRenderTree/qt/LayoutTestControllerQt.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/9564544

[Caio Marcelo de Oliveira Filho]

Created attachment 105671 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 105671 [details]
Patch for landing

Rejecting attachment 105671 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
tTestController.h
patching file Tools/DumpRenderTree/chromium/LayoutTestController.cpp
patching file Tools/DumpRenderTree/chromium/LayoutTestController.h
patching file Tools/DumpRenderTree/chromium/WebViewHost.cpp
patching file Tools/DumpRenderTree/gtk/DumpRenderTree.cpp
patching file Tools/DumpRenderTree/qt/LayoutTestControllerQt.cpp
patching file Tools/DumpRenderTree/qt/LayoutTestControllerQt.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/9567518

[Caio Marcelo de Oliveira Filho]

Created attachment 105684 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 105684 [details]
Patch for landing

Clearing flags on attachment: 105684

Committed r94105: <http://trac.webkit.org/changeset/94105>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r94105 cherry-picked into qtwebkit-2.2 with commit 3f29aef <http://gitorious.org/webkit/qtwebkit/commit/3f29aef>