Adding XMPP ping functionality to CLoudPrint. XMPP ping and timeout is controlled thorugh Service S…

Adding XMPP ping functionality to CloudPrint. XMPP ping and timeout is controlled thorugh Service State file.

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

[Vitaly Buka (NO REVIEWS), 2012-10-22 20:52:53]

https://codereview.chromium.org/11232048/diff/1/chrome/service/cloud_print/cl...
File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):

https://codereview.chromium.org/11232048/diff/1/chrome/service/cloud_print/cl...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:154: // Indicates
timeout between XMPP pings. ) if XMPP pings are disabled.
")"?

https://codereview.chromium.org/11232048/diff/1/chrome/service/cloud_print/cl...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:157: bool
xmpp_ping_scheduled_;
you should use xmpp_ping_timeout_sec_ and xmpp_ping_enabled_ from  settings_
directly

https://codereview.chromium.org/11232048/diff/1/chrome/service/cloud_print/cl...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:162: std::string
robot_email_;
Not sure, but maybe better solution if you remove robot_email_ and
pending_xmpp_pings_
and add them as parameters to
 void PingXmppServer();
 void ScheduleXmppPing();
 void CheckXmppPingStatus();

https://codereview.chromium.org/11232048/diff/1/chrome/service/cloud_print/cl...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:267: VLOG(1) <<
"CP_CONNECTOR: XMPP ping timeout is less then minimal value";
LOG(WARNING) ?

https://codereview.chromium.org/11232048/diff/1/chrome/service/cloud_print/cl...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:267: VLOG(1) <<
"CP_CONNECTOR: XMPP ping timeout is less then minimal value";
ConnectorSettings::InitFrom is best place to validate value. Already makes some
validation of other settings

https://codereview.chromium.org/11232048/diff/1/chrome/service/cloud_print/co...
File chrome/service/cloud_print/connector_settings.cc (left):

https://codereview.chromium.org/11232048/diff/1/chrome/service/cloud_print/co...
chrome/service/cloud_print/connector_settings.cc:20:
ConnectorSettings::ConnectorSettings()
pleas update connector_settings_unittest.cc

https://codereview.chromium.org/11232048/diff/1/chrome/service/cloud_print/co...
File chrome/service/cloud_print/connector_settings.cc (right):

https://codereview.chromium.org/11232048/diff/1/chrome/service/cloud_print/co...
chrome/service/cloud_print/connector_settings.cc:22:
connect_new_printers_(true),
xmpp_ping_enabled_ is not initialized

https://codereview.chromium.org/11232048/diff/1/chrome/service/cloud_print/co...
chrome/service/cloud_print/connector_settings.cc:84: delete_on_enum_fail_ =
source.delete_on_enum_fail();
xmpp_ping_enabled_ is not copied

That's why i'd just use xmpp_ping_timeout_sec_ = 0 and function
is_xmpp_ping_enabled() {return xmpp_ping_timeout_sec_ != 0}

https://codereview.chromium.org/11232048/diff/1/chrome/service/service_proces...
File chrome/service/service_process_prefs.cc (right):

https://codereview.chromium.org/11232048/diff/1/chrome/service/service_proces...
chrome/service/service_process_prefs.cc:61: if (prefs_->GetValue(key, &value) !=
PersistentPrefStore::READ_OK ||
misaligned

https://codereview.chromium.org/11232048/diff/1/jingle/notifier/listener/noti...
File jingle/notifier/listener/notification_defines.h (right):

https://codereview.chromium.org/11232048/diff/1/jingle/notifier/listener/noti...
jingle/notifier/listener/notification_defines.h:52: // Test code.
What does it mean here // Test code.
I see that you set this flag in back-end code.

https://codereview.chromium.org/11232048/diff/1/jingle/notifier/listener/push...
File jingle/notifier/listener/push_notifications_listen_task.cc (right):

https://codereview.chromium.org/11232048/diff/1/jingle/notifier/listener/push...
jingle/notifier/listener/push_notifications_listen_task.cc:56: return true;
Following order looks better to me


if (!IsValidNotification(stanza))
   return false;
QueueStanza(stanza);
return true;

https://codereview.chromium.org/11232048/diff/1/jingle/notifier/listener/push...
jingle/notifier/listener/push_notifications_listen_task.cc:66: (stanza->Name()
== buzz::QN_MESSAGE) || (stanza->Name() == buzz::QN_IQ);
return only looks weird to me, how about?

return (stanza->Name() == buzz::QN_MESSAGE) ||
       (stanza->Name() == buzz::QN_IQ);

https://codereview.chromium.org/11232048/diff/1/jingle/notifier/listener/push...
File jingle/notifier/listener/push_notifications_send_update_task.cc (right):

https://codereview.chromium.org/11232048/diff/1/jingle/notifier/listener/push...
jingle/notifier/listener/push_notifications_send_update_task.cc:103:
MakeIq(buzz::STR_GET, buzz::Jid(buzz::STR_EMPTY), task_id);
buzz::XmlElement* stanza = MakeIq(buzz::STR_GET,
                                  buzz::Jid(buzz::STR_EMPTY),
                                  task_id);

https://codereview.chromium.org/11232048/diff/1/jingle/notifier/listener/push...
File jingle/notifier/listener/push_notifications_send_update_task.h (right):

https://codereview.chromium.org/11232048/diff/1/jingle/notifier/listener/push...
jingle/notifier/listener/push_notifications_send_update_task.h:38: std::string
task_id);
const std::string&

