Browser Plugin: New Implementation (Browser Side)

content/browser/browser_plugin/browser_plugin_guest.h

+// Copyright (c) 2012 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.
+
+// A BrowserPluginGuest represents the browser side of browser <--> renderer
+// communication. A BrowserPlugin (a WebPlugin) is on the renderer side of
+// browser <--> guest renderer communication. The 'guest' renderer is a
+// <browser> tag.
+//
+// BrowserPluginGuest lives on the UI thread of the browser process. It has a
+// helper, BrowserPluginGuestHelper, which is a RenderViewHostObserver. The
+// helper object receives messages (ViewHostMsg_*) directed at the browser
+// plugin and redirects them to this class. Any messages the embedder might be
+// interested in knowing or modifying about the guest should be listened for
+// here.
+//
+// Since BrowserPlugin is a WebPlugin, we need to provide overridden behaviors
+// for messages like handleInputEvent, updateGeometry. Such messages get
+// routed into BrowserPluginGuest via its embedder (BrowserPluginEmbedder).
+// These are BrowserPluginHost_* messages sent from the BrowserPlugin.
+//
+// BrowserPluginGuest knows about its embedder process. Communication to
+// renderer happens through the embedder process.
+//
+// A BrowserPluginGuest is also associated directly with the WebContents related
+// to the BrowserPlugin. BrowserPluginGuest is a WebContentsDelegate and
+// WebContentsObserver for the WebContents.
+
+#ifndef CONTENT_BROWSER_BROWSER_PLUGIN_BROWSER_PLUGIN_GUEST_H_
+#define CONTENT_BROWSER_BROWSER_PLUGIN_BROWSER_PLUGIN_GUEST_H_
+
+#include <string>

[jam, 2012/09/17 17:47:52]

nit: order. also, including string is really unnecessary...

[lazyboy, 2012/09/17 20:22:24]

Done.

+#include <map>
+
+#include "base/compiler_specific.h"
+#include "base/id_map.h"
+#include "base/time.h"
+#include "content/browser/browser_plugin/browser_plugin_host_factory.h"
+#include "content/public/browser/render_view_host_observer.h"

[jam, 2012/09/17 17:47:53]

nit: why?

[lazyboy, 2012/09/17 20:22:24]

Removed (this class used to be RVHObserver).

+#include "content/public/browser/web_contents_delegate.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "ipc/ipc_channel_handle.h"

[jam, 2012/09/17 17:47:53]

nit: why?

[lazyboy, 2012/09/17 20:22:24]

Removed.

+#include "ipc/ipc_sync_message.h"

[jam, 2012/09/17 17:47:53]

why?

[lazyboy, 2012/09/17 20:22:24]

Removed.

+#include "third_party/WebKit/Source/WebKit/chromium/public/WebInputEvent.h"

[jam, 2012/09/17 17:47:53]

forward declare instead of including?

[lazyboy, 2012/09/17 20:22:24]

Done.

+#include "ui/surface/transport_dib.h"
+#include "ui/gfx/rect.h"
+#include "ui/gfx/size.h"

[jam, 2012/09/17 17:47:53]

not needed since you include rect

[lazyboy, 2012/09/17 20:22:24]

Done.
Doesn't this contradict style rule: "Do not depend on the symbol being brought
in transitively via headers not directly included."

[jam, 2012/09/17 20:47:57]

we don't include parent headers. i.e. if you include view_messages.h, you don't
also include ipc_message.h and ipc_sync_message.h.  similarly, gfx::Rect is
composed of a gfx::Point and gfx::Size by definition, so if you include rect you
don't need the other two.

[lazyboy, 2012/09/17 21:07:54]

I see, found one such view_message occurrence, Removed.

