Add plugins::TestPluginPlaceholder class and allow its use in Blink layout tests.

Add plugins::TestPluginPlaceholder class and allow its use in Blink layout tests.

BUG=436562
Committed: https://crrev.com/8a377e6551f1937fb9c29a5ee4f2cb9688e86979
Cr-Commit-Position: refs/heads/master@{#332235}

[chrishtr, 2015-05-26 19:52:09]

chrishtr@chromium.org changed reviewers:
+ jochen@chromium.org, tommycli@chromium.org

[chrishtr, 2015-05-26 19:52:10]

The corresponding Blink CL is https://codereview.chromium.org/1153103004. This
will allow us
to test integration issues between Blink and plugins::PluginPlaceholder.

[tommycli, 2015-05-26 20:08:18]

chrishtr: It is an awesome thing that you are making some comprehensive tests
for PluginPlaceholder. It has been sorely needed for a long time, and has caused
many regressions.

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

https://codereview.chromium.org/1158063002/diff/1/components/plugins.gypi#new...
components/plugins.gypi:62: # GN version: //components/plugins/renderer
This isn't in the GN file.

https://codereview.chromium.org/1158063002/diff/1/components/plugins/renderer...
File components/plugins/renderer/test_plugin_placeholder.cc (right):

https://codereview.chromium.org/1158063002/diff/1/components/plugins/renderer...
components/plugins/renderer/test_plugin_placeholder.cc:11: :
PluginPlaceholder(nullptr, frame, params, "<div>Test content</div>")
Can we just eliminate this class entirely? Why not just create a
PluginPlaceholder with the <div>Test content</div> html_data in your test code?

https://codereview.chromium.org/1158063002/diff/1/components/plugins/renderer...
File components/plugins/renderer/test_plugin_placeholder.h (right):

https://codereview.chromium.org/1158063002/diff/1/components/plugins/renderer...
components/plugins/renderer/test_plugin_placeholder.h:13: 
No need for blank line here

https://codereview.chromium.org/1158063002/diff/1/components/plugins/renderer...
components/plugins/renderer/test_plugin_placeholder.h:15: public:
formatting doesn't look right here. maybe git cl format?

[chrishtr, 2015-05-26 20:52:36]

https://codereview.chromium.org/1158063002/diff/1/components/plugins/renderer...
File components/plugins/renderer/test_plugin_placeholder.cc (right):

https://codereview.chromium.org/1158063002/diff/1/components/plugins/renderer...
components/plugins/renderer/test_plugin_placeholder.cc:11: :
PluginPlaceholder(nullptr, frame, params, "<div>Test content</div>")
On 2015/05/26 at 20:08:18, tommycli wrote:
> Can we just eliminate this class entirely? Why not just create a
PluginPlaceholder with the <div>Test content</div> html_data in your test code?

The reason is that someone has to include the string of HTML. Maybe I can
extract that from the child nodes of the containing <embed> tag, will try that.
It would be great for the layout test to easily specify plugin webview content
itself.

[tommycli, 2015-05-26 21:18:02]

https://codereview.chromium.org/1158063002/diff/1/components/plugins/renderer...
File components/plugins/renderer/test_plugin_placeholder.cc (right):

https://codereview.chromium.org/1158063002/diff/1/components/plugins/renderer...
components/plugins/renderer/test_plugin_placeholder.cc:11: :
PluginPlaceholder(nullptr, frame, params, "<div>Test content</div>")
On 2015/05/26 20:52:36, chrishtr wrote:
> On 2015/05/26 at 20:08:18, tommycli wrote:
> > Can we just eliminate this class entirely? Why not just create a
> PluginPlaceholder with the <div>Test content</div> html_data in your test
code?
> 
> The reason is that someone has to include the string of HTML. Maybe I can
> extract that from the child nodes of the containing <embed> tag, will try
that.
> It would be great for the layout test to easily specify plugin webview content
> itself.

Perhaps I am misunderstanding something. Can "<div>Test content</div>" just be a
string literal within web_test_proxy.cc?

[chrishtr, 2015-05-26 21:23:26]

On 2015/05/26 at 21:18:02, tommycli wrote:
>
https://codereview.chromium.org/1158063002/diff/1/components/plugins/renderer...
> File components/plugins/renderer/test_plugin_placeholder.cc (right):
> 
>
https://codereview.chromium.org/1158063002/diff/1/components/plugins/renderer...
> components/plugins/renderer/test_plugin_placeholder.cc:11: :
PluginPlaceholder(nullptr, frame, params, "<div>Test content</div>")
> On 2015/05/26 20:52:36, chrishtr wrote:
> > On 2015/05/26 at 20:08:18, tommycli wrote:
> > > Can we just eliminate this class entirely? Why not just create a
> > PluginPlaceholder with the <div>Test content</div> html_data in your test
code?
> > 
> > The reason is that someone has to include the string of HTML. Maybe I can
> > extract that from the child nodes of the containing <embed> tag, will try
that.
> > It would be great for the layout test to easily specify plugin webview
content
> > itself.
> 
> Perhaps I am misunderstanding something. Can "<div>Test content</div>" just be
a string literal within web_test_proxy.cc?

It could, but it would be even better to be able to inject arbitrary content
into the plugin's webview, in order to test corner cases. Blink has its own
PluginPlaceholder class that is tested in this way (with some special layout
test methods to help with injection). We do some of that with some other testing
of plugin placeholders (that unfortuantely do not amount to good integration
testing with non-Blink plugin code).

[jochen (gone - plz use gerrit), 2015-05-27 14:59:43]

https://codereview.chromium.org/1158063002/diff/1/content/content_shell.gypi
File content/content_shell.gypi (right):

https://codereview.chromium.org/1158063002/diff/1/content/content_shell.gypi#...
content/content_shell.gypi:53:
'../components/components.gyp:test_plugins_renderer',
this needs to be added in the gn version as well

[chrishtr, 2015-05-28 20:11:05]

On 2015/05/27 at 14:59:43, jochen wrote:
> https://codereview.chromium.org/1158063002/diff/1/content/content_shell.gypi
> File content/content_shell.gypi (right):
> 
>
https://codereview.chromium.org/1158063002/diff/1/content/content_shell.gypi#...
> content/content_shell.gypi:53:
'../components/components.gyp:test_plugins_renderer',
> this needs to be added in the gn version as well

Done.

[chrishtr, 2015-05-28 20:11:44]

On 2015/05/26 at 21:18:02, tommycli wrote:
>
https://codereview.chromium.org/1158063002/diff/1/components/plugins/renderer...
> File components/plugins/renderer/test_plugin_placeholder.cc (right):
> 
>
https://codereview.chromium.org/1158063002/diff/1/components/plugins/renderer...
> components/plugins/renderer/test_plugin_placeholder.cc:11: :
PluginPlaceholder(nullptr, frame, params, "<div>Test content</div>")
> On 2015/05/26 20:52:36, chrishtr wrote:
> > On 2015/05/26 at 20:08:18, tommycli wrote:
> > > Can we just eliminate this class entirely? Why not just create a
> > PluginPlaceholder with the <div>Test content</div> html_data in your test
code?
> > 
> > The reason is that someone has to include the string of HTML. Maybe I can
> > extract that from the child nodes of the containing <embed> tag, will try
that.
> > It would be great for the layout test to easily specify plugin webview
content
> > itself.
> 
> Perhaps I am misunderstanding something. Can "<div>Test content</div>" just be
a string literal within web_test_proxy.cc?

I moved TestPluginPlaceholder into the content/plugins/renderer directory. Still
need the class because there is a pure virtual method now on PluginPlaceholder,
and I don't want to depend on chrome/.

[chrishtr, 2015-05-28 20:13:57]

FYI this patch would have prevented a canary crash bug I introduced yesterday.

Both :( and :) apply....

https://codereview.chromium.org/1158063002/diff/1/components/plugins/renderer...
File components/plugins/renderer/test_plugin_placeholder.h (right):

https://codereview.chromium.org/1158063002/diff/1/components/plugins/renderer...
components/plugins/renderer/test_plugin_placeholder.h:13: 
On 2015/05/26 at 20:08:18, tommycli wrote:
> No need for blank line here

Done.

https://codereview.chromium.org/1158063002/diff/1/components/plugins/renderer...
components/plugins/renderer/test_plugin_placeholder.h:15: public:
On 2015/05/26 at 20:08:18, tommycli wrote:
> formatting doesn't look right here. maybe git cl format?

Ran git cl format.

[tommycli, 2015-05-28 21:13:34]

chrishtr: see below. i apologize for the current state of PluginPlaceholder

https://codereview.chromium.org/1158063002/diff/40001/content/shell/renderer/...
File content/shell/renderer/test_runner/test_plugin_placeholder.cc (right):

https://codereview.chromium.org/1158063002/diff/40001/content/shell/renderer/...
content/shell/renderer/test_runner/test_plugin_placeholder.cc:17: LOG(ERROR) <<
"TestPluginPlaceholder::TestPluginPlaceholder";
Stray log message.

https://codereview.chromium.org/1158063002/diff/40001/content/shell/renderer/...
File content/shell/renderer/test_runner/test_plugin_placeholder.h (right):

https://codereview.chromium.org/1158063002/diff/40001/content/shell/renderer/...
content/shell/renderer/test_runner/test_plugin_placeholder.h:17: void
BindWebFrame(blink::WebFrame* frame) override {}
This cannot be a no-op, as then this object isn't collected by V8 garbage
collection and leaks.

It's an oversight that BindWebFrame is pure virtual in PluginPlaceholder. The
two subclasses of PluginPlaceholder (LoadablePluginPlaceholder and
MobileYoutubePlugin) both implement that BindWebFrame method in the exact same
way.

You can copy their implementations into here if you want.

Otherwise, if this patch lands and sticks:
https://codereview.chromium.org/1148223004/, then this class will be
unnecessary.

[chrishtr, 2015-05-28 21:16:52]

https://codereview.chromium.org/1158063002/diff/40001/content/shell/renderer/...
File content/shell/renderer/test_runner/test_plugin_placeholder.cc (right):

https://codereview.chromium.org/1158063002/diff/40001/content/shell/renderer/...
content/shell/renderer/test_runner/test_plugin_placeholder.cc:17: LOG(ERROR) <<
"TestPluginPlaceholder::TestPluginPlaceholder";
On 2015/05/28 at 21:13:33, tommycli wrote:
> Stray log message.

Fixed.

https://codereview.chromium.org/1158063002/diff/40001/content/shell/renderer/...
File content/shell/renderer/test_runner/test_plugin_placeholder.h (right):

https://codereview.chromium.org/1158063002/diff/40001/content/shell/renderer/...
content/shell/renderer/test_runner/test_plugin_placeholder.h:17: void
BindWebFrame(blink::WebFrame* frame) override {}
On 2015/05/28 at 21:13:34, tommycli wrote:
> This cannot be a no-op, as then this object isn't collected by V8 garbage
collection and leaks.
> 
> It's an oversight that BindWebFrame is pure virtual in PluginPlaceholder. The
two subclasses of PluginPlaceholder (LoadablePluginPlaceholder and
MobileYoutubePlugin) both implement that BindWebFrame method in the exact same
way.
> 
> You can copy their implementations into here if you want.
> 
> Otherwise, if this patch lands and sticks:
https://codereview.chromium.org/1148223004/, then this class will be
unnecessary.
Done.

[tommycli, 2015-05-28 21:19:10]

LGTM

[jochen (gone - plz use gerrit), 2015-05-29 11:18:40]

test_runner must not depend on content, however, this introduces a dependency
via the plugin component

can we either change the plugin placeholder to be independent of content, or
move the test plugin to content/shell/renderer/layout_test?

[chrishtr, 2015-05-29 19:27:29]

On 2015/05/29 at 11:18:41, jochen wrote:
> test_runner must not depend on content, however, this introduces a dependency
via the plugin component
> 
> can we either change the plugin placeholder to be independent of content, or
move the test plugin to content/shell/renderer/layout_test?

Done.

It's a little unclear whether I have the build files right, since they conflate
layout_test and non-layout_test.

[chrishtr, 2015-05-29 22:30:52]

Also, I failed one test, because of:

ERROR in
/b/build/slave/linux/build/src/content/shell/renderer/layout_test/test_plugin_placeholder.h
  Illegal include: "components/plugins/renderer/plugin_placeholder.h"
    Because of no rule applying.

Should I add a DEPS file there?

[jochen (gone - plz use gerrit), 2015-06-01 13:01:12]

you can just add it to content/shell/renderer/DEPS

lgtm with that

[chrishtr, 2015-06-01 15:18:39]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-06-01 15:18:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1158063002/#ps180001
(title: " ")

[commit-bot: I haz the power, 2015-06-01 15:19:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1158063002/180001

[chrishtr, 2015-06-01 15:19:22]

On 2015/06/01 at 13:01:12, jochen wrote:
> you can just add it to content/shell/renderer/DEPS
> 
> lgtm with that

Done.

[commit-bot: I haz the power, 2015-06-01 18:01:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-01 18:01:44]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chrishtr, 2015-06-01 18:09:22]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2015-06-01 18:10:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1158063002/180001

[commit-bot: I haz the power, 2015-06-01 19:20:19]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-06-01 19:21:09]

Patchset 10 (id:??) landed as
https://crrev.com/8a377e6551f1937fb9c29a5ee4f2cb9688e86979
Cr-Commit-Position: refs/heads/master@{#332235}