Bug 64223 – WebFrame should have a method to determine its visual emptiness

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Ian Henderson]

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.

[Ian Henderson]

Created attachment 100197 [details]
proposed patch

[Ian Henderson]

<rdar://problem/9434637>

[Brent Fulgham]

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.

[Ian Henderson]

(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.

[Alexey Proskuryakov]

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.

[Ian Henderson]

We can get the same result by asking the WebFrame if it's visually non-empty when the load commits.  Updating bug title.

[Ian Henderson]

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.

[Antti Koivisto]

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.

[Ian Henderson]

Comment on attachment 101108 [details]
proposed patch

This is no longer necessary.

[Ian Henderson]

Apparently this is necessary after all.  I'll post a more up-to-date patch.

[Ian Henderson]

Created attachment 101912 [details]
proposed patch

[WebKit Review Bot]

Comment on attachment 101912 [details]
proposed patch

Clearing flags on attachment: 101912

Committed r91727: <http://trac.webkit.org/changeset/91727>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.