Bug 134398

Summary: Flush throttling with remote layers
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bunhere, cdumez, commit-queue, esprehn+autocc, glenn, gyuyoung.kim, japhet, kondapallykalyan, sergio, simon.fraser, thorton
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
non-Cocoa build fix
simon.fraser: review-
updated patch darin: review+

Description Antti Koivisto 2014-06-27 09:34:31 PDT
The new remote layer code has largely defeated the existing flush throttling. We should bring it back.
Comment 1 Antti Koivisto 2014-06-27 09:34:53 PDT
<rdar://problem/17455967>
Comment 2 Antti Koivisto 2014-06-27 09:59:36 PDT
Created attachment 233991 [details]
patch
Comment 3 Antti Koivisto 2014-06-27 10:49:25 PDT
Created attachment 233997 [details]
non-Cocoa build fix
Comment 4 Simon Fraser (smfr) 2014-06-27 11:16:19 PDT
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.
Comment 5 Simon Fraser (smfr) 2014-06-27 11:16:29 PDT
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);
Comment 6 Antti Koivisto 2014-06-27 14:25:32 PDT
Created attachment 234017 [details]
updated patch
Comment 7 Darin Adler 2014-06-27 14:31:52 PDT
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 8 Simon Fraser (smfr) 2014-06-27 14:40:08 PDT
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.
Comment 9 Antti Koivisto 2014-06-27 14:47:03 PDT
> Could the callers work directly on FrameView instead of calling through FrameLoader?

I don't want to make ProgressTracker know about views.
Comment 10 Antti Koivisto 2014-06-27 14:59:02 PDT
https://trac.webkit.org/r170557