Browser Plugin: New Implementation (Browser Side)

content/renderer/browser_plugin/browser_plugin.cc

 // 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.
 
 #include "content/renderer/browser_plugin/browser_plugin.h"
 
 #include "base/message_loop.h"
 #include "base/string_util.h"
 #include "content/common/browser_plugin_messages.h"
 #include "content/public/common/content_client.h"
@@ skipping 36 matching lines @@
     RenderViewImpl* render_view,
     WebKit::WebFrame* frame,
     const WebPluginParams& params)
     : instance_id_(instance_id),
       render_view_(render_view),
       container_(NULL),
       damage_buffer_(NULL),
       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));
 
   std::string src;
   if (ParseSrcAttribute(params, &src))
     SetSrcAttribute(src);
 }
 
 BrowserPlugin::~BrowserPlugin() {
@@ skipping 13 matching lines @@
   if (damage_buffer_) {
     RenderProcess::current()->FreeTransportDIB(damage_buffer_);
     damage_buffer_ = NULL;
   }
 }
 
 std::string BrowserPlugin::GetSrcAttribute() const {
   return src_;
 }
 
 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;
 }
 
 bool BrowserPlugin::ParseSrcAttribute(
     const WebKit::WebPluginParams& params,
     std::string* src) {
   // Get the src attribute from the attributes vector
   for (unsigned i = 0; i < params.attributeNames.size(); ++i) {
@@ skipping 210 matching lines @@
 
 void BrowserPlugin::updateGeometry(
     const WebRect& window_rect,
     const WebRect& clip_rect,
     const WebVector<WebRect>& cut_outs_rects,
     bool is_visible) {
   int old_width = width();
   int old_height = height();
   plugin_rect_ = window_rect;
   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 *
                       stride *
                       GetDeviceScaleFactor() *
                       GetDeviceScaleFactor();
 
   // Don't drop the old damage buffer until after we've made sure that the
   // browser process has dropped it.
   TransportDIB* new_damage_buffer =
@@ skipping 70 matching lines @@
                                                  void* notify_data) {
 }
 
 void BrowserPlugin::didFailLoadingFrameRequest(
     const WebKit::WebURL& url,
     void* notify_data,
     const WebKit::WebURLError& error) {
 }
 
 }  // namespace content