Make plugins work in a RenderViewTest

Make plugins work in a RenderViewTest

BUG=none
Test=Some new browser plugin tests that will be upstreamed independently


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

[jam, 2012-07-30 20:31:24]

is there another way to do this that doesn't involve adding more stuff to the
public API of RenderThread? the public api is meant for consumers of content, so
it's best to avoid changing it just for internal usage.

i.e. instead of !content::RenderThread::Get()->IsPluginRefreshAllowed, perhaps
RenderViewTest::OnMessageReceived can dispatch ViewHostMsg_GetPlugins?

[Fady Samuel, 2012-07-30 20:36:34]

On 2012/07/30 20:31:24, John Abd-El-Malek wrote:
> is there another way to do this that doesn't involve adding more stuff to the
> public API of RenderThread? the public api is meant for consumers of content,
so
> it's best to avoid changing it just for internal usage.
> 
> i.e. instead of !content::RenderThread::Get()->IsPluginRefreshAllowed, perhaps
> RenderViewTest::OnMessageReceived can dispatch ViewHostMsg_GetPlugins?

Sorry, I'm not sure I follow. The
!content::RenderThread::Get()->IsPluginRefreshAllowed is in
renderer_webkitplatformsupport_impl.cc.

[jam, 2012-07-30 20:38:46]

On Mon, Jul 30, 2012 at 1:36 PM, <fsamuel@chromium.org> wrote:

> On 2012/07/30 20:31:24, John Abd-El-Malek wrote:
>
>> is there another way to do this that doesn't involve adding more stuff to
>> the
>> public API of RenderThread? the public api is meant for consumers of
>> content,
>>
> so
>
>> it's best to avoid changing it just for internal usage.
>>
>
>  i.e. instead of !content::RenderThread::Get()-**>IsPluginRefreshAllowed,
>> perhaps
>> RenderViewTest::**OnMessageReceived can dispatch ViewHostMsg_GetPlugins?
>>
>
> Sorry, I'm not sure I follow. The
> !content::RenderThread::Get()-**>IsPluginRefreshAllowed is in
> renderer_**webkitplatformsupport_impl.cc.
>

I mean let that code send it without having any check. then dispatch it
somewhere in the test harness

>
>
https://chromiumcodereview.**appspot.com/10823086/<https://chromiumcodereview...
>

[Fady Samuel, 2012-07-30 22:19:48]

On 2012/07/30 20:38:46, John Abd-El-Malek wrote:
> On Mon, Jul 30, 2012 at 1:36 PM, <mailto:fsamuel@chromium.org> wrote:
> 
> > On 2012/07/30 20:31:24, John Abd-El-Malek wrote:
> >
> >> is there another way to do this that doesn't involve adding more stuff to
> >> the
> >> public API of RenderThread? the public api is meant for consumers of
> >> content,
> >>
> > so
> >
> >> it's best to avoid changing it just for internal usage.
> >>
> >
> >  i.e. instead of !content::RenderThread::Get()-**>IsPluginRefreshAllowed,
> >> perhaps
> >> RenderViewTest::**OnMessageReceived can dispatch ViewHostMsg_GetPlugins?
> >>
> >
> > Sorry, I'm not sure I follow. The
> > !content::RenderThread::Get()-**>IsPluginRefreshAllowed is in
> > renderer_**webkitplatformsupport_impl.cc.
> >
> 
> I mean let that code send it without having any check. then dispatch it
> somewhere in the test harness
> 
> >
> >
>
https://chromiumcodereview.**appspot.com/10823086/%3Chttps://chromiumcoderevi...>
> >
This seems to be the only use of plugin_refresh_allowed. Do we care about this
or shall I remove it? I'm not very familiar with this code.

[jam, 2012-07-30 22:20:53]

On Mon, Jul 30, 2012 at 3:19 PM, <fsamuel@chromium.org> wrote:

> On 2012/07/30 20:38:46, John Abd-El-Malek wrote:
>
>  On Mon, Jul 30, 2012 at 1:36 PM, <mailto:fsamuel@chromium.org> wrote:
>>
>
>  > On 2012/07/30 20:31:24, John Abd-El-Malek wrote:
>> >
>> >> is there another way to do this that doesn't involve adding more stuff
>> to
>> >> the
>> >> public API of RenderThread? the public api is meant for consumers of
>> >> content,
>> >>
>> > so
>> >
>> >> it's best to avoid changing it just for internal usage.
>> >>
>> >
>> >  i.e. instead of !content::RenderThread::Get()-**
>> **>IsPluginRefreshAllowed,
>> >> perhaps
>> >> RenderViewTest::****OnMessageReceived can dispatch
>> ViewHostMsg_GetPlugins?
>>
>> >>
>> >
>> > Sorry, I'm not sure I follow. The
>> > !content::RenderThread::Get()-****>IsPluginRefreshAllowed is in
>> > renderer_****webkitplatformsupport_impl.cc.
>> >
>>
>
>  I mean let that code send it without having any check. then dispatch it
>> somewhere in the test harness
>>
>
>  >
>> >
>>
>
> https://chromiumcodereview.**a**ppspot.com/10823086/%3Chttps:/**
>
/chromiumcodereview.appspot.**com/10823086/<http://appspot.com/10823086/%3Chttps://chromiumcodereview.appspot.com/10823086/>
> >
>
>> >
>>
> This seems to be the only use of plugin_refresh_allowed. Do we care about
> this
> or shall I remove it? I'm not very familiar with this code.
>

I assume it got added for a reason. Have you tracked the history?

>
>
>
>
https://chromiumcodereview.**appspot.com/10823086/<https://chromiumcodereview...
>

