Bug 185627 – Pause display links when window is not visible.

• Component: WebKit2
• Status: NEW

[Per Arne Vollan]

Display links running in the UI process will currently fire when the window is hidden.

[Per Arne Vollan]

Created attachment 340361 [details]
Patch

[Per Arne Vollan]

<rdar://problem/39401106>

[Simon Fraser (smfr)]

Comment on attachment 340361 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:1602
> +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400
> +            if (m_displayLink)
> +                m_displayLink->pause();
> +#endif

Should we use this display list machinery on all OSes, not just new ones?

> Source/WebKit/UIProcess/mac/DisplayLink.cpp:96
> +bool DisplayLink::pause()
> +{
> +    if (!CVDisplayLinkIsRunning(m_displayLink))
> +        return true;
> +    return CVDisplayLinkStop(m_displayLink) == kCVReturnSuccess;
> +}
> +
> +bool DisplayLink::resume()
> +{
> +    if (CVDisplayLinkIsRunning(m_displayLink))
> +        return true;
> +    return CVDisplayLinkStart(m_displayLink) == kCVReturnSuccess;
> +}

I don't think there's any point returning bools here. Your call sites ignore them; what is the caller supposed to do? Maybe just add an assertion to catch programmer mistakes.

[Per Arne Vollan]

Created attachment 340436 [details]
Patch

[Per Arne Vollan]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 340361 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340361&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:1602
> > +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400
> > +            if (m_displayLink)
> > +                m_displayLink->pause();
> > +#endif
> 
> Should we use this display list machinery on all OSes, not just new ones?

I was planning to replace __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400 with ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING) in a coming patch. Would that be ok?

> 
> > Source/WebKit/UIProcess/mac/DisplayLink.cpp:96
> > +bool DisplayLink::pause()
> > +{
> > +    if (!CVDisplayLinkIsRunning(m_displayLink))
> > +        return true;
> > +    return CVDisplayLinkStop(m_displayLink) == kCVReturnSuccess;
> > +}
> > +
> > +bool DisplayLink::resume()
> > +{
> > +    if (CVDisplayLinkIsRunning(m_displayLink))
> > +        return true;
> > +    return CVDisplayLinkStart(m_displayLink) == kCVReturnSuccess;
> > +}
> 
> I don't think there's any point returning bools here. Your call sites ignore
> them; what is the caller supposed to do? Maybe just add an assertion to
> catch programmer mistakes.

I removed the bool return type.

Thanks for reviewing, Simon!

[WebKit Commit Bot]

Comment on attachment 340436 [details]
Patch

Clearing flags on attachment: 340436

Committed r231818: <https://trac.webkit.org/changeset/231818>

[EWS Watchlist]

Comment on attachment 340436 [details]
Patch

Attachment 340436 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7692614

New failing tests:
transitions/interrupted-transition-hardware.html

[EWS Watchlist]

Created attachment 340449 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4