Code review comment for lp:~dandrader/qtmir/bogusRespawn-lp1575577

Revision history for this message
Daniel d'Andrada (dandrader) wrote :

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.

Removed canBeResumed() and updated tests.

« Back to merge proposal