Bug 60412

<rdar://problem/9369450>

Created attachment 92651 [details] proposed patch

Comment on attachment 92651 [details] proposed patch Code change looks fine. I know it's probably not possible to test this right now, but it should be possible adding a "setDefersLoading" call to DRT.

The history.back() JS function doesn't exhibit the issue, since it uses a NavigationScheduler. I'm going to have to add two hooks -- one for disabling / enabling load deferral, and one for calling -[WebView goBack] directly.

Created attachment 92843 [details] with tests!

Created attachment 92847 [details] ...and applying cleanly on ToT last patch didn't apply cleanly on ToT, here's a new one

Attachment 92847 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/load..." exit_code: 1 Tools/DumpRenderTree/LayoutTestController.h:102: The parameter name "defers" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 92847 [details] ...and applying cleanly on ToT Attachment 92847 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8660677 New failing tests: loader/navigation-while-deferring-loads.html

Created attachment 92854 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 92862 [details] perhaps this will appease the bots?

Attachment 92862 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/load..." exit_code: 1 LayoutTests/platform/chromium/test_expectations.txt:210: Missing expectations. [u' loader/navigation-while-deferring-loads.html'] [test/expectations] [5] Total errors found: 1 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 92862 [details] perhaps this will appease the bots? View in context: https://bugs.webkit.org/attachment.cgi?id=92862&action=review > Tools/DumpRenderTree/LayoutTestController.cpp:2339 > { "setDeferMainResourceDataLoad", setDeferMainResourceDataLoadCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete }, > + { "setDefersLoading", setDefersLoadingCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete }, FWIW, I'm not able to tell what the difference between these two functions is.

Comment on attachment 92862 [details] perhaps this will appease the bots? View in context: https://bugs.webkit.org/attachment.cgi?id=92862&action=review >> LayoutTests/platform/chromium/test_expectations.txt:210 >> +SKIP : loader/navigation-while-deferring-loads.html > > Missing expectations. [u' loader/navigation-while-deferring-loads.html'] [test/expectations] [5] You need a goofy " = FAIL" at the end of this line.

Comment on attachment 92862 [details] perhaps this will appease the bots? View in context: https://bugs.webkit.org/attachment.cgi?id=92862&action=review > Source/WebCore/page/Page.h:407 > + RefPtr<HistoryItem> m_deferredItem; > + FrameLoadType m_deferredFrameLoadType; I suspect that this isn't the best place for this data. Generally, the large "hub" objects like Frame and Page shouldn't hold much data themselves because they become "dumping grounds" for lots of unrelated stuff. I haven't fully understood what your patch does, but these seem related to history, so perhaps the HistoryController is a more appropriate place?

Comment on attachment 92862 [details] perhaps this will appease the bots? View in context: https://bugs.webkit.org/attachment.cgi?id=92862&action=review >> Tools/DumpRenderTree/LayoutTestController.cpp:2339 >> + { "setDefersLoading", setDefersLoadingCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete }, > > FWIW, I'm not able to tell what the difference between these two functions is. DocumentLoader has methods with both of these names. I'd argue setDefersLoading would be more common anyways. The main resource one is very old, and I don't know who really uses it.

