[NTP::Push] Add Content Suggestions GCM App Handler

components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h

+// Copyright 2017 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 COMPONENTS_NTP_SNIPPETS_REMOTE_CONTENT_SUGGESTIONS_GCM_APP_HANDLER_H_
+#define COMPONENTS_NTP_SNIPPETS_REMOTE_CONTENT_SUGGESTIONS_GCM_APP_HANDLER_H_

[Peter Beverloo, 2017/06/02 14:02:03]

micro nit: update

[mamir, 2017/06/04 15:10:14]

Done.

+
+#include "base/memory/weak_ptr.h"
+#include "components/gcm_driver/gcm_app_handler.h"
+#include "components/gcm_driver/instance_id/instance_id.h"
+#include "components/ntp_snippets/breaking_news/breaking_news_subscription_manager.h"
+
+namespace gcm {
+class GCMDriver;
+}
+
+namespace instance_id {
+class InstanceIDDriver;
+}
+
+namespace ntp_snippets {
+// Class responsible for GCM functionality. It retrieves a subscription token

[sfiera, 2017/06/02 13:41:24]

Here and below, I think it's normal style to put a blank line before comments,
to ensure they group visually with thing after and not the thing before.

[mamir, 2017/06/04 15:10:14]

Done.

+// from the GCM server, registers/deregisters itself with the GCM service to be

[Peter Beverloo, 2017/06/02 14:02:02]

micro nit: deregisters -> unregisters (or "subscribes/unsubscribes" if you'd
like to use the latest terminology, but the rest of Chrome hasn't updated to
that yet.)

[mamir, 2017/06/04 15:10:14]

I will stay consistent with the rest of Chrome.

+// called upon received push content suggestions.
+class ContentSuggestionsGCMAppHandler : public gcm::GCMAppHandler {
+ public:
+  ContentSuggestionsGCMAppHandler(
+      gcm::GCMDriver* gcm_driver,
+      instance_id::InstanceIDDriver* instance_id_driver,
+      std::unique_ptr<BreakingNewsSubscriptionManager> subscriptionManager);

[Peter Beverloo, 2017/06/02 14:02:03]

nit: subscription_manager

[mamir, 2017/06/04 15:10:14]

Done.

+  ~ContentSuggestionsGCMAppHandler();

[sfiera, 2017/06/02 13:41:24]

Comment: if still listening, calls StopListening()

[Peter Beverloo, 2017/06/02 14:02:02]

nit: this should be marked as `override` since the parent class' destructor is
virtual

[mamir, 2017/06/04 15:10:14]

Done.

[mamir, 2017/06/04 15:10:14]

Done.

+  // Subscribe to the GCM service if necessary and start listening for pushed
+  // content suggestions. Must not be called if already listening.
+  void StartListening();
+  // Remove the handler, and stop listening for incoming GCM messages. Any
+  // further pushed content suggestions will be ignored. Must be called while
+  // listening.
+  void StopListening();
+
+ private:
+  gcm::GCMDriver* const gcm_driver_;

[Peter Beverloo, 2017/06/02 14:02:02]

style nit: methods before member variables.

[mamir, 2017/06/04 15:10:14]

Done.

+  instance_id::InstanceIDDriver* const instance_id_driver_;

[Peter Beverloo, 2017/06/02 14:02:03]

In order to guarantee the lifetime of these two pointers, it is important that
whatever class will own this (which presumably can be traced back to a
KeyedService too?) declares a dependency on the GCMProfileServiceFactory and the
InstanceIDProfileServiceFactory.

[mamir, 2017/06/04 15:10:14]

I added it to the constructor. This is enough.

[mamir, 2017/06/04 16:19:01]

Actually, GCMProfileServiceFactory (and InstanceIDProfileServiceFactory) is in
chrome/browser and IIUC components shouldn't depends on browser.

+  const std::unique_ptr<BreakingNewsSubscriptionManager> subscription_manager_;
+  base::WeakPtrFactory<ContentSuggestionsGCMAppHandler> weak_factory_;
+  // Retrieves a subscription token that allows the content suggestions server
+  // to push content via GCM messages. Calling this method multiple times is not
+  // necessary but does not harm since the same token is returned everytime.
+  void Subscribe();
+  // Called after the subscription is obtained from the GCM server.
+  void DidSubscribe(const std::string& subscription_id,
+                    instance_id::InstanceID::Result result);
+
+  // GCMAppHandler overrides.
+  void ShutdownHandler() override;
+  void OnStoreReset() override;
+  void OnMessage(const std::string& app_id,
+                 const gcm::IncomingMessage& message) override;
+  void OnMessagesDeleted(const std::string& app_id) override;
+  void OnSendError(const std::string& app_id,
+                   const gcm::GCMClient::SendErrorDetails& details) override;
+  void OnSendAcknowledged(const std::string& app_id,
+                          const std::string& message_id) override;

[Peter Beverloo, 2017/06/02 14:02:02]

It doesn't per se hurt, but given that the parent class defines these as public
they'll just be accessible, so you may as well move them to the `public`
section.

[mamir, 2017/06/04 15:10:14]

Done.

+};

[Peter Beverloo, 2017/06/02 14:02:02]

DISALLOW_COPY_AND_ASSIGN

[mamir, 2017/06/04 15:10:14]

Done.

+}  // namespace ntp_snippets
+
+#endif  // COMPONENTS_NTP_SNIPPETS_REMOTE_CONTENT_SUGGESTIONS_GCM_APP_HANDLER_H_