Bug 204713

Summary: Throttling requestAnimationFrame should be controlled by RenderingUpdateScheduler
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: AnimationsAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, ashley, cdumez, commit-queue, dbates, dino, esprehn+autocc, ews-watchlist, gyuyoung.kim, kangil.han, kondapallykalyan, mcatanzaro, rniwa, ryanhaddad, ryuan.choi, sergio, simon.fraser, thorton, tsavell, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=202269
https://bugs.webkit.org/show_bug.cgi?id=206839
https://bugs.webkit.org/show_bug.cgi?id=206860
https://bugs.webkit.org/show_bug.cgi?id=211470
https://bugs.webkit.org/show_bug.cgi?id=211482
https://bugs.webkit.org/show_bug.cgi?id=211657
https://bugs.webkit.org/show_bug.cgi?id=177484
https://bugs.webkit.org/show_bug.cgi?id=215745
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from webkit-cq-03 for mac-mojave
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Said Abou-Hallawa 2019-11-30 01:50:50 PST
There should be two types of throttling:

1. Page throttling: This throttling will slow down all the RenderingUpdate steps.
2. Document throttling: This throttling will slow down the requestAnimationFrame step of a specific document.
Comment 1 Said Abou-Hallawa 2019-11-30 02:47:32 PST
Created attachment 384535 [details]
Patch
Comment 2 Said Abou-Hallawa 2019-11-30 16:28:50 PST
Created attachment 384557 [details]
Patch
Comment 3 Said Abou-Hallawa 2019-11-30 23:48:43 PST
Created attachment 384565 [details]
Patch
Comment 4 Said Abou-Hallawa 2019-12-01 15:33:38 PST
Created attachment 384582 [details]
Patch
Comment 5 Said Abou-Hallawa 2019-12-01 16:12:25 PST
Created attachment 384585 [details]
Patch
Comment 6 Said Abou-Hallawa 2019-12-02 00:52:51 PST
Created attachment 384597 [details]
Patch
Comment 7 Said Abou-Hallawa 2019-12-02 01:01:53 PST
<rdar://problem/55745055>
Comment 8 Simon Fraser (smfr) 2019-12-02 11:13:59 PST
Comment on attachment 384597 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        1) Page throttling which slows down all the steps of RenderingUpdate.
> +        2) Document throttling which only slows down the rAF of the document.

Does this mean that a rendering update might happen but rAF does not fire because of Document throttling? I don't think we ever want this. I think if a rendering update happens, rAF has to fire. That implies that throttling only impacts rendering updates, which in turns means that we need to be able to throttle rendering updates in subframes independently from the Page.

It might be tricky to decide whether a subframe gets layout so that it can be painted by its parent, even if it didn't have a rendering update.
Comment 9 Said Abou-Hallawa 2019-12-03 15:06:52 PST
Comment on attachment 384597 [details]
Patch

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

>> Source/WebCore/ChangeLog:15
>> +        2) Document throttling which only slows down the rAF of the document.
> 
> Does this mean that a rendering update might happen but rAF does not fire because of Document throttling? I don't think we ever want this. I think if a rendering update happens, rAF has to fire. That implies that throttling only impacts rendering updates, which in turns means that we need to be able to throttle rendering updates in subframes independently from the Page.
> 
> It might be tricky to decide whether a subframe gets layout so that it can be painted by its parent, even if it didn't have a rendering update.

This patch fixes the bug which causes the rAF to run every 11ms instead of 30ms when a low power mode is turned on on an iOS device. Also it will not change any behavior. I think what you are asking is an enhancement which can be done in a separate patch. And I think the change should be small. For throttled iframes, we will need to move the decision to skip the RenderingUpdate from the ScriptedAnimationController to the Document or the Page. All we need is to skip more from the rendering update steps rather than the rAF only which we used to do before introducing the RenderingUpdate r244182.
Comment 10 Simon Fraser (smfr) 2019-12-03 16:42:40 PST
Comment on attachment 384597 [details]
Patch

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

> Source/WebCore/dom/ScriptedAnimationController.h:54
> +    int registerCallback(Ref<RequestAnimationFrameCallback>&&);

