Browser Plugin: Renderer-side changes

content/renderer/browser_plugin/guest_to_host_channel.cc

Index: content/renderer/browser_plugin/guest_to_host_channel.cc
diff --git a/content/renderer/browser_plugin/guest_to_host_channel.cc b/content/renderer/browser_plugin/guest_to_host_channel.cc
new file mode 100644
index 0000000000000000000000000000000000000000..22e560cbbc59c8f28cc76b6beeec0d6f0c89c1b3
--- /dev/null
+++ b/content/renderer/browser_plugin/guest_to_host_channel.cc
@@ -0,0 +1,243 @@
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/renderer/browser_plugin/guest_to_host_channel.h"
+
+#include "base/process_util.h"
+#include "content/common/browser_plugin_messages.h"
+#include "content/common/child_process.h"
+#include "content/renderer/browser_plugin/guest_render_view_observer.h"
+#include "content/renderer/render_thread_impl.h"
+#include "content/renderer/render_view_impl.h"
+#include "ipc/ipc_message_macros.h"
+#include "ppapi/c/pp_graphics_3d.h"
+#include "ppapi/proxy/ppapi_command_buffer_proxy.h"
+#include "ppapi/proxy/ppapi_messages.h"
+#include "ppapi/shared_impl/api_id.h"
+#include "webkit/plugins/ppapi/event_conversion.h"
+
+base::MessageLoopProxy*
+    GuestToHostChannel::ProxyChannelDelegateImpl::GetIPCMessageLoop() {
+  DCHECK(ChildProcess::current()) << "Must be in the renderer.";
+  return ChildProcess::current()->io_message_loop_proxy();
+}
+
+base::WaitableEvent*
+    GuestToHostChannel::ProxyChannelDelegateImpl::GetShutdownEvent() {
+  DCHECK(ChildProcess::current()) << "Must be in the renderer.";
+  return ChildProcess::current()->GetShutDownEvent();
+}
+
+GuestToHostChannel::GuestToHostChannel(GuestRenderViewObserver* guest,
+                                       WebKit::WebView* webview):

[jam, 2012/04/06 21:05:23]

