Bug 42298 - Regression (r63100): m_loadType being used to reset scroll position after being reset
Summary: Regression (r63100): m_loadType being used to reset scroll position after bei...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-14 15:46 PDT by Nate Chapin
Modified: 2010-07-16 01:29 PDT (History)
6 users (show)

See Also:


Attachments
Fix + uncomment the gtk test assertion that broke (3.95 KB, patch)
2010-07-14 15:52 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2010-07-14 15:46:22 PDT
In http://trac.webkit.org/changeset/63100, I changed FrameLoader::handledOnloadEvents() to reset m_loadType to FrameLoadTypeStandard (to ensure that loads after the load event would not propagate the caching behavior of the original request).  It turns out this solution won't work, since there are some other behaviors that depend on m_loadType after the load event.  Notably, in didFirstLayout(), we decide whether or not to call history()->restoreScrollPositionAndViewState() based on m_loadType, so scroll position restoring for back/forward navigations broke (which caused gtk test to fail).

This change should be fixable by reverting my changes to handledOnloadEvents() and being a little more careful in addExtraFieldsToRequest().
Comment 1 Nate Chapin 2010-07-14 15:52:29 PDT
Created attachment 61577 [details]
Fix + uncomment the gtk test assertion that broke

I don't have a gtk build, but msanchez was kind enough to patch this in and confirm the assertion now passes.
Comment 2 Mario Sanchez Prada 2010-07-15 23:51:27 PDT
(In reply to comment #1)
> Created an attachment (id=61577) [details]
> Fix + uncomment the gtk test assertion that broke
> 
> I don't have a gtk build, but msanchez was kind enough to patch this in and 
> confirm the assertion now passes.

Yes, I confirm both that the assertion passes and that there were no regressions found while re-running the layout tests with 'run-webkit-tests --gtk'

Thanks Nate for fixing this issue
Comment 3 WebKit Commit Bot 2010-07-16 00:46:41 PDT
Comment on attachment 61577 [details]
Fix + uncomment the gtk test assertion that broke

Clearing flags on attachment: 61577

Committed r63528: <http://trac.webkit.org/changeset/63528>
Comment 4 WebKit Commit Bot 2010-07-16 00:46:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 WebKit Review Bot 2010-07-16 01:10:43 PDT
http://trac.webkit.org/changeset/63528 might have broken Qt Linux Release
Comment 6 Mario Sanchez Prada 2010-07-16 01:29:03 PDT
(In reply to comment #5)
> http://trac.webkit.org/changeset/63528 might have broken Qt Linux Release

Output in the build bot [1] says this for the failing test(fast/dom/Range/range-compareNode.html):

CONSOLE MESSAGE: line 198: TypeError: Result of expression 'layoutTestController.dumpAsText' [undefined] is not a function.

I'm not 100% sure, of course, but I don't see how this patch could get this test failing this way. Hence, I've got the feeling is something else.

Any guessing/idea?


[1] http://build.webkit.org/results/Qt%20Linux%20Release/r63529%20(15543)/fast/dom/Range/range-compareNode-pretty-diff.html