Why did the typedef go away?

> Source/WebCore/dom/ScriptedAnimationController.h:63
> -    enum class ThrottlingReason {
> -        VisuallyIdle                    = 1 << 0,
> -        OutsideViewport                 = 1 << 1,
> -        LowPowerMode                    = 1 << 2,
> -        NonInteractedCrossOriginFrame   = 1 << 3,
> -    };
> -    void addThrottlingReason(ThrottlingReason);
> -    void removeThrottlingReason(ThrottlingReason);
> -
> -    WEBCORE_EXPORT bool isThrottled() const;
> -    WEBCORE_EXPORT Seconds interval() const;
> +    
> +    void setOutsideViewport(bool outsideViewport) { m_outsideViewport = outsideViewport; }
> +    void setNonInteractedCrossOriginFrame(bool nonInteractedCrossOriginFrame) { m_nonInteractedCrossOriginFrame = nonInteractedCrossOriginFrame; }
> +    bool isThrottled() const { return interval() > fullSpeedAnimationInterval; }

I prefer the old version, which should use an OptionSet<> of reasons.

> Source/WebCore/page/Page.h:714
> +    Seconds preferredRenderingUpdateInterval() const;
> +    unsigned preferredFramesPerSecond() const;

It's really confusing to have both of these.

> Source/WebCore/page/RenderingUpdateScheduler.cpp:73
> +    if (interval > 1_s || !DisplayRefreshMonitorManager::sharedManager().scheduleAnimation(*this, m_page.preferredFramesPerSecond()))

Why the 1s cutoff here?

> Source/WebCore/page/RenderingUpdateScheduler.h:50
> +    void updateRenderingUpdateRate();

Let's call this adjustRenderingUpdateFrequency()

> Source/WebCore/page/RenderingUpdateScheduler.h:58
> +    void updatePreferredFramesPerSecond();

Confusing to have this and updateRenderingUpdateRate()

> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.h:48
> +    void setPreferredFramesPerSecond(DisplayRefreshMonitorClient&, unsigned);
> +    bool scheduleAnimation(DisplayRefreshMonitorClient&, unsigned = MaxFramesPerSecond);

That unsigned arg needs to have a name. And it's odd to have both setPreferredFramesPerSecond and this argument to scheduleAnimation().
Comment 11 Said Abou-Hallawa 2019-12-06 16:34:40 PST
Created attachment 385056 [details]
Patch
Comment 12 Said Abou-Hallawa 2019-12-06 17:21:36 PST
Created attachment 385070 [details]
Patch
Comment 13 Said Abou-Hallawa 2019-12-07 22:14:51 PST
Created attachment 385114 [details]
Patch
Comment 14 Said Abou-Hallawa 2019-12-08 07:43:11 PST
Comment on attachment 384597 [details]
Patch

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

>> Source/WebCore/dom/ScriptedAnimationController.h:54
>> +    int registerCallback(Ref<RequestAnimationFrameCallback>&&);
> 
> Why did the typedef go away?

Because RequestAnimationFrameCallback::m_id is of type int and not CallbackId. But I put it back.

>> Source/WebCore/dom/ScriptedAnimationController.h:63
>> +    bool isThrottled() const { return interval() > fullSpeedAnimationInterval; }
> 
> I prefer the old version, which should use an OptionSet<> of reasons.

I put it back.

>> Source/WebCore/page/Page.h:714
>> +    unsigned preferredFramesPerSecond() const;
> 
> It's really confusing to have both of these.

The reason for having both of these functions is there is no mathematical formula that can convert from a frame interval to fps accurately. DisplayRefreshMonitor needs a fps while the timer needs an interval. So I removed preferredFramesPerSecond() from here added another function (not in Page.h) which converts an interval to fps.

>> Source/WebCore/page/RenderingUpdateScheduler.cpp:73
>> +    if (interval > 1_s || !DisplayRefreshMonitorManager::sharedManager().scheduleAnimation(*this, m_page.preferredFramesPerSecond()))
> 
> Why the 1s cutoff here?