per the style guide, this should be
    : ProxyChannel(...,
      webview_(...

[Fady Samuel, 2012/04/06 22:46:32]

Done.

+  ProxyChannel(base::GetCurrentProcessHandle()),
+  webview_(webview),
+  guest_(guest),
+  instance_(0),
+  context_(NULL),
+  width_(0),
+  height_(0) {
+}
+
+bool GuestToHostChannel::OnMessageReceived(const IPC::Message& message) {
+  bool handled = true;
+  bool msg_is_ok = true;
+  IPC_BEGIN_MESSAGE_MAP_EX(GuestToHostChannel, message, msg_is_ok)
+    IPC_MESSAGE_HANDLER(PpapiMsg_SupportsInterface, OnSupportsInterface)
+    IPC_MESSAGE_HANDLER(PpapiMsg_SetPreferences, OnSetPreferences)
+    IPC_MESSAGE_HANDLER(PpapiMsg_ReserveInstanceId, OnReserveInstanceId)
+    IPC_MESSAGE_HANDLER(PpapiMsg_PPPInstance_DidCreate,
+                        OnDidCreate)
+    IPC_MESSAGE_HANDLER(PpapiMsg_PPPInstance_DidChangeView,
+                        OnDidChangeView)
+    IPC_MESSAGE_HANDLER(PpapiMsg_PPPInstance_DidChangeFocus,
+                        OnDidChangeFocus)
+    IPC_MESSAGE_HANDLER(PpapiMsg_PPPMessaging_HandleMessage,
+                        OnHandleMessage)
+    IPC_MESSAGE_HANDLER(PpapiMsg_PPPInputEvent_HandleFilteredInputEvent,
+                        OnHandleFilteredInputEvent)
+    IPC_MESSAGE_HANDLER(PpapiMsg_PPPGraphics3D_ContextLost,
+                        OnContextLost)
+    // Have the super handle all other messages.
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+
+  if (!msg_is_ok) {
+    // The message had a handler, but its deserialization failed.
+    // Kill the renderer to avoid potential spoofing attacks.
+    CHECK(false) << "Unable to deserialize message in ProxyChannel.";
+  }
+  return handled;
+}
+
+bool GuestToHostChannel::Send(IPC::Message* message) {
+  message->set_unblock(true);

[jam, 2012/04/06 21:05:23]

are you sure you need this?

[Fady Samuel, 2012/04/06 22:46:32]

Yes, I believe so. Lots of weird things failing if not. piman@ has a better
explanation, I don't fully understand it myself. According to piman@, in pepper,
plugin->renderer messages always reenter, renderer->plugin messages never
reenter. Antoine, could you please explain a bit further why? It would be nice
to put a good comment in here, so that someone reading this code in 6 months
will understand why we're doing what we're doing here.

[piman, 2012/04/06 23:19:26]

Actually, yes. We need pepper->renderer messages to re-enter (even async
messages) to preserve ordering. See also ppapi::proxy::PluginDispatcher::Send.
There is a comment there to explain why, I think it's worth copying back here.

[Fady Samuel, 2012/04/11 22:06:34]

Done.

+  return ProxyChannel::Send(message);
+}
+
+void GuestToHostChannel::OnChannelError() {
+  // Cleanup should happen in BrowserPluginWebContentsObserver now.
+}
+
+WebGraphicsContext3DCommandBufferImpl*
+GuestToHostChannel::GetWebGraphicsContext3D(

[jam, 2012/04/06 21:05:23]

nit: the convention is when you have to put the function name on a new line,
indent it by 4 spaces (and indent the next line by another 4 spaces)

[Fady Samuel, 2012/04/06 22:46:32]

Done.

+    const WebKit::WebGraphicsContext3D::Attributes& attributes) {
+  attributes_ = attributes;
+  context_= new WebGraphicsContext3DCommandBufferImpl(

[piman, 2012/04/06 23:19:26]

As mentioned in my comment in render_view_impl.cc, caching this context locally
(and assuming on only have one at a time) is asking for trouble.

[Fady Samuel, 2012/04/11 22:06:34]

Done. This required some significant refactoring however.

+      0, GURL(), NULL,
+      static_cast<RenderViewImpl*>(guest_->render_view())->AsWeakPtr());
+  return context_;
+}
+
+void GuestToHostChannel::IssueSwapBuffers() {
+  IPC::Message* msg = new PpapiHostMsg_PPBGraphics3D_SwapBuffers(
+      ppapi::API_ID_PPB_GRAPHICS_3D, resource_);
+  Send(msg);

[jam, 2012/04/06 21:05:23]

nit: just Send(new Papi...)

[Fady Samuel, 2012/04/06 22:46:32]

Done.

+}
+
+bool GuestToHostChannel::InitChannel(const IPC::ChannelHandle& channel_handle) {
+  return ProxyChannel::InitWithChannel(&delegate_, channel_handle, false);
+}
+
+void GuestToHostChannel::OnSupportsInterface(
+    const std::string& interface_name,
+    bool* result) {
+
+  // TODO: This is a hack to avoid getting GetInstanceObject messages
+  // and failing a CHECK. A more correct solution is to implement
+  // VarSerializationRules for GuestToHostChannel.
+  if (interface_name.find("PPP_Instance_Private") != std::string::npos)

[piman, 2012/04/06 23:19:26]

*result = !(interface_name.find("PPP_Instance_Private") != std::string::npos)

Does it need to be a find, or would a simple string equality work?
Can you describe why you claim to support every PPP interface but not this one
in particular? Shouldn't you whitelist the ones that you do support?

[Fady Samuel, 2012/04/11 22:06:34]

Fixed. I do mention this above. This is currently a hack. A proper fix (where we
just support all interfaces) will come in a subsequent issue/bug. My hope here
is we agree on the barebones design first and these details will be addressed in
later patches.

+    *result = false;
+  else
+    *result = true;
+}
+
+void GuestToHostChannel::OnSetPreferences(const ppapi::Preferences& prefs) {
+  // TODO(fsamuel): Do we care about these preferences?
+  // These look like some font stuff from WebPreferences.
+  // Perhaps this should be plumbed into our associated RenderView?
+  NOTIMPLEMENTED();
+}
+
+void GuestToHostChannel::OnReserveInstanceId(PP_Instance instance,
+                                             bool* usable) {
+  // TODO(fsamuel): Only one instance per guest, for now.
+  // If we wish to support "live thumbnails" of guests this may
+  // change in the future.
+  DCHECK(!instance_);
+  *usable = true;
+}
+
+void GuestToHostChannel::RequestInputEvents() {
+  // Request receipt of input events
+  Send(new PpapiHostMsg_PPBInstance_RequestInputEvents(
+      ppapi::API_ID_PPB_INSTANCE, instance_, true,
+      PP_INPUTEVENT_CLASS_MOUSE |
+      PP_INPUTEVENT_CLASS_KEYBOARD |
+      PP_INPUTEVENT_CLASS_WHEEL |
+      PP_INPUTEVENT_CLASS_TOUCH));
+}
+
+void GuestToHostChannel::CreateGraphicsContext() {
+  std::vector<int32_t> attribs;
+  attribs.push_back(PP_GRAPHICS3DATTRIB_WIDTH);
+  attribs.push_back(webview_->size().width);
+  attribs.push_back(PP_GRAPHICS3DATTRIB_HEIGHT);
+  attribs.push_back(webview_->size().height);
+  attribs.push_back(PP_GRAPHICS3DATTRIB_NONE);
+
+  // Reset the HostResource if it's already set.
+  resource_ = ppapi::HostResource();
+  bool success = Send(new PpapiHostMsg_PPBGraphics3D_Create(
+        ppapi::API_ID_PPB_GRAPHICS_3D, instance_, attribs, &resource_));
+  DCHECK(success && !resource_.is_null());

[piman, 2012/04/06 23:19:26]

You won't get a resource if accelerated compositing is broken.
You should fail gracefully...

[Fady Samuel, 2012/04/11 22:06:34]

Done.

+  PP_Bool result = PP_FALSE;
+  Send(new PpapiHostMsg_PPBInstance_BindGraphics(
+       ppapi::API_ID_PPB_INSTANCE, instance_, resource_,
+       &result));
+  DCHECK(result == PP_TRUE);
+
+  CommandBufferProxy* command_buffer =
+      new ppapi::proxy::PpapiCommandBufferProxy(resource_, this);
+  command_buffer->Initialize();
+  context_->InitializeWithCommandBuffer(
+          command_buffer,

[jam, 2012/04/06 21:05:23]

nit: here and below, this is odd indentation

[Fady Samuel, 2012/04/06 22:46:32]

Done.

+          attributes_,
+          false /* bind generates resources */);
+
+  content::RenderThread::Get()->GetMessageLoop()->PostTask(
+           FROM_HERE,
+           base::Bind(&RenderViewImpl::OnGuestReady,
+              base::Unretained(

[piman, 2012/04/06 23:19:26]

Unretained? What guarantees that the lifetime of the renderview exceeds that of
the task?

[Fady Samuel, 2012/04/11 22:06:34]

Done.

+                  static_cast<RenderViewImpl*>(guest_->render_view()))));

[piman, 2012/04/06 23:19:26]

again, scary static_cast...

[Fady Samuel, 2012/04/11 22:06:34]

Done.

+}
+
+// Message handlers from ppp_instance_proxy

[jam, 2012/04/06 21:05:23]

nit: this comment is redundant (see the style guide about when to write/not
write comments)

[Fady Samuel, 2012/04/06 22:46:32]

Done.

+void GuestToHostChannel::OnDidCreate(PP_Instance instance,
+    const std::vector<std::string>& argn,
+    const std::vector<std::string>& argv,
+    PP_Bool* result) {
+  // Creation is always successful if we've gotten this far.
+  *result = PP_TRUE;
+  // Request input events from the host renderer after we reply to this message.
+  instance_ = instance;
+  // Request input events from the host renderer.

[jam, 2012/04/06 21:05:23]

ditto and below (comments shouldn't describe what code does)

[Fady Samuel, 2012/04/06 22:46:32]

Done.

+  RequestInputEvents();
+  // Request a graphics context from the host renderer.
+  CreateGraphicsContext();

[piman, 2012/04/06 23:19:26]

Do you need to create now?
In particular, at this point, you won't have a size yet, will you?

[Fady Samuel, 2012/04/11 22:06:34]

Done.

+}
+
+void GuestToHostChannel::OnDidChangeView(
+    PP_Instance instance,
+    const ppapi::ViewData& new_data,
+    PP_Bool flash_fullscreen) {
+  // If size has changed, let the browser process know.

[jam, 2012/04/06 21:05:23]

ditto

[Fady Samuel, 2012/04/06 22:46:32]

Done.

+  if (new_data.rect.size.width != width_ ||
+      new_data.rect.size.height != height_) {
+    width_ = new_data.rect.size.width;
+    height_ = new_data.rect.size.height;
+    guest_->Send(
+        new BrowserPluginHostMsg_ResizeGuest(guest_->routing_id(),
+                                             width_, height_));

[piman, 2012/04/06 23:19:26]

I'm a bit confused about the entire architecture, but if I read correctly, when
the host wants to resize the guest the following happens:
- host renderer webkit resizes the plugin, sends a message to guest
- (here) guest receives OnDidChangeView, sends a message to the browser
- browser receives the ResizeGuest, sends a message to the guest to resize it
- guest resizes the renderview.

With all the round trips it can't be fast, especially if it has to go to the UI
thread... Wouldn't it be preferable and simpler to not involve the browser, and
drive the renderview from here?

[Fady Samuel, 2012/04/11 22:06:34]

I'm worried about the implications of the browser having a stale view of the
size of the RenderView, but I haven't tried to see if this causes problems.

+  }
+}
+
+void GuestToHostChannel::OnDidChangeFocus(PP_Instance instance,
+                                          PP_Bool has_focus) {
+  NOTIMPLEMENTED();
+}
+
+void GuestToHostChannel::OnHandleMessage(
+    PP_Instance instance,
+    ppapi::proxy::SerializedVarReceiveInput message_data) {
+  // TODO(fsamuel): API messages to the browser plugin should probably
+  // be handled here.
+  NOTIMPLEMENTED();
+}
+
+void GuestToHostChannel::OnHandleFilteredInputEvent(
+    PP_Instance instance,
+    const ppapi::InputEventData& data,
+    PP_Bool* result) {
+  // We only support one instance per channel at this point in time.
+  DCHECK(instance == instance_);
+  scoped_ptr<WebKit::WebInputEvent> web_input_event(
+      webkit::ppapi::CreateWebInputEvent(data));
+  if (webview_->handleInputEvent(*web_input_event))
+    *result = PP_TRUE;

[piman, 2012/04/06 23:19:26]

Use PP_FromBool (in ppapi/c/pp_bool.h)?

[Fady Samuel, 2012/04/11 22:06:34]

Done.

+  else
+    *result = PP_FALSE;
+}
+
+void GuestToHostChannel::OnContextLost(PP_Instance instance) {
+  DCHECK(instance == instance_);
+  ppapi::proxy::PpapiCommandBufferProxy* command_buffer =
+    static_cast<ppapi::proxy::PpapiCommandBufferProxy*>(
+        context_->GetCommandBufferProxy());
+  DCHECK(command_buffer);

[jam, 2012/04/06 21:05:23]

nit this isn't useful. in release builds it does nothing. in debug, the crash on
the the next line is just as useful

[Fady Samuel, 2012/04/06 22:46:32]

Done.

[piman, 2012/04/06 23:19:26]

Actually, I disagree with that. If ReportChannelError is not virtual, just
calling it may not crash.
If e.g. PpapiCommandBufferProxy::ReportChannelError is implemented as
PostTask(..., this, &PpapiCommandBufferProxy::SomeVirtualMethod) then we posted
a task that will crash when executing, and it's super hard to debug where it
comes from.
Better safe than sorry, verifying expectations is good...

[Fady Samuel, 2012/04/11 22:06:34]

Done.

+  context_= GetWebGraphicsContext3D(attributes_);
+  command_buffer->ReportChannelError();
+
+  // Create a new graphics context

[jam, 2012/04/06 21:05:23]

ditto

[Fady Samuel, 2012/04/06 22:46:32]

Done.

+  CreateGraphicsContext();
+}