Merge lp:~unity-team/qtmir/fix-wakelocks into lp:qtmir

Proposed by Michał Sawicz on 2015-12-07
Status: Merged
Approved by: Michał Sawicz on 2015-12-09
Approved revision: 427
Merged at revision: 427
Proposed branch: lp:~unity-team/qtmir/fix-wakelocks
Merge into: lp:qtmir
Prerequisite: lp:~unity-team/qtmir/multiSurfaceApp
Diff against target: 314 lines (+155/-3)
6 files modified
src/modules/Unity/Application/application.cpp (+37/-2)
src/modules/Unity/Application/application.h (+4/-0)
src/modules/Unity/Application/application_manager.cpp (+2/-0)
src/modules/Unity/Application/application_manager.h (+1/-1)
tests/modules/ApplicationManager/application_manager_test.cpp (+108/-0)
tests/modules/common/qtmir_test.cpp (+3/-0)
To merge this branch: bzr merge lp:~unity-team/qtmir/fix-wakelocks
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration 2015-12-07 Needs Fixing on 2015-12-07
Daniel d'Andrada 2015-12-07 Pending
Review via email: mp+279762@code.launchpad.net

This proposal supersedes a proposal from 2015-12-03.

Commit message

Don't hold a wakelock on apps that are exempt from lifecycle management.

This supports the new exemptFromLifecycle flag for applications as well as bringing back the internal RunningInBackground state.

The tests were revived from the same branch that removed them when we removed RunningInBackground (revision 400)

Description of the change

Don't hold a wakelock on apps that are exempt from lifecycle management.

This supports the new exemptFromLifecycle flag for applications as well as bringing back the internal RunningInBackground state.

The tests were revived from the same branch that removed them when we removed RunningInBackground [1] (and in hindsight, I should have thought harder about the implications of simply removing a test that confirmed we were releasing wakelocks).

This branch is related to unity-api [2] and unity8 [3] MPS. All of which are in service of fixing bug 1518764.

[1] https://code.launchpad.net/~mterry/qtmir/no-touch-no-lifecycle/+merge/272791

[2]https://code.launchpad.net/~mterry/unity-api/fix-wakelocks/+merge/279499
[3] https://code.launchpad.net/~mterry/unity8/fix-wakelocks/+merge/279502

To post a comment you must log in.
Daniel d'Andrada (dandrader) wrote : Posted in a previous version of this proposal

In Application::setInternalState(Application::InternalState state):

"""
        case InternalState::RunningInBackground:
            releaseWakelock();
            break;
"""

could be simply:

"""
        case InternalState::RunningInBackground:

"""

as the next case does the same.

Michael Terry (mterry) wrote : Posted in a previous version of this proposal

> In Application::setInternalState(Application::InternalState state):
> [snip]

True. That whole switch statement could use a lot of cleanup as there are several duplicate entries. But it looks like an intentional choice on Gerry's part, and this change in this branch is really just a revert to bring this code back to what it looked like.

I can clean up if you really want it more slim. But I'd prefer to either (a) clean up the whole switch or (b) leave this code as it was. With a preference for (b).

Daniel d'Andrada (dandrader) wrote : Posted in a previous version of this proposal

On 03/12/2015 18:22, Michael Terry wrote:
>> In Application::setInternalState(Application::InternalState state):
>> [snip]
> True. That whole switch statement could use a lot of cleanup as there are several duplicate entries. But it looks like an intentional choice on Gerry's part, and this change in this branch is really just a revert to bring this code back to what it looked like.
>

I wrote that code, not Gerry.

What I do want to keep is the order in which the enum items appear in
the switch: They should be the same as in the enum declaration.

And there's already a precedent for that: the Starting case and the
SuspendingWait* ones.

Michael Terry (mterry) wrote : Posted in a previous version of this proposal

> I wrote that code, not Gerry.

Whoops, sorry!

> What I do want to keep is the order in which the enum items appear in
> the switch: They should be the same as in the enum declaration.
>
> And there's already a precedent for that: the Starting case and the
> SuspendingWait* ones.

That switch isn't in enum declaration order. But I gotcha. I condensed the new switch statement.

Daniel d'Andrada (dandrader) wrote : Posted in a previous version of this proposal

Code looks ok. Didn't test yet

review: Approve (code)
Daniel d'Andrada (dandrader) wrote : Posted in a previous version of this proposal

And fixes the bug

review: Approve
Michał Sawicz (saviq) wrote :

