Created attachment 252559 [details] Patch

Comment on attachment 252559 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252559&action=review r=me but please fix the red EWS. > Source/WebCore/dom/ScriptedAnimationController.cpp:87 > + LOG(Animations, "%p - Setting RequestAnimationFrame throttling state to %d in frame", this, isThrottled); in frame what? Would be nice to know fi this is the main frame. > Source/WebCore/page/FrameView.cpp:2181 > + if (auto* scriptedAnimationController = frame().document()->scriptedAnimationController()) { Just auto, right? > Source/WebCore/page/FrameView.cpp:2182 > + bool shouldThrottle = forceThrottling || windowToContents(windowClipRect()).isEmpty(); Since we're calling this just after resumeVisibleImageAnimationsIncludingSubframes(), we've probably computed windowToContents(windowClipRect()) already. > Source/WebCore/page/FrameView.cpp:2190 > + for (Frame* childFrame = frame().tree().firstChild(); childFrame; childFrame = childFrame->tree().nextSibling()) { This makes me wonder if we do things in display:none iframes. I added firstRenderedChild()/nextRenderedSibling() to only hit iframes which are not display:none, but if you have a visible frame running animations, and you display:none it, what happens? > Source/WebCore/page/FrameView.cpp:2191 > + if (auto* childView = childFrame->view()) auto? > Source/WebCore/testing/Internals.cpp:772 > + auto* scriptedAnimationController = contextDocument()->scriptedAnimationController(); auto?

Comment on attachment 252559 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252559&action=review >> Source/WebCore/page/FrameView.cpp:2181 >> + if (auto* scriptedAnimationController = frame().document()->scriptedAnimationController()) { > > Just auto, right? I actually prefer auto* to make it clear it is a pointer. I have seen Darin make similar comments. >> Source/WebCore/page/FrameView.cpp:2182 >> + bool shouldThrottle = forceThrottling || windowToContents(windowClipRect()).isEmpty(); > > Since we're calling this just after resumeVisibleImageAnimationsIncludingSubframes(), we've probably computed windowToContents(windowClipRect()) already. Yes, I'll try and leverage this. >> Source/WebCore/page/FrameView.cpp:2190 >> + for (Frame* childFrame = frame().tree().firstChild(); childFrame; childFrame = childFrame->tree().nextSibling()) { > > This makes me wonder if we do things in display:none iframes. I added firstRenderedChild()/nextRenderedSibling() to only hit iframes which are not display:none, but if you have a visible frame running animations, and you display:none it, what happens? Very good question. I add a test and update the code if needed.

Created attachment 252605 [details] Patch

Attachment 252605 [details] did not pass style-queue: ERROR: Source/WebCore/page/FrameView.h:602: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/page/FrameView.cpp:2152: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 252605 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252605&action=review > Source/WebCore/ChangeLog:12 > + Tests: fast/animation/request-animation-frame-throttle-subframe-display-none.html Added a new layout test for display: none subframes. > Source/WebCore/dom/ScriptedAnimationController.cpp:102 > +#if USE(REQUEST_ANIMATION_FRAME_TIMER) && USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR) Should fix red ews. > Source/WebCore/page/FrameView.cpp:1792 > + applyRecursively([] (FrameView& frameView, IntRect visibleRect) { Added a new applyRecursively() utility function so that the frame tree is only traversed once and we avoid redundant windowClipRect() computations. > Source/WebCore/page/FrameView.cpp:2196 > + bool shouldThrottle = !frame().ownerRenderer() || visibleRect.isEmpty(); Added this !frame().ownerRenderer() check to make the "display: none" subframe case work. Sadly, this is insufficient for subframes of a "display: none" frame because they get a non-null ownerRenderer. I am planning to look into this in a follow-up.

Comment on attachment 252605 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252605&action=review > Source/WebCore/page/FrameView.cpp:2157 > +void FrameView::applyRecursively(const std::function<void (FrameView& frameView, IntRect windowClipRect)>& apply) This name is a bit generic given the windowClipRect code that's here. If this is intended to be the One True Way to recurse through subframes, it should be a bit more generic. We should probably also have an argument, or variant that only hits rendered subframes (firstRenderedChild etc).

Comment on attachment 252605 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252605&action=review >> Source/WebCore/page/FrameView.cpp:2157 >> +void FrameView::applyRecursively(const std::function<void (FrameView& frameView, IntRect windowClipRect)>& apply) > > This name is a bit generic given the windowClipRect code that's here. If this is intended to be the One True Way to recurse through subframes, it should be a bit more generic. We should probably also have an argument, or variant that only hits rendered subframes (firstRenderedChild etc). Ideally, I would find a less generic name for this, given the visibleRect argument. Maybe applyRecursivelyGivenVisibleRect()?

Comment on attachment 252605 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252605&action=review >>> Source/WebCore/page/FrameView.cpp:2157 >>> +void FrameView::applyRecursively(const std::function<void (FrameView& frameView, IntRect windowClipRect)>& apply) >> >> This name is a bit generic given the windowClipRect code that's here. If this is intended to be the One True Way to recurse through subframes, it should be a bit more generic. We should probably also have an argument, or variant that only hits rendered subframes (firstRenderedChild etc). > > Ideally, I would find a less generic name for this, given the visibleRect argument. Maybe applyRecursivelyGivenVisibleRect()? I don't really need a version that only traverses rendered frames at the moment. Also, if a client wanted to do so, it would be easy enough to do a frame().ownerRenderer() check in the lamdba.

Created attachment 252623 [details] Patch

Attachment 252623 [details] did not pass style-queue: ERROR: Source/WebCore/page/FrameView.h:600: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/page/FrameView.cpp:2102: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 252623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252623&action=review > Source/WebCore/page/FrameView.cpp:2157 > +void FrameView::applyRecursivelyGivenVisibleRect(const std::function<void (FrameView& frameView, IntRect visibleRect)>& apply) I renamed this method to make it less generic. Let me know if you have a better naming proposal.

Created attachment 252679 [details] Patch

Attachment 252679 [details] did not pass style-queue: ERROR: Source/WebCore/page/FrameView.h:600: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/page/FrameView.cpp:2107: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 252679 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252679&action=review > Source/WebCore/page/FrameView.h:600 > + void applyRecursivelyGivenVisibleRect(const std::function<void (FrameView& frameView, IntRect visibleRect)>&); I'd say applyRecursivelyWithVisibleRect. > Source/WebCore/page/FrameView.h:603 > + void updateThrottledDOMTimersState(IntRect visibleRect); > + void resumeVisibleImageAnimations(IntRect visibleRect); > + void updateScriptedAnimationsThrottlingState(IntRect visibleRect); Are we passing IntRects (32 bytes) by value now?

Created attachment 252680 [details] Patch

Attachment 252680 [details] did not pass style-queue: ERROR: Source/WebCore/page/FrameView.h:600: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/page/FrameView.cpp:2107: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 252680 [details] Patch Clearing flags on attachment: 252680 Committed r183985: <http://trac.webkit.org/changeset/183985>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by bug 144796

CRASHING TEST: loader/go-back-to-different-window-size.html https://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r183987%20(11604)/loader/go-back-to-different-window-size-crash-log.txt Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.JavaScriptCore 0x000000010eba1aba WTFCrash + 42 (Assertions.cpp:321) 1 com.apple.WebCore 0x0000000110bbd315 WebCore::FrameView::windowClipRect() const + 85 (FrameView.cpp:3277) 2 com.apple.WebCore 0x0000000110bbb570 WebCore::FrameView::applyRecursivelyWithVisibleRect(std::__1::function<void (WebCore::FrameView&, WebCore::IntRect const&)> const&) + 32 (FrameView.cpp:2109) 3 com.apple.WebCore 0x0000000110bb4f43 WebCore::FrameView::viewportContentsChanged() + 99 (FrameView.cpp:1766) 4 com.apple.WebCore 0x0000000110bb4e0f WebCore::FrameView::setFrameRect(WebCore::IntRect const&) + 239 (FrameView.cpp:466) 5 com.apple.WebCore 0x0000000110b86912 WebCore::FrameLoader::open(WebCore::CachedFrameBase&) + 786 (FrameLoader.cpp:2073) 6 com.apple.WebCore 0x000000011047f029 WebCore::CachedFrame::open() + 201 (CachedFrame.cpp:219) 7 com.apple.WebCore 0x000000011048a768 WebCore::CachedPage::restore(WebCore::Page&) + 328 (CachedPage.cpp:87) 8 com.apple.WebCore 0x0000000110b84da3 WebCore::FrameLoader::commitProvisionalLoad() + 2643 (FrameLoader.cpp:1796) 9 com.apple.WebCore 0x0000000110b88c17 WebCore::FrameLoader::loadProvisionalItemFromCachedPage() + 295 (FrameLoader.cpp:3093) 10 com.apple.WebCore 0x0000000110b83185 WebCore::FrameLoader::continueLoadAfterNavigationPolicy(WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool, WebCore::AllowNavigationToInvalidURL) + 1029 (FrameLoader.cpp:2944) 11 com.apple.WebCore 0x0000000110b8e36e WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*, WebCore::FrameLoadType, WTF::PassRefPtr<WebCore::FormState>, WebCore::AllowNavigationToInvalidURL)::$_4::operator()(WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool) const + 94 (FrameLoader.cpp:1458) 12 com.apple.WebCore 0x0000000110b8e2f5 std::__1::__function::__func<WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*, WebCore::FrameLoadType, WTF::PassRefPtr<WebCore::FormState>, WebCore::AllowNavigationToInvalidURL)::$_4, std::__1::allocator<WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*, WebCore::FrameLoadType, WTF::PassRefPtr<WebCore::FormState>, WebCore::AllowNavigationToInvalidURL)::$_4>, void (WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool)>::operator()(WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>&&, bool&&) + 229 (__functional_base:413) 13 com.apple.WebCore 0x0000000111942077 std::__1::function<void (WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool)>::operator()(WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool) const + 87 (functional:1755) 14 com.apple.WebCore 0x0000000111941706 WebCore::PolicyCallback::call(bool) + 134 (PolicyCallback.cpp:95) 15 com.apple.WebCore 0x0000000111942c9a WebCore::PolicyChecker::continueAfterNavigationPolicy(WebCore::PolicyAction) + 682 (PolicyChecker.cpp:206) 16 com.apple.WebCore 0x0000000111946ace WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest const&, WebCore::DocumentLoader*, WTF::PassRefPtr<WebCore::FormState>, std::__1::function<void (WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool)>)::$_1::operator()(WebCore::PolicyAction) const + 30 (PolicyChecker.cpp:124) 17 com.apple.WebCore 0x0000000111946a9e std::__1::__function::__func<WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest const&, WebCore::DocumentLoader*, WTF::PassRefPtr<WebCore::FormState>, std::__1::function<void (WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool)>)::$_1, std::__1::allocator<WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest const&, WebCore::DocumentLoader*, WTF::PassRefPtr<WebCore::FormState>, std::__1::function<void (WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool)>)::$_1>, void (WebCore::PolicyAction)>::operator()(WebCore::PolicyAction&&) + 94 (functional:1370) 18 com.apple.WebKit 0x000000010b5b615c std::__1::function<void (WebCore::PolicyAction)>::operator()(WebCore::PolicyAction) const + 44 (functional:1755) 19 com.apple.WebKit 0x000000010b5bc52f WebKit::WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(WebCore::NavigationAction const&, WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, std::__1::function<void (WebCore::PolicyAction)>) + 431 (WebFrameLoaderClient.cpp:756) 20 com.apple.WebCore 0x0000000111942922 WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest const&, WebCore::DocumentLoader*, WTF::PassRefPtr<WebCore::FormState>, std::__1::function<void (WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool)>) + 1442 (PolicyChecker.cpp:122) 21 com.apple.WebCore 0x0000000110b82864 WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*, WebCore::FrameLoadType, WTF::PassRefPtr<WebCore::FormState>, WebCore::AllowNavigationToInvalidURL) + 1668 (FrameLoader.cpp:1457) 22 com.apple.WebCore 0x0000000110b7f0e6 WebCore::FrameLoader::loadDifferentDocumentItem(WebCore::HistoryItem&, WebCore::FrameLoadType, WebCore::FrameLoader::FormSubmissionCacheLoadPolicy) + 358 (FrameLoader.cpp:3159) 23 com.apple.WebCore 0x0000000110b89966 WebCore::FrameLoader::loadItem(WebCore::HistoryItem&, WebCore::FrameLoadType) + 166 (FrameLoader.cpp:3245) 24 com.apple.WebCore 0x0000000110c9ae60 WebCore::HistoryController::recursiveGoToItem(WebCore::HistoryItem&, WebCore::HistoryItem*, WebCore::FrameLoadType) + 96 (HistoryController.cpp:737) 25 com.apple.WebCore 0x0000000110c9ac15 WebCore::HistoryController::goToItem(WebCore::HistoryItem&, WebCore::FrameLoadType) + 405 (HistoryController.cpp:302) 26 com.apple.WebCore 0x00000001118ba319 WebCore::Page::goToItem(WebCore::HistoryItem&, WebCore::FrameLoadType) + 201 (Page.cpp:448) 27 com.apple.WebCore 0x000000011042b3b4 WebCore::BackForwardController::goBackOrForward(int) + 228 (BackForwardController.cpp:78) 28 com.apple.WebCore 0x000000011184e9e5 WebCore::ScheduledHistoryNavigation::fire(WebCore::Frame&) + 293 (NavigationScheduler.cpp:232) 29 com.apple.WebCore 0x000000011184ab1a WebCore::NavigationScheduler::timerFired() + 570 (NavigationScheduler.cpp:486) 30 com.apple.WebCore 0x00000001118511a3 std::__1::__function::__func<std::__1::__bind<void (WebCore::NavigationScheduler::*&)(), WebCore::NavigationScheduler*>, std::__1::allocator<std::__1::__bind<void (WebCore::NavigationScheduler::*&)(), WebCore::NavigationScheduler*> >, void ()>::operator()() + 259 (functional:1370) 31 com.apple.WebCore 0x00000001102c317a std::__1::function<void ()>::operator()() const + 26 (functional:1755) 32 com.apple.WebCore 0x00000001102c312c WebCore::Timer::fired() + 28 (Timer.h:134) 33 com.apple.WebCore 0x00000001121f4e9c WebCore::ThreadTimers::sharedTimerFiredInternal() + 396 (ThreadTimers.cpp:135) 34 com.apple.WebCore 0x00000001121f4b59 WebCore::ThreadTimers::sharedTimerFired() + 25 (ThreadTimers.cpp:108) 35 com.apple.WebCore 0x0000000111398d1a WebCore::timerFired(__CFRunLoopTimer*, void*) + 42 (SharedTimerCF.cpp:83) 36 com.apple.CoreFoundation 0x00007fff8b2a13e4 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20 37 com.apple.CoreFoundation 0x00007fff8b2a0f1f __CFRunLoopDoTimer + 1151 38 com.apple.CoreFoundation 0x00007fff8b3125aa __CFRunLoopDoTimers + 298 39 com.apple.CoreFoundation 0x00007fff8b25c6a5 __CFRunLoopRun + 1525 40 com.apple.CoreFoundation 0x00007fff8b25be75 CFRunLoopRunSpecific + 309 41 com.apple.HIToolbox 0x00007fff85e17a0d RunCurrentEventLoopInMode + 226 42 com.apple.HIToolbox 0x00007fff85e177b7 ReceiveNextEventCommon + 479 43 com.apple.HIToolbox 0x00007fff85e175bc _BlockUntilNextEventMatchingListInModeWithFilter + 65 44 com.apple.AppKit 0x00007fff84c9224e _DPSNextEvent + 1434 45 com.apple.AppKit 0x00007fff84c9189b -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 122 46 com.apple.AppKit 0x00007fff84c8599c -[NSApplication run] + 553 47 com.apple.AppKit 0x00007fff84c70783 NSApplicationMain + 940 48 com.apple.XPCService 0x00007fff90021c0f _xpc_main + 385 49 libxpc.dylib 0x00007fff901e0bde xpc_main + 399 50 com.apple.WebKit.WebContent.Development 0x0000000107393195 main + 37 51 libdyld.dylib 0x00007fff8709d5fd start + 1

Also broke some tests on Windows: https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r183985%20(51686)/results.html

(In reply to comment #22) > Also broke some tests on Windows: > > https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/ > r183985%20(51686)/results.html Yes, those are the 2 new tests my patch is adding. They are timing out on Windows, probably due to DRT limitations as they are passing elsewhere and the code is cross-platform.

(In reply to comment #23) > (In reply to comment #22) > > Also broke some tests on Windows: > > > > https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/ > > r183985%20(51686)/results.html > > Yes, those are the 2 new tests my patch is adding. They are timing out on > Windows, probably due to DRT limitations as they are passing elsewhere and > the code is cross-platform. It is because USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR) is only enabled on COCOA.

Created attachment 252727 [details] Patch

Attachment 252727 [details] did not pass style-queue: ERROR: Source/WebCore/page/FrameView.h:600: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/page/FrameView.cpp:2107: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 252728 [details] Patch

Attachment 252728 [details] did not pass style-queue: ERROR: Source/WebCore/page/FrameView.h:600: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/page/FrameView.cpp:2107: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 252728 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252728&action=review > Source/WebCore/loader/FrameLoader.cpp:2076 > - if (m_frame.view()) > - view->setFrameRect(m_frame.view()->frameRect()); > + Optional<IntRect> previousViewFrameRect = m_frame.view() ? m_frame.view()->frameRect() : Optional<IntRect>(Nullopt); > m_frame.setView(view); > + > + // Use the previous ScrollView's frame rect. > + if (previousViewFrameRect) > + view->setFrameRect(previousViewFrameRect.value()); I'm slightly nervous about behavior changes here. Any code that consulted the view's frameRect inside of setView() is now getting an empty rect (but there doesn't seem to be much). FYI, I added FrameView::isViewForDocumentInFrame() to detect the state of switching views.

Comment on attachment 252728 [details] Patch Clearing flags on attachment: 252728 Committed r183998: <http://trac.webkit.org/changeset/183998>

All reviewed patches have been landed. Closing bug.

*** Bug 165695 has been marked as a duplicate of this bug. ***

*** Bug 100257 has been marked as a duplicate of this bug. ***