+#include "webkit/glue/webcursor.h"
+
+namespace gfx {
+class Size;

[jam, 2012/09/17 17:47:53]

not needed since you include the header.

[lazyboy, 2012/09/17 20:22:24]

Done.

+}
+
+struct BrowserPluginHostMsg_ResizeGuest_Params;
+struct ViewHostMsg_UpdateRect_Params;
+
+namespace content {
+
+class BrowserPluginHostFactory;
+class BrowserPluginEmbedder;
+class RenderProcessHost;
+
+// A browser plugin guest provides functionality for WebContents to operate in
+// the guest role and implements guest specific overrides for ViewHostMsg_*
+// messages.
+//
+// BrowserPluginEmbedder is responsible for creating and destroying a guest.
+class CONTENT_EXPORT BrowserPluginGuest : public WebContentsDelegate,
+                                          public WebContentsObserver {
+ public:
+  virtual ~BrowserPluginGuest();
+
+  static BrowserPluginGuest* Create(int instance_id,
+                                    WebContentsImpl* web_contents,
+                                    content::RenderViewHost* render_view_host);
+
+  // Overrides factory for testing. Default (NULL) value indicates regular
+  // (non-test) environment.
+  static void set_factory_for_testing(BrowserPluginHostFactory* factory) {
+    content::BrowserPluginGuest::factory_ = factory;
+  }
+
+  void SetGuestHangTimeoutForTesting(const base::TimeDelta& timeout) {

[jam, 2012/09/17 17:47:53]

nit: since this is inline: set_guest_hang_timeout_for_testing()

[lazyboy, 2012/09/17 20:22:24]

Done.

+    guest_hang_timeout_ = timeout;
+  }
+
+  // WebContentsObserver implementation.
+  virtual void DidCommitProvisionalLoadForFrame(
+      int64 frame_id,
+      bool is_main_frame,
+      const GURL& url,
+      PageTransition transition_type,
+      RenderViewHost* render_view_host) OVERRIDE;
+  virtual void RenderViewGone(base::TerminationStatus status) OVERRIDE;
+
+  // WebContentsDelegate implementation.
+  virtual void RendererUnresponsive(WebContents* source) OVERRIDE;
+
+ private:
+  friend class BrowserPluginEmbedder;
+  friend class BrowserPluginGuestHelper;
+  friend class TestBrowserPluginGuest;
+
+  BrowserPluginGuest(int instance_id,
+                     WebContentsImpl* web_contents,
+                     RenderViewHost* render_view_host);
+
+  void set_embedder_render_process_host(

[jam, 2012/09/17 17:47:53]

to make this header easier to read, can you:

-put all the setters/getters together for one block (with the _for_testing ones
at the end)
-put all the virtual methods that are overridden for tests in one block

[lazyboy, 2012/09/17 20:22:24]

Done.

[jam, 2012/09/17 22:13:36]

doesn't look like it?

[lazyboy, 2012/09/17 22:37:48]

Sorry, not sure if I follow the first one: this function and the next 6 are
getters/setters.
Did you mean to put SetDamageBuffer out of the group since this is not inline
setter?

I misread the second one. Fixed.

[jam, 2012/09/17 23:08:56]

exactly

+      RenderProcessHost* render_process_host) {
+    embedder_render_process_host_ = render_process_host;
+  }
+  RenderProcessHost* embedder_render_process_host() {
+    return embedder_render_process_host_;
+  }
+  // Returns the identifier that uniquely identifies a browser plugin guest
+  // within an embedder.
+  int instance_id() const { return instance_id_; }
+
+  void SetDamageBuffer(TransportDIB* damage_buffer,
+#if defined(OS_WIN)
+                       int damage_buffer_size,
+#endif
+                       const gfx::Size& damage_view_size,
+                       float scale_factor);
+  TransportDIB* damage_buffer() const { return damage_buffer_.get(); }
+  const gfx::Size& damage_view_size() const { return damage_view_size_; }
+  float damage_buffer_scale_factor() const {
+    return damage_buffer_scale_factor_;
+  }
+
+  void UpdateRect(RenderViewHost* render_view_host,
+                  const ViewHostMsg_UpdateRect_Params& params);
+  void UpdateRectACK(int message_id, const gfx::Size& size);
+  // Handles input event routed through the embedder (which is initiated in the
+  // browser plugin (renderer side of the embedder)).
+  void HandleInputEvent(RenderViewHost* render_view_host,
+                        const gfx::Rect& guest_rect,
+                        const WebKit::WebInputEvent& event,
+                        IPC::Message* reply_message);
+  // Overrides default ShowWidget message so we show them on the correct
+  // coordinates.
+  void ShowWidget(RenderViewHost* render_view_host,
+                  int route_id,
+                  const gfx::Rect& initial_pos);
+  // Overridden in tests.
+  virtual void SetFocus(bool focused);
+  void SetCursor(const WebCursor& cursor);
+  // Handles input event acks so they are sent to browser plugin host (via
+  // embedder) instead of default view/widget host.
+  void HandleInputEventAck(RenderViewHost* render_view_host, bool handled);
+
+  // Helper to send messages to embedder. Overridden in test implementation
+  // since we want to intercept certain messages for testing.
+  virtual void SendMessageToEmbedder(IPC::Message*);

[jam, 2012/09/17 17:47:53]

nit: chrome style is to always list the parameter name

[lazyboy, 2012/09/17 20:22:24]

Done.

+
+  // Static factory instance (always NULL for non-test).
+  static content::BrowserPluginHostFactory* factory_;
+
+  // Overridden in tests.
+  virtual bool ViewTakeFocus(bool reverse);
+
+  RenderProcessHost* embedder_render_process_host_;
+  // An identifier that uniquely identifies a browser plugin guest within an
+  // embedder.
+  int instance_id_;
+  scoped_ptr<TransportDIB> damage_buffer_;
+#if defined(OS_WIN)
+  size_t damage_buffer_size_;
+#endif
+  gfx::Size damage_view_size_;
+  float damage_buffer_scale_factor_;
+  scoped_ptr<IPC::Message> pending_input_event_reply_;
+  gfx::Rect guest_rect_;
+  WebCursor cursor_;
+  IDMap<RenderViewHost> pending_updates_;
+  int pending_update_counter_;
+  base::TimeDelta guest_hang_timeout_;
+
+  DISALLOW_COPY_AND_ASSIGN(BrowserPluginGuest);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_BROWSER_PLUGIN_BROWSER_PLUGIN_GUEST_H_