Bug 116463 - Clients should have a way to extend rendering suppression
Summary: Clients should have a way to extend rendering suppression
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Keywords: InRadar
Depends on:
Reported: 2013-05-20 13:27 PDT by Tim Horton
Modified: 2013-05-28 13:37 PDT (History)
6 users (show)

See Also:

bad patch (13.28 KB, patch)
2013-05-20 14:14 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
patch (13.63 KB, patch)
2013-05-20 15:37 PDT, Tim Horton
aestes: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2013-05-20 13:27:29 PDT
For example, some apps do DOM manipulation after the page has finished loading, and would like to defer painting/etc. until after that happens.

Comment 1 Tim Horton 2013-05-20 14:14:50 PDT
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).
Comment 2 Andy Estes 2013-05-20 15:03:36 PDT
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);
Comment 3 Tim Horton 2013-05-20 15:37:00 PDT
Created attachment 202327 [details]
Comment 4 Andy Estes 2013-05-20 16:22:11 PDT
Comment on attachment 202327 [details]

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.
Comment 5 Tim Horton 2013-05-20 16:43:17 PDT
Comment 6 Tim Horton 2013-05-28 13:37:02 PDT
(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.