Added class to subscribe to GCD notifications over XMPP.

Added class to subscribe to GCD notifications over XMPP.

Instances of this class are created in an upcoming patch.

BUG=471928
Committed: https://crrev.com/cb58e5e73fb79375392544576e345ed4133773af
Cr-Commit-Position: refs/heads/master@{#330628}

[John Williams, 2015-05-06 18:50:26]

jrw@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Sergey Ulanov, 2015-05-07 18:22:39]

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
File remoting/host/gcd_notification_subscriber.cc (right):

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
remoting/host/gcd_notification_subscriber.cc:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
remove (c)

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
remoting/host/gcd_notification_subscriber.cc:34: DLOG(INFO) << "subscribing to
GCD command notifications";
nit: s/subscribing/Subscribing/
Add . at the end

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
remoting/host/gcd_notification_subscriber.cc:38: size_t slash_index =
bare_jid.find('/');
There are at least two other places where we need to separate JID resource_id: 
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/protocol/...
and 
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/signaling...

Maybe it's worth adding a helper method for this in
remoting/signaling/jid_util.h

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
remoting/host/gcd_notification_subscriber.cc:44: buzz::XmlElement* itemElement =
item_element

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
remoting/host/gcd_notification_subscriber.cc:48: buzz::XmlElement*
subscribeElement =
subscribe_element.
In other code that builds XmlElement we normally create elements in the same
order in which they appear in XML and I suggest following the same pattern here,
i.e.:

buzz::XmlElement* subscribe_element =
  new buzz::XmlElement(buzz::QName(kNsGooglePush, "subscribe"));

buzz::XmlElement* item_element = 
  new buzz::XmlElement(buzz::QName(kNsGooglePush, "item"));
subscribe_element->AddElement(item_element);

This makes it easier to read this code.

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
remoting/host/gcd_notification_subscriber.cc:70:
DCHECK_EQ(response->Attr(buzz::QName("", "type")), "result")
This shouldn't be DCHECK. Crashing in response to a message received from a
remote service is never right thing to do, even in debug builds. You also don't
want to ignore the error in Release builds. I think you always want to check the
result and log an error if the request has failed.

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
File remoting/host/gcd_notification_subscriber.h (right):

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
remoting/host/gcd_notification_subscriber.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
remove (c)

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
remoting/host/gcd_notification_subscriber.h:20: // channel.  No actual
notifications are used as of this writing, but
s/as of this writing/currently/ or just remove this sentence.

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
remoting/host/gcd_notification_subscriber.h:23: class GcdNotificationSubscriber
: public SignalStrategy::Listener {
Is the intent to make have a separate class receive notifications? If yes, then
why? I'd prefer having the same class subscribe and listen for notifications.
And then it should be called Receiver instead of Subscriber.

I think this class can be more generic and usable for any push notifications. It
doesn't need to be GCD-specific. Maybe call it PushNotificationReceiver and make
it work for any push notifications?

E.g. see jingle/notifier/listener/push_notifications_subscribe_task.cc - that
code is used by sync and CloudPrint to receive push notification. Similarly you
can pass list of notifications the caller want's to receive.
jingle/notifier is in a semi-deprecated state (because the XMPP code it depends
on in WebRTC is deprecated). In future I'd like to move remoting/signaling
somewhere where it can be reused by other components (e.g. components/xmpp) and
then this class will be a replacement for the current push notifications code in
jingle/notifier. 

Also please move this file to remoting/signaling.

[Sergey Ulanov, 2015-05-07 18:25:47]

Also please add some unittests :-)

[John Williams, 2015-05-15 00:51:12]

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
File remoting/host/gcd_notification_subscriber.cc (right):

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
remoting/host/gcd_notification_subscriber.cc:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
On 2015/05/07 18:22:38, Sergey Ulanov wrote:
> remove (c)