Approving per superseded.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/modules/Unity/Application/application.cpp'
2--- src/modules/Unity/Application/application.cpp 2015-12-07 12:24:10 +0000
3+++ src/modules/Unity/Application/application.cpp 2015-12-07 12:24:10 +0000
4@@ -53,6 +53,7 @@
5 , m_requestedState(RequestedRunning)
6 , m_processState(ProcessUnknown)
7 , m_closeTimer(0)
8+ , m_exemptFromLifecycle(false)
9 {
10 qCDebug(QTMIR_APPLICATIONS) << "Application::Application - appId=" << desktopFileReader->appId();
11
12@@ -79,6 +80,7 @@
13 switch (m_state) {
14 case InternalState::Starting:
15 case InternalState::Running:
16+ case InternalState::RunningInBackground:
17 case InternalState::SuspendingWaitSession:
18 case InternalState::SuspendingWaitProcess:
19 wipeQMLCache();
20@@ -210,6 +212,8 @@
21 return "Starting";
22 case InternalState::Running:
23 return "Running";
24+ case InternalState::RunningInBackground:
25+ return "RunningInBackground";
26 case InternalState::SuspendingWaitSession:
27 return "SuspendingWaitSession";
28 case InternalState::SuspendingWaitProcess:
29@@ -278,6 +282,7 @@
30 case InternalState::Starting:
31 return Starting;
32 case InternalState::Running:
33+ case InternalState::RunningInBackground:
34 case InternalState::SuspendingWaitSession:
35 case InternalState::SuspendingWaitProcess:
36 case InternalState::Closing:
37@@ -328,6 +333,7 @@
38 case InternalState::Running:
39 // already where it's wanted to be
40 break;
41+ case InternalState::RunningInBackground:
42 case InternalState::SuspendingWaitSession:
43 case InternalState::Suspended:
44 resume();
45@@ -360,6 +366,7 @@
46 Q_ASSERT(m_processState == ProcessUnknown);
47 }
48 break;
49+ case InternalState::RunningInBackground:
50 case InternalState::SuspendingWaitSession:
51 case InternalState::SuspendingWaitProcess:
52 case InternalState::Suspended:
53@@ -406,6 +413,7 @@
54 case InternalState::Running:
55 doClose();
56 break;
57+ case InternalState::RunningInBackground:
58 case InternalState::SuspendingWaitSession:
59 case InternalState::SuspendingWaitProcess:
60 case InternalState::Suspended:
61@@ -466,6 +474,7 @@
62 switch (m_state) {
63 case InternalState::Starting:
64 case InternalState::Running:
65+ case InternalState::RunningInBackground:
66 case InternalState::Closing:
67 m_session->resume();
68 break;
69@@ -524,6 +533,7 @@
70 case InternalState::Running:
71 acquireWakelock();
72 break;
73+ case InternalState::RunningInBackground:
74 case InternalState::Suspended:
75 releaseWakelock();
76 break;
77@@ -622,8 +632,15 @@
78 Q_ASSERT(m_state == InternalState::Running);
79 Q_ASSERT(m_session != nullptr);
80
81- setInternalState(InternalState::SuspendingWaitSession);
82- m_session->suspend();
83+ if (exemptFromLifecycle()) {
84+ // There's no need to keep the wakelock as the process is never suspended
85+ // and thus has no cleanup to perform when (for example) the display is
86+ // blanked.
87+ setInternalState(InternalState::RunningInBackground);
88+ } else {
89+ setInternalState(InternalState::SuspendingWaitSession);
90+ m_session->suspend();
91+ }
92 }
93
94 void Application::resume()
95@@ -640,6 +657,8 @@
96 } else if (m_state == InternalState::SuspendingWaitSession) {
97 setInternalState(InternalState::Running);
98 m_session->resume();
99+ } else if (m_state == InternalState::RunningInBackground) {
100+ setInternalState(InternalState::Running);
101 }
102 }
103
104@@ -672,6 +691,22 @@
105 return m_desktopData->isTouchApp();
106 }
107
108+bool Application::exemptFromLifecycle() const
109+{
110+ return m_exemptFromLifecycle;
111+}
112+
113+void Application::setExemptFromLifecycle(bool exemptFromLifecycle)
114+{
115+ if (m_exemptFromLifecycle != exemptFromLifecycle)
116+ {
117+ // We don't adjust current suspension state, we only care about exempt
118+ // status going into a suspend.
119+ m_exemptFromLifecycle = exemptFromLifecycle;
120+ Q_EMIT exemptFromLifecycleChanged(m_exemptFromLifecycle);
121+ }
122+}
123+
124 QString Application::longAppId() const
125 {
126 return m_longAppId;
127
128=== modified file 'src/modules/Unity/Application/application.h'
129--- src/modules/Unity/Application/application.h 2015-12-07 12:24:10 +0000
130+++ src/modules/Unity/Application/application.h 2015-12-07 12:24:10 +0000
131@@ -68,6 +68,7 @@
132 enum class InternalState {
133 Starting,
134 Running,
135+ RunningInBackground,
136 SuspendingWaitSession,
137 SuspendingWaitProcess,
138 Suspended,
139@@ -104,6 +105,8 @@
140 Qt::ScreenOrientations supportedOrientations() const override;
141 bool rotatesWindowContents() const override;
142 bool isTouchApp() const override;
143+ bool exemptFromLifecycle() const override;
144+ void setExemptFromLifecycle(bool) override;
145
146 void setStage(Stage stage);
147
148@@ -185,6 +188,7 @@
149 RequestedState m_requestedState;
150 ProcessState m_processState;
151 int m_closeTimer;
152+ bool m_exemptFromLifecycle;
153
154 friend class ApplicationManager;
155 friend class SessionManager;
156
157=== modified file 'src/modules/Unity/Application/application_manager.cpp'
158--- src/modules/Unity/Application/application_manager.cpp 2015-12-07 12:24:10 +0000
159+++ src/modules/Unity/Application/application_manager.cpp 2015-12-07 12:24:10 +0000
160@@ -229,6 +229,8 @@
161 return QVariant::fromValue(application->focused());
162 case RoleIsTouchApp:
163 return QVariant::fromValue(application->isTouchApp());
164+ case RoleExemptFromLifecycle:
165+ return QVariant::fromValue(application->exemptFromLifecycle());
166 case RoleSession:
167 return QVariant::fromValue(application->session());
168 case RoleFullscreen:
169
170=== modified file 'src/modules/Unity/Application/application_manager.h'
171--- src/modules/Unity/Application/application_manager.h 2015-12-07 12:24:10 +0000
172+++ src/modules/Unity/Application/application_manager.h 2015-12-07 12:24:10 +0000
173@@ -68,7 +68,7 @@
174
175 // FIXME: these roles should be added to unity-api and removed from here
176 enum MoreRoles {
177- RoleSession = RoleIsTouchApp+1,
178+ RoleSession = RoleExemptFromLifecycle+1,
179 RoleFullscreen,
180 };
181
182
183=== modified file 'tests/modules/ApplicationManager/application_manager_test.cpp'
184--- tests/modules/ApplicationManager/application_manager_test.cpp 2015-12-07 12:24:10 +0000
185+++ tests/modules/ApplicationManager/application_manager_test.cpp 2015-12-07 12:24:10 +0000
186@@ -1737,6 +1737,114 @@
187 }
188 }
189
190+TEST_F(ApplicationManagerTests, lifecycleExemptAppIsNotSuspended)
191+{
192+ using namespace ::testing;
193+
194+ const QString appId("testAppId");
195+ const quint64 procId = 12345;
196+
197+ // Set up Mocks & signal watcher
198+ auto mockDesktopFileReader = new NiceMock<MockDesktopFileReader>(appId, QFileInfo());
199+ ON_CALL(*mockDesktopFileReader, loaded()).WillByDefault(Return(true));
200+ ON_CALL(*mockDesktopFileReader, appId()).WillByDefault(Return(appId));
201+
202+ ON_CALL(desktopFileReaderFactory, createInstance(appId, _)).WillByDefault(Return(mockDesktopFileReader));
203+
204+ EXPECT_CALL(appController, startApplicationWithAppIdAndArgs(appId, _))
205+ .Times(1)
206+ .WillOnce(Return(true));
207+
208+ auto the_app = applicationManager.startApplication(appId, ApplicationManager::NoFlag);
209+ applicationManager.onProcessStarting(appId);
210+ std::shared_ptr<mir::scene::Session> session = std::make_shared<MockSession>("", procId);
211+ bool authed = true;
212+ applicationManager.authorizeSession(procId, authed);
213+ onSessionStarting(session);
214+
215+ // App creates surface, focuses it so state is running
216+ FakeMirSurface *surface = new FakeMirSurface;
217+ onSessionCreatedSurface(session.get(), surface);
218+ surface->drawFirstFrame();
219+
220+ // Test normal lifecycle management as a control group
221+ ASSERT_EQ(Application::InternalState::Running, the_app->internalState());
222+ ASSERT_EQ(Application::ProcessState::ProcessRunning, the_app->processState());
223+
224+ EXPECT_CALL(*(mir::scene::MockSession*)session.get(), set_lifecycle_state(mir_lifecycle_state_will_suspend));
225+ the_app->setRequestedState(Application::RequestedSuspended);
226+ ASSERT_EQ(Application::InternalState::SuspendingWaitSession, the_app->internalState());
227+
228+ static_cast<qtmir::Session*>(the_app->session())->doSuspend();
229+ ASSERT_EQ(Application::InternalState::SuspendingWaitProcess, the_app->internalState());
230+ applicationManager.onProcessSuspended(the_app->appId());
231+ ASSERT_EQ(Application::InternalState::Suspended, the_app->internalState());
232+
233+ EXPECT_CALL(*(mir::scene::MockSession*)session.get(), set_lifecycle_state(mir_lifecycle_state_resumed));
234+ the_app->setRequestedState(Application::RequestedRunning);
235+
236+ EXPECT_EQ(Application::Running, the_app->state());
237+
238+ // Now mark the app as exempt from lifecycle management and retest
239+ the_app->setExemptFromLifecycle(true);
240+
241+ EXPECT_EQ(Application::Running, the_app->state());
242+
243+ EXPECT_CALL(*(mir::scene::MockSession*)session.get(), set_lifecycle_state(_)).Times(0);
244+ the_app->setRequestedState(Application::RequestedSuspended);
245+
246+ // And expect it to be running still
247+ ASSERT_EQ(Application::InternalState::RunningInBackground, the_app->internalState());
248+
249+ the_app->setRequestedState(Application::RequestedRunning);
250+
251+ EXPECT_EQ(Application::Running, the_app->state());
252+ ASSERT_EQ(Application::InternalState::Running, the_app->internalState());
253+}
254+
255+/*
256+ * Test lifecycle exempt applications have their wakelocks released when shell tries to suspend them
257+ */
258+TEST_F(ApplicationManagerTests, lifecycleExemptAppHasWakelockReleasedOnAttemptedSuspend)
259+{
260+ using namespace ::testing;
261+
262+ const QString appId("testAppId");
263+ const quint64 procId = 12345;
264+
265+ // Set up Mocks & signal watcher
266+ auto mockDesktopFileReader = new NiceMock<MockDesktopFileReader>(appId, QFileInfo());
267+ ON_CALL(*mockDesktopFileReader, loaded()).WillByDefault(Return(true));
268+ ON_CALL(*mockDesktopFileReader, appId()).WillByDefault(Return(appId));
269+
270+ ON_CALL(desktopFileReaderFactory, createInstance(appId, _)).WillByDefault(Return(mockDesktopFileReader));
271+
272+ EXPECT_CALL(appController, startApplicationWithAppIdAndArgs(appId, _))
273+ .Times(1)
274+ .WillOnce(Return(true));
275+
276+ auto application = applicationManager.startApplication(appId, ApplicationManager::NoFlag);
277+ applicationManager.onProcessStarting(appId);
278+ std::shared_ptr<mir::scene::Session> session = std::make_shared<MockSession>("", procId);
279+ bool authed = true;
280+ applicationManager.authorizeSession(procId, authed);
281+ onSessionStarting(session);
282+
283+ // App creates surface, focuses it so state is running
284+ FakeMirSurface *surface = new FakeMirSurface;
285+ onSessionCreatedSurface(session.get(), surface);
286+ surface->drawFirstFrame();
287+
288+ // Mark app as exempt
289+ application->setExemptFromLifecycle(true);
290+
291+ application->setRequestedState(Application::RequestedSuspended);
292+
293+ EXPECT_FALSE(sharedWakelock.enabled());
294+ ASSERT_EQ(Application::InternalState::RunningInBackground, application->internalState());
295+ EXPECT_EQ(Application::Running, application->state());
296+}
297+
298 /*
299 * Test that when user stops an application, application does not delete QML cache
300 */
301
302=== modified file 'tests/modules/common/qtmir_test.cpp'
303--- tests/modules/common/qtmir_test.cpp 2015-10-01 16:48:38 +0000
304+++ tests/modules/common/qtmir_test.cpp 2015-12-07 12:24:10 +0000
305@@ -26,6 +26,9 @@
306 case Application::InternalState::Running:
307 *os << "Running";
308 break;
309+ case Application::InternalState::RunningInBackground:
310+ *os << "RunningInBackground";
311+ break;
312 case Application::InternalState::SuspendingWaitSession:
313 *os << "SuspendingWaitSession";
314 break;

Subscribers

People subscribed via source and target branches