Browser Plugin: browser process side changes

content/browser/browser_plugin/browser_plugin_host.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.
+
+#ifndef CONTENT_BROWSER_BROWSER_PLUGIN_BROWSER_PLUGIN_HOST_H__
+#define CONTENT_BROWSER_BROWSER_PLUGIN_BROWSER_PLUGIN_HOST_H__
+#pragma once
+
+#include <map>
+#include <set>
+
+#include "content/public/browser/notification_registrar.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "googleurl/src/gurl.h"
+#include "ppapi/c/pp_instance.h"
+#include "ui/gfx/size.h"
+
+class WebContentsImpl;
+
+namespace content {
+
+// The primary purpose of BrowserPluginHost is to allow an embedder
+// to manage the lifetime of its guests. It cleans up its guests
+// on navigation, crashes, and 'hides" guests when it hides.

[Charlie Reis, 2012/05/17 21:24:05]

nit: 'hides" -> "hides"
DRAFT: Should that be swaps out instead of hides?

[Fady Samuel, 2012/05/18 17:37:52]

Fixed. That will make tabbing back to the embedder render view less responsive
because it will have to swap in the guest RenderViews again?

[Charlie Reis, 2012/05/18 20:35:24]

Ah, sorry about the draft comment.  I was going to check whether "hiding" was
referring to simply hiding the tab in favor of another or to navigating away.  I
think it's fine as is.

+// For the guest, BrowserPluginHost keeps track of its embedder,
+// its BrowserPlugin instance ID, and some initialization state
+// such as initial size and URL.
+class BrowserPluginHost: public WebContentsObserver,
+                         public NotificationObserver {
+ public:
+  BrowserPluginHost(WebContentsImpl* web_contents);
+
+  virtual ~BrowserPluginHost();
+
+  // Get a guest BrowserPluginHost by its container ID specified
+  // in BrowserPlugin.
+  BrowserPluginHost* GetGuestByContainerID(int container_id);
+
+  // Associate a guest by its container ID.

[Charlie Reis, 2012/05/17 21:24:05]

nit: by -> with

[Fady Samuel, 2012/05/18 17:37:52]

Done.

+  void RegisterContainerInstance(
+      int container_id,
+      BrowserPluginHost* observer);
+
+  // An embedder BrowserPluginHost keeps track of
+  // its guests so that if it navigates away, its associated RenderView
+  // crashes or it is hidden, it takes appropriate action on the guest.
+  void AddGuest(WebContentsImpl* guest, int64 frame_id);
+
+  // This removes a guest from an embedder's guest list.
+  // TODO(fsamuel): Use this when deleting guest after they crash.
+  // ToT plugin crash handling seems to be broken in this case and so I can't
+  // test this scenario at the moment. Delete this comment once fixed.
+  void RemoveGuest(WebContentsImpl* guest);
+
+  WebContentsImpl* embedder() const { return embedder_; }
+  void set_embedder(WebContentsImpl* embedder) { embedder_ = embedder; }
+  int instance_id() const { return instance_id_; }
+  void set_instance_id(int instance_id) { instance_id_ = instance_id; }
+  const GURL& url() const { return url_; }
+  void set_url(const GURL& url) { url_ = url; }

[Charlie Reis, 2012/05/17 21:24:05]

This makes me nervous.  Why does the BrowserPluginHost need to keep track of the
URL?  This is something that can change in a lot of ways and mean different
things at different points in time (e.g., while loads are pending).  Best to
leave it within WebContents if possible, to avoid any risk of spoofing-like
bugs.

+  const gfx::Size& initial_size() const { return initial_size_; }
+  void set_initial_size(const gfx::Size& size) { initial_size_ = size; }
+  RenderViewHost* pending_render_view_host() const {
+    return pending_render_view_host_;
+  }
+
+  void OnCrossProcessNavigation(RenderViewHost* dest_rvh);

[Charlie Reis, 2012/05/17 21:24:05]

This naming is unclear: is it when the navigation starts or finishes?

+
+  // WebContentObserver implementation.
+  virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
+  virtual void DidCommitProvisionalLoadForFrame(
+      int64 frame_id,
+      bool is_main_frame,
+      const GURL& url,
+      PageTransition transition_type) OVERRIDE;
+  virtual void RenderViewDeleted(RenderViewHost* render_view_host) OVERRIDE;
+  virtual void RenderViewGone(base::TerminationStatus status) OVERRIDE;
+  virtual void WebContentsDestroyed(WebContents* web_contents) OVERRIDE;
+
+ private:
+  typedef std::map<WebContentsImpl*, int64> GuestMap;
+  typedef std::map<int, BrowserPluginHost*> ContainerInstanceMap;
+
+  void OnNavigateFromEmbedder(int32 instance_id,
+                              long long frame_id,
+                              const std::string& src,
+                              const gfx::Size& size);
+
+  void OnNavigateFromGuest(PP_Instance instance,
+                           const std::string& src);
+
+  void OnMapInstance(int routing_id, PP_Instance instance);
+
+  void OnResizeGuest(int width, int height);
+
+  void OnConnectToChannel(const IPC::ChannelHandle& handle);
+
+  void DestroyGuests();
+
+  // NotificationObserver method override.
+  virtual void Observe(int type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details) OVERRIDE;
+
+  // A scoped container for notification registries.
+  NotificationRegistrar registrar_;
+  WebContentsImpl* embedder_;
+  // An identifier that uniquely identifies a browser plugin container
+  // within an embedder.
+  int instance_id_;
+  GURL url_;
+  gfx::Size initial_size_;
+  GuestMap guests_;
+  ContainerInstanceMap guests_by_container_id_;
+  RenderViewHost* pending_render_view_host_;

[Charlie Reis, 2012/05/17 21:24:05]

I'm still not a fan of keeping track of this outside WebContentsImpl.  I'd
rather see anything that depends on it get pushed inside RenderViewHostImpl or
WebContentsImpl if possible.

+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_BROWSER_PLUGIN_BROWSER_PLUGIN_HOST_H_