Done.

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
remoting/host/gcd_notification_subscriber.cc:34: DLOG(INFO) << "subscribing to
GCD command notifications";
On 2015/05/07 18:22:38, Sergey Ulanov wrote:
> nit: s/subscribing/Subscribing/
> Add . at the end

Done.

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
remoting/host/gcd_notification_subscriber.cc:38: size_t slash_index =
bare_jid.find('/');
On 2015/05/07 18:22:38, Sergey Ulanov wrote:
> There are at least two other places where we need to separate JID resource_id:

>
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/protocol/...
> and 
>
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/signaling...
> 
> Maybe it's worth adding a helper method for this in
> remoting/signaling/jid_util.h

Done.

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
remoting/host/gcd_notification_subscriber.cc:44: buzz::XmlElement* itemElement =
On 2015/05/07 18:22:38, Sergey Ulanov wrote:
> item_element

Done.

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
remoting/host/gcd_notification_subscriber.cc:48: buzz::XmlElement*
subscribeElement =
On 2015/05/07 18:22:38, Sergey Ulanov wrote:
> subscribe_element.
> In other code that builds XmlElement we normally create elements in the same
> order in which they appear in XML and I suggest following the same pattern
here,
> i.e.:
> 
> buzz::XmlElement* subscribe_element =
>   new buzz::XmlElement(buzz::QName(kNsGooglePush, "subscribe"));
> 
> buzz::XmlElement* item_element = 
>   new buzz::XmlElement(buzz::QName(kNsGooglePush, "item"));
> subscribe_element->AddElement(item_element);
> 
> This makes it easier to read this code.

Done.

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
remoting/host/gcd_notification_subscriber.cc:70:
DCHECK_EQ(response->Attr(buzz::QName("", "type")), "result")
On 2015/05/07 18:22:38, Sergey Ulanov wrote:
> This shouldn't be DCHECK. Crashing in response to a message received from a
> remote service is never right thing to do, even in debug builds. You also
don't
> want to ignore the error in Release builds. I think you always want to check
the
> result and log an error if the request has failed.

Done.

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
File remoting/host/gcd_notification_subscriber.h (right):

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
remoting/host/gcd_notification_subscriber.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
On 2015/05/07 18:22:39, Sergey Ulanov wrote:
> remove (c)

Done.

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
remoting/host/gcd_notification_subscriber.h:20: // channel.  No actual
notifications are used as of this writing, but
On 2015/05/07 18:22:39, Sergey Ulanov wrote:
> s/as of this writing/currently/ or just remove this sentence.

