Bug 64223 - WebFrame should have a method to determine its visual emptiness
Summary: WebFrame should have a method to determine its visual emptiness
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-08 18:27 PDT by Ian Henderson
Modified: 2012-08-16 13:45 PDT (History)
6 users (show)

See Also:


Attachments
proposed patch (1.99 KB, patch)
2011-07-08 18:35 PDT, Ian Henderson
ian: review-
ian: commit-queue-
Details | Formatted Diff | Diff
proposed patch (3.19 KB, patch)
2011-07-16 18:05 PDT, Ian Henderson
ian: review-
Details | Formatted Diff | Diff
proposed patch (3.13 KB, patch)
2011-07-25 14:36 PDT, Ian Henderson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ian Henderson 2011-07-08 18:27:46 PDT
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.
Comment 1 Ian Henderson 2011-07-08 18:35:28 PDT
Created attachment 100197 [details]
proposed patch
Comment 2 Ian Henderson 2011-07-08 18:36:13 PDT
<rdar://problem/9434637>
Comment 3 Brent Fulgham 2011-07-10 19:21:44 PDT
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.
Comment 4 Ian Henderson 2011-07-11 11:54:19 PDT
(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.
Comment 5 Alexey Proskuryakov 2011-07-11 13:56:23 PDT
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.
Comment 6 Ian Henderson 2011-07-16 17:35:29 PDT
We can get the same result by asking the WebFrame if it's visually non-empty when the load commits.  Updating bug title.
Comment 7 Ian Henderson 2011-07-16 18:05:54 PDT
Created attachment 101108 [details]
proposed patch
Comment 8 mitz 2011-07-17 21:53:52 PDT
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 9 mitz 2011-07-17 21:54:43 PDT
Comment on attachment 101108 [details]
proposed patch

Clearing the cq flag because of my naming suggestion.
Comment 10 Antti Koivisto 2011-07-18 06:35:07 PDT
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 11 Ian Henderson 2011-07-18 12:37:46 PDT
Comment on attachment 101108 [details]
proposed patch

This is no longer necessary.
Comment 12 Ian Henderson 2011-07-25 14:10:03 PDT
Apparently this is necessary after all.  I'll post a more up-to-date patch.
Comment 13 Ian Henderson 2011-07-25 14:36:14 PDT
Created attachment 101912 [details]
proposed patch
Comment 14 WebKit Review Bot 2011-07-25 17:12:42 PDT
Comment on attachment 101912 [details]
proposed patch

Clearing flags on attachment: 101912

Committed r91727: <http://trac.webkit.org/changeset/91727>
Comment 15 WebKit Review Bot 2011-07-25 17:12:47 PDT
All reviewed patches have been landed.  Closing bug.