From f73f651f7c9c4de6509af4ed3f9ccebcd54a0dac Mon Sep 17 00:00:00 2001 From: toofar Date: Sat, 24 May 2025 18:41:17 +1200 Subject: [PATCH] Switch to two lifecycle state timers The previous implementation of the lifecycle timer had one timer that we would change based on what state were were heading into. Because we connected a new function to it every time we had to disconnect previous connections before using it again which was a bit awkward, it raised a TypeError if there were no connections, which isn't very specific. This commit is an attempt to switch to using two timers, each with their own callback. I took a stateless/idempotent approach where the logic doesn't know what state it might have been in previously so needs to stop any timers that might be set each time, and needs to check if the timer it wants to set is already active. Potentially the line count could be reduced by encoding a bit more state in the logic. But hopefully it's simpler to maintain like this. Since I had need to (potentially) stop the timers at several points in the logic, I pulled that out to a helper function which is where most of the complexity is now. I tried to make the helper function a little more dynamic by using a dict lookup to get the new timer and a loop to reactive the old one. This didn't really add much, I think it's a slightly higher line count, and probably a little harder to parse. The previous implementation just looked a bit repetitive and hard to scan over: to_start = delay = None if state == QWebEnginePage.LifecycleState.Frozen: to_start = self._lifecycle_timer_freeze self._lifecycle_timer_discard.stop() delay = config.val.qt.chromium.lifecycle_state_freeze_delay elif state == QWebEnginePage.LifecycleState.Discarded: self._lifecycle_timer_freeze.stop() to_start = self._lifecycle_timer_discard delay = config.val.qt.chromium.lifecycle_state_discard_delay elif state == None: self._lifecycle_timer_freeze.stop() self._lifecycle_timer_discard.stop() else: raise utils.Unreachable(recommended_state) if to_start and not to_start.isActive() and delay != -1: log.webview.debug(f"Scheduling recommended lifecycle change {delay=} {state=} tab={self}") to_start.start(delay) I re-worked the logic of the `_on_recommended_state_changed()` changed function a little. Not moving into the `Active` state is done immediately instead of via a 0ms timer. So it's more like an guard clause and early exit. I think it flows a little better now with all the decisions layed out clearly and acted on one at a time instead of having the disabled check right at the start but still proceeding through the function in one case. --- qutebrowser/browser/webengine/webenginetab.py | 87 ++++++++++++------- .../browser/webengine/test_webenginetab.py | 37 +++++--- 2 files changed, 81 insertions(+), 43 deletions(-) diff --git a/qutebrowser/browser/webengine/webenginetab.py b/qutebrowser/browser/webengine/webenginetab.py index 285d442a1..078867404 100644 --- a/qutebrowser/browser/webengine/webenginetab.py +++ b/qutebrowser/browser/webengine/webenginetab.py @@ -1316,8 +1316,14 @@ class WebEngineTab(browsertab.AbstractTab): self._child_event_filter = None self._saved_zoom = None self._scripts.init() - self._lifecycle_timer = usertypes.Timer(self) - self._lifecycle_timer.setSingleShot(True) + + self._lifecycle_timer_freeze = usertypes.Timer(self) + self._lifecycle_timer_freeze.setSingleShot(True) + self._lifecycle_timer_freeze.timeout.connect(functools.partial(self._set_lifecycle_state, QWebEnginePage.LifecycleState.Frozen)) + self._lifecycle_timer_discard = usertypes.Timer(self) + self._lifecycle_timer_discard.setSingleShot(True) + self._lifecycle_timer_discard.timeout.connect(functools.partial(self._set_lifecycle_state, QWebEnginePage.LifecycleState.Discarded)) + # WORKAROUND for https://bugreports.qt.io/browse/QTBUG-65223 self._needs_qtbug65223_workaround = ( version.qtwebengine_versions().webengine < utils.VersionNumber(5, 15, 5)) @@ -1734,44 +1740,61 @@ class WebEngineTab(browsertab.AbstractTab): else: selection.selectNone() + def _schedule_lifecycle_transition( + self, + state: Optional[QWebEnginePage.LifecycleState] = None, + ) -> None: + """Schedule, or cancel, a page lifecycle transition. + + Schedule a lifecycle transition to `state`, according to the user's + config. + If a transition into `state` is already schedule, do nothing. + If `state` is `None`, cancel any scheduled transition. + """ + timers = { + QWebEnginePage.LifecycleState.Frozen: ( + self._lifecycle_timer_freeze, + config.val.qt.chromium.lifecycle_state_freeze_delay, + ), + QWebEnginePage.LifecycleState.Discarded: ( + self._lifecycle_timer_discard, + config.val.qt.chromium.lifecycle_state_discard_delay, + ), + } + + to_start = delay = None + if state is not None: + try: + to_start, delay = timers[state] + except KeyError: + raise utils.Unreachable(state) + + for timer, _ in timers.values(): + if timer != to_start: + timer.stop() + + if to_start and not to_start.isActive() and delay != -1: + log.webview.debug(f"Scheduling recommended lifecycle change {delay=} {state=} tab={self}") + to_start.start(delay) + @pyqtSlot(QWebEnginePage.LifecycleState) def _on_recommended_state_changed( self, recommended_state: QWebEnginePage.LifecycleState, ) -> None: + if self._widget.page().lifecycleState() == recommended_state: + self._schedule_lifecycle_transition(None) + return + disabled = not config.val.qt.chromium.use_recommended_page_lifecycle_state - # If the config is changed at runtime, stop freezing/discarding pages, but do - # recover pages that become active again. - if disabled and recommended_state != QWebEnginePage.LifecycleState.Active: - return - - if recommended_state == QWebEnginePage.LifecycleState.Frozen: - delay = config.val.qt.chromium.lifecycle_state_freeze_delay - elif recommended_state == QWebEnginePage.LifecycleState.Discarded: - delay = config.val.qt.chromium.lifecycle_state_discard_delay - elif recommended_state == QWebEnginePage.LifecycleState.Active: - delay = 0 + if recommended_state == QWebEnginePage.LifecycleState.Active: + self._schedule_lifecycle_transition(None) + self._set_lifecycle_state(recommended_state) + elif disabled or self.data.pinned: + self._schedule_lifecycle_transition(None) else: - raise utils.Unreachable(recommended_state) - - try: - self._lifecycle_timer.timeout.disconnect() - except TypeError: - pass - - if self._widget.page().lifecycleState() == recommended_state: - return - - if delay < 0: - return - - if self.data.pinned and recommended_state != QWebEnginePage.LifecycleState.Active: - return - - log.webview.debug(f"Scheduling recommended lifecycle change {delay=} {recommended_state=} tab={self}") - self._lifecycle_timer.timeout.connect(lambda: self._set_lifecycle_state(recommended_state)) - self._lifecycle_timer.start(delay) + self._schedule_lifecycle_transition(recommended_state) def _connect_signals(self): view = self._widget diff --git a/tests/unit/browser/webengine/test_webenginetab.py b/tests/unit/browser/webengine/test_webenginetab.py index c0d7393b8..b19693d50 100644 --- a/tests/unit/browser/webengine/test_webenginetab.py +++ b/tests/unit/browser/webengine/test_webenginetab.py @@ -292,6 +292,14 @@ class TestPageLifecycle: config_stub.val.qt.chromium.lifecycle_state_discard_delay = discard_delay config_stub.val.qt.chromium.use_recommended_page_lifecycle_state = enabled + def timer_for(self, tab, state): # pylint: disable=inconsistent-return-statements + if state == QWebEnginePage.LifecycleState.Frozen: + return tab._lifecycle_timer_freeze + elif state == QWebEnginePage.LifecycleState.Discarded: + return tab._lifecycle_timer_discard + else: + pytest.fail(f"Unknown lifecycle state `{state}`") + def test_qt_method_is_called( self, webengine_tab: webenginetab.WebEngineTab, @@ -299,8 +307,9 @@ class TestPageLifecycle: qtbot, ): """Basic test to show that we call QT after going through our code.""" - webengine_tab._on_recommended_state_changed(QWebEnginePage.LifecycleState.Discarded) - with qtbot.wait_signal(webengine_tab._lifecycle_timer.timeout): + state = QWebEnginePage.LifecycleState.Discarded + webengine_tab._on_recommended_state_changed(state) + with qtbot.wait_signal(self.timer_for(webengine_tab, state).timeout): pass set_state_mock.assert_called_once_with(QWebEnginePage.LifecycleState.Discarded) @@ -332,7 +341,7 @@ class TestPageLifecycle: webengine_tab._on_recommended_state_changed(new_state) - timer = webengine_tab._lifecycle_timer + timer = self.timer_for(webengine_tab, new_state) assert timer.remainingTime() == ( freeze_delay if new_state == QWebEnginePage.LifecycleState.Frozen @@ -354,8 +363,10 @@ class TestPageLifecycle: config_stub, discard_delay=-1, ) - webengine_tab._on_recommended_state_changed(QWebEnginePage.LifecycleState.Discarded) - assert not webengine_tab._lifecycle_timer.isActive() + state = QWebEnginePage.LifecycleState.Discarded + webengine_tab._on_recommended_state_changed(state) + timer = self.timer_for(webengine_tab, state) + assert not timer.isActive() def test_pinned_tabs_untouched( self, @@ -365,8 +376,10 @@ class TestPageLifecycle: ): """Don't change lifecycle state for a pinned tab.""" webengine_tab.set_pinned(True) - webengine_tab._on_recommended_state_changed(QWebEnginePage.LifecycleState.Frozen) - assert not webengine_tab._lifecycle_timer.isActive() + state = QWebEnginePage.LifecycleState.Frozen + webengine_tab._on_recommended_state_changed(state) + timer = self.timer_for(webengine_tab, state) + assert not timer.isActive() def test_timer_interrupted( self, @@ -381,13 +394,15 @@ class TestPageLifecycle: freeze_delay=1, discard_delay=3, ) - timer = webengine_tab._lifecycle_timer + freeze_timer = webengine_tab._lifecycle_timer_freeze + discard_timer = webengine_tab._lifecycle_timer_discard + webengine_tab._on_recommended_state_changed(QWebEnginePage.LifecycleState.Frozen) - assert timer.remainingTime() == 1 + assert freeze_timer.remainingTime() == 1 webengine_tab._on_recommended_state_changed(QWebEnginePage.LifecycleState.Discarded) - assert timer.remainingTime() == 3 + assert discard_timer.remainingTime() == 3 - with qtbot.wait_signal(webengine_tab._lifecycle_timer.timeout): + with qtbot.wait_signal(discard_timer.timeout): pass set_state_mock.assert_called_once_with(QWebEnginePage.LifecycleState.Discarded)