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

components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h

+// Copyright 2016 The Chromium Authors. All rights reserved.

[sfiera, 2017/06/02 08:43:05]

2017

[mamir, 2017/06/02 13:13:57]

Done.

+// 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_
+
+#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 ContentSuggestionsGCMAppHandler : public gcm::GCMAppHandler {
+ private:

[sfiera, 2017/06/02 08:43:05]

Public declarations first

[mamir, 2017/06/02 13:13:57]

Done.

+  gcm::GCMDriver* gcm_driver_;

[sfiera, 2017/06/02 08:43:05]

I'm all in favor of declaring "gcm::GCMDriver* const gcm_driver_" and same for
subscription_manager_.

If you declare a member variable const and you don't initialize it in the
constructor, the compiler will warn you.

[mamir, 2017/06/02 13:13:57]

Done.

+  std::unique_ptr<BreakingNewsSubscriptionManager> subscription_manager_;
+
+  base::WeakPtrFactory<ContentSuggestionsGCMAppHandler> weak_factory_;
+
+ public:
+  ContentSuggestionsGCMAppHandler(
+      gcm::GCMDriver* gcm_driver,
+      std::unique_ptr<BreakingNewsSubscriptionManager> subscriptionManager);
+  void DoSubscribe(instance_id::InstanceIDDriver* instance_id_driver);

[sfiera, 2017/06/02 08:43:05]

Please add some comments to these methods.

Are DoSubscribe() and Shutdown() intended to be used by callers?
Is it OK to call them multiple times?
What's valid after Shutdown()? Can you re-subscribe?
Can DoSubscribe() fail? What happens then?

[mamir, 2017/06/02 13:13:57]

PTAL

+  void DidSubscribe(const std::string& subscription_id,

[sfiera, 2017/06/02 08:43:05]

Private?

[mamir, 2017/06/02 13:13:57]

Done.

+                    instance_id::InstanceID::Result result);
+
+  void Shutdown();
+
+  // GCMAppHandler overrides.

[sfiera, 2017/06/02 08:43:05]

Typically we place these sorts of overrides in the private section, because
they're not part of the public interface. Many people have mixed feelings about
this, and you don't have to if you find it weird, but it's something we
typically do :)

[mamir, 2017/06/02 13:13:57]

Done.

+  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;
+};
+}  // namespace ntp_snippets
+
+#endif  // COMPONENTS_NTP_SNIPPETS_REMOTE_CONTENT_SUGGESTIONS_GCM_APP_HANDLER_H_