BrowserPlugin/WebView - Move plugin lifetime to DOM (Chromium-side)

BrowserPlugin/WebView - Move plugin lifetime to DOM (Chromium-side)

This CL provides the Chromium-side support to https://codereview.chromium.org/23618022/. It adds the plumbing required for a BrowserPlugin to request early-creation and persistence, and provides basic tests for this functionality.

BUG=156219

[wjmaclean, 2013-10-07 19:19:23]

I've cleaned this up and added some tests ... can you please take a look?

[Fady Samuel, 2013-10-07 19:54:50]

https://codereview.chromium.org/23591016/diff/5001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/23591016/diff/5001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:151:
this.browserPluginNode_.setAttribute('type', 'application/browser-plugin');
Is this change necessary? <webview> seems to work fine for me without it

https://codereview.chromium.org/23591016/diff/5001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/23591016/diff/5001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:551: //
TODO(wjmaclean) Do we need to do something here to verify the display:none
Yes, you need to change something in the guest process. I'd inject
document.title = 'someothervalue' and then check for 'someothervalue' below.

[wjmaclean, 2013-10-07 19:58:15]

https://codereview.chromium.org/23591016/diff/5001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/web_view.js (right):

https://codereview.chromium.org/23591016/diff/5001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:151:
this.browserPluginNode_.setAttribute('type', 'application/browser-plugin');
On 2013/10/07 19:54:50, Fady Samuel wrote:
> Is this change necessary? <webview> seems to work fine for me without it

I made this change a while back, and (at that time at least) it was necessary. I
will re-try without the change to see if it's still necessary.

https://codereview.chromium.org/23591016/diff/5001/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/23591016/diff/5001/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:551: //
TODO(wjmaclean) Do we need to do something here to verify the display:none
On 2013/10/07 19:54:50, Fady Samuel wrote:
> Yes, you need to change something in the guest process. I'd inject
> document.title = 'someothervalue' and then check for 'someothervalue' below.

Sure, that sounds reasonable, will add this.

[wjmaclean, 2013-10-07 20:37:06]

So it doesn't seem the change in where the src attribute was being set is still
required, so I removed this change.

Added the alternate document title in the test.

[Fady Samuel, 2013-10-07 21:19:40]

I'm happy with this patch once Eric is happy with the Blink side change! I'm
really excited about this. This will allow for a lot more flexibility.

[eseidel, 2013-10-17 14:45:52]

Sorry I was slow on this.  I need a bit more context here.  could you update the
descriptions and ideally link to a bug?  Although I like some of the ideas in
these patches, I'm not sure they're a good feature to add to the platform as-is.

[wjmaclean, 2013-10-23 14:44:36]

On 2013/10/17 14:45:52, eseidel wrote:
> Sorry I was slow on this.  I need a bit more context here.  could you update
the
> descriptions and ideally link to a bug?

Will do.

> Although I like some of the ideas in
> these patches, I'm not sure they're a good feature to add to the platform
as-is.

Hmm, could you be a bit more specific? By "as is" I assume you mean you're not
ok with the way I've tried to implement some (or all) of the concepts, is that
right? If so, can you say why, so I know what needs revising, or if revising is
even possible?

[eseidel, 2013-11-18 22:11:07]

https://codereview.chromium.org/23591016/diff/40001/content/test/data/browser...
File content/test/data/browser_plugin_embedder_late_attach.html (right):

https://codereview.chromium.org/23591016/diff/40001/content/test/data/browser...
content/test/data/browser_plugin_embedder_late_attach.html:12: thePlugin.type =
"application/browser-plugin";
That's quite the generic mimetype ya'll have chosen. :)

[Fady Samuel, 2014-04-04 20:42:28]

On 2013/11/18 22:11:07, eseidel wrote:
>
https://codereview.chromium.org/23591016/diff/40001/content/test/data/browser...
> File content/test/data/browser_plugin_embedder_late_attach.html (right):
> 
>
https://codereview.chromium.org/23591016/diff/40001/content/test/data/browser...
> content/test/data/browser_plugin_embedder_late_attach.html:12: thePlugin.type
=
> "application/browser-plugin";
> That's quite the generic mimetype ya'll have chosen. :)

BrowserPlugin is a plugin that can embed anything Chrome can host, in theory.
:-)

[Fady Samuel, 2014-04-04 20:43:10]

browser_plugin lgtm

[wjmaclean, 2014-04-08 17:03:02]

+creis@ for content/renderer ? (jamesr@ is not available ...)

[Charlie Reis, 2014-04-08 18:51:47]

Rubber stamp LGTM for content/renderer/, since this just looks like a use for
all the stuff that has already landed.  One small nit below.

https://codereview.chromium.org/23591016/diff/210001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/23591016/diff/210001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1252: bool
RenderFrameImpl::canCreatePluginWithoutRenderer(
nit: Please put this after cookieJar so that it's in the same order as the .h
file.

[wjmaclean, 2014-04-09 16:50:02]

https://codereview.chromium.org/23591016/diff/210001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/23591016/diff/210001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1252: bool
RenderFrameImpl::canCreatePluginWithoutRenderer(
On 2014/04/08 18:51:48, Charlie Reis wrote:
> nit: Please put this after cookieJar so that it's in the same order as the .h
> file.

Done.

[Charlie Reis, 2014-04-09 19:09:08]

https://codereview.chromium.org/23591016/diff/210001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/23591016/diff/210001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1252: bool
RenderFrameImpl::canCreatePluginWithoutRenderer(
On 2014/04/09 16:50:03, wjmaclean wrote:
> On 2014/04/08 18:51:48, Charlie Reis wrote:
> > nit: Please put this after cookieJar so that it's in the same order as the
.h
> > file.
> 
> Done.

I don't see an updated patch.  Please upload the new version before committing,
but otherwise feel free to proceed.

[wjmaclean, 2014-04-09 19:20:27]

On 2014/04/09 19:09:08, Charlie Reis wrote:
>
https://codereview.chromium.org/23591016/diff/210001/content/renderer/render_...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/23591016/diff/210001/content/renderer/render_...
> content/renderer/render_frame_impl.cc:1252: bool
> RenderFrameImpl::canCreatePluginWithoutRenderer(
> On 2014/04/09 16:50:03, wjmaclean wrote:
> > On 2014/04/08 18:51:48, Charlie Reis wrote:
> > > nit: Please put this after cookieJar so that it's in the same order as the
> .h
> > > file.
> > 
> > Done.
> 
> I don't see an updated patch.  Please upload the new version before
committing,
> but otherwise feel free to proceed.

I haven't uploaded the new patch yet, but will do so before landing. Fady and I
are discussing some lifetime ideas and I'm holding off until we finish that
discussion.

[eseidel, 2014-05-28 22:30:18]

Looks like this stalled out.  dcheng may be your best reviewer for this kind of
blink/chromium integration code these days.

[wjmaclean, 2014-06-02 12:53:28]

On 2014/05/28 22:30:18, eseidel wrote:
> Looks like this stalled out.  dcheng may be your best reviewer for this kind
of
> blink/chromium integration code these days.

It's not currently block on lack of reviews. Rather, it's blocked on
crbug.com/363612, and I've been asked not to work on that for the time being;
I've been assigned other tasks.