RESOLVED FIXED 193741
DidFirstVisuallyNonEmptyLayout milestone should always fire at some point.
https://bugs.webkit.org/show_bug.cgi?id=193741
Summary DidFirstVisuallyNonEmptyLayout milestone should always fire at some point.
zalan
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, zalan
no flags
Patch (6.78 KB, patch)
2019-01-23 18:44 PST, zalan
no flags
Patch (6.76 KB, patch)
2019-01-24 08:47 PST, zalan
no flags
Patch (3.70 KB, patch)
2019-01-24 10:17 PST, zalan
no flags
Patch (1.93 KB, patch)
2019-01-24 10:55 PST, zalan
no flags
Patch (9.41 KB, patch)
2019-01-24 13:57 PST, zalan
no flags
Patch (9.40 KB, patch)
2019-01-24 14:31 PST, zalan
no flags
zalan
Comment 1 2019-01-23 15:21:29 PST
zalan
Comment 2 2019-01-23 15:40:59 PST
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
zalan
Comment 4 2019-01-23 18:44:05 PST
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
zalan
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.
zalan
Comment 8 2019-01-24 08:47:02 PST
zalan
Comment 9 2019-01-24 10:17:10 PST
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.
zalan
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.
zalan
Comment 12 2019-01-24 10:51:47 PST
>I'm not sure this does everything that needs to be done. It does.
zalan
Comment 13 2019-01-24 10:55:56 PST
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
zalan
Comment 16 2019-01-24 13:57:29 PST
zalan
Comment 17 2019-01-24 14:31:00 PST
WebKit Commit Bot
Comment 18 2019-01-24 14:58:15 PST
Comment on attachment 360039 [details] Patch Clearing flags on attachment: 360039 Committed r240450: <https://trac.webkit.org/changeset/240450>
WebKit Commit Bot
Comment 19 2019-01-24 14:58:16 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.