Created attachment 187992 [details] [review]
Reviewable implementation

This patch is a squashed version of: http://cgit.collabora.com/git/user/stefw/gnome-keyring.git/log/?h=gnupg-photos

Review of attachment 187992 [details] [review]:

A few comments about missing documentation and some questions about typing. There are a couple of TODO comments in the patch which I don't know if you intended to leave in before committing.

::: gcr/gcr-gnupg-collection.c
@@ +289,3 @@
+
+		if (!g_hash_table_steal (load->attributes, fingerprint))
+			g_assert_not_reached ();

Won't this cause a leak of the memory for the hash table key?

::: gcr/gcr-gnupg-key.c
@@ +249,3 @@
+
+	/**
+	 * GcrGnupgKey::icon:

Property syntax in gtk-doc uses one colon, not two.

@@ +427,3 @@
 
+const gchar*
+_gcr_gnupg_key_get_fingerprint_for_records (GPtrArray *records)

Documentation comment needed.

@@ +460,3 @@
+			continue;
+
+		/* TODO: Validity? */

TODO comment.

@@ +467,3 @@
+		/* Header is 16 bytes long */
+		if (n_data <= IMAGE_HEADER_LEN)
+			continue;

If this branch is taken, data is leaked.

@@ +472,3 @@
+		g_assert (IMAGE_JPEG_SIG_LEN < IMAGE_HEADER_LEN);
+		if (memcmp (data, IMAGE_JPEG_SIG, IMAGE_JPEG_SIG_LEN) != 0)
+			continue;

Same here: if this branch is taken, data is leaked.

@@ +484,3 @@
+
+GIcon*
+_gcr_gnupg_key_get_icon (GcrGnupgKey *self)

gtk-doc comment needed.

@@ +491,3 @@
+		self->pv->icon = load_user_attribute_icon (self);
+		if (self->pv->icon == NULL)
+			self->pv->icon = g_themed_icon_new ("folder"); /* TODO: proper icon */

TODO comment.

@@ +516,3 @@
 	};
 
+	columns[0].property_type = columns[0].column_type = G_TYPE_ICON;

Why aren't these set above?

::: gcr/gcr-gnupg-util.c
@@ +30,3 @@
+GcrRecord*
+_gcr_gnupg_build_xa1_record (GcrRecord *meta, gpointer attribute,
+                             gsize n_attribute)

gtk-doc comment needed.

@@ +71,3 @@
+		status = "e";
+	else if (flags & 0x01)
+		status = "P";

A comment or some #defines to explain these magic values would be clearer.

::: gcr/gcr-memory-icon.c
@@ +56,3 @@
+
+	if (self->pv->destroy)
+		(self->pv->destroy) (self->pv->data);

Might want to check for (self->pv->data != NULL) as well, just in case self->pv->destroy() explodes on NULL values.

@@ +67,3 @@
+	GObjectClass *gobject_class = G_OBJECT_CLASS (klass);
+
+	_gcr_memory_icon_parent_class = g_type_class_peek_parent (klass);

Initialising *_parent_class is implicitly done by G_DEFINE_TYPE_WITH_CODE(), so this line's unnecessary.

@@ +81,3 @@
+	if (icon1 == icon2)
+		return TRUE;
+	if (!g_str_equal (one->pv->image_type, two->pv->image_type))

Why use g_str_equal() instead of strcmp() directly?

@@ +129,3 @@
+	                                          self->pv->n_data, NULL);
+	g_object_set_data_full (G_OBJECT (is), "back-reference", g_object_ref (self),
+	                        g_object_unref);

Is the purpose of this to ensure that the data isn't freed until after the GMemoryInputStream is finalised? If so, a comment saying so would be useful.

@@ +166,3 @@
+
+GIcon*
+_gcr_memory_icon_new (const gchar *image_type, gconstpointer data, gsize n_data)

gtk-doc comment needed. Wouldn't the data pointer be better typed as (const guint8 *) to indicate that it's a bag of bytes?

@@ +177,3 @@
+GIcon*
+_gcr_memory_icon_new_full (const gchar *image_type, gpointer data, gsize n_data,
+                           goffset offset, GDestroyNotify destroy)

