Refactor IqRequest.

remoting/jingle_glue/iq_sender.h

+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef REMOTING_JINGLE_GLUE_IQ_SENDER_H_
+#define REMOTING_JINGLE_GLUE_IQ_SENDER_H_
+
+#include <map>
+#include <string>
+
+#include "base/callback.h"
+#include "base/compiler_specific.h"
+#include "base/gtest_prod_util.h"
+#include "base/memory/scoped_ptr.h"
+#include "remoting/jingle_glue/signal_strategy.h"
+
+namespace buzz {
+class XmlElement;
+}  // namespace buzz
+
+namespace remoting {
+
+class IqRequest;
+class SignalStrategy;
+
+// IqRegistry handles routing of iq responses to corresponding

[Wez, 2011/11/03 02:09:30]

IqRegistry -> IqSender?

[Sergey Ulanov, 2011/11/03 02:41:31]

Done.

+// IqRequest objects. Created in SignalStrategy and should not be used
+// directly.

[Wez, 2011/11/03 02:09:30]

This comment is out of date?

[Sergey Ulanov, 2011/11/03 02:41:31]

Done.

+class IqSender : public SignalStrategy::Listener {
+ public:
+  typedef base::Callback<void(const buzz::XmlElement*)> ReplyCallback;
+
+  static buzz::XmlElement* MakeIqStanza(const std::string& type,
+                                        const std::string& addressee,
+                                        buzz::XmlElement* iq_body);

[Wez, 2011/11/03 02:09:30]

Why not fold MakeIqStanza() in to SendIq()?

[Sergey Ulanov, 2011/11/03 02:41:31]

This is a static helper that creates Iq stanzas. It doesn't send anything.

[Wez, 2011/11/03 17:09:06]

Yes, but given that all(?) the stanzas we send are built with MakeIqStanza(), a
shortcut on IqSender that takes the extra two paremeters and calls MakeIqStanza,
then passes the result to SendIq, could be helpful.

[Sergey Ulanov, 2011/11/03 17:56:00]

Not all outgoing stanzas are built with MakeIqStanza(). Some are created by
JingleMessage::ToXml(). I've removed this method and added second SendIq() that
also formats messages.

+
+  explicit IqSender(SignalStrategy* signal_strategy);
+  virtual ~IqSender();
+
+  // Send an iq stanza. Returns an IqRequest object that represends
+  // the request. |callback| is called when response to |stanza| is
+  // received. Destroy the returned IqRequest to cancel the callback.

[Wez, 2011/11/03 02:09:30]

Who owns |stanza|?

[Sergey Ulanov, 2011/11/03 02:41:31]

Done.

+  IqRequest* SendIq(buzz::XmlElement* stanza, const ReplyCallback& callback);

[Wez, 2011/11/03 02:09:30]

Who is responsible for cleaning up IqRequest?  What happens if the caller holds
on to it after having torn down the SignalStrategy or IqSender?

[Sergey Ulanov, 2011/11/03 02:41:31]

Done.

+
+  // SignalStrategy::Listener implementation.
+  virtual bool OnIncomingStanza(const buzz::XmlElement* stanza) OVERRIDE;
+
+ private:
+  typedef std::map<std::string, IqRequest*> IqRequestMap;
+  friend class IqRequest;
+
+  // Removes |request| from the list of pending requests. Called by IqRequest.
+  void RemoveRequest(IqRequest* request);
+
+  SignalStrategy* signal_strategy_;
+  IqRequestMap requests_;
+
+  DISALLOW_COPY_AND_ASSIGN(IqSender);
+};
+
+// This call must only be used on the thread it was created on.
+class IqRequest {
+ public:
+  IqRequest(IqSender* sender, const IqSender::ReplyCallback& callback);
+  ~IqRequest();
+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(IqRequestTest, MakeIqStanza);
+  friend class IqSender;
+
+  // Called by IqSender when a response is received.
+  void OnResponse(const buzz::XmlElement* stanza);
+
+  SignalStrategy* signal_strategy_;

[Wez, 2011/11/03 02:09:30]

Do you need to keep a copy of the SignalStrategy here, rather than accessing it
via |sender_|?

[Sergey Ulanov, 2011/11/03 02:41:31]

No. SignalStratgy may be used by other object (in fact there may be multiple
objects that create IqSender instances for the same signal strategy).

[Wez, 2011/11/03 17:09:06]

Right.  But each IqSender only has a single SignalStrategy, so if the IqRequest
knows its IqSender then it can use that to get back to the SignalStrategy?

[Sergey Ulanov, 2011/11/03 17:56:00]

Oh, sorry, I misunderstood. This field wasn't even used. Removed it now.

+  IqSender* sender_;
+  IqSender::ReplyCallback callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(IqRequest);
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_JINGLE_GLUE_IQ_SENDER_H_