Implement BrowserPluginPlaceholder.

Implement BrowserPluginPlaceholder.

A browser plugin is a plugin container that hosts an out-of-process RenderView (guest).

Loading up a new process, creating a new RenderView, navigating to a given URL,
and establishing a guest-to-host channel can take hundreds of milliseconds.

Furthermore, a RenderView's associated browser-side WebContents, RenderViewHost, and
SiteInstance must be created and accessed on the UI thread thread on the browser.

Thus, we must avoid blocking the host RenderView as well to avoid introducing the potential
for a deadlock.

To address the two issues above, we use a BrowserPluginPlaceholder (currently an empty
WebViewPlugin wrapper) to take place of the guest renderer until the guest renderer is
ready.

BUG=117894
TEST=manually

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126534

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126598

[jam, 2012-03-12 16:30:09]

this is a large feature so it need a bug to explain it and track all the changes

http://codereview.chromium.org/9668031/diff/1/content/public/common/content_s...
File content/public/common/content_switches.h (right):

http://codereview.chromium.org/9668031/diff/1/content/public/common/content_s...
content/public/common/content_switches.h:81: CONTENT_EXPORT extern const char
kEnableBrowserPlugin[];
nit: no need for CONTENT_EXPORT. that's only needed for switches that embedders
of content (i.e. chrome) use

http://codereview.chromium.org/9668031/diff/1/content/renderer/plugins/browse...
File content/renderer/plugins/browser_plugin_placeholder.cc (right):

http://codereview.chromium.org/9668031/diff/1/content/renderer/plugins/browse...
content/renderer/plugins/browser_plugin_placeholder.cc:24: // the global list of
all renderer processes
nit: per style guide comments need to start with a capital letter and end with a
period

http://codereview.chromium.org/9668031/diff/1/content/renderer/plugins/browse...
content/renderer/plugins/browser_plugin_placeholder.cc:28: 
nit: extra line

http://codereview.chromium.org/9668031/diff/1/content/renderer/plugins/browse...
content/renderer/plugins/browser_plugin_placeholder.cc:134: CHECK(plugin_);
nit: no need for this. if it's null the next line will crash and it'll be just
as good of a signal

http://codereview.chromium.org/9668031/diff/1/content/renderer/plugins/browse...
File content/renderer/plugins/browser_plugin_placeholder.h (right):

http://codereview.chromium.org/9668031/diff/1/content/renderer/plugins/browse...
content/renderer/plugins/browser_plugin_placeholder.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
content/renderer/plugins is a bit misleading, since there's a bunch of generic
plugins code already in content/renderer. this stuff is also more
"browser_plugin" or "browser_tag" than just "plugins".

do you see a lot of files added for this feature to justify its own
subdirectory?

http://codereview.chromium.org/9668031/diff/1/content/renderer/plugins/browse...
content/renderer/plugins/browser_plugin_placeholder.h:10: #include
"content/public/renderer/render_process_observer.h"
why?

http://codereview.chromium.org/9668031/diff/1/content/renderer/plugins/browse...
content/renderer/plugins/browser_plugin_placeholder.h:11: #include
"content/public/renderer/render_view_observer.h"
ditto

http://codereview.chromium.org/9668031/diff/1/content/renderer/plugins/browse...
content/renderer/plugins/browser_plugin_placeholder.h:18: class RenderThread;
why?

http://codereview.chromium.org/9668031/diff/1/content/renderer/plugins/browse...
content/renderer/plugins/browser_plugin_placeholder.h:23: class WebFrame;
nit: not needed since thi sis in webview_plugin.h

http://codereview.chromium.org/9668031/diff/1/content/renderer/plugins/browse...
content/renderer/plugins/browser_plugin_placeholder.h:27: class
BrowserPluginPlaceholder: public webkit::WebViewPlugin::Delegate {
there should be a comment on this class saying what it's used for

http://codereview.chromium.org/9668031/diff/1/content/renderer/plugins/browse...
content/renderer/plugins/browser_plugin_placeholder.h:35: static
BrowserPluginPlaceholder* FromID(int id);
it's hard to see if this method is needed or not, or if there's an alternative
to avoiding the static map, since the code that uses this isn't sent for review
yet. why is this needed?

http://codereview.chromium.org/9668031/diff/1/content/renderer/plugins/browse...
content/renderer/plugins/browser_plugin_placeholder.h:38: void
UnregisterPlaceholder(int id);
ditto

http://codereview.chromium.org/9668031/diff/1/content/renderer/plugins/browse...
content/renderer/plugins/browser_plugin_placeholder.h:75: int height_;
nit: instead of width_ and height_, use gfx::Size

http://codereview.chromium.org/9668031/diff/1/content/renderer/render_view_im...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/9668031/diff/1/content/renderer/render_view_im...
content/renderer/render_view_impl.cc:2074: UTF16ToASCII(params.mimeType) ==
kBrowserPluginMimeType)
this constant isn't in webkit/plugins/plugin_constants.h...

[Fady Samuel, 2012-03-12 22:11:43]

