latest from 20050629/trunk but not 20050628/trunk Scroll position can not be retained, when moving forward and backward between the nested pages. steps are: 1. load a large page and scroll down 2. click a link and go to new page 3. repeat from 1 to 2 of the step(about 5 times) 4. click Back button until return to the first page 5. click Forward button until return to the last page 6. repeat from 4 to 5 of the step actual result: the first page or last page is reloaded and scroll positions are reset. expected result: scroll position is retained in spite of the number of nests.

After the patch for bug 292965 landed, the number of cached pages stored depends on the amount of memory available. If you want to have more pages stored, then change browser.sessionhistory.max_total_viewers to the number of pages you want, instead of leaving it at -1 which causes it to be based on the available memory.

now wait a second, whether the scroll position is restored is unrelated to bfcache. reopening.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20051227 Firefox/1.6a1 I tried reproducing this in a couple of ways: first I tried it with bfcache off, and couldn't reproduce it, then I tried it with bfcache based on the amount of memory available (max_total_viewers at -1) -- same thing. Then I tried with max_total_viewers at 5, and still couldn't reproduce it even if there were > 5 pages in the history. So, this works for me. Hideo, could you provide more detailed steps to reproduce? Such as what page you start out and, and end up at, etc.?

some files(link.html, link2.html, ...) in testcase holder I can reproduce it first loading link.html on testcase. I can not reproduce it, if browser.sessionhistory.max_total_viewers is changed to zero, however, I can reproduce it, if it is -1, 1 or 10.

Before swaping the viewer, the layout state is captured since the scroll position will be restored after exceeding cached viewers of session history.

Comment on attachment 207255 [details] [diff] [review] patch I don't really know this code. Try bryner. That said, does this also fix bug 321778 by any chance?

Comment on attachment 207255 [details] [diff] [review] patch This patch is effective only about restoration of a scroll position.

This patch is effective about restoration of a scroll position and loss of text on text control area.

I tested setting the browser.sessionhistory.max_total_viewers preference to default(-1), scroll down and type text in the text area, click a link, and click Back button.

So why does that patch help? I recommend putting the answer in comments in the code...

Comment on attachment 207341 [details] [diff] [review] patch I think _probably_ what needs to happen is just to uncomment the "XXX PersistLayoutHistoryState();" up towards the beginning of RestoreFromHistory. That's the equivalent ordering for what happens in Embed(). I'm confused by the EvictWindowContentViewer change, what problem is this fixing?

I think this is all it needs... not sure why I didn't catch that XXX comment before checking this in originally.

bryner, please check this patch again, because your patch is failed for my testing. The cause of loss of added text on text control area is that added text is not synchronized when closing and destruction of the DocumentViewer frame issue from exceeding the number of cached viewers of session history. I think that viewer->Destroy() is called after ownerEntry->SyncPresentationState is called.

Comment on attachment 207408 [details] [diff] [review] patch bryner, I am sorry for my excessive request. My testing for your patch is successful as to restored scroll position.

checked in on trunk

Comment on attachment 207400 [details] [diff] [review] patch We should get this in for FF2. It probably doesn't fit the criteria for 1.5.0.x but I'll go ahead and nominate it in case branch drivers want it.

Comment on attachment 207400 [details] [diff] [review] patch not approved for 1.8.0 branch

Jonathan, I think the key for this bug was that the first page loaded got evicted from bfcache after being initially placed into it. So the testcase needs to assert that (at the very least) the last back() call executed leads to a load that's not coming from bfcache. That way if we change the cache limit to more than 5 the test will fail and we'll know we need to bump the limit. To make that easier, you should probably make the 5 a const declared up top in the script. And for a spelling nit, s/it's/its/. ;)

Oh, and it might be good to also assert that each of the pages does go into bfcache as we navigate initially (so assert that the pagehides all have persisted == true).

Oh, and also nix the random whitespace change in the makefile?

Thanks for the review Boris. I'm attaching a new version which addresses all your comments. Instead of making a constant for 5 (the number of pages we cycle through in the test), I made one for MAX_BFCACHE_PAGES = 3 (the max number of pages stored in the bfcache for the current session history). Test passes on trunk.

Comment on attachment 387729 [details] [diff] [review] mochitest test case Yeah, that looks much better. I just realized that this test would pass if the scrolling just did nothing (e.g. if the window were too big for some reason). Can you add a check that the scroll position after scrolling is nonzero and before scrolling is 0? With that, looks great.

Thanks for the suggestion Boris; I've added the pre- and post-scroll checks as you recommended.

Pushed test as http://hg.mozilla.org/mozilla-central/rev/d732ead520aa