[webkit-reviews] review requested: [Bug 61328] [Qt] QtWebkit never finishes loading sites when they are loaded after an initial QUrl fails to load. : [Attachment 96184] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jun 6 19:52:52 PDT 2011


Caio Marcelo de Oliveira Filho <cmarcelo at webkit.org> has asked	for review:
Bug 61328: [Qt] QtWebkit never finishes loading sites when they are loaded
after an initial QUrl fails to load.
https://bugs.webkit.org/show_bug.cgi?id=61328

Attachment 96184: Patch
https://bugs.webkit.org/attachment.cgi?id=96184&action=review

------- Additional Comments from Caio Marcelo de Oliveira Filho
<cmarcelo at webkit.org>
Just some context:

- The core of the problem is that when FrameLoaderClient makes callbacks to
user code, we must be sure that the FrameLoader is in a consistent state.

- I tried enqueueing the signals, but this led to other problems like: changing
order of expected signals in DRT, and ultimately, the need of enqueueing
setUrl()/load() calls as well to avoid loops due to setUrl() -> cancel load ->
enqueue loadFinished(false) -> start load -> process loadFinished(false) ->
setUrl() -> ...

- I could try to manipulate FrameLoader code to make sure that state will be
consistent, but I think this would be always hard to keep, specially because
other APIs doesn't use progress notification for this kind of signal. This new
approach is used both by Mac and Gtk. I'm not really sure if the FrameLoader
need to be ready for taking a new load() at every point in FrameLoaderClient.
The progress notification were a gray area for me in that matter. I would be
glad to hear opinions about this particular point.

- Now every frame will have its own loadStarted() and loadFinished(). We didn't
emitted those for every frame before, only for the "originating frame", because
the progress notification only happen in the "originating frame". QWebPage
signals continue to be emitted only when the "originating frame" start/finish.

- I'm assuming that: the last frame to be loaded will be the "originating
frame". This seems to match pieces of code I looked elsewhere.

- This commit will surface the issue reported in
https://bugs.webkit.org/show_bug.cgi?id=28851. I prefer that we fix this other
problem in a separate commit, since it might demand different testing. We can
try to workaround at Qt level too, but I don't see why not use 28851 :-)


More information about the webkit-reviews mailing list