----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104337/#review11596 -----------------------------------------------------------
Nice, thanks and sorry for the noise, and thanks for making the branch. - Stephen Kelly On March 18, 2012, 10:25 p.m., Dario Freddi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/104337/ > ----------------------------------------------------------- > > (Updated March 18, 2012, 10:25 p.m.) > > > Review request for kdelibs, Kevin Ottens, David Faure, and Alexander Neundorf. > > > Description > ------- > > Preamble - sorry for having to name-call people but apparently we still don't > have a frameworks way for reviewing code (which sucks). And sorry for the > long summary, but it's worth reading. However. > > This huge patchsets brings KAuth in the marvelous world of Frameworks. If you > dislike ReviewBoard's way of displaying diffs or simply want to see a commit > list, please refer to the URL in "Branch". > > First of all, I pulled in a dependency on KJob after a chat with Kevin. This > makes KAuth tier2, but shouldn't be a big issue. > > Then there's the hard part: source compatibility is reasonably broken here. > The changes I had to do were mostly for the sake of revamping the internal > workflow of the library. The main problem KAuth had was the fact it was > completely synchronous, leading to a multitude of problems. After these > changes it's fully asynchronous instead (reason for pulling in KJob), the API > was simplified, and some unused features like multiple action execution have > been removed. > > The main changes at a glance: > > * Some renaming to the enums > * Moving Action & ActionReply to be implicitly shared > * Removing ActionWatcher (now useless due to the new semantics of execute > * Removing some useless APIs from Action, namely executeActions, > execute(helper) > * execute() now returns a KJob > * helperID() -> helperId() > * Static action replies are now static accessors returning a new instance. > This was a complete mistake in the first place, but it's still there with a > different semantic to ease porting. The main use case for changing this is a > failure to handle implicitly shared classes in multithreaded environments > with that approach. > > Of course, while it would be awesome to have all the code reviewed, I > understand it's a very big change so I'd like at least some feedback on the > following points: > > * General sanity of the new API > * Consistency of the enums. StatusInvalid vs. ExecuteMode vs. > AuthorizationDeniedError. While the semantic seems correct to me, I'd like to > have some feedback on whether consistency is valuable in the ordering of > <type><value> vs. <value><type> and which one should be preferred in case. > * Whether to deprecate static accessors such as static const ActionReply > SuccessReply(). I strongly favor this. > * Whether the new dependency of kcoreaddons for the sake of using KJob is > reasonable or I should go for a different alternative. > * CMake sanity for the new dependency of kcoreaddons. > > The code is pretty much unit-tested and it should have a decent coverage, > even if I had no way to check this. For unit tests, I had to create a fake > authorization backend for testing purposes, whereas I managed to reuse the > dbus backend for helper communication, so that I could even test that. For > running the helper and the client in the same process, in the unit test I am > resorting to making the dbus service of the helper live in a separate thread, > to prevent asynchronous DBus calls from failing due to QDBus' local-loop > optimization. The test is also run on the session bus. > > > Diffs > ----- > > staging/kauth/CMakeLists.txt PRE-CREATION > staging/kauth/autotests/BackendsManager.h PRE-CREATION > staging/kauth/autotests/BackendsManager.cpp PRE-CREATION > staging/kauth/autotests/CMakeLists.txt PRE-CREATION > staging/kauth/autotests/HelperTest.cpp PRE-CREATION > staging/kauth/autotests/SetupActionTest.cpp PRE-CREATION > staging/kauth/autotests/TestBackend.h PRE-CREATION > staging/kauth/autotests/TestBackend.cpp PRE-CREATION > staging/kauth/autotests/TestHelper.h PRE-CREATION > staging/kauth/autotests/TestHelper.cpp PRE-CREATION > staging/kauth/src/AuthBackend.h PRE-CREATION > staging/kauth/src/CMakeLists.txt PRE-CREATION > staging/kauth/src/HelperProxy.h PRE-CREATION > staging/kauth/src/backends/dbus/DBusHelperProxy.h PRE-CREATION > staging/kauth/src/backends/dbus/DBusHelperProxy.cpp PRE-CREATION > staging/kauth/src/backends/dbus/org.kde.auth.xml PRE-CREATION > staging/kauth/src/backends/fake/FakeBackend.cpp PRE-CREATION > staging/kauth/src/backends/fakehelper/FakeHelperProxy.h PRE-CREATION > staging/kauth/src/backends/fakehelper/FakeHelperProxy.cpp PRE-CREATION > staging/kauth/src/backends/mac/AuthServicesBackend.cpp PRE-CREATION > staging/kauth/src/backends/policykit/PolicyKitBackend.cpp PRE-CREATION > staging/kauth/src/backends/polkit-1/Polkit1Backend.cpp PRE-CREATION > staging/kauth/src/kauthaction.h PRE-CREATION > staging/kauth/src/kauthaction.cpp PRE-CREATION > staging/kauth/src/kauthactionreply.h PRE-CREATION > staging/kauth/src/kauthactionreply.cpp PRE-CREATION > staging/kauth/src/kauthactionwatcher.h PRE-CREATION > staging/kauth/src/kauthactionwatcher.cpp PRE-CREATION > staging/kauth/src/kauthexecutejob.h PRE-CREATION > staging/kauth/src/kauthexecutejob.cpp PRE-CREATION > > Diff: http://git.reviewboard.kde.org/r/104337/diff/ > > > Testing > ------- > > New unit tests pass 100% > > > Thanks, > > Dario Freddi > >