If the interval > 1_s, the fps will be < 1 and the preferredFramesPerSecond has to be integer..

>> Source/WebCore/page/RenderingUpdateScheduler.h:50
>> +    void updateRenderingUpdateRate();
> 
> Let's call this adjustRenderingUpdateFrequency()

Done.

>> Source/WebCore/page/RenderingUpdateScheduler.h:58
>> +    void updatePreferredFramesPerSecond();
> 
> Confusing to have this and updateRenderingUpdateRate()

This function did not have an implementation. It was left in the header file by mistake.

>> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.h:48
>> +    bool scheduleAnimation(DisplayRefreshMonitorClient&, unsigned = MaxFramesPerSecond);
> 
> That unsigned arg needs to have a name. And it's odd to have both setPreferredFramesPerSecond and this argument to scheduleAnimation().

A new type called FramesPerSecond was added. The reason for having this argument passed to scheduleAnimation() was I needed to set preferredFramesPerSecond when the DisplayRefreshMonitor is created.

I removed this argument and I made setting the initial preferredFramesPerSecond in RenderingUpdateScheduler::scheduleTimedRenderingUpdate().
Comment 15 Simon Fraser (smfr) 2019-12-09 10:23:35 PST
Comment on attachment 385114 [details]
Patch

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

> Source/WebCore/dom/ScriptedAnimationController.h:66
>      WEBCORE_EXPORT bool isThrottled() const;

Does anyone still call isThrottled()?

> Source/WebCore/page/RenderingUpdateScheduler.cpp:45
> +#if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR) && PLATFORM(IOS)

PLATFORM(IOS_FAMILY)

> Source/WebCore/page/RenderingUpdateScheduler.cpp:49
> +    if (interval < 1_s)

Why is 1_s special? Doesn't DisplayRefreshMonitorManager handle any refresh rate?

> Source/WebCore/page/RenderingUpdateScheduler.cpp:56
> +#if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR) && PLATFORM(IOS)

PLATFORM(IOS_FAMILY)