(In reply to comment #14) > (From update of attachment 92862 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=92862&action=review > > > Source/WebCore/page/Page.h:407 > > + RefPtr<HistoryItem> m_deferredItem; > > + FrameLoadType m_deferredFrameLoadType; > > I suspect that this isn't the best place for this data. Generally, the large "hub" objects like Frame and Page shouldn't hold much data themselves because they become "dumping grounds" for lots of unrelated stuff. I haven't fully understood what your patch does, but these seem related to history, so perhaps the HistoryController is a more appropriate place? Thanks for the review. That makes a lot of sense; I'll post a patch that holds the data in HistoryController instead.

Created attachment 93626 [details] move state to HistoryController

Created attachment 93627 [details] move state to HistoryController… and merge cleanly

Comment on attachment 93627 [details] move state to HistoryController… and merge cleanly OK

Comment on attachment 93627 [details] move state to HistoryController… and merge cleanly Rejecting attachment 93627 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'bu..." exit_code: 2 Last 500 characters of output: tests/xmlhttprequest ................................................................................................................................................................................ http/tests/xmlhttprequest/web-apps ............... http/tests/xmlhttprequest/workers ........... http/tests/xmlviewer . http/tests/xmlviewer/dumpAsText ........... 779.13s total testing time 23537 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 16 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/8705100

Created attachment 93698 [details] Archive of layout-test-results from eseidel-cq-sf The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: eseidel-cq-sf Port: Mac Platform: Mac OS X 10.6.7

That's because the test doesn't work on Chromium right now. I tried to skip it by adding a line to LayoutTests/platform/chromium/test_expectations.txt, but apparently that didn't work. Is there something else I need to do?

(In reply to comment #22) > That's because the test doesn't work on Chromium right now. I tried to skip it by adding a line to LayoutTests/platform/chromium/test_expectations.txt, but apparently that didn't work. Is there something else I need to do? Oh, never mind. Does the commit queue rebuild DumpRenderTree?

The commit-queue runs build-webkit and run-webkit-tests on Mac. So yes, it rebuilds DRT. It runs with a Release build.

Created attachment 93707 [details] modified layout test Sorry for the noise. I believe the test was depending on the "dumpAsText" flag persisting between page loads. This new patch contains a (hopefully) fixed layout test.

Comment on attachment 93707 [details] modified layout test Re-adding Darin's r+. The patch also looks good to me.

The commit-queue encountered the following flaky tests while processing attachment 93707 [details]: http/tests/xmlhttprequest/cross-origin-no-authorization.html bug 33357 (author: ap@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 93707 [details] modified layout test Clearing flags on attachment: 93707 Committed r86644: <http://trac.webkit.org/changeset/86644>

All reviewed patches have been landed. Closing bug.

Comment on attachment 93707 [details] modified layout test View in context: https://bugs.webkit.org/attachment.cgi?id=93707&action=review > Tools/DumpRenderTree/mac/LayoutTestControllerMac.mm:438 > + [[mainFrame webView] goBack]; isn't this redundant, no? why not just use layoutTestController.queueBackNavigation? that is implemented using [[mainFrame webView] goBack] (see WorkQueueItemMac.mm)

(In reply to comment #30) > (From update of attachment 93707 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93707&action=review > > > Tools/DumpRenderTree/mac/LayoutTestControllerMac.mm:438 > > + [[mainFrame webView] goBack]; > > isn't this redundant, no? why not just use layoutTestController.queueBackNavigation? > > that is implemented using [[mainFrame webView] goBack] (see WorkQueueItemMac.mm) Good point. The method is redundant. I filed <http://webkit.org/b/60976> Remove redundant goBack method on layout test controller.

Comment on attachment 93707 [details] modified layout test View in context: https://bugs.webkit.org/attachment.cgi?id=93707&action=review > Source/WebCore/loader/HistoryController.cpp:252 > + if (m_defersLoading) { one more question: why not put this at the top of the function? it seems like goToItem should have no side-effects in the case that it is deferred. as is, the client's shouldGoToHistoryItem will be called once when the operation is deferred and once when it becomes un-deferred. i think it would be safer to put the check at the top of the function.

(In reply to comment #32) > (From update of attachment 93707 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93707&action=review > > > Source/WebCore/loader/HistoryController.cpp:252 > > + if (m_defersLoading) { > > one more question: why not put this at the top of the function? it seems like > goToItem should have no side-effects in the case that it is deferred. as is, > the client's shouldGoToHistoryItem will be called once when the operation is > deferred and once when it becomes un-deferred. i think it would be safer to > put the check at the top of the function. It seems incorrect for "m_frame->loader()->client()->shouldGoToHistoryItem(targetItem)" to have any side-effects. I figured that if the client doesn't want us to go to the given history item now, we shouldn't save the navigation for later, either.

(In reply to comment #33) > > one more question: why not put this at the top of the function? it seems like > > goToItem should have no side-effects in the case that it is deferred. as is, > > the client's shouldGoToHistoryItem will be called once when the operation is > > deferred and once when it becomes un-deferred. i think it would be safer to > > put the check at the top of the function. > > It seems incorrect for "m_frame->loader()->client()->shouldGoToHistoryItem(targetItem)" to have any side-effects. I figured that if the client doesn't want us to go to the given history item now, we shouldn't save the navigation for later, either. I understand, but the client will presumably answer the question of whether or not they want to go to the given history item later too. Why ask the client twice? It doesn't seem to add much value. It only adds the risk that the client might have side effects.