Bug 185627 - Pause display links when window is not visible.
Summary: Pause display links when window is not visible.
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-05-14 14:31 PDT by Per Arne Vollan
Modified: 2018-05-15 17:06 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.52 KB, patch)
2018-05-14 14:37 PDT, Per Arne Vollan
simon.fraser: review+
simon.fraser: commit-queue-
Details | Formatted Diff | Diff
Patch (3.40 KB, patch)
2018-05-15 15:24 PDT, Per Arne Vollan
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.19 MB, application/zip)
2018-05-15 17:06 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2018-05-14 14:31:18 PDT
Display links running in the UI process will currently fire when the window is hidden.
Comment 1 Per Arne Vollan 2018-05-14 14:37:04 PDT
Created attachment 340361 [details]
Patch
Comment 2 Per Arne Vollan 2018-05-14 14:40:21 PDT
<rdar://problem/39401106>
Comment 3 Simon Fraser (smfr) 2018-05-15 14:59:13 PDT
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.
Comment 4 Per Arne Vollan 2018-05-15 15:24:19 PDT
Created attachment 340436 [details]
Patch
Comment 5 Per Arne Vollan 2018-05-15 15:26:54 PDT
(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!
Comment 6 WebKit Commit Bot 2018-05-15 16:06:35 PDT
Comment on attachment 340436 [details]
Patch

Clearing flags on attachment: 340436

Committed r231818: <https://trac.webkit.org/changeset/231818>
Comment 7 EWS Watchlist 2018-05-15 17:06:35 PDT
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
Comment 8 EWS Watchlist 2018-05-15 17:06:36 PDT
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