Use templates for some functions in WebRtcEventLogManager

Some functions ended up having more overloaded versions than
is reasonable; templates to the rescue.

1. Move MaybeReply from .h to .cc, made it into a template, and
   used it in a few additional cases.
2. Made MaybeReply take a base::Location&, rather than hard-code
   the location of the helper function itself.
3. Created a template for Reply and ReplyClosure in the unit test
   suite.

Bug: 775415
Change-Id: Ie2c9e31defd045535845d198c658f0859ef99228
Reviewed-on: https://chromium-review.googlesource.com/1114615
Commit-Queue: Elad Alon <eladalon@chromium.org>
Reviewed-by: Sergey Ulanov <sergeyu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571101}

chrome/browser/media/webrtc/webrtc_event_log_manager.cc

@@ -53,6 +53,29 @@ const BrowserContext* GetBrowserContext(int render_process_id) {
   return host ? host->GetBrowserContext() : nullptr;
 }
 
+// Post reply back if non-empty.
+template <typename... Args>
+inline void MaybeReply(const base::Location& location,
+                       base::OnceCallback<void(Args...)> reply,
+                       Args... args) {
+  if (reply) {
+    BrowserThread::PostTask(BrowserThread::UI, location,
+                            base::BindOnce(std::move(reply), args...));
+  }
+}
+
+// No style-guide-permited way of forcing const-ref inference at the moment.
+inline void MaybeReply(const base::Location& location,
+                       base::OnceCallback<void(bool, const std::string&)> reply,
+                       bool bool_val,
+                       const std::string& str_val) {
+  if (reply) {
+    BrowserThread::PostTask(
+        BrowserThread::UI, location,
+        base::BindOnce(std::move(reply), bool_val, str_val));
+  }
+}
+
 }  // namespace
 
 const size_t kWebRtcEventLogManagerUnlimitedFileSize = 0;