[Fady Samuel, 2012-07-30 22:37:27]

> I assume it got added for a reason. Have you tracked the history?
> 
Yup, http://codereview.chromium.org/88020. I suppose this doesn't look
absolutely necessary as RenderMessageFilter::OnGetPlugins sets a minimum
threshold between refreshes, and so the second of two consecutive requests to
refresh plugins will be ignored.

[Fady Samuel, 2012-07-31 19:57:10]

Updated according to jam@'s comments :-)

[jam, 2012-07-31 20:05:07]

lgtm

http://codereview.chromium.org/10823086/diff/9001/content/renderer/renderer_w...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

http://codereview.chromium.org/10823086/diff/9001/content/renderer/renderer_w...
content/renderer/renderer_webkitplatformsupport_impl.cc:188:
content::RenderThread* render_thread = content::RenderThread::Get();
nit: just do a "using content::RenderThread" at the top. it'll make this easier
to read, and this code should move into the content namespace anyways in the
future.

http://codereview.chromium.org/10823086/diff/9001/content/renderer/renderer_w...
content/renderer/renderer_webkitplatformsupport_impl.cc:574: RenderThreadImpl*
thread = RenderThreadImpl::current();
what about here and the few usages below of RenderThreadImpl? if someone writes
a RenderViewTest and these end up being called, things will still blow up.

you dont have to fix it now, since this existed before, but this seems like
something that'll bite us again.

http://codereview.chromium.org/10823086/diff/9001/content/renderer/renderer_w...
File content/renderer/renderer_webkitplatformsupport_impl.h (right):

http://codereview.chromium.org/10823086/diff/9001/content/renderer/renderer_w...
content/renderer/renderer_webkitplatformsupport_impl.h:36: bool
plugin_refresh_allowed() const { return plugin_refresh_allowed_; }
nit: not needed, so no point adding it

[Fady Samuel, 2012-07-31 20:20:59]

Fixed nits, added TODOs for remaining uses of RenderThreadImpl + committing.

http://codereview.chromium.org/10823086/diff/9001/content/renderer/renderer_w...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

http://codereview.chromium.org/10823086/diff/9001/content/renderer/renderer_w...
content/renderer/renderer_webkitplatformsupport_impl.cc:188:
content::RenderThread* render_thread = content::RenderThread::Get();
On 2012/07/31 20:05:07, John Abd-El-Malek wrote:
> nit: just do a "using content::RenderThread" at the top. it'll make this
easier
> to read, and this code should move into the content namespace anyways in the
> future.

Done.

http://codereview.chromium.org/10823086/diff/9001/content/renderer/renderer_w...
content/renderer/renderer_webkitplatformsupport_impl.cc:574: RenderThreadImpl*
thread = RenderThreadImpl::current();
On 2012/07/31 20:05:07, John Abd-El-Malek wrote:
> what about here and the few usages below of RenderThreadImpl? if someone
writes
> a RenderViewTest and these end up being called, things will still blow up.
> 
> you dont have to fix it now, since this existed before, but this seems like
> something that'll bite us again.

Added TODOs.

http://codereview.chromium.org/10823086/diff/9001/content/renderer/renderer_w...
File content/renderer/renderer_webkitplatformsupport_impl.h (right):

http://codereview.chromium.org/10823086/diff/9001/content/renderer/renderer_w...
content/renderer/renderer_webkitplatformsupport_impl.h:36: bool
plugin_refresh_allowed() const { return plugin_refresh_allowed_; }
On 2012/07/31 20:05:07, John Abd-El-Malek wrote:
> nit: not needed, so no point adding it

Done.

[Fady Samuel, 2012-07-31 20:20:59]

Fixed nits, added TODOs for remaining uses of RenderThreadImpl + committing.

http://codereview.chromium.org/10823086/diff/9001/content/renderer/renderer_w...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

http://codereview.chromium.org/10823086/diff/9001/content/renderer/renderer_w...
content/renderer/renderer_webkitplatformsupport_impl.cc:188:
content::RenderThread* render_thread = content::RenderThread::Get();
On 2012/07/31 20:05:07, John Abd-El-Malek wrote:
> nit: just do a "using content::RenderThread" at the top. it'll make this
easier
> to read, and this code should move into the content namespace anyways in the
> future.

Done.

http://codereview.chromium.org/10823086/diff/9001/content/renderer/renderer_w...
content/renderer/renderer_webkitplatformsupport_impl.cc:574: RenderThreadImpl*
thread = RenderThreadImpl::current();
On 2012/07/31 20:05:07, John Abd-El-Malek wrote:
> what about here and the few usages below of RenderThreadImpl? if someone
writes
> a RenderViewTest and these end up being called, things will still blow up.
> 
> you dont have to fix it now, since this existed before, but this seems like
> something that'll bite us again.

Added TODOs.

http://codereview.chromium.org/10823086/diff/9001/content/renderer/renderer_w...
File content/renderer/renderer_webkitplatformsupport_impl.h (right):

http://codereview.chromium.org/10823086/diff/9001/content/renderer/renderer_w...
content/renderer/renderer_webkitplatformsupport_impl.h:36: bool
plugin_refresh_allowed() const { return plugin_refresh_allowed_; }
On 2012/07/31 20:05:07, John Abd-El-Malek wrote:
> nit: not needed, so no point adding it

Done.

[commit-bot: I haz the power, 2012-07-31 20:21:18]

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

[commit-bot: I haz the power, 2012-07-31 20:57:42]

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

[commit-bot: I haz the power, 2012-07-31 22:34:55]

Change committed as 149297