[Vitaly Buka (NO REVIEWS), 2012-10-22 20:59:09]

https://codereview.chromium.org/11232048/diff/1/chrome/service/cloud_print/cl...
File chrome/service/cloud_print/cloud_print_consts.h (right):

https://codereview.chromium.org/11232048/diff/1/chrome/service/cloud_print/cl...
chrome/service/cloud_print/cloud_print_consts.h:70: const int
kMinimumXmppPingTimeoutSecs = 2*60;  // 2 minutes in seconds
Usually I am not in favor of single file that know about all constants in
project. Only if constants are shared.

I'd rather put firt two into connector_settings.cc and the rest into
cloud_print_proxy_backend.cc

[gene, 2012-10-22 21:44:12]

https://codereview.chromium.org/11232048/diff/1/chrome/service/cloud_print/cl...
File chrome/service/cloud_print/cloud_print_consts.h (right):

https://codereview.chromium.org/11232048/diff/1/chrome/service/cloud_print/cl...
chrome/service/cloud_print/cloud_print_consts.h:70: const int
kMinimumXmppPingTimeoutSecs = 2*60;  // 2 minutes in seconds
I am trying to be consistent here with the rest of the consts.

https://codereview.chromium.org/11232048/diff/1/chrome/service/cloud_print/cl...
File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):

https://codereview.chromium.org/11232048/diff/1/chrome/service/cloud_print/cl...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:154: // Indicates
timeout between XMPP pings. ) if XMPP pings are disabled.
On 2012/10/22 20:52:53, Vitaly Buka wrote:
> ")"?

Done.

