Summary: | Pause display links when window is not visible. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Per Arne Vollan <pvollan> | ||||||||
Component: | WebKit2 | Assignee: | Per Arne Vollan <pvollan> | ||||||||
Status: | NEW --- | ||||||||||
Severity: | Normal | CC: | bfulgham, commit-queue, ews-watchlist, simon.fraser, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Per Arne Vollan
2018-05-14 14:31:18 PDT
Created attachment 340361 [details]
Patch
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. Created attachment 340436 [details]
Patch
(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 on attachment 340436 [details] Patch Clearing flags on attachment: 340436 Committed r231818: <https://trac.webkit.org/changeset/231818> 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 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
|