Fix a variety of small issues with ScopedClosureRunner.

* Explicit deletion of copy constructors not necessary when move
  constructors are given.
* No way to check if the runner had a closure inside.
* RunAndReset() crashed if called with no closure inside.
* operator=() failed to run the outgoing closure.

Bug: none
Change-Id: I8c92dfaca95619fe6661af10c018a75dae7e25ff
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2537787
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827526}

base/callback_helpers.cc

@@ -11,22 +11,25 @@ ScopedClosureRunner::ScopedClosureRunner() = default;
 ScopedClosureRunner::ScopedClosureRunner(OnceClosure closure)
     : closure_(std::move(closure)) {}
 
-ScopedClosureRunner::~ScopedClosureRunner() {
-  if (!closure_.is_null())
-    std::move(closure_).Run();
-}
-
 ScopedClosureRunner::ScopedClosureRunner(ScopedClosureRunner&& other)
     : closure_(other.Release()) {}
 
 ScopedClosureRunner& ScopedClosureRunner::operator=(
     ScopedClosureRunner&& other) {
-  ReplaceClosure(other.Release());
+  if (this != &other) {
+    RunAndReset();
+    ReplaceClosure(other.Release());
+  }
   return *this;
 }
 
+ScopedClosureRunner::~ScopedClosureRunner() {
+  RunAndReset();
+}
+
 void ScopedClosureRunner::RunAndReset() {
-  std::move(closure_).Run();
+  if (closure_)
+    std::move(closure_).Run();
 }
 
 void ScopedClosureRunner::ReplaceClosure(OnceClosure closure) {

base/callback_helpers.h

@@ -113,15 +113,15 @@ class BASE_EXPORT ScopedClosureRunner {
  public:
   ScopedClosureRunner();
   explicit ScopedClosureRunner(OnceClosure closure);
-  ScopedClosureRunner(const ScopedClosureRunner&) = delete;
-  ScopedClosureRunner& operator=(const ScopedClosureRunner&) = delete;
-  ~ScopedClosureRunner();
-
   ScopedClosureRunner(ScopedClosureRunner&& other);
-
-  // Releases the current closure if it's set and replaces it with the closure
-  // from |other|.
+  // Runs the current closure if it's set, then replaces it with the closure
+  // from |other|. This is akin to how unique_ptr frees the contained pointer in
+  // its move assignment operator. If you need to explicitly avoid running any
+  // current closure, use ReplaceClosure().
   ScopedClosureRunner& operator=(ScopedClosureRunner&& other);
+  ~ScopedClosureRunner();
+
+  explicit operator bool() const { return !!closure_; }
 
   // Calls the current closure and resets it, so it wont be called again.
   void RunAndReset();

base/callback_helpers_unittest.cc

@@ -87,7 +87,15 @@ void Increment(int* value) {
   (*value)++;
 }
 
-TEST(CallbackHelpersTest, TestScopedClosureRunnerExitScope) {
+TEST(CallbackHelpersTest, ScopedClosureRunnerHasClosure) {
+  base::ScopedClosureRunner runner1;
+  EXPECT_FALSE(runner1);
+
+  base::ScopedClosureRunner runner2{base::DoNothing()};
+  EXPECT_TRUE(runner2);
+}
+
+TEST(CallbackHelpersTest, ScopedClosureRunnerExitScope) {
   int run_count = 0;
   {
     base::ScopedClosureRunner runner(base::BindOnce(&Increment, &run_count));
@@ -96,7 +104,7 @@ TEST(CallbackHelpersTest, TestScopedClosureRunnerExitScope) {
   EXPECT_EQ(1, run_count);
 }
 
-TEST(CallbackHelpersTest, TestScopedClosureRunnerRelease) {
+TEST(CallbackHelpersTest, ScopedClosureRunnerRelease) {
   int run_count = 0;
   base::OnceClosure c;
   {
@@ -109,7 +117,7 @@ TEST(CallbackHelpersTest, TestScopedClosureRunnerRelease) {
   EXPECT_EQ(1, run_count);
 }
 
-TEST(CallbackHelpersTest, TestScopedClosureRunnerReplaceClosure) {
+TEST(CallbackHelpersTest, ScopedClosureRunnerReplaceClosure) {
   int run_count_1 = 0;
   int run_count_2 = 0;
   {
@@ -123,7 +131,7 @@ TEST(CallbackHelpersTest, TestScopedClosureRunnerReplaceClosure) {
   EXPECT_EQ(1, run_count_2);
 }
 
-TEST(CallbackHelpersTest, TestScopedClosureRunnerRunAndReset) {
+TEST(CallbackHelpersTest, ScopedClosureRunnerRunAndResetNonNull) {
   int run_count_3 = 0;
   {
     base::ScopedClosureRunner runner(base::BindOnce(&Increment, &run_count_3));
@@ -134,7 +142,12 @@ TEST(CallbackHelpersTest, TestScopedClosureRunnerRunAndReset) {
   EXPECT_EQ(1, run_count_3);
 }
 
-TEST(CallbackHelpersTest, TestScopedClosureRunnerMoveConstructor) {
+TEST(CallbackHelpersTest, ScopedClosureRunnerRunAndResetNull) {
+  base::ScopedClosureRunner runner;
+  runner.RunAndReset();  // Should not crash.
+}
+
+TEST(CallbackHelpersTest, ScopedClosureRunnerMoveConstructor) {
   int run_count = 0;
   {
     std::unique_ptr<base::ScopedClosureRunner> runner(
@@ -146,7 +159,7 @@ TEST(CallbackHelpersTest, TestScopedClosureRunnerMoveConstructor) {
   EXPECT_EQ(1, run_count);
 }
 
-TEST(CallbackHelpersTest, TestScopedClosureRunnerMoveAssignment) {
+TEST(CallbackHelpersTest, ScopedClosureRunnerMoveAssignment) {
   int run_count_1 = 0;
   int run_count_2 = 0;
   {
@@ -155,17 +168,17 @@ TEST(CallbackHelpersTest, TestScopedClosureRunnerMoveAssignment) {
       base::ScopedClosureRunner runner2(
           base::BindOnce(&Increment, &run_count_2));
       runner = std::move(runner2);
-      EXPECT_EQ(0, run_count_1);
+      EXPECT_EQ(1, run_count_1);
       EXPECT_EQ(0, run_count_2);
     }
-    EXPECT_EQ(0, run_count_1);
+    EXPECT_EQ(1, run_count_1);
     EXPECT_EQ(0, run_count_2);
   }
-  EXPECT_EQ(0, run_count_1);
+  EXPECT_EQ(1, run_count_1);
   EXPECT_EQ(1, run_count_2);
 }
 
-TEST(CallbackHelpersTest, TestAdaptCallbackForRepeating) {
+TEST(CallbackHelpersTest, AdaptCallbackForRepeating) {
   int count = 0;
   base::OnceCallback<void(int*)> cb =
       base::BindOnce([](int* count) { ++*count; });

content/browser/web_contents/web_contents_view_aura.cc

@@ -1070,12 +1070,13 @@ void WebContentsViewAura::StartDragging(
   // If drag is still in progress that means we haven't received drop targeting
   // callback yet. So we have to make sure to delay calling EndDrag until drop
   // is done.
-  if (!drag_in_progress_)
+  if (!drag_in_progress_) {
     EndDrag(std::move(source_rwh_weak_ptr), ConvertToWeb(result_op));
-  else
-    end_drag_runner_ = base::ScopedClosureRunner(base::BindOnce(
+  } else {
+    end_drag_runner_.ReplaceClosure(base::BindOnce(
         &WebContentsViewAura::EndDrag, weak_ptr_factory_.GetWeakPtr(),
         std::move(source_rwh_weak_ptr), ConvertToWeb(result_op)));
+  }
 }
 
 void WebContentsViewAura::UpdateDragCursor(blink::DragOperation operation) {