[Bug 41179]

[9-stef]

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.

-- 
You received this bug notification because you are a member of Mozilla
Bugs, which is a bug assignee.
https://bugs.launchpad.net/bugs/41179

Title:
  Integrate with Gnome Keyring

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/41179/+subscriptions