[Bug 42288] Review Chan.I.FileTransfer.Metadata

[bugzilla-daemon at freedesktop.org]

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

--- Comment #2 from Jonny Lamb <jonny.lamb at collabora.co.uk> 2011-10-31 07:53:46 PDT ---
(In reply to comment #1)
> +  service_name = g_strdup (field->raw_value_contents[0]);
> 
> If the form has a ServiceName field but no <value/> elements, will parsing the
> form fail, or will this crash?

Let's be sure. I added another patch.

> +  if (field == NULL)
> +    {
> +      DEBUG ("ServiceName propery not present in data form; odd...");
> +      goto out;
> +    }
> +
> +  service_name = g_strdup (field->raw_value_contents[0]);
> +
> +out:
> 
> Eh, why not
> 
>    if (field != NULL)
>      service_name = ...
>    else
>      DEBUG ("ServiceName...");
> 
> rather than goto? I don't think the goto makes it clearer.

OK, done.

> Blah, if ServiceName is not going to be stashed in the same data form as the
> metadata, then it shouldn't be a data form. it should just be
> 
>   <service-name xmlns="im:telepathy:file-transfer:service"
> name='com.example.HiMum'/>

We discussed this in real life and I thought you conceded that using another
data form just made things easier?

> and… im: is not a URI scheme. http://telepathy.im/...

Okay. :-)

> +      WockyStanza *tmp = wocky_stanza_build (WOCKY_STANZA_TYPE_IQ,
> +          WOCKY_STANZA_SUB_TYPE_RESULT, NULL, NULL,
> +          '(', "x",
> +            ':', NS_X_DATA,
> +            '@', "type", "result",
> +            '(', "field",
> +              '@', "var", "FORM_TYPE",
> +              '@', "type", "hidden",
> +              '(', "value",
> +                '$', NS_TP_FT_METADATA_SERVICE,
> +              ')',
> +            ')',
> +            '(', "field",
> +              '@', "var", "ServiceName",
> +              '(', "value",
> +                '$', self->priv->service_name,
> +              ')',
> +            ')',
> +          ')',
> +          NULL);
> +      WockyNode *x = wocky_node_get_first_child (wocky_stanza_get_top_node
> (tmp));
> +      WockyNodeTree *tree = wocky_node_tree_new_from_node (x);
> +
> +      wocky_node_add_node_tree (file, tree);
> +      g_object_unref (tree);
> +      g_object_unref (tmp);
> 
> Ugh this is dumb. There should be a wocky_node_tree_build().

I agree. I opened bug #42433.

> +          WockyNode *field = wocky_node_add_child (x, "field");
> +
> +          wocky_node_set_attribute (field, "var", (const gchar *) key);
> +
> +          wocky_node_add_child_with_content (field, "value",
> +              (const gchar *) val);
> 
> wocky_node_add_build (x,
>   '(', "field",
>     '@', "var", key,
>     '(', "value",
>       '$', val,
>     ')',
>   ')', NULL);
> 
> or is that less clear?

Well let's do it and see.

> I don't really see why the metadata-specific test is the one which explicitly
> sends no metadata. I guess it's testing edge cases…

Yes. It tests that no dataform is sent. I didn't want it to send an empty
dataform in that case.

> +        requests_iface = dbus.Interface(self.conn, cs.CONN_IFACE_REQUESTS)
> 
> Just use self.conn.Requests

OK!

> +add_file_transfer_channel_class (GPtrArray *arr,
> +    gboolean metadata)
> 
> Giving the variable a more descriptive name like "include_metadata_properties"
> might be nice.

OK!

> +      service_name_value = tp_g_value_slice_new (G_TYPE_STRING);
> +      g_value_set_string (service_name_value, service_name_str);
> 
> is secret code for tp_g_value_slice_new_string (service_name_str);

YEAAAHHH.

> The huge test is huge.

Why not?

>     if expect_disco:
>         # Gabble looks up our capabilities
>         event = q.expect('stream-iq', to=contact, query_ns=ns.DISCO_INFO)
>         query_node = xpath.queryForNodes('/iq/query', event.stanza)[0]
>         assert query_node.attributes['node'] == \
>             client + '#' + c['ver']
> 
>         # send good reply
>         result = make_result_iq(stream, event.stanza)
>         query = result.firstChildElement()
>         query['node'] = client + '#' + c['ver']
> 
>         for f in features:
>             feature = query.addElement('feature')
>             feature['var'] = f
> 
>         stream.send(result)
> 
> This is secret code for caps_helper.expect_disco();
> caps_helper.send_disco_reply(). In fact, most of receive_caps in that test is
> just caps_helper.presence_and_disco().

Yep, I replaced lots of it with just that.

> All the dbus.Dictionary(a.items() + {...}.items()) stuff is really noisy. def
> dict_union(a, b) maybe?

I guess, done.

Branch updated, thanks for the review!

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA Contact for the bug.
You are the assignee for the bug.