https://codereview.chromium.org/11232048/diff/1/chrome/service/cloud_print/cl...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:157: bool
xmpp_ping_scheduled_;
unfortunately not :(.
current run-time settings may be different from ones in the Service State file

https://codereview.chromium.org/11232048/diff/1/chrome/service/cloud_print/cl...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:162: std::string
robot_email_;
not sure what you mean here, lets discuss offline

https://codereview.chromium.org/11232048/diff/1/chrome/service/cloud_print/cl...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:267: VLOG(1) <<
"CP_CONNECTOR: XMPP ping timeout is less then minimal value";
On 2012/10/22 20:52:53, Vitaly Buka wrote:
> LOG(WARNING) ?

Done.

https://codereview.chromium.org/11232048/diff/1/chrome/service/cloud_print/co...
File chrome/service/cloud_print/connector_settings.cc (left):

https://codereview.chromium.org/11232048/diff/1/chrome/service/cloud_print/co...
chrome/service/cloud_print/connector_settings.cc:20:
ConnectorSettings::ConnectorSettings()
On 2012/10/22 20:52:53, Vitaly Buka wrote:
> pleas update connector_settings_unittest.cc 

Done.

https://codereview.chromium.org/11232048/diff/1/chrome/service/cloud_print/co...
File chrome/service/cloud_print/connector_settings.cc (right):

https://codereview.chromium.org/11232048/diff/1/chrome/service/cloud_print/co...
chrome/service/cloud_print/connector_settings.cc:22:
connect_new_printers_(true),
On 2012/10/22 20:52:53, Vitaly Buka wrote:
> xmpp_ping_enabled_ is not initialized

Done.

https://codereview.chromium.org/11232048/diff/1/chrome/service/cloud_print/co...
chrome/service/cloud_print/connector_settings.cc:84: delete_on_enum_fail_ =
source.delete_on_enum_fail();
On 2012/10/22 20:52:53, Vitaly Buka wrote:
> xmpp_ping_enabled_ is not copied
> 
> That's why i'd just use xmpp_ping_timeout_sec_ = 0 and function
> is_xmpp_ping_enabled() {return xmpp_ping_timeout_sec_ != 0}

Done.

https://codereview.chromium.org/11232048/diff/1/chrome/service/service_proces...
File chrome/service/service_process_prefs.cc (right):

https://codereview.chromium.org/11232048/diff/1/chrome/service/service_proces...
chrome/service/service_process_prefs.cc:61: if (prefs_->GetValue(key, &value) !=
PersistentPrefStore::READ_OK ||
On 2012/10/22 20:52:53, Vitaly Buka wrote:
> misaligned 

Done.

https://codereview.chromium.org/11232048/diff/1/jingle/notifier/listener/noti...
File jingle/notifier/listener/notification_defines.h (right):

https://codereview.chromium.org/11232048/diff/1/jingle/notifier/listener/noti...
jingle/notifier/listener/notification_defines.h:52: // Test code.
On 2012/10/22 20:52:53, Vitaly Buka wrote:
> What does it mean here // Test code.
> I see that you set this flag in back-end code.

Done.

https://codereview.chromium.org/11232048/diff/1/jingle/notifier/listener/push...
File jingle/notifier/listener/push_notifications_listen_task.cc (right):

https://codereview.chromium.org/11232048/diff/1/jingle/notifier/listener/push...
jingle/notifier/listener/push_notifications_listen_task.cc:56: return true;
This is an original code.

https://codereview.chromium.org/11232048/diff/1/jingle/notifier/listener/push...
jingle/notifier/listener/push_notifications_listen_task.cc:66: (stanza->Name()
== buzz::QN_MESSAGE) || (stanza->Name() == buzz::QN_IQ);
On 2012/10/22 20:52:53, Vitaly Buka wrote:
> return only looks weird to me, how about?
> 
> return (stanza->Name() == buzz::QN_MESSAGE) ||
>        (stanza->Name() == buzz::QN_IQ);

Done.

https://codereview.chromium.org/11232048/diff/1/jingle/notifier/listener/push...
File jingle/notifier/listener/push_notifications_send_update_task.cc (right):

https://codereview.chromium.org/11232048/diff/1/jingle/notifier/listener/push...
jingle/notifier/listener/push_notifications_send_update_task.cc:103:
MakeIq(buzz::STR_GET, buzz::Jid(buzz::STR_EMPTY), task_id);
On 2012/10/22 20:52:53, Vitaly Buka wrote:
> buzz::XmlElement* stanza = MakeIq(buzz::STR_GET,
>                                   buzz::Jid(buzz::STR_EMPTY),
>                                   task_id);

Done.

https://codereview.chromium.org/11232048/diff/1/jingle/notifier/listener/push...
File jingle/notifier/listener/push_notifications_send_update_task.h (right):

https://codereview.chromium.org/11232048/diff/1/jingle/notifier/listener/push...
jingle/notifier/listener/push_notifications_send_update_task.h:38: std::string
task_id);
On 2012/10/22 20:52:53, Vitaly Buka wrote:
> const std::string&

Done.

[gene, 2012-10-22 22:08:37]

make changes as we discussed, please take a look

[Vitaly Buka (NO REVIEWS), 2012-10-22 22:34:05]

lgtm

https://codereview.chromium.org/11232048/diff/13001/chrome/service/cloud_prin...
File chrome/service/cloud_print/connector_settings_unittest.cc (right):

https://codereview.chromium.org/11232048/diff/13001/chrome/service/cloud_prin...
chrome/service/cloud_print/connector_settings_unittest.cc:126:
EXPECT_TRUE(settings2.IsPrinterBlacklisted("prn1"));
Would be nice to have setter test for valid/invalid values.

[gene, 2012-10-22 23:36:22]

https://codereview.chromium.org/11232048/diff/13001/chrome/service/cloud_prin...
File chrome/service/cloud_print/connector_settings_unittest.cc (right):

https://codereview.chromium.org/11232048/diff/13001/chrome/service/cloud_prin...
chrome/service/cloud_print/connector_settings_unittest.cc:126:
EXPECT_TRUE(settings2.IsPrinterBlacklisted("prn1"));
On 2012/10/22 22:34:05, Vitaly Buka wrote:
> Would be nice to have setter test for valid/invalid values.

Done.

[akalin, 2012-10-23 00:37:21]

lgtm

This is implementing http://xmpp.org/extensions/xep-0199.html right?  If so, I
think it can be done more simply.

You can instead create your own PingTask class (inheriting from buzz::XmppTask)
that sends off a <ping> iq stanza and listens for the response.

Then you can add a function to PushClient/XmppPushClient that looks like the
following:

void SendPing(const base::Callback<void(...)> reply_callback);

which will create a PingTask with reply_callback, when the response is received,
reply_callback will be called with some data.

Alternatively, you can add a function to PushClientObserver like
OnPingResponse(...) and have SendPing() take an ID which will be passed to
OnPingResponse.

The current implementation kind of mixes together two separate things --
notifications and pings -- so I'm not a fan of it.

What do you think?

http://codereview.chromium.org/11232048/diff/14002/jingle/notifier/listener/n...
File jingle/notifier/listener/notification_defines.h (right):

http://codereview.chromium.org/11232048/diff/14002/jingle/notifier/listener/n...
jingle/notifier/listener/notification_defines.h:53: bool ping;
i think this can be done without adding this field to Notification

http://codereview.chromium.org/11232048/diff/14002/jingle/notifier/listener/p...
File jingle/notifier/listener/push_notifications_listen_task.cc (right):

http://codereview.chromium.org/11232048/diff/14002/jingle/notifier/listener/p...
jingle/notifier/listener/push_notifications_listen_task.cc:46:
ProcessPingStanza(stanza);
I think it's better if we had a separate PingTask class so that each
client-to-server ping can listen for its own response

http://codereview.chromium.org/11232048/diff/14002/jingle/notifier/listener/p...
File jingle/notifier/listener/push_notifications_send_update_task.cc (right):

http://codereview.chromium.org/11232048/diff/14002/jingle/notifier/listener/p...
jingle/notifier/listener/push_notifications_send_update_task.cc:31:
stanza.reset(MakePingStanza(notification_, task_id()));
i think it's better to have a separate PingTask class so that each ping can
listen for its server response

[akalin, 2012-10-23 00:37:25]

lgtm

This is implementing http://xmpp.org/extensions/xep-0199.html right?  If so, I
think it can be done more simply.

You can instead create your own PingTask class (inheriting from buzz::XmppTask)
that sends off a <ping> iq stanza and listens for the response.

Then you can add a function to PushClient/XmppPushClient that looks like the
following:

void SendPing(const base::Callback<void(...)> reply_callback);

which will create a PingTask with reply_callback, when the response is received,
reply_callback will be called with some data.

Alternatively, you can add a function to PushClientObserver like
OnPingResponse(...) and have SendPing() take an ID which will be passed to
OnPingResponse.

The current implementation kind of mixes together two separate things --
notifications and pings -- so I'm not a fan of it.

What do you think?

[akalin, 2012-10-23 00:37:37]

ugh, didn't mean to lgtm. :/

[gene, 2012-10-23 17:22:45]

Thanks for suggestions. I initially wanted to do exactly what you described, but
during implementation abandon it for the current version. It looked like an
overkill to implement a parallel infrastructure just for pings.
Let me take another look, may be it is not that bad :). I am not a big fan
myself adding ping field to notification class. I'll give it a try and send you
a new version later today.

 