Creating the bug to track progress on the Browser Plugin now. In the meantime,
I've addressed your other comments.

https://chromiumcodereview.appspot.com/9668031/diff/1/content/public/common/c...
File content/public/common/content_switches.h (right):

https://chromiumcodereview.appspot.com/9668031/diff/1/content/public/common/c...
content/public/common/content_switches.h:81: CONTENT_EXPORT extern const char
kEnableBrowserPlugin[];
On 2012/03/12 16:30:09, John Abd-El-Malek wrote:
> nit: no need for CONTENT_EXPORT. that's only needed for switches that
embedders
> of content (i.e. chrome) use

Yup, it did in a previous patch but I forgot to remove it after your comments.
Thanks. Removed.

https://chromiumcodereview.appspot.com/9668031/diff/1/content/renderer/plugin...
File content/renderer/plugins/browser_plugin_placeholder.cc (right):

https://chromiumcodereview.appspot.com/9668031/diff/1/content/renderer/plugin...
content/renderer/plugins/browser_plugin_placeholder.cc:24: // the global list of
all renderer processes
On 2012/03/12 16:30:09, John Abd-El-Malek wrote:
> nit: per style guide comments need to start with a capital letter and end with
a
> period
Fixed. Also, comment was wrong (from a copy-and-paste from another file). I
fixed that.

https://chromiumcodereview.appspot.com/9668031/diff/1/content/renderer/plugin...
content/renderer/plugins/browser_plugin_placeholder.cc:28: 
On 2012/03/12 16:30:09, John Abd-El-Malek wrote:
> nit: extra line

Fixed

https://chromiumcodereview.appspot.com/9668031/diff/1/content/renderer/plugin...
content/renderer/plugins/browser_plugin_placeholder.cc:134: CHECK(plugin_);
On 2012/03/12 16:30:09, John Abd-El-Malek wrote:
> nit: no need for this. if it's null the next line will crash and it'll be just
> as good of a signal

Done.

https://chromiumcodereview.appspot.com/9668031/diff/1/content/renderer/plugin...
File content/renderer/plugins/browser_plugin_placeholder.h (right):

https://chromiumcodereview.appspot.com/9668031/diff/1/content/renderer/plugin...
content/renderer/plugins/browser_plugin_placeholder.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2012/03/12 16:30:09, John Abd-El-Malek wrote:
> content/renderer/plugins is a bit misleading, since there's a bunch of generic
> plugins code already in content/renderer. this stuff is also more
> "browser_plugin" or "browser_tag" than just "plugins".
> 
> do you see a lot of files added for this feature to justify its own
> subdirectory?

Currently there are about a half dozen new files. However, once the browser
plugin has an API, I'm sure that will blow up to many more. I believe the
current thought is the browser tag is a shadow DOM facade on top of the browser
plugin so it seems to make sense to me to name it based on what it is, rather
than how it appears. Moving it to a browser_plugin directory.

https://chromiumcodereview.appspot.com/9668031/diff/1/content/renderer/plugin...
content/renderer/plugins/browser_plugin_placeholder.h:10: #include
"content/public/renderer/render_process_observer.h"
On 2012/03/12 16:30:09, John Abd-El-Malek wrote:
> why?

Doesn't look like they are necessary. Probably a remnant of an earlier version
of this file. Thanks for catching that.

https://chromiumcodereview.appspot.com/9668031/diff/1/content/renderer/plugin...
content/renderer/plugins/browser_plugin_placeholder.h:11: #include
"content/public/renderer/render_view_observer.h"
On 2012/03/12 16:30:09, John Abd-El-Malek wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/9668031/diff/1/content/renderer/plugin...
content/renderer/plugins/browser_plugin_placeholder.h:18: class RenderThread;
On 2012/03/12 16:30:09, John Abd-El-Malek wrote:
> why?

Done.

https://chromiumcodereview.appspot.com/9668031/diff/1/content/renderer/plugin...
content/renderer/plugins/browser_plugin_placeholder.h:23: class WebFrame;
On 2012/03/12 16:30:09, John Abd-El-Malek wrote:
> nit: not needed since thi sis in webview_plugin.h

Done.