@@ -162,7 +185,7 @@ void WebRtcEventLogManager::PeerConnectionAdded(
   RenderProcessHost* rph = RenderProcessHost::FromID(render_process_id);
   if (!rph) {
     // RPH died before processing of this notification.
-    MaybeReply(std::move(reply), false);
+    MaybeReply(FROM_HERE, std::move(reply), false);
     return;
   }
 
@@ -198,7 +221,7 @@ void WebRtcEventLogManager::PeerConnectionRemoved(
   if (browser_context_id == kNullBrowserContextId) {
     // RPH died before processing of this notification. This is handled by
     // RenderProcessExited() / RenderProcessHostDestroyed.
-    MaybeReply(std::move(reply), false);
+    MaybeReply(FROM_HERE, std::move(reply), false);
     return;
   }
 
@@ -263,7 +286,7 @@ void WebRtcEventLogManager::OnWebRtcEventLogWrite(
   const BrowserContext* browser_context = GetBrowserContext(render_process_id);
   if (!browser_context) {
     // RPH died before processing of this notification.
-    MaybeReply(std::move(reply), false, false);
+    MaybeReply(FROM_HERE, std::move(reply), std::make_pair(false, false));
     return;
   }
 
@@ -290,25 +313,22 @@ void WebRtcEventLogManager::StartRemoteLogging(
     base::OnceCallback<void(bool, const std::string&)> reply) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  if (!remote_logs_manager_) {
-    MaybeReply(std::move(reply), false,
-               std::string(kStartRemoteLoggingFailureFeatureDisabled));
-    return;
-  }
-
   const BrowserContext* browser_context = GetBrowserContext(render_process_id);
-  if (!browser_context) {
-    // RPH died before processing of this notification.
-    MaybeReply(std::move(reply), false,
-               std::string(kStartRemoteLoggingFailureGeneric));
-    return;
-  }
+  const char* error = nullptr;
 
-  if (browser_context->IsOffTheRecord()) {
+  if (!remote_logs_manager_) {
+    error = kStartRemoteLoggingFailureFeatureDisabled;
+  } else if (!browser_context) {
+    // RPH died before processing of this notification.
+    error = kStartRemoteLoggingFailureGeneric;
+  } else if (browser_context->IsOffTheRecord()) {
     // Feature disable in incognito. Since the feature can be disabled for
     // non-incognito sessions, this should not expose incognito mode.
-    MaybeReply(std::move(reply), false,
-               std::string(kStartRemoteLoggingFailureFeatureDisabled));
+    error = kStartRemoteLoggingFailureFeatureDisabled;
+  }
+
+  if (error) {
+    MaybeReply(FROM_HERE, std::move(reply), false, std::string(error));
     return;
   }
 
@@ -505,9 +525,7 @@ void WebRtcEventLogManager::EnableForBrowserContextInternal(
                                                   browser_context_dir);
   }
 
-  if (reply) {
-    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, std::move(reply));
-  }
+  MaybeReply(FROM_HERE, std::move(reply));
 }
 
 void WebRtcEventLogManager::DisableForBrowserContextInternal(
@@ -519,9 +537,7 @@ void WebRtcEventLogManager::DisableForBrowserContextInternal(
     remote_logs_manager_->DisableForBrowserContext(browser_context_id);
   }
 
-  if (reply) {
-    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, std::move(reply));
-  }
+  MaybeReply(FROM_HERE, std::move(reply));
 }
 
 void WebRtcEventLogManager::PeerConnectionAddedInternal(
@@ -538,10 +554,7 @@ void WebRtcEventLogManager::PeerConnectionAddedInternal(
     DCHECK_EQ(local_result, remote_result);
   }
 
-  if (reply) {
-    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                            base::BindOnce(std::move(reply), local_result));
-  }
+  MaybeReply(FROM_HERE, std::move(reply), local_result);
 }
 
 void WebRtcEventLogManager::PeerConnectionRemovedInternal(
@@ -555,10 +568,7 @@ void WebRtcEventLogManager::PeerConnectionRemovedInternal(
     DCHECK_EQ(local_result, remote_result);
   }
 
-  if (reply) {
-    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                            base::BindOnce(std::move(reply), local_result));
-  }
+  MaybeReply(FROM_HERE, std::move(reply), local_result);
 }
 
 void WebRtcEventLogManager::EnableLocalLoggingInternal(
@@ -570,10 +580,7 @@ void WebRtcEventLogManager::EnableLocalLoggingInternal(
   const bool result =
       local_logs_manager_.EnableLogging(base_path, max_file_size_bytes);
 
-  if (reply) {
-    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                            base::BindOnce(std::move(reply), result));
-  }
+  MaybeReply(FROM_HERE, std::move(reply), result);
 }
 
 void WebRtcEventLogManager::DisableLocalLoggingInternal(
@@ -582,10 +589,7 @@ void WebRtcEventLogManager::DisableLocalLoggingInternal(
 
   const bool result = local_logs_manager_.DisableLogging();
 
-  if (reply) {
-    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                            base::BindOnce(std::move(reply), result));
-  }
+  MaybeReply(FROM_HERE, std::move(reply), result);
 }
 
 void WebRtcEventLogManager::OnWebRtcEventLogWriteInternal(
@@ -601,12 +605,8 @@ void WebRtcEventLogManager::OnWebRtcEventLogWriteInternal(
           ? remote_logs_manager_->EventLogWrite(key, message)
           : false;
 
-  if (reply) {
-    BrowserThread::PostTask(
-        BrowserThread::UI, FROM_HERE,
-        base::BindOnce(std::move(reply),
-                       std::make_pair(local_result, remote_result)));
-  }
+  MaybeReply(FROM_HERE, std::move(reply),
+             std::make_pair(local_result, remote_result));
 }
 
 void WebRtcEventLogManager::StartRemoteLoggingInternal(
@@ -624,11 +624,8 @@ void WebRtcEventLogManager::StartRemoteLoggingInternal(
       browser_context_dir, max_file_size_bytes, &error_message);
   DCHECK_EQ(result, error_message.empty());  // Error set iff has failed.
 
-  if (reply) {
-    BrowserThread::PostTask(
-        BrowserThread::UI, FROM_HERE,
-        base::BindOnce(std::move(reply), result, error_message));
-  }
+  MaybeReply(FROM_HERE,
+             base::BindOnce(std::move(reply), result, error_message));
 }
 
 void WebRtcEventLogManager::ClearCacheForBrowserContextInternal(
@@ -675,45 +672,6 @@ void WebRtcEventLogManager::SetRemoteLogsObserverInternal(
   }
 }
 
-void WebRtcEventLogManager::MaybeReply(base::OnceClosure reply) {
-  if (!reply) {
-    return;
-  }
-  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, std::move(reply));
-}
-
-void WebRtcEventLogManager::MaybeReply(base::OnceCallback<void(bool)> reply,
-                                       bool value) {
-  if (!reply) {
-    return;
-  }
-  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                          base::BindOnce(std::move(reply), value));
-}
-
-void WebRtcEventLogManager::MaybeReply(
-    base::OnceCallback<void(bool, const std::string&)> reply,
-    bool bool_val,
-    const std::string& str_val) {
-  if (!reply) {
-    return;
-  }
-  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                          base::BindOnce(std::move(reply), bool_val, str_val));
-}
-
-void WebRtcEventLogManager::MaybeReply(
-    base::OnceCallback<void(std::pair<bool, bool>)> reply,
-    bool first,
-    bool second) {
-  if (!reply) {
-    return;
-  }
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      base::BindOnce(std::move(reply), std::make_pair(first, second)));
-}
-
 void WebRtcEventLogManager::SetClockForTesting(base::Clock* clock,
                                                base::OnceClosure reply) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);

