Bug 83216 – When page is restored from the page cache, never send first layout delegate callback

• Status: REOPENED
• Component: Layout and Rendering

[zalan]

1. load page A 
2, FrameView::layout() -> didFirstVisuallyNonEmptyLayout is called on Page A (correct behavior)
3, load page B 
4, FrameView::layout() -> didFirstVisuallyNonEmptyLayout is called on Page B (correct behavior)

5, do history.back()
6, FrameLoader::open() calls FrameView::clear() and resets B's frameview. Visually non-empty flags are set to default.
7, page A is restored from page cache.
8, FrameView::forceLayout() -> NO didFirstVisuallyNonEmptyLayout is called on Page A (correct behavior)

9, do history.forward()
10, FrameLoader::open() calls FrameView::clear() and resets A's frameview. Visually non-empty flags are set to default.
11, page B is restored from page cache
12, FrameView::forceLayout() -> didFirstVisuallyNonEmptyLayout is called on Page B (mismatched behavior)

13, do history.back() again
...
14, FrameView::forceLayout() -> didFirstVisuallyNonEmptyLayout is called on Page A (mismatched behavior)

[zalan]

Looking at FrameLoader::open() history, it seems resetting the current frameview has been there almost from the beginning (with the change of going from all frame reset to mainframe only).
One idea would be to not call reset() on the frameview at all. At this point the current frameview either ends up in PageCache or gets destroyed shorty after.
In case of ending up in PageCache, timers still needs to be stopped. It is partially done by frame->clearTimers(); (missing postLayout and deferredRepaint).
If we consider the PageCache context as a temporary frozen state, the other flags should stay intact. However it might introduce some regression.

[zalan]

Created attachment 137538 [details]
Patch

[zalan]

As a side note, this line 'clear(true, true, cachedFrame.isMainFrame());' clears the _current_ FrameView depending on whether the _cached_ Frame is mainframe. (cachedFrame->view() != current view)

[Brady Eidson]

Comment on attachment 137538 [details]
Patch

I have not looked at the contents of the patch closely.

I have a problem with the very notion of overloading "first visually non-empty layout" for this when restoration from the page cache is not (necessarily) layout.

What problem are you trying to solve, and can we solve it without making the name "first visually non-empty layout" completely meaningless?

[Brady Eidson]

(I feel like this has come in in another bug very recently but I suck at searching in bugzilla and couldn't find anything)

[zalan]

(In reply to comment #5)
> (I feel like this has come in in another bug very recently but I suck at searching in bugzilla and couldn't find anything)
My bad, should have mentioned it in the summary.
https://bugs.webkit.org/show_bug.cgi?id=74402

[zalan]

(In reply to comment #4)
> (From update of attachment 137538 [details])
> I have not looked at the contents of the patch closely.
> 
> I have a problem with the very notion of overloading "first visually non-empty layout" for this when restoration from the page cache is not (necessarily) layout.
> 
> What problem are you trying to solve, and can we solve it without making the name "first visually non-empty layout" completely meaningless?

Qt WK2 used to reset viewport related properties on the UI process side, when the 'first visually non-empty' layout delegate callback is called. (see bug 74402). Even though the dependency got removed, I still thought it would be worth fixing this inconsistent behavior, should any port be relying on this callback in the future. So for the question, "What problem are you trying to solve" I don't have a bug or use case anymore, only those made-up layout test cases, I added to the patch.

[Brady Eidson]

(In reply to comment #7)
> (In reply to comment #4)
> > (From update of attachment 137538 [details] [details])
> > I have not looked at the contents of the patch closely.
> > 
> > I have a problem with the very notion of overloading "first visually non-empty layout" for this when restoration from the page cache is not (necessarily) layout.
> > 
> > What problem are you trying to solve, and can we solve it without making the name "first visually non-empty layout" completely meaningless?
> 
> Qt WK2 used to reset viewport related properties on the UI process side, when the 'first visually non-empty' layout delegate callback is called. (see bug 74402). Even though the dependency got removed, I still thought it would be worth fixing this inconsistent behavior, should any port be relying on this callback in the future. So for the question, "What problem are you trying to solve" I don't have a bug or use case anymore, only those made-up layout test cases, I added to the patch.

Logically, page cache restoration is not first layout.  That first layout has already occurred.  I've alluded to this in 74402.

It seems to me that overloading this method to mean more than just "didFirstLayout" is not needed to make a world class web browser, as many world class web browsers built on WebKit as-is don't need it.

If there's no known client needing this right now, I suggest we don't need this bug right now.  If this comes up again in the future, we'll look specifically at that client's use case.

[zalan]

> Logically, page cache restoration is not first layout.  That first layout has already occurred.  I've alluded to this in 74402.
Correct and this patch ensures that during page restoration the first layout delegate callback is _not_ dispatched (unless it is the first one, though that's not possible with the current code base) as we concluded in bug 74402.

> It seems to me that overloading this method to mean more than just "didFirstLayout" is not needed to make a world class web browser, as many world class web browsers built on WebKit as-is don't need it.
Agree and that's why I came up with this fix so that no one gets the idea to use it for something it's not meant to be. (and if some ports start using it, they get consistent behavior as opposed to what we've got now on trunk.)

> If there's no known client needing this right now, I suggest we don't need this bug right now.  If this comes up again in the future, we'll look specifically at that client's use case.
Ok. Do you think if 'clear(true, true, cachedFrame.isMainFrame());' at FrameLoader::open(CachedFrameBase& cachedFrame) should still be corrected  
as right now, we clear the _current_ (soon to be discarded/page cached) FrameView depending whether the _cached_ (soon to be restored) frame is main frame. or is it intentional? Thanks.

[Brady Eidson]

(In reply to comment #9)
> > Logically, page cache restoration is not first layout.  That first layout has already occurred.  I've alluded to this in 74402.
> Correct and this patch ensures that during page restoration the first layout delegate callback is _not_ dispatched (unless it is the first one, though that's not possible with the current code base) as we concluded in bug 74402.

Based on the title of the bug led me to believe this was about consistently *sending* it.  I misunderstood.  

Retitling to "when page is restored from the page cache, never send first layout delegate callback"

I'll look at the patch closer now.