Page::goToItem doesn't work while loading is deferred

[Ian Henderson]

If you try to use Page::goToItem while loading is deferred, it just doesn't work.  We should save the passed-in item and try going to it again after loading resumes.

[Ian Henderson]

<rdar://problem/9369450>

[Ian Henderson]

Created attachment 92651 [details]
proposed patch

[Brady Eidson]

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.

[Ian Henderson]

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.

[Ian Henderson]

Created attachment 92843 [details]
with tests!

[Ian Henderson]

Created attachment 92847 [details]
...and applying cleanly on ToT

last patch didn't apply cleanly on ToT, here's a new one

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Ian Henderson]

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

[WebKit Review Bot]

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.

[Alexey Proskuryakov]

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.

[Adam Barth]

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.

[Adam Barth]

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?

[Joseph Pecoraro]

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.

[Ian Henderson]

(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.

[Ian Henderson]

Created attachment 93626 [details]
move state to HistoryController

[Ian Henderson]

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

[Darin Adler]

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

OK

[WebKit Commit Bot]

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

[WebKit Commit Bot]

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

[Ian Henderson]

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?

[Ian Henderson]

(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?

[Eric Seidel (no email)]

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

[Ian Henderson]

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.

[Joseph Pecoraro]

Comment on attachment 93707 [details]
modified layout test

Re-adding Darin's r+. The patch also looks good to me.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 93707 [details]:

http/tests/xmlhttprequest/cross-origin-no-authorization.html bug 33357 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 93707 [details]
modified layout test

Clearing flags on attachment: 93707

Committed r86644: <http://trac.webkit.org/changeset/86644>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Fisher (:fishd, Google)]

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)

[Ian Henderson]

(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.

[Darin Fisher (:fishd, Google)]

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.

[Ian Henderson]

(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.

[Darin Fisher (:fishd, Google)]

(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.