Refactor video capture to new design

Refactor video capture to new design, as part of the whole Pepper resource redesign.

New design provides higher performance and involves writing much less code.  See the pepper implementation doc for detail.

http://www.chromium.org/developers/design-documents/pepper-plugin-implementation

BUG=

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

[victorhsieh, 2012-10-25 09:16:42]

I've tested it with ppapi_example_vc and a flash with webcam.

But please check my comment in the host file.  I need to return
PP_OK_COMPLETIONPENDING in OnOpen instead to make the "Default" case/link in
ppapi_example_vc work, otherwise it will crash and complain that no
corresponding callback for the sequence number.  Not sure this is that right
solution.

This change depends on http://codereview.chromium.org/11267034/ .

http://codereview.chromium.org/11274036/diff/4026/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_video_capture_host.cc (right):

http://codereview.chromium.org/11274036/diff/4026/content/renderer/pepper/pep...
content/renderer/pepper/pepper_video_capture_host.cc:284: return
PP_OK_COMPLETIONPENDING;
The orignal impl returns PP_OK, but that will fail in current messaging code. 
Does it make sense to return PP_OK_COMPLETIONPENDING?

[yzshen1, 2012-10-25 20:41:05]

Here are a few high-level comments.

I am OOO today and will take a closer look at the CL later.

Brett: if you are busy, I can review the details. (Because I wrote part of the
original code and I am adding a new feature to this API.)

http://codereview.chromium.org/11274036/diff/4026/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_video_capture_host.cc (right):

