Summary: | Clients should have a way to extend rendering suppression | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tim Horton <thorton> | ||||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | aestes, bdakin, commit-queue, esprehn+autocc, sam, simon.fraser | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Tim Horton
2013-05-20 13:27:29 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 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); Created attachment 202327 [details]
patch
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. (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. |