Bug 193741

Summary:

DidFirstVisuallyNonEmptyLayout milestone should always fire at some point.

Product:

WebKit

Reporter:

alan <zalan>

Component:

Layout and Rendering

Assignee:

alan <zalan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, commit-queue, ggaren, koivisto, simon.fraser, zalan

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

alan

Reported 2019-01-23 15:21:13 PST

It's important that this milestone gets fired.

Attachments
Patch (6.42 KB, patch) 2019-01-23 15:40 PST, alan	no flags	Details Formatted Diff Diff
Patch (6.78 KB, patch) 2019-01-23 18:44 PST, alan	no flags	Details Formatted Diff Diff
Patch (6.76 KB, patch) 2019-01-24 08:47 PST, alan	no flags	Details Formatted Diff Diff
Patch (3.70 KB, patch) 2019-01-24 10:17 PST, alan	no flags	Details Formatted Diff Diff
Patch (1.93 KB, patch) 2019-01-24 10:55 PST, alan	no flags	Details Formatted Diff Diff
Patch (9.41 KB, patch) 2019-01-24 13:57 PST, alan	no flags	Details Formatted Diff Diff
Patch (9.40 KB, patch) 2019-01-24 14:31 PST, alan	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

alan

Comment 1 2019-01-23 15:21:29 PST

<rdar://problem/47135030>

alan

Comment 2 2019-01-23 15:40:59 PST

Created attachment 359960 [details] Patch

Simon Fraser (smfr)

Comment 3 2019-01-23 15:58:43 PST

Comment on attachment 359960 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359960&action=review > Source/WebCore/page/FrameView.cpp:4550 > +void FrameView::firstVisuallyNonEmptyLayoutMilestoneWatchdogFired() Firing your watchdog means that you weren't happy with how it was doing its job, so you let it go. This is a watchdog timer getting fired. > Source/WebCore/page/FrameView.cpp:4559 > +void FrameView::fireFirstVisuallyNonEmptyLayoutMilestoneIfRequested() This function seems to do more than fire the milestone; it clears some state, and triggers the DidFirstMeaningfulPaint milestone. > Source/WebCore/page/FrameView.h:836 > + Timer m_firstVisuallyNonEmptyMilestoneWatchdog; m_firstVisuallyNonEmptyMilestoneWatchdogTimer

alan

Comment 4 2019-01-23 18:44:05 PST

Created attachment 359983 [details] Patch

Simon Fraser (smfr)

Comment 5 2019-01-23 18:46:37 PST

Comment on attachment 359983 [details] Patch Can we layout-test this?

Simon Fraser (smfr)

Comment 6 2019-01-23 18:46:46 PST

Or API test

alan

Comment 7 2019-01-23 18:56:23 PST

(In reply to Simon Fraser (smfr) from comment #6) > Or API test We've got some API tests in this area. Need to think about how to simulate this case though.

alan

Comment 8 2019-01-24 08:47:02 PST

Created attachment 360014 [details] Patch

alan

Comment 9 2019-01-24 10:17:10 PST

Created attachment 360020 [details] Patch

Antti Koivisto

Comment 10 2019-01-24 10:34:22 PST

Comment on attachment 360020 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360020&action=review As someone mentioned, a test would be cool. > Source/WebCore/page/FrameView.cpp:4557 > +void FrameView::handleFirstVisuallyNonEmptyLayoutMilestone() > +{ > + ASSERT(!m_isVisuallyNonEmpty); > + ASSERT(m_firstVisuallyNonEmptyLayoutCallbackPending); > + m_isVisuallyNonEmpty = true; > + m_firstVisuallyNonEmptyLayoutCallbackPending = false; > + addPaintPendingMilestones(DidFirstMeaningfulPaint); > + > + if (!frame().isMainFrame()) > + return; > + if (!frame().page() || !frame().page()->requestedLayoutMilestones().contains(DidFirstVisuallyNonEmptyLayout)) > + return; > + frame().loader().didReachLayoutMilestone(DidFirstVisuallyNonEmptyLayout); > +} Can't we just call fireLayoutRelatedMilestonesIfNeeded() instead of this? I'm not sure why another function doing the firing is needed. I'm not sure this does everything that needs to be done.

alan

Comment 11 2019-01-24 10:47:58 PST

(In reply to Antti Koivisto from comment #10) > Comment on attachment 360020 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=360020&action=review > > As someone mentioned, a test would be cool. > > > Source/WebCore/page/FrameView.cpp:4557 > > +void FrameView::handleFirstVisuallyNonEmptyLayoutMilestone() > > +{ > > + ASSERT(!m_isVisuallyNonEmpty); > > + ASSERT(m_firstVisuallyNonEmptyLayoutCallbackPending); > > + m_isVisuallyNonEmpty = true; > > + m_firstVisuallyNonEmptyLayoutCallbackPending = false; > > + addPaintPendingMilestones(DidFirstMeaningfulPaint); > > + > > + if (!frame().isMainFrame()) > > + return; > > + if (!frame().page() || !frame().page()->requestedLayoutMilestones().contains(DidFirstVisuallyNonEmptyLayout)) > > + return; > > + frame().loader().didReachLayoutMilestone(DidFirstVisuallyNonEmptyLayout); > > +} > > Can't we just call fireLayoutRelatedMilestonesIfNeeded() instead of this? > I'm not sure why another function doing the firing is needed. I'm not sure > this does everything that needs to be done. Since this is not an actual layout, I thought it would be incorrect to call fireLayoutRelatedMilestonesIfNeeded() -but obviously it fixes the bug as well. Let's go back to that.

alan

Comment 12 2019-01-24 10:51:47 PST

>I'm not sure this does everything that needs to be done. It does.

alan

Comment 13 2019-01-24 10:55:56 PST

Created attachment 360022 [details] Patch

Simon Fraser (smfr)

Comment 14 2019-01-24 11:08:37 PST

Comment on attachment 360022 [details] Patch Please rename m_firstVisuallyNonEmptyLayoutCallbackPending. It's not a callback.

Antti Koivisto

Comment 15 2019-01-24 11:11:32 PST

Comment on attachment 360022 [details] Patch r=me