chrome/browser/media/webrtc/webrtc_event_log_manager.h

@@ -247,16 +247,6 @@ class WebRtcEventLogManager final : public content::RenderProcessHostObserver,
   void SetRemoteLogsObserverInternal(WebRtcRemoteEventLogsObserver* observer,
                                      base::OnceClosure reply);
 
-  // Non-empty replies get posted to BrowserThread::UI.
-  void MaybeReply(base::OnceClosure reply);
-  void MaybeReply(base::OnceCallback<void(bool)> reply, bool value);
-  void MaybeReply(base::OnceCallback<void(bool, const std::string&)> reply,
-                  bool bool_val,
-                  const std::string& str_val);
-  void MaybeReply(base::OnceCallback<void(std::pair<bool, bool>)> reply,
-                  bool first,
-                  bool second);
-
   // Injects a fake clock, to be used by tests. For example, this could be
   // used to inject a frozen clock, thereby allowing unit tests to know what a
   // local log's filename would end up being.

chrome/browser/media/webrtc/webrtc_event_log_manager_unittest.cc

@@ -250,49 +250,50 @@ class WebRtcEventLogManagerTestBase : public ::testing::TestWithParam<bool> {
     run_loop_.reset(new base::RunLoop);  // Allow re-blocking.
   }
 
-  void VoidReply() { run_loop_->QuitWhenIdle(); }
+  void Reply() { run_loop_->QuitWhenIdle(); }
 