Done.

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
remoting/host/gcd_notification_subscriber.h:23: class GcdNotificationSubscriber
: public SignalStrategy::Listener {
On 2015/05/07 18:22:38, Sergey Ulanov wrote:
> Is the intent to make have a separate class receive notifications? If yes,
then
> why? I'd prefer having the same class subscribe and listen for notifications.
> And then it should be called Receiver instead of Subscriber.
> 
> I think this class can be more generic and usable for any push notifications.
It
> doesn't need to be GCD-specific. Maybe call it PushNotificationReceiver and
make
> it work for any push notifications?
> 
> E.g. see jingle/notifier/listener/push_notifications_subscribe_task.cc - that
> code is used by sync and CloudPrint to receive push notification. Similarly
you
> can pass list of notifications the caller want's to receive.
> jingle/notifier is in a semi-deprecated state (because the XMPP code it
depends
> on in WebRTC is deprecated). In future I'd like to move remoting/signaling
> somewhere where it can be reused by other components (e.g. components/xmpp)
and
> then this class will be a replacement for the current push notifications code
in
> jingle/notifier. 
> 
> Also please move this file to remoting/signaling.

The reason I called it "subscriber" is that it literally just subscribes; it
doesn't even have any logic to detect when an XMPP stanza is a notification,
because none of the notifications GCD sends are useful to us. It's likely we'll
never need to process GCD notifications, since we're sending commands over the
raw XMPP channel rather than using GCD's "command" API.

[Sergey Ulanov, 2015-05-15 18:22:37]

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
File remoting/host/gcd_notification_subscriber.h (right):

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
remoting/host/gcd_notification_subscriber.h:23: class GcdNotificationSubscriber
: public SignalStrategy::Listener {
On 2015/05/15 00:51:12, John Williams wrote:
> On 2015/05/07 18:22:38, Sergey Ulanov wrote:
> > Is the intent to make have a separate class receive notifications? If yes,
> then
> > why? I'd prefer having the same class subscribe and listen for
notifications.
> > And then it should be called Receiver instead of Subscriber.
> > 
> > I think this class can be more generic and usable for any push
notifications.
> It
> > doesn't need to be GCD-specific. Maybe call it PushNotificationReceiver and
> make
> > it work for any push notifications?
> > 
> > E.g. see jingle/notifier/listener/push_notifications_subscribe_task.cc -
that
> > code is used by sync and CloudPrint to receive push notification. Similarly
> you
> > can pass list of notifications the caller want's to receive.
> > jingle/notifier is in a semi-deprecated state (because the XMPP code it
> depends
> > on in WebRTC is deprecated). In future I'd like to move remoting/signaling
> > somewhere where it can be reused by other components (e.g. components/xmpp)
> and
> > then this class will be a replacement for the current push notifications
code
> in
> > jingle/notifier. 
> > 
> > Also please move this file to remoting/signaling.
> 
> The reason I called it "subscriber" is that it literally just subscribes; it
> doesn't even have any logic to detect when an XMPP stanza is a notification,
> because none of the notifications GCD sends are useful to us. It's likely
we'll
> never need to process GCD notifications, since we're sending commands over the
> raw XMPP channel rather than using GCD's "command" API.

I still think it's worth making this class more generic as I described in my
previous comment. You don't need to make it receive and process notifications.
Just rename it PushNotificationSubscriber and pass list of subscriptions from
outside instead of hardcoding it in this class.

https://codereview.chromium.org/1123153002/diff/60001/remoting/signaling/jid_...
File remoting/signaling/jid_util.cc (right):

https://codereview.chromium.org/1123153002/diff/60001/remoting/signaling/jid_...
remoting/signaling/jid_util.cc:23: if (bare_jid != nullptr) {
nit: don't need "!=nullptr" part

https://codereview.chromium.org/1123153002/diff/60001/remoting/signaling/jid_...
remoting/signaling/jid_util.cc:30: } else {
don't need else because the if above always returns.
See last item in
https://www.chromium.org/developers/coding-style#Code_Formatting

https://codereview.chromium.org/1123153002/diff/60001/remoting/signaling/jid_...
File remoting/signaling/jid_util.h (right):

https://codereview.chromium.org/1123153002/diff/60001/remoting/signaling/jid_...
remoting/signaling/jid_util.h:21: // e.g. "user@domain/resource" ->
"user@domain", "/resource", true
I think it's better to remove / from the returned resource, i.e. return
"resource" instead of "/resource". / is not part of the resource.

[John Williams, 2015-05-15 21:59:43]

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
File remoting/host/gcd_notification_subscriber.h (right):

https://codereview.chromium.org/1123153002/diff/1/remoting/host/gcd_notificat...
remoting/host/gcd_notification_subscriber.h:23: class GcdNotificationSubscriber
: public SignalStrategy::Listener {
On 2015/05/15 18:22:36, Sergey Ulanov wrote:
> On 2015/05/15 00:51:12, John Williams wrote:
> > On 2015/05/07 18:22:38, Sergey Ulanov wrote:
> > > Is the intent to make have a separate class receive notifications? If yes,
> > then
> > > why? I'd prefer having the same class subscribe and listen for
> notifications.
> > > And then it should be called Receiver instead of Subscriber.
> > > 
> > > I think this class can be more generic and usable for any push
> notifications.
> > It
> > > doesn't need to be GCD-specific. Maybe call it PushNotificationReceiver
and
> > make
> > > it work for any push notifications?
> > > 
> > > E.g. see jingle/notifier/listener/push_notifications_subscribe_task.cc -
> that
> > > code is used by sync and CloudPrint to receive push notification.
Similarly
> > you
> > > can pass list of notifications the caller want's to receive.
> > > jingle/notifier is in a semi-deprecated state (because the XMPP code it
> > depends
> > > on in WebRTC is deprecated). In future I'd like to move remoting/signaling
> > > somewhere where it can be reused by other components (e.g.
components/xmpp)
> > and
> > > then this class will be a replacement for the current push notifications
> code
> > in
> > > jingle/notifier. 
> > > 
> > > Also please move this file to remoting/signaling.
> > 
> > The reason I called it "subscriber" is that it literally just subscribes; it
> > doesn't even have any logic to detect when an XMPP stanza is a notification,
> > because none of the notifications GCD sends are useful to us. It's likely
> we'll
> > never need to process GCD notifications, since we're sending commands over
the
> > raw XMPP channel rather than using GCD's "command" API.
> 
> I still think it's worth making this class more generic as I described in my
> previous comment. You don't need to make it receive and process notifications.
> Just rename it PushNotificationSubscriber and pass list of subscriptions from
> outside instead of hardcoding it in this class.

Done.

https://codereview.chromium.org/1123153002/diff/60001/remoting/signaling/jid_...
File remoting/signaling/jid_util.cc (right):

https://codereview.chromium.org/1123153002/diff/60001/remoting/signaling/jid_...
remoting/signaling/jid_util.cc:23: if (bare_jid != nullptr) {
On 2015/05/15 18:22:37, Sergey Ulanov wrote:
> nit: don't need "!=nullptr" part

Done.

https://codereview.chromium.org/1123153002/diff/60001/remoting/signaling/jid_...
remoting/signaling/jid_util.cc:30: } else {
On 2015/05/15 18:22:37, Sergey Ulanov wrote:
> don't need else because the if above always returns.
> See last item in
> https://www.chromium.org/developers/coding-style#Code_Formatting

Done.

https://codereview.chromium.org/1123153002/diff/60001/remoting/signaling/jid_...
File remoting/signaling/jid_util.h (right):

https://codereview.chromium.org/1123153002/diff/60001/remoting/signaling/jid_...
remoting/signaling/jid_util.h:21: // e.g. "user@domain/resource" ->
"user@domain", "/resource", true
On 2015/05/15 18:22:37, Sergey Ulanov wrote:
> I think it's better to remove / from the returned resource, i.e. return
> "resource" instead of "/resource". / is not part of the resource.

Done.

[Sergey Ulanov, 2015-05-16 00:56:08]

https://codereview.chromium.org/1123153002/diff/80001/remoting/signaling/jid_...
File remoting/signaling/jid_util.cc (right):

https://codereview.chromium.org/1123153002/diff/80001/remoting/signaling/jid_...
remoting/signaling/jid_util.cc:34: if (bare_jid != nullptr) {
don't need != nullptr

https://codereview.chromium.org/1123153002/diff/80001/remoting/signaling/jid_...
remoting/signaling/jid_util.cc:35: *bare_jid = std::string(full_jid, 0,
slash_index);
use substr() here and to extract resource?

https://codereview.chromium.org/1123153002/diff/80001/remoting/signaling/push...
File remoting/signaling/push_notification_subscriber.cc (right):

https://codereview.chromium.org/1123153002/diff/80001/remoting/signaling/push...
remoting/signaling/push_notification_subscriber.cc:18: const char
kNsGooglePush[] = "google:push";
maybe call it kGooglePushNamespace?

https://codereview.chromium.org/1123153002/diff/80001/remoting/signaling/push...
remoting/signaling/push_notification_subscriber.cc:36: HOST_LOG << "Subscribing
to push notifications on channel: "
I think this should be VLOG()

https://codereview.chromium.org/1123153002/diff/80001/remoting/signaling/push...
remoting/signaling/push_notification_subscriber.cc:48:
item_element->SetAttr(buzz::QName("", "channel"), push_channel_);
here and everywhere else: use std::string() instead of ""

https://codereview.chromium.org/1123153002/diff/80001/remoting/signaling/push...
File remoting/signaling/push_notification_subscriber.h (right):

https://codereview.chromium.org/1123153002/diff/80001/remoting/signaling/push...
remoting/signaling/push_notification_subscriber.h:25: const std::string&
push_channel);
Can you please replace this with a list of subscriptions? see
https://code.google.com/p/chromium/codesearch#chromium/src/jingle/notifier/li...
for example

[John Williams, 2015-05-16 02:10:07]

https://codereview.chromium.org/1123153002/diff/80001/remoting/signaling/jid_...
File remoting/signaling/jid_util.cc (right):

https://codereview.chromium.org/1123153002/diff/80001/remoting/signaling/jid_...
remoting/signaling/jid_util.cc:34: if (bare_jid != nullptr) {
On 2015/05/16 00:56:07, Sergey Ulanov wrote:
> don't need != nullptr

Done.

https://codereview.chromium.org/1123153002/diff/80001/remoting/signaling/jid_...
remoting/signaling/jid_util.cc:35: *bare_jid = std::string(full_jid, 0,
slash_index);
On 2015/05/16 00:56:07, Sergey Ulanov wrote:
> use substr() here and to extract resource?

Done.

https://codereview.chromium.org/1123153002/diff/80001/remoting/signaling/push...
File remoting/signaling/push_notification_subscriber.cc (right):

https://codereview.chromium.org/1123153002/diff/80001/remoting/signaling/push...
remoting/signaling/push_notification_subscriber.cc:18: const char
kNsGooglePush[] = "google:push";
On 2015/05/16 00:56:08, Sergey Ulanov wrote:
> maybe call it kGooglePushNamespace?

Done.

https://codereview.chromium.org/1123153002/diff/80001/remoting/signaling/push...
remoting/signaling/push_notification_subscriber.cc:36: HOST_LOG << "Subscribing
to push notifications on channel: "
On 2015/05/16 00:56:08, Sergey Ulanov wrote:
> I think this should be VLOG()

Done.

https://codereview.chromium.org/1123153002/diff/80001/remoting/signaling/push...
remoting/signaling/push_notification_subscriber.cc:48:
item_element->SetAttr(buzz::QName("", "channel"), push_channel_);
On 2015/05/16 00:56:08, Sergey Ulanov wrote:
> here and everywhere else: use std::string() instead of ""

Done.

https://codereview.chromium.org/1123153002/diff/80001/remoting/signaling/push...
File remoting/signaling/push_notification_subscriber.h (right):

https://codereview.chromium.org/1123153002/diff/80001/remoting/signaling/push...
remoting/signaling/push_notification_subscriber.h:25: const std::string&
push_channel);
On 2015/05/16 00:56:08, Sergey Ulanov wrote:
> Can you please replace this with a list of subscriptions? see
>
https://code.google.com/p/chromium/codesearch#chromium/src/jingle/notifier/li...
> for example

Done.

[Sergey Ulanov, 2015-05-19 20:48:52]

lgtm

[John Williams, 2015-05-19 21:13:32]

The CQ bit was checked by jrw@chromium.org

[John Williams, 2015-05-19 21:13:33]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/1123153002/#ps120001
(title: "sync to head")

[commit-bot: I haz the power, 2015-05-19 21:14:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1123153002/120001

[commit-bot: I haz the power, 2015-05-19 22:09:53]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-05-19 22:10:42]

Patchset 7 (id:??) landed as
https://crrev.com/cb58e5e73fb79375392544576e345ed4133773af
Cr-Commit-Position: refs/heads/master@{#330628}