[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 #5 from Mario Sanchez Prada <msanchez at igalia.com>  2010-12-06 04:45:45 PST ---
Thanks for the review Eric, but I have some doubts that are currently blocking me from providing a better patch. See below...

(In reply to comment #4)
> (From update of attachment 75287 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75287&action=review
> 
> This needs a test.

Not sure whether you mean a Layout Test (this is my guessing, though) or an unit test.

The problem with writing a Layout Test for this is that, as I said, the bug happens when  using webkit_web_view_load_string() in an unit test for the GTK port, which basically loads an HTML content from a (const gchar*) instead of loading the content from an URI. And that is (not having an URI) precisely the problem causing this bug, since history->currentItem() will be 0 at the point we're trying to restore the scroll position, right after finishing the reload process.

Hence, I don't know how to test this with a layout test, since (1) a layout test has always an URI (the path to the test), so it wouldn't be testing the problem I try to fix, and (2) since the problem was detected from a function of the GTK port.


In case you're talking about an unit test (or perhaps you were not, but my paragraphs above has finally convinced you :-)), I do not see any problem on writing an small unit test in the GTK port for that, which would be a ultra-reduced version of testWebkitAtkDocumentReloadEvents() (provided with patch for bug 25831), in case that was fine.

So, as you can see, I need some extra input here to continue working on this :-)

> > WebCore/loader/FrameLoader.cpp:2408
> > +            // 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()) {
> 
> Seems rather overzealous line wrapping. :)

I don't think I deserve credit for this: emacs was the author of such a beautiful piece of art!

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