Refactor IqRequest.

Refactor IqRequest.

Remove CreateIqRequest from SignalStrategy interface. Intead to send an Iq 
stanza the new IqSender now need to be used. IqSender creats of IqRequest 
objects and handling iq responses.

BUG=None
TEST=Unittests.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108606

[Sergey Ulanov, 2011-11-03 01:36:32]

ping

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

http://codereview.chromium.org/8432009/diff/5001/remoting/host/heartbeat_send...
File remoting/host/heartbeat_sender.h (right):

http://codereview.chromium.org/8432009/diff/5001/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender.h:16: #include "base/gtest_prod_util.h"
Why do we need this?

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
File remoting/jingle_glue/iq_sender.cc (right):

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.cc:78: it->second->OnResponse(stanza);
OnResponse() runs an arbitrary callback that was associated with the IqRequest;
what guarantee do we have that that doesn't tear down the IqSender?

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.cc:97: VLOG(1) << "No callback, so dropping: " <<
stanza->Str();
Rather than checking here, why even enter the request into the map if it has no
callback?

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
File remoting/jingle_glue/iq_sender.h (right):

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.h:26: // IqRegistry handles routing of iq
responses to corresponding
IqRegistry -> IqSender?

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.h:28: // directly.
This comment is out of date?

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.h:35: buzz::XmlElement* iq_body);
Why not fold MakeIqStanza() in to SendIq()?

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.h:42: // received. Destroy the returned IqRequest
to cancel the callback.
Who owns |stanza|?

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.h:43: IqRequest* SendIq(buzz::XmlElement* stanza,
const ReplyCallback& callback);
Who is responsible for cleaning up IqRequest?  What happens if the caller holds
on to it after having torn down the SignalStrategy or IqSender?

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.h:74: SignalStrategy* signal_strategy_;
Do you need to keep a copy of the SignalStrategy here, rather than accessing it
via |sender_|?

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
File remoting/jingle_glue/iq_sender_unittest.cc (right):

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender_unittest.cc:66: kNamespacePrefix, kNamespace);
nit: This entire stanza could be a constant, couldn't it?

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/javascr...
File remoting/jingle_glue/javascript_signal_strategy.cc (right):

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/javascr...
remoting/jingle_glue/javascript_signal_strategy.cc:77: std::string
JavascriptSignalStrategy::GetNextId() {
CalledOnValidThread?

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/javascr...
remoting/jingle_glue/javascript_signal_strategy.cc:82: void
JavascriptSignalStrategy::OnIq(const std::string& stanza_str) {
CalledOnValidThread?

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/jingle_...
File remoting/jingle_glue/jingle_info_request.h (right):

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_info_request.h:30: // JingleInfoRequest handles
requesting STUN/Relay infromation from
typo: infromation

(while you're here...)

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_info_request.h:41: // Callback to receive the Jingle
configuration settings.  The argumetns are
typo: argumetns

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_info_request.h:49: explicit
JingleInfoRequest(SignalStrategy* signal_strategy);
Why create with a SignalStrategy, rather than passing in an IqSender?

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/signal_...
File remoting/jingle_glue/signal_strategy.h (right):

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/signal_...
remoting/jingle_glue/signal_strategy.h:48: // object is destroyed.
This comment is out of date.

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

http://codereview.chromium.org/8432009/diff/5001/remoting/host/heartbeat_send...
File remoting/host/heartbeat_sender.h (right):

http://codereview.chromium.org/8432009/diff/5001/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender.h:16: #include "base/gtest_prod_util.h"
On 2011/11/03 02:09:30, Wez wrote:
> Why do we need this?
For FRIEND_TEST_ALL_PREFIXES() macro.

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
File remoting/jingle_glue/iq_sender.cc (right):

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.cc:78: it->second->OnResponse(stanza);
On 2011/11/03 02:09:30, Wez wrote:
> OnResponse() runs an arbitrary callback that was associated with the
IqRequest;
> what guarantee do we have that that doesn't tear down the IqSender?

Good catch. Fixed.

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.cc:97: VLOG(1) << "No callback, so dropping: " <<
stanza->Str();
On 2011/11/03 02:09:30, Wez wrote:
> Rather than checking here, why even enter the request into the map if it has
no
> callback?

Done.

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
File remoting/jingle_glue/iq_sender.h (right):

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.h:26: // IqRegistry handles routing of iq
responses to corresponding
On 2011/11/03 02:09:30, Wez wrote:
> IqRegistry -> IqSender?

Done.

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.h:28: // directly.
On 2011/11/03 02:09:30, Wez wrote:
> This comment is out of date?

Done.

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.h:35: buzz::XmlElement* iq_body);
On 2011/11/03 02:09:30, Wez wrote:
> Why not fold MakeIqStanza() in to SendIq()?
This is a static helper that creates Iq stanzas. It doesn't send anything.

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.h:42: // received. Destroy the returned IqRequest
to cancel the callback.
On 2011/11/03 02:09:30, Wez wrote:
> Who owns |stanza|?

Done.

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.h:43: IqRequest* SendIq(buzz::XmlElement* stanza,
const ReplyCallback& callback);
On 2011/11/03 02:09:30, Wez wrote:
> Who is responsible for cleaning up IqRequest?  What happens if the caller
holds
> on to it after having torn down the SignalStrategy or IqSender?

Done.

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.h:74: SignalStrategy* signal_strategy_;
On 2011/11/03 02:09:30, Wez wrote:
> Do you need to keep a copy of the SignalStrategy here, rather than accessing
it
> via |sender_|?
No. SignalStratgy may be used by other object (in fact there may be multiple
objects that create IqSender instances for the same signal strategy).

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
File remoting/jingle_glue/iq_sender_unittest.cc (right):

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender_unittest.cc:66: kNamespacePrefix, kNamespace);
On 2011/11/03 02:09:30, Wez wrote:
> nit: This entire stanza could be a constant, couldn't it?

Yes, but it means we would duplicate some data. Don't think it matters much.

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/javascr...
File remoting/jingle_glue/javascript_signal_strategy.cc (right):

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/javascr...
remoting/jingle_glue/javascript_signal_strategy.cc:77: std::string
JavascriptSignalStrategy::GetNextId() {
On 2011/11/03 02:09:30, Wez wrote:
> CalledOnValidThread?

Done.

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/javascr...
remoting/jingle_glue/javascript_signal_strategy.cc:82: void
JavascriptSignalStrategy::OnIq(const std::string& stanza_str) {
On 2011/11/03 02:09:30, Wez wrote:
> CalledOnValidThread?

Done.

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/jingle_...
File remoting/jingle_glue/jingle_info_request.h (right):

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_info_request.h:30: // JingleInfoRequest handles
requesting STUN/Relay infromation from
On 2011/11/03 02:09:30, Wez wrote:
> typo: infromation
> 
> (while you're here...)

Done.

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_info_request.h:41: // Callback to receive the Jingle
configuration settings.  The argumetns are
On 2011/11/03 02:09:30, Wez wrote:
> typo: argumetns

Done.

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_info_request.h:49: explicit
JingleInfoRequest(SignalStrategy* signal_strategy);
On 2011/11/03 02:09:30, Wez wrote:
> Why create with a SignalStrategy, rather than passing in an IqSender?
The idea is that IqSender is created by whoever needs to send an Iq message -
let me know if you disagree with this approach. It's also easier to test this
code when we pass around virtual interfaces.

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/signal_...
File remoting/jingle_glue/signal_strategy.h (right):

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/signal_...
remoting/jingle_glue/signal_strategy.h:48: // object is destroyed.
On 2011/11/03 02:09:30, Wez wrote:
> This comment is out of date.

Done.

[Wez, 2011-11-03 17:09:05]

I think you still need to upload the latest version of the patch?

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
File remoting/jingle_glue/iq_sender.h (right):

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.h:35: buzz::XmlElement* iq_body);
On 2011/11/03 02:41:31, sergeyu wrote:
> On 2011/11/03 02:09:30, Wez wrote:
> > Why not fold MakeIqStanza() in to SendIq()?
> This is a static helper that creates Iq stanzas. It doesn't send anything.

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.

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.h:74: SignalStrategy* signal_strategy_;
On 2011/11/03 02:41:31, sergeyu wrote:
> On 2011/11/03 02:09:30, Wez wrote:
> > Do you need to keep a copy of the SignalStrategy here, rather than accessing
> it
> > via |sender_|?
> No. SignalStratgy may be used by other object (in fact there may be multiple
> objects that create IqSender instances for the same signal strategy).

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?

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/jingle_...
File remoting/jingle_glue/jingle_info_request.h (right):

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_info_request.h:49: explicit
JingleInfoRequest(SignalStrategy* signal_strategy);
On 2011/11/03 02:41:31, sergeyu wrote:
> On 2011/11/03 02:09:30, Wez wrote:
> > Why create with a SignalStrategy, rather than passing in an IqSender?
> The idea is that IqSender is created by whoever needs to send an Iq message -
> let me know if you disagree with this approach. It's also easier to test this
> code when we pass around virtual interfaces.

When we discussed this previously, the model I had was:

IqRequest - Resource managing a single outstanding Iq request, providing control
over the lifetime of the callback.
IqSender - Manager class, instantiated once by each class that needs to dispatch
IqRequests, to manage those requests' lifetimes en masse, so that if the caller
is destroyed, teardown of the IqSender will ensure no further callbacks are
dispatched to the caller.  There may be many IqSenders using a single
SignalStrategy, and each IqSender can manage any number of requests.
SignalStrategy - The actual signalling mechanism.

The question I was asking here was simply that since this class only really
needs to see an IqSender, not a SignalStrategy, would it make more sense for the
caller to create the IqSender and pass it in?  But I think the current layout is
fine.

[Sergey Ulanov, 2011-11-03 17:55:59]

> I think you still need to upload the latest version of the patch?

Sorry. Uploaded now.

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
File remoting/jingle_glue/iq_sender.h (right):

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.h:35: buzz::XmlElement* iq_body);
On 2011/11/03 17:09:06, Wez wrote:
> On 2011/11/03 02:41:31, sergeyu wrote:
> > On 2011/11/03 02:09:30, Wez wrote:
> > > Why not fold MakeIqStanza() in to SendIq()?
> > This is a static helper that creates Iq stanzas. It doesn't send anything.
> 
> 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.

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.

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.h:74: SignalStrategy* signal_strategy_;
On 2011/11/03 17:09:06, Wez wrote:
> On 2011/11/03 02:41:31, sergeyu wrote:
> > On 2011/11/03 02:09:30, Wez wrote:
> > > Do you need to keep a copy of the SignalStrategy here, rather than
accessing
> > it
> > > via |sender_|?
> > No. SignalStratgy may be used by other object (in fact there may be multiple
> > objects that create IqSender instances for the same signal strategy).
> 
> 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?
Oh, sorry, I misunderstood. This field wasn't even used. Removed it now.

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/jingle_...
File remoting/jingle_glue/jingle_info_request.h (right):

http://codereview.chromium.org/8432009/diff/5001/remoting/jingle_glue/jingle_...
remoting/jingle_glue/jingle_info_request.h:49: explicit
JingleInfoRequest(SignalStrategy* signal_strategy);
On 2011/11/03 17:09:06, Wez wrote:
> On 2011/11/03 02:41:31, sergeyu wrote:
> > On 2011/11/03 02:09:30, Wez wrote:
> > > Why create with a SignalStrategy, rather than passing in an IqSender?
> > The idea is that IqSender is created by whoever needs to send an Iq message
-
> > let me know if you disagree with this approach. It's also easier to test
this
> > code when we pass around virtual interfaces.
> 
> When we discussed this previously, the model I had was:
> 
> IqRequest - Resource managing a single outstanding Iq request, providing
control
> over the lifetime of the callback.
> IqSender - Manager class, instantiated once by each class that needs to
dispatch
> IqRequests, to manage those requests' lifetimes en masse, so that if the
caller
> is destroyed, teardown of the IqSender will ensure no further callbacks are
> dispatched to the caller.  There may be many IqSenders using a single
> SignalStrategy, and each IqSender can manage any number of requests.
> SignalStrategy - The actual signalling mechanism.
> 
> The question I was asking here was simply that since this class only really
> needs to see an IqSender, not a SignalStrategy, would it make more sense for
the
> caller to create the IqSender and pass it in?  But I think the current layout
is
> fine.

Yes, potentially we could pass IqSender here. It may be more efficient in some
cases, but given that we never create more than 3 IqSenders it doesn't make much
difference. I think passing signal_strategy is cleaner.

[Wez, 2011-11-03 20:29:19]

LGTM, with one style-guide nit.

http://codereview.chromium.org/8432009/diff/9033/remoting/jingle_glue/iq_send...
File remoting/jingle_glue/iq_sender.h (right):

http://codereview.chromium.org/8432009/diff/9033/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.h:47: const ReplyCallback& callback);
Overloads make style pixie cry.

[Sergey Ulanov, 2011-11-03 20:52:12]

http://codereview.chromium.org/8432009/diff/9033/remoting/jingle_glue/iq_send...
File remoting/jingle_glue/iq_sender.h (right):

http://codereview.chromium.org/8432009/diff/9033/remoting/jingle_glue/iq_send...
remoting/jingle_glue/iq_sender.h:47: const ReplyCallback& callback);
On 2011/11/03 20:29:19, Wez wrote:
> Overloads make style pixie cry.

I think this is a case when function overload is acceptable, though I can rename
it if you disagree.

[commit-bot: I haz the power, 2011-11-03 23:37:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/8432009/9033

[Wez, 2011-11-03 23:50:39]

On 2011/11/03 20:52:12, sergeyu wrote:
>
http://codereview.chromium.org/8432009/diff/9033/remoting/jingle_glue/iq_send...
> File remoting/jingle_glue/iq_sender.h (right):
> 
>
http://codereview.chromium.org/8432009/diff/9033/remoting/jingle_glue/iq_send...
> remoting/jingle_glue/iq_sender.h:47: const ReplyCallback& callback);
> On 2011/11/03 20:29:19, Wez wrote:
> > Overloads make style pixie cry.
> 
> I think this is a case when function overload is acceptable, though I can
rename
> it if you disagree.

I'd say this was one of the cases where it's not acceptable, because the
overload do different things - one sends the stanza and the other modifies it
with supplied fields, then sends it.  The style-guide examples of acceptable
overloads are functionally equivalent, but accept different parameter types.  If
we can think of a good name for it then we can rename it in a follow-up CL.

[commit-bot: I haz the power, 2011-11-04 01:00:51]

Change committed as 108606

[Sergey Ulanov, 2011-11-04 02:11:33]

On 2011/11/03 23:50:39, Wez wrote:
> On 2011/11/03 20:52:12, sergeyu wrote:
> >
>
http://codereview.chromium.org/8432009/diff/9033/remoting/jingle_glue/iq_send...
> > File remoting/jingle_glue/iq_sender.h (right):
> > 
> >
>
http://codereview.chromium.org/8432009/diff/9033/remoting/jingle_glue/iq_send...
> > remoting/jingle_glue/iq_sender.h:47: const ReplyCallback& callback);
> > On 2011/11/03 20:29:19, Wez wrote:
> > > Overloads make style pixie cry.
> > 
> > I think this is a case when function overload is acceptable, though I can
> rename
> > it if you disagree.
> 
> I'd say this was one of the cases where it's not acceptable, because the
> overload do different things - one sends the stanza and the other modifies it
> with supplied fields, then sends it. 

It doesn't modify the message, it creates the message and sends it. You can look
at it as a different way to specify the stanza: each stanza is essentially a
union of stanza type, receiver jid and stanza body. First overload accepts it as
one parameter, second one - as three separate parameters that when combined
create the stanza.

> The style-guide examples of acceptable
> overloads are functionally equivalent, but accept different parameter types.

Here the two methods do the same thing, they just accept the stanza differently.

> If  we can think of a good name for it then we can rename it in a follow-up
CL.