Convert the async device ID getter to a chrome resource host

Convert the async device ID getter to a chrome resource host

[brettw, 2012-09-09 19:48:55]

John: can you look at the content API integration?

I added a new function on content_browser_client that a PPAPI plugin was
created. The renderer version is on the RenderView observer because that was
convenient. But in the browser I need it on the I/O thread and there doesn't
seem like any alternative.

It seems like it would make the most sense to make the oldrenderer like this
browser version I'm adding and move it to the content_renderer_client. This
would also allow me to delete the chrome\renderer\pepper\pepper_helper file
whose sole purpose is to hook this stuff up.

[jam, 2012-09-10 16:22:21]

It's a little confusing that chrome now dispatches ppapi messages as well as
content...

On 2012/09/09 19:48:55, brettw wrote:
> John: can you look at the content API integration?
> 
> I added a new function on content_browser_client that a PPAPI plugin was
> created. The renderer version is on the RenderView observer because that was
> convenient. But in the browser I need it on the I/O thread and there doesn't
> seem like any alternative.
> 
> It seems like it would make the most sense to make the oldrenderer like this
> browser version I'm adding and move it to the content_renderer_client. This
> would also allow me to delete the chrome\renderer\pepper\pepper_helper file
> whose sole purpose is to hook this stuff up.

I don't understand this second paragraph, I must be missing something.

https://codereview.chromium.org/10909138/diff/3024/content/public/browser/bro...
File content/public/browser/browser_ppapi_host.h (right):

https://codereview.chromium.org/10909138/diff/3024/content/public/browser/bro...
content/public/browser/browser_ppapi_host.h:30: struct RenderViewIDs {
nit: we've avoied a struct for these two so far afaik; i think it wouldn't be
consistent to use it in some places.

https://codereview.chromium.org/10909138/diff/3024/content/public/browser/con...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/10909138/diff/3024/content/public/browser/con...
content/public/browser/content_browser_client.h:438: // Notifification that a
pepper plugin has just been spawned. This allows the
nit: Notification

[jam, 2012-09-10 22:14:27]

On 2012/09/10 19:26:52, brettw wrote:

empty comment?

[yzshen1, 2012-09-11 00:53:17]

only some nits.

Thanks!

https://chromiumcodereview.appspot.com/10909138/diff/3024/chrome/browser/rend...
File chrome/browser/renderer_host/pepper/DEPS (right):

https://chromiumcodereview.appspot.com/10909138/diff/3024/chrome/browser/rend...
chrome/browser/renderer_host/pepper/DEPS:2: "+ppapi/host",
We already have line2&3 in chrome/browser/DEPS.

https://chromiumcodereview.appspot.com/10909138/diff/3024/chrome/browser/rend...
File chrome/browser/renderer_host/pepper/pepper_flash_device_id_host.h (right):

https://chromiumcodereview.appspot.com/10909138/diff/3024/chrome/browser/rend...
chrome/browser/renderer_host/pepper/pepper_flash_device_id_host.h:66: // Access
only on IO thread (this class is destroyed on the IO thread
1. Access -> Accessed
2. Unbalanced '('

https://chromiumcodereview.appspot.com/10909138/diff/3024/content/public/brow...
File content/public/browser/browser_ppapi_host.h (right):

https://chromiumcodereview.appspot.com/10909138/diff/3024/content/public/brow...
content/public/browser/browser_ppapi_host.h:47: // The PP_Instance should be
already validated by the host before creating
I don't understand this comment. Is it out-dated or misplaced?

https://chromiumcodereview.appspot.com/10909138/diff/3024/content/public/brow...
content/public/browser/browser_ppapi_host.h:68: void
PostOnUIThreadWithRenderViewAndReply(
This method is not used in this CL. Is it obsolete?

https://chromiumcodereview.appspot.com/10909138/diff/3024/ppapi/examples/2d/p...
File ppapi/examples/2d/paint_manager_example.cc (right):

https://chromiumcodereview.appspot.com/10909138/diff/3024/ppapi/examples/2d/p...
ppapi/examples/2d/paint_manager_example.cc:11: #include
"ppapi/cpp/private/flash_device_id.h"
I think you are not going to commit this file, right?

https://chromiumcodereview.appspot.com/10909138/diff/3024/ppapi/host/ppapi_ho...
File ppapi/host/ppapi_host.cc (right):

https://chromiumcodereview.appspot.com/10909138/diff/3024/ppapi/host/ppapi_ho...
ppapi/host/ppapi_host.cc:135: NOTREACHED();
According to the comment in resource_host.h
"Some (but not all) resources have a corresponding object in the host side"

So it seems okay to get NULL here.

https://chromiumcodereview.appspot.com/10909138/diff/3024/ppapi/proxy/flash_d...
File ppapi/proxy/flash_device_id_resource.h (right):

https://chromiumcodereview.appspot.com/10909138/diff/3024/ppapi/proxy/flash_d...
ppapi/proxy/flash_device_id_resource.h:5: #ifndef
PPAPI_RESOURCE_FLASH_DEVICE_ID_RESOURCE_H_
it should be the same as the file path.

[brettw, 2012-09-11 20:25:19]

New snap up and responses to comments.

https://chromiumcodereview.appspot.com/10909138/diff/3024/content/public/brow...
File content/public/browser/browser_ppapi_host.h (right):

https://chromiumcodereview.appspot.com/10909138/diff/3024/content/public/brow...
content/public/browser/browser_ppapi_host.h:30: struct RenderViewIDs {
I insert these into a map. I can use a pair but then the review comment would be
that I should use a named struct. I could keep the struct internal to this class
and expose two out params for the GetRenderViewIDsForInstance.

https://chromiumcodereview.appspot.com/10909138/diff/3024/content/public/brow...
content/public/browser/browser_ppapi_host.h:47: // The PP_Instance should be
already validated by the host before creating
I clarified this a bit. I just didn't want everybody to feel like they have to
check for errors every time they call this.

https://chromiumcodereview.appspot.com/10909138/diff/3024/content/public/brow...
content/public/browser/browser_ppapi_host.h:68: void
PostOnUIThreadWithRenderViewAndReply(
Yeah, I removed this and stashed it for when we need it later.

https://chromiumcodereview.appspot.com/10909138/diff/3024/ppapi/host/ppapi_ho...
File ppapi/host/ppapi_host.cc (right):

https://chromiumcodereview.appspot.com/10909138/diff/3024/ppapi/host/ppapi_ho...
ppapi/host/ppapi_host.cc:135: NOTREACHED();
If there isn't a corresponding host object, the proxy shouldn't be sending a
"created" message to the browser/renderer to create the host.

[yzshen1, 2012-09-11 20:43:22]

LGTM

https://chromiumcodereview.appspot.com/10909138/diff/12001/content/public/bro...
File content/public/browser/browser_ppapi_host.h (right):

https://chromiumcodereview.appspot.com/10909138/diff/12001/content/public/bro...
content/public/browser/browser_ppapi_host.h:50: // from this function except as
a last-minute sanity check if you conver the
conver->convert

[brettw, 2012-09-11 22:54:25]

John: the struct of IDs is no longer public, I made it private and the getter
just has two int out params.

[jam, 2012-09-11 23:50:25]

content/public lgtm

[commit-bot: I haz the power, 2012-09-15 04:15:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brettw@chromium.org/10909138/2030

[commit-bot: I haz the power, 2012-09-15 04:17:09]

Step "update" is always a major failure.
Look at the try server FAQ for more details.