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..01f021b76793d22c75a0844383088aa499cee142 100644
--- a/content/renderer/browser_plugin/browser_plugin.cc
+++ b/content/renderer/browser_plugin/browser_plugin.cc
@@ -54,6 +54,7 @@ BrowserPlugin::BrowserPlugin(
       sad_guest_(NULL),
       guest_crashed_(false),
       resize_pending_(false),
+      navigate_src_sent_(false),
       parent_frame_(frame->identifier()) {
   BrowserPluginManager::Get()->AddBrowserPlugin(instance_id, this);
   bindings_.reset(new BrowserPluginBindings(this));
@@ -90,13 +91,21 @@ std::string BrowserPlugin::GetSrcAttribute() const {
 void BrowserPlugin::SetSrcAttribute(const std::string& src) {

[awong, 2012/09/06 19:55:26]

On Charlie's comment that on src.empty(), we should navigate to about:blank,
what's the expected behavior.

if someone does in JS

  guest.src = "";

and then later does

  alert(guest.src);

Should we get:
  1) alert with "", and the embedder has no guest.
  2) alert with "", and the guest is rendering about:blank behind the scenes?
  3) alert with "about:blank", and the guest is rendering about:blank?

[lazyboy, 2012/09/07 19:33:19]

If we haven't loaded a guest at all in |guest|, we'd get alert with ="",
embedder has no guest.
But say we loaded a |guest| with src="something" (and we have a guest) and later
set src="", we'd get alert with "", the WebContents will load with "" as src,
and js will alert with "".

[awong, 2012/09/07 20:51:03]

If we navigate the existing WebContents to "", what gets rendered?

Charlie's specific comment was:

"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.)"

I'm trying to understand if this addresses his concern...

[lazyboy, 2012/09/08 02:12:22]

If we navigate the existing WebContents to "", what gets rendered?
-> white rectangle, see BrowserPlugin::paint()

I think manually assigning src to "" should cause a
navigation to about:blank, not swap out the guest. 
-> a. If we have src="" in the beginning, we don't have any guest (browser
side); b. If we have src='something', we have a guest GG (browser side), now if
we set src='', we still have GG, but its WC now did LoadUrl(''). I think the
concern was if we swap out GG, which we are not doing. (Again, if I understand
the concern correctly).

On 2012/09/07 20:51:03, awong wrote:

   if (src == src_ && !guest_crashed_)
     return;
-  if (!src.empty()) {
+  if (!src.empty() || navigate_src_sent_) {
     BrowserPluginManager::Get()->Send(
-        new BrowserPluginHostMsg_NavigateOrCreateGuest(
+        new BrowserPluginHostMsg_NavigateGuest(
             render_view_->GetRoutingID(),
             instance_id_,
             parent_frame_,
-            src));
+            src,
+            gfx::Size(width(), height())));
+    // Record that we sent a NavigateGuest message to embedder. Once we send
+    // such a message, subsequent SetSrcAttribute() calls  must always send

[awong, 2012/09/06 19:55:26]

nit: extra space after calls

[lazyboy, 2012/09/07 19:33:19]

Done.

+    // NavigateGuest messages to the embedder (even if |src| is empty), so
+    // resize works correctly for all cases (e.g. The embedder can reset the
+    // guest's |src| to empty value, resize and then set the |src| to a
+    // non-empty value).

[awong, 2012/09/06 19:55:26]

Awesome. That's way more clear :)

[lazyboy, 2012/09/07 19:33:19]

Done.

+    navigate_src_sent_ = true;
   }
   src_ = src;
   guest_crashed_ = false;
@@ -329,6 +338,10 @@ void BrowserPlugin::updateGeometry(
   if (old_width == window_rect.width &&
       old_height == window_rect.height)

[awong, 2012/09/06 19:55:26]

use braces if you have a multi-line conditional 

[lazyboy, 2012/09/07 19:33:19]

Done.

     return;
+  // Don't send anything to embedder until an actual navigation occurs, since

[awong, 2012/09/06 19:55:26]

nit: remove comment.

Also, I think it we should callout that the plugin's geometry still should be
updated.  Maybe phrase it like:

"Until an actual navigation occurs, there is no browser side embedder present to
notify about geometry updates. In this case, after we've updated the
BrowserPlugin's state we are done and can return immediately."

[Fady Samuel, 2012/09/06 21:08:15]

As discussed offline, please also verify that we're actually navigation away
from the current page when src is set to "". 

[lazyboy, 2012/09/07 19:33:19]

Done.

[lazyboy, 2012/09/07 19:33:19]

I've tested with
1. set guest src to a infinite loop (for(;;);) page
2. after 3 seconds set the src to an empty page (src="")
3. send input to the guest and observe it doesn't crash.
So, we do navigate away from the current page, however, I can't verify this way
if the old guest is still lying around for some reason (no way to send any js
message from guest to embedder to track it), but would be possible to do it in a
browsertest (Fady volunteered to write one :))

+  // there is no browser side embedder present to handle it.
+  if (src_.empty() && !navigate_src_sent_)
+    return;
 
   const size_t stride = skia::PlatformCanvas::StrideForWidth(window_rect.width);
   const size_t size = window_rect.height *