Browser Plugin: New Implementation (Browser Side)

content/renderer/browser_plugin/browser_plugin.cc

Index: content/renderer/browser_plugin/browser_plugin.cc
diff --git a/content/renderer/browser_plugin/browser_plugin.cc b/content/renderer/browser_plugin/browser_plugin.cc
index 8ba5421f33aa700fe63e733024d9a5185ed74684..88a2a91421bc130d6f7e7ca7232125bf9f3d7cc8 100644
--- a/content/renderer/browser_plugin/browser_plugin.cc
+++ b/content/renderer/browser_plugin/browser_plugin.cc
@@ -92,11 +92,12 @@ void BrowserPlugin::SetSrcAttribute(const std::string& src) {
     return;
   if (!src.empty()) {
     BrowserPluginManager::Get()->Send(
-        new BrowserPluginHostMsg_NavigateOrCreateGuest(
+        new BrowserPluginHostMsg_NavigateGuest(
             render_view_->GetRoutingID(),
             instance_id_,
             parent_frame_,
-            src));
+            src,
+            gfx::Size(width(), height())));
   }
   src_ = src;
   guest_crashed_ = false;
@@ -329,6 +330,10 @@ void BrowserPlugin::updateGeometry(
   if (old_width == window_rect.width &&
       old_height == window_rect.height)
     return;
+  // TODO(fsamuel): Check if this is correct: When src is not set, we shouldn't
+  // send UpdateRect messages to the host.

[Charlie Reis, 2012/08/27 20:23:59]

Can we determine if this is correct or not, rather than committing with the
TODO?

[lazyboy, 2012/08/28 19:07:14]

Fady can you asnwer this one?:
I've added this so empty src doesn't trigger UpdateRect to browser process.
Initially on page when there's plugin with src="", we don't send any
NavigateGuest_Msg and the browser side yet doesn't have any browser plugin
embedder. Now if we send UpdateRect_Msg from here it will hang since we won't
receive any reply (no browser_plugin_embedder message handler exists).

[Fady Samuel, 2012/08/29 08:16:20]

So when the src attribute is not set, we may or may not have a
BrowserPluginGuest. If we have a BrowserPluginGuest, its WebContents doesn't
need to be resized because we're not doing anything with it. If we don't have a
BrowserPluginGuest, we really shouldn't be creating one just to resize it. This
seems reasonable to me.

A point Sean made a couple of weeks ago that may be worth considering is what
happens if we navigate to a page (say http://www.google.com) and then set
src="". Should we keep a BrowserPluginGuest, and the old RVH lingering around
while src=""? Certainly, js might still be running on that old page, and it
might be sending updated pixels to the BrowserPlugin, which we're discarding.
This seems like a waste of cycles? Perhaps setting src="" should swap out the
guest RenderView?

Another concern (I can't remember if I tested this or not, it might be worth
writing a test for this) is the following:

1. set src="http://www.google.com" size (640, 480)
2. set src=""
3. set size (400, 400)
4. Navigate to "http://www.chromium.org"

Will chromium.org layout in the correct (400, 400) size?

I think I fixed this right before vacation (NavigateGuest takes a size now
right?) but could you please verify this?

[Charlie Reis, 2012/08/29 18:08:12]

As for Sean's question, I think manually assigning src to "" should cause a
navigation to about:blank, not swap out the guest.  (A swapped out RVH should
never be visible.)

[lazyboy, 2012/08/31 00:38:04]

I've resolved this issue.

Ignoring everything when the src="" is not sufficient since Fady's case won't be
handled properly (because if have special empty src case handling in
BrowserPluginGuest::SetSrcAttribute(..)

Right now I ignore only initial empty src attributes; All other src attributes
(including empty ones) after a proper navigate (/w non empty src) are always
sent to embedder.

Tested with the test case scenario mentioned by Fady, works well now. Also
checked without the step 3 (to make sure resize isn't forcing some kind of
painting).

+  if (src_.empty())
+    return;
 
   const size_t stride = skia::PlatformCanvas::StrideForWidth(window_rect.width);
   const size_t size = window_rect.height *