[NTP::Push] Adding Breaking News Subscription Manager

components/ntp_snippets/breaking_news/breaking_news_subscription_manager.cc

Index: components/ntp_snippets/breaking_news/breaking_news_subscription_manager.cc
diff --git a/components/ntp_snippets/breaking_news/breaking_news_subscription_manager.cc b/components/ntp_snippets/breaking_news/breaking_news_subscription_manager.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b4bc3f43d96a6849c05d1cf8f789508e645a45bc
--- /dev/null
+++ b/components/ntp_snippets/breaking_news/breaking_news_subscription_manager.cc
@@ -0,0 +1,39 @@
+// 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/breaking_news_subscription_manager.h"
+#include "base/bind.h"
+#include "components/ntp_snippets/breaking_news/subscription_json_request.h"
+
+namespace ntp_snippets {
+
+using internal::SubscriptionJsonRequest;
+
+BreakingNewsSubscriptionManager::BreakingNewsSubscriptionManager(
+    scoped_refptr<net::URLRequestContextGetter> url_request_context_getter)
+    : url_request_context_getter_(std::move(url_request_context_getter)) {}
+
+void BreakingNewsSubscriptionManager::Subscribe(const std::string& token) {
+  SubscriptionJsonRequest::Builder builder;
+  builder.SetToken(token)
+      .SetUrlRequestContextGetter(url_request_context_getter_)
+      .SetUrl(subscribe_url_);
+
+  std::unique_ptr<SubscriptionJsonRequest> request = builder.Build();
+  SubscriptionJsonRequest* raw_request = request.get();
+  raw_request->Start(
+      base::BindOnce(&BreakingNewsSubscriptionManager::SubscribeDone,
+                     base::Unretained(this), std::move(request)));

[fhorschig, 2017/06/02 09:11:49]

I see that you did, what I did. Having aged 6 month, I  wouldn't take 
that path. I would (and will for my existing class) either:

a) Create a set/vector of requests as member and only pass on a ID, or
whatever helps me find/use/destroy the request. This is verbose and
destroying the manager has to cancel the request, but it makes 
ownership clear and makes failure cases easier to understand/debug.

b) Dive deep into ownership of pointers and make sure the callback
destroys all parameters if it's destroyed itself [1]. 
Also, double-check whether base::Unretained(this) can blow up if the
manager is destroyed before the request returns.
My assumption is, that this would end up using a weak_ptr [2] 
instead of base::Unretained. Also, base::Passed or base::Owned are
likely necessary to describe the right form of ownership [3].
Be aware that the request could in this case outlive the manager.


----


Some details why I would probably try a):
The callback has to own the request if the manager doesn't. 
This basically works. But if it fails, ASan will complain and 
you (or maintainers) will have a hard time debugging this.
And failing means in this case: 
If (in b) ) the manager is destroyed before the callback, the moved
request is basically owner-less. You basically create a leak if the
owning callback is not called in just one case.

[1]
https://www.chromium.org/developers/coding-style/important-abstractions-and-d...
[2]
https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h?type=cs&q=basE::w...
[3]
https://cs.chromium.org/chromium/src/base/bind_helpers.h?type=cs&q=base::Unre...

[mamir, 2017/06/04 10:17:03]

I have changed the design as per our offline discussion.

+}
+
+void BreakingNewsSubscriptionManager::SubscribeDone(
+    std::unique_ptr<SubscriptionJsonRequest> request,
+    const ntp_snippets::Status& status) {
+  // TODO(mamir): Handle success and failure cases.
+}
+
+void BreakingNewsSubscriptionManager::Unsubscribe(const std::string& token) {
+  // TODO(mamir): Implement.
+}
+}  // namespace ntp_snippets