Bug 69298 – Add an option to suppress rendering until the document's load event fires.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Andy Estes]

Add an option to suppress rendering until the document's load event fires.

[Andy Estes]

<rdar://problem/10196544>

[Andy Estes]

Created attachment 109565 [details]
Patch

[Dave Hyatt]

Comment on attachment 109565 [details]
Patch

We already have some paint suppression code for when styles haven't loaded yet. I wonder if you could latch on to that spot instead of adding a new spot in FrameView?

Check out the top of paintLayer.

[Simon Fraser (smfr)]

Comment on attachment 109565 [details]
Patch

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

r- pending discussion of how to slice the preferences.

> Source/WebCore/page/Settings.h:469
> +        void setSuppressRenderingWhileInitiallyLoading(bool flag) { m_suppressRenderingWhileInitiallyLoading = flag; }
> +        bool suppressRenderingWhileInitiallyLoading() const { return m_suppressRenderingWhileInitiallyLoading; }

Is this a good name for the setting? I wonder if we might want to extend the influence of this setting layer to do things like avoiding painting partially-loaded images. There's a risk of setting-creep here.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:251
> +        && m_renderView->document()->settings()->suppressRenderingWhileInitiallyLoading()
> +        && !m_renderView->document()->loadEventFinished())

Would be nice to wrap these two tests in their own method (on document?), like Document::visibleUpdatesAllowed() or something.

[Andy Estes]

(In reply to comment #3)
> (From update of attachment 109565 [details])
> We already have some paint suppression code for when styles haven't loaded yet. I wonder if you could latch on to that spot instead of adding a new spot in FrameView?
> 
> Check out the top of paintLayer.

I modeled this approach off of the FOUC suppression, and initially tried what you suggested, but it caused flashes during navigation since the FOUC check allows the root layer to paint.

[Andy Estes]

(In reply to comment #4)
> (From update of attachment 109565 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109565&action=review
> 
> r- pending discussion of how to slice the preferences.
> 
> > Source/WebCore/page/Settings.h:469
> > +        void setSuppressRenderingWhileInitiallyLoading(bool flag) { m_suppressRenderingWhileInitiallyLoading = flag; }
> > +        bool suppressRenderingWhileInitiallyLoading() const { return m_suppressRenderingWhileInitiallyLoading; }
> 
> Is this a good name for the setting? I wonder if we might want to extend the influence of this setting layer to do things like avoiding painting partially-loaded images. There's a risk of setting-creep here.

My plan was to make that a separate setting for maximum flexibility, but it sounds like you want to avoid that. It's possible that no client would want this setting without having non-incremental images as well, though.

> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:251
> > +        && m_renderView->document()->settings()->suppressRenderingWhileInitiallyLoading()
> > +        && !m_renderView->document()->loadEventFinished())
> 
> Would be nice to wrap these two tests in their own method (on document?), like Document::visibleUpdatesAllowed() or something.

That's a nice idea.

[Andy Estes]

Created attachment 109865 [details]
Patch

[Andy Estes]

(In reply to comment #7)
> Created an attachment (id=109865) [details]
> Patch

A new patch with a new (and hopefully improved) preference name. I also moved the painting suppression check out of FrameView and integrated it into the FOUC check in RenderLayer, per Hyatt's suggestion.

[Simon Fraser (smfr)]

Comment on attachment 109865 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        No tests currently possible. Testing this would require the ability for
> +        DRT to dump state while resources are loading, which it doesn't
> +        currently do.

Can't we use an http test that trickles in the main resource, and a pixel test for this?

> Source/WebCore/rendering/RenderLayer.cpp:2629
> +    ASSERT_ARG(layer, layer);

This isn't much use. We'll crash on the next line if layer is null.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:250
> +    // Compositing layers will be updated in Document::implicitClose() if suppressed here.
> +    if (!m_renderView->document()->visualUpdatesAllowed())
> +        return;

This is a different throttling mechanism to that used elsewhere. See LayerTreeHostCAMac::setLayerFlushSchedulingEnabled(). Maybe we should move that logic down into WebCore. That can be done later though.

[Andy Estes]

Thanks!

(In reply to comment #9)
> (From update of attachment 109865 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109865&action=review
> 
> > Source/WebCore/ChangeLog:16
> > +        No tests currently possible. Testing this would require the ability for
> > +        DRT to dump state while resources are loading, which it doesn't
> > +        currently do.
> 
> Can't we use an http test that trickles in the main resource, and a pixel test for this?

I tried writing a test like that. The problem is that DumpRenderTree won't dump while a main frame load is in progress, even if you call notifyDone(). I intend to file a bug about making this feature testable.

> 
> > Source/WebCore/rendering/RenderLayer.cpp:2629
> > +    ASSERT_ARG(layer, layer);
> 
> This isn't much use. We'll crash on the next line if layer is null.

Indeed. I'll take it out.

> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:250
> > +    // Compositing layers will be updated in Document::implicitClose() if suppressed here.
> > +    if (!m_renderView->document()->visualUpdatesAllowed())
> > +        return;
> 
> This is a different throttling mechanism to that used elsewhere. See LayerTreeHostCAMac::setLayerFlushSchedulingEnabled(). Maybe we should move that logic down into WebCore. That can be done later though.

Okay, I'll take a look at this.

[Andy Estes]

Committed r96786: <http://trac.webkit.org/changeset/96786>