Bug 193741 - DidFirstVisuallyNonEmptyLayout milestone should always fire at some point.
Summary: DidFirstVisuallyNonEmptyLayout milestone should always fire at some point.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-23 15:21 PST by zalan
Modified: 2019-01-24 14:58 PST (History)
6 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2019-01-23 15:21:13 PST
It's important that this milestone gets fired.
Comment 1 zalan 2019-01-23 15:21:29 PST
<rdar://problem/47135030>
Comment 2 zalan 2019-01-23 15:40:59 PST
Created attachment 359960 [details]
Patch
Comment 3 Simon Fraser (smfr) 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
Comment 4 zalan 2019-01-23 18:44:05 PST
Created attachment 359983 [details]
Patch
Comment 5 Simon Fraser (smfr) 2019-01-23 18:46:37 PST
Comment on attachment 359983 [details]
Patch

Can we layout-test this?
Comment 6 Simon Fraser (smfr) 2019-01-23 18:46:46 PST
Or API test
Comment 7 zalan 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.
Comment 8 zalan 2019-01-24 08:47:02 PST
Created attachment 360014 [details]
Patch
Comment 9 zalan 2019-01-24 10:17:10 PST
Created attachment 360020 [details]
Patch
Comment 10 Antti Koivisto 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.
Comment 11 zalan 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.
Comment 12 zalan 2019-01-24 10:51:47 PST
>I'm not sure this does everything that needs to be done.
It does.
Comment 13 zalan 2019-01-24 10:55:56 PST
Created attachment 360022 [details]
Patch
Comment 14 Simon Fraser (smfr) 2019-01-24 11:08:37 PST
Comment on attachment 360022 [details]
Patch

Please rename m_firstVisuallyNonEmptyLayoutCallbackPending. It's not a callback.
Comment 15 Antti Koivisto 2019-01-24 11:11:32 PST
Comment on attachment 360022 [details]
Patch

r=me
Comment 16 zalan 2019-01-24 13:57:29 PST
Created attachment 360034 [details]
Patch
Comment 17 zalan 2019-01-24 14:31:00 PST
Created attachment 360039 [details]
Patch
Comment 18 WebKit Commit Bot 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>
Comment 19 WebKit Commit Bot 2019-01-24 14:58:16 PST
All reviewed patches have been landed.  Closing bug.