The new remote layer code has largely defeated the existing flush throttling. We should bring it back.
<rdar://problem/17455967>
Created attachment 233991 [details] patch
Created attachment 233997 [details] non-Cocoa build fix
Comment on attachment 233997 [details] non-Cocoa build fix View in context: https://bugs.webkit.org/attachment.cgi?id=233997&action=review > Source/WebCore/page/FrameView.cpp:2257 > + if (frame().page()->chrome().client().usesRemoteLayerTree()) { > + frame().page()->chrome().client().disableLayerFlushThrottlingTemporarilyForInteraction(); Icky knowledge of DrawingArea in FrameView.
FrameView::updateLayerFlushThrottlingInAllFrames() should call FrameView::frameLoadProgressingStatusChanged(Frame*) FrameView::frameLoadProgressingStatusChanged() calls updateLayerFlushThrottlingInAllFrames(); adjustTiledBackingCoverage(); FrameView::updateLayerFlushThrottlingInAllFrames() { bool isMainLoadProgressing = frame().page()->progress().isMainLoadProgressing(); if (frame().page()->chrome().client().adjustLayerFlushThrottling(isMainLoadProgressing)) return; // client handled throttling ... } Maybe have: enum ThottleState { MainLoadProgressing = 1 << 0, UserIsInteracting = 1 << 1 ... }; typedef unsigned ThottleStateFlags; ChromeClient::adjustLayerFlushThrottling(ThottleStateFlags);
Created attachment 234017 [details] updated patch
Comment on attachment 234017 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=234017&action=review > Source/WebCore/loader/FrameLoader.cpp:3413 > void FrameLoader::loadProgressingStatusChanged() > { > - FrameView* view = m_frame.mainFrame().view(); > - if (!view) > - return; > - > - view->updateLayerFlushThrottlingInAllFrames(); > - view->adjustTiledBackingCoverage(); > + if (auto* view = m_frame.mainFrame().view()) > + view->loadProgressingStatusChanged(); > } Could the callers work directly on FrameView instead of calling through FrameLoader? > Source/WebCore/page/FrameView.cpp:2276 > - bool isMainLoadProgressing = frame().page()->progress().isMainLoadProgressing(); > + ASSERT(frame().isMainFrame()); > + > + bool mainLoadProgressing = frame().page()->progress().isMainLoadProgressing(); I think the old name with "is" is better. > Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:259 > + static const auto initialFlushDelay = 500_ms; > + static const auto flushDelay = 1500_ms; I don’t think the “static” here are helpful. > Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:284 > + // Re-schedule the flush if we stopped throttling. > + if (!wasThrottlingLayerFlushes || m_isThrottlingLayerFlushes) > + return true; > + if (!m_layerFlushTimer.isActive()) > + return true; > + m_layerFlushTimer.stop(); > + > + scheduleCompositingLayerFlush(); I think we could rewrite this to match the comment better: if (wasThrottlingLayerFlushes && !m_isThrottlingLayerFlushes && m_layerFlushTimer.isActive()) { m_layerFlushTimer.stop(); scheduleCompositingLayerFlush(); } Seems clearer to me than the early return version.
Comment on attachment 234017 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=234017&action=review > Source/WebCore/page/FrameView.cpp:2261 > + bool mainLoadProgressing = frame().page()->progress().isMainLoadProgressing(); > + > + LayerFlushThrottleState::Flags flags = LayerFlushThrottleState::UserIsInteracting | (mainLoadProgressing ? LayerFlushThrottleState::MainLoadProgressing : 0); > + if (frame().page()->chrome().client().adjustLayerFlushThrottling(flags)) > + return; > + Would be nice if this could just call updateLayerFlushThrottling(LayerFlushThrottleState::UserIsInteracting) at some point.
> Could the callers work directly on FrameView instead of calling through FrameLoader? I don't want to make ProgressTracker know about views.
https://trac.webkit.org/r170557