https://chromiumcodereview.appspot.com/9668031/diff/1/content/renderer/plugin...
content/renderer/plugins/browser_plugin_placeholder.h:27: class
BrowserPluginPlaceholder: public webkit::WebViewPlugin::Delegate {
On 2012/03/12 16:30:09, John Abd-El-Malek wrote:
> there should be a comment on this class saying what it's used for

Done.

https://chromiumcodereview.appspot.com/9668031/diff/1/content/renderer/plugin...
content/renderer/plugins/browser_plugin_placeholder.h:35: static
BrowserPluginPlaceholder* FromID(int id);
On 2012/03/12 16:30:09, John Abd-El-Malek wrote:
> it's hard to see if this method is needed or not, or if there's an alternative
> to avoiding the static map, since the code that uses this isn't sent for
review
> yet. why is this needed?

One RenderView can have multiple browser plugins within it. I needed a way to
grab the right one and talk to it. I solved this problem by giving each
BrowserPluginPlaceholder a unique ID before requesting the creation of a new
guest RenderView from the browser process. That id is passed to the browser
process. Once the new RenderView is up and running, it sends an IPC to the host
RenderView back with the ID of the BrowserPluginPlaceholder. Now it knows which
placeholder to swap out with a guest.

https://chromiumcodereview.appspot.com/9668031/diff/1/content/renderer/plugin...
content/renderer/plugins/browser_plugin_placeholder.h:38: void
UnregisterPlaceholder(int id);
On 2012/03/12 16:30:09, John Abd-El-Malek wrote:
> ditto

Please see above.

https://chromiumcodereview.appspot.com/9668031/diff/1/content/renderer/plugin...
content/renderer/plugins/browser_plugin_placeholder.h:75: int height_;
On 2012/03/12 16:30:09, John Abd-El-Malek wrote:
> nit: instead of width_ and height_, use gfx::Size

Done.

https://chromiumcodereview.appspot.com/9668031/diff/1/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/9668031/diff/1/content/renderer/render...
content/renderer/render_view_impl.cc:2074: UTF16ToASCII(params.mimeType) ==
kBrowserPluginMimeType)
On 2012/03/12 16:30:09, John Abd-El-Malek wrote:
> this constant isn't in webkit/plugins/plugin_constants.h...

Yes, it is. Added in this cl:
const char kBrowserPluginMimeType[] = "application/browser-plugin

[jam, 2012-03-13 17:14:24]

http://codereview.chromium.org/9668031/diff/6001/webkit/plugins/plugin_consta...
File webkit/plugins/plugin_constants.h (right):

http://codereview.chromium.org/9668031/diff/6001/webkit/plugins/plugin_consta...
webkit/plugins/plugin_constants.h:10: WEBKIT_PLUGINS_EXPORT extern const char
kBrowserPluginName[];
why are these in webkit/plugins/plugin_constants.h? that file is for constants
that the webkit module needs to know about, which isn't the case here.

[Fady Samuel, 2012-03-13 18:56:11]

Moved browser plugin constants to content/common/browser_plugin_constants.h/cc.
Is this a good location?

[jam, 2012-03-13 19:27:57]

On 2012/03/13 18:56:11, Fady Samuel wrote:
> Moved browser plugin constants to
content/common/browser_plugin_constants.h/cc.
> Is this a good location?

check out
http://dev.chromium.org/developers/how-tos/getting-around-the-chrome-source-code
we put stuff in common if it's used by multiple directories inside content. in
this case, it's only used by content/renderer.

[Fady Samuel, 2012-03-13 19:43:37]

On 2012/03/13 19:27:57, John Abd-El-Malek wrote:
> On 2012/03/13 18:56:11, Fady Samuel wrote:
> > Moved browser plugin constants to
> content/common/browser_plugin_constants.h/cc.
> > Is this a good location?
> 
> check out
>
http://dev.chromium.org/developers/how-tos/getting-around-the-chrome-source-code
> we put stuff in common if it's used by multiple directories inside content. in
> this case, it's only used by content/renderer.

Moved the constants to the content/renderer/browser_plugin directory. Thanks.

[Fady Samuel, 2012-03-13 19:43:53]

Moved constants to content/renderer/browser_plugin directory.

[jam, 2012-03-13 19:46:54]

On 2012/03/13 19:43:53, Fady Samuel wrote:
> Moved constants to content/renderer/browser_plugin directory.

is there a reason to put them in a separate file from
browser_plugin_placeholder? I don't know all the code that you'll land after
this, so it's hard to guess whether this is necessary or not

[Fady Samuel, 2012-03-13 19:49:52]

On 2012/03/13 19:46:54, John Abd-El-Malek wrote:
> On 2012/03/13 19:43:53, Fady Samuel wrote:
> > Moved constants to content/renderer/browser_plugin directory.
> 
> is there a reason to put them in a separate file from
> browser_plugin_placeholder? I don't know all the code that you'll land after
> this, so it's hard to guess whether this is necessary or not

Yes, it is used in pepper_plugin_delegate_impl.cc:
http://codereview.chromium.org/9609008/patch/28001/29021

[jam, 2012-03-13 19:53:16]

lgtm

[commit-bot: I haz the power, 2012-03-13 19:54:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/9668031/6023

[commit-bot: I haz the power, 2012-03-14 01:54:15]

Change committed as 126534

[commit-bot: I haz the power, 2012-03-14 03:26:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/9668031/16001

[commit-bot: I haz the power, 2012-03-14 03:36:34]

Can't apply patch for file
content/browser/renderer_host/render_process_host_impl.cc.
While running patch -p1 --forward --force;
patching file content/browser/renderer_host/render_process_host_impl.cc
Hunk #1 FAILED at 678.
1 out of 1 hunk FAILED -- saving rejects to file
content/browser/renderer_host/render_process_host_impl.cc.rej

[commit-bot: I haz the power, 2012-03-14 03:51:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/9668031/14004

[commit-bot: I haz the power, 2012-03-14 06:42:55]

Change committed as 126598