http://codereview.chromium.org/11274036/diff/4026/content/renderer/pepper/pep...
content/renderer/pepper/pepper_video_capture_host.cc:251:
instance->delegate()->EnumerateDevices(
FYI: I don't think we want to use the plugin delegate any more in the new-style
host. But it is fine that you use it for now, because I am moving device
enumeration/open/close out of plugin delegate in another CL.

http://codereview.chromium.org/11274036/diff/4026/content/renderer/pepper/pep...
content/renderer/pepper/pepper_video_capture_host.cc:284: return
PP_OK_COMPLETIONPENDING;
On 2012/10/25 09:16:42, Victor Hsieh wrote:
> The orignal impl returns PP_OK, but that will fail in current messaging code. 
> Does it make sense to return PP_OK_COMPLETIONPENDING?

Yes. The original proxy code uses EnterHostFromHostResourceForceCallback in
OnMsgOpen(). It would force callback even if you return PP_OK.

http://codereview.chromium.org/11274036/diff/4026/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_video_capture_host.h (right):

http://codereview.chromium.org/11274036/diff/4026/content/renderer/pepper/pep...
content/renderer/pepper/pepper_video_capture_host.h:80: virtual int32_t
InternalStartCapture0_1(
We can probably remove the support of V0.1 entirely.
You may want to send a mail to pepper-team to see whether it will break
anything.

http://codereview.chromium.org/11274036/diff/4026/ppapi/shared_impl/ppb_video...
File ppapi/shared_impl/ppb_video_capture_shared.h (right):

http://codereview.chromium.org/11274036/diff/4026/ppapi/shared_impl/ppb_video...
ppapi/shared_impl/ppb_video_capture_shared.h:15: class PPAPI_SHARED_EXPORT
PPB_VideoCapture_Shared {
I think this class is not very useful now.
All it does is checking the open_state_. (And SetStatus() is only used by the
host side.)
Does it make sense to remove this class and verify the open state at only one
side?

[victorhsieh, 2012-10-26 02:34:54]

http://codereview.chromium.org/11274036/diff/4026/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_video_capture_host.cc (right):

http://codereview.chromium.org/11274036/diff/4026/content/renderer/pepper/pep...
content/renderer/pepper/pepper_video_capture_host.cc:251:
instance->delegate()->EnumerateDevices(
On 2012/10/25 20:41:05, yzshen1 wrote:
> FYI: I don't think we want to use the plugin delegate any more in the
new-style
> host. But it is fine that you use it for now, because I am moving device
> enumeration/open/close out of plugin delegate in another CL.

I see.

http://codereview.chromium.org/11274036/diff/4026/ppapi/shared_impl/ppb_video...
File ppapi/shared_impl/ppb_video_capture_shared.h (right):

http://codereview.chromium.org/11274036/diff/4026/ppapi/shared_impl/ppb_video...
ppapi/shared_impl/ppb_video_capture_shared.h:15: class PPAPI_SHARED_EXPORT
PPB_VideoCapture_Shared {
On 2012/10/25 20:41:05, yzshen1 wrote:
> I think this class is not very useful now.
> All it does is checking the open_state_. (And SetStatus() is only used by the
> host side.)
> Does it make sense to remove this class and verify the open state at only one
> side?

The resource still uses open_state_ indirectly.  For example, in shared
StartCapture, it checks the status for both sides, and resource can return
failure immediately when happens.  We might still need the state machine on
resource side, since many of the API definitions say it will return error in
some state?

If we can't change this behavior, I can try to refactor this class.

[yzshen1, 2012-10-29 18:34:22]

A few more comments.

Haven't looked into the details of .*_host.* and .*_resource.* . Will do that
when we agree on how to deal with .*_shared.*.

https://chromiumcodereview.appspot.com/11274036/diff/4026/content/renderer/pe...
File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right):

https://chromiumcodereview.appspot.com/11274036/diff/4026/content/renderer/pe...
content/renderer/pepper/content_renderer_pepper_host_factory.cc:45:
PepperVideoCaptureHost* host = new PepperVideoCaptureHost(
This is not a stable interface. Add it to the dev section.

https://chromiumcodereview.appspot.com/11274036/diff/4026/content/renderer/pe...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

https://chromiumcodereview.appspot.com/11274036/diff/4026/content/renderer/pe...
content/renderer/pepper/pepper_in_process_resource_creation.cc:78: return 0;  //
VideoCapture is not supported in process now.
We add those non-supported ones to resource_creation_impl instead of here.

https://chromiumcodereview.appspot.com/11274036/diff/4026/ppapi/proxy/interfa...
File ppapi/proxy/interface_list.cc (right):

https://chromiumcodereview.appspot.com/11274036/diff/4026/ppapi/proxy/interfa...
ppapi/proxy/interface_list.cc:122: #include
"ppapi/proxy/video_capture_resource.h"
Do we need this include here?

https://chromiumcodereview.appspot.com/11274036/diff/4026/ppapi/proxy/ppapi_m...
File ppapi/proxy/ppapi_messages.h (right):

https://chromiumcodereview.appspot.com/11274036/diff/4026/ppapi/proxy/ppapi_m...
ppapi/proxy/ppapi_messages.h:1297: // VideoCapture_Dev, plugin -> host
- nit, optional: The src/dest of those messages are pretty clear
(Ppapi{Host|Plugin}Msg). There is no need to split them into three sub-sections.

- Please move these messages to the bottom of the file. All messages for
new-style host/proxy are there.

https://chromiumcodereview.appspot.com/11274036/diff/4026/ppapi/proxy/ppapi_m...
ppapi/proxy/ppapi_messages.h:1310:
IPC_MESSAGE_CONTROL2(PpapiPluginMsg_VideoCapture_EnumerateDevicesResponse,
Please use *Reply, in order to be consistent with other messages used by the
new-style host/proxy.

https://chromiumcodereview.appspot.com/11274036/diff/4026/ppapi/proxy/ppapi_m...
ppapi/proxy/ppapi_messages.h:1311: int32_t /* result */,
You don't need |result|. (Please see the reason below.)

https://chromiumcodereview.appspot.com/11274036/diff/4026/ppapi/proxy/ppapi_m...
ppapi/proxy/ppapi_messages.h:1317:
IPC_MESSAGE_CONTROL1(PpapiPluginMsg_VideoCapture_OpenACK,
Please use *Reply, in order to be consistent with other messages used by the
new-style host/proxy.

https://chromiumcodereview.appspot.com/11274036/diff/4026/ppapi/proxy/ppapi_m...
ppapi/proxy/ppapi_messages.h:1318: int32_t /* result */)
You don't need this. The enclosing message has already got a result field.
(Please look at the definition of PpapiPluginMsg_ResourceReply and corresponding
*Params.)

https://chromiumcodereview.appspot.com/11274036/diff/4026/ppapi/shared_impl/p...
File ppapi/shared_impl/ppb_video_capture_shared.h (right):

https://chromiumcodereview.appspot.com/11274036/diff/4026/ppapi/shared_impl/p...
ppapi/shared_impl/ppb_video_capture_shared.h:15: class PPAPI_SHARED_EXPORT
PPB_VideoCapture_Shared {
The reason why we needed the state machine on both sides was to maintain the
same behavior for in-process and out-of-process mode.
Now that we only have out-of-process mode (in-process mode fakes an IPC channel
and runs the same code), we can have this state machine only at the resource
side, right?

I think that would make the code cleaner.

(Of course, we need to make sure the host side should still work safely without
this state machine.)


On 2012/10/26 02:34:54, Victor Hsieh wrote:
> On 2012/10/25 20:41:05, yzshen1 wrote:
> > I think this class is not very useful now.
> > All it does is checking the open_state_. (And SetStatus() is only used by
the
> > host side.)
> > Does it make sense to remove this class and verify the open state at only
one
> > side?
> 
> The resource still uses open_state_ indirectly.  For example, in shared
> StartCapture, it checks the status for both sides, and resource can return
> failure immediately when happens.  We might still need the state machine on
> resource side, since many of the API definitions say it will return error in
> some state?
> 
> If we can't change this behavior, I can try to refactor this class.

[victorhsieh, 2012-10-30 09:43:28]

PTAL

https://chromiumcodereview.appspot.com/11274036/diff/4026/content/renderer/pe...
File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right):

https://chromiumcodereview.appspot.com/11274036/diff/4026/content/renderer/pe...
content/renderer/pepper/content_renderer_pepper_host_factory.cc:45:
PepperVideoCaptureHost* host = new PepperVideoCaptureHost(
On 2012/10/29 18:34:23, yzshen1 wrote:
> This is not a stable interface. Add it to the dev section.

Done.  BTW, there is TODO before the dev section mentioning a bug, which has
been closed.  Do you think it's safe to remove to TODO and restore the "if" in
this CL?

https://chromiumcodereview.appspot.com/11274036/diff/4026/content/renderer/pe...
File content/renderer/pepper/pepper_in_process_resource_creation.cc (right):

https://chromiumcodereview.appspot.com/11274036/diff/4026/content/renderer/pe...
content/renderer/pepper/pepper_in_process_resource_creation.cc:78: return 0;  //
VideoCapture is not supported in process now.
On 2012/10/29 18:34:23, yzshen1 wrote:
> We add those non-supported ones to resource_creation_impl instead of here.

Done.

https://chromiumcodereview.appspot.com/11274036/diff/4026/content/renderer/pe...
File content/renderer/pepper/pepper_video_capture_host.h (right):

https://chromiumcodereview.appspot.com/11274036/diff/4026/content/renderer/pe...
content/renderer/pepper/pepper_video_capture_host.h:80: virtual int32_t
InternalStartCapture0_1(
On 2012/10/25 20:41:05, yzshen1 wrote:
> We can probably remove the support of V0.1 entirely.
> You may want to send a mail to pepper-team to see whether it will break
> anything.

I'm removing all the v0.1 interface, including those in
ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c.  Hope it's fine.

https://chromiumcodereview.appspot.com/11274036/diff/4026/ppapi/proxy/interfa...
File ppapi/proxy/interface_list.cc (right):

https://chromiumcodereview.appspot.com/11274036/diff/4026/ppapi/proxy/interfa...
ppapi/proxy/interface_list.cc:122: #include
"ppapi/proxy/video_capture_resource.h"
On 2012/10/29 18:34:23, yzshen1 wrote:
> Do we need this include here?

Removed.

https://chromiumcodereview.appspot.com/11274036/diff/4026/ppapi/proxy/ppapi_m...
File ppapi/proxy/ppapi_messages.h (right):

https://chromiumcodereview.appspot.com/11274036/diff/4026/ppapi/proxy/ppapi_m...
ppapi/proxy/ppapi_messages.h:1297: // VideoCapture_Dev, plugin -> host
On 2012/10/29 18:34:23, yzshen1 wrote:
> - nit, optional: The src/dest of those messages are pretty clear
> (Ppapi{Host|Plugin}Msg). There is no need to split them into three
sub-sections.
> 
> - Please move these messages to the bottom of the file. All messages for
> new-style host/proxy are there.

Moved.  And I'd keep those plugin->host comments, which could bring readability
by providing visual clue.

https://chromiumcodereview.appspot.com/11274036/diff/4026/ppapi/proxy/ppapi_m...
ppapi/proxy/ppapi_messages.h:1310:
IPC_MESSAGE_CONTROL2(PpapiPluginMsg_VideoCapture_EnumerateDevicesResponse,
On 2012/10/29 18:34:23, yzshen1 wrote:
> Please use *Reply, in order to be consistent with other messages used by the
> new-style host/proxy.

Done.

https://chromiumcodereview.appspot.com/11274036/diff/4026/ppapi/proxy/ppapi_m...
ppapi/proxy/ppapi_messages.h:1311: int32_t /* result */,
On 2012/10/29 18:34:23, yzshen1 wrote:
> You don't need |result|. (Please see the reason below.)

Done.

https://chromiumcodereview.appspot.com/11274036/diff/4026/ppapi/proxy/ppapi_m...
ppapi/proxy/ppapi_messages.h:1317:
IPC_MESSAGE_CONTROL1(PpapiPluginMsg_VideoCapture_OpenACK,
On 2012/10/29 18:34:23, yzshen1 wrote:
> Please use *Reply, in order to be consistent with other messages used by the
> new-style host/proxy.

Done.

https://chromiumcodereview.appspot.com/11274036/diff/4026/ppapi/proxy/ppapi_m...
ppapi/proxy/ppapi_messages.h:1318: int32_t /* result */)
On 2012/10/29 18:34:23, yzshen1 wrote:
> You don't need this. The enclosing message has already got a result field.
> (Please look at the definition of PpapiPluginMsg_ResourceReply and
corresponding
> *Params.)

Done.

https://chromiumcodereview.appspot.com/11274036/diff/4026/ppapi/shared_impl/p...
File ppapi/shared_impl/ppb_video_capture_shared.h (right):

https://chromiumcodereview.appspot.com/11274036/diff/4026/ppapi/shared_impl/p...
ppapi/shared_impl/ppb_video_capture_shared.h:15: class PPAPI_SHARED_EXPORT
PPB_VideoCapture_Shared {
Done.  Removed everything of 0.1 from IDL, api, thunk, etc.

On 2012/10/29 18:34:23, yzshen1 wrote:
> The reason why we needed the state machine on both sides was to maintain the
> same behavior for in-process and out-of-process mode.
> Now that we only have out-of-process mode (in-process mode fakes an IPC
channel
> and runs the same code), we can have this state machine only at the resource
> side, right?
> 
> I think that would make the code cleaner.
> 
> (Of course, we need to make sure the host side should still work safely
without
> this state machine.)
> 
> 
> On 2012/10/26 02:34:54, Victor Hsieh wrote:
> > On 2012/10/25 20:41:05, yzshen1 wrote:
> > > I think this class is not very useful now.
> > > All it does is checking the open_state_. (And SetStatus() is only used by
> the
> > > host side.)
> > > Does it make sense to remove this class and verify the open state at only
> one
> > > side?
> > 
> > The resource still uses open_state_ indirectly.  For example, in shared
> > StartCapture, it checks the status for both sides, and resource can return
> > failure immediately when happens.  We might still need the state machine on
> > resource side, since many of the API definitions say it will return error in
> > some state?
> > 
> > If we can't change this behavior, I can try to refactor this class.
>

[brettw, 2012-10-31 23:50:26]

LGTM for a high-level review. I'll defer to Yuzhu for the rest.

http://codereview.chromium.org/11274036/diff/16001/ppapi/proxy/video_capture_...
File ppapi/proxy/video_capture_resource.cc (right):

http://codereview.chromium.org/11274036/diff/16001/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:72:
base::Bind(&VideoCaptureResource::OnEnumerateDevicesComplete, this));
Cool thing you can do here: Set "devices" and the callback as a bound parameter
to your function here and then you can delete your class member. See
printing_resource.cc for an example. Then you can have a bool for whether the
call is pending which I think is a little more clear than tracking the callback
pending state.

[yzshen1, 2012-11-01 18:03:07]

On 2012/10/29 18:34:23, yzshen1 wrote:
> The reason why we needed the state machine on both sides was to maintain the
> same behavior for in-process and out-of-process mode.
> Now that we only have out-of-process mode (in-process mode fakes an IPC
channel
> and runs the same code), we can have this state machine only at the resource
> side, right?
> 
> I think that would make the code cleaner.
> 
> (Of course, we need to make sure the host side should still work safely
without
> this state machine.)
> 
> 
> On 2012/10/26 02:34:54, Victor Hsieh wrote:
> > On 2012/10/25 20:41:05, yzshen1 wrote:
> > > I think this class is not very useful now.
> > > All it does is checking the open_state_. (And SetStatus() is only used by
> the
> > > host side.)
> > > Does it make sense to remove this class and verify the open state at only
> one
> > > side?
> > 
> > The resource still uses open_state_ indirectly.  For example, in shared
> > StartCapture, it checks the status for both sides, and resource can return
> > failure immediately when happens.  We might still need the state machine on
> > resource side, since many of the API definitions say it will return error in
> > some state?
> > 
> > If we can't change this behavior, I can try to refactor this class.

Hi, Victor.

What do you think about this comment?
Do you want me to continue reviewing the host/resource files? Or you would like
me to wait till you make changes?

Thanks!

[victorhsieh, 2012-11-02 04:28:47]

https://chromiumcodereview.appspot.com/11274036/diff/4026/ppapi/shared_impl/p...
File ppapi/shared_impl/ppb_video_capture_shared.h (right):

https://chromiumcodereview.appspot.com/11274036/diff/4026/ppapi/shared_impl/p...
ppapi/shared_impl/ppb_video_capture_shared.h:15: class PPAPI_SHARED_EXPORT
PPB_VideoCapture_Shared {
Oops, I guess I missed this comment.  The code is done.

So in the original code, there are 2 states: open_states_ and status_. 
open_states_ are moved to resource for opening state tracking.  But the host
should of course handle mis-state request properly.

status_ is moved to the host, since it's updated passively by SetStatus on
certain media events from browser, and the report to plugin resource.  Chance is
that we might event remove status_ for state transition check? But that's
another story.

Please see if it make sense.

On 2012/10/29 18:34:23, yzshen1 wrote:
> The reason why we needed the state machine on both sides was to maintain the
> same behavior for in-process and out-of-process mode.
> Now that we only have out-of-process mode (in-process mode fakes an IPC
channel
> and runs the same code), we can have this state machine only at the resource
> side, right?
> 
> I think that would make the code cleaner.
> 
> (Of course, we need to make sure the host side should still work safely
without
> this state machine.)
> 
> 
> On 2012/10/26 02:34:54, Victor Hsieh wrote:
> > On 2012/10/25 20:41:05, yzshen1 wrote:
> > > I think this class is not very useful now.
> > > All it does is checking the open_state_. (And SetStatus() is only used by
> the
> > > host side.)
> > > Does it make sense to remove this class and verify the open state at only
> one
> > > side?
> > 
> > The resource still uses open_state_ indirectly.  For example, in shared
> > StartCapture, it checks the status for both sides, and resource can return
> > failure immediately when happens.  We might still need the state machine on
> > resource side, since many of the API definitions say it will return error in
> > some state?
> > 
> > If we can't change this behavior, I can try to refactor this class.
>

https://chromiumcodereview.appspot.com/11274036/diff/16001/ppapi/proxy/video_...
File ppapi/proxy/video_capture_resource.cc (right):

https://chromiumcodereview.appspot.com/11274036/diff/16001/ppapi/proxy/video_...
ppapi/proxy/video_capture_resource.cc:72:
base::Bind(&VideoCaptureResource::OnEnumerateDevicesComplete, this));
On 2012/10/31 23:50:26, brettw wrote:
> Cool thing you can do here: Set "devices" and the callback as a bound
parameter
> to your function here and then you can delete your class member. See
> printing_resource.cc for an example. Then you can have a bool for whether the
> call is pending which I think is a little more clear than tracking the
callback
> pending state.

Done.  Thanks for the hint.  But I didn't apply this to Open since we still need
to hold a callback reference, so that Close can cancel it.

[yzshen1, 2012-11-06 06:51:40]

More comments.

I haven't reviewed *_host.*.

http://codereview.chromium.org/11274036/diff/4026/content/renderer/pepper/con...
File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right):

http://codereview.chromium.org/11274036/diff/4026/content/renderer/pepper/con...
content/renderer/pepper/content_renderer_pepper_host_factory.cc:45:
PepperVideoCaptureHost* host = new PepperVideoCaptureHost(
On 2012/10/30 09:43:28, Victor Hsieh wrote:
> On 2012/10/29 18:34:23, yzshen1 wrote:
> > This is not a stable interface. Add it to the dev section.
> 
> Done.  BTW, there is TODO before the dev section mentioning a bug, which has
> been closed.  Do you think it's safe to remove to TODO and restore the "if" in
> this CL?

I haven't looked into the relevant code. (Brett might know.)
But it is better to be in a different CL and link to the appropriate bug.

http://codereview.chromium.org/11274036/diff/25033/content/renderer/pepper/co...
File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right):

http://codereview.chromium.org/11274036/diff/25033/content/renderer/pepper/co...
content/renderer/pepper/content_renderer_pepper_host_factory.cc:59:
PepperVideoCaptureHost* host = new PepperVideoCaptureHost(
better to directly put it in a scoped_ptr so that we don't need to do manual
delete below.

Note that there is a scoped_ptr(scoped_ptr<U> other) constructor, so it doesn't
require any extra effort to convert to scoped_ptr<ResourceHost>.

http://codereview.chromium.org/11274036/diff/25033/ppapi/api/dev/ppb_video_ca...
File ppapi/api/dev/ppb_video_capture_dev.idl (right):

http://codereview.chromium.org/11274036/diff/25033/ppapi/api/dev/ppb_video_ca...
ppapi/api/dev/ppb_video_capture_dev.idl:68: [version=0.2]
There is no need to keep this line. (And same for some other methods below.)

http://codereview.chromium.org/11274036/diff/25033/ppapi/cpp/dev/video_captur...
File ppapi/cpp/dev/video_capture_dev.cc (left):

http://codereview.chromium.org/11274036/diff/25033/ppapi/cpp/dev/video_captur...
ppapi/cpp/dev/video_capture_dev.cc:84: requested_info_ = requested_info;
You need to remove those unnecessary class members.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/resource_messa...
File ppapi/proxy/resource_message_params.h (right):

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/resource_messa...
ppapi/proxy/resource_message_params.h:48: void GetAllSharedMemoryHandles(
Please see my recent change
http://src.chromium.org/viewvc/chrome?view=rev&revision=165799
and update accordingly.

Basically, you should use Take* instead of Get*.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
File ppapi/proxy/video_capture_resource.cc (right):

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:25: if (!sent_create_to_renderer())
You don't need to test this condition.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:101: 
I notice that the previous code will do a
SetStatus(PP_VIDEO_CAPTURE_STATUS_STARTING, false) and check the outcome here.
Is that not needed any more? Will it cause behavior change of the interface?

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:117: 
I notice that the previous code will do a
SetStatus(PP_VIDEO_CAPTURE_STATUS_STOPPING, false) and check the outcome here.
Is that not needed any more? Will it cause behavior change of the interface?

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:122: void VideoCaptureResource::Close() {
Please make sure the second and subsequent Close() calls are no-ops.
You should put "if (open_state_ == CLOSED) return;" at the beginning.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:141: DCHECK(handles.size() ==
buffers.size());
Please use CHECK().

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:145: resources[i] =
ppapi::proxy::PPB_Buffer_Proxy::AddProxyResource(
The resources are leaked!
You should release them after use.
Because according to the document, the plugin is expected to add its own
references to the buffers.
It is your responsibility to release the reference returned by
AddProxyResource().

Please be careful about references. Once we get it wrong, it is hard to find out
later.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:189: // completed synchronously.
the 'synchronously' part is not correct in this case, please remove it.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:199:
DCHECK(has_pending_enum_devices_callback_);
- you should set has_pending_enum_devices_callback_ to false in this method.

- you should test whether |callback| is pending. It may have been aborted, if
the last plugin reference to this resource has gone away. In that case, we must
not access |devices_output|. Because it may be invalid already.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:205: // in VideoCapture_Dev.
This is not accurate. Users can use c interface directly, it may not be from the
cpp wrapper.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
File ppapi/proxy/video_capture_resource.h (right):

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.h:12: #include "ppapi/shared_impl/api_id.h"
why do you need this?

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.h:18: class PPAPI_PROXY_EXPORT
VideoCaptureResource
I don't think it is necessary to be exported.
(And you could remove line 10 and the NON_EXPORTED_BASE below.)

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.h:22: VideoCaptureResource(Connection
connection, PP_Instance instance,
According to style guide, you should not put multiple parameters on the same
line, if they cannot all fit in the same line as the method name. (This is the
rule for declaration/definition, not for method call.)

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.h:24: ~VideoCaptureResource();
virtual

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.h:26: void OnReplyReceived(const
ResourceMessageReplyParams& params,
- virtual.
- add "<some_base_class> implementation." or "<some_base_class> overrides.",
depending on whether you implement an interface or override it.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.h:29: thunk::PPB_VideoCapture_API*
AsPPB_VideoCapture_API() OVERRIDE {
ditto.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.h:44: const std::vector<DeviceRefData>&
GetDeviceRefData() const OVERRIDE {
you don't need this method.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.h:66: void OnOpenComplete(const
ResourceMessageReplyParams& params);
nit: The convention is to use OnPluginMsg<the last part of the message name>.
So this one should be OnPluginMsgOpenReply.
(And same comment for other method names.)

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.h:74: // Points to the client implementation
of pp::VideoCaptureClient_Dev.
nit: inaccurate comment, this doesn't point to the cpp wrapper, it is a pointer
to the c interface.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.h:85: std::vector<DeviceRefData>
devices_data_;
you don't need this one.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.h:87: };
DISALLOW_COPY_AND_ASSIGN, please.

http://codereview.chromium.org/11274036/diff/25033/ppapi/thunk/ppb_video_capt...
File ppapi/thunk/ppb_video_capture_api.h (right):

http://codereview.chromium.org/11274036/diff/25033/ppapi/thunk/ppb_video_capt...
ppapi/thunk/ppb_video_capture_api.h:38: virtual const
std::vector<DeviceRefData>& GetDeviceRefData() const = 0;
I don't think you still need this.

[yzshen1, 2012-11-06 21:56:23]

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
File content/renderer/pepper/pepper_video_capture_host.cc (right):

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:7: #include
"content/public/renderer/renderer_ppapi_host.h"
you have included it in .h.

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:9: #include
"ppapi/host/host_message_context.h"
ditto.

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:15: #include
"ppapi/shared_impl/ppb_device_ref_shared.h"
ditto.

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:19: #include
"webkit/plugins/ppapi/ppb_buffer_impl.h"
ditto.

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:43: PP_Instance instance,
wrong indent.

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:45: :
ResourceHost(host->GetPpapiHost(), instance, resource),
wrong indent: four spaces.

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:46:
status_(PP_VIDEO_CAPTURE_STATUS_STOPPED) {
please init buffer_count_hint_.

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:59: const IPC::Message&
msg,
wrong indent.

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:197: HostResource
host_resource;
You have to add one more ref for the plugin side. The plugin side will take
ownership of that reference. It will be removed when the plugin side destroy the
plugin-side object and call PluginResourceTracker::RemoveResource().

Please be careful with references. It is hard to find out later. You might want
to read the implementation of resource_tracker/plugin_resource_tracker.

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:242: return
ResourceHelper::PPInstanceToPluginInstance(pp_instance());
I thought this wouldn't link. Maybe you are using a non-component build?
(I did it in a different way in PepperAudioInputHost::GetDelegate() in
http://codereview.chromium.org/11366038/diff/6002/content/renderer/pepper/pep...)

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:251: enum_reply_context_ =
context->MakeReplyMessageContext();
If we receive a second enumerate devices request before the first completes, we
will overwrite the context.

One possible way is to use scoped_ptr<> to store a context, and reject requests
if the pointer is not NULL. Or have a bool flag.

(I agree that if the plugin side works properly, it is not likely to receive
multiple requests in this way, though.)

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:284: return
PP_OK_COMPLETIONPENDING;
You don't need this line.

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:295:
DCHECK(platform_video_capture_.get());
If we want this code to handle unexpected requests in a robust way, we need to
return if platform_video_capture_ is NULL here. (The check on line 292 won't
help if we receive a start capture request before an open.)

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
File content/renderer/pepper/pepper_video_capture_host.h (right):

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.h:23: class CONTENT_EXPORT
PepperVideoCaptureHost
I don't think we need to export it. (and please remove line 10)

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.h:32:
~PepperVideoCaptureHost();
nit: please add 'virtual'.

[victorhsieh, 2012-11-08 09:20:18]

PTAL

Thanks for detail review!

http://codereview.chromium.org/11274036/diff/4026/content/renderer/pepper/con...
File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right):

http://codereview.chromium.org/11274036/diff/4026/content/renderer/pepper/con...
content/renderer/pepper/content_renderer_pepper_host_factory.cc:45:
PepperVideoCaptureHost* host = new PepperVideoCaptureHost(
I see.  Will leave it untouched here.

On 2012/11/06 06:51:40, yzshen1 wrote:
> On 2012/10/30 09:43:28, Victor Hsieh wrote:
> > On 2012/10/29 18:34:23, yzshen1 wrote:
> > > This is not a stable interface. Add it to the dev section.
> > 
> > Done.  BTW, there is TODO before the dev section mentioning a bug, which has
> > been closed.  Do you think it's safe to remove to TODO and restore the "if"
in
> > this CL?
> 
> I haven't looked into the relevant code. (Brett might know.)
> But it is better to be in a different CL and link to the appropriate bug.

http://codereview.chromium.org/11274036/diff/25033/content/renderer/pepper/co...
File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right):

http://codereview.chromium.org/11274036/diff/25033/content/renderer/pepper/co...
content/renderer/pepper/content_renderer_pepper_host_factory.cc:59:
PepperVideoCaptureHost* host = new PepperVideoCaptureHost(
On 2012/11/06 06:51:40, yzshen1 wrote:
> better to directly put it in a scoped_ptr so that we don't need to do manual
> delete below.
> 
> Note that there is a scoped_ptr(scoped_ptr<U> other) constructor, so it
doesn't
> require any extra effort to convert to scoped_ptr<ResourceHost>.

Turns out it's not trivial.  scoped_ptr is declared MOVE_ONLY_TYPE_FOR_CPP_03 at
the first line of the class.  I can't make it return from a scoped_ptr<MyHost>
type.  Do you have any idea?

http://codereview.chromium.org/11274036/diff/25033/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_video_capture_host.cc (right):

http://codereview.chromium.org/11274036/diff/25033/content/renderer/pepper/pe...
content/renderer/pepper/pepper_video_capture_host.cc:7: #include
"content/public/renderer/renderer_ppapi_host.h"
On 2012/11/06 21:56:23, yzshen1 wrote:
> you have included it in .h.

IWYU says we should include them.
http://code.google.com/p/include-what-you-use/

http://codereview.chromium.org/11274036/diff/25033/content/renderer/pepper/pe...
content/renderer/pepper/pepper_video_capture_host.cc:43: PP_Instance instance,
On 2012/11/06 21:56:23, yzshen1 wrote:
> wrong indent.

Done.

http://codereview.chromium.org/11274036/diff/25033/content/renderer/pepper/pe...
content/renderer/pepper/pepper_video_capture_host.cc:45: :
ResourceHost(host->GetPpapiHost(), instance, resource),
On 2012/11/06 21:56:23, yzshen1 wrote:
> wrong indent: four spaces.

Done.

http://codereview.chromium.org/11274036/diff/25033/content/renderer/pepper/pe...
content/renderer/pepper/pepper_video_capture_host.cc:46:
status_(PP_VIDEO_CAPTURE_STATUS_STOPPED) {
On 2012/11/06 21:56:23, yzshen1 wrote:
> please init buffer_count_hint_.

Done.

http://codereview.chromium.org/11274036/diff/25033/content/renderer/pepper/pe...
content/renderer/pepper/pepper_video_capture_host.cc:59: const IPC::Message&
msg,
On 2012/11/06 21:56:23, yzshen1 wrote:
> wrong indent.

Done.

http://codereview.chromium.org/11274036/diff/25033/content/renderer/pepper/pe...
content/renderer/pepper/pepper_video_capture_host.cc:197: HostResource
host_resource;
On 2012/11/06 21:56:23, yzshen1 wrote:
> You have to add one more ref for the plugin side. The plugin side will take
> ownership of that reference. It will be removed when the plugin side destroy
the
> plugin-side object and call PluginResourceTracker::RemoveResource().
> 
> Please be careful with references. It is hard to find out later. You might
want
> to read the implementation of resource_tracker/plugin_resource_tracker.

Done.  But how does that work when running out of process?

http://codereview.chromium.org/11274036/diff/25033/content/renderer/pepper/pe...
content/renderer/pepper/pepper_video_capture_host.cc:242: return
ResourceHelper::PPInstanceToPluginInstance(pp_instance());
On 2012/11/06 21:56:23, yzshen1 wrote:
> I thought this wouldn't link. Maybe you are using a non-component build?
> (I did it in a different way in PepperAudioInputHost::GetDelegate() in
>
http://codereview.chromium.org/11366038/diff/6002/content/renderer/pepper/pep...)

Actually I just learned about component build this week.  Yes, we need to add
resource_helper.cc to gyp to make this work, but I'll switch to your new code
until it is checked in.

In addition to this, we also need to add
webkit/plugin/ppapi/ppb_buffer_impl.{cc,h}  to gyp.  Do you have a better
suggestion than that?  Will that cause "multiple definition" error on compile?

http://codereview.chromium.org/11274036/diff/25033/content/renderer/pepper/pe...
content/renderer/pepper/pepper_video_capture_host.cc:251: enum_reply_context_ =
context->MakeReplyMessageContext();
On 2012/11/06 21:56:23, yzshen1 wrote:
> If we receive a second enumerate devices request before the first completes,
we
> will overwrite the context.
> 
> One possible way is to use scoped_ptr<> to store a context, and reject
requests
> if the pointer is not NULL. Or have a bool flag.
> 
> (I agree that if the plugin side works properly, it is not likely to receive
> multiple requests in this way, though.)

I don't see any risk here if we don't check?  enum_reply_context_ isn't a
pointer so no leaking will happen.  In the callback, it would send a message
with wrong sequence number.  That will only cause some error message.

http://codereview.chromium.org/11274036/diff/25033/content/renderer/pepper/pe...
content/renderer/pepper/pepper_video_capture_host.cc:284: return
PP_OK_COMPLETIONPENDING;
On 2012/11/06 21:56:23, yzshen1 wrote:
> You don't need this line.

Done.

http://codereview.chromium.org/11274036/diff/25033/content/renderer/pepper/pe...
content/renderer/pepper/pepper_video_capture_host.cc:295:
DCHECK(platform_video_capture_.get());
On 2012/11/06 21:56:23, yzshen1 wrote:
> If we want this code to handle unexpected requests in a robust way, we need to
> return if platform_video_capture_ is NULL here. (The check on line 292 won't
> help if we receive a start capture request before an open.)

Done.

http://codereview.chromium.org/11274036/diff/25033/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_video_capture_host.h (right):

http://codereview.chromium.org/11274036/diff/25033/content/renderer/pepper/pe...
content/renderer/pepper/pepper_video_capture_host.h:23: class CONTENT_EXPORT
PepperVideoCaptureHost
On 2012/11/06 21:56:23, yzshen1 wrote:
> I don't think we need to export it. (and please remove line 10)

Done.

http://codereview.chromium.org/11274036/diff/25033/content/renderer/pepper/pe...
content/renderer/pepper/pepper_video_capture_host.h:32:
~PepperVideoCaptureHost();
On 2012/11/06 21:56:23, yzshen1 wrote:
> nit: please add 'virtual'.

Done.

http://codereview.chromium.org/11274036/diff/25033/ppapi/api/dev/ppb_video_ca...
File ppapi/api/dev/ppb_video_capture_dev.idl (right):

http://codereview.chromium.org/11274036/diff/25033/ppapi/api/dev/ppb_video_ca...
ppapi/api/dev/ppb_video_capture_dev.idl:68: [version=0.2]
On 2012/11/06 06:51:40, yzshen1 wrote:
> There is no need to keep this line. (And same for some other methods below.)

Done.

http://codereview.chromium.org/11274036/diff/25033/ppapi/cpp/dev/video_captur...
File ppapi/cpp/dev/video_capture_dev.cc (left):

http://codereview.chromium.org/11274036/diff/25033/ppapi/cpp/dev/video_captur...
ppapi/cpp/dev/video_capture_dev.cc:84: requested_info_ = requested_info;
On 2012/11/06 06:51:40, yzshen1 wrote:
> You need to remove those unnecessary class members.

Done.  Thanks for catching.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/resource_messa...
File ppapi/proxy/resource_message_params.h (right):

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/resource_messa...
ppapi/proxy/resource_message_params.h:48: void GetAllSharedMemoryHandles(
On 2012/11/06 06:51:40, yzshen1 wrote:
> Please see my recent change
> http://src.chromium.org/viewvc/chrome?view=rev&revision=165799
> and update accordingly.
> 
> Basically, you should use Take* instead of Get*.

Done.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
File ppapi/proxy/video_capture_resource.cc (right):

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:25: if (!sent_create_to_renderer())
On 2012/11/06 06:51:40, yzshen1 wrote:
> You don't need to test this condition.

Done.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:101: 
On 2012/11/06 06:51:40, yzshen1 wrote:
> I notice that the previous code will do a
> SetStatus(PP_VIDEO_CAPTURE_STATUS_STARTING, false) and check the outcome here.
> Is that not needed any more? Will it cause behavior change of the interface? 

SetStatus checks the current state before jumping the the new.  The previous
state allowed of STARTING is STOPPED, which only happens either at the initial
state or any error is happened.

In that case, I add another line of code here in OnMsgOnError to reset the
state.  That should make behavior consistent to the old code.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:117: 
On 2012/11/06 06:51:40, yzshen1 wrote:
> I notice that the previous code will do a
> SetStatus(PP_VIDEO_CAPTURE_STATUS_STOPPING, false) and check the outcome here.
> Is that not needed any more? Will it cause behavior change of the interface? 

Similarly to STARTING, the preceding states that enter STOPPING are STARTING,
STARTED and PAUSED, and that's everything except STOPPED.  The old code returns
PP_ERROR_FAILED if already stopped.  Since status_ is removed, the "if" at the
beginning will cover that.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:122: void VideoCaptureResource::Close() {
On 2012/11/06 06:51:40, yzshen1 wrote:
> Please make sure the second and subsequent Close() calls are no-ops.
> You should put "if (open_state_ == CLOSED) return;" at the beginning.

Done.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:141: DCHECK(handles.size() ==
buffers.size());
On 2012/11/06 06:51:40, yzshen1 wrote:
> Please use CHECK().

Done.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:145: resources[i] =
ppapi::proxy::PPB_Buffer_Proxy::AddProxyResource(
On 2012/11/06 06:51:40, yzshen1 wrote:
> The resources are leaked!
> You should release them after use.
> Because according to the document, the plugin is expected to add its own
> references to the buffers.
> It is your responsibility to release the reference returned by
> AddProxyResource().
> 
> Please be careful about references. Once we get it wrong, it is hard to find
out
> later.
> 

Done.  Please advise if this is not correct.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:189: // completed synchronously.
On 2012/11/06 06:51:40, yzshen1 wrote:
> the 'synchronously' part is not correct in this case, please remove it.

Done.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:199:
DCHECK(has_pending_enum_devices_callback_);
On 2012/11/06 06:51:40, yzshen1 wrote:
> - you should set has_pending_enum_devices_callback_ to false in this method.
> 
> - you should test whether |callback| is pending. It may have been aborted, if
> the last plugin reference to this resource has gone away. In that case, we
must
> not access |devices_output|. Because it may be invalid already.

Done.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:205: // in VideoCapture_Dev.
On 2012/11/06 06:51:40, yzshen1 wrote:
> This is not accurate. Users can use c interface directly, it may not be from
the
> cpp wrapper.

Done.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
File ppapi/proxy/video_capture_resource.h (right):

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.h:12: #include "ppapi/shared_impl/api_id.h"
On 2012/11/06 06:51:40, yzshen1 wrote:
> why do you need this?

Done.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.h:18: class PPAPI_PROXY_EXPORT
VideoCaptureResource
On 2012/11/06 06:51:40, yzshen1 wrote:
> I don't think it is necessary to be exported.
> (And you could remove line 10 and the NON_EXPORTED_BASE below.)

Done.  It runs well, but I saw many resource headers have it?

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.h:22: VideoCaptureResource(Connection
connection, PP_Instance instance,
On 2012/11/06 06:51:40, yzshen1 wrote:
> According to style guide, you should not put multiple parameters on the same
> line, if they cannot all fit in the same line as the method name. (This is the
> rule for declaration/definition, not for method call.)

Done.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.h:24: ~VideoCaptureResource();
On 2012/11/06 06:51:40, yzshen1 wrote:
> virtual

Done.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.h:26: void OnReplyReceived(const
ResourceMessageReplyParams& params,
On 2012/11/06 06:51:40, yzshen1 wrote:
> - virtual.
> - add "<some_base_class> implementation." or "<some_base_class> overrides.",
> depending on whether you implement an interface or override it. 

Done and moved to private.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.h:29: thunk::PPB_VideoCapture_API*
AsPPB_VideoCapture_API() OVERRIDE {
On 2012/11/06 06:51:40, yzshen1 wrote:
> ditto.

Done.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.h:44: const std::vector<DeviceRefData>&
GetDeviceRefData() const OVERRIDE {
On 2012/11/06 06:51:40, yzshen1 wrote:
> you don't need this method.

It's still referenced by PepperFlashHost.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.h:66: void OnOpenComplete(const
ResourceMessageReplyParams& params);
On 2012/11/06 06:51:40, yzshen1 wrote:
> nit: The convention is to use OnPluginMsg<the last part of the message name>.
> So this one should be OnPluginMsgOpenReply.
> (And same comment for other method names.)

Done.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.h:74: // Points to the client implementation
of pp::VideoCaptureClient_Dev.
On 2012/11/06 06:51:40, yzshen1 wrote:
> nit: inaccurate comment, this doesn't point to the cpp wrapper, it is a
pointer
> to the c interface.

Done.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.h:85: std::vector<DeviceRefData>
devices_data_;
On 2012/11/06 06:51:40, yzshen1 wrote:
> you don't need this one.

ditto, still referenced.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.h:87: };
On 2012/11/06 06:51:40, yzshen1 wrote:
> DISALLOW_COPY_AND_ASSIGN, please.

Done.

http://codereview.chromium.org/11274036/diff/25033/ppapi/thunk/ppb_video_capt...
File ppapi/thunk/ppb_video_capture_api.h (right):

http://codereview.chromium.org/11274036/diff/25033/ppapi/thunk/ppb_video_capt...
ppapi/thunk/ppb_video_capture_api.h:38: virtual const
std::vector<DeviceRefData>& GetDeviceRefData() const = 0;
On 2012/11/06 06:51:40, yzshen1 wrote:
> I don't think you still need this.

Done.

[yzshen1, 2012-11-10 01:14:40]

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right):

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/content_renderer_pepper_host_factory.cc:59:
PepperVideoCaptureHost* host = new PepperVideoCaptureHost(
Ah. Didn't know about that. Don't bother please. :)

On 2012/11/08 09:20:18, Victor Hsieh wrote:
> On 2012/11/06 06:51:40, yzshen1 wrote:
> > better to directly put it in a scoped_ptr so that we don't need to do manual
> > delete below.
> > 
> > Note that there is a scoped_ptr(scoped_ptr<U> other) constructor, so it
> doesn't
> > require any extra effort to convert to scoped_ptr<ResourceHost>.
> 
> Turns out it's not trivial.  scoped_ptr is declared MOVE_ONLY_TYPE_FOR_CPP_03
at
> the first line of the class.  I can't make it return from a scoped_ptr<MyHost>
> type.  Do you have any idea?

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
File content/renderer/pepper/pepper_video_capture_host.cc (right):

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:7: #include
"content/public/renderer/renderer_ppapi_host.h"
I think that is not true:
--------------------
"Include what you use" means this: for every symbol (type, function variable, or
macro) that you use in foo.cc, ****either foo.cc or foo.h**** should #include a
.h file that exports the declaration of that symbol.
--------------------

Whatever you have included in foo.h file shouldn't be included again in foo.cc.


On 2012/11/08 09:20:18, Victor Hsieh wrote:
> On 2012/11/06 21:56:23, yzshen1 wrote:
> > you have included it in .h.
> 
> IWYU says we should include them.
> http://code.google.com/p/include-what-you-use/

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:197: HostResource
host_resource;
What I said above is for out-of-process. And please read those files. :)

In short, we will send back a message when the plugin side no longer holds this
ref. See PpapiHostMsg_PPBCore_ReleaseResource.

On 2012/11/08 09:20:18, Victor Hsieh wrote:
> On 2012/11/06 21:56:23, yzshen1 wrote:
> > You have to add one more ref for the plugin side. The plugin side will take
> > ownership of that reference. It will be removed when the plugin side destroy
> the
> > plugin-side object and call PluginResourceTracker::RemoveResource().
> > 
> > Please be careful with references. It is hard to find out later. You might
> want
> > to read the implementation of resource_tracker/plugin_resource_tracker.
> 
> Done.  But how does that work when running out of process?

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:242: return
ResourceHelper::PPInstanceToPluginInstance(pp_instance());
On 2012/11/08 09:20:18, Victor Hsieh wrote:
> On 2012/11/06 21:56:23, yzshen1 wrote:
> > I thought this wouldn't link. Maybe you are using a non-component build?
> > (I did it in a different way in PepperAudioInputHost::GetDelegate() in
> >
>
http://codereview.chromium.org/11366038/diff/6002/content/renderer/pepper/pep...)
> 
> Actually I just learned about component build this week.  Yes, we need to add
> resource_helper.cc to gyp to make this work, but I'll switch to your new code
> until it is checked in.
It has been checked in.
> 
> In addition to this, we also need to add
> webkit/plugin/ppapi/ppb_buffer_impl.{cc,h}  to gyp.  Do you have a better
> suggestion than that?  Will that cause "multiple definition" error on compile?
export them?

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:251: enum_reply_context_ =
context->MakeReplyMessageContext();
Okay.
On 2012/11/08 09:20:18, Victor Hsieh wrote:
> On 2012/11/06 21:56:23, yzshen1 wrote:
> > If we receive a second enumerate devices request before the first completes,
> we
> > will overwrite the context.
> > 
> > One possible way is to use scoped_ptr<> to store a context, and reject
> requests
> > if the pointer is not NULL. Or have a bool flag.
> > 
> > (I agree that if the plugin side works properly, it is not likely to receive
> > multiple requests in this way, though.)
> 
> I don't see any risk here if we don't check?  enum_reply_context_ isn't a
> pointer so no leaking will happen.  In the callback, it would send a message
> with wrong sequence number.  That will only cause some error message.

https://chromiumcodereview.appspot.com/11274036/diff/25033/ppapi/proxy/video_...
File ppapi/proxy/video_capture_resource.cc (right):

https://chromiumcodereview.appspot.com/11274036/diff/25033/ppapi/proxy/video_...
ppapi/proxy/video_capture_resource.cc:101: 
What is the reason to keep the status_ state machine at the host side instead of
the plugin side?

On 2012/11/08 09:20:18, Victor Hsieh wrote:
> On 2012/11/06 06:51:40, yzshen1 wrote:
> > I notice that the previous code will do a
> > SetStatus(PP_VIDEO_CAPTURE_STATUS_STARTING, false) and check the outcome
here.
> > Is that not needed any more? Will it cause behavior change of the interface?

> 
> SetStatus checks the current state before jumping the the new.  The previous
> state allowed of STARTING is STOPPED, which only happens either at the initial
> state or any error is happened.
> 
> In that case, I add another line of code here in OnMsgOnError to reset the
> state.  That should make behavior consistent to the old code.

https://chromiumcodereview.appspot.com/11274036/diff/25033/ppapi/proxy/video_...
File ppapi/proxy/video_capture_resource.h (right):

https://chromiumcodereview.appspot.com/11274036/diff/25033/ppapi/proxy/video_...
ppapi/proxy/video_capture_resource.h:44: const std::vector<DeviceRefData>&
GetDeviceRefData() const OVERRIDE {
You have to also change PepperFlashHost. It will break with the current code
because we don't have a real resource at the host side anymore. Please talk to
Raymes.

On 2012/11/08 09:20:18, Victor Hsieh wrote:
> On 2012/11/06 06:51:40, yzshen1 wrote:
> > you don't need this method.
> 
> It's still referenced by PepperFlashHost.

https://chromiumcodereview.appspot.com/11274036/diff/25033/ppapi/proxy/video_...
ppapi/proxy/video_capture_resource.h:85: std::vector<DeviceRefData>
devices_data_;
ditto.
On 2012/11/08 09:20:18, Victor Hsieh wrote:
> On 2012/11/06 06:51:40, yzshen1 wrote:
> > you don't need this one.
> 
> ditto, still referenced.

https://chromiumcodereview.appspot.com/11274036/diff/25033/ppapi/thunk/ppb_vi...
File ppapi/thunk/ppb_video_capture_api.h (right):

https://chromiumcodereview.appspot.com/11274036/diff/25033/ppapi/thunk/ppb_vi...
ppapi/thunk/ppb_video_capture_api.h:38: virtual const
std::vector<DeviceRefData>& GetDeviceRefData() const = 0;
As I said in the other file, we can remove it because PepperFlashHost should be
changed as well.
On 2012/11/08 09:20:18, Victor Hsieh wrote:
> On 2012/11/06 06:51:40, yzshen1 wrote:
> > I don't think you still need this.
> 
> Done.

https://chromiumcodereview.appspot.com/11274036/diff/22004/content/content_re...
File content/content_renderer.gypi (right):

https://chromiumcodereview.appspot.com/11274036/diff/22004/content/content_re...
content/content_renderer.gypi:275: '../webkit/plugins/ppapi/ppb_buffer_impl.cc',
I don't think it is okay to add these files here.

Does it make sense to export them?

https://chromiumcodereview.appspot.com/11274036/diff/22004/ppapi/proxy/video_...
File ppapi/proxy/video_capture_resource.cc (right):

https://chromiumcodereview.appspot.com/11274036/diff/22004/ppapi/proxy/video_...
ppapi/proxy/video_capture_resource.cc:136:
VideoCaptureResource::GetDeviceRefData() const {
Why do you need this method?

https://chromiumcodereview.appspot.com/11274036/diff/22004/ppapi/proxy/video_...
ppapi/proxy/video_capture_resource.cc:164:
ppp_video_capture_impl_->OnDeviceInfo(
I think we need CallWhileUnlocked() for all the calls into the PPP interface.
(There are several places in this file that need to be changed.)

https://chromiumcodereview.appspot.com/11274036/diff/22004/ppapi/proxy/video_...
File ppapi/proxy/video_capture_resource.h (right):

https://chromiumcodereview.appspot.com/11274036/diff/22004/ppapi/proxy/video_...
ppapi/proxy/video_capture_resource.h:26: thunk::PPB_VideoCapture_API*
AsPPB_VideoCapture_API() OVERRIDE {
virtual, please

[victorhsieh, 2012-11-13 03:10:53]

PTAL

Raymes, please help to review flash* files.  I didn't remove flash_host but let
me know if it makes sense to delete it.

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
File content/renderer/pepper/pepper_video_capture_host.cc (right):

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:7: #include
"content/public/renderer/renderer_ppapi_host.h"
On 2012/11/10 01:14:40, yzshen1 wrote:
> I think that is not true:
> --------------------
> "Include what you use" means this: for every symbol (type, function variable,
or
> macro) that you use in foo.cc, ****either foo.cc or foo.h**** should #include
a
> .h file that exports the declaration of that symbol.
> --------------------
> 
> Whatever you have included in foo.h file shouldn't be included again in
foo.cc.
> 
> 
> On 2012/11/08 09:20:18, Victor Hsieh wrote:
> > On 2012/11/06 21:56:23, yzshen1 wrote:
> > > you have included it in .h.
> > 
> > IWYU says we should include them.
> > http://code.google.com/p/include-what-you-use/
> 

I see.  Done.

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:9: #include
"ppapi/host/host_message_context.h"
On 2012/11/06 21:56:23, yzshen1 wrote:
> ditto.

Done.

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:15: #include
"ppapi/shared_impl/ppb_device_ref_shared.h"
On 2012/11/06 21:56:23, yzshen1 wrote:
> ditto.

Done.

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:19: #include
"webkit/plugins/ppapi/ppb_buffer_impl.h"
On 2012/11/06 21:56:23, yzshen1 wrote:
> ditto.

Done.

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:197: HostResource
host_resource;
On 2012/11/10 01:14:40, yzshen1 wrote:
> What I said above is for out-of-process. And please read those files. :)
> 
> In short, we will send back a message when the plugin side no longer holds
this
> ref. See PpapiHostMsg_PPBCore_ReleaseResource.
> 
> On 2012/11/08 09:20:18, Victor Hsieh wrote:
> > On 2012/11/06 21:56:23, yzshen1 wrote:
> > > You have to add one more ref for the plugin side. The plugin side will
take
> > > ownership of that reference. It will be removed when the plugin side
destroy
> > the
> > > plugin-side object and call PluginResourceTracker::RemoveResource().
> > > 
> > > Please be careful with references. It is hard to find out later. You might
> > want
> > > to read the implementation of resource_tracker/plugin_resource_tracker.
> > 
> > Done.  But how does that work when running out of process?
> 

Done.  I was thinking in different way and got confused.  Also, I now changed
PpapiGlobals to HostGlobals.  Both works in this case, but HostGlobals makes
more sense here.  Correct me if I'm wrong.

https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:242: return
ResourceHelper::PPInstanceToPluginInstance(pp_instance());
On 2012/11/10 01:14:40, yzshen1 wrote:
> On 2012/11/08 09:20:18, Victor Hsieh wrote:
> > On 2012/11/06 21:56:23, yzshen1 wrote:
> > > I thought this wouldn't link. Maybe you are using a non-component build?
> > > (I did it in a different way in PepperAudioInputHost::GetDelegate() in
> > >
> >
>
http://codereview.chromium.org/11366038/diff/6002/content/renderer/pepper/pep...)
> > 
> > Actually I just learned about component build this week.  Yes, we need to
add
> > resource_helper.cc to gyp to make this work, but I'll switch to your new
code
> > until it is checked in.
> It has been checked in.
> > 
> > In addition to this, we also need to add
> > webkit/plugin/ppapi/ppb_buffer_impl.{cc,h}  to gyp.  Do you have a better
> > suggestion than that?  Will that cause "multiple definition" error on
compile?
> export them?

Done.

https://chromiumcodereview.appspot.com/11274036/diff/25033/ppapi/proxy/video_...
File ppapi/proxy/video_capture_resource.cc (right):

https://chromiumcodereview.appspot.com/11274036/diff/25033/ppapi/proxy/video_...
ppapi/proxy/video_capture_resource.cc:101: 
On 2012/11/10 01:14:40, yzshen1 wrote:
> What is the reason to keep the status_ state machine at the host side instead
of
> the plugin side?
> 
> On 2012/11/08 09:20:18, Victor Hsieh wrote:
> > On 2012/11/06 06:51:40, yzshen1 wrote:
> > > I notice that the previous code will do a
> > > SetStatus(PP_VIDEO_CAPTURE_STATUS_STARTING, false) and check the outcome
> here.
> > > Is that not needed any more? Will it cause behavior change of the
interface?
> 
> > 
> > SetStatus checks the current state before jumping the the new.  The previous
> > state allowed of STARTING is STOPPED, which only happens either at the
initial
> > state or any error is happened.
> > 
> > In that case, I add another line of code here in OnMsgOnError to reset the
> > state.  That should make behavior consistent to the old code.
> 

The idea to keep status_ on the host when I first did this, is to keep 
OnStarted/OnStopped/etc. still sending status to the plugin only in expected
state.

https://chromiumcodereview.appspot.com/11274036/diff/25033/ppapi/proxy/video_...
File ppapi/proxy/video_capture_resource.h (right):

https://chromiumcodereview.appspot.com/11274036/diff/25033/ppapi/proxy/video_...
ppapi/proxy/video_capture_resource.h:44: const std::vector<DeviceRefData>&
GetDeviceRefData() const OVERRIDE {
On 2012/11/10 01:14:40, yzshen1 wrote:
> You have to also change PepperFlashHost. It will break with the current code
> because we don't have a real resource at the host side anymore. Please talk to
> Raymes.
> 
> On 2012/11/08 09:20:18, Victor Hsieh wrote:
> > On 2012/11/06 06:51:40, yzshen1 wrote:
> > > you don't need this method.
> > 
> > It's still referenced by PepperFlashHost.
> 

Done.

https://chromiumcodereview.appspot.com/11274036/diff/25033/ppapi/proxy/video_...
ppapi/proxy/video_capture_resource.h:85: std::vector<DeviceRefData>
devices_data_;
On 2012/11/10 01:14:40, yzshen1 wrote:
> ditto.
> On 2012/11/08 09:20:18, Victor Hsieh wrote:
> > On 2012/11/06 06:51:40, yzshen1 wrote:
> > > you don't need this one.
> > 
> > ditto, still referenced.
> 

Done.

https://chromiumcodereview.appspot.com/11274036/diff/22004/content/content_re...
File content/content_renderer.gypi (right):

https://chromiumcodereview.appspot.com/11274036/diff/22004/content/content_re...
content/content_renderer.gypi:275: '../webkit/plugins/ppapi/ppb_buffer_impl.cc',
On 2012/11/10 01:14:40, yzshen1 wrote:
> I don't think it is okay to add these files here.
> 
> Does it make sense to export them?

Removed with export.

https://chromiumcodereview.appspot.com/11274036/diff/22004/ppapi/proxy/video_...
File ppapi/proxy/video_capture_resource.cc (right):

https://chromiumcodereview.appspot.com/11274036/diff/22004/ppapi/proxy/video_...
ppapi/proxy/video_capture_resource.cc:136:
VideoCaptureResource::GetDeviceRefData() const {
On 2012/11/10 01:14:40, yzshen1 wrote:
> Why do you need this method?

Removed.

https://chromiumcodereview.appspot.com/11274036/diff/22004/ppapi/proxy/video_...
ppapi/proxy/video_capture_resource.cc:164:
ppp_video_capture_impl_->OnDeviceInfo(
On 2012/11/10 01:14:40, yzshen1 wrote:
> I think we need CallWhileUnlocked() for all the calls into the PPP interface.
> (There are several places in this file that need to be changed.)

Done.  I have no idea about when we should lock/unlock in pepper.  Would you
mind to briefly explain a bit?

https://chromiumcodereview.appspot.com/11274036/diff/22004/ppapi/proxy/video_...
File ppapi/proxy/video_capture_resource.h (right):

https://chromiumcodereview.appspot.com/11274036/diff/22004/ppapi/proxy/video_...
ppapi/proxy/video_capture_resource.h:26: thunk::PPB_VideoCapture_API*
AsPPB_VideoCapture_API() OVERRIDE {
On 2012/11/10 01:14:40, yzshen1 wrote:
> virtual, please 

Oops, done.

[raymes, 2012-11-13 15:53:45]

The *flash* files look good. Please also test the enumerate devices ppapi
example and make sure it works with this CL.

Thanks.

On 2012/11/13 03:10:53, Victor Hsieh wrote:
> PTAL
> 
> Raymes, please help to review flash* files.  I didn't remove flash_host but
let
> me know if it makes sense to delete it.
> 
>
https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
> File content/renderer/pepper/pepper_video_capture_host.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
> content/renderer/pepper/pepper_video_capture_host.cc:7: #include
> "content/public/renderer/renderer_ppapi_host.h"
> On 2012/11/10 01:14:40, yzshen1 wrote:
> > I think that is not true:
> > --------------------
> > "Include what you use" means this: for every symbol (type, function
variable,
> or
> > macro) that you use in foo.cc, ****either foo.cc or foo.h**** should
#include
> a
> > .h file that exports the declaration of that symbol.
> > --------------------
> > 
> > Whatever you have included in foo.h file shouldn't be included again in
> foo.cc.
> > 
> > 
> > On 2012/11/08 09:20:18, Victor Hsieh wrote:
> > > On 2012/11/06 21:56:23, yzshen1 wrote:
> > > > you have included it in .h.
> > > 
> > > IWYU says we should include them.
> > > http://code.google.com/p/include-what-you-use/
> > 
> 
> I see.  Done.
> 
>
https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
> content/renderer/pepper/pepper_video_capture_host.cc:9: #include
> "ppapi/host/host_message_context.h"
> On 2012/11/06 21:56:23, yzshen1 wrote:
> > ditto.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
> content/renderer/pepper/pepper_video_capture_host.cc:15: #include
> "ppapi/shared_impl/ppb_device_ref_shared.h"
> On 2012/11/06 21:56:23, yzshen1 wrote:
> > ditto.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
> content/renderer/pepper/pepper_video_capture_host.cc:19: #include
> "webkit/plugins/ppapi/ppb_buffer_impl.h"
> On 2012/11/06 21:56:23, yzshen1 wrote:
> > ditto.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
> content/renderer/pepper/pepper_video_capture_host.cc:197: HostResource
> host_resource;
> On 2012/11/10 01:14:40, yzshen1 wrote:
> > What I said above is for out-of-process. And please read those files. :)
> > 
> > In short, we will send back a message when the plugin side no longer holds
> this
> > ref. See PpapiHostMsg_PPBCore_ReleaseResource.
> > 
> > On 2012/11/08 09:20:18, Victor Hsieh wrote:
> > > On 2012/11/06 21:56:23, yzshen1 wrote:
> > > > You have to add one more ref for the plugin side. The plugin side will
> take
> > > > ownership of that reference. It will be removed when the plugin side
> destroy
> > > the
> > > > plugin-side object and call PluginResourceTracker::RemoveResource().
> > > > 
> > > > Please be careful with references. It is hard to find out later. You
might
> > > want
> > > > to read the implementation of resource_tracker/plugin_resource_tracker.
> > > 
> > > Done.  But how does that work when running out of process?
> > 
> 
> Done.  I was thinking in different way and got confused.  Also, I now changed
> PpapiGlobals to HostGlobals.  Both works in this case, but HostGlobals makes
> more sense here.  Correct me if I'm wrong.
> 
>
https://chromiumcodereview.appspot.com/11274036/diff/25033/content/renderer/p...
> content/renderer/pepper/pepper_video_capture_host.cc:242: return
> ResourceHelper::PPInstanceToPluginInstance(pp_instance());
> On 2012/11/10 01:14:40, yzshen1 wrote:
> > On 2012/11/08 09:20:18, Victor Hsieh wrote:
> > > On 2012/11/06 21:56:23, yzshen1 wrote:
> > > > I thought this wouldn't link. Maybe you are using a non-component build?
> > > > (I did it in a different way in PepperAudioInputHost::GetDelegate() in
> > > >
> > >
> >
>
http://codereview.chromium.org/11366038/diff/6002/content/renderer/pepper/pep...)
> > > 
> > > Actually I just learned about component build this week.  Yes, we need to
> add
> > > resource_helper.cc to gyp to make this work, but I'll switch to your new
> code
> > > until it is checked in.
> > It has been checked in.
> > > 
> > > In addition to this, we also need to add
> > > webkit/plugin/ppapi/ppb_buffer_impl.{cc,h}  to gyp.  Do you have a better
> > > suggestion than that?  Will that cause "multiple definition" error on
> compile?
> > export them?
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/11274036/diff/25033/ppapi/proxy/video_...
> File ppapi/proxy/video_capture_resource.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11274036/diff/25033/ppapi/proxy/video_...
> ppapi/proxy/video_capture_resource.cc:101: 
> On 2012/11/10 01:14:40, yzshen1 wrote:
> > What is the reason to keep the status_ state machine at the host side
instead
> of
> > the plugin side?
> > 
> > On 2012/11/08 09:20:18, Victor Hsieh wrote:
> > > On 2012/11/06 06:51:40, yzshen1 wrote:
> > > > I notice that the previous code will do a
> > > > SetStatus(PP_VIDEO_CAPTURE_STATUS_STARTING, false) and check the outcome
> > here.
> > > > Is that not needed any more? Will it cause behavior change of the
> interface?
> > 
> > > 
> > > SetStatus checks the current state before jumping the the new.  The
previous
> > > state allowed of STARTING is STOPPED, which only happens either at the
> initial
> > > state or any error is happened.
> > > 
> > > In that case, I add another line of code here in OnMsgOnError to reset the
> > > state.  That should make behavior consistent to the old code.
> > 
> 
> The idea to keep status_ on the host when I first did this, is to keep 
> OnStarted/OnStopped/etc. still sending status to the plugin only in expected
> state.
> 
>
https://chromiumcodereview.appspot.com/11274036/diff/25033/ppapi/proxy/video_...
> File ppapi/proxy/video_capture_resource.h (right):
> 
>
https://chromiumcodereview.appspot.com/11274036/diff/25033/ppapi/proxy/video_...
> ppapi/proxy/video_capture_resource.h:44: const std::vector<DeviceRefData>&
> GetDeviceRefData() const OVERRIDE {
> On 2012/11/10 01:14:40, yzshen1 wrote:
> > You have to also change PepperFlashHost. It will break with the current code
> > because we don't have a real resource at the host side anymore. Please talk
to
> > Raymes.
> > 
> > On 2012/11/08 09:20:18, Victor Hsieh wrote:
> > > On 2012/11/06 06:51:40, yzshen1 wrote:
> > > > you don't need this method.
> > > 
> > > It's still referenced by PepperFlashHost.
> > 
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/11274036/diff/25033/ppapi/proxy/video_...
> ppapi/proxy/video_capture_resource.h:85: std::vector<DeviceRefData>
> devices_data_;
> On 2012/11/10 01:14:40, yzshen1 wrote:
> > ditto.
> > On 2012/11/08 09:20:18, Victor Hsieh wrote:
> > > On 2012/11/06 06:51:40, yzshen1 wrote:
> > > > you don't need this one.
> > > 
> > > ditto, still referenced.
> > 
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/11274036/diff/22004/content/content_re...
> File content/content_renderer.gypi (right):
> 
>
https://chromiumcodereview.appspot.com/11274036/diff/22004/content/content_re...
> content/content_renderer.gypi:275:
'../webkit/plugins/ppapi/ppb_buffer_impl.cc',
> On 2012/11/10 01:14:40, yzshen1 wrote:
> > I don't think it is okay to add these files here.
> > 
> > Does it make sense to export them?
> 
> Removed with export.
> 
>
https://chromiumcodereview.appspot.com/11274036/diff/22004/ppapi/proxy/video_...
> File ppapi/proxy/video_capture_resource.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11274036/diff/22004/ppapi/proxy/video_...
> ppapi/proxy/video_capture_resource.cc:136:
> VideoCaptureResource::GetDeviceRefData() const {
> On 2012/11/10 01:14:40, yzshen1 wrote:
> > Why do you need this method?
> 
> Removed.
> 
>
https://chromiumcodereview.appspot.com/11274036/diff/22004/ppapi/proxy/video_...
> ppapi/proxy/video_capture_resource.cc:164:
> ppp_video_capture_impl_->OnDeviceInfo(
> On 2012/11/10 01:14:40, yzshen1 wrote:
> > I think we need CallWhileUnlocked() for all the calls into the PPP
interface.
> > (There are several places in this file that need to be changed.)
> 
> Done.  I have no idea about when we should lock/unlock in pepper.  Would you
> mind to briefly explain a bit?
> 
>
https://chromiumcodereview.appspot.com/11274036/diff/22004/ppapi/proxy/video_...
> File ppapi/proxy/video_capture_resource.h (right):
> 
>
https://chromiumcodereview.appspot.com/11274036/diff/22004/ppapi/proxy/video_...
> ppapi/proxy/video_capture_resource.h:26: thunk::PPB_VideoCapture_API*
> AsPPB_VideoCapture_API() OVERRIDE {
> On 2012/11/10 01:14:40, yzshen1 wrote:
> > virtual, please 
> 
> Oops, done.

[raymes, 2012-11-13 15:54:15]

http://codereview.chromium.org/11274036/diff/27043/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_flash_host.cc (left):

http://codereview.chromium.org/11274036/diff/27043/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_host.cc:36: int32_t
PepperFlashHost::OnResourceMessageReceived(
Please leave this here and just return PP_ERROR_FAILED.

http://codereview.chromium.org/11274036/diff/27043/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_flash_host.h (left):

http://codereview.chromium.org/11274036/diff/27043/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_host.h:26: virtual int32_t
OnResourceMessageReceived(
Please leave this function here.

[victorhsieh, 2012-11-14 01:07:50]

Done.  And add jln@ for ppapi_messages.h review.

http://codereview.chromium.org/11274036/diff/27043/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_flash_host.cc (left):

http://codereview.chromium.org/11274036/diff/27043/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_host.cc:36: int32_t
PepperFlashHost::OnResourceMessageReceived(
On 2012/11/13 15:54:16, raymes wrote:
> Please leave this here and just return PP_ERROR_FAILED.

Done.

http://codereview.chromium.org/11274036/diff/27043/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_flash_host.h (left):

http://codereview.chromium.org/11274036/diff/27043/content/renderer/pepper/pe...
content/renderer/pepper/pepper_flash_host.h:26: virtual int32_t
OnResourceMessageReceived(
On 2012/11/13 15:54:16, raymes wrote:
> Please leave this function here.

Done.

[jln (very slow on Chromium), 2012-11-14 01:25:03]

On 2012/11/14 01:07:50, Victor Hsieh wrote:
> Done.  And add jln@ for ppapi_messages.h review.

The commit message is not very descriptive, and there is no bug linked. Is there
any information somewhere?

At a glance, it looks like a number of existing IPCs are being renamed / moved.
Is there any way you could help me assess the new attack surface created?

(Also, if this new design already, or the old one, got a security review, please
include the people involved in it for the IPC review).

[victorhsieh, 2012-11-14 01:40:57]

On 2012/11/14 01:25:03, Julien Tinnes wrote:
> On 2012/11/14 01:07:50, Victor Hsieh wrote:
> > Done.  And add jln@ for ppapi_messages.h review.
> 
> The commit message is not very descriptive, and there is no bug linked. Is
there
> any information somewhere?
> 
> At a glance, it looks like a number of existing IPCs are being renamed /
moved.
> Is there any way you could help me assess the new attack surface created?
> 
> (Also, if this new design already, or the old one, got a security review,
please
> include the people involved in it for the IPC review).

Pepper team is having a refactoring and this is part of it.  This is the only
one document that I'm aware of (see the Old/New resource and proxy design
sections)
 
http://www.chromium.org/developers/design-documents/pepper-plugin-implementation

As you can see in the message change, there are fewer information to be sent. 
But other than that, the change shouldn't change the behavior.  Not sure what
else can I provide here?

I don't know if anyone has done this review before.  Security team were added to
OWNERS last Friday, so maybe not?

[yzshen1, 2012-11-14 06:54:16]

http://codereview.chromium.org/11274036/diff/25033/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_video_capture_host.cc (right):

http://codereview.chromium.org/11274036/diff/25033/content/renderer/pepper/pe...
content/renderer/pepper/pepper_video_capture_host.cc:197: HostResource
host_resource;
On 2012/11/13 03:10:53, Victor Hsieh wrote:
> On 2012/11/10 01:14:40, yzshen1 wrote:
> > What I said above is for out-of-process. And please read those files. :)
> > 
> > In short, we will send back a message when the plugin side no longer holds
> this
> > ref. See PpapiHostMsg_PPBCore_ReleaseResource.
> > 
> > On 2012/11/08 09:20:18, Victor Hsieh wrote:
> > > On 2012/11/06 21:56:23, yzshen1 wrote:
> > > > You have to add one more ref for the plugin side. The plugin side will
> take
> > > > ownership of that reference. It will be removed when the plugin side
> destroy
> > > the
> > > > plugin-side object and call PluginResourceTracker::RemoveResource().
> > > > 
> > > > Please be careful with references. It is hard to find out later. You
might
> > > want
> > > > to read the implementation of resource_tracker/plugin_resource_tracker.
> > > 
> > > Done.  But how does that work when running out of process?
> > 
> 
> Done.  I was thinking in different way and got confused.  Also, I now changed
> PpapiGlobals to HostGlobals.  Both works in this case, but HostGlobals makes
> more sense here.  Correct me if I'm wrong.
HostGlobals SGTM.

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
File ppapi/proxy/video_capture_resource.cc (right):

http://codereview.chromium.org/11274036/diff/25033/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:101: 
IMO, it is easier to ensure the implementation has consistent behavior as the
previous implementation, if status_ is on the other side.
But I am fine with your approach.


On 2012/11/13 03:10:53, Victor Hsieh wrote:
> On 2012/11/10 01:14:40, yzshen1 wrote:
> > What is the reason to keep the status_ state machine at the host side
instead
> of
> > the plugin side?
> > 
> > On 2012/11/08 09:20:18, Victor Hsieh wrote:
> > > On 2012/11/06 06:51:40, yzshen1 wrote:
> > > > I notice that the previous code will do a
> > > > SetStatus(PP_VIDEO_CAPTURE_STATUS_STARTING, false) and check the outcome
> > here.
> > > > Is that not needed any more? Will it cause behavior change of the
> interface?
> > 
> > > 
> > > SetStatus checks the current state before jumping the the new.  The
previous
> > > state allowed of STARTING is STOPPED, which only happens either at the
> initial
> > > state or any error is happened.
> > > 
> > > In that case, I add another line of code here in OnMsgOnError to reset the
> > > state.  That should make behavior consistent to the old code.
> > 
> 
> The idea to keep status_ on the host when I first did this, is to keep 
> OnStarted/OnStopped/etc. still sending status to the plugin only in expected
> state.

http://codereview.chromium.org/11274036/diff/22004/ppapi/proxy/video_capture_...
File ppapi/proxy/video_capture_resource.cc (right):

http://codereview.chromium.org/11274036/diff/22004/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:164:
ppp_video_capture_impl_->OnDeviceInfo(
Please see
https://docs.google.com/a/google.com/document/d/185jwXWL4Cg6Ty-UruGZ5GjSlBLm3...

Especially the section called Releasing the Proxy Lock
On 2012/11/13 03:10:53, Victor Hsieh wrote:
> On 2012/11/10 01:14:40, yzshen1 wrote:
> > I think we need CallWhileUnlocked() for all the calls into the PPP
interface.
> > (There are several places in this file that need to be changed.)
> 
> Done.  I have no idea about when we should lock/unlock in pepper.  Would you
> mind to briefly explain a bit?

http://codereview.chromium.org/11274036/diff/33022/ppapi/proxy/flash_resource.h
File ppapi/proxy/flash_resource.h (right):

http://codereview.chromium.org/11274036/diff/33022/ppapi/proxy/flash_resource...
ppapi/proxy/flash_resource.h:13: struct PP_ArrayOutput;
you don't need this now.

http://codereview.chromium.org/11274036/diff/33022/ppapi/proxy/video_capture_...
File ppapi/proxy/video_capture_resource.cc (right):

http://codereview.chromium.org/11274036/diff/33022/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:148: int result =
SyncCall<PpapiPluginMsg_VideoCapture_EnumerateDevicesReply>(
Please use int32_t.

http://codereview.chromium.org/11274036/diff/33022/ppapi/proxy/video_capture_...
File ppapi/proxy/video_capture_resource.h (right):

http://codereview.chromium.org/11274036/diff/33022/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.h:31: int32_t EnumerateDevices(PP_Resource*
devices,
add 'virtual' for all these methods

http://codereview.chromium.org/11274036/diff/33022/ppapi/thunk/ppb_video_capt...
File ppapi/thunk/ppb_video_capture_api.h (right):

http://codereview.chromium.org/11274036/diff/33022/ppapi/thunk/ppb_video_capt...
ppapi/thunk/ppb_video_capture_api.h:40: virtual int32_t
EnumerateDevicesSync(PP_ArrayOutput* devices) = 0;
Please use const PP_ArrayOutput&. PP_ArrayOutput is not some output buffer
itself. So I don't think the 'output parameter should be a pointer' style guide
should be followed here.

[victorhsieh, 2012-11-14 08:22:27]

http://codereview.chromium.org/11274036/diff/22004/ppapi/proxy/video_capture_...
File ppapi/proxy/video_capture_resource.cc (right):

http://codereview.chromium.org/11274036/diff/22004/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:164:
ppp_video_capture_impl_->OnDeviceInfo(
On 2012/11/14 06:54:16, yzshen1 wrote:
> Please see
>
https://docs.google.com/a/google.com/document/d/185jwXWL4Cg6Ty-UruGZ5GjSlBLm3...
> 
> Especially the section called Releasing the Proxy Lock
> On 2012/11/13 03:10:53, Victor Hsieh wrote:
> > On 2012/11/10 01:14:40, yzshen1 wrote:
> > > I think we need CallWhileUnlocked() for all the calls into the PPP
> interface.
> > > (There are several places in this file that need to be changed.)
> > 
> > Done.  I have no idea about when we should lock/unlock in pepper.  Would you
> > mind to briefly explain a bit?
> 

Thanks!

http://codereview.chromium.org/11274036/diff/33022/ppapi/proxy/flash_resource.h
File ppapi/proxy/flash_resource.h (right):

http://codereview.chromium.org/11274036/diff/33022/ppapi/proxy/flash_resource...
ppapi/proxy/flash_resource.h:13: struct PP_ArrayOutput;
On 2012/11/14 06:54:16, yzshen1 wrote:
> you don't need this now.

Done.

http://codereview.chromium.org/11274036/diff/33022/ppapi/proxy/video_capture_...
File ppapi/proxy/video_capture_resource.cc (right):

http://codereview.chromium.org/11274036/diff/33022/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:148: int result =
SyncCall<PpapiPluginMsg_VideoCapture_EnumerateDevicesReply>(
On 2012/11/14 06:54:16, yzshen1 wrote:
> Please use int32_t.

Done.

http://codereview.chromium.org/11274036/diff/33022/ppapi/proxy/video_capture_...
File ppapi/proxy/video_capture_resource.h (right):

http://codereview.chromium.org/11274036/diff/33022/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.h:31: int32_t EnumerateDevices(PP_Resource*
devices,
On 2012/11/14 06:54:16, yzshen1 wrote:
> add 'virtual' for all these methods

Done.

http://codereview.chromium.org/11274036/diff/33022/ppapi/thunk/ppb_video_capt...
File ppapi/thunk/ppb_video_capture_api.h (right):

http://codereview.chromium.org/11274036/diff/33022/ppapi/thunk/ppb_video_capt...
ppapi/thunk/ppb_video_capture_api.h:40: virtual int32_t
EnumerateDevicesSync(PP_ArrayOutput* devices) = 0;
On 2012/11/14 06:54:16, yzshen1 wrote:
> Please use const PP_ArrayOutput&. PP_ArrayOutput is not some output buffer
> itself. So I don't think the 'output parameter should be a pointer' style
guide
> should be followed here.

Make sense.  Done.

[Pam (message me for reviews), 2012-11-14 13:06:45]

Drive-by: Please be more complete in your change description. What new design?
Why? How is it different, what had to change, what problems does it solve? A
lengthy discussion isn't necessary (although a pointer to one is good to add),
but a sentence or two motivating the change (without the need to follow any
links or look up any bugs) is. For example,

Refactor video capture to the new design, which uses FrobozzSword instead of
VorpalBlade to attack the Wizard, saving lots of memory.

[victorhsieh, 2012-11-14 13:22:34]

On 2012/11/14 13:06:45, Pam wrote:
> Drive-by: Please be more complete in your change description. What new design?
> Why? How is it different, what had to change, what problems does it solve? A
> lengthy discussion isn't necessary (although a pointer to one is good to add),
> but a sentence or two motivating the change (without the need to follow any
> links or look up any bugs) is. For example,
> 
> Refactor video capture to the new design, which uses FrobozzSword instead of
> VorpalBlade to attack the Wizard, saving lots of memory.

It's actually part of a large refactoring effort, that we migrate resources one
by one.  Anyway, I've updated the description to provide more information.

[yzshen1, 2012-11-14 17:55:36]

http://codereview.chromium.org/11274036/diff/33022/ppapi/thunk/ppb_video_capt...
File ppapi/thunk/ppb_video_capture_api.h (right):

http://codereview.chromium.org/11274036/diff/33022/ppapi/thunk/ppb_video_capt...
ppapi/thunk/ppb_video_capture_api.h:40: virtual int32_t
EnumerateDevicesSync(PP_ArrayOutput* devices) = 0;
I mean const &, please
On 2012/11/14 08:22:27, Victor Hsieh wrote:
> On 2012/11/14 06:54:16, yzshen1 wrote:
> > Please use const PP_ArrayOutput&. PP_ArrayOutput is not some output buffer
> > itself. So I don't think the 'output parameter should be a pointer' style
> guide
> > should be followed here.
> 
> Make sense.  Done.

http://codereview.chromium.org/11274036/diff/30058/ppapi/proxy/video_capture_...
File ppapi/proxy/video_capture_resource.cc (right):

http://codereview.chromium.org/11274036/diff/30058/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:138: PP_ArrayOutput& devices) {
const please.

[victorhsieh, 2012-11-14 22:47:35]

http://codereview.chromium.org/11274036/diff/30058/ppapi/proxy/video_capture_...
File ppapi/proxy/video_capture_resource.cc (right):

http://codereview.chromium.org/11274036/diff/30058/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:138: PP_ArrayOutput& devices) {
On 2012/11/14 17:55:36, yzshen1 wrote:
> const please.

I tried to avoid having a const here, that's why I chose a pointer first.  I
know the variable itself is not modified at all, but it's conceptually an
output.  I'd concern about the confusion that const might lead here.  The
trade-off is no compiler check for const-ness here, but in this care I'm fine
with it.  Make sense?

[brettw, 2012-11-14 22:51:51]

I think you make a reasonable point. It looks like we use const in other places
for this, and I don't think this is particularly ambiguous ("output" is in the
name), so let's just standardize on const for this.

[yzshen1, 2012-11-14 23:01:02]

http://codereview.chromium.org/11274036/diff/30058/ppapi/proxy/video_capture_...
File ppapi/proxy/video_capture_resource.cc (right):

http://codereview.chromium.org/11274036/diff/30058/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:138: PP_ArrayOutput& devices) {
On 2012/11/14 22:47:35, Victor Hsieh wrote:
> On 2012/11/14 17:55:36, yzshen1 wrote:
> > const please.
> 
> I tried to avoid having a const here, that's why I chose a pointer first.  I
> know the variable itself is not modified at all, but it's conceptually an
> output.  I'd concern about the confusion that const might lead here.  The
> trade-off is no compiler check for const-ness here, but in this care I'm fine
> with it.  Make sense?

This is not an output "buffer" itself. Strictly speaking, it points to "a way"
that can generate an output buffer. "Const" here indicates that "the way" won't
be changed by the method. :)

[victorhsieh, 2012-11-14 23:23:08]

Sometimes I had the same dilemma on different code base.  Nice to see a standard
for this case.

And fix ppapi_tests thanks to reminder from Raymes.

http://codereview.chromium.org/11274036/diff/33022/ppapi/thunk/ppb_video_capt...
File ppapi/thunk/ppb_video_capture_api.h (right):

http://codereview.chromium.org/11274036/diff/33022/ppapi/thunk/ppb_video_capt...
ppapi/thunk/ppb_video_capture_api.h:40: virtual int32_t
EnumerateDevicesSync(PP_ArrayOutput* devices) = 0;
On 2012/11/14 17:55:36, yzshen1 wrote:
> I mean const &, please
> On 2012/11/14 08:22:27, Victor Hsieh wrote:
> > On 2012/11/14 06:54:16, yzshen1 wrote:
> > > Please use const PP_ArrayOutput&. PP_ArrayOutput is not some output buffer
> > > itself. So I don't think the 'output parameter should be a pointer' style
> > guide
> > > should be followed here.
> > 
> > Make sense.  Done.
> 

Done.

http://codereview.chromium.org/11274036/diff/30058/ppapi/proxy/video_capture_...
File ppapi/proxy/video_capture_resource.cc (right):

http://codereview.chromium.org/11274036/diff/30058/ppapi/proxy/video_capture_...
ppapi/proxy/video_capture_resource.cc:138: PP_ArrayOutput& devices) {
On 2012/11/14 23:01:03, yzshen1 wrote:
> On 2012/11/14 22:47:35, Victor Hsieh wrote:
> > On 2012/11/14 17:55:36, yzshen1 wrote:
> > > const please.
> > 
> > I tried to avoid having a const here, that's why I chose a pointer first.  I
> > know the variable itself is not modified at all, but it's conceptually an
> > output.  I'd concern about the confusion that const might lead here.  The
> > trade-off is no compiler check for const-ness here, but in this care I'm
fine
> > with it.  Make sense?
> 
> This is not an output "buffer" itself. Strictly speaking, it points to "a way"
> that can generate an output buffer. "Const" here indicates that "the way"
won't
> be changed by the method. :)

Done.

[yzshen1, 2012-11-14 23:34:44]

LGTM

Thanks!

[victorhsieh, 2012-11-15 04:39:57]

Julien,

To answer your question in our offline discussion, the security level
should remains the same after this patch.  In general, the change doesn't
change the code logic overall, but the IPC message routing path from old
design (plugin <-> plugin proxy <-> host proxy <-> host on renderer) to the
new (plugin <-> host).  Other than that, behavior change should be little.

I should probably point out that there's a change in flash, but that only
changes how the call is delegated to the platform.


On Thu, Nov 15, 2012 at 7:34 AM, <yzshen@chromium.org> wrote:

> LGTM
>
> Thanks!
>
>
http://codereview.chromium.**org/11274036/<http://codereview.chromium.org/112...
>

[jln (very slow on Chromium), 2012-11-16 00:27:01]

On 2012/11/15 04:39:57, Victor Hsieh wrote:
> Julien,
> 
> To answer your question in our offline discussion, the security level
> should remains the same after this patch.  In general, the change doesn't
> change the code logic overall, but the IPC message routing path from old
> design (plugin <-> plugin proxy <-> host proxy <-> host on renderer) to the
> new (plugin <-> host).  Other than that, behavior change should be little.
> 
> I should probably point out that there's a change in flash, but that only
> changes how the call is delegated to the platform.

Excellent.

I did a quick sanity check and it looks like these IPCs were preexisting, except
perhaps PpapiPluginMsg_VideoCapture_OpenReply which seems harmless.

As such, this change LGTM for ppapi_messages.h.

As part of this sanity check I looked at (pre-existing) code that raised a few
questions, so I opened and assigned to you a bug to address these.

Sorry for the delay!

[yzshen1, 2012-11-16 00:53:46]

On 2012/11/16 00:27:01, Julien Tinnes wrote:
> On 2012/11/15 04:39:57, Victor Hsieh wrote:
> > Julien,
> > 
> > To answer your question in our offline discussion, the security level
> > should remains the same after this patch.  In general, the change doesn't
> > change the code logic overall, but the IPC message routing path from old
> > design (plugin <-> plugin proxy <-> host proxy <-> host on renderer) to the
> > new (plugin <-> host).  Other than that, behavior change should be little.
> > 
> > I should probably point out that there's a change in flash, but that only
> > changes how the call is delegated to the platform.
> 
> Excellent.
> 
> I did a quick sanity check and it looks like these IPCs were preexisting,
except
> perhaps PpapiPluginMsg_VideoCapture_OpenReply which seems harmless.
> 
> As such, this change LGTM for ppapi_messages.h.
> 
> As part of this sanity check I looked at (pre-existing) code that raised a few
> questions, so I opened and assigned to you a bug to address these.
> 
> Sorry for the delay!

Would you please point me to the bug?
I might be responsible for the code. So I would like to learn about the
problems.
I searched crbug.com but didn't find the bug.
Thanks!

[commit-bot: I haz the power, 2012-11-16 02:02:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/11274036/42001

[commit-bot: I haz the power, 2012-11-16 03:19:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/11274036/43043

[commit-bot: I haz the power, 2012-11-16 07:36:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/victorhsieh@chromium.org/11274036/43043

[commit-bot: I haz the power, 2012-11-16 07:53:10]

Change committed as 168163