Merge lp://staging/~ted/unity8-policy-kit/initial-work into lp://staging/unity8-policy-kit

Partial review inline, I'm EODing and doing a dump of what I've got so far.

No showstoppers, just a lot of piddly stuff.

Not sure what I'll do when I get to reviewing the pieces that got truncated by launchpad...

Review pt 2 of N.
Dumping into the comments rather than inline as discussed on IRC :)

auth-manager.h:
member fields 'inFlight' and 'thread' don't use the same 'member field' notation as other classes

agent-glib.cpp's initiate_authentication:
do we know for certain that all the arguments passed into this will be non-null?
otherwise we'll get a crash when they're converted into std::string as we pass them along as args to agentlib->cpp->authRequest()

auth-manager.cpp
* I'm a little concerned that notify_init() and get_server_caps() are called basically immediately on startup,
and that startup happens on unity8 startup.
indicator-datetime did something similar and had a startup timing issue with unity-notifications,
it now watches for the notifications name to be owned on the bus

auth-manager.cpp
> if (std::string(capname) == "x-canonical-private-synchronous")
I'm still paranoid about passing untested input into std::string(), IMO !g_strcmp0(capname, "x-canonical-private-synchronous") is safer.
Also, can break out of the loop once hasDialogs is true

auth-manager.cpp
> auto success = thread.executeOnThread<bool>([]() {
> thread.executeOnThread<bool>([this]() {
same question as earlier, these returned bools are unused, so why not executeOnThread<>(... instead?

auth-manager.cpp
> auto capname = reinterpret_cast<const gchar *>(cap->data);
static_cast because cap->data is a void*

auth-manger.cpp
> return thread.executeOnThread<std::string>([this, &action_id, &message, &icon_name, &cookie, &identities, &finishedCallback]() {
after a bunch of nitpicking, here's a legit Needs Fixing, none of these captured variables should be references because they'll be
pointing at out-of-scope objects as soon as agent_glib_class_init()'s initiate_dead objects lambda exits

auth-manager.cpp
> throw std::runtime_error("Handle for Authentication '" + cookie +
where is this caught?
this doesn't seem like it should be a fatal error but I don't see where it's caught?

On Wed, 2016-04-13 at 22:57 +0000, Charles Kerr wrote:
> > === added directory 'po'
> > === added file 'po/CMakeLists.txt'
> > --- po/CMakeLists.txt 1970-01-01 00:00:00 +0000
> > +++ po/CMakeLists.txt 2016-04-13 15:08:01 +0000
> > @@ -0,0 +1,39 @@
> Might be better to use cmake-extras' FindXGettext instead of rolling
> our own here in po/
Punted for future optimization.
> > -include_directories(${CMAKE_CURRENT_SOURCE_DIR})
> > +include_directories(${POLICYKIT_INCLUDE_DIRS})
> > +include_directories(${GIO_INCLUDE_DIRS})
> > +include_directories(${NOTIFY_INCLUDE_DIRS})
> >

>
>
> These directories should be flagged as holding system headers and can be folded into one call, eg:
>
> include_directories(SYSTEM
> ${POLICYKIT_INCLUDE_DIRS}
> ${GIO_INCLUDE_DIRS}
> ${NOTIFY_INCLUDE_DIRS}
> )
>
> >

Fixed r28

> > + listener->initiate_authentication = [](PolkitAgentListener *agent_listener, const gchar *action_id,
> >

>
>
> these are pretty big lambdas, it might be more readable to make them normal functions in a file-visible namespace
>
> >

Yeah, they started small... fixed r29
> > + const gchar *message, const gchar *icon_name, PolkitDetails *details,
> > + const gchar *cookie, GList *identities, GCancellable *cancellable,
> > + GAsyncReadyCallback callback, gpointer user_data) {
> > + auto agentglib = reinterpret_cast<AgentGlib *>(agent_listener);
> >

>
>
> move this declaration down to just before agentglib is used
>

Also fixed in r29
> >
> > +
> > + /* Turn identities into an STL list */
> > + std::list<std::string> idents;
> > + for (GList *identhead = identities; identhead != nullptr; identhead = g_list_next(identhead))
> > + {
> > + auto ident = reinterpret_cast<PolkitIdentity *>(identhead->data);
> >

>
>
> static_cast would be better here because identhead->data is a void*
>
>
> >
> > + auto identstr = polkit_identity_to_string(ident);
> > + idents.push_back(identstr);
> > + g_free(identstr);
> > + }
> > +
> > + /* Get a C++ shared ptr for cancellable */
> > + auto cancel = std::shared_ptr<GCancellable>(reinterpret_cast<GCancellable *>(g_object_ref(cancellable)),
> >

>
>
> static_cast<> would be better here because g_object_ref() returns a void*, not a GObject*
>
>
> >
> > + [](GCancellable *cancel) { g_clear_object(&cancel); });
> > + auto task = std::shared_ptr<GTask>(g_task_new(agent_listener, nullptr, callback, user_data),
> > + [](GTask *task) { g_clear_object(&task); });
> > +
> > + /* Make a function object for the callback */
> > + auto call = [task](Authentication::State state) -> void {
> > + if (state == Authentication::State::CANCELLED)
> > + {
> > + g_task_return_new_error(task.get(), agent_glib_error_quark(), 0, "Authentication Error: Cancelled");
> > + }
> > + else
> > + {
> > +...

Part 3 of 3. Suggestions but no blockers.

session-iface.cpp
> public:
Minor since it's an impl class, but lots of this should be private, eg requestCb, infoCb, errorCb, completeCb, session, sessionComplete

> ~Impl()
Asking since I'm not familiar with the ins & outs of polkitagent, are we sure we hold the sole 'session' reference? if not, we should g_signal_handlers_disconnect_by_data(session, this)

> g_signal_connect(G_OBJECT(session), ...
oddly enough, g_signal_connect takes a gpointer, so the G_OBJECT() cast is unnecessary

session-iface.h
> /** Someone should implement this */
lol

notifications-mock.h
not a blocker for the deadline, but why not use the dbusmock template?

agent-test.cpp
> class AuthCallbackDelay
this is nice stuff

> EXPECT_EQ(false, beginfuture.get());
EXPECT_FALSE(beginfuture.get())

On Thu, 2016-04-14 at 18:10 +0000, Charles Kerr wrote:
> auth-manager.h:
> member fields 'inFlight' and 'thread' don't use the same 'member
> field' notation as other classes
Fixed r35
> agent-glib.cpp's initiate_authentication:
> do we know for certain that all the arguments passed into this will be non-null?
> otherwise we'll get a crash when they're converted into std::string as we pass them along as args to agentlib->cpp->authRequest()
>

They *should* be, but I think protecting isn't a bad thing. Helper
inline func in r36
> auth-manager.cpp
> * I'm a little concerned that notify_init() and get_server_caps() are called basically immediately on startup,
> and that startup happens on unity8 startup.
> indicator-datetime did something similar and had a startup timing issue with unity-notifications,
> it now watches for the notifications name to be owned on the bus
>

I think we've fixed that in unity8's startup now, it should export the
interface and block startup until it is complete. So things that are
started unity8 should work. Since we're exiting and will get restarted
on that case, I'm okay leaving it as-is now, and then watching daisy
for reports.
> auth-manager.cpp
>
> >
> > if (std::string(capname) == "x-canonical-private-synchronous")
> >

>
> I'm still paranoid about passing untested input into std::string(), IMO !g_strcmp0(capname, "x-canonical-private-synchronous") is safer.
> Also, can break out of the loop once hasDialogs is true
>

One of my favorite features of C++ is being about use == with strings,
so I put an if nullptr continue statement in instead of gstrcmp :-)
 r37
> auth-manager.cpp
>
> >
> > auto success = thread.executeOnThread<bool>([]() {
> > thread.executeOnThread<bool>([this]() {
> >

>
> same question as earlier, these returned bools are unused, so why not executeOnThread<>(... instead?
>

Same answer ;-)
> auth-manager.cpp
>
> >
> > auto capname = reinterpret_cast<const gchar *>(cap->data);
> >

>
> static_cast because cap->data is a void*
>

Fixed r38
> auth-manger.cpp
>
> >
> > return thread.executeOnThread<std::string>([this, &action_id, &message, &icon_name, &cookie, &identities, &finishedCallback]() {
> >

>
> after a bunch of nitpicking, here's a legit Needs Fixing, none of these captured variables should be references because they'll be
> pointing at out-of-scope objects as soon as agent_glib_class_init()'s initiate_dead objects lambda exits
>

No, because the executeOnThread is blocking so the lambda will finish
executing before the execute returns. The buildAuthentication lambda
does make copies because it is executed later.
> auth-manager.cpp
>
> >
> > throw std::runtime_error("Handle for Authentication '" + cookie +
> >

>
> where is this caught?
> this doesn't seem like it should be a fatal error but I don't see where it's caught?
>

I was thinking it should be fatal. Generally this would mean our data
structures are corrupt, let Upstart restart us.

On Thu, 2016-04-14 at 18:56 +0000, Charles Kerr wrote:
> session-iface.cpp
> > public:
> Minor since it's an impl class, but lots of this should be private,
> eg requestCb, infoCb, errorCb, completeCb, session, sessionComplete
Yeah, good idea though. r39
> > ~Impl()
> >

>
> Asking since I'm not familiar with the ins & outs of polkitagent, are we sure we hold the sole 'session' reference? if not, we should g_signal_handlers_disconnect_by_data(session, this)
Seems we should be from the docs, but I think that's a good idea
anyway. r40
> notifications-mock.h
> not a blocker for the deadline, but why not use the dbusmock template?
>

Two reasons, one it's impossible to change the capabilities in the
dbusmock template. The second is all the helper functions that make the
tests look nice. The dbusmock templates only work for simple ping and
check, it's hard to change what the functions return in them.
> > EXPECT_EQ(false, beginfuture.get());
> >

>
> EXPECT_FALSE(beginfuture.get())
>

r41

Thanks for the fixes ted!

I don't see any showstoppers here that would stop this from moving forward on the current deadline.

I'll do a followup pass on the documentation and spend 30m to see if I can get FindXGettext working.