Clean up plug-in placeholders:

Clean up plug-in placeholders:

* Add separate class for missing plug-in placeholder, and factor out a common base class
* Move plug-in related classes to plugins/ subdirectory
* Move custom menu command IDs to a separate header file.

TBR=darin@chromium.org
BUG=62079
TEST=none

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

[Bernhard Bauer, 2011-11-21 19:49:07]

Hey Chris, 

would you mind doing this code review?

Thanks :)

[Bernhard Bauer, 2011-11-22 09:19:23]

Dominic, can you take a look?

[battre, 2011-11-22 09:57:15]

http://codereview.chromium.org/8461011/diff/2001/chrome/renderer/plugins/bloc...
File chrome/renderer/plugins/blocked_plugin.cc (right):

http://codereview.chromium.org/8461011/diff/2001/chrome/renderer/plugins/bloc...
chrome/renderer/plugins/blocked_plugin.cc:50: 
nit: -empty line

http://codereview.chromium.org/8461011/diff/2001/chrome/renderer/plugins/bloc...
chrome/renderer/plugins/blocked_plugin.cc:81: return blocked_plugin->plugin();
This looks like a memory leak.

http://codereview.chromium.org/8461011/diff/2001/chrome/renderer/plugins/bloc...
File chrome/renderer/plugins/blocked_plugin.h (right):

