Code review comment for lp:~ted/qtmir/ual-pause

Revision history for this message
Gerry Boland (gerboland) wrote :

Hey Ted, thanks for this! Lots of red, yay! Couple of things I noticed before doing a full review:

Please add checklist: https://wiki.ubuntu.com/Process/Merges/Checklists/QtMir
with the list of required MRs

As you're removing ProcessController, I suspect you can remove qtmir's dependence on process-cpp package.

+++ src/modules/Unity/Application/taskcontroller.h
-/*
- * Copyright (C) 2013-2014 Canonical, Ltd.
+/* Copyright (C) 2013-2014 Canonical, Ltd.
Unnecessary change

+++ src/modules/Unity/Application/upstart/applicationcontroller.cpp
+bool ApplicationController::pauseApplicationWithAppId(const QString& appId)
+ qDebug() << "ApplicationController::pauseApplication FAILED to stop appId=" << appId;
failed to _pause_ appId

+ qDebug() << "ApplicationController::resumeApplication FAILED to stop appId=" << appId;
failed to _resume_

+++ tests/modules/common/mock_application_controller.h
+ (void) appId;
Us Qt people tend to use a macro for this: Q_UNUSED(appId);

And couple of higher-level questions about UAL's behaviour (mostly confirmations for me):
1. UAL changes the OOM-killer score when we pause/resume apps, yes?
2. UAL gives app a unlikely-to-be-killed OOM score on start of that app?

Something that worries me about this change is that apps not managed by upstart will no longer have correct OOM-killer weights. Are trust helpers managed by upstart these days? Does the OSK get a good OOM-killer value? I need to test this carefully.

review: Needs Fixing

« Back to merge proposal