Bug 28851 - Emit last progress notification before calling dispatchDidFinishLoad
Summary: Emit last progress notification before calling dispatchDidFinishLoad
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Caio Marcelo de Oliveira Filho
URL:
Keywords:
Depends on:
Blocks: 66243 66772 66773
  Show dependency treegraph
 
Reported: 2009-08-31 11:51 PDT by Xan Lopez
Modified: 2011-09-05 09:41 PDT (History)
8 users (show)

See Also:


Attachments
progressbeforefinished.patch (2.14 KB, patch)
2009-08-31 11:53 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
Patch (11.10 KB, patch)
2011-08-10 14:52 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (23.71 KB, patch)
2011-08-17 14:46 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (23.74 KB, patch)
2011-08-18 07:16 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (24.93 KB, patch)
2011-08-23 06:31 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch for landing (24.88 KB, patch)
2011-08-30 12:47 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch for landing (24.91 KB, patch)
2011-08-30 13:29 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xan Lopez 2009-08-31 11:51:18 PDT
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.
Comment 1 Xan Lopez 2009-08-31 11:53:22 PDT
Created attachment 38827 [details]
progressbeforefinished.patch
Comment 2 Eric Seidel 2009-08-31 15:45:46 PDT
I believe Brady is still our loader expert.
Comment 3 Xan Lopez 2009-08-31 23:01:01 PDT
(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 :)
Comment 4 Xan Lopez 2009-09-11 00:44:37 PDT
Ping?
Comment 5 Brady Eidson 2009-09-11 09:34:03 PDT
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...  ;)
Comment 6 Brady Eidson 2009-09-11 09:34:36 PDT
Comment on attachment 38827 [details]
progressbeforefinished.patch

Code change looks good, but we can't land this without layout tests.
Comment 7 Xan Lopez 2009-09-17 04:54:37 PDT
(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?
Comment 8 Caio Marcelo de Oliveira Filho 2011-08-10 14:52:48 PDT
Created attachment 103537 [details]
Patch
Comment 9 Caio Marcelo de Oliveira Filho 2011-08-10 14:54:39 PDT
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.
Comment 10 Adam Barth 2011-08-10 15:00:41 PDT
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.
Comment 11 Caio Marcelo de Oliveira Filho 2011-08-17 14:46:29 PDT
Created attachment 104251 [details]
Patch
Comment 12 Caio Marcelo de Oliveira Filho 2011-08-17 14:53:35 PDT
(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.
Comment 13 WebKit Review Bot 2011-08-17 18:03:20 PDT
Comment on attachment 104251 [details]
Patch

Attachment 104251 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9420263
Comment 14 Caio Marcelo de Oliveira Filho 2011-08-18 07:16:27 PDT
Created attachment 104336 [details]
Patch
Comment 15 Caio Marcelo de Oliveira Filho 2011-08-23 06:31:44 PDT
Created attachment 104834 [details]
Patch
Comment 16 Caio Marcelo de Oliveira Filho 2011-08-23 06:34:25 PDT
(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.
Comment 17 Caio Marcelo de Oliveira Filho 2011-08-23 06:35:45 PDT
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?
Comment 18 Martin Robinson 2011-08-24 09:33:16 PDT
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?
Comment 19 Caio Marcelo de Oliveira Filho 2011-08-24 10:40:02 PDT
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.
Comment 20 Martin Robinson 2011-08-24 10:51:41 PDT
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.
Comment 21 Adam Barth 2011-08-30 12:30:50 PDT
Looks like everyone is happy with this patch.
Comment 22 WebKit Review Bot 2011-08-30 12:41:18 PDT
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
Comment 23 Caio Marcelo de Oliveira Filho 2011-08-30 12:47:59 PDT
Created attachment 105671 [details]
Patch for landing
Comment 24 WebKit Review Bot 2011-08-30 13:24:06 PDT
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
Comment 25 Caio Marcelo de Oliveira Filho 2011-08-30 13:29:04 PDT
Created attachment 105684 [details]
Patch for landing
Comment 26 WebKit Review Bot 2011-08-30 13:41:53 PDT
Comment on attachment 105684 [details]
Patch for landing

Clearing flags on attachment: 105684

Committed r94105: <http://trac.webkit.org/changeset/94105>
Comment 27 WebKit Review Bot 2011-08-30 13:41:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Ademar Reis 2011-09-05 09:41:26 PDT
Revision r94105 cherry-picked into qtwebkit-2.2 with commit 3f29aef <http://gitorious.org/webkit/qtwebkit/commit/3f29aef>