http://codereview.chromium.org/8461011/diff/2001/chrome/renderer/plugins/bloc...
chrome/renderer/plugins/blocked_plugin.h:20: static
webkit::npapi::WebViewPlugin* Create(
Document owernship of passed pointers?

http://codereview.chromium.org/8461011/diff/2001/chrome/renderer/plugins/bloc...
chrome/renderer/plugins/blocked_plugin.h:79: bool allow_loading_;
DISALLOW_COPY_AND_ASSIGN?

http://codereview.chromium.org/8461011/diff/2001/chrome/renderer/plugins/miss...
File chrome/renderer/plugins/missing_plugin.h (right):

http://codereview.chromium.org/8461011/diff/2001/chrome/renderer/plugins/miss...
chrome/renderer/plugins/missing_plugin.h:14: static
webkit::npapi::WebViewPlugin* Create(
Document ownership of passed pointers?

http://codereview.chromium.org/8461011/diff/2001/chrome/renderer/plugins/plug...
File chrome/renderer/plugins/plugin_placeholder.h (right):

http://codereview.chromium.org/8461011/diff/2001/chrome/renderer/plugins/plug...
chrome/renderer/plugins/plugin_placeholder.h:18: class PluginPlaceholder :
public content::RenderViewObserver,
Add a short description of this class? Why would you want to use a placeholder?
Is this a generic wrapper or do you use it only in some cases?

http://codereview.chromium.org/8461011/diff/2001/chrome/renderer/plugins/plug...
chrome/renderer/plugins/plugin_placeholder.h:22:
PluginPlaceholder(content::RenderView* render_view,
Document owernship of passed pointers?

http://codereview.chromium.org/8461011/diff/2001/chrome/renderer/plugins/plug...
chrome/renderer/plugins/plugin_placeholder.h:31: void LoadPlugin(const
webkit::WebPluginInfo& plugin_info);
Please rename these functions if they occur with the same name in the derived
classes. Maybe LoadPluginImpl?

[Bernhard Bauer, 2011-11-22 17:04:49]

PTAL. I also moved webview_plugin.* from webkit/plugins/npapi to webkit/plugins,
as it's not NPAPI-specific.

http://codereview.chromium.org/8461011/diff/2001/chrome/renderer/plugins/bloc...
File chrome/renderer/plugins/blocked_plugin.cc (right):

http://codereview.chromium.org/8461011/diff/2001/chrome/renderer/plugins/bloc...
chrome/renderer/plugins/blocked_plugin.cc:50: 
On 2011/11/22 09:57:15, battre wrote:
> nit: -empty line

gone.

http://codereview.chromium.org/8461011/diff/2001/chrome/renderer/plugins/bloc...
chrome/renderer/plugins/blocked_plugin.cc:81: return blocked_plugin->plugin();
On 2011/11/22 09:57:15, battre wrote:
> This looks like a memory leak.

BlockedPlugin is a WebViewPluginDelegate. WebViewPlugin doesn't own it's
delegate, so it notifies it instead that it is going away (via
WillDestroyPlugin), then the delegate destroys itself.  I'll add a comment
explaining that.

http://codereview.chromium.org/8461011/diff/2001/chrome/renderer/plugins/plug...
File chrome/renderer/plugins/plugin_placeholder.h (right):

http://codereview.chromium.org/8461011/diff/2001/chrome/renderer/plugins/plug...
chrome/renderer/plugins/plugin_placeholder.h:18: class PluginPlaceholder :
public content::RenderViewObserver,
On 2011/11/22 09:57:15, battre wrote:
> Add a short description of this class? Why would you want to use a
placeholder?
> Is this a generic wrapper or do you use it only in some cases?

Done.

http://codereview.chromium.org/8461011/diff/2001/chrome/renderer/plugins/plug...
chrome/renderer/plugins/plugin_placeholder.h:22:
PluginPlaceholder(content::RenderView* render_view,
On 2011/11/22 09:57:15, battre wrote:
> Document owernship of passed pointers?

Done.

http://codereview.chromium.org/8461011/diff/2001/chrome/renderer/plugins/plug...
chrome/renderer/plugins/plugin_placeholder.h:31: void LoadPlugin(const
webkit::WebPluginInfo& plugin_info);
On 2011/11/22 09:57:15, battre wrote:
> Please rename these functions if they occur with the same name in the derived
> classes. Maybe LoadPluginImpl?

...Internal?

[Bernhard Bauer, 2011-11-22 17:07:42]

Also, +John for OWNERS review, and in case you're interested :)

[jam, 2011-11-22 18:03:32]

content/webkit lgtm

http://codereview.chromium.org/8461011/diff/11026/webkit/plugins/webview_plug...
File webkit/plugins/webview_plugin.h (right):

http://codereview.chromium.org/8461011/diff/11026/webkit/plugins/webview_plug...
webkit/plugins/webview_plugin.h:5: #ifndef
WEBKIT_PLUGINS_NPAPI_WEBVIEW_PLUGIN_H_
nit: update ifdef guards

http://codereview.chromium.org/8461011/diff/11026/webkit/plugins/webview_plug...
webkit/plugins/webview_plugin.h:48: virtual void ShowContextMenu(const
WebKit::WebMouseEvent&) {}
nit: it's much better to have interfaces that are all pure. why did you need to
change this?

[Bernhard Bauer, 2011-11-22 18:53:33]

http://codereview.chromium.org/8461011/diff/11026/webkit/plugins/webview_plug...
File webkit/plugins/webview_plugin.h (right):

http://codereview.chromium.org/8461011/diff/11026/webkit/plugins/webview_plug...
webkit/plugins/webview_plugin.h:5: #ifndef
WEBKIT_PLUGINS_NPAPI_WEBVIEW_PLUGIN_H_
On 2011/11/22 18:03:32, John Abd-El-Malek wrote:
> nit: update ifdef guards

Done.

http://codereview.chromium.org/8461011/diff/11026/webkit/plugins/webview_plug...
webkit/plugins/webview_plugin.h:48: virtual void ShowContextMenu(const
WebKit::WebMouseEvent&) {}
On 2011/11/22 18:03:32, John Abd-El-Malek wrote:
> nit: it's much better to have interfaces that are all pure. why did you need
to
> change this?

Hm, I guess that's from an earlier version where I didn't have the shared base
class. 

Out of curiosity, why is it much better to have pure interfaces? I like the
convenience of having empty implementations :)

[battre, 2011-11-23 16:10:19]

LGTM

[commit-bot: I haz the power, 2011-11-23 16:20:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/8461011/13002

[commit-bot: I haz the power, 2011-11-23 16:20:41]

Presubmit check for 8461011-13002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: webkit/glue/webkit_glue.gypi

Presubmit checks took 2.8s to calculate.

[Bernhard Bauer, 2011-11-23 16:44:17]

Darin: OWNERS review for webkit/glue/webkit_glue.gypi please?

[commit-bot: I haz the power, 2011-11-25 10:30:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/8461011/13002

[commit-bot: I haz the power, 2011-11-25 11:55:14]

Try job failure for 8461011-13002 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-11-25 12:00:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/8461011/13002