Summary: | DidFirstVisuallyNonEmptyLayout milestone should always fire at some point. | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | zalan <zalan> | ||||||||||||||||
Component: | Layout and Rendering | Assignee: | zalan <zalan> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | bfulgham, commit-queue, ggaren, koivisto, simon.fraser, zalan | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
zalan
2019-01-23 15:21:13 PST
Created attachment 359960 [details]
Patch
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 Created attachment 359983 [details]
Patch
Comment on attachment 359983 [details]
Patch
Can we layout-test this?
Or API test (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. Created attachment 360014 [details]
Patch
Created attachment 360020 [details]
Patch
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. (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. >I'm not sure this does everything that needs to be done.
It does.
Created attachment 360022 [details]
Patch
Comment on attachment 360022 [details]
Patch
Please rename m_firstVisuallyNonEmptyLayoutCallbackPending. It's not a callback.
Comment on attachment 360022 [details]
Patch
r=me
Created attachment 360034 [details]
Patch
Created attachment 360039 [details]
Patch
Comment on attachment 360039 [details] Patch Clearing flags on attachment: 360039 Committed r240450: <https://trac.webkit.org/changeset/240450> All reviewed patches have been landed. Closing bug. |