On 2012/10/23 00:37:21, akalin wrote:
> lgtm
> 
> This is implementing http://xmpp.org/extensions/xep-0199.html right?  If so, I
> think it can be done more simply.
> 
> You can instead create your own PingTask class (inheriting from
buzz::XmppTask)
> that sends off a <ping> iq stanza and listens for the response.
> 
> Then you can add a function to PushClient/XmppPushClient that looks like the
> following:
> 
> void SendPing(const base::Callback<void(...)> reply_callback);
> 
> which will create a PingTask with reply_callback, when the response is
received,
> reply_callback will be called with some data.
> 
> Alternatively, you can add a function to PushClientObserver like
> OnPingResponse(...) and have SendPing() take an ID which will be passed to
> OnPingResponse.
> 
> The current implementation kind of mixes together two separate things --
> notifications and pings -- so I'm not a fan of it.
> 
> What do you think?
> 
>
http://codereview.chromium.org/11232048/diff/14002/jingle/notifier/listener/n...
> File jingle/notifier/listener/notification_defines.h (right):
> 
>
http://codereview.chromium.org/11232048/diff/14002/jingle/notifier/listener/n...
> jingle/notifier/listener/notification_defines.h:53: bool ping;
> i think this can be done without adding this field to Notification
> 
>
http://codereview.chromium.org/11232048/diff/14002/jingle/notifier/listener/p...
> File jingle/notifier/listener/push_notifications_listen_task.cc (right):
> 
>
http://codereview.chromium.org/11232048/diff/14002/jingle/notifier/listener/p...
> jingle/notifier/listener/push_notifications_listen_task.cc:46:
> ProcessPingStanza(stanza);
> I think it's better if we had a separate PingTask class so that each
> client-to-server ping can listen for its own response
> 
>
http://codereview.chromium.org/11232048/diff/14002/jingle/notifier/listener/p...
> File jingle/notifier/listener/push_notifications_send_update_task.cc (right):
> 
>
http://codereview.chromium.org/11232048/diff/14002/jingle/notifier/listener/p...
> jingle/notifier/listener/push_notifications_send_update_task.cc:31:
> stanza.reset(MakePingStanza(notification_, task_id()));
> i think it's better to have a separate PingTask class so that each ping can
> listen for its server response

[gene, 2012-10-23 19:29:09]

Done. Could you please take a look?

[akalin, 2012-10-24 20:13:48]

http://codereview.chromium.org/11232048/diff/14003/jingle/jingle.gyp
File jingle/jingle.gyp (right):

http://codereview.chromium.org/11232048/diff/14003/jingle/jingle.gyp#newcode95
jingle/jingle.gyp:95: 'notifier/listener/push_notifications_send_ping_task.h',
i don't see these files in this CL?

Also, since sending a ping doesn't have to do with push notifications per se,
you can name them 'send_ping_task.{h,cc}'

http://codereview.chromium.org/11232048/diff/14003/jingle/notifier/listener/p...
File jingle/notifier/listener/push_client.h (right):

http://codereview.chromium.org/11232048/diff/14003/jingle/notifier/listener/p...
jingle/notifier/listener/push_client.h:54: // Sends a notification (with no
reliability guarantees).
fix comment.

http://codereview.chromium.org/11232048/diff/14003/jingle/notifier/listener/p...
File jingle/notifier/listener/push_client_observer.h (right):

http://codereview.chromium.org/11232048/diff/14003/jingle/notifier/listener/p...
jingle/notifier/listener/push_client_observer.h:43: // Called when a ping
response is received. Default implementation do
do -> does

[gene, 2012-10-24 21:39:13]

Comments addressed, added missing files, renamed ping task.
Please take a look.

[akalin, 2012-10-24 21:45:34]

few more

https://codereview.chromium.org/11232048/diff/21001/jingle/notifier/listener/...
File jingle/notifier/listener/fake_push_client.cc (right):

