WebOTP: Comments + cleanup

This change adds cleanup and comments we discussed during
code walk through.

Change-Id: I0b5ce06501257c1285c7e907d77fd942bbc076f1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2140906Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Ayu Ishii <ayui@chromium.org>
Cr-Commit-Position: refs/heads/master@{#758103}

content/browser/sms/sms_fetcher_impl.cc

@@ -54,9 +54,8 @@ SmsFetcher* SmsFetcher::Get(BrowserContext* context,
 void SmsFetcherImpl::Subscribe(const url::Origin& origin,
                                SmsQueue::Subscriber* subscriber) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
-  if (subscribers_.HasSubscriber(origin, subscriber))
-    return;
+  // Should not be called multiple times for the same subscriber and origin.
+  DCHECK(!subscribers_.HasSubscriber(origin, subscriber));
 
   subscribers_.Push(origin, subscriber);
 
@@ -73,6 +72,8 @@ void SmsFetcherImpl::Subscribe(const url::Origin& origin,
 
 void SmsFetcherImpl::Unsubscribe(const url::Origin& origin,
                                  SmsQueue::Subscriber* subscriber) {
+  // Unsubscribe does not make a call to the provider because currently there
+  // is no mechanism to cancel a subscription.
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   subscribers_.Remove(origin, subscriber);
 }
@@ -80,13 +81,13 @@ void SmsFetcherImpl::Unsubscribe(const url::Origin& origin,
 bool SmsFetcherImpl::Notify(const url::Origin& origin,
                             const std::string& one_time_code) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  // The received OTP is returned to the first subscriber for the origin.
   auto* subscriber = subscribers_.Pop(origin);
 
   if (!subscriber)
     return false;
 
   subscriber->OnReceive(one_time_code);
-
   return true;
 }
 

content/browser/sms/sms_fetcher_impl_unittest.cc

@@ -200,20 +200,4 @@ TEST_F(SmsFetcherImplTest, TwoOriginsTwoSubscribers) {
   provider()->NotifyReceive(kOrigin1, "123");
 }
 
-TEST_F(SmsFetcherImplTest, SubscribeIsIdempotent) {
-  const url::Origin kOrigin = url::Origin::Create(GURL("https://a.com"));
-
-  StrictMock<MockSubscriber> subscriber;
-
-  SmsFetcherImpl fetcher(nullptr, base::WrapUnique(provider()));
-  fetcher.Subscribe(kOrigin, &subscriber);
-  fetcher.Subscribe(kOrigin, &subscriber);
-
-  EXPECT_TRUE(fetcher.HasSubscribers());
-
-  fetcher.Unsubscribe(kOrigin, &subscriber);
-
-  EXPECT_FALSE(fetcher.HasSubscribers());
-}
-
 }  // namespace content

content/browser/sms/sms_service.cc

@@ -35,7 +35,9 @@ SmsService::SmsService(
     mojo::PendingReceiver<blink::mojom::SmsReceiver> receiver)
     : FrameServiceBase(host, std::move(receiver)),
       fetcher_(fetcher),
-      origin_(origin) {}
+      origin_(origin) {
+  DCHECK(fetcher_);
+}
 
 SmsService::SmsService(
     SmsFetcher* fetcher,
@@ -49,6 +51,7 @@ SmsService::SmsService(
 SmsService::~SmsService() {
   if (callback_)
     Process(SmsStatus::kTimeout, base::nullopt);
+  DCHECK(!callback_);
 }
 
 // static

content/public/browser/sms_fetcher.h

@@ -25,6 +25,9 @@ class RenderFrameHost;
 // There is one SmsFetcher per profile.
 class SmsFetcher {
  public:
+  SmsFetcher() = default;
+  virtual ~SmsFetcher() = default;
+
   // Retrieval for devices that exclusively listen for SMSes coming from other
   // telephony devices. (eg. desktop)
   CONTENT_EXPORT static SmsFetcher* Get(BrowserContext* context);