On 03/05/2016 11:44, Gerry Boland wrote:
> Review: Needs Fixing
>
> + switch(m_state) {
> Missing space between "switch" and the bracket.
Fixed.
>
> + /* killed by upstart while in background. Keep it in the window list
> upstart doesn't kill apps in background, the kernel OOM killer does.
Fixed.
> + /* we're not able to respawn this application */
> I think it's a good idea to explain some reasons why, e.g. "not managed by upstart (launched via cmd line by user)"
Done.
> +void Application::onSessionStopped()
> name sounds like a slot. Not a complaint, just initial thought on reading the code.
Suggestions?
> Nothing (except existing tests) is using the canBeResumed() method now. Please either use it in this code, or remove it completely. I would lean toward keeping it, as it interprets the meaning of "processState == ProcessUnknown" slightly more clearly in the code. But I'll not argue with your decision.
On 03/05/2016 11:44, Gerry Boland wrote:
> Review: Needs Fixing
>
> + switch(m_state) {
> Missing space between "switch" and the bracket.
Fixed.
>
> + /* killed by upstart while in background. Keep it in the window list
> upstart doesn't kill apps in background, the kernel OOM killer does.
Fixed.
> + /* we're not able to respawn this application */
> I think it's a good idea to explain some reasons why, e.g. "not managed by upstart (launched via cmd line by user)"
Done.
> +void Application: :onSessionStopp ed()
> name sounds like a slot. Not a complaint, just initial thought on reading the code.
Suggestions?
> Nothing (except existing tests) is using the canBeResumed() method now. Please either use it in this code, or remove it completely. I would lean toward keeping it, as it interprets the meaning of "processState == ProcessUnknown" slightly more clearly in the code. But I'll not argue with your decision.
Removed canBeResumed() and updated tests.