https://codereview.chromium.org/11232048/diff/21001/jingle/notifier/listener/...
jingle/notifier/listener/fake_push_client.cc:38: void FakePushClient::SendPing()
{
add the comment "// Do nothing."

https://codereview.chromium.org/11232048/diff/21001/jingle/notifier/listener/...
File jingle/notifier/listener/push_notifications_listen_task.cc (right):

https://codereview.chromium.org/11232048/diff/21001/jingle/notifier/listener/...
jingle/notifier/listener/push_notifications_listen_task.cc:46:
ProcessPingStanza(stanza);
I think this logic can be removed from this class and moved to SendPingTask.  In
particular, you can just implement ProcessResponse and call to the delegate,
right?

https://codereview.chromium.org/11232048/diff/21001/jingle/notifier/listener/...
File jingle/notifier/listener/send_ping_task.cc (right):

https://codereview.chromium.org/11232048/diff/21001/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.cc:9: #include "base/base64.h"
no need for base64

https://codereview.chromium.org/11232048/diff/21001/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.cc:12: #include
"jingle/notifier/listener/notification_constants.h"
no need for notification_constants?

https://codereview.chromium.org/11232048/diff/21001/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.cc:30: scoped_ptr<buzz::XmlElement>
stanza;
just initialize stanza directly from MakePingStanza()

[gene, 2012-10-24 22:30:09]

Done. Please take a look.

https://codereview.chromium.org/11232048/diff/21001/jingle/notifier/listener/...
File jingle/notifier/listener/fake_push_client.cc (right):

https://codereview.chromium.org/11232048/diff/21001/jingle/notifier/listener/...
jingle/notifier/listener/fake_push_client.cc:38: void FakePushClient::SendPing()
{
On 2012/10/24 21:45:35, akalin wrote:
> add the comment "// Do nothing."

Done.

https://codereview.chromium.org/11232048/diff/21001/jingle/notifier/listener/...
File jingle/notifier/listener/push_notifications_listen_task.cc (right):

https://codereview.chromium.org/11232048/diff/21001/jingle/notifier/listener/...
jingle/notifier/listener/push_notifications_listen_task.cc:46:
ProcessPingStanza(stanza);
On 2012/10/24 21:45:35, akalin wrote:
> I think this logic can be removed from this class and moved to SendPingTask. 
In
> particular, you can just implement ProcessResponse and call to the delegate,
> right?

Done.

https://codereview.chromium.org/11232048/diff/21001/jingle/notifier/listener/...
File jingle/notifier/listener/send_ping_task.cc (right):

https://codereview.chromium.org/11232048/diff/21001/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.cc:9: #include "base/base64.h"
On 2012/10/24 21:45:35, akalin wrote:
> no need for base64

Done.

https://codereview.chromium.org/11232048/diff/21001/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.cc:12: #include
"jingle/notifier/listener/notification_constants.h"
On 2012/10/24 21:45:35, akalin wrote:
> no need for notification_constants?

Done.

https://codereview.chromium.org/11232048/diff/21001/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.cc:30: scoped_ptr<buzz::XmlElement>
stanza;
On 2012/10/24 21:45:35, akalin wrote:
> just initialize stanza directly from MakePingStanza()

Done.

[akalin, 2012-10-24 22:43:20]

few more

https://codereview.chromium.org/11232048/diff/22025/jingle/notifier/listener/...
File jingle/notifier/listener/send_ping_task.cc (right):

https://codereview.chromium.org/11232048/diff/22025/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.cc:22: : XmppTask(parent,
buzz::XmppEngine::HL_TYPE), delegate_(delegate) {
this should be HL_SINGLE, since you're listening only for the reply to the ping

https://codereview.chromium.org/11232048/diff/22025/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.cc:32: LOG(WARNING) << "Could not send
stanza " << XmlElementToString(*stanza);
return STATE_ERROR; in this case

https://codereview.chromium.org/11232048/diff/22025/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.cc:45: if (stanza->Name() ==
buzz::QN_IQ) {
maybe:

if (stanza->Name() != buzz::QN_IQ) {
  DLOG(WARNING) << ...;
  return STATE_ERROR;
}
const std::string& type = stanza->Attr(buzz::QN_TYPE);
if (type != buzz::STR_RESULT) {
  DLOG(WARNING) << "...";
  return STATE_ERROR;
}
return STATE_RESPONSE;

?

but see below

https://codereview.chromium.org/11232048/diff/22025/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.cc:54: LOG(WARNING) << "Unknwon stanza "
<< XmlElementToString(*stanza);
Unknwon -> Unknown

https://codereview.chromium.org/11232048/diff/22025/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.cc:60: if (stanza->Name() ==
buzz::QN_IQ) {
if you're filtering out non-IQ stanzas here, then you don't have to check in
ProcessResponse.

There's also a MatchResponseIq() function in xmpptask.h.  You may just want to
use that.  You can either use it here, and have ProcessResponse just return
STATE_RESPONSE, or you can filter by IQ here, and have ProcessResponse call
MatchResponseIq and return STATE_RESPONSE STATE_ERROR appropraitely.

https://codereview.chromium.org/11232048/diff/22025/jingle/notifier/listener/...
File jingle/notifier/listener/send_ping_task.h (right):

https://codereview.chromium.org/11232048/diff/22025/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.h:28: virtual ~Delegate() {}
can you de-inline the destructor?

https://codereview.chromium.org/11232048/diff/22025/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.h:40: static buzz::XmlElement*
MakePingStanza(const std::string& task_id);
may as well make this return a scoped_ptr<>

https://codereview.chromium.org/11232048/diff/22025/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.h:44: Delegate* delegate_;
Delegate* const delegate_;

https://codereview.chromium.org/11232048/diff/22025/jingle/notifier/listener/...
File jingle/notifier/listener/send_ping_task_unittest.cc (right):

https://codereview.chromium.org/11232048/diff/22025/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task_unittest.cc:39: }
perhaps add another test for ProcessResponse/HandleStanza?

[gene, 2012-10-24 23:49:03]

Code comments addressed. Please take a look.
I'll add new unittest in the meantime

https://codereview.chromium.org/11232048/diff/22025/jingle/notifier/listener/...
File jingle/notifier/listener/send_ping_task.cc (right):

https://codereview.chromium.org/11232048/diff/22025/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.cc:22: : XmppTask(parent,
buzz::XmppEngine::HL_TYPE), delegate_(delegate) {
On 2012/10/24 22:43:20, akalin wrote:
> this should be HL_SINGLE, since you're listening only for the reply to the
ping

Done.

https://codereview.chromium.org/11232048/diff/22025/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.cc:32: LOG(WARNING) << "Could not send
stanza " << XmlElementToString(*stanza);
On 2012/10/24 22:43:20, akalin wrote:
> return STATE_ERROR; in this case

Done.

https://codereview.chromium.org/11232048/diff/22025/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.cc:45: if (stanza->Name() ==
buzz::QN_IQ) {
On 2012/10/24 22:43:20, akalin wrote:
> maybe:
> 
> if (stanza->Name() != buzz::QN_IQ) {
>   DLOG(WARNING) << ...;
>   return STATE_ERROR;
> }
> const std::string& type = stanza->Attr(buzz::QN_TYPE);
> if (type != buzz::STR_RESULT) {
>   DLOG(WARNING) << "...";
>   return STATE_ERROR;
> }
> return STATE_RESPONSE;
> 
> ?
> 
> but see below

Done.

https://codereview.chromium.org/11232048/diff/22025/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.cc:54: LOG(WARNING) << "Unknwon stanza "
<< XmlElementToString(*stanza);
On 2012/10/24 22:43:20, akalin wrote:
> Unknwon -> Unknown

Done.

https://codereview.chromium.org/11232048/diff/22025/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.cc:60: if (stanza->Name() ==
buzz::QN_IQ) {
Done.
Note: MatchResponseIq() only accept empty Jids (see XmppTask::MatchStanzaFrom()
for details) -> comment added here.

https://codereview.chromium.org/11232048/diff/22025/jingle/notifier/listener/...
File jingle/notifier/listener/send_ping_task.h (right):

https://codereview.chromium.org/11232048/diff/22025/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.h:28: virtual ~Delegate() {}
On 2012/10/24 22:43:20, akalin wrote:
> can you de-inline the destructor?

Done. I assume the same applies to PushNotificationsListenTask.

https://codereview.chromium.org/11232048/diff/22025/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.h:40: static buzz::XmlElement*
MakePingStanza(const std::string& task_id);
I would like keeping code consistent with PushNotificationsSendUpdateTask. If
you strongly feel I should use scoped_ptr here, I'll fix it in both places.

https://codereview.chromium.org/11232048/diff/22025/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.h:44: Delegate* delegate_;
I have rarely see this syntax in Chrome. I would prefer keeping it just
Delegate* delegate_ both here and in PushNotificationsListenTask

[gene, 2012-10-25 00:04:26]

https://codereview.chromium.org/11232048/diff/22025/jingle/notifier/listener/...
File jingle/notifier/listener/send_ping_task_unittest.cc (right):

https://codereview.chromium.org/11232048/diff/22025/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task_unittest.cc:39: }
It looks like a trivial code and writing this test may require more code than it
is testing. It is hard to move HandleStanza() + ProcessResponse() into static
function because they are using XmppTask members. Instantiating SendPingTask()
also looks like more code than it actually testing (nor PushNotificationSendTask
or PushNotificationsListenTask do so).
Would you mind leaving this unittest as is?

[akalin, 2012-10-25 00:10:13]

Fix "CLoudPrint" -> "CloudPrint" in CL description

Is there a particular problem this fixes?  Can you add a bug and cite it in the
CL description?

https://codereview.chromium.org/11232048/diff/21002/chrome/service/cloud_prin...
File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):

https://codereview.chromium.org/11232048/diff/21002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:257:
pending_xmpp_pings_(kDefaultXmppPingTimeoutSecs) {
is this really the right constant to use?

https://codereview.chromium.org/11232048/diff/21002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:459: // Check ping
status is we a close to the limit.
fix typos in comment

https://codereview.chromium.org/11232048/diff/21002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:477:
base::RandInt(settings_.xmpp_ping_timeout_sec() * 0.9,
might want to do an additive interval, since if the timeout is small enough
you'll end up with an interval of a single number

https://codereview.chromium.org/11232048/diff/21002/chrome/service/cloud_prin...
File chrome/service/cloud_print/connector_settings.h (right):

https://codereview.chromium.org/11232048/diff/21002/chrome/service/cloud_prin...
chrome/service/cloud_print/connector_settings.h:84: int xmpp_ping_timeout_sec_;
why not store this as a base::TimeDelta?

https://codereview.chromium.org/11232048/diff/21002/jingle/notifier/listener/...
File jingle/notifier/listener/fake_push_client.cc (right):

https://codereview.chromium.org/11232048/diff/21002/jingle/notifier/listener/...
jingle/notifier/listener/fake_push_client.cc:39: // Do nothing.
actually, this should probably trigger the ping notification on the observers
(probably needed for the unit tests I request elsewhere)

https://codereview.chromium.org/11232048/diff/21002/jingle/notifier/listener/...
File jingle/notifier/listener/non_blocking_push_client.h (right):

https://codereview.chromium.org/11232048/diff/21002/jingle/notifier/listener/...
jingle/notifier/listener/non_blocking_push_client.h:52: virtual void SendPing()
OVERRIDE;
add unit test for this please

https://codereview.chromium.org/11232048/diff/21002/jingle/notifier/listener/...
File jingle/notifier/listener/send_ping_task.cc (right):

https://codereview.chromium.org/11232048/diff/21002/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.cc:62: // MatchResponseIq() does not
match supplied Jid with to field in stanza.
the comment is a little confusing.  Perhaps something like:

// MatchResponseIq() matches the given Jid with the "from" field of the given
stanza, which in this case should be the empty string (signifying the server).

https://codereview.chromium.org/11232048/diff/21002/jingle/notifier/listener/...
File jingle/notifier/listener/send_ping_task_unittest.cc (right):

https://codereview.chromium.org/11232048/diff/21002/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task_unittest.cc:34: base::StringPrintf(
no need to use stringprintf, you can just use +

https://codereview.chromium.org/11232048/diff/21002/jingle/notifier/listener/...
File jingle/notifier/listener/xmpp_push_client.h (right):

https://codereview.chromium.org/11232048/diff/21002/jingle/notifier/listener/...
jingle/notifier/listener/xmpp_push_client.h:54: virtual void SendPing()
OVERRIDE;
add a unit test for this please

[gene, 2012-10-25 00:43:26]

Done. Please take a look.

https://codereview.chromium.org/11232048/diff/21002/chrome/service/cloud_prin...
File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):

https://codereview.chromium.org/11232048/diff/21002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:257:
pending_xmpp_pings_(kDefaultXmppPingTimeoutSecs) {
Good catch! Too many redesigns :)

https://codereview.chromium.org/11232048/diff/21002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:459: // Check ping
status is we a close to the limit.
On 2012/10/25 00:10:13, akalin wrote:
> fix typos in comment

Done.

https://codereview.chromium.org/11232048/diff/21002/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:477:
base::RandInt(settings_.xmpp_ping_timeout_sec() * 0.9,
Actually there is a minimal timeout value that should prevent this. This way it
is easier to predict when disconnect will happen.

https://codereview.chromium.org/11232048/diff/21002/chrome/service/cloud_prin...
File chrome/service/cloud_print/connector_settings.h (right):

https://codereview.chromium.org/11232048/diff/21002/chrome/service/cloud_prin...
chrome/service/cloud_print/connector_settings.h:84: int xmpp_ping_timeout_sec_;
I assume you suggesting to do
base::TimeDelta::FromSeconds(
      base::RandInt(settings_.xmpp_ping_timeout_sec() * 0.9,
                    settings_.xmpp_ping_timeout_sec() * 1.1))
and store the result?

This actually represent a state of the Service State file and I would prefer to
keep it closer to the content of the file.

https://codereview.chromium.org/11232048/diff/21002/jingle/notifier/listener/...
File jingle/notifier/listener/fake_push_client.cc (right):

https://codereview.chromium.org/11232048/diff/21002/jingle/notifier/listener/...
jingle/notifier/listener/fake_push_client.cc:39: // Do nothing.
On 2012/10/25 00:10:13, akalin wrote:
> actually, this should probably trigger the ping notification on the observers
> (probably needed for the unit tests I request elsewhere)

Done.

https://codereview.chromium.org/11232048/diff/21002/jingle/notifier/listener/...
File jingle/notifier/listener/non_blocking_push_client.h (right):

https://codereview.chromium.org/11232048/diff/21002/jingle/notifier/listener/...
jingle/notifier/listener/non_blocking_push_client.h:52: virtual void SendPing()
OVERRIDE;
On 2012/10/25 00:10:13, akalin wrote:
> add unit test for this please

Done.

https://codereview.chromium.org/11232048/diff/21002/jingle/notifier/listener/...
File jingle/notifier/listener/send_ping_task.cc (right):

https://codereview.chromium.org/11232048/diff/21002/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.cc:62: // MatchResponseIq() does not
match supplied Jid with to field in stanza.
On 2012/10/25 00:10:13, akalin wrote:
> the comment is a little confusing.  Perhaps something like:
> 
> // MatchResponseIq() matches the given Jid with the "from" field of the given
> stanza, which in this case should be the empty string (signifying the server).

Done.

https://codereview.chromium.org/11232048/diff/21002/jingle/notifier/listener/...
File jingle/notifier/listener/send_ping_task_unittest.cc (right):

https://codereview.chromium.org/11232048/diff/21002/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task_unittest.cc:34: base::StringPrintf(
Do you mean using string += ?
Done.

https://codereview.chromium.org/11232048/diff/21002/jingle/notifier/listener/...
File jingle/notifier/listener/xmpp_push_client.h (right):

https://codereview.chromium.org/11232048/diff/21002/jingle/notifier/listener/...
jingle/notifier/listener/xmpp_push_client.h:54: virtual void SendPing()
OVERRIDE;
On 2012/10/25 00:10:13, akalin wrote:
> add a unit test for this please

Done.

[akalin, 2012-10-25 00:51:10]

few more nits, just waiting on the remaining tests

https://codereview.chromium.org/11232048/diff/22026/chrome/service/cloud_prin...
File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):

https://codereview.chromium.org/11232048/diff/22026/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:576: VLOG(1) <<
"CP_CONNECTOR: Ping response received.";
i'm guessing VLOG over DVLOG is deliberate?

https://codereview.chromium.org/11232048/diff/22026/chrome/service/cloud_prin...
File chrome/service/cloud_print/connector_settings.cc (right):

https://codereview.chromium.org/11232048/diff/22026/chrome/service/cloud_prin...
chrome/service/cloud_print/connector_settings.cc:98: LOG(WARNING) <<
DLOG maybe?

https://codereview.chromium.org/11232048/diff/22026/jingle/notifier/listener/...
File jingle/notifier/listener/non_blocking_push_client_unittest.cc (right):

https://codereview.chromium.org/11232048/diff/22026/jingle/notifier/listener/...
jingle/notifier/listener/non_blocking_push_client_unittest.cc:117: const
Notification notification = MakeTestNotification();
no need for test notification

https://codereview.chromium.org/11232048/diff/22026/jingle/notifier/listener/...
File jingle/notifier/listener/send_ping_task.cc (right):

https://codereview.chromium.org/11232048/diff/22026/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.cc:36: LOG(WARNING) << "Could not send
stanza " << XmlElementToString(*stanza);
I prefer DLOG (i guess make the equiv. change to the subscription task)

https://codereview.chromium.org/11232048/diff/22026/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.cc:52: LOG(WARNING) << "No
type=\"result\" attribute found in stanza "
DLOG here too

https://codereview.chromium.org/11232048/diff/22026/jingle/notifier/listener/...
File jingle/notifier/listener/send_ping_task_unittest.cc (right):

https://codereview.chromium.org/11232048/diff/22026/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task_unittest.cc:40: 
still writing the test for ProcessStanza / OnResponse?

https://codereview.chromium.org/11232048/diff/22026/jingle/notifier/listener/...
File jingle/notifier/listener/xmpp_push_client_unittest.cc (right):

https://codereview.chromium.org/11232048/diff/22026/jingle/notifier/listener/...
jingle/notifier/listener/xmpp_push_client_unittest.cc:114: // Make sure nothing
blows up when the XMPP push client sends a ping
append period.

Add:

// TODO(akalin): Figure out how to test that the ping was actually sent.

[gene, 2012-10-25 01:06:22]

Please take a look.
I incline to skip unittest for SendPingTask ProcessResponse and/or HandleStanza.
It looks like it will take more effort to set this up vs. actual value of
testing trivial methods. I assume PushNotificationSendUpdateTask only tests
static member, while PushNotificationsListenTask lack unittest for the same
reason.

https://codereview.chromium.org/11232048/diff/22026/chrome/service/cloud_prin...
File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):

https://codereview.chromium.org/11232048/diff/22026/chrome/service/cloud_prin...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:576: VLOG(1) <<
"CP_CONNECTOR: Ping response received.";
Yes. We often need to debug customer issue over internet/phone that is
reproducible only on site.

https://codereview.chromium.org/11232048/diff/22026/chrome/service/cloud_prin...
File chrome/service/cloud_print/connector_settings.cc (right):

https://codereview.chromium.org/11232048/diff/22026/chrome/service/cloud_prin...
chrome/service/cloud_print/connector_settings.cc:98: LOG(WARNING) <<
Actually, I would like to leave LOG here. That will indicate if customer have an
incorrect setting in Service State file.

https://codereview.chromium.org/11232048/diff/22026/jingle/notifier/listener/...
File jingle/notifier/listener/non_blocking_push_client_unittest.cc (right):

https://codereview.chromium.org/11232048/diff/22026/jingle/notifier/listener/...
jingle/notifier/listener/non_blocking_push_client_unittest.cc:117: const
Notification notification = MakeTestNotification();
On 2012/10/25 00:51:10, akalin wrote:
> no need for test notification

Done.

https://codereview.chromium.org/11232048/diff/22026/jingle/notifier/listener/...
File jingle/notifier/listener/send_ping_task.cc (right):

https://codereview.chromium.org/11232048/diff/22026/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.cc:36: LOG(WARNING) << "Could not send
stanza " << XmlElementToString(*stanza);
On 2012/10/25 00:51:10, akalin wrote:
> I prefer DLOG (i guess make the equiv. change to the subscription task)

Done.

https://codereview.chromium.org/11232048/diff/22026/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task.cc:52: LOG(WARNING) << "No
type=\"result\" attribute found in stanza "
On 2012/10/25 00:51:10, akalin wrote:
> DLOG here too

Done.

https://codereview.chromium.org/11232048/diff/22026/jingle/notifier/listener/...
File jingle/notifier/listener/send_ping_task_unittest.cc (right):

https://codereview.chromium.org/11232048/diff/22026/jingle/notifier/listener/...
jingle/notifier/listener/send_ping_task_unittest.cc:40: 
Sorry, no.
I was in the process when upload, but I think it is too much overhead. I think
PushNotificationSendUpdateTask only tests static member, while
PushNotificationsListenTask lack them for the same reason.

https://codereview.chromium.org/11232048/diff/22026/jingle/notifier/listener/...
File jingle/notifier/listener/xmpp_push_client_unittest.cc (right):

https://codereview.chromium.org/11232048/diff/22026/jingle/notifier/listener/...
jingle/notifier/listener/xmpp_push_client_unittest.cc:114: // Make sure nothing
blows up when the XMPP push client sends a ping
On 2012/10/25 00:51:10, akalin wrote:
> append period.
> 
> Add:
> 
> // TODO(akalin): Figure out how to test that the ping was actually sent.

Done.

[akalin, 2012-10-25 16:14:34]

ok, lgtm

[commit-bot: I haz the power, 2012-10-25 16:49:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gene@chromium.org/11232048/25003

[commit-bot: I haz the power, 2012-10-25 16:49:58]

Presubmit check for 11232048-25003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 2.8s to calculate.

[gene, 2012-10-25 18:15:29]

+sky@ for chrome/OWNERS

[sky, 2012-10-25 18:16:15]

chrome/common LGTM

[commit-bot: I haz the power, 2012-10-25 18:18:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gene@chromium.org/11232048/25003

[commit-bot: I haz the power, 2012-10-26 17:55:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gene@chromium.org/11232048/25003

[commit-bot: I haz the power, 2012-10-26 19:59:42]

Change committed as 164385