dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed.
Looks good, apart from a few minor things. INLINE COMMENTS > job.cpp:276 > + // Whether or not sub-jobs have PrivilegeExecution flag set > doesn't matter. > + // If its not set in parent job then don't continue. > + return false; its -> it's > job_p.h:197 > + * > + * @since 5.37 > + */ Private header -> no need for @since doc, it's not part of the API. > simplejob.cpp:346 > + bool confirmed = tryAskPrivilegeOpConfirmation(); > + m_slave->send(MSG_PRIVILEGE_EXEC, QByteArray::number(confirmed)); > +} I'm not sure if the bool->int conversion is safe and guaranteed (AFAIK, in theory a bool can convert to 2 or anything else that is not 0, depending on how it was set in the first place) This would be safer if it said QByteArray::number(confirmed ? 1 : 0), but then that's even better written as confirmed ? "1" : "0", no need to call int->string conversion code; REVISION DETAIL https://phabricator.kde.org/D6832 To: chinmoyr, dfaure, #frameworks Cc: #frameworks