-  base::OnceClosure VoidReplyClosure() {
-    return base::BindOnce(&WebRtcEventLogManagerTestBase::VoidReply,
-                          base::Unretained(this));
+  base::OnceClosure ReplyClosure() {
+    // Intermediary pointer used to help the compiler distinguish between
+    // the overloaded Reply() functions.
+    void (WebRtcEventLogManagerTestBase::*function)() =
+        &WebRtcEventLogManagerTestBase::Reply;
+    return base::BindOnce(function, base::Unretained(this));
   }
 
-  void BoolReply(bool* output, bool value) {
-    *output = value;
+  template <typename T>
+  void Reply(T* output, T val) {
+    *output = val;
     run_loop_->QuitWhenIdle();
   }
 
-  base::OnceCallback<void(bool)> BoolReplyClosure(bool* output) {
-    return base::BindOnce(&WebRtcEventLogManagerTestBase::BoolReply,
-                          base::Unretained(this), output);
+  template <typename T>
+  base::OnceCallback<void(T)> ReplyClosure(T* output) {
+    // Intermediary pointer used to help the compiler distinguish between
+    // the overloaded Reply() functions.
+    void (WebRtcEventLogManagerTestBase::*function)(T*, T) =
+        &WebRtcEventLogManagerTestBase::Reply;
+    return base::BindOnce(function, base::Unretained(this), output);
   }
 
-  void BoolAndStringReply(bool* output_bool,
-                          std::string* output_str,
-                          bool bool_val,
-                          const std::string& str_val) {
+  void Reply(bool* output_bool,
+             std::string* output_str,
+             bool bool_val,
+             const std::string& str_val) {
     *output_bool = bool_val;
     *output_str = str_val;
     run_loop_->QuitWhenIdle();
   }
 
-  base::OnceCallback<void(bool, const std::string&)> BoolAndStringReplyClosure(
+  base::OnceCallback<void(bool, const std::string&)> ReplyClosure(
       bool* output_bool,
       std::string* output_str) {
-    return base::BindOnce(&WebRtcEventLogManagerTestBase::BoolAndStringReply,
-                          base::Unretained(this), output_bool, output_str);
-  }
-
-  void BoolPairReply(std::pair<bool, bool>* output,
-                     std::pair<bool, bool> value) {
-    *output = value;
-    run_loop_->QuitWhenIdle();
-  }
-
-  base::OnceCallback<void(std::pair<bool, bool>)> BoolPairReplyClosure(
-      std::pair<bool, bool>* output) {
-    return base::BindOnce(&WebRtcEventLogManagerTestBase::BoolPairReply,
-                          base::Unretained(this), output);
+    // Intermediary pointer used to help the compiler distinguish between
+    // the overloaded Reply() functions.
+    void (WebRtcEventLogManagerTestBase::*function)(bool*, std::string*, bool,
+                                                    const std::string&) =
+        &WebRtcEventLogManagerTestBase::Reply;
+    return base::BindOnce(function, base::Unretained(this), output_bool,
+                          output_str);
   }
 
   bool PeerConnectionAdded(int render_process_id,
@@ -306,7 +307,7 @@ class WebRtcEventLogManagerTestBase : public ::testing::TestWithParam<bool> {
 
     bool result;
     event_log_manager_->PeerConnectionAdded(
-        render_process_id, lid, peer_connection_id, BoolReplyClosure(&result));
+        render_process_id, lid, peer_connection_id, ReplyClosure(&result));
     WaitForReply();
     return result;
   }
@@ -314,7 +315,7 @@ class WebRtcEventLogManagerTestBase : public ::testing::TestWithParam<bool> {
   bool PeerConnectionRemoved(int render_process_id, int lid) {
     bool result;
     event_log_manager_->PeerConnectionRemoved(render_process_id, lid,
-                                              BoolReplyClosure(&result));
+                                              ReplyClosure(&result));
     WaitForReply();
     return result;
   }
@@ -322,7 +323,7 @@ class WebRtcEventLogManagerTestBase : public ::testing::TestWithParam<bool> {
   bool PeerConnectionStopped(int render_process_id, int lid) {
     bool result;
     event_log_manager_->PeerConnectionStopped(render_process_id, lid,
-                                              BoolReplyClosure(&result));
+                                              ReplyClosure(&result));
     WaitForReply();
     return result;
   }
@@ -337,14 +338,14 @@ class WebRtcEventLogManagerTestBase : public ::testing::TestWithParam<bool> {
       size_t max_size_bytes = kWebRtcEventLogManagerUnlimitedFileSize) {
     bool result;
     event_log_manager_->EnableLocalLogging(local_logs_base_path, max_size_bytes,
-                                           BoolReplyClosure(&result));
+                                           ReplyClosure(&result));
     WaitForReply();
     return result;
   }
 
   bool DisableLocalLogging() {
     bool result;
-    event_log_manager_->DisableLocalLogging(BoolReplyClosure(&result));
+    event_log_manager_->DisableLocalLogging(ReplyClosure(&result));
     WaitForReply();
     return result;
   }
@@ -355,14 +356,19 @@ class WebRtcEventLogManagerTestBase : public ::testing::TestWithParam<bool> {
                           std::string* error_message_output = nullptr) {
     bool result;
     std::string error_message;
+
     event_log_manager_->StartRemoteLogging(
         render_process_id, peer_connection_id, max_size_bytes,
-        BoolAndStringReplyClosure(&result, &error_message));
+        ReplyClosure(&result, &error_message));
+
     WaitForReply();
+
     DCHECK_EQ(result, error_message.empty());  // Error report iff call failed.
+
     if (error_message_output) {
       *error_message_output = error_message;
     }
+
     return result;
   }
 
@@ -378,24 +384,24 @@ class WebRtcEventLogManagerTestBase : public ::testing::TestWithParam<bool> {
       const base::Time& delete_begin,
       const base::Time& delete_end) {
     event_log_manager_->ClearCacheForBrowserContext(
-        browser_context, delete_begin, delete_end, VoidReplyClosure());
+        browser_context, delete_begin, delete_end, ReplyClosure());
     WaitForReply();
   }
 
   void SetLocalLogsObserver(WebRtcLocalEventLogsObserver* observer) {
-    event_log_manager_->SetLocalLogsObserver(observer, VoidReplyClosure());
+    event_log_manager_->SetLocalLogsObserver(observer, ReplyClosure());
     WaitForReply();
   }
 
   void SetRemoteLogsObserver(WebRtcRemoteEventLogsObserver* observer) {
-    event_log_manager_->SetRemoteLogsObserver(observer, VoidReplyClosure());
+    event_log_manager_->SetRemoteLogsObserver(observer, ReplyClosure());
     WaitForReply();
   }
 
   void SetWebRtcEventLogUploaderFactoryForTesting(
       std::unique_ptr<WebRtcEventLogUploader::Factory> factory) {
     event_log_manager_->SetWebRtcEventLogUploaderFactoryForTesting(
-        std::move(factory), VoidReplyClosure());
+        std::move(factory), ReplyClosure());
     WaitForReply();
   }
 
@@ -404,7 +410,7 @@ class WebRtcEventLogManagerTestBase : public ::testing::TestWithParam<bool> {
                                               const std::string& message) {
     std::pair<bool, bool> result;
     event_log_manager_->OnWebRtcEventLogWrite(render_process_id, lid, message,
-                                              BoolPairReplyClosure(&result));
+                                              ReplyClosure(&result));
     WaitForReply();
     return result;
   }
@@ -414,7 +420,7 @@ class WebRtcEventLogManagerTestBase : public ::testing::TestWithParam<bool> {
     ASSERT_TRUE(
         base::Time::FromLocalExploded(frozen_time_exploded, &frozen_time));
     frozen_clock_.SetNow(frozen_time);
-    event_log_manager_->SetClockForTesting(&frozen_clock_, VoidReplyClosure());
+    event_log_manager_->SetClockForTesting(&frozen_clock_, ReplyClosure());
     WaitForReply();
   }
 
@@ -438,7 +444,7 @@ class WebRtcEventLogManagerTestBase : public ::testing::TestWithParam<bool> {
       std::unique_ptr<WebRtcEventLogManager::PeerConnectionTrackerProxy>
           pc_tracker_proxy) {
     event_log_manager_->SetPeerConnectionTrackerProxyForTesting(
-        std::move(pc_tracker_proxy), VoidReplyClosure());
+        std::move(pc_tracker_proxy), ReplyClosure());
     WaitForReply();
   }