WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(6.78 KB, patch)
2019-01-23 18:44 PST
,
zalan
no flags
Details
Formatted Diff
Diff
Patch
(6.76 KB, patch)
2019-01-24 08:47 PST
,
zalan
no flags
Details
Formatted Diff
Diff
Patch
(3.70 KB, patch)
2019-01-24 10:17 PST
,
zalan
no flags
Details
Formatted Diff
Diff
Patch
(1.93 KB, patch)
2019-01-24 10:55 PST
,
zalan
no flags
Details
Formatted Diff
Diff
Patch
(9.41 KB, patch)
2019-01-24 13:57 PST
,
zalan
no flags
Details
Formatted Diff
Diff
Patch
(9.40 KB, patch)
2019-01-24 14:31 PST
,
zalan
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
zalan
Comment 1
2019-01-23 15:21:29 PST
<
rdar://problem/47135030
>
zalan
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
zalan
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
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
Created
attachment 360014
[details]
Patch
zalan
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.
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
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
zalan
Comment 16
2019-01-24 13:57:29 PST
Created
attachment 360034
[details]
Patch
zalan
Comment 17
2019-01-24 14:31:00 PST
Created
attachment 360039
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug