Add a PepperHelper::EnablePepperSupportForChannel function to content.

Add a PepperHelper::EnablePepperSupportForChannel function to content.

Adds a function that enables messages from PPAPI plugins to be dispatched
in the browser process. The function adds a PepperMessageFilter to the
plugin-browser channel.
BUG=116317
TEST=manual

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

[brettw, 2012-09-06 23:38:09]

This definitely will create a problem not having the filter right away.
Moreover, we can't do anything that could cause the plugin or renderer to block
waiting for the UI thread without the possibility of deadlock.

There's only one function that needs the host resolver and I think it's
asynchronous from the plugin's perspective. So that's safe to hop to the UI
thread and back. That would also allow us to clean up the rediculous
GetHostResolver stuff I put in there when I had a similar problem using this for
Pepper plugin processes. It seems like this is the only reasonable solution.

http://codereview.chromium.org/10907102/diff/3004/chrome/browser/nacl_host/na...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

http://codereview.chromium.org/10907102/diff/3004/chrome/browser/nacl_host/na...
chrome/browser/nacl_host/nacl_process_host.cc:733: base::Unretained(this),
You're using unretained here and below. What ensures that this object is not
destroyed in the meantime?

I notice there is a weak ptr factory on the NaClProcessHost that you can use.
Weak ptrs are not threadsafe so you have to be careful and not dereference or
copy it while you're on the UI thread. I think the normal Bind stuff will copy
it.

Maybe you can come up with a better design here. One idea is to make a helper
class that's refcounted to do the hop to the UI thread and back, and then that
class would own a weak ptr to the NaClProcessHost and do the final call back to
the NaClProcessHost to complete.

[jam, 2012-09-07 16:09:30]

https://codereview.chromium.org/10907102/diff/3004/content/content_browser.gypi
File content/content_browser.gypi (right):

https://codereview.chromium.org/10907102/diff/3004/content/content_browser.gy...
content/content_browser.gypi:489: 'browser/pepper_helper.cc',
nit: also add the public file above

https://codereview.chromium.org/10907102/diff/3004/content/public/browser/pep...
File content/public/browser/pepper_helper.h (right):

https://codereview.chromium.org/10907102/diff/3004/content/public/browser/pep...
content/public/browser/pepper_helper.h:19: class CONTENT_EXPORT PepperHelper {
nit: no need for class, just the content namespace is enough (by convention we
don't add classes to scope static functions in the content api)

[bbudge, 2012-09-08 01:17:27]

Addressed comments. This now depends on http://codereview.chromium.org/10919169/

http://codereview.chromium.org/10907102/diff/3004/chrome/browser/nacl_host/na...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

http://codereview.chromium.org/10907102/diff/3004/chrome/browser/nacl_host/na...
chrome/browser/nacl_host/nacl_process_host.cc:733: base::Unretained(this),
On 2012/09/06 23:38:09, brettw wrote:
> You're using unretained here and below. What ensures that this object is not
> destroyed in the meantime?
> 
> I notice there is a weak ptr factory on the NaClProcessHost that you can use.
> Weak ptrs are not threadsafe so you have to be careful and not dereference or
> copy it while you're on the UI thread. I think the normal Bind stuff will copy
> it.
> 
> Maybe you can come up with a better design here. One idea is to make a helper
> class that's refcounted to do the hop to the UI thread and back, and then that
> class would own a weak ptr to the NaClProcessHost and do the final call back
to
> the NaClProcessHost to complete.

Moving this to PMF. Done.

http://codereview.chromium.org/10907102/diff/3004/content/content_browser.gypi
File content/content_browser.gypi (right):

http://codereview.chromium.org/10907102/diff/3004/content/content_browser.gyp...
content/content_browser.gypi:489: 'browser/pepper_helper.cc',
On 2012/09/07 16:09:30, John Abd-El-Malek wrote:
> nit: also add the public file above

Done.

http://codereview.chromium.org/10907102/diff/3004/content/public/browser/pepp...
File content/public/browser/pepper_helper.h (right):

http://codereview.chromium.org/10907102/diff/3004/content/public/browser/pepp...
content/public/browser/pepper_helper.h:19: class CONTENT_EXPORT PepperHelper {
On 2012/09/07 16:09:30, John Abd-El-Malek wrote:
> nit: no need for class, just the content namespace is enough (by convention we
> don't add classes to scope static functions in the content api)

Done.

[jam, 2012-09-10 05:39:11]

lgtm

https://codereview.chromium.org/10907102/diff/6008/content/public/browser/pep...
File content/public/browser/pepper_helper.h (right):

https://codereview.chromium.org/10907102/diff/6008/content/public/browser/pep...
content/public/browser/pepper_helper.h:11: class HostResolver;
nit: not needed anymore

https://codereview.chromium.org/10907102/diff/6008/content/public/browser/pep...
content/public/browser/pepper_helper.h:22: int renderer_process_id);
nit: render_process_id by convention

[bbudge, 2012-09-10 17:05:07]

http://codereview.chromium.org/10907102/diff/6008/content/public/browser/pepp...
File content/public/browser/pepper_helper.h (right):

http://codereview.chromium.org/10907102/diff/6008/content/public/browser/pepp...
content/public/browser/pepper_helper.h:11: class HostResolver;
On 2012/09/10 05:39:11, John Abd-El-Malek wrote:
> nit: not needed anymore

Done.

http://codereview.chromium.org/10907102/diff/6008/content/public/browser/pepp...
content/public/browser/pepper_helper.h:22: int renderer_process_id);
On 2012/09/10 05:39:11, John Abd-El-Malek wrote:
> nit: render_process_id by convention

Done.

[bbudge, 2012-09-11 15:17:06]

Changed back to getting the HostResolver in NaClProcessHost and leaving
PepperMessageFilter as-is.

[bbudge, 2012-09-11 17:13:26]

Removed incorrect OVERRIDE from destructor.

[bbudge, 2012-09-11 22:23:55]

PTAL

[brettw, 2012-09-11 22:40:13]

LGTM, woot!

http://codereview.chromium.org/10907102/diff/15001/content/public/browser/pep...
File content/public/browser/pepper_helper.h (right):

http://codereview.chromium.org/10907102/diff/15001/content/public/browser/pep...
content/public/browser/pepper_helper.h:13: namespace IPC {
Nit: can you put this above? I like to keep the namespace alphabetized when it's
easy.

[bbudge, 2012-09-11 22:44:18]

http://codereview.chromium.org/10907102/diff/15001/content/public/browser/pep...
File content/public/browser/pepper_helper.h (right):

http://codereview.chromium.org/10907102/diff/15001/content/public/browser/pep...
content/public/browser/pepper_helper.h:13: namespace IPC {
On 2012/09/11 22:40:13, brettw wrote:
> Nit: can you put this above? I like to keep the namespace alphabetized when
it's
> easy.

Done.

[commit-bot: I haz the power, 2012-09-11 22:44:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/10907102/2009

[commit-bot: I haz the power, 2012-09-12 02:56:56]

Change committed as 156218