Again, gtk-doc comment needed, especially to document that data is consumed by the function. Same for the data pointer type as above.

@@ +183,3 @@
+	g_return_val_if_fail (image_type != NULL, NULL);
+	g_return_val_if_fail (data != NULL, NULL);
+	g_return_val_if_fail (offset <= n_data, NULL);

Is it wise to allow zero-sized images to be created?

::: gcr/gcr-memory-icon.h
@@ +51,3 @@
+};
+
+GType               _gcr_memory_icon_get_type             (void);

G_GNUC_CONST.

::: gcr/gcr-record.c
@@ +217,3 @@
 }
 
+gpointer

Why not guint8* or guchar* as a more specific return type?

@@ +218,3 @@
 
+gpointer
+_gcr_record_get_base64 (GcrRecord *record, guint column, gsize *n_data)

gtk-doc comment needed, specifying that the return value is (transfer full).

::: gcr/tests/frob-gnupg-selector.c
@@ +60,2 @@
 	collection = _gcr_gnupg_collection_new (NULL);
+	selector = gcr_selector_new (collection, GCR_GNUPG_KEY_COLUMNS, GCR_SELECTOR_MODE_SINGLE);

Is this change related to the rest of the patch?

::: gcr/tests/test-gnupg-collection.c
@@ +166,3 @@
+		g_assert (key == l->data);
+	}
+	g_list_free (objects);

This won't catch the case where gcr_collection_get_objects() (erroneously) returns a correctly-sized list of duplicate objects. What we did in libfolks to test for this kind of case is to duplicate the hash table before the loop, then remove each successfully-looked-up hash key from the table in each loop iteration. After the loop, we then assert that the copied hash table is empty.

::: gcr/tests/test-memory-icon.c
@@ +38,3 @@
+} Test;
+
+static const unsigned char test_data[256] = {

s/unsigned char/guint8/ perhaps?

Thanks for the review. Made most of those changes, merged into master. A few comments:

(In reply to comment #2)
> ::: gcr/gcr-gnupg-collection.c
> @@ +289,3 @@
> +
> +        if (!g_hash_table_steal (load->attributes, fingerprint))
> +            g_assert_not_reached ();
> 
> Won't this cause a leak of the memory for the hash table key?

Good catch, fixed.

> @@ +516,3 @@
>      };
> 
> +    columns[0].property_type = columns[0].column_type = G_TYPE_ICON;
> 
> Why aren't these set above?

Because they're not constant values, and can't be used when initializing arrays. :(

> ::: gcr/gcr-memory-icon.c
> @@ +56,3 @@
> +
> +    if (self->pv->destroy)
> +        (self->pv->destroy) (self->pv->data);
> 
> Might want to check for (self->pv->data != NULL) as well, just in case
> self->pv->destroy() explodes on NULL values.

We want to be able to have a callback that's run with NULL data though. So I left this as is.

> @@ +81,3 @@
> +    if (icon1 == icon2)
> +        return TRUE;
> +    if (!g_str_equal (one->pv->image_type, two->pv->image_type))
> 
> Why use g_str_equal() instead of strcmp() directly?

For readability.

> @@ +166,3 @@
> +
> +GIcon*
> +_gcr_memory_icon_new (const gchar *image_type, gconstpointer data, gsize
> n_data)
> 
> gtk-doc comment needed. Wouldn't the data pointer be better typed as (const
> guint8 *) to indicate that it's a bag of bytes?

GMemoryInputStream uses a void pointer here too, so I left it that way.

> ::: gcr/tests/test-gnupg-collection.c
> @@ +166,3 @@
> +        g_assert (key == l->data);
> +    }
> +    g_list_free (objects);
> 
> This won't catch the case where gcr_collection_get_objects() (erroneously)
> returns a correctly-sized list of duplicate objects. What we did in libfolks to
> test for this kind of case is to duplicate the hash table before the loop, then
> remove each successfully-looked-up hash key from the table in each loop
> iteration. After the loop, we then assert that the copied hash table is empty.

Good point. Did it slightly differently: created a hash table which gets filled in for each object returned. Check the length of hash table after end of loop. This means we only have to iterate through the objects once.