[sms] Refactor SmsDialog to use event handler

This CL refactors SmsDialog to use event handler. An enum SmsDialogEvent is added to SmsDialog class and can be used by both C++ and Java code. Bug: 985441 Change-Id: I1ecc4e70609479124c80a7166a9551ffa77566a7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1737875 Commit-Queue: Jun Cai <juncai@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Reilly Grant <reillyg@chromium.org> Reviewed-by: Ayu Ishii <ayui@chromium.org> Cr-Commit-Position: refs/heads/master@{#686524}
chromium · Aug 13, 2019 · 34b6053 · 34b6053
1 parent 879306f
commit 34b6053
Show file tree

Hide file tree

Showing 11 changed files with 144 additions and 140 deletions.
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/sms/SmsReceiverDialog.java b/chrome/android/java/src/org/chromium/chrome/browser/sms/SmsReceiverDialog.java
@@ -23,6 +23,7 @@
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.NativeMethods;
 import org.chromium.chrome.R;
+import org.chromium.content_public.browser.sms.Event;
 import org.chromium.ui.base.WindowAndroid;
 
 /**
@@ -72,13 +73,13 @@ private void createAndShowDialog(WindowAndroid windowAndroid) {
         Button cancelButton = (Button) dialogContainer.findViewById(R.id.cancel_button);
         cancelButton.setOnClickListener(v -> {
             assert mNativeSmsDialogAndroid != 0;
-            SmsReceiverDialogJni.get().onCancel(mNativeSmsDialogAndroid);
+            SmsReceiverDialogJni.get().onEvent(mNativeSmsDialogAndroid, Event.CANCEL);
         });
 
         mConfirmButton = (Button) dialogContainer.findViewById(R.id.confirm_button);
         mConfirmButton.setOnClickListener(v -> {
             assert mNativeSmsDialogAndroid != 0;
-            SmsReceiverDialogJni.get().onConfirm(mNativeSmsDialogAndroid);
+            SmsReceiverDialogJni.get().onEvent(mNativeSmsDialogAndroid, Event.CONFIRM);
         });
 
         mDialog = new Dialog(mActivity);
@@ -149,7 +150,6 @@ boolean isDialogDestroyedForTesting() {
 
     @NativeMethods
     interface Natives {
-        void onCancel(long nativeSmsDialogAndroid);
-        void onConfirm(long nativeSmsDialogAndroid);
+        void onEvent(long nativeSmsDialogAndroid, int eventType);
     }
 }
diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/sms/SmsReceiverDialogTest.java b/chrome/android/javatests/src/org/chromium/chrome/browser/sms/SmsReceiverDialogTest.java
@@ -27,6 +27,7 @@
 import org.chromium.chrome.browser.ChromeSwitches;
 import org.chromium.chrome.test.ChromeActivityTestRule;
 import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
+import org.chromium.content_public.browser.sms.Event;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;
 import org.chromium.content_public.browser.test.util.TestTouchUtils;
 import org.chromium.ui.base.ActivityWindowAndroid;
@@ -52,13 +53,17 @@ public class SmsReceiverDialogTest {
 
     private class TestSmsReceiverDialogJni implements SmsReceiverDialog.Natives {
         @Override
-        public void onCancel(long nativeSmsDialogAndroid) {
-            mCancelButtonClickedCallback.notifyCalled();
-        }
-
-        @Override
-        public void onConfirm(long nativeSmsDialogAndroid) {
-            mConfirmButtonClickedCallback.notifyCalled();
+        public void onEvent(long nativeSmsDialogAndroid, int eventType) {
+            switch (eventType) {
+                case Event.CANCEL:
+                    mCancelButtonClickedCallback.notifyCalled();
+                    return;
+                case Event.CONFIRM:
+                    mConfirmButtonClickedCallback.notifyCalled();
+                    return;
+                default:
+                    assert false : "|eventType| is invalid";
+            }
         }
     }
 

diff --git a/chrome/browser/ui/android/sms/sms_dialog_android.cc b/chrome/browser/ui/android/sms/sms_dialog_android.cc
@@ -32,10 +32,8 @@ SmsDialogAndroid::~SmsDialogAndroid() {
 }
 
 void SmsDialogAndroid::Open(content::RenderFrameHost* host,
-                            base::OnceClosure on_confirm,
-                            base::OnceClosure on_cancel) {
-  on_confirm_ = std::move(on_confirm);
-  on_cancel_ = std::move(on_cancel);
+                            EventHandler handler) {
+  handler_ = std::move(handler);
 
   content::WebContents* web_contents =
       content::WebContents::FromRenderFrameHost(host);
@@ -55,10 +53,6 @@ void SmsDialogAndroid::SmsReceived() {
   Java_SmsReceiverDialog_smsReceived(AttachCurrentThread(), java_dialog_);
 }
 
-void SmsDialogAndroid::OnConfirm(JNIEnv* env) {
-  std::move(on_confirm_).Run();
-}
-
-void SmsDialogAndroid::OnCancel(JNIEnv* env) {
-  std::move(on_cancel_).Run();
+void SmsDialogAndroid::OnEvent(JNIEnv* env, jint event_type) {
+  std::move(handler_).Run(static_cast<Event>(event_type));
 }
diff --git a/chrome/browser/ui/android/sms/sms_dialog_android.h b/chrome/browser/ui/android/sms/sms_dialog_android.h
@@ -19,22 +19,16 @@ class SmsDialogAndroid : public content::SmsDialog {
   explicit SmsDialogAndroid(const url::Origin& origin);
   ~SmsDialogAndroid() override;
 
-  void Open(content::RenderFrameHost*,
-            base::OnceClosure on_confirm,
-            base::OnceClosure on_cancel) override;
+  void Open(content::RenderFrameHost*, EventHandler handler) override;
   void Close() override;
   void SmsReceived() override;
 
-  // Report the user manually clicks the 'Confirm' button.
-  void OnConfirm(JNIEnv* env);
-
-  // Report the user manually dismisses the SMS dialog.
-  void OnCancel(JNIEnv* env);
+  // Report the user's action through |event_type|.
+  void OnEvent(JNIEnv* env, jint event_type);
 
  private:
   base::android::ScopedJavaGlobalRef<jobject> java_dialog_;
-  base::OnceClosure on_confirm_;
-  base::OnceClosure on_cancel_;
+  EventHandler handler_;
   DISALLOW_COPY_AND_ASSIGN(SmsDialogAndroid);
 };
 

diff --git a/content/browser/sms/sms_browsertest.cc b/content/browser/sms/sms_browsertest.cc
@@ -72,20 +72,19 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, Receive) {
 
   auto* dialog = new NiceMock<MockSmsDialog>();
 
-  base::OnceClosure on_confirm_callback;
+  SmsDialog::EventHandler hdl;
 
   EXPECT_CALL(delegate_, CreateSmsDialog(_))
       .WillOnce(Return(ByMove(base::WrapUnique(dialog))));
 
-  EXPECT_CALL(*dialog, Open(_, _, _))
-      .WillOnce(Invoke([&on_confirm_callback](content::RenderFrameHost*,
-                                              base::OnceClosure on_confirm,
-                                              base::OnceClosure on_cancel) {
-        on_confirm_callback = std::move(on_confirm);
-      }));
+  EXPECT_CALL(*dialog, Open(_, _))
+      .WillOnce(
+          Invoke([&hdl](RenderFrameHost*, SmsDialog::EventHandler handler) {
+            hdl = std::move(handler);
+          }));
 
-  EXPECT_CALL(*dialog, SmsReceived()).WillOnce(Invoke([&on_confirm_callback]() {
-    std::move(on_confirm_callback).Run();
+  EXPECT_CALL(*dialog, SmsReceived()).WillOnce(Invoke([&hdl]() {
+    std::move(hdl).Run(SmsDialog::Event::kConfirm);
   }));
 
   auto* provider = new NiceMock<MockSmsProvider>();
@@ -117,20 +116,19 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, AtMostOnePendingSmsRequest) {
 
   auto* dialog = new NiceMock<MockSmsDialog>();
 
-  base::OnceClosure on_confirm_callback;
+  SmsDialog::EventHandler hdl;
 
   EXPECT_CALL(delegate_, CreateSmsDialog(_))
       .WillOnce(Return(ByMove(base::WrapUnique(dialog))));
 
-  EXPECT_CALL(*dialog, Open(_, _, _))
-      .WillOnce(Invoke([&on_confirm_callback](content::RenderFrameHost*,
-                                              base::OnceClosure on_confirm,
-                                              base::OnceClosure on_cancel) {
-        on_confirm_callback = std::move(on_confirm);
-      }));
+  EXPECT_CALL(*dialog, Open(_, _))
+      .WillOnce(
+          Invoke([&hdl](RenderFrameHost*, SmsDialog::EventHandler handler) {
+            hdl = std::move(handler);
+          }));
 
-  EXPECT_CALL(*dialog, SmsReceived()).WillOnce(Invoke([&on_confirm_callback]() {
-    std::move(on_confirm_callback).Run();
+  EXPECT_CALL(*dialog, SmsReceived()).WillOnce(Invoke([&hdl]() {
+    std::move(hdl).Run(SmsDialog::Event::kConfirm);
   }));
 
   auto* provider = new NiceMock<MockSmsProvider>();
@@ -244,7 +242,8 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, TwoTabsSameOrigin) {
     true
   )";
 
-  base::OnceClosure on_confirm1, on_confirm2;
+  SmsDialog::EventHandler hdl_1;
+  SmsDialog::EventHandler hdl_2;
 
   {
     base::RunLoop loop;
@@ -260,12 +259,11 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, TwoTabsSameOrigin) {
       loop.Quit();
     }));
 
-    EXPECT_CALL(*dialog, Open(_, _, _))
-        .WillOnce(Invoke([&on_confirm1](content::RenderFrameHost*,
-                                        base::OnceClosure on_confirm,
-                                        base::OnceClosure on_cancel) {
-          on_confirm1 = std::move(on_confirm);
-        }));
+    EXPECT_CALL(*dialog, Open(_, _))
+        .WillOnce(
+            Invoke([&hdl_1](RenderFrameHost*, SmsDialog::EventHandler handler) {
+              hdl_1 = std::move(handler);
+            }));
 
     // First tab registers an observer.
     EXPECT_EQ(true, EvalJs(tab1, script));
@@ -287,12 +285,11 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, TwoTabsSameOrigin) {
       loop.Quit();
     }));
 
-    EXPECT_CALL(*dialog, Open(_, _, _))
-        .WillOnce(Invoke([&on_confirm2](content::RenderFrameHost*,
-                                        base::OnceClosure on_confirm,
-                                        base::OnceClosure on_cancel) {
-          on_confirm2 = std::move(on_confirm);
-        }));
+    EXPECT_CALL(*dialog, Open(_, _))
+        .WillOnce(
+            Invoke([&hdl_2](RenderFrameHost*, SmsDialog::EventHandler handler) {
+              hdl_2 = std::move(handler);
+            }));
 
     // Second tab registers an observer.
     EXPECT_EQ(true, EvalJs(tab2, script));
@@ -304,15 +301,15 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, TwoTabsSameOrigin) {
 
   provider->NotifyReceive(url::Origin::Create(url), "hello1");
 
-  std::move(on_confirm1).Run();
+  std::move(hdl_1).Run(SmsDialog::Event::kConfirm);
 
   EXPECT_EQ("hello1", EvalJs(tab1, "sms"));
 
   ASSERT_TRUE(provider->HasObservers());
 
   provider->NotifyReceive(url::Origin::Create(url), "hello2");
 
-  std::move(on_confirm2).Run();
+  std::move(hdl_2).Run(SmsDialog::Event::kConfirm);
 
   EXPECT_EQ("hello2", EvalJs(tab2, "sms"));
 
@@ -356,26 +353,24 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, TwoTabsDifferentOrigin) {
   auto* dialog1 = new NiceMock<MockSmsDialog>();
   auto* dialog2 = new NiceMock<MockSmsDialog>();
 
-  base::OnceClosure on_confirm_callback1;
-  base::OnceClosure on_confirm_callback2;
+  SmsDialog::EventHandler hdl_1;
+  SmsDialog::EventHandler hdl_2;
 
   EXPECT_CALL(delegate_, CreateSmsDialog(_))
       .WillOnce(Return(ByMove(base::WrapUnique(dialog1))))
       .WillOnce(Return(ByMove(base::WrapUnique(dialog2))));
 
-  EXPECT_CALL(*dialog1, Open(_, _, _))
-      .WillOnce(Invoke([&on_confirm_callback1](content::RenderFrameHost*,
-                                               base::OnceClosure on_confirm,
-                                               base::OnceClosure on_cancel) {
-        on_confirm_callback1 = std::move(on_confirm);
-      }));
+  EXPECT_CALL(*dialog1, Open(_, _))
+      .WillOnce(
+          Invoke([&hdl_1](RenderFrameHost*, SmsDialog::EventHandler handler) {
+            hdl_1 = std::move(handler);
+          }));
 
-  EXPECT_CALL(*dialog2, Open(_, _, _))
-      .WillOnce(Invoke([&on_confirm_callback2](content::RenderFrameHost*,
-                                               base::OnceClosure on_confirm,
-                                               base::OnceClosure on_cancel) {
-        on_confirm_callback2 = std::move(on_confirm);
-      }));
+  EXPECT_CALL(*dialog2, Open(_, _))
+      .WillOnce(
+          Invoke([&hdl_2](RenderFrameHost*, SmsDialog::EventHandler handler) {
+            hdl_2 = std::move(handler);
+          }));
 
   EXPECT_EQ(true, EvalJs(tab1, script));
   EXPECT_EQ(true, EvalJs(tab2, script));
@@ -386,15 +381,15 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, TwoTabsDifferentOrigin) {
 
   provider->NotifyReceive(url::Origin::Create(url1), "hello1");
 
-  std::move(on_confirm_callback1).Run();
+  std::move(hdl_1).Run(SmsDialog::Event::kConfirm);
 
   EXPECT_EQ("hello1", EvalJs(tab1, "sms"));
 
   ASSERT_TRUE(provider->HasObservers());
 
   provider->NotifyReceive(url::Origin::Create(url2), "hello2");
 
-  std::move(on_confirm_callback2).Run();
+  std::move(hdl_2).Run(SmsDialog::Event::kConfirm);
 
   EXPECT_EQ("hello2", EvalJs(tab2, "sms"));
 
@@ -446,13 +441,11 @@ IN_PROC_BROWSER_TEST_F(SmsBrowserTest, Cancels) {
   EXPECT_CALL(delegate, CreateSmsDialog(_))
       .WillOnce(Return(ByMove(base::WrapUnique(dialog))));
 
-  EXPECT_CALL(*dialog, Open(_, _, _))
-      .WillOnce(
-          Invoke([](content::RenderFrameHost*, base::OnceClosure on_confirm,
-                    base::OnceClosure on_cancel) {
-            // Simulates the user pressing "cancel".
-            std::move(on_cancel).Run();
-          }));
+  EXPECT_CALL(*dialog, Open(_, _))
+      .WillOnce(Invoke([](RenderFrameHost*, SmsDialog::EventHandler handler) {
+        // Simulates the user pressing "cancel".
+        std::move(handler).Run(SmsDialog::Event::kCancel);
+      }));
 
   EXPECT_CALL(*dialog, Close()).WillOnce(Return());
 

diff --git a/content/browser/sms/sms_service.cc b/content/browser/sms/sms_service.cc
@@ -10,6 +10,7 @@
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
+#include "base/logging.h"
 #include "base/optional.h"
 #include "content/browser/sms/sms_metrics.h"
 #include "content/public/browser/sms_dialog.h"
@@ -143,16 +144,27 @@ void SmsService::OnCancel() {
   Process(SmsStatus::kCancelled, base::nullopt);
 }
 
+void SmsService::OnEvent(SmsDialog::Event event_type) {
+  switch (event_type) {
+    case SmsDialog::Event::kConfirm:
+      OnConfirm();
+      return;
+    case SmsDialog::Event::kCancel:
+      OnCancel();
+      return;
+  }
+  DVLOG(1) << "Unsupported event type: " << event_type;
+  NOTREACHED();
+}
+
 void SmsService::Prompt() {
   WebContents* web_contents =
-      content::WebContents::FromRenderFrameHost(render_frame_host());
+      WebContents::FromRenderFrameHost(render_frame_host());
   const url::Origin origin = render_frame_host()->GetLastCommittedOrigin();
   prompt_ = web_contents->GetDelegate()->CreateSmsDialog(origin);
   if (prompt_) {
-    prompt_->Open(
-        render_frame_host(),
-        base::BindOnce(&SmsService::OnConfirm, base::Unretained(this)),
-        base::BindOnce(&SmsService::OnCancel, base::Unretained(this)));
+    prompt_->Open(render_frame_host(),
+                  base::BindOnce(&SmsService::OnEvent, base::Unretained(this)));
   }
 }
 

diff --git a/content/browser/sms/sms_service.h b/content/browser/sms/sms_service.h
@@ -15,6 +15,7 @@
 #include "content/browser/sms/sms_provider.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/frame_service_base.h"
+#include "content/public/browser/sms_dialog.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
 #include "third_party/blink/public/mojom/sms/sms_receiver.mojom.h"
 #include "url/origin.h"
@@ -60,11 +61,14 @@ class CONTENT_EXPORT SmsService
 
   // Callback when the |timer_| times out.
   void OnTimeout();
-  // Callback when the user manually clicks 'Confirm' button.
+  // Called when the user manually clicks 'Confirm' button.
   void OnConfirm();
-  // Callback when the user manually dismisses the dialog.
+  // Called when the user manually dismisses the dialog.
   void OnCancel();
 
+  // Handles the user's action.
+  void OnEvent(SmsDialog::Event event_type);
+
   // |sms_provider_| is safe because all instances of SmsProvider are owned
   // by a SmsKeyedService, which is owned by a Profile, which transitively
   // owns SmsServices.