Bug 116463 – Clients should have a way to extend rendering suppression

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Tim Horton]

For example, some apps do DOM manipulation after the page has finished loading, and would like to defer painting/etc. until after that happens.

<rdar://problem/13738496>

[Tim Horton]

Created attachment 202319 [details]
bad patch

a patch, but I need to rename everything to make it clear that this extends the existing rendering suppression mechanism, but is not itself an effective one (for example, grabbing a token at random while the page is up will *not* stop rendering from happening... only if you have one when the page load finishes).

[Andy Estes]

Comment on attachment 202319 [details]
bad patch

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

> Source/WebCore/dom/Document.cpp:1329
> +#ifndef NDEBUG
> +    if (!view()->visualUpdatesAllowedByClient())
> +        WTFLogAlways("Visual updates are disallowed by client, but watchdog is forcing visual updates to be allowed.\n");
> +#endif

I think it's weird that we still fire this timer even if the client is explicitly suppressing visual updates. As you mentioned on IRC, if they want some sort of slow-loading timer then they can implement it themselves.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePagePrivate.h:86
> +WK_EXPORT unsigned WKBundlePageAcquireRenderingSuppressionToken(WKBundlePageRef page) WARN_UNUSED_RETURN;
> +WK_EXPORT void WKBundlePageRelinquishRenderingSuppressionToken(WKBundlePageRef page, unsigned token);

We talked about these names on IRC. Here was my suggestion:

typedef unsigned WKRenderingSuppressionToken;
WK_EXPORT WKRenderingSuppressionToken WKBundlePageExtendIncrementalRenderingSuppression(WKBundlePageRef) WARN_UNUSED_RETURN;

And then you suggested this for the corresponding stop function:

WK_EXPORT void WKBundlePageStopExtendingIncrementalRenderingSuppression(WKBundlePageRef, WKRenderingSuppressionToken);

[Tim Horton]

Created attachment 202327 [details]
patch

[Andy Estes]

Comment on attachment 202327 [details]
patch

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

> Source/WebCore/page/FrameView.h:426
> +    bool visualUpdatesAllowedByClient() { return m_visualUpdatesAllowedByClient; }

This can be const-qualified.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePagePrivate.h:87
> +WK_EXPORT WKRenderingSuppressionToken WKBundlePageExtendIncrementalRenderingSuppression(WKBundlePageRef page) WARN_UNUSED_RETURN;
> +WK_EXPORT void WKBundlePageStopExtendingIncrementalRenderingSuppression(WKBundlePageRef page, WKRenderingSuppressionToken token);

You don't need to name these arguments since their types are self-descriptive.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4189
> +    unsigned token = ++m_maximumRenderingSuppressionToken;

Could be m_maximumRenderingSuppressionToken++ so that 0 is allowed to be a token.

[Tim Horton]

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

[Tim Horton]

(In reply to comment #4)
> (From update of attachment 202327 [details])
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4189
> > +    unsigned token = ++m_maximumRenderingSuppressionToken;
> 
> Could be m_maximumRenderingSuppressionToken++ so that 0 is allowed to be a token.

In retrospect, this was a Bad Idea because 0 is not a good thing to try to put in a HashSet, apparently.