[webkit-reviews] review granted: [Bug 206839] REGRESSION (r255158): http/tests/frame-throttling/raf-throttle-in-cross-origin-subframe.html is a flaky failure : [Attachment 389075] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 206839: REGRESSION (r255158):
http/tests/frame-throttling/raf-throttle-in-cross-origin-subframe.html is a
flaky failure
https://bugs.webkit.org/show_bug.cgi?id=206839

Attachment 389075: Patch

https://bugs.webkit.org/attachment.cgi?id=389075&action=review




--- Comment #9 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 389075
  --> https://bugs.webkit.org/attachment.cgi?id=389075
Patch

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

> Source/WebCore/dom/ScriptedAnimationController.cpp:99
> +void ScriptedAnimationController::renderingUpdateThrottlingEnabledChanged()
> +{
> +    m_throttlingReasons = { };
> +}

I think this should just be called clearThrottlingReasons.

> Source/WebCore/page/Page.cpp:1168
> +	  
m_throttlingReasonsOverrideForTestingMask.add(ThrottlingReason::LowPowerMode);

This seems wrong if enabled is set but false.

> Source/WebCore/page/Page.cpp:1382
> +    // Currently this function can only be called through changing the
Settings by a layout test.
> +    // So disable ThrottlingReason::VisuallyIdle always.
> +    m_throttlingReasonsOverrideForTestingMask =
ThrottlingReason::VisuallyIdle;

This function, Page::renderingUpdateThrottlingEnabledChanged(), is not
obviously testing-related but has test-only behavior, so it probably needs to
have "ForTesting" in the name.

> Source/WebCore/page/Page.cpp:1392
> +	      
scriptedAnimationController->renderingUpdateThrottlingEnabledChanged();

I'm not clear on why just clearing the throttling reasons on the
scriptedAnimationControllers is correct. Might that not clobber some previous
state set by a test?

> Source/WebCore/page/Page.h:1001
> +    OptionSet<ThrottlingReason> m_throttlingReasonsOverrideForTestingMask;

Let's just call this m_throttlingReasonsOverridenForTesting.