Comment 101 for bug 41179

Comment on attachment 713377
Store Master Password by using libsecret to Gnome Keyring patch v1

Review of attachment 713377:
-----------------------------------------------------------------

Overall looks good. Haven't tested. Just a few comments.

::: configure.in
@@ +4989,5 @@
> +
> + if test "$MOZ_ENABLE_LIBSECRET"
> + then
> + PKG_CHECK_MODULES(MOZ_LIBSECRET, libsecret-1 >= $LIBSECRET_VERSION,[
> + MOZ_LIBSECRET_LIBS=`echo $MOZ_LIBSECRET_LIBS | sed 's/-llinc\>//'`

Are you sure you need this? If so, I'd be interested to know on which platform libsecret has that in its pkg-config file.

::: security/manager/ssl/src/nsNSSCallbacks.cpp
@@ +751,5 @@
> +on_lookup_finished(GObject *source,
> + GAsyncResult *result,
> + gpointer data)
> +{
> + gchar *pwd = secret_password_lookup_finish(result, nullptr);

Is it worth retrieving the error and logging it if things fail? May make our lives easier in the future if people run into problems and we have to diagnose them remotely.

@@ +803,5 @@
> + nsCString keyDescription;
> + keyDescription.Assign(MOZ_APP_NAME);
> + keyDescription.Append(" - ");
> + keyDescription.Append(profileName);
> + mRunning = true;

May I suggest using (the equivalent of):

MOZ_APP_NAME + " Master Password - " + profileName

So that it's clear in the password manager what this is?

@@ +808,5 @@
> +
> + secret_password_lookup(MOZILLA_SECRET_SCHEMA, nullptr, on_lookup_finished, this,
> + "application", MOZ_APP_NAME,
> + "profile", profileName.get(),
> + NULL);

Are you sure you need to do this as async? I don't understand all aspects of how SyncRunnableBase works, but if this is a separate thread, you could just use secret_password_lookup_sync().

::: security/manager/ssl/src/nsPK11TokenDB.cpp
@@ +388,5 @@
> + };
> + return &the_schema;
> +}
> +
> +#define MOZILLA_SECRET_SCHEMA get_mozilla_secret_schema()

Is there a reason the above function and macro can't be shared between nsNSSCallbacks.cpp and nsPK11TokenDB.cpp?

@@ +400,5 @@
> +on_libsecret_password_stored(GObject *source,
> + GAsyncResult *result,
> + gpointer data)
> +{
> + secret_password_store_finish (result, nullptr);

Is it worth retrieving the error and logging it if things fail? May make our lives easier in the future if people run into problems and we have to diagnose them remotely.

@@ +416,5 @@
> +on_libsecret_password_cleared(GObject *source,
> + GAsyncResult *result,
> + gpointer data)
> +{
> + secret_password_clear_finish(result, nullptr);

Ditto about the error.