[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-frameworks-devel
Subject: Fwd: Review Request: Make KAuth ready for frameworks + API Changes
From: Dario Freddi <drf54321 () gmail ! com>
Date: 2012-03-18 22:27:25
Message-ID: CAFFVnfOuitx2=3ywLWej5U_2W4kzccH8CN+zHzR+yV84v7gGkw () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
For interested people (hopefully many)
---------- Messaggio inoltrato ----------
Da: Dario Freddi <drf@kde.org>
Date: 18 marzo 2012 23:25
Oggetto: Review Request: Make KAuth ready for frameworks + API Changes
A: Kevin Ottens <ervin@kde.org>, David Faure <faure@kde.org>, Alexander
Neundorf <neundorf@kde.org>
Cc: Dario Freddi <drf@kde.org>, kdelibs <kde-core-devel@kde.org>, Dario
Freddi <drf@kde.org>, kdelibs <kde-core-devel@kde.org>
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
Neundorf.
By Dario Freddi.
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.
Testing
New unit tests pass 100%
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)
View Diff <http://git.reviewboard.kde.org/r/104337/diff/>
[Attachment #5 (text/html)]
For interested people (hopefully many)<br><br><div class="gmail_quote">---------- \
Messaggio inoltrato ----------<br>Da: <b class="gmail_sendername">Dario Freddi</b> \
<span dir="ltr"><<a \
href="mailto:drf@kde.org">drf@kde.org</a>></span><br>
Date: 18 marzo 2012 23:25<br>Oggetto: Review Request: Make KAuth ready for frameworks \
+ API Changes<br>A: Kevin Ottens <<a \
href="mailto:ervin@kde.org">ervin@kde.org</a>>, David Faure <<a \
href="mailto:faure@kde.org">faure@kde.org</a>>, Alexander Neundorf <<a \
href="mailto:neundorf@kde.org">neundorf@kde.org</a>><br>
Cc: Dario Freddi <<a href="mailto:drf@kde.org">drf@kde.org</a>>, kdelibs <<a \
href="mailto:kde-core-devel@kde.org">kde-core-devel@kde.org</a>>, Dario Freddi \
<<a href="mailto:drf@kde.org">drf@kde.org</a>>, kdelibs <<a \
href="mailto:kde-core-devel@kde.org">kde-core-devel@kde.org</a>><br> <br><br>
<div>
<div style="font-family:Verdana,Arial,Helvetica,Sans-Serif">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border:1px #c9c399 \
solid"> <tbody><tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/104337/" \
target="_blank">http://git.reviewboard.kde.org/r/104337/</a> </td>
</tr>
</tbody></table>
<br>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image:url('');background-repeat:repeat-x;border:1px black \
solid"> <tbody><tr>
<td>
<div>Review request for kdelibs, Kevin Ottens, David Faure, and Alexander \
Neundorf.</div> <div>By Dario Freddi.</div>
<h1 style="color:#575012;font-size:10pt;margin-top:1.5em">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border:1px solid #b8b5a0"> <tbody><tr>
<td>
<pre style="margin:0;padding:0;white-space:pre-wrap;white-space:-moz-pre-wrap;white-space:-pre-wrap;white-space:-o-pre-wrap;word-wrap:break-word">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.</pre>
</td>
</tr>
</tbody></table>
<h1 style="color:#575012;font-size:10pt;margin-top:1.5em">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border:1px solid #b8b5a0"> <tbody><tr>
<td>
<pre style="margin:0;padding:0;white-space:pre-wrap;white-space:-moz-pre-wrap;white-space:-pre-wrap;white-space:-o-pre-wrap;word-wrap:break-word">New \
unit tests pass 100%</pre> </td>
</tr>
</tbody></table>
<h1 style="color:#575012;font-size:10pt;margin-top:1.5em">Diffs </h1>
<ul style="margin-left:3em;padding-left:0">
<li>staging/kauth/CMakeLists.txt <span style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/autotests/BackendsManager.h <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/autotests/BackendsManager.cpp <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/autotests/CMakeLists.txt <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/autotests/HelperTest.cpp <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/autotests/SetupActionTest.cpp <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/autotests/TestBackend.h <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/autotests/TestBackend.cpp <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/autotests/TestHelper.h <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/autotests/TestHelper.cpp <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/AuthBackend.h <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/CMakeLists.txt <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/HelperProxy.h <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/backends/dbus/DBusHelperProxy.h <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/backends/dbus/DBusHelperProxy.cpp <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/backends/dbus/org.kde.auth.xml <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/backends/fake/FakeBackend.cpp <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/backends/fakehelper/FakeHelperProxy.h <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/backends/fakehelper/FakeHelperProxy.cpp <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/backends/mac/AuthServicesBackend.cpp <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/backends/policykit/PolicyKitBackend.cpp <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/backends/polkit-1/Polkit1Backend.cpp <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/kauthaction.h <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/kauthaction.cpp <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/kauthactionreply.h <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/kauthactionreply.cpp <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/kauthactionwatcher.h <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/kauthactionwatcher.cpp <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/kauthexecutejob.h <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/kauthexecutejob.cpp <span \
style="color:grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/104337/diff/" style="margin-left:3em" \
target="_blank">View Diff</a></p>
</td>
</tr>
</tbody></table>
</div>
</div>
</div><br>
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic