Make PepperWebPlugin not use RenderViews.

Make PepperWebPlugin not use RenderView.

BUG=304341
R=nasko@chromium.org, yoz@chromium.org, yzshen@chromium.org

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

[jam, 2013-12-19 19:09:58]

Yoyo: please look at files with extensions in the path
Yuzhu: please look at files with pepper in the path
Nasko: the rest, and of course feel free to look at anything else

[Yoyo Zhou, 2013-12-20 00:28:37]

extensions LGTM

https://chromiumcodereview.appspot.com/105553005/diff/40001/chrome/browser/ex...
File chrome/browser/extensions/extension_function_dispatcher.h (right):

https://chromiumcodereview.appspot.com/105553005/diff/40001/chrome/browser/ex...
chrome/browser/extensions/extension_function_dispatcher.h:108: // The response
is sent to the corresponding render view in an
Does this comment need updating?

https://chromiumcodereview.appspot.com/105553005/diff/40001/extensions/browse...
File extensions/browser/extension_function.h (right):

https://chromiumcodereview.appspot.com/105553005/diff/40001/extensions/browse...
extensions/browser/extension_function.h:341: // The RenderFrameHost we will send
responses too.
nit: to (here and 338)

[yzshen1, 2013-12-20 01:52:48]

Pepper stuff LGTM

Thanks!

https://codereview.chromium.org/105553005/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_function_dispatcher.h (right):

https://codereview.chromium.org/105553005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_function_dispatcher.h:111:
content::RenderViewHost* render_view_host);
Now the two Dispatch.*() methods don't use the same Render.*Host parameter. I
guess it is because you don't want to touch all other extension-related code in
this same CL?

If there is a plan to also change that part, maybe we could add a TODO.

https://codereview.chromium.org/105553005/diff/40001/content/common/pepper_re...
File content/common/pepper_renderer_instance_data.cc (right):

https://codereview.chromium.org/105553005/diff/40001/content/common/pepper_re...
content/common/pepper_renderer_instance_data.cc:16: int render_frame,
nit: it is nice to use the same name in the declaration and definition.

[nasko, 2013-12-20 14:53:53]

LGTM with a few nits.

https://codereview.chromium.org/105553005/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/105553005/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_function_dispatcher.cc:329: RenderViewHost*
render_view_host,
nit: prefix RVH with content, so it is consistent with RFH below.

https://codereview.chromium.org/105553005/diff/80001/chrome/browser/renderer_...
File chrome/browser/renderer_host/pepper/pepper_flash_drm_host.cc (right):

https://codereview.chromium.org/105553005/diff/80001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_flash_drm_host.cc:72: void
FetchMonitorFromWidget() {
nit: The method name doesn't match its functionality now.

https://codereview.chromium.org/105553005/diff/80001/chrome/browser/renderer_...
File
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc
(right):

https://codereview.chromium.org/105553005/diff/80001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:171:
LOG(WARNING) << "RenderFrameHost is not alive.";
nit: Technically, it doesn't exist. It can exist and not be alive, but this is
not the check here.

https://codereview.chromium.org/105553005/diff/80001/extensions/browser/exten...
File extensions/browser/extension_function.cc (right):

https://codereview.chromium.org/105553005/diff/80001/extensions/browser/exten...
extensions/browser/extension_function.cc:189: render_frame_host_ =
render_frame_host;
nit: Should we DCHECK somewhere that one or the other is set, but not both?

[jam, 2013-12-20 17:00:48]

https://codereview.chromium.org/105553005/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_function_dispatcher.h (right):

https://codereview.chromium.org/105553005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_function_dispatcher.h:108: // The response
is sent to the corresponding render view in an
On 2013/12/20 00:28:37, Yoyo Zhou wrote:
> Does this comment need updating?

Done.

https://codereview.chromium.org/105553005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_function_dispatcher.h:111:
content::RenderViewHost* render_view_host);
On 2013/12/20 01:52:49, yzshen1 wrote:
> Now the two Dispatch.*() methods don't use the same Render.*Host parameter. I
> guess it is because you don't want to touch all other extension-related code
in
> this same CL?

exactly

> 
> If there is a plan to also change that part, maybe we could add a TODO.

added

https://codereview.chromium.org/105553005/diff/40001/content/common/pepper_re...
File content/common/pepper_renderer_instance_data.cc (right):

https://codereview.chromium.org/105553005/diff/40001/content/common/pepper_re...
content/common/pepper_renderer_instance_data.cc:16: int render_frame,
On 2013/12/20 01:52:49, yzshen1 wrote:
> nit: it is nice to use the same name in the declaration and definition.

yeah I agree. It looked like in this case it was done like that to make it clear
which variable is the member one vs the parameter

https://codereview.chromium.org/105553005/diff/40001/extensions/browser/exten...
File extensions/browser/extension_function.h (right):

https://codereview.chromium.org/105553005/diff/40001/extensions/browser/exten...
extensions/browser/extension_function.h:341: // The RenderFrameHost we will send
responses too.
On 2013/12/20 00:28:37, Yoyo Zhou wrote:
> nit: to (here and 338)

Done.

https://codereview.chromium.org/105553005/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

https://codereview.chromium.org/105553005/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_function_dispatcher.cc:329: RenderViewHost*
render_view_host,
On 2013/12/20 14:53:54, nasko wrote:
> nit: prefix RVH with content, so it is consistent with RFH below.

the background is when I started content refactoring I put "using content::" foo
in a lot of places. I now regret that, and I'm trying not to do that with
RenderFrameHost.

https://codereview.chromium.org/105553005/diff/80001/chrome/browser/renderer_...
File chrome/browser/renderer_host/pepper/pepper_flash_drm_host.cc (right):

https://codereview.chromium.org/105553005/diff/80001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_flash_drm_host.cc:72: void
FetchMonitorFromWidget() {
On 2013/12/20 14:53:54, nasko wrote:
> nit: The method name doesn't match its functionality now.

i've clarified in RenderFrameHost::GetNativeView that it returns the widget's
native view

https://codereview.chromium.org/105553005/diff/80001/chrome/browser/renderer_...
File
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc
(right):

https://codereview.chromium.org/105553005/diff/80001/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_output_protection_message_filter.cc:171:
LOG(WARNING) << "RenderFrameHost is not alive.";
On 2013/12/20 14:53:54, nasko wrote:
> nit: Technically, it doesn't exist. It can exist and not be alive, but this is
> not the check here.

I had just left the same terminology that was used before..

https://codereview.chromium.org/105553005/diff/80001/extensions/browser/exten...
File extensions/browser/extension_function.cc (right):

https://codereview.chromium.org/105553005/diff/80001/extensions/browser/exten...
extensions/browser/extension_function.cc:189: render_frame_host_ =
render_frame_host;
On 2013/12/20 14:53:54, nasko wrote:
> nit: Should we DCHECK somewhere that one or the other is set, but not both?

Done.