> Source/WebCore/page/RenderingUpdateScheduler.cpp:81
> +    if (interval < 1_s) {

Same weird cutoff.

> Source/WebCore/page/RenderingUpdateScheduler.cpp:82
> +#if PLATFORM(IOS)

PLATFORM(IOS_FAMILY)

> Source/WebCore/page/RenderingUpdateScheduler.h:57
> +#if PLATFORM(IOS)

PLATFORM(IOS_FAMILY)

> Source/WebCore/page/RenderingUpdateScheduler.h:74
> +#if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR) && PLATFORM(IOS)

PLATFORM(IOS_FAMILY)

> Source/WebCore/page/RenderingUpdateScheduler.h:75
> +    std::once_flag m_createdMonitor;

These are always static.

> Source/WebCore/platform/graphics/AnimationFrameRate.h:48
> +constexpr const FramesPerSecond ZeroFramesPerSecond = 60;

This is very confusing.

> Source/WebCore/platform/graphics/AnimationFrameRate.h:50
> +constexpr const FramesPerSecond MaxFramesPerSecond = 60;

Maybe FullSpeedFramesPerSecond.

> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:60
> -    LOG(RequestAnimationFrame, "DisplayRefreshMonitorManager::createMonitorForClient() - created monitor %p", monitor.get());
> +    LOG(RequestAnimationFrame, "DisplayRefreshMonitorManager::monitorForClient() - created monitor %p", monitor.get());

This is createMonitorForClient
Comment 16 Said Abou-Hallawa 2019-12-09 11:54:50 PST
Created attachment 385174 [details]
Patch
Comment 17 Said Abou-Hallawa 2019-12-09 12:01:02 PST
Comment on attachment 385114 [details]
Patch

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

>> Source/WebCore/dom/ScriptedAnimationController.h:66
>>      WEBCORE_EXPORT bool isThrottled() const;
> 
> Does anyone still call isThrottled()?

Yes Internals::isRequestAnimationFrameThrottled() which is used by:

LayoutTests/fast/animation/request-animation-frame-throttle-inside-overflow-scroll.html:
LayoutTests/fast/animation/request-animation-frame-throttle-subframe-display-none.html
LayoutTests/fast/animation/request-animation-frame-throttle-subframe-zero-size.html
LayoutTests/fast/animation/request-animation-frame-throttle-subframe.html
LayoutTests/fast/animation/request-animation-frame-throttling-detached-iframe.html

>> Source/WebCore/page/RenderingUpdateScheduler.cpp:45
>> +#if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR) && PLATFORM(IOS)
> 
> PLATFORM(IOS_FAMILY)

Fixed.

>> Source/WebCore/page/RenderingUpdateScheduler.cpp:49
>> +    if (interval < 1_s)
> 
> Why is 1_s special? Doesn't DisplayRefreshMonitorManager handle any refresh rate?

No. CADisplayLink.preferredFramesPerSecond is an integer. So a fraction PreferredFramesPerSecond can't be set. RenderingUpdateScheduler has to fall back to use the timer in this case.

>> Source/WebCore/page/RenderingUpdateScheduler.cpp:56
>> +#if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR) && PLATFORM(IOS)
> 
> PLATFORM(IOS_FAMILY)

Fixed.

>> Source/WebCore/page/RenderingUpdateScheduler.cpp:81
>> +    if (interval < 1_s) {
> 
> Same weird cutoff.

A comment is added.

>> Source/WebCore/page/RenderingUpdateScheduler.cpp:82
>> +#if PLATFORM(IOS)
> 
> PLATFORM(IOS_FAMILY)

Fixed.

>> Source/WebCore/page/RenderingUpdateScheduler.h:57
>> +#if PLATFORM(IOS)
> 
> PLATFORM(IOS_FAMILY)

Fixed.

>> Source/WebCore/page/RenderingUpdateScheduler.h:74
>> +#if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR) && PLATFORM(IOS)
> 
> PLATFORM(IOS_FAMILY)

Fixed.

>> Source/WebCore/page/RenderingUpdateScheduler.h:75
>> +    std::once_flag m_createdMonitor;
> 
> These are always static.

A member bool is used instead since it has to be called once per RenderingUpdateScheduler.

>> Source/WebCore/platform/graphics/AnimationFrameRate.h:48
>> +constexpr const FramesPerSecond ZeroFramesPerSecond = 60;
> 
> This is very confusing.

Yes. This was a mistake. It has to be = 0;

>> Source/WebCore/platform/graphics/AnimationFrameRate.h:50
>> +constexpr const FramesPerSecond MaxFramesPerSecond = 60;
> 
> Maybe FullSpeedFramesPerSecond.

Fixed.
Comment 18 WebKit Commit Bot 2019-12-09 13:12:25 PST
Comment on attachment 385174 [details]
Patch

Clearing flags on attachment: 385174

Committed r253299: <https://trac.webkit.org/changeset/253299>
Comment 19 WebKit Commit Bot 2019-12-09 13:12:28 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Truitt Savell 2019-12-09 15:16:25 PST
The changes in https://trac.webkit.org/changeset/253299/webkit

appear to have caused 60+ imported/ test failures on Catalina Release wk2 and 35 on other Mac platforms.

build:
https://build.webkit.org/builders/Apple-Catalina-Release-WK2-Tests/builds/1365

It looks like EWS did catch this.
Comment 21 Said Abou-Hallawa 2019-12-09 16:12:15 PST
(In reply to Truitt Savell from comment #20)
> The changes in https://trac.webkit.org/changeset/253299/webkit
> 
> appear to have caused 60+ imported/ test failures on Catalina Release wk2
> and 35 on other Mac platforms.
> 
> build:
> https://build.webkit.org/builders/Apple-Catalina-Release-WK2-Tests/builds/
> 1365
> 
> It looks like EWS did catch this.

But EWS did not catch it with attachment 385114 [details]. The difference between attachment  385114 [details] and attachment 385174 [details] was just adding few comments and changing the directive PLATFORM(IOS) to PLATFORM(IOS_FAMILY).
Comment 22 Truitt Savell 2019-12-09 16:47:15 PST
Reverted r253299 for reason:

Casued 30+ imported/ test failures on Mac wk2

Committed r253308: <https://trac.webkit.org/changeset/253308>
Comment 23 Said Abou-Hallawa 2020-01-23 23:43:19 PST
Created attachment 388657 [details]
Patch
Comment 24 Said Abou-Hallawa 2020-01-24 08:40:40 PST
Created attachment 388698 [details]
Patch
Comment 25 Said Abou-Hallawa 2020-01-24 11:15:50 PST
I uploaded a pa(In reply to Said Abou-Hallawa from comment #24)
> Created attachment 388698 [details]
> Patch

This patch and the previous one are the same as r253299 but with some logging. I was expecting to see failures on EWS and from the logging I can understand the nature of the problem. But EWS did not fail. The failures that caused this patch to be rolled out was on Catalina Release wk2. But the Catalina is used only on post commit testing which means I need to land this patch with the logging and wait for the failures on Catalina.
Comment 26 Said Abou-Hallawa 2020-01-24 13:37:08 PST
*** Bug 202269 has been marked as a duplicate of this bug. ***
Comment 27 Said Abou-Hallawa 2020-01-24 19:05:36 PST
Created attachment 388754 [details]
Patch
Comment 28 Said Abou-Hallawa 2020-01-24 20:27:43 PST
Created attachment 388757 [details]
Patch
Comment 29 Tim Horton 2020-01-25 00:30:00 PST
Comment on attachment 388757 [details]
Patch

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

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:997
> +    [preferences setRenderingUpdateThrottlingEnabled:NO];

Do you need to turn it off for Windows DRT too? That might explain the failures.

> LayoutTests/fast/animation/request-animation-frame-throttling-outside-viewport.html:18
> +                ++ifarmeFarmesPerSecond;

ifarme, farme, etc.
Comment 30 Said Abou-Hallawa 2020-01-25 00:50:45 PST
Created attachment 388764 [details]
Patch
Comment 31 Said Abou-Hallawa 2020-01-25 11:16:59 PST
Created attachment 388783 [details]
Patch
Comment 32 Said Abou-Hallawa 2020-01-25 11:47:11 PST
Created attachment 388784 [details]
Patch
Comment 33 Said Abou-Hallawa 2020-01-25 20:07:57 PST
Created attachment 388795 [details]
Patch
Comment 34 Said Abou-Hallawa 2020-01-26 02:00:40 PST
Created attachment 388800 [details]
Patch
Comment 35 Said Abou-Hallawa 2020-01-26 10:28:57 PST
The Windows EWS seems to fail because it needs a clean build. I think the change I did in Settings.ymal did not force generating new derived sources for Settings.h and Settings.cpp.

https://bugs.webkit.org/show_bug.cgi?id=201641 was changing Settings.ymal and the Womndows EWS failed processing the batch and it gave exactly the same error.
Comment 36 WebKit Commit Bot 2020-01-26 11:51:26 PST
Comment on attachment 388800 [details]
Patch

Rejecting attachment 388800 [details] from commit-queue.

New failing tests:
editing/spelling/spellcheck-async-remove-frame.html
Full output: https://webkit-queues.webkit.org/results/13311845
Comment 37 WebKit Commit Bot 2020-01-26 11:51:28 PST
Created attachment 388809 [details]
Archive of layout-test-results from webkit-cq-03 for mac-mojave

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-03  Port: mac-mojave  Platform: Mac OS X 10.14.6
Comment 38 Said Abou-Hallawa 2020-01-26 12:23:08 PST
Created attachment 388811 [details]
Patch
Comment 39 WebKit Commit Bot 2020-01-26 13:35:38 PST
Comment on attachment 388811 [details]
Patch

Clearing flags on attachment: 388811

Committed r255131: <https://trac.webkit.org/changeset/255131>
Comment 40 WebKit Commit Bot 2020-01-26 13:35:40 PST
All reviewed patches have been landed.  Closing bug.
Comment 41 Truitt Savell 2020-01-27 09:15:05 PST
It looks like the changes in https://trac.webkit.org/changeset/255131/webkit

caused testing to exit early on iOS Debug with 50+ crashes
https://build.webkit.org/builders/Apple%20iOS%2013%20Simulator%20Debug%20WK2%20%28Tests%29/builds/1874
Comment 42 Ryan Haddad 2020-01-27 09:26:14 PST
Reverted r255131 for reason:

Caused assertion failures on iOS debug bots.

Committed r255150: <https://trac.webkit.org/changeset/255150>
Comment 43 Said Abou-Hallawa 2020-01-27 10:17:36 PST
Created attachment 388871 [details]
Patch
Comment 44 Said Abou-Hallawa 2020-01-27 10:23:17 PST
(In reply to Truitt Savell from comment #41)
> It looks like the changes in https://trac.webkit.org/changeset/255131/webkit
> 
> caused testing to exit early on iOS Debug with 50+ crashes
> https://build.webkit.org/builders/
> Apple%20iOS%2013%20Simulator%20Debug%20WK2%20%28Tests%29/builds/1874

This was caused by the change I did in [WebLowPowerModeObserver initWithNotifier] where I was trying to set the initial state of the low power mode. Without this change, rAF will not be throttling if MobileSafari starts and the device is in low power mode. It will throttle only when the low power mode is turned off and then it is turned on.

Since this change broke the iOS layout tests, I am going to undo it. I am going to file a new bug for setting the initial state of the low power mode.
Comment 45 WebKit Commit Bot 2020-01-27 11:53:54 PST
The commit-queue encountered the following flaky tests while processing attachment 388871 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org)
The commit-queue is continuing to process your patch.
Comment 46 WebKit Commit Bot 2020-01-27 11:54:47 PST
Comment on attachment 388871 [details]
Patch

Clearing flags on attachment: 388871

Committed r255158: <https://trac.webkit.org/changeset/255158>
Comment 47 WebKit Commit Bot 2020-01-27 11:54:50 PST
All reviewed patches have been landed.  Closing bug.
Comment 48 Said Abou-Hallawa 2020-02-13 09:13:58 PST
Reverted r255158, 255405 and r255486 because they cause test flakiness and PLT regression.

Committed r256512: <https://trac.webkit.org/changeset/256512>
Comment 49 Said Abou-Hallawa 2020-04-29 20:02:42 PDT
Created attachment 398026 [details]
Patch
Comment 50 Said Abou-Hallawa 2020-04-30 15:04:34 PDT
Created attachment 398097 [details]
Patch
Comment 51 Said Abou-Hallawa 2020-04-30 15:07:50 PDT
(In reply to Said Abou-Hallawa from comment #50)
> Created attachment 398097 [details]
> Patch

The goal is to fix the layout tests failures and flakiness without the internal setting 'RenderingUpdateThrottling'. This will ensure the page load will not be affected by ThrottlingReason::VisuallyIdle.

In this patch I replaced isRequestAnimationFrameThrottled() by requestAnimationFrameThrottlingReasons() which returns a String with the throttling reasons.
Comment 52 Said Abou-Hallawa 2020-04-30 16:19:42 PDT
Created attachment 398115 [details]
Patch
Comment 53 Said Abou-Hallawa 2020-04-30 16:20:25 PDT
Created attachment 398116 [details]
Patch
Comment 54 Said Abou-Hallawa 2020-04-30 19:54:36 PDT
Created attachment 398147 [details]
Patch
Comment 55 Said Abou-Hallawa 2020-04-30 21:04:08 PDT
I added the following log message to Page::setIsVisuallyIdleInternal():

WTFLogAlways("In Page::setIsVisuallyIdleInternal(): isVisuallyIdle = %d, m_updateRenderingCount = %ld", isVisuallyIdle, m_updateRenderingCount);

And from the failed tests, we got the following message:

In Page::setIsVisuallyIdleInternal(): isVisuallyIdle = 1, m_updateRenderingCount = 0
Comment 56 Said Abou-Hallawa 2020-04-30 22:44:35 PDT
Created attachment 398162 [details]
Patch
Comment 57 Said Abou-Hallawa 2020-05-01 00:13:13 PDT
Created attachment 398167 [details]
Patch
Comment 58 Said Abou-Hallawa 2020-05-01 01:43:26 PDT
Created attachment 398169 [details]
Patch
Comment 59 Said Abou-Hallawa 2020-05-01 09:59:26 PDT
Created attachment 398195 [details]
Patch
Comment 60 Simon Fraser (smfr) 2020-05-01 12:24:10 PDT
Comment on attachment 398195 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        requestAnimationFrame is throttled by a timer although its callback are

callbacks

> Source/WebCore/ChangeLog:60
> +        is greater than the preferred rAF interval .

extra space

> Source/WebCore/page/Page.cpp:1497
> +    WTFLogAlways("In Page::setIsVisuallyIdleInternal(): isVisuallyIdle = %d", isVisuallyIdle);

Log left here.

> Source/WebCore/platform/graphics/AnimationFrameRate.h:63
> +//    if (reasons.containsAny({ ThrottlingReason::VisuallyIdle, ThrottlingReason::OutsideViewport }))
> +        return AggressiveThrottlingAnimationInterval;
> +
> +    if (reasons.containsAny({ ThrottlingReason::LowPowerMode, ThrottlingReason::NonInteractedCrossOriginFrame }))
> +        return HalfSpeedThrottlingAnimationInterval;
> +
> +//    ASSERT(reasons.isEmpty());
> +    return FullSpeedAnimationInterval;

Commented out code.

> Source/WebCore/platform/graphics/AnimationFrameRate.h:78
> +inline const char* throttlingReasonToString(ThrottlingReason reason)

Maybe do WTF::TextStream& operator<<(WTF::TextStream&, ThrottlingReason) instead, assuming throttlingReasonsToString is just used for testing.
Comment 61 Said Abou-Hallawa 2020-05-01 15:30:24 PDT
Created attachment 398253 [details]
Patch
Comment 62 Said Abou-Hallawa 2020-05-04 11:45:03 PDT
Created attachment 398395 [details]
Patch
Comment 63 Said Abou-Hallawa 2020-05-04 12:06:02 PDT
Comment on attachment 398395 [details]
Patch

This is scoped implementation for the Page and Document throttling. Only the "Half speed throttling" is implemented by this patch. The "Aggressive throttling" will be done in following patches.
Comment 64 EWS 2020-05-04 14:24:54 PDT
Committed r261113: <https://trac.webkit.org/changeset/261113>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398395 [details].
Comment 65 Michael Catanzaro 2020-05-12 09:34:28 PDT
Said, this introduced an unused variable warning:

[436/1151] Building CXX object Source/...ources/UnifiedSource-be65d27a-17.cpp.o
In file included from DerivedSources/WebCore/unified-sources/UnifiedSource-be65d27a-17.cpp:2:
/home/mcatanzaro/Projects/WebKit/Source/WebCore/dom/ScriptedAnimationController.cpp: In member function ‘WTF::Seconds WebCore::ScriptedAnimationController::preferredScriptedAnimationInterval() const’:
/home/mcatanzaro/Projects/WebKit/Source/WebCore/dom/ScriptedAnimationController.cpp:82:15: warning: unused variable ‘page’ [-Wunused-variable]
   82 |     if (auto* page = this->page())
      |               ^~~~

Right here:

Seconds ScriptedAnimationController::preferredScriptedAnimationInterval() const
{
    if (auto* page = this->page())
        return preferredFrameInterval(m_throttlingReasons);
    return FullSpeedAnimationInterval;
}

What is the desired behavior there?
Comment 66 Said Abou-Hallawa 2020-05-15 00:09:25 PDT
Thanks Michael for pointing out this issue. I fixed it in <https://trac.webkit.org/changeset/261732>.

It happened because of a last minute change. This function should be like this after the fix of https://bugs.webkit.org/show_bug.cgi?id=211657:

Seconds ScriptedAnimationController::preferredScriptedAnimationInterval() const
{
    if (auto* page = this->page())
        return preferredFrameInterval(page->throttlingReasons() | m_throttlingReasons);
    return FullSpeedAnimationInterval;
}

Before submitting r261113, I took out the part "page->throttlingReasons()" but did not fix the unused variable warning.