Move renderer plugin code into a new component.

Move renderer plugin code into a new component.

Android Webview needs to use some of the renderer plugin code, however
this code is currently in chrome. This commit moves the parts of this
code that don't depend on the rest of chrome into a new component.

BUG=283713
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228211

[aberent, 2013-09-09 16:46:44]

Now ready for review. Webview changes to use this will follow as a separate CL.

[Bernhard Bauer, 2013-09-09 17:28:06]

https://codereview.chromium.org/23606022/diff/8001/chrome/renderer/plugins/ch...
File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right):

https://codereview.chromium.org/23606022/diff/8001/chrome/renderer/plugins/ch...
chrome/renderer/plugins/chrome_plugin_placeholder.cc:286: ->Send(new
ChromeViewHostMsg_GetPluginInfo(routing_id(),
Can you format this differently, so you don't have break right before the arrow?
I find that clang-format works pretty well.

https://codereview.chromium.org/23606022/diff/8001/chrome/renderer/plugins/ch...
File chrome/renderer/plugins/chrome_plugin_placeholder.h (right):

https://codereview.chromium.org/23606022/diff/8001/chrome/renderer/plugins/ch...
chrome/renderer/plugins/chrome_plugin_placeholder.h:16:
ChromePluginPlaceholder(content::RenderView* render_view,
Do we ever directly construct this object outside of the class, or could we make
the constructor private?

https://codereview.chromium.org/23606022/diff/8001/chrome/renderer/plugins/ch...
chrome/renderer/plugins/chrome_plugin_placeholder.h:76: // Local methods:
Super-nit: For blocks of virtual methods it makes sense to state what interface
they're implementing, but for non-virtual methods the fact that these are local
can be inferred from the fact that they're non-virtual anyway (we do turn on
warnings about missing virtual, so there are no false positives), so I don't
think the comment adds a lot of value here.

https://codereview.chromium.org/23606022/diff/8001/components/plugins/rendere...
File components/plugins/renderer/plugin_placeholder.h (right):

https://codereview.chromium.org/23606022/diff/8001/components/plugins/rendere...
components/plugins/renderer/plugin_placeholder.h:51: static std::string
GetYoutubeVideoId(const WebKit::WebPluginParams& params);
As discussed off-review, please look into whether we could extract this to a
separate subclass. Thanks!

https://codereview.chromium.org/23606022/diff/8001/components/plugins/rendere...
components/plugins/renderer/plugin_placeholder.h:93: // Arguments are required
by the caller, but not used.
Nit: The other callback methods above state that they don't take arguments and
return nothing. Can you add that back here too? You could move this comment to
the start of the section, as it applies to all of these callbacks.

https://codereview.chromium.org/23606022/diff/8001/components/plugins/rendere...
components/plugins/renderer/plugin_placeholder.h:94: void
OpenAboutPluginsCallback(const webkit_glue::CppArgumentList& args,
Could you only register the callback in the subclass? Then you could get rid of
the virtual method below.

https://codereview.chromium.org/23606022/diff/8001/components/plugins/rendere...
File components/plugins/renderer/plugin_uma.h (right):

https://codereview.chromium.org/23606022/diff/8001/components/plugins/rendere...
components/plugins/renderer/plugin_uma.h:18: class PluginUMAReporter {
I think this is only used by ChromePluginPlaceholder, so it does not need to be
part of the component.

[Jói, 2013-09-09 17:46:17]

https://codereview.chromium.org/23606022/diff/8001/chrome/renderer/plugins/ch...
File chrome/renderer/plugins/chrome_plugin_placeholder.h (right):

https://codereview.chromium.org/23606022/diff/8001/chrome/renderer/plugins/ch...
chrome/renderer/plugins/chrome_plugin_placeholder.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
This file and the next seem to be modifications/extractions from
plugin_placeholder.h and .cc. Could you try uploading with similarity set lower
to see if we get a diff from the previous version? E.g. git cl upload
--similarity=50

https://codereview.chromium.org/23606022/diff/8001/components/plugins/rendere...
File components/plugins/renderer/plugin_placeholder.cc (right):

https://codereview.chromium.org/23606022/diff/8001/components/plugins/rendere...
components/plugins/renderer/plugin_placeholder.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
Likewise for this file, let's see if --similarity=50 gets us a diff from the
original.

https://codereview.chromium.org/23606022/diff/8001/components/plugins/rendere...
File components/plugins/renderer/plugin_uma.h (right):

https://codereview.chromium.org/23606022/diff/8001/components/plugins/rendere...
components/plugins/renderer/plugin_uma.h:5: #ifndef
CHROME_RENDERER_PLUGINS_PLUGIN_UMA_H_
Need to update the header guard.

https://codereview.chromium.org/23606022/diff/8001/components/plugins/rendere...
File components/plugins/renderer/webview_plugin.h (right):

https://codereview.chromium.org/23606022/diff/8001/components/plugins/rendere...
components/plugins/renderer/webview_plugin.h:5: #ifndef
CHROME_RENDERER_PLUGINS_WEBVIEW_PLUGIN_H_
Need to update the header guard.

For future reference, note that tools/git/move_source_file.py or
tools/git/mass-rename.sh would have done this for you.

[aberent, 2013-09-10 13:24:01]

https://codereview.chromium.org/23606022/diff/8001/chrome/renderer/plugins/ch...
File chrome/renderer/plugins/chrome_plugin_placeholder.cc (right):

https://codereview.chromium.org/23606022/diff/8001/chrome/renderer/plugins/ch...
chrome/renderer/plugins/chrome_plugin_placeholder.cc:286: ->Send(new
ChromeViewHostMsg_GetPluginInfo(routing_id(),
On 2013/09/09 17:28:07, Bernhard Bauer wrote:
> Can you format this differently, so you don't have break right before the
arrow?
> I find that clang-format works pretty well.

I don't particularly like it either, but this is actually the output of git cl
format, which runs clang-format. Running it though clang-format directly gave
this again. I could format it differently manually, but clang-format will put it
back to this every time it is used.

https://codereview.chromium.org/23606022/diff/8001/chrome/renderer/plugins/ch...
File chrome/renderer/plugins/chrome_plugin_placeholder.h (right):

https://codereview.chromium.org/23606022/diff/8001/chrome/renderer/plugins/ch...
chrome/renderer/plugins/chrome_plugin_placeholder.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2013/09/09 17:46:17, Jói wrote:
> This file and the next seem to be modifications/extractions from
> plugin_placeholder.h and .cc. Could you try uploading with similarity set
lower
> to see if we get a diff from the previous version? E.g. git cl upload
> --similarity=50

I think 50 is the default. I will try lowering it to 30 on the next upload.
Unfortunately I don't know a way of testing this without uploading.

https://codereview.chromium.org/23606022/diff/8001/chrome/renderer/plugins/ch...
chrome/renderer/plugins/chrome_plugin_placeholder.h:16:
ChromePluginPlaceholder(content::RenderView* render_view,
On 2013/09/09 17:28:07, Bernhard Bauer wrote:
> Do we ever directly construct this object outside of the class, or could we
make
> the constructor private?

Done.

https://codereview.chromium.org/23606022/diff/8001/chrome/renderer/plugins/ch...
chrome/renderer/plugins/chrome_plugin_placeholder.h:76: // Local methods:
On 2013/09/09 17:28:07, Bernhard Bauer wrote:
> Super-nit: For blocks of virtual methods it makes sense to state what
interface
> they're implementing, but for non-virtual methods the fact that these are
local
> can be inferred from the fact that they're non-virtual anyway (we do turn on
> warnings about missing virtual, so there are no false positives), so I don't
> think the comment adds a lot of value here.

Done.

https://codereview.chromium.org/23606022/diff/8001/components/plugins/rendere...
File components/plugins/renderer/plugin_placeholder.cc (right):

https://codereview.chromium.org/23606022/diff/8001/components/plugins/rendere...
components/plugins/renderer/plugin_placeholder.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/09/09 17:46:17, Jói wrote:
> Likewise for this file, let's see if --similarity=50 gets us a diff from the
> original.

I will experiment

https://codereview.chromium.org/23606022/diff/8001/components/plugins/rendere...
File components/plugins/renderer/plugin_placeholder.h (right):

https://codereview.chromium.org/23606022/diff/8001/components/plugins/rendere...
components/plugins/renderer/plugin_placeholder.h:51: static std::string
GetYoutubeVideoId(const WebKit::WebPluginParams& params);
On 2013/09/09 17:28:07, Bernhard Bauer wrote:
> As discussed off-review, please look into whether we could extract this to a
> separate subclass. Thanks!

Done.

https://codereview.chromium.org/23606022/diff/8001/components/plugins/rendere...
components/plugins/renderer/plugin_placeholder.h:93: // Arguments are required
by the caller, but not used.
On 2013/09/09 17:28:07, Bernhard Bauer wrote:
> Nit: The other callback methods above state that they don't take arguments and
> return nothing. Can you add that back here too? You could move this comment to
> the start of the section, as it applies to all of these callbacks.

Done.

https://codereview.chromium.org/23606022/diff/8001/components/plugins/rendere...
components/plugins/renderer/plugin_placeholder.h:94: void
OpenAboutPluginsCallback(const webkit_glue::CppArgumentList& args,
On 2013/09/09 17:28:07, Bernhard Bauer wrote:
> Could you only register the callback in the subclass? Then you could get rid
of
> the virtual method below.

Done.

https://codereview.chromium.org/23606022/diff/8001/components/plugins/rendere...
File components/plugins/renderer/plugin_uma.h (right):

https://codereview.chromium.org/23606022/diff/8001/components/plugins/rendere...
components/plugins/renderer/plugin_uma.h:5: #ifndef
CHROME_RENDERER_PLUGINS_PLUGIN_UMA_H_
On 2013/09/09 17:46:17, Jói wrote:
> Need to update the header guard.

Done.

https://codereview.chromium.org/23606022/diff/8001/components/plugins/rendere...
components/plugins/renderer/plugin_uma.h:18: class PluginUMAReporter {
On 2013/09/09 17:28:07, Bernhard Bauer wrote:
> I think this is only used by ChromePluginPlaceholder, so it does not need to
be
> part of the component.

Also called from PluginPlaceholder::ReplacePlugin()

[Bernhard Bauer, 2013-09-10 13:47:10]

https://codereview.chromium.org/23606022/diff/8001/chrome/renderer/plugins/ch...
File chrome/renderer/plugins/chrome_plugin_placeholder.h (right):

https://codereview.chromium.org/23606022/diff/8001/chrome/renderer/plugins/ch...
chrome/renderer/plugins/chrome_plugin_placeholder.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2013/09/10 13:24:01, aberent wrote:
> On 2013/09/09 17:46:17, Jói wrote:
> > This file and the next seem to be modifications/extractions from
> > plugin_placeholder.h and .cc. Could you try uploading with similarity set
> lower
> > to see if we get a diff from the previous version? E.g. git cl upload
> > --similarity=50
> 
> I think 50 is the default. I will try lowering it to 30 on the next upload.
> Unfortunately I don't know a way of testing this without uploading.

FTR, you can delete individual patch sets, so it's fine to upload something as a
test and delete it again if it looks bad (or simply for clarity).

https://codereview.chromium.org/23606022/diff/8001/components/plugins/rendere...
File components/plugins/renderer/plugin_uma.h (right):

https://codereview.chromium.org/23606022/diff/8001/components/plugins/rendere...
components/plugins/renderer/plugin_uma.h:18: class PluginUMAReporter {
On 2013/09/10 13:24:01, aberent wrote:
> On 2013/09/09 17:28:07, Bernhard Bauer wrote:
> > I think this is only used by ChromePluginPlaceholder, so it does not need to
> be
> > part of the component.
> 
> Also called from PluginPlaceholder::ReplacePlugin()

Hm... I think we should move that call to
ChromePluginPlaceholder::OnPluginListChanged(). The other callsite to
ReplacePlugin() (PluginPlaceholder::LoadPlugin()) gets the "real" plug-in from
the RenderView, and if that fails, it means an error during plug-in loading, not
that the plug-in wasn't found.

https://codereview.chromium.org/23606022/diff/19001/components/plugins/render...
File components/plugins/renderer/mobile_youtube_plugin.h (right):

https://codereview.chromium.org/23606022/diff/19001/components/plugins/render...
components/plugins/renderer/mobile_youtube_plugin.h:26: 
Nit: no empty line after public/protected/private sections, only before.

[aberent, 2013-09-13 19:46:22]

All comments to date now resolved (I believe). Now looking for lgtm so that I
can land this and Android Webview can start using it. Added darin@ and xwhang@
since I need lgtms for the directories added to the DEPS files.

https://codereview.chromium.org/23606022/diff/8001/components/plugins/rendere...
File components/plugins/renderer/plugin_uma.h (right):

https://codereview.chromium.org/23606022/diff/8001/components/plugins/rendere...
components/plugins/renderer/plugin_uma.h:18: class PluginUMAReporter {
On 2013/09/10 13:47:10, Bernhard Bauer wrote:
> On 2013/09/10 13:24:01, aberent wrote:
> > On 2013/09/09 17:28:07, Bernhard Bauer wrote:
> > > I think this is only used by ChromePluginPlaceholder, so it does not need
to
> > be
> > > part of the component.
> > 
> > Also called from PluginPlaceholder::ReplacePlugin()
> 
> Hm... I think we should move that call to
> ChromePluginPlaceholder::OnPluginListChanged(). The other callsite to
> ReplacePlugin() (PluginPlaceholder::LoadPlugin()) gets the "real" plug-in from
> the RenderView, and if that fails, it means an error during plug-in loading,
not
> that the plug-in wasn't found.

Done.

https://codereview.chromium.org/23606022/diff/19001/components/plugins/render...
File components/plugins/renderer/mobile_youtube_plugin.h (right):

https://codereview.chromium.org/23606022/diff/19001/components/plugins/render...
components/plugins/renderer/mobile_youtube_plugin.h:26: 
On 2013/09/10 13:47:10, Bernhard Bauer wrote:
> Nit: no empty line after public/protected/private sections, only before.

Done.

[Bernhard Bauer, 2013-09-13 20:04:10]

LGTM, with a question below (but it's okay with me to resolve it in a followup
CL if turns out to be necessary).

https://codereview.chromium.org/23606022/diff/19001/components/plugins/render...
File components/plugins/renderer/plugin_placeholder.cc (right):

https://codereview.chromium.org/23606022/diff/19001/components/plugins/render...
components/plugins/renderer/plugin_placeholder.cc:41: const char* const
kPluginPlaceholderDataURL = "chrome://pluginplaceholderdata/";
Why are you moving this?

https://codereview.chromium.org/23606022/diff/35001/components/plugins/render...
File components/plugins/renderer/mobile_youtube_plugin.h (right):

https://codereview.chromium.org/23606022/diff/35001/components/plugins/render...
components/plugins/renderer/mobile_youtube_plugin.h:23: GURL
placeholderDataUrl);
Nit: It's more common to pass GURLs as const-ref than by copy.

[aberent, 2013-09-13 20:11:30]

https://codereview.chromium.org/23606022/diff/19001/components/plugins/render...
File components/plugins/renderer/plugin_placeholder.cc (right):

https://codereview.chromium.org/23606022/diff/19001/components/plugins/render...
components/plugins/renderer/plugin_placeholder.cc:41: const char* const
kPluginPlaceholderDataURL = "chrome://pluginplaceholderdata/";
On 2013/09/13 20:04:10, Bernhard Bauer wrote:
> Why are you moving this?

Because it is not an appropriate URL for Webview. Webview doesn't know anything
about chrome: URLs.

[Bernhard Bauer, 2013-09-13 20:37:40]

https://codereview.chromium.org/23606022/diff/19001/components/plugins/render...
File components/plugins/renderer/plugin_placeholder.cc (right):

https://codereview.chromium.org/23606022/diff/19001/components/plugins/render...
components/plugins/renderer/plugin_placeholder.cc:41: const char* const
kPluginPlaceholderDataURL = "chrome://pluginplaceholderdata/";
On 2013/09/13 20:11:30, aberent wrote:
> On 2013/09/13 20:04:10, Bernhard Bauer wrote:
> > Why are you moving this?
> 
> Because it is not an appropriate URL for Webview. Webview doesn't know
anything
> about chrome: URLs.

But content does, and WebView uses content. WebView doesn't know about
*specific* chrome: URLs (e.g. chrome://plugins or chrome://settings), but I
think the concept of chrome: URLs is in content.

[Jói, 2013-09-19 15:15:40]

LGTM

[xhwang1, 2013-09-19 19:45:07]

https://codereview.chromium.org/23606022/diff/35001/components/plugins/render...
File components/plugins/renderer/DEPS (right):

https://codereview.chromium.org/23606022/diff/35001/components/plugins/render...
components/plugins/renderer/DEPS:9: "+third_party/widevine",
could you please point to me where this is needed? thanks

[aberent, 2013-09-20 17:12:52]

https://codereview.chromium.org/23606022/diff/35001/components/plugins/render...
File components/plugins/renderer/DEPS (right):

https://codereview.chromium.org/23606022/diff/35001/components/plugins/render...
components/plugins/renderer/DEPS:9: "+third_party/widevine",
On 2013/09/19 19:45:07, Xiaohan Wang wrote:
> could you please point to me where this is needed? thanks
It isn't. New patch coming that removes it. Originally I also moved plugin_uma.*
into the component, which did need it, but I was persuaded that it didn't belong
in the component.

[darin (slow to review), 2013-10-01 17:22:58]

https://codereview.chromium.org/23606022/diff/1/components/plugins.gypi
File components/plugins.gypi (right):

https://codereview.chromium.org/23606022/diff/1/components/plugins.gypi#newcode8
components/plugins.gypi:8: 'target_name': 'plugins_renderer',
I find the name "plugins_renderer" a bit confusing. I think you are trying to
indicate that these are plugins that are intended to only run in the renderer
process. We typically refer to those as "in process plugins", so perhaps you
could rename this component in_process_plugins and use that as the namespace?

I'd imagine that the directory would also be changed from
components/plugins/renderer/ to components/in_process_plugins/.

Sound OK?

https://codereview.chromium.org/23606022/diff/1/components/plugins.gypi#newco...
components/plugins.gypi:21: # 'plugins/renderer/plugin_uma_unittest.cc',
nit: delete this commented out line?

[aberent, 2013-10-01 18:14:55]

https://codereview.chromium.org/23606022/diff/1/components/plugins.gypi
File components/plugins.gypi (right):

https://codereview.chromium.org/23606022/diff/1/components/plugins.gypi#newcode8
components/plugins.gypi:8: 'target_name': 'plugins_renderer',
I originally called it just "plugins" but didn't want to leave it like that
because there is a substantial plugins module in chrome/browser that I can see
we might in future want to move to a component. 

In calling it "plugins_renderer" I believe I am following the pattern
established by the visitedlink component which has "visitedlink_browser",
"visitedlink_common" and "visitedlink_renderer", and the autofill component,
which has (among other targets) "autofill_content_browser" and
"autofill_content_renderer" 

On 2013/10/01 17:22:59, darin wrote:
> I find the name "plugins_renderer" a bit confusing. I think you are trying to
> indicate that these are plugins that are intended to only run in the renderer
> process. We typically refer to those as "in process plugins", so perhaps you
> could rename this component in_process_plugins and use that as the namespace?
> 
> I'd imagine that the directory would also be changed from
> components/plugins/renderer/ to components/in_process_plugins/.
> 
> Sound OK?

https://codereview.chromium.org/23606022/diff/1/components/plugins.gypi#newco...
components/plugins.gypi:21: # 'plugins/renderer/plugin_uma_unittest.cc',
On 2013/10/01 17:22:59, darin wrote:
> nit: delete this commented out line?

Done. You appear to be looking at patch set 1. This has been fixed in more
recent patch sets (the current version is patch set 7).

[darin (slow to review), 2013-10-05 06:20:49]

On 2013/10/01 18:14:55, aberent wrote:
> https://codereview.chromium.org/23606022/diff/1/components/plugins.gypi
> File components/plugins.gypi (right):
> 
>
https://codereview.chromium.org/23606022/diff/1/components/plugins.gypi#newcode8
> components/plugins.gypi:8: 'target_name': 'plugins_renderer',
> I originally called it just "plugins" but didn't want to leave it like that
> because there is a substantial plugins module in chrome/browser that I can see
> we might in future want to move to a component. 
> 
> In calling it "plugins_renderer" I believe I am following the pattern
> established by the visitedlink component which has "visitedlink_browser",
> "visitedlink_common" and "visitedlink_renderer", and the autofill component,
> which has (among other targets) "autofill_content_browser" and
> "autofill_content_renderer" 

I see. I wasn't aware of the existing convention. Agreed that following it
makes sense. Sorry about the delay.

LGTM

[commit-bot: I haz the power, 2013-10-07 10:23:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aberent@chromium.org/23606022/59001

[commit-bot: I haz the power, 2013-10-07 10:48:44]

Retried try job too often on ios_dbg_simulator for step(s) base_unittests,
compile, content_unittests, crypto_unittests, net_unittests, sql_unittests,
ui_unittests, url_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-10-07 11:10:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aberent@chromium.org/23606022/59001

[commit-bot: I haz the power, 2013-10-07 11:31:33]

Retried try job too often on ios_rel_device for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[commit-bot: I haz the power, 2013-10-10 15:07:56]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[commit-bot: I haz the power, 2013-10-10 19:03:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aberent@chromium.org/23606022/124001

[commit-bot: I haz the power, 2013-10-10 19:36:01]

Retried try job too often on ios_dbg_simulator for step(s) base_unittests,
compile, content_unittests, crypto_unittests, net_unittests, sql_unittests,
ui_unittests, url_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-10-11 10:31:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aberent@chromium.org/23606022/142001

[commit-bot: I haz the power, 2013-10-11 15:16:19]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-10-11 15:55:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aberent@chromium.org/23606022/142001

[commit-bot: I haz the power, 2013-10-11 19:08:38]

Change committed as 228211