[Webkit-unassigned] [Bug 50331] ASSERT failing restoring scroll position from HistoryItem after reload

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=50331





--- Comment #12 from Mario Sanchez Prada <msanchez at igalia.com>  2011-01-03 07:27:45 PST ---
Oops! forgot to reply this other comment...

(In reply to comment #10)
> (From update of attachment 75704 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75704&action=review
> 
> Thanks for tackling this.
> 
> > WebCore/loader/FrameLoader.cpp:2410
> > +            // If the user had a scroll point, scroll to it, overriding the anchor
> > +            // point if any. Also, make sure there's a valid current item in the
> > +            // history before trying to scroll because it could happen there was
> > +            // none (e.g. loading from a string instead of an URI).
> > +            if (m_frame->page() && history()->currentItem()) {
> >                  if (isBackForwardLoadType(m_loadType) || m_loadType == FrameLoadTypeReload || m_loadType == FrameLoadTypeReloadFromOrigin)
> >                      history()->restoreScrollPositionAndViewState();
> >              }
> 
> It’s pretty clear looking at this code that the currentItem check belongs inside the 
> restoreScrollPositionAndViewState function, not here at the call site.

As I said in the description, I was not sure about that being the right option, although I also considered as a possibility:

" [...] Hence I think the problem could be solved by either removing that ASSERT from HistoryController::restoreScrollPositionAndViewState(), accepting that there are still situations (like this one) where m_currentItem could be 0, or just to try to protect the calls to that function from the callers when needed. So far, I found that the only place that would require such that extra check would be the following one [...]" 


So, I initially proposed adding the extra check in the call site in order not to remove the ASSERT in HistoryController::restoreScrollPositionAndViewState(), which I thought perhaps could be seen as the wrong thing to do. But now you commented on this I'm more for adding the protection inside that function, replacing the ASSERT right there with just a null check to avoid doing anything if currentItem is null.

So, expect a new patch in some minutes...

Thanks for the feedback!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.