[Bug 23963] Support for presence optimisations

[bugzilla-daemon at freedesktop.org]

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

Will Thompson <will.thompson at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Status Whiteboard|NB#217283                   |NB#217283 review-

--- Comment #11 from Will Thompson <will.thompson at collabora.co.uk> 2011-02-24 09:12:47 PST ---
(In reply to comment #10)
> Moving to Gabble, since the spec already has a way for MC to tell the CM to go
> into idle mode. Senko has some branches:
> 
> 
> Stanza queueing in wocky:
> http://git.collabora.co.uk/?p=user/ptlo/wocky/.git;a=shortlog;h=refs/heads/stanza-queueing

I'm slightly surprised that there's a global “is this stanza important?” hook,
rather than each handler specifying whether it minds being delayed. I'd
envisaged something more like:

typedef enum {
  WOCKY_DELIVER_IMMEDIATELY,
  WOCKY_ALLOW_QUEUEING
} WockyStanzaImportance;

wocky_porter_register_handler (..., WockyStanzaImportance i, ...);

and

wocky_porter_enable_queueing()
wocky_porter_disable_queueing().

This might mean for slightly more code when registering handlers, but would
avoid having to encode all the knowledge about interestingness in one function
in the application. For example, Gabble doesn't actually handle PEP events
itself, it uses WockyPepService. With this branch, it suddenly has to re-gain
knowledge of what PEP events look like, so that it can decide whether a Wocky
component cares about them.

It also makes dispatch quite hairy when queueing is enabled: you'd find
matching handlers, but if none of them mind waiting a bit, you just push the
stanza on a queue. If one *does* want the stanza immediately, you have to go
back and dispatch all queued stanzas, and then to all the previous matching
handlers for this stanza, then continue as normal. So I can see benefits to
both ways…

If we do it the way you've implemented:

+void
+wocky_porter_register_important_handler (WockyPorter *self,
+    WockyPorterHandlerFunc callback,
+    gpointer user_data)

This should say ‘set’, not ‘register’; register implies that there can be more
than one. wocky_porter_set_stanza_importance_func()?

+  priv->important_stanza_handler = stanza_handler_new (WOCKY_STANZA_TYPE_NONE,
+      WOCKY_STANZA_SUB_TYPE_NONE, NULL, 0, NULL, callback, user_data);

Reusing the StanzaHandler struct when actually only two fields of it are
relevant feels like a bit of a hack.

The patch “WockyPorter: properly ref/unref queued stanza stanza-queueing”
should have been squashed into “WockyPorter: support for queueing unimportant
stanza”.

> Using the above wocky API additions to queue presence stanzas:
> http://git.collabora.co.uk/?p=user/ptlo/telepathy-gabble/.git;a=shortlog;h=refs/heads/pm

<http://dave.cridland.net/xeps/google-queue.html#sect-id280551>, Dave
Cridland's proposed historical XEP about google:queue, suggests that only
<presence> and <presence type='unavailable'/> should be queued. This seems
sensible: subscription requests should come through immediately.

In “conn-power-saving: rename google-specific methods”:

@@ -127,14 +127,15 @@ conn_power_saving_set_power_saving (
       return;
     }

+
   queueing_context = g_slice_new0 (ToggleQueueingContext);

Why the extra newline?

+  /* FIXME: we want this cached since it's used on every incoming stanza
+   * if in power-saving mode; but this will leak the pattern on exit */
+  if (pep_pattern == NULL)
+    {
+      pep_pattern = wocky_stanza_build (WOCKY_STANZA_TYPE_MESSAGE,
+          WOCKY_STANZA_SUB_TYPE_NONE, NULL, NULL,
+          '(', "event",
+            ':', WOCKY_XMPP_NS_PUBSUB_EVENT,
+          ')',
+          NULL);
+    }

If the importance was specified when the handler was registered, this would be
a non-issue. :)

+      /* FIXME: copy-pasted from toggle_google_queueing_cb, refactor */
+      if (enable != enabled)
+        {
+          g_object_set (G_OBJECT (self), "power-saving", enable, NULL);
+          tp_svc_connection_interface_power_saving_emit_power_saving_changed (
+              self, enable);
+        }

So yeah. Refactoring? :)

-- 
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.