[Bug 66459] Integrate new accounts-sso support into mission-control

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=66459

--- Comment #5 from Simon McVittie <simon.mcvittie at collabora.co.uk> ---
Comment on attachment 81807
  --> https://bugs.freedesktop.org/attachment.cgi?id=81807
UOA patch

Review of attachment 81807:
 --> (https://bugs.freedesktop.org/page.cgi?id=splinter.html&bug=66459&attachment=81807)
-----------------------------------------------------------------

I haven't really reviewed the code here, only the build system...

::: configure.ac
@@ +211,5 @@
>    AC_DEFINE([ENABLE_GNOME_KEYRING], [0], [Define whether gnome-keyring support is enabled])
>  fi
>  
> +# ----------------------------------------------------------------
> +# dnl libaccounts-glib SSO

Please don't commit commented-out code without a very good reason. If the
current libaccounts-sso plugin is terrible (which I can well believe) then we
should just delete it (Bug #71230).

@@ +289,5 @@
> +# -----------------------------------------------------------
> +AC_ARG_ENABLE(ubuntu-online-accounts,
> +              AS_HELP_STRING([--enable-ubuntu-online-accounts=@<:@no/yes/auto@:>@],
> +                             [build Ubuntu Online Accounts plugins]), ,
> +                             enable_ubuntu_online_accounts=auto)

If you use correct quoting (one level of [] per level of ()) then you should
never need @<:@.

AC_ARG_ENABLE([ubuntu-online-accounts],
  [AS_HELP_STRING([--enable-...=[no/yes/auto]],
    [build Ubuntu Online Accounts plugins]),
    [],
    [enable_ubuntu_online_accounts=auto])

@@ +291,5 @@
> +              AS_HELP_STRING([--enable-ubuntu-online-accounts=@<:@no/yes/auto@:>@],
> +                             [build Ubuntu Online Accounts plugins]), ,
> +                             enable_ubuntu_online_accounts=auto)
> +
> +if test "x$enable_ubuntu_online_accounts" != "xno"; then

AS_IF, please. See https://bugs.freedesktop.org/show_bug.cgi?id=53445

@@ +300,5 @@
> +    ], have_uoa="yes", have_uoa="no")
> +
> +   # provider plugin dir
> +#    AC_MSG_CHECKING([Accounts provider plugin dir])
> +#    ACCOUNTS_PROVIDER_PLUGIN_DIR=`pkg-config --variable=provider_plugindir account-plugin`

Again, no commented-out code, please.

::: plugins/Makefile.am
@@ +26,5 @@
> +        $(ERROR_CFLAGS)
> +
> +pluginsdir = $(libdir)
> +plugins_LTLIBRARIES = \
> +        mcp-account-manager-uoa.la

This should be conditionally-compiled.

::: plugins/empathy-webcredentials-monitor.c
@@ +1,1 @@
> +#include "config.h"

Licensing header?

Implementation not reviewed; it would be good to have some sort of agreement on
how this is meant to work in a "generic libaccounts" world. Either it's
necessary, in which case libaccounts should have a way to do it, or it isn't.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.