Browser Plugin: Simplified BrowserPluginManager

Browser Plugin: Simplified BrowserPluginManager

BrowserPluginManager is no longer a singleton.

BrowserPluginManager is now a RenderViewObserver.
All BrowserPluginMsg_* are routed which means they're targeted at a particular RenderView.

This means that more than one BrowserPluginManager can exist per process.

BrowserPluginManager is now refcounted and so it exists as long as a BrowserPlugin or RenderViewImpl
holds a reference to it.

In tests, we inject a BrowserPluginManagerFactory that creates MockBrowserPluginManagers.

BUG=159936
Test=BrowserPluginTest.*, BrowserPluginHostTest.*


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

[Fady Samuel, 2012-11-19 21:08:23]

Hi Charlie,

I believe I've addressed all the concerns raised a couple of weeks ago regarding
BrowserPluginManager.

There's a lot of code cleanup here. See the issue description for more info on
what's changed.

Thanks,

Fady

[Charlie Reis, 2012-11-20 00:33:34]

Nice.  LGTM, but please run try jobs first.

https://codereview.chromium.org/11416074/diff/5001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin_manager.h (right):

https://codereview.chromium.org/11416074/diff/5001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin_manager.h:62: // Don't destroy
the BrowserPluginManager when the RenderViewImpl goes away.
nit: Blank line before.

https://codereview.chromium.org/11416074/diff/5001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/11416074/diff/5001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:5817: if (has_browser_plugin_manager())
Great-- I'm glad we can avoid creating it.

Would a null check on browser_plugin_manager_ work here, or is there a reason to
have a method for it?

https://codereview.chromium.org/11416074/diff/5001/content/renderer/render_vi...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/11416074/diff/5001/content/renderer/render_vi...
content/renderer/render_view_impl.h:1048: bool has_browser_plugin_manager()
const { return browser_plugin_manager_; }
return !!browser_plugin_manager_;

[Fady Samuel, 2012-11-20 02:04:14]

Fixed nits. Running through some bots then CQ'ing.

http://codereview.chromium.org/11416074/diff/5001/content/renderer/browser_pl...
File content/renderer/browser_plugin/browser_plugin_manager.h (right):

http://codereview.chromium.org/11416074/diff/5001/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin_manager.h:62: // Don't destroy
the BrowserPluginManager when the RenderViewImpl goes away.
On 2012/11/20 00:33:34, creis wrote:
> nit: Blank line before.

Done.

http://codereview.chromium.org/11416074/diff/5001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/11416074/diff/5001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:5817: if (has_browser_plugin_manager())
On 2012/11/20 00:33:34, creis wrote:
> Great-- I'm glad we can avoid creating it.
> 
> Would a null check on browser_plugin_manager_ work here, or is there a reason
to
> have a method for it?

Done.

http://codereview.chromium.org/11416074/diff/5001/content/renderer/render_vie...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/11416074/diff/5001/content/renderer/render_vie...
content/renderer/render_view_impl.h:1048: bool has_browser_plugin_manager()
const { return browser_plugin_manager_; }
On 2012/11/20 00:33:34, creis wrote:
> return !!browser_plugin_manager_;

Deleted.

[commit-bot: I haz the power, 2012-11-20 16:27:40]

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

[commit-bot: I haz the power, 2012-11-20 16:27:49]

Presubmit check for 11416074-4003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/common/browser_plugin_messages.h

Presubmit checks took 1.8s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Fady Samuel, 2012-11-20 16:28:55]

+cdn@ for browser_plugin_messages.h changes.

Thanks,

Fady

[Fady Samuel, 2012-11-20 18:15:09]

Hi Justin,

I think Cris might be OOO? Could you please take a look?

Thanks,

Fady

[Cris Neckar, 2012-11-20 19:07:17]

lgtm for IPC changes

[commit-bot: I haz the power, 2012-11-20 19:15:52]

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

[commit-bot: I haz the power, 2012-11-20 20:25:02]

Retried try job too often for step(s) content_browsertests

[commit-bot: I haz the power, 2012-11-20 20:27:03]

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

[commit-bot: I haz the power, 2012-11-20 23:58:08]

Change committed as 168920