cc: Make separate interface for BeginFrame ipc from OutputSurface

content/renderer/gpu/compositor_forwarding_message_filter.cc

Index: content/renderer/gpu/compositor_forwarding_message_filter.cc
diff --git a/content/renderer/gpu/compositor_forwarding_message_filter.cc b/content/renderer/gpu/compositor_forwarding_message_filter.cc
new file mode 100644
index 0000000000000000000000000000000000000000..e01c9c6ea66da62a340e2fd0b8875d674354b265
--- /dev/null
+++ b/content/renderer/gpu/compositor_forwarding_message_filter.cc
@@ -0,0 +1,80 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/renderer/gpu//compositor_forwarding_message_filter.h"
+
+#include "base/bind.h"
+#include "base/location.h"
+#include "content/common/view_messages.h"
+#include "ipc/ipc_message.h"
+
+namespace content {
+
+CompositorForwardingMessageFilter* CompositorForwardingMessageFilter::Create(

[piman, 2014/10/24 20:49:06]

If we're making a specific class for the compositor, I think we can simplify it
significantly rather than just copy&paste from ForwardingMessageFilter... See
below.

[simonhong, 2014/10/29 14:47:14]

Done.

+    base::TaskRunner* target_task_runner) {
+  uint32 messages_to_filter[] = {
+    ViewMsg_BeginFrame::ID,
+    ViewMsg_ReclaimCompositorResources::ID,
+    ViewMsg_SwapCompositorFrameAck::ID,
+    ViewMsg_UpdateVSyncParameters::ID
+  };
+
+  return new CompositorForwardingMessageFilter(
+      messages_to_filter, arraysize(messages_to_filter),
+      target_task_runner);
+}
+
+CompositorForwardingMessageFilter::CompositorForwardingMessageFilter(
+    const uint32* message_ids_to_filter,
+    size_t num_message_ids_to_filter,
+    base::TaskRunner* target_task_runner)
+    : target_task_runner_(target_task_runner) {
+  DCHECK(target_task_runner_.get());
+  for (size_t i = 0; i < num_message_ids_to_filter; i++)
+    message_ids_to_filter_.insert(message_ids_to_filter[i]);
+}
+
+CompositorForwardingMessageFilter::~CompositorForwardingMessageFilter() {
+}
+
+void CompositorForwardingMessageFilter::AddRoute(int routing_id,
+                                                 const Handler& handler) {
+  DCHECK(!handler.is_null());
+  base::AutoLock locked(handlers_lock_);
+  multi_handlers_.insert(std::make_pair(routing_id, handler));
+}
+
+void CompositorForwardingMessageFilter::RemoveRoute(int routing_id,
+                                                    const Handler& handler) {
+  base::AutoLock locked(handlers_lock_);
+  auto handlers = multi_handlers_.equal_range(routing_id);
+  for (auto it = handlers.first; it != handlers.second; ++it) {
+    if (it->second.Equals(handler)) {
+      multi_handlers_.erase(it);
+      return;
+    }
+  }
+  NOTREACHED();
+}
+
+bool CompositorForwardingMessageFilter::OnMessageReceived(
+    const IPC::Message& message) {
+  if (message_ids_to_filter_.find(message.type()) ==
+      message_ids_to_filter_.end())

[piman, 2014/10/24 20:49:06]

The message id list is constant. We can have a simple switch statement on the
list of messages, rather than a costly std::set.

[simonhong, 2014/10/29 14:47:14]

Done.

+    return false;
+
+  {
+    base::AutoLock locked(handlers_lock_);
+    auto handlers = multi_handlers_.equal_range(message.routing_id());
+    if (handlers.first == handlers.second)
+      return false;
+
+    for (auto it = handlers.first; it != handlers.second; ++it)
+      target_task_runner_->PostTask(FROM_HERE, base::Bind(it->second, message));

[brianderson, 2014/10/24 20:46:58]

This was probably discussed already, but why do we iterate through the handlers
here (before the post task) instead of within a posted task to the compositor
thread?

[piman, 2014/10/24 20:49:06]

This posts N tasks if we have N handlers, even though only one will handle the
message.
Better would be to post a single task to the target_task_runner, and then
dispatch to the right handler on the compositor thread.

As an added bonus, it means we always access multi_handlers_ on the same thread
(both AddRoute and RemoveRoute are called on the compositor thread), so we can
completely remove the lock (replace with a thread checker maybe).

[simonhong, 2014/10/29 14:47:13]

Modified to dispatch by posted task on compositor thread.

+  }
+
+  return true;
+}
+
+}  // namespace content