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

components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.cc

+// 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.
+
+#include "components/ntp_snippets/breaking_news/content_suggestions_gcm_app_handler.h"
+
+#include "base/strings/string_util.h"
+#include "components/gcm_driver/gcm_driver.h"
+#include "components/gcm_driver/gcm_profile_service.h"
+#include "components/gcm_driver/instance_id/instance_id.h"
+#include "components/gcm_driver/instance_id/instance_id_driver.h"
+#include "content/public/common/push_messaging_status.h"

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

This is specific to the Push API, I wouldn't add a dependency on
//content/public/ just to use the enumeration of another feature, most of which
isn't applicable to you. Consider just using InstanceID::Result unless you
really need your own layer of status codes -- the former provides the best
insight in case of GCM Driver-related issues.

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

Ah, sorry.
I guess you can guess how this code came here by mistake ;-)

+
+using instance_id::InstanceID;
+
+namespace ntp_snippets {
+
+const char* kContentSuggestionsGCMAppID = "com.google.contentsuggestions.gcm";
+// The sender ID is used in the registration process.
+// See: https://developers.google.com/cloud-messaging/gcm#senderid
+const char* kContentSuggestionsGCMSenderId = "128223710667";

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

style nit: define string constants as:

  const char kFoo[] = "bar";

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

Done.

+
+// OAuth2 Scope passed to getToken to obtain GCM registration tokens.
+// Must match Java GoogleCloudMessaging.INSTANCE_ID_SCOPE.
+const char kGCMScope[] = "GCM";
+
+ContentSuggestionsGCMAppHandler::ContentSuggestionsGCMAppHandler(
+    gcm::GCMDriver* gcm_driver,
+    instance_id::InstanceIDDriver* instance_id_driver,
+    std::unique_ptr<BreakingNewsSubscriptionManager> subscription_manager)
+    : gcm_driver_(gcm_driver),
+      instance_id_driver_(instance_id_driver),
+      subscription_manager_(std::move(subscription_manager)),
+      weak_factory_(this) {
+  Subscribe();

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

The subscription isn't necessary until StartListening(), is it? I would defer
until then.

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

+1. May I also ask you to consider adding the following code in
StartListening(), *before* adding the app handler and/or creating the
subscription:

#if !defined(OS_ANDROID)
  NOTREACHED()
      << "The ContentSuggestionsGCMAppHandler should only be used on Android.";
#endif

It's a bit yuk, but having something that paints the bots red if we enable this
on non-Android platforms by accident is important. Under the same banner, it'd
be great if the connection could be guarded behind a base::Feature so that
there's an explicit kill switch.

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

The whole content provider instantiation will be guarded behind a base::Feature.
Is this enough? or you mean something else by guarding the connection?

+}
+ContentSuggestionsGCMAppHandler::~ContentSuggestionsGCMAppHandler() {
+  if (gcm_driver_->GetAppHandler(kContentSuggestionsGCMAppID) == this) {
+    StopListening();

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

Something to consider: it's safe to just always call RemoveAppHandler.

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

That because AddAppHandler DCHECKs?
So we are sure no other handler has been registered to handle content
suggestions pushes?

[Peter Beverloo, 2017/06/08 07:02:39]

No - calling RemoveAppHandler() with an unregistered app id will just silently
fail, which is fine.

[mamir, 2017/06/08 10:31:38]

Done.

+  }
+}
+
+void ContentSuggestionsGCMAppHandler::StartListening() {
+  DCHECK(!gcm_driver_->GetAppHandler(kContentSuggestionsGCMAppID));
+  if (gcm_driver_->GetAppHandler(kContentSuggestionsGCMAppID) == this) {

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

Redundant. By the DCHECK(), we already know that there is no handler installed.

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

AddAppHandler() itself will DCHECK, so you can remove the DCHECK here too. The
comment in the class' header file is great!

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

I got confused.  So we remove the whole if check as well. Just add the handler
directly?

[Peter Beverloo, 2017/06/08 07:02:39]

Yes.

[mamir, 2017/06/08 10:31:38]

Done.

+    // GCM app handler already added.
+    return;
+  }
+  gcm_driver_->AddAppHandler(kContentSuggestionsGCMAppID, this);
+}
+
+void ContentSuggestionsGCMAppHandler::StopListening() {
+  DCHECK_EQ(gcm_driver_->GetAppHandler(kContentSuggestionsGCMAppID), this);
+  gcm_driver_->RemoveAppHandler(kContentSuggestionsGCMAppID);
+}
+
+void ContentSuggestionsGCMAppHandler::Subscribe() {
+  // TODO (mamir): Check the prefs for the cached subscription_id before calling
+  // the GCM service.
+  DVLOG(1) << "Implement subscription token caching.";
+  instance_id_driver_->GetInstanceID(kContentSuggestionsGCMAppID)
+      ->GetToken(kContentSuggestionsGCMSenderId, kGCMScope,
+                 std::map<std::string, std::string>() /* options */,
+                 base::Bind(&ContentSuggestionsGCMAppHandler::DidSubscribe,
+                            weak_factory_.GetWeakPtr()));
+}
+
+void ContentSuggestionsGCMAppHandler::DidSubscribe(
+    const std::string& subscription_id,
+    InstanceID::Result result) {
+  content::PushRegistrationStatus status =
+      content::PUSH_REGISTRATION_STATUS_SERVICE_ERROR;
+
+  switch (result) {
+    case InstanceID::SUCCESS:
+      // TODO (mamir): cache the subscription_id in prefs.
+      DVLOG(1) << "Implement subscription token caching.";

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

nit: we usually don't add D(V)LOG()s for TODOs.

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

Done.

+      subscription_manager_->Subscribe(subscription_id);
+      return;
+    case InstanceID::INVALID_PARAMETER:
+    case InstanceID::DISABLED:
+    case InstanceID::ASYNC_OPERATION_PENDING:
+    case InstanceID::SERVER_ERROR:
+    case InstanceID::UNKNOWN_ERROR:
+      DLOG(WARNING)
+          << "Push messaging subscription failed; InstanceID::Result = "
+          << result;
+      status = content::PUSH_REGISTRATION_STATUS_SERVICE_ERROR;
+      break;
+    case InstanceID::NETWORK_ERROR:
+      status = content::PUSH_REGISTRATION_STATUS_NETWORK_ERROR;
+      break;
+  }
+  DLOG(WARNING) << "Status = " << status;
+}
+
+void ContentSuggestionsGCMAppHandler::ShutdownHandler() {}
+
+void ContentSuggestionsGCMAppHandler::OnStoreReset() {}

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

You need to clear your token caching in preferences when this happens.

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

Added a TODO for now.

+
+void ContentSuggestionsGCMAppHandler::OnMessage(
+    const std::string& app_id,
+    const gcm::IncomingMessage& message) {
+  // TODO (mamir): Implement Show notification and update the feed.

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

Nit: I think TODO() should have no space (like a function)

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

Done.

+}
+
+void ContentSuggestionsGCMAppHandler::OnMessagesDeleted(
+    const std::string& app_id) {
+  // Messages don't get deleted.

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

fwiw, better to be strict and NOTREACHED() in that case. Same with the two
following methods.

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

Done.

+}
+
+void ContentSuggestionsGCMAppHandler::OnSendError(
+    const std::string& app_id,
+    const gcm::GCMClient::SendErrorDetails& details) {
+  // Should never be called because we don't send GCM messages to
+  // the server.
+}
+
+void ContentSuggestionsGCMAppHandler::OnSendAcknowledged(
+    const std::string& app_id,
+    const std::string& message_id) {
+  // Should never be called because we don't send GCM messages to
+  // the server.
+}
+
+}  // namespace ntp_snippets