From kde-core-devel Sun Mar 18 22:25:52 2012 From: "Dario Freddi" Date: Sun, 18 Mar 2012 22:25:52 +0000 To: kde-core-devel Subject: Review Request: Make KAuth ready for frameworks + API Changes Message-Id: <20120318222552.5332.32105 () vidsolbach ! de> X-MARC-Message: https://marc.info/?l=kde-core-devel&m=133210966423742 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--===============1159849340872646497==" --===============1159849340872646497== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104337/ ----------------------------------------------------------- Review request for kdelibs, Kevin Ottens, David Faure, and Alexander Neundo= rf. 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 th= e long summary, but it's worth reading. However. This huge patchsets brings KAuth in the marvelous world of Frameworks. If y= ou dislike ReviewBoard's way of displaying diffs or simply want to see a co= mmit list, please refer to the URL in "Branch". First of all, I pulled in a dependency on KJob after a chat with Kevin. Thi= s 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 co= mpletely synchronous, leading to a multitude of problems. After these chang= es it's fully asynchronous instead (reason for pulling in KJob), the API wa= s 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(h= elper) * 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 i= s a failure to handle implicitly shared classes in multithreaded environmen= ts with that approach. Of course, while it would be awesome to have all the code reviewed, I under= stand it's a very big change so I'd like at least some feedback on the foll= owing points: * General sanity of the new API * Consistency of the enums. StatusInvalid vs. ExecuteMode vs. Authorizatio= nDeniedError. While the semantic seems correct to me, I'd like to have some= feedback on whether consistency is valuable in the ordering of vs. and which one should be preferred in case. * Whether to deprecate static accessors such as static const ActionReply S= uccessReply(). 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, e= ven if I had no way to check this. For unit tests, I had to create a fake a= uthorization backend for testing purposes, whereas I managed to reuse the d= bus backend for helper communication, so that I could even test that. For r= unning 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 thre= ad, to prevent asynchronous DBus calls from failing due to QDBus' local-loo= p 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 --===============1159849340872646497== Content-Type: text/html; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable
This is an automatically generated e-mail. To reply, visit: http://git.revie= wboard.kde.org/r/104337/

Review request for kdelibs, Kevin Ottens, David Faure, and Alexander N= eundorf.
By Dario Freddi.

Descripti= on

Preamble - sorry for having to name-call people but apparent=
ly 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 y=
ou 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. Thi=
s makes KAuth tier2, but shouldn't be a big issue.

Then there's the hard part: source compatibility is reasonably broken h=
ere. The changes I had to do were mostly for the sake of revamping the inte=
rnal workflow of the library. The main problem KAuth had was the fact it wa=
s completely synchronous, leading to a multitude of problems. After these c=
hanges it's fully asynchronous instead (reason for pulling in KJob), th=
e API was simplified, and some unused features like multiple action executi=
on 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(h=
elper)
 * 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 w=
ith a different semantic to ease porting. The main use case for changing th=
is is a failure to handle implicitly shared classes in multithreaded enviro=
nments with that approach.

Of course, while it would be awesome to have all the code reviewed, I under=
stand 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. Authorizatio=
nDeniedError. While the semantic seems correct to me, I'd like to have =
some feedback on whether consistency is valuable in the ordering of <typ=
e><value> vs. <value><type> and which one should be pr=
eferred in case.
 * Whether to deprecate static accessors such as static const ActionReply S=
uccessReply(). 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, e=
ven if I had no way to check this. For unit tests, I had to create a fake a=
uthorization backend for testing purposes, whereas I managed to reuse the d=
bus backend for helper communication, so that I could even test that. For r=
unning 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 thre=
ad, to prevent asynchronous DBus calls from failing due to QDBus' local=
-loop optimization. The test is also run on the session bus.

Testing <= /h1>
New unit tests pass 100%

Diffs=

  • staging/kauth/CMakeLists.txt (PRE-CREATION= )
  • staging/kauth/autotests/BackendsManager.h = (PRE-CREATION)
  • staging/kauth/autotests/BackendsManager.cpp (PR= E-CREATION)
  • staging/kauth/autotests/HelperTest.cpp (PR= E-CREATION)
  • staging/kauth/autotests/SetupActionTest.cpp (PRE= -CREATION)
  • staging/kauth/autotests/TestBackend.cpp (P= RE-CREATION)
  • staging/kauth/autotests/TestHelper.h (PRE-= CREATION)
  • staging/kauth/autotests/TestHelper.cpp (PR= E-CREATION)
  • staging/kauth/src/AuthBackend.h (PRE-CREAT= ION)
  • staging/kauth/src/CMakeLists.txt (PRE-CREA= TION)
  • staging/kauth/src/HelperProxy.h (PRE-CREAT= ION)
  • 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-CREAT= ION)
  • staging/kauth/src/kauthaction.cpp (PRE-CRE= ATION)
  • staging/kauth/src/kauthactionreply.h (PRE-= CREATION)
  • staging/kauth/src/kauthactionreply.cpp (PR= E-CREATION)
  • staging/kauth/src/kauthactionwatcher.h (PR= E-CREATION)
  • staging/kauth/src/kauthactionwatcher.cpp (= PRE-CREATION)
  • staging/kauth/src/kauthexecutejob.h (PRE-C= REATION)
  • staging/kauth/src/kauthexecutejob.cpp (PRE= -CREATION)

View Diff

--===============1159849340872646497==--