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 74 matching lines @@
 
 std::string BrowserPlugin::GetSrcAttribute() const {
   return src_;
 }
 
 void BrowserPlugin::SetSrcAttribute(const std::string& src) {
   if (src == src_ && !guest_crashed_)
     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;
 }
 
 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 212 matching lines @@
     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)
     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 *
                       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