Bug 69298 - Add an option to suppress rendering until the document's load event fires.
Summary: Add an option to suppress rendering until the document's load event fires.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy Estes
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-10-03 15:17 PDT by Andy Estes
Modified: 2011-10-05 21:17 PDT (History)
4 users (show)

See Also:


Attachments
Patch (15.65 KB, patch)
2011-10-03 18:10 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (17.41 KB, patch)
2011-10-05 14:43 PDT, Andy Estes
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2011-10-03 15:17:52 PDT
Add an option to suppress rendering until the document's load event fires.
Comment 1 Andy Estes 2011-10-03 15:25:55 PDT
<rdar://problem/10196544>
Comment 2 Andy Estes 2011-10-03 18:10:19 PDT
Created attachment 109565 [details]
Patch
Comment 3 Dave Hyatt 2011-10-04 09:48:55 PDT
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.
Comment 4 Simon Fraser (smfr) 2011-10-04 09:49:11 PDT
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.
Comment 5 Andy Estes 2011-10-04 10:00:59 PDT
(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.
Comment 6 Andy Estes 2011-10-04 10:04:52 PDT
(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.
Comment 7 Andy Estes 2011-10-05 14:43:16 PDT
Created attachment 109865 [details]
Patch
Comment 8 Andy Estes 2011-10-05 14:44:53 PDT
(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.
Comment 9 Simon Fraser (smfr) 2011-10-05 20:27:24 PDT
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.
Comment 10 Andy Estes 2011-10-05 20:41:11 PDT
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.
Comment 11 Andy Estes 2011-10-05 21:17:04 PDT
Committed r96786: <http://trac.webkit.org/changeset/96786>