Bug 115330 – Need a LayoutMilestone to fire when we have done our first paint after suppressing incremental layout

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Beth Dakin]

Clients that use the suppressesIncrementalRendering API have requested a LayoutMilestone indicating when rendering is not longer being suppressed. Patch forthcoming.

<rdar://problem/12722365>

[Beth Dakin]

Created attachment 199967 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 199967 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=199967&action=review

> Source/WebCore/dom/Document.cpp:1302
>      if (renderer())
>          renderer()->repaint();

This is so bogus. It doesn't cause compositing layers to paint.

> Source/WebCore/dom/Document.cpp:1305
> +    if (page()->requestedLayoutMilestones() & DidFirstPaintAfterSuppressedIncrementalRendering)
> +        frame()->loader()->didLayout(DidFirstPaintAfterSuppressedIncrementalRendering);

There's some irony here: didLayout(DidFirstPaint... (layout vs. paint).

And I think this is wrong. I think you need to wait for the first compositing layer flush after this, especially since we have logic to suppress compositing layer flushes.

> Source/WebCore/page/LayoutMilestones.h:36
> +    DidFirstPaintAfterSuppressedIncrementalRendering = 1 << 4

Many of our so-called layout milestones are more about painting :-\

[Simon Fraser (smfr)]

(In reply to comment #2)
> (From update of attachment 199967 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=199967&action=review
> 
> > Source/WebCore/dom/Document.cpp:1302
> >      if (renderer())
> >          renderer()->repaint();
> 
> This is so bogus. It doesn't cause compositing layers to paint.
> 
> > Source/WebCore/dom/Document.cpp:1305
> > +    if (page()->requestedLayoutMilestones() & DidFirstPaintAfterSuppressedIncrementalRendering)
> > +        frame()->loader()->didLayout(DidFirstPaintAfterSuppressedIncrementalRendering);
> 
> There's some irony here: didLayout(DidFirstPaint... (layout vs. paint).
> 
> And I think this is wrong. I think you need to wait for the first compositing layer flush after this, especially since we have logic to suppress compositing layer flushes.

... if we're in compositing mode.

[Sam Weinig]

I vote for renderingMilestones!

[Beth Dakin]

Created attachment 199971 [details]
Patch

[Andy Estes]

(In reply to comment #2)
> (From update of attachment 199967 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=199967&action=review
> 
> > Source/WebCore/dom/Document.cpp:1302
> >      if (renderer())
> >          renderer()->repaint();
> 
> This is so bogus. It doesn't cause compositing layers to paint.

This has been here since App Mode was implemented, so it's probably my fault. What's the right thing to do here? At the time I did this my understanding was that the combination of renderer()->repaint() and FrameView::updateCompositingLayersAfterLayout() did the right thing.

[Andy Estes]

Is RenderView::repaintViewAndCompositedLayers() what we want?

[Simon Fraser (smfr)]

(In reply to comment #7)
> Is RenderView::repaintViewAndCompositedLayers() what we want?

Yes

[Beth Dakin]

Created attachment 200033 [details]
Patch

Oh yeah, sorry I forgot to address that. Thanks for brining it up again, Andy! Here's a patch that uses repaintViewAndCompositedLayers() instead.

[Simon Fraser (smfr)]

Comment on attachment 200033 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=200033&action=review

> Source/WebCore/rendering/RenderLayerCompositor.cpp:385
> +        if (m_headerLayerAwaitingFirstFlush) {
> +            m_headerLayerAwaitingFirstFlush = false;
> +            if (page->requestedLayoutMilestones() & DidFirstFlushForHeaderLayer)
> +                milestonesAchieved |= DidFirstFlushForHeaderLayer;
> +        }
> +
> +        if (frameView && frameView->isAwaitingFirstPaintAfterSuppressedRendering()) {

Why is m_headerLayerAwaitingFirstFlush here, but isAwaitingFirstPaintAfterSuppressedRendering on FrameView? Because the first is strictly layer-based?

Maybe we should add FrameView::m_milestonesPendingPaint or something, rather than a new flag for each one.

> Source/WebKit2/ChangeLog:21
> +        This null-check is necessary now since this code ends up running at 
> +        WebFrame::init() time while we're setting up the Document. 
> +        setVisualUpdatesAllowed(true) has always been called as a part of that process, 
> +        and now the updateLayout(), ends up calling this code too, but we don't actually 
> +        have a mainFrame yet since it's still being created.

Does this mean we're doing an extra layout from before? Why isn't true the default value for VisualUpdatesAllowed?

[Beth Dakin]

(In reply to comment #10)
> (From update of attachment 200033 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200033&action=review
> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:385
> > +        if (m_headerLayerAwaitingFirstFlush) {
> > +            m_headerLayerAwaitingFirstFlush = false;
> > +            if (page->requestedLayoutMilestones() & DidFirstFlushForHeaderLayer)
> > +                milestonesAchieved |= DidFirstFlushForHeaderLayer;
> > +        }
> > +
> > +        if (frameView && frameView->isAwaitingFirstPaintAfterSuppressedRendering()) {
> 
> Why is m_headerLayerAwaitingFirstFlush here, but isAwaitingFirstPaintAfterSuppressedRendering on FrameView? Because the first is strictly layer-based?
> 

That was my thinking, yes.

> Maybe we should add FrameView::m_milestonesPendingPaint or something, rather than a new flag for each one.
> 

That's a good idea! I will do that.

> > Source/WebKit2/ChangeLog:21
> > +        This null-check is necessary now since this code ends up running at 
> > +        WebFrame::init() time while we're setting up the Document. 
> > +        setVisualUpdatesAllowed(true) has always been called as a part of that process, 
> > +        and now the updateLayout(), ends up calling this code too, but we don't actually 
> > +        have a mainFrame yet since it's still being created.
> 
> Does this mean we're doing an extra layout from before? Why isn't true the default value for VisualUpdatesAllowed?

Simon and I looked at this in person. It does default to true, but ends up getting set to false and then true again because of code propagated by FrameLoader::init(). It's kind of weird, but it seems okay.

[Beth Dakin]

Created attachment 200043 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 200043 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=200043&action=review

> Source/WebCore/page/FrameView.cpp:3567
> +    if (Page* page = m_frame->page()) {
> +        if (m_milestonesPendingPaint & DidFirstPaintAfterSuppressedIncrementalRendering) {
> +            removePaintPendingMilestones(DidFirstPaintAfterSuppressedIncrementalRendering);
> +            if (page->requestedLayoutMilestones() & DidFirstPaintAfterSuppressedIncrementalRendering)
> +                frame()->loader()->didLayout(DidFirstPaintAfterSuppressedIncrementalRendering);
> +        }
> +    }

It would be nice to wrap up the "remove and fire" code here and in RenderLayerCompositor into one place.

[Beth Dakin]

(In reply to comment #13)
> (From update of attachment 200043 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200043&action=review
> 
> > Source/WebCore/page/FrameView.cpp:3567
> > +    if (Page* page = m_frame->page()) {
> > +        if (m_milestonesPendingPaint & DidFirstPaintAfterSuppressedIncrementalRendering) {
> > +            removePaintPendingMilestones(DidFirstPaintAfterSuppressedIncrementalRendering);
> > +            if (page->requestedLayoutMilestones() & DidFirstPaintAfterSuppressedIncrementalRendering)
> > +                frame()->loader()->didLayout(DidFirstPaintAfterSuppressedIncrementalRendering);
> > +        }
> > +    }
> 
> It would be nice to wrap up the "remove and fire" code here and in RenderLayerCompositor into one place.

I consolidated this in a new function called FrameView::firePaintRelatedMilestones()

http://trac.webkit.org/changeset/149317