Steps to reproduce: 1. Fire up dtrace, set probes for didFirstVisuallyNonEmptyLayout and dispatchDidCommitLoad. 2. Load http://news.ycombinator.com or another site that is cacheable in the page cache. 3. Click on a link. 4. Go back. Notice that the load has committed, but didFirstVisuallyNonEmptyLayout never happens.
Created attachment 100197 [details] proposed patch
<rdar://problem/9434637>
Comment on attachment 100197 [details] proposed patch Do any of the other layout state flags need to be reset as well? E.g., m_hasPendingPostLayoutTasks seems like it might fall into the same bad state.
(In reply to comment #3) > (From update of attachment 100197 [details]) > Do any of the other layout state flags need to be reset as well? E.g., m_hasPendingPostLayoutTasks seems like it might fall into the same bad state. I'm not sure, maybe someone more familiar with WebCore layout can answer that. The issue here is that m_firstVisuallyNonEmptyLayoutCallbackPending is only set once, in FrameView's constructor. I guess the assumption was that page loads correspond to new FrameViews. This assumption is broken when a page is restored from the b/f cache.
This fix looks rather scary to me. There is a ton of callbacks that are not made when a page comes back from b/f cache, and that even includes most basic loading callbacks. Sending didFirstVisuallyNonEmptyLayout alone could be confusing to clients.
We can get the same result by asking the WebFrame if it's visually non-empty when the load commits. Updating bug title.
Created attachment 101108 [details] proposed patch
Comment on attachment 101108 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=101108&action=review > Source/WebKit/mac/WebView/WebFramePrivate.h:77 > - (BOOL)_firstLayoutDone; > +- (BOOL)_didFirstVisuallyNonEmptyLayout; I’d call this _firstVisuallyNonEmptyLayoutDone. I think a message named did… often implies that the sender did something and is notifying the receiver of that. Perhaps a better name for both would have been hasDone….
Comment on attachment 101108 [details] proposed patch Clearing the cq flag because of my naming suggestion.
Does this issue actually exist anymore? I believe we dispatch didFirstVisuallyNonEmptyLayout for cached pages too after http://trac.webkit.org/changeset/90900 (and the followup in 91027). GDB seems to agree.
Comment on attachment 101108 [details] proposed patch This is no longer necessary.
Apparently this is necessary after all. I'll post a more up-to-date patch.
Created attachment 101912 [details] proposed patch
Comment on attachment 101912 [details] proposed patch Clearing flags on attachment: 101912 Committed r91727: <http://trac.webkit.org/changeset/91727>
All reviewed patches have been landed. Closing bug.