MediaLog: Replace task posting in AddEvent with lock

content/renderer/media/render_media_log.cc

Index: content/renderer/media/render_media_log.cc
diff --git a/content/renderer/media/render_media_log.cc b/content/renderer/media/render_media_log.cc
index 180d2b22f4c7317225b132bfd56e15cd3c003990..95e966f04fddc190246b3347c994d3f1749678c1 100644
--- a/content/renderer/media/render_media_log.cc
+++ b/content/renderer/media/render_media_log.cc
@@ -35,56 +35,56 @@ namespace content {
 RenderMediaLog::RenderMediaLog()
     : task_runner_(base::ThreadTaskRunnerHandle::Get()),
       tick_clock_(new base::DefaultTickClock()),
-      last_ipc_send_time_(tick_clock_->NowTicks()) {
+      last_ipc_send_time_(tick_clock_->NowTicks()),
+      ipc_send_pending_(false) {
   DCHECK(RenderThread::Get())
       << "RenderMediaLog must be constructed on the render thread";
 }
 
 void RenderMediaLog::AddEvent(scoped_ptr<media::MediaLogEvent> event) {
-  // Always post to preserve the correct order of events.
-  // TODO(xhwang): Consider using sorted containers to keep the order and
-  // avoid extra posting.
-  task_runner_->PostTask(FROM_HERE,
-                         base::Bind(&RenderMediaLog::AddEventInternal, this,
-                                    base::Passed(&event)));
-}
-
-void RenderMediaLog::AddEventInternal(scoped_ptr<media::MediaLogEvent> event) {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-
-  Log(event.get());
-
-  // If there is an event waiting to be sent, there must be a send task pending.
-  const bool delayed_ipc_send_pending =
-      !queued_media_events_.empty() || last_buffered_extents_changed_event_;
-
-  // Keep track of the latest buffered extents properties to avoid sending
-  // thousands of events over IPC. See http://crbug.com/352585 for details.
-  if (event->type == media::MediaLogEvent::BUFFERED_EXTENTS_CHANGED)
-    last_buffered_extents_changed_event_.swap(event);
-  else
-    queued_media_events_.push_back(*event);
-
-  if (delayed_ipc_send_pending)
-    return;
-
-  // Delay until it's been a second since the last ipc message was sent.
-  base::TimeDelta delay_for_next_ipc_send =
-      base::TimeDelta::FromSeconds(1) -
-      (tick_clock_->NowTicks() - last_ipc_send_time_);
-  if (delay_for_next_ipc_send > base::TimeDelta()) {
-    task_runner_->PostDelayedTask(
-        FROM_HERE, base::Bind(&RenderMediaLog::SendQueuedMediaEvents, this),
-        delay_for_next_ipc_send);
-    return;
+  {
+    base::AutoLock auto_lock(lock_);
+
+    Log(event.get());

[xhwang, 2016/04/01 05:42:55]

nit: This can be outside of the lock.

[wolenetz, 2016/04/04 18:30:32]

Done.

+
+    // Keep track of the latest buffered extents properties to avoid sending
+    // thousands of events over IPC. See http://crbug.com/352585 for details.
+    if (event->type == media::MediaLogEvent::BUFFERED_EXTENTS_CHANGED)
+      last_buffered_extents_changed_event_.swap(event);
+    else
+      queued_media_events_.push_back(*event);
+
+    if (ipc_send_pending_)
+      return;
+
+    ipc_send_pending_ = true;
+
+    // Delay until it's been a second since the last ipc message was sent.
+    base::TimeDelta delay_for_next_ipc_send =
+        base::TimeDelta::FromSeconds(1) -
+        (tick_clock_->NowTicks() - last_ipc_send_time_);
+    if (delay_for_next_ipc_send > base::TimeDelta()) {
+      task_runner_->PostDelayedTask(
+          FROM_HERE, base::Bind(&RenderMediaLog::SendQueuedMediaEvents, this),
+          delay_for_next_ipc_send);
+      return;
+    }
   }
 
-  // It's been more than a second so send now.
-  SendQueuedMediaEvents();
+  // It's been more than a second so send ASAP.
+  if (task_runner_->BelongsToCurrentThread())
+    SendQueuedMediaEvents();
+  else
+    task_runner_->PostTask(
+        FROM_HERE, base::Bind(&RenderMediaLog::SendQueuedMediaEvents, this));
 }
 
 void RenderMediaLog::SendQueuedMediaEvents() {
   DCHECK(task_runner_->BelongsToCurrentThread());
+  base::AutoLock auto_lock(lock_);
+
+  DCHECK(ipc_send_pending_);
+  ipc_send_pending_ = false;
 
   if (last_buffered_extents_changed_event_) {
     queued_media_events_.push_back(*last_buffered_extents_changed_event_);
@@ -102,12 +102,14 @@ RenderMediaLog::~RenderMediaLog() {
 
 void RenderMediaLog::SetTickClockForTesting(
     scoped_ptr<base::TickClock> tick_clock) {
+  base::AutoLock auto_lock(lock_);
   tick_clock_.swap(tick_clock);
   last_ipc_send_time_ = tick_clock_->NowTicks();
 }
 
 void RenderMediaLog::SetTaskRunnerForTesting(
     const scoped_refptr<base::SingleThreadTaskRunner>& task_runner) {
+  base::AutoLock auto_lock(lock_);

[xhwang, 2016/04/01 05:42:55]

It seems odd the protect the task runner especially since we are not always
protecting the use of task_runner under a lock.

[wolenetz, 2016/04/04 18:30:32]

Yeah. This does seem odd. I'll not lock in this method. Done.

   task_runner_ = task_runner;
 }