PPAPI: implement GetSupportedPreviewSizes

PPAPI: implement GetSupportedPreviewSizes

1. Support PPB_CameraCapabilities_Private.GetSupportedPreviewSizes()
2. Implement basic skeleton of image capture PPAPI

BUG=387547
TEST=Call the API from a trusted plugin and print all preview
sizes.

Committed: https://crrev.com/add2f05d32384647371c56bba67bec3fc2f1ee2f
Cr-Commit-Position: refs/heads/master@{#316552}

[Justin Chuang, 2015-01-14 06:05:09]

jchuang@chromium.org changed reviewers:
+ wuchengli@chromium.org

[Justin Chuang, 2015-01-14 06:10:30]

On 2015/01/14 06:05:09, Justin Chuang wrote:
> mailto:jchuang@chromium.org changed reviewers:
> + mailto:wuchengli@chromium.org

This CL is roughly 668913002 - Still Image Capture API + PPB Camera
Capabilities. Hopefully it is smaller for review.

[wuchengli, 2015-01-19 14:05:04]

https://codereview.chromium.org/848863002/diff/80001/chrome/browser/component...
File chrome/browser/component_updater/ppapi_utils.cc (right):

https://codereview.chromium.org/848863002/diff/80001/chrome/browser/component...
chrome/browser/component_updater/ppapi_utils.cc:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
Update the subject. This CL is not only the skeleton anymore.

The first line of the change description should be the same as the Rietveld
subject. I think that will be used as git subject.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right):

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/content_renderer_pepper_host_factory.cc:209: switch
(message.type()) {
There is only one case. Using if is enough.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/pepper_image_capture_host.cc (right):

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_image_capture_host.cc:7: #include
"content/renderer/pepper/pepper_media_device_manager.h"
is this used?

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_image_capture_host.cc:10: #include
"content/renderer/render_frame_impl.h"
is this used?

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_image_capture_host.cc:37:
PPAPI_DISPATCH_HOST_RESOURCE_CALL(PpapiHostMsg_ImageCapture_Open, OnOpen)
Most other pepper host use 2 space indentation.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_image_capture_host.cc:45: void
PepperImageCaptureHost::OnPlatformInitialized(bool succeeded) {
s/OnPlatformInitialized/OnInitialized/. I'd prefer being consistent with
PepperVideoCaptureHost.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_image_capture_host.cc:65: 
Do we need to check platform_image_capture_ != NULL here? Is it possible someone
calls Close before preview sizes are enumerated?

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_image_capture_host.cc:66: if (sizes.size() > 0)
GetDeviceSupportedFormats doesn't say about size == 0 case. I think we should
return PP_OK even when sizes.size() == 0.

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_image_capture_host.cc:87:
platform_image_capture_.reset(new PepperPlatformImageCapture(
How about using PepperPlatformVideoCapture? We'll only need one API --
GetSupportedPreviewSizes. We can also save some code.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_image_capture_host.cc:118: int32_t
PepperImageCaptureHost::Close() {
Close and DetachPlatformImageCapture are the same. Remove Close and let others
call DetachPlatformImageCapture directly.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/pepper_image_capture_host.h (right):

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_image_capture_host.h:20: class VideoFrame;
Remove. Not used in this file.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/pepper_platform_image_capture.cc (right):

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_platform_image_capture.cc:39:
PP_DEVICETYPE_DEV_VIDEOCAPTURE,  // TODO(jchuang): add new type?
I don't think we need to add a new type. Remove TODO.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_platform_image_capture.cc:92: NOTREACHED();
I'd prefer following PepperPlatformVideoCapture::OnDeviceOpened unless you are
sure about this. Otherwise remove this if check and add if (handler_) in line
109.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_platform_image_capture.cc:104:
PP_DEVICETYPE_DEV_VIDEOCAPTURE, label);  // TODO: new device type?
Remove TODO.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_platform_image_capture.cc:116: NOTREACHED();
PepperPlatformVideoCapture::OnStateUpdate doesn't have NOTREACHED(). Is it
possible PepperImageCaptureHost::Close() is called before
OnDeviceSupportedFormatsEnumerated arrives?

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_platform_image_capture.cc:120: // TODO(jchuang):
Do we need fallback if the device doesn't support format
Remove TODO. VideoCaptureImpl::GetDeviceSupportedFormats doesn't say some
devices won't supports this. Also, this API will only be used by CrOS, which
supports GetDeviceSupportedFormats.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_platform_image_capture.cc:125:
sizes.push_back(PP_FromGfxSize(it->frame_size));
We should du-dup here. Right? Some frame sizes will be the same.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/resource_creation_impl.h (right):

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/resource_creation_impl.h:63: PP_Resource
CreateImageCaptureConfigPrivate(
Is it possible not to have CaptureConfig in this CL?

https://codereview.chromium.org/848863002/diff/80001/media/video/capture/vide...
File media/video/capture/video_capture_types.cc (right):

https://codereview.chromium.org/848863002/diff/80001/media/video/capture/vide...
media/video/capture/video_capture_types.cc:80: (frame_size.height() <
media::limits::kMaxDimension) &&
This is the limit for video. Image capture shouldn't have the same limit. Remove
frame_size.width/height check. Also remove frame_size.GetArea() check.

https://codereview.chromium.org/848863002/diff/80001/media/video/capture/vide...
File media/video/capture/video_capture_types.h (right):

https://codereview.chromium.org/848863002/diff/80001/media/video/capture/vide...
media/video/capture/video_capture_types.h:119: class MEDIA_EXPORT
ImageCaptureParams {
Remove this. No need a class with only one variable. Just use
ImageCaptureFormat.

https://codereview.chromium.org/848863002/diff/80001/ppapi/api/private/ppb_ca...
File ppapi/api/private/ppb_camera_capabilities_private.idl (right):

https://codereview.chromium.org/848863002/diff/80001/ppapi/api/private/ppb_ca...
ppapi/api/private/ppb_camera_capabilities_private.idl:14: M40 = 0.1
Tot is M42.

https://codereview.chromium.org/848863002/diff/80001/ppapi/api/private/ppb_im...
File ppapi/api/private/ppb_image_capture_private.idl (right):

https://codereview.chromium.org/848863002/diff/80001/ppapi/api/private/ppb_im...
ppapi/api/private/ppb_image_capture_private.idl:29: [in] uint64_t sequence_id);
Please revert all the changes not related to GetSupportedPreviewSizes. For
example, ShutterCallback, uesr_data, ErrorCallback, ReuseBuffers,
CaptureStillImage, and etc.

https://codereview.chromium.org/848863002/diff/80001/ppapi/cpp/private/camera...
File ppapi/cpp/private/camera_capabilities_private.h (right):

https://codereview.chromium.org/848863002/diff/80001/ppapi/cpp/private/camera...
ppapi/cpp/private/camera_capabilities_private.h:62: void
GetSupportedPreviewSizes(std::vector<Size>& preview_sizes);
Google c++ style guide says output should use pointer.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Para...

https://codereview.chromium.org/848863002/diff/80001/ppapi/ppapi_proxy.gypi
File ppapi/ppapi_proxy.gypi (right):

https://codereview.chromium.org/848863002/diff/80001/ppapi/ppapi_proxy.gypi#n...
ppapi/ppapi_proxy.gypi:89: 'proxy/image_capture_resource.h',
ppapi/BUILD.gn also needs to be updated.

https://codereview.chromium.org/848863002/diff/80001/ppapi/proxy/camera_capab...
File ppapi/proxy/camera_capabilities_resource.cc (right):

https://codereview.chromium.org/848863002/diff/80001/ppapi/proxy/camera_capab...
ppapi/proxy/camera_capabilities_resource.cc:38: void
CameraCapabilitiesResource::SetPreviewSizes(
SetXXX usually implies this can be called more than once. This better be done in
the constructor. Same for SetJpegSizes.

https://codereview.chromium.org/848863002/diff/80001/ppapi/proxy/camera_capab...
ppapi/proxy/camera_capabilities_resource.cc:41: DCHECK(!preview_sizes_.get());
// only initialized once
s/initialized/initialize/.

https://codereview.chromium.org/848863002/diff/80001/ppapi/proxy/image_captur...
File ppapi/proxy/image_capture_config_resource.cc (right):

https://codereview.chromium.org/848863002/diff/80001/ppapi/proxy/image_captur...
ppapi/proxy/image_capture_config_resource.cc:12: preview_size_.width = 0;
PP_Size is 0, 0 by default. Remove.

https://codereview.chromium.org/848863002/diff/80001/ppapi/proxy/image_captur...
File ppapi/proxy/image_capture_resource.cc (right):

https://codereview.chromium.org/848863002/diff/80001/ppapi/proxy/image_captur...
ppapi/proxy/image_capture_resource.cc:39: 
remove extra blank line

https://codereview.chromium.org/848863002/diff/80001/ppapi/proxy/image_captur...
ppapi/proxy/image_capture_resource.cc:40:
PPAPI_DISPATCH_PLUGIN_RESOURCE_CALL_UNHANDLED(NOTREACHED())
You mean everything should be already handled in line 33? If the only handler is
NOTREACHED(). Can we remove the entire PPAPI_BEGIN_MESSAGE_MAP block and just
have NOTREACHED()?

https://codereview.chromium.org/848863002/diff/80001/ppapi/proxy/image_captur...
ppapi/proxy/image_capture_resource.cc:150: // TODO(jchuang): support query of
JPEG sizes, too.
Remove TODO.

https://codereview.chromium.org/848863002/diff/80001/ppapi/proxy/ppapi_messag...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/848863002/diff/80001/ppapi/proxy/ppapi_messag...
ppapi/proxy/ppapi_messages.h:1563:
IPC_MESSAGE_CONTROL0(PpapiPluginMsg_ImageCapture_GetPreviewSizes)
s/GetPreviewSizes/GetSupportedPreviewSizes/. Same for below.

https://codereview.chromium.org/848863002/diff/80001/ppapi/proxy/resource_cre...
File ppapi/proxy/resource_creation_proxy.cc (right):

https://codereview.chromium.org/848863002/diff/80001/ppapi/proxy/resource_cre...
ppapi/proxy/resource_creation_proxy.cc:471: return (new
ImageCaptureResource(GetConnection(), instance))->GetReference();
Just curious, why some functions in this file do not have GetReference?

https://codereview.chromium.org/848863002/diff/80001/ppapi/thunk/ppb_camera_c...
File ppapi/thunk/ppb_camera_capabilities_api.h (right):

https://codereview.chromium.org/848863002/diff/80001/ppapi/thunk/ppb_camera_c...
ppapi/thunk/ppb_camera_capabilities_api.h:8: #include <string>
remove

https://codereview.chromium.org/848863002/diff/80001/ppapi/thunk/ppb_camera_c...
ppapi/thunk/ppb_camera_capabilities_api.h:14: class TrackedCallback;
remove

[Justin Chuang, 2015-01-26 15:00:51]

Thanks for the reviews, WuCheng

Pity I can't address all review comments today.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right):

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/content_renderer_pepper_host_factory.cc:209: switch
(message.type()) {
On 2015/01/19 14:05:02, wuchengli wrote:
> There is only one case. Using if is enough.

I think it's okay because the whole file does this.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/pepper_image_capture_host.cc (right):

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_image_capture_host.cc:7: #include
"content/renderer/pepper/pepper_media_device_manager.h"
On 2015/01/19 14:05:02, wuchengli wrote:
> is this used?

Done.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_image_capture_host.cc:10: #include
"content/renderer/render_frame_impl.h"
On 2015/01/19 14:05:02, wuchengli wrote:
> is this used?

yes, must be kept

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_image_capture_host.cc:37:
PPAPI_DISPATCH_HOST_RESOURCE_CALL(PpapiHostMsg_ImageCapture_Open, OnOpen)
On 2015/01/19 14:05:02, wuchengli wrote:
> Most other pepper host use 2 space indentation.

Done.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_image_capture_host.cc:45: void
PepperImageCaptureHost::OnPlatformInitialized(bool succeeded) {
On 2015/01/19 14:05:02, wuchengli wrote:
> s/OnPlatformInitialized/OnInitialized/. I'd prefer being consistent with
> PepperVideoCaptureHost. 

Done.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_image_capture_host.cc:65: 
On 2015/01/19 14:05:02, wuchengli wrote:
> Do we need to check platform_image_capture_ != NULL here? Is it possible
someone
> calls Close before preview sizes are enumerated?

No. Because the function is called directly in platform_image_capture_ in the
same thread, it won't be NULL, and we've got all data we need in this callback.

If the platform object is closed, the weak pointer in lower layer will become
NULL. Callback won't be invoked.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_image_capture_host.cc:66: if (sizes.size() > 0)
On 2015/01/19 14:05:02, wuchengli wrote:
> GetDeviceSupportedFormats doesn't say about size == 0 case. I think we should
> return PP_OK even when sizes.size() == 0.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

Yes, but you can see code in media stream treat it as a failed case.
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

I tend to treat it as failed because it is supposed to return a list of
supported resolutions. If it doesn't, then it's error. But it is open question
either higher or lower layer should return PP_ERROR.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_image_capture_host.cc:87:
platform_image_capture_.reset(new PepperPlatformImageCapture(
On 2015/01/19 14:05:02, wuchengli wrote:
> How about using PepperPlatformVideoCapture? We'll only need one API --
> GetSupportedPreviewSizes. We can also save some code.

Good point. I prefer to put it in PepperPlatformImageCapture so it's possible to
add more Image Capture PPAPI in the future. 
If we don't plan to extend Image Capture PPAPI, we can put "get_resolution_list"
API in Media Stream PPAPI instead. 

In addition, I expect both Image Capture and Video Capture will be converged in
video_capture_impl_manager layer (one layer downward), only the PPAPI host are
different. So there won't be a lot of duplicated code I think.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_image_capture_host.cc:118: int32_t
PepperImageCaptureHost::Close() {
On 2015/01/19 14:05:02, wuchengli wrote:
> Close and DetachPlatformImageCapture are the same. Remove Close and let others
> call DetachPlatformImageCapture directly.

When we support CaptureStillImage, then it will become two separate functions
because we need to handle operation abortion. I think it's okay to have two
functions look the same for now. It doesn't cause confusions, IMO.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/pepper_image_capture_host.h (right):

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_image_capture_host.h:20: class VideoFrame;
On 2015/01/19 14:05:02, wuchengli wrote:
> Remove. Not used in this file.

Done.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/pepper_platform_image_capture.cc (right):

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_platform_image_capture.cc:39:
PP_DEVICETYPE_DEV_VIDEOCAPTURE,  // TODO(jchuang): add new type?
On 2015/01/19 14:05:03, wuchengli wrote:
> I don't think we need to add a new type. Remove TODO.

Done.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_platform_image_capture.cc:92: NOTREACHED();
On 2015/01/19 14:05:03, wuchengli wrote:
> I'd prefer following PepperPlatformVideoCapture::OnDeviceOpened unless you are
> sure about this. Otherwise remove this if check and add if (handler_) in line
> 109.

I think the code is correct. handler_ is deregistered immediately before
PepperPlatformImageCapture is destroyed in upper layer. No harm to add
NONREACHED() here.  But I'm also okay to remove it since it shouldn't assert.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_platform_image_capture.cc:104:
PP_DEVICETYPE_DEV_VIDEOCAPTURE, label);  // TODO: new device type?
On 2015/01/19 14:05:03, wuchengli wrote:
> Remove TODO.

Done.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_platform_image_capture.cc:116: NOTREACHED();
On 2015/01/19 14:05:03, wuchengli wrote:
> PepperPlatformVideoCapture::OnStateUpdate doesn't have NOTREACHED(). Is it
> possible PepperImageCaptureHost::Close() is called before
> OnDeviceSupportedFormatsEnumerated arrives?

dittos. The pointer to this object is passed as weak pointer.  And it should be
invoked in the same loop.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_platform_image_capture.cc:120: // TODO(jchuang):
Do we need fallback if the device doesn't support format
On 2015/01/19 14:05:03, wuchengli wrote:
> Remove TODO. VideoCaptureImpl::GetDeviceSupportedFormats doesn't say some
> devices won't supports this. Also, this API will only be used by CrOS, which
> supports GetDeviceSupportedFormats.

Why it won't happen on CrOS? It depends on how the camera module implements UVC.

Anyway, I'll remove the TODO, and let's discuss error handling of format
enumeration in pepper_image_capture.cc

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_platform_image_capture.cc:125:
sizes.push_back(PP_FromGfxSize(it->frame_size));
On 2015/01/19 14:05:03, wuchengli wrote:
> We should du-dup here. Right? Some frame sizes will be the same.

Thanks. Can you paste result with dup? I don't see it on my webcam.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/resource_creation_impl.h (right):

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/resource_creation_impl.h:63: PP_Resource
CreateImageCaptureConfigPrivate(
On 2015/01/19 14:05:03, wuchengli wrote:
> Is it possible not to have CaptureConfig in this CL?

Will update to this CL later.

https://codereview.chromium.org/848863002/diff/80001/media/video/capture/vide...
File media/video/capture/video_capture_types.cc (right):

https://codereview.chromium.org/848863002/diff/80001/media/video/capture/vide...
media/video/capture/video_capture_types.cc:80: (frame_size.height() <
media::limits::kMaxDimension) &&
On 2015/01/19 14:05:03, wuchengli wrote:
> This is the limit for video. Image capture shouldn't have the same limit.
Remove
> frame_size.width/height check. Also remove frame_size.GetArea() check.

Removed the class completely.

https://codereview.chromium.org/848863002/diff/80001/media/video/capture/vide...
File media/video/capture/video_capture_types.h (right):

https://codereview.chromium.org/848863002/diff/80001/media/video/capture/vide...
media/video/capture/video_capture_types.h:119: class MEDIA_EXPORT
ImageCaptureParams {
On 2015/01/19 14:05:03, wuchengli wrote:
> Remove this. No need a class with only one variable. Just use
> ImageCaptureFormat.

Removed. I forgot to remove this. Thx

https://codereview.chromium.org/848863002/diff/80001/ppapi/api/private/ppb_ca...
File ppapi/api/private/ppb_camera_capabilities_private.idl (right):

https://codereview.chromium.org/848863002/diff/80001/ppapi/api/private/ppb_ca...
ppapi/api/private/ppb_camera_capabilities_private.idl:14: M40 = 0.1
On 2015/01/19 14:05:03, wuchengli wrote:
> Tot is M42.

Done, but it requires to uprev pp_macro.h

https://codereview.chromium.org/848863002/diff/80001/ppapi/api/private/ppb_im...
File ppapi/api/private/ppb_image_capture_private.idl (right):

https://codereview.chromium.org/848863002/diff/80001/ppapi/api/private/ppb_im...
ppapi/api/private/ppb_image_capture_private.idl:29: [in] uint64_t sequence_id);
On 2015/01/19 14:05:03, wuchengli wrote:
> Please revert all the changes not related to GetSupportedPreviewSizes. For
> example, ShutterCallback, uesr_data, ErrorCallback, ReuseBuffers,
> CaptureStillImage, and etc.

Will update to this CL later.

https://codereview.chromium.org/848863002/diff/80001/ppapi/cpp/private/camera...
File ppapi/cpp/private/camera_capabilities_private.h (right):

https://codereview.chromium.org/848863002/diff/80001/ppapi/cpp/private/camera...
ppapi/cpp/private/camera_capabilities_private.h:62: void
GetSupportedPreviewSizes(std::vector<Size>& preview_sizes);
On 2015/01/19 14:05:03, wuchengli wrote:
> Google c++ style guide says output should use pointer.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Para...

Done. Test plugin code needs to be updated, too.

https://codereview.chromium.org/848863002/diff/80001/ppapi/ppapi_proxy.gypi
File ppapi/ppapi_proxy.gypi (right):

https://codereview.chromium.org/848863002/diff/80001/ppapi/ppapi_proxy.gypi#n...
ppapi/ppapi_proxy.gypi:89: 'proxy/image_capture_resource.h',
On 2015/01/19 14:05:03, wuchengli wrote:
> ppapi/BUILD.gn also needs to be updated.

Leave it to the last.

https://codereview.chromium.org/848863002/diff/80001/ppapi/proxy/camera_capab...
File ppapi/proxy/camera_capabilities_resource.cc (right):

https://codereview.chromium.org/848863002/diff/80001/ppapi/proxy/camera_capab...
ppapi/proxy/camera_capabilities_resource.cc:38: void
CameraCapabilitiesResource::SetPreviewSizes(
On 2015/01/19 14:05:03, wuchengli wrote:
> SetXXX usually implies this can be called more than once. This better be done
in
> the constructor. Same for SetJpegSizes.

You are right. Actually I found I should do it in thunk layer, not proxy layer.
So the code is wrong.  

Will update in later CL.

https://codereview.chromium.org/848863002/diff/80001/ppapi/proxy/image_captur...
File ppapi/proxy/image_capture_config_resource.cc (right):

https://codereview.chromium.org/848863002/diff/80001/ppapi/proxy/image_captur...
ppapi/proxy/image_capture_config_resource.cc:12: preview_size_.width = 0;
On 2015/01/19 14:05:03, wuchengli wrote:
> PP_Size is 0, 0 by default. Remove.

No, it's struct, neither class nor PP object.

https://codereview.chromium.org/848863002/diff/80001/ppapi/proxy/image_captur...
File ppapi/proxy/image_capture_resource.cc (right):

https://codereview.chromium.org/848863002/diff/80001/ppapi/proxy/image_captur...
ppapi/proxy/image_capture_resource.cc:39: 
On 2015/01/19 14:05:03, wuchengli wrote:
> remove extra blank line

Done.

https://codereview.chromium.org/848863002/diff/80001/ppapi/proxy/image_captur...
ppapi/proxy/image_capture_resource.cc:40:
PPAPI_DISPATCH_PLUGIN_RESOURCE_CALL_UNHANDLED(NOTREACHED())
On 2015/01/19 14:05:03, wuchengli wrote:
> You mean everything should be already handled in line 33? If the only handler
is
> NOTREACHED(). Can we remove the entire PPAPI_BEGIN_MESSAGE_MAP block and just
> have NOTREACHED()?

I think keeping it as-as is okay.

https://codereview.chromium.org/848863002/diff/80001/ppapi/proxy/image_captur...
ppapi/proxy/image_capture_resource.cc:150: // TODO(jchuang): support query of
JPEG sizes, too.
On 2015/01/19 14:05:03, wuchengli wrote:
> Remove TODO.

Why not keep it?

https://codereview.chromium.org/848863002/diff/80001/ppapi/proxy/ppapi_messag...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/848863002/diff/80001/ppapi/proxy/ppapi_messag...
ppapi/proxy/ppapi_messages.h:1563:
IPC_MESSAGE_CONTROL0(PpapiPluginMsg_ImageCapture_GetPreviewSizes)
On 2015/01/19 14:05:03, wuchengli wrote:
> s/GetPreviewSizes/GetSupportedPreviewSizes/. Same for below.

Done.

https://codereview.chromium.org/848863002/diff/80001/ppapi/proxy/resource_cre...
File ppapi/proxy/resource_creation_proxy.cc (right):

https://codereview.chromium.org/848863002/diff/80001/ppapi/proxy/resource_cre...
ppapi/proxy/resource_creation_proxy.cc:471: return (new
ImageCaptureResource(GetConnection(), instance))->GetReference();
On 2015/01/19 14:05:03, wuchengli wrote:
> Just curious, why some functions in this file do not have GetReference?

If it uses PPB_XXX_Proxy I guess

https://codereview.chromium.org/848863002/diff/80001/ppapi/thunk/ppb_camera_c...
File ppapi/thunk/ppb_camera_capabilities_api.h (right):

https://codereview.chromium.org/848863002/diff/80001/ppapi/thunk/ppb_camera_c...
ppapi/thunk/ppb_camera_capabilities_api.h:8: #include <string>
On 2015/01/19 14:05:04, wuchengli wrote:
> remove

Done.

https://codereview.chromium.org/848863002/diff/80001/ppapi/thunk/ppb_camera_c...
ppapi/thunk/ppb_camera_capabilities_api.h:14: class TrackedCallback;
On 2015/01/19 14:05:04, wuchengli wrote:
> remove

Done. Thanks

[wuchengli, 2015-01-27 03:29:51]

wuchengli@chromium.org changed reviewers:
+ dmichael@chromium.org

[wuchengli, 2015-01-27 03:32:25]

Hi Dave. Please take a look. Thanks.

[wuchengli, 2015-01-27 06:20:30]

Next time please separate your changes and rebase to two patchsets -- your
changes in the first and the rebase in the second. That will be easier for
reviewers to review.

[dmichael (off chromium), 2015-01-28 00:00:36]

Still looking, but here are some initial comments.

https://codereview.chromium.org/848863002/diff/120001/ppapi/api/private/ppb_c...
File ppapi/api/private/ppb_camera_capabilities_private.idl (right):

https://codereview.chromium.org/848863002/diff/120001/ppapi/api/private/ppb_c...
ppapi/api/private/ppb_camera_capabilities_private.idl:47: [in] PP_Resource
resource);
I know this is minor, but I would rather see functions in the IDL formatted
roughly like they are for C or C++. I don't know why some of our IDL looks like
this, but when it can fit on previous line, it should go there...  and
similarly, in multi-line situations, if they can align with the paren instead of
a 4-space indent, they should do that. Thanks.

https://codereview.chromium.org/848863002/diff/120001/ppapi/api/private/ppb_i...
File ppapi/api/private/ppb_image_capture_private.idl (right):

https://codereview.chromium.org/848863002/diff/120001/ppapi/api/private/ppb_i...
ppapi/api/private/ppb_image_capture_private.idl:179: [in] PP_CompletionCallback
callback);
Is it overkill to have the config as a separate resource? Would it be better to
just have 2 Gets and 2 Sets here?

https://codereview.chromium.org/848863002/diff/120001/ppapi/api/private/ppb_i...
ppapi/api/private/ppb_image_capture_private.idl:209: [in] PP_Resource
image_capture);
I would rather this worked like our existing ImageData cache. When the plugin
finishes with an ImageData resource, they just de-reference it and it
automatically goes in to a cache for possible reuse. In that case, you wouldn't
need this function. (Admittedly, this is different because the browser is giving
the plugin an ImageData, rather than the other way around...  so the existing
ImageDataCache might not work. )

https://codereview.chromium.org/848863002/diff/120001/ppapi/api/private/ppb_i...
ppapi/api/private/ppb_image_capture_private.idl:223: * by the thread that calls
this function.
It's probably worth noting that there must be a PPB_MessageLoop bound to this
thread that will run to dispatch the callbacks.

https://codereview.chromium.org/848863002/diff/120001/ppapi/api/private/ppb_i...
ppapi/api/private/ppb_image_capture_private.idl:264: [in]
PPB_ImageCapture_Private_ErrorCallback error_callback,
Can you look at the way PPB_Messaging::RegisterMessageLoop and
PPP_MessageHandler work?

I'm afraid you might find what you have too limiting, because there's no way to
specify what thread the callbacks should be invoked on.

You might want to have two parameters here to replace the callback function
pointers:
const PPP_ImageCapture_Private* callbacks,
PP_Resource message_loop
|callbacks| is a struct containing all the callback function pointers,
message_loop is a PPB_MessageLoop resource where you want the calls to be
invoked.

https://codereview.chromium.org/848863002/diff/120001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_config_resource.h (right):

https://codereview.chromium.org/848863002/diff/120001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_config_resource.h:20:
ImageCaptureConfigResource(PP_Instance instance);
nit: explicit

https://codereview.chromium.org/848863002/diff/120001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.cc (right):

https://codereview.chromium.org/848863002/diff/120001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:39:
PPAPI_DISPATCH_PLUGIN_RESOURCE_CALL_UNHANDLED(NOTREACHED())
If you're not actually handling any messages here, I think you can just not
override OnReplyReceived at all...

https://codereview.chromium.org/848863002/diff/120001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:53: if (!source_string_var.get() ||
source_string_var->value().size() == 0)
no need to say ".get()"
and ".empty()" is better than ".size() == 0"

[wuchengli, 2015-01-28 08:51:42]

https://codereview.chromium.org/848863002/diff/120001/ppapi/api/private/ppb_i...
File ppapi/api/private/ppb_image_capture_private.idl (right):

https://codereview.chromium.org/848863002/diff/120001/ppapi/api/private/ppb_i...
ppapi/api/private/ppb_image_capture_private.idl:179: [in] PP_CompletionCallback
callback);
On 2015/01/28 00:00:35, dmichael wrote:
> Is it overkill to have the config as a separate resource? Would it be better
to
> just have 2 Gets and 2 Sets here?
Some settings have to be sent to the driver atomically. For example, apps want
to reduce exposure time and increase gain together. Having two image functions,
SetExposure and SetGain, in image capture class won't be atomic.

FYI. Config cannot be a simple struct because API may be added in different
chrome releases and only resource can have release function decorators if I
understand correctly.

Capabilities are camera characteristics that do not change. It can have lots of
things in the future like supported Jpeg sizes, supported JPEG sizes, focus
length, min aperture, max fps, and etc. Apps can just get capabilities once. It
should not be merged to config so GetConfig can be faster.

https://codereview.chromium.org/848863002/diff/120001/ppapi/api/private/ppb_i...
ppapi/api/private/ppb_image_capture_private.idl:209: [in] PP_Resource
image_capture);
On 2015/01/28 00:00:35, dmichael wrote:
> I would rather this worked like our existing ImageData cache. When the plugin
> finishes with an ImageData resource, they just de-reference it and it
> automatically goes in to a cache for possible reuse. In that case, you
wouldn't
> need this function. (Admittedly, this is different because the browser is
giving
> the plugin an ImageData, rather than the other way around...  so the existing
> ImageDataCache might not work. )
JPEG size is dynamic. If we want to make plugin give browser an ImageData, we
need another API to get the maximum JPEG size from the driver because the max
will be different for different driver. We'll also need to pass two ImageData to
CaptureStillImage.

The benefits of ImageData cache is to allow multiple Jpeg ImagaData.
ReuseBuffers assumes only one JPEG buffer at a time.

In the future (not gonna happen any time soon), ImageData can be backed by a
different memory that can be accessed by the driver efficiently, e.g. dmabuf. So
we may want to allocate the memory and decide the type in the implementation,
not in the plugin.

Justin. This change should belong to another CL. This CL should only contain
GetSupportedPreviewSizes. Also, FYI, this is ImageDataCache Dave talked about.
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/ppb_im...

https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/ppb_im...

https://codereview.chromium.org/848863002/diff/120001/ppapi/api/private/ppb_i...
ppapi/api/private/ppb_image_capture_private.idl:264: [in]
PPB_ImageCapture_Private_ErrorCallback error_callback,
On 2015/01/28 00:00:35, dmichael wrote:
> Can you look at the way PPB_Messaging::RegisterMessageLoop and
> PPP_MessageHandler work?
> 
> I'm afraid you might find what you have too limiting, because there's no way
to
> specify what thread the callbacks should be invoked on.
> 
> You might want to have two parameters here to replace the callback function
> pointers:
> const PPP_ImageCapture_Private* callbacks,
> PP_Resource message_loop
> |callbacks| is a struct containing all the callback function pointers,
> message_loop is a PPB_MessageLoop resource where you want the calls to be
> invoked.
This was an unfinished discussion in a previous CL when you were OOO. :) The
benefits of PPP_ImageCapture is that plugin doesn't need to declare lots of
callbacks. On the other hand, the current method allows the plugin only
implementing the callbacks it needs. PPB_Audio_Callback is also a callback.
Though we have lots of callbacks and probably we should change to
PPP_ImageCapture. I'm OK with either way.

Agree with specifying a thread to invoke on. This should belong to another CL.

[wuchengli, 2015-01-29 13:52:14]

Don't forget to revert all the changes not related to GetSupportedPreviewSizes.
For example, ShutterCallback, uesr_data, ErrorCallback, ReuseBuffers,
CaptureStillImage, and etc.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/pepper_image_capture_host.cc (right):

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_image_capture_host.cc:66: if (sizes.size() > 0)
On 2015/01/26 15:00:50, Justin Chuang wrote:
> On 2015/01/19 14:05:02, wuchengli wrote:
> > GetDeviceSupportedFormats doesn't say about size == 0 case. I think we
should
> > return PP_OK even when sizes.size() == 0.
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
> 
> Yes, but you can see code in media stream treat it as a failed case.
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
Agreed to have PP_ERROR here. This API won't work without formats enumeration
support and a camera should have at least one preview size.

From https://codereview.chromium.org/195363002, Mac QTKit and Win DirectShow do
not support formats enumeration. I asked Pawel. On Chrome OS, VIDIOC_ENUM_FMT is
required to be supported.
> 
> I tend to treat it as failed because it is supposed to return a list of
> supported resolutions. If it doesn't, then it's error. But it is open question
> either higher or lower layer should return PP_ERROR.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_image_capture_host.cc:87:
platform_image_capture_.reset(new PepperPlatformImageCapture(
On 2015/01/26 15:00:50, Justin Chuang wrote:
> On 2015/01/19 14:05:02, wuchengli wrote:
> > How about using PepperPlatformVideoCapture? We'll only need one API --
> > GetSupportedPreviewSizes. We can also save some code.
> 
> Good point. I prefer to put it in PepperPlatformImageCapture so it's possible
to
> add more Image Capture PPAPI in the future. 
Using PepperPlatformVideoCapture in this CL doesn't mean we cannot add
PepperPlatformImageCapture in the future. Usually a changelist should be
self-contained and add only the necessary code that are used now. If some code
are not used now, they should be added in the future. Having said that, I'm fine
with using PepperPlatformImageCapture in this CL. It's up to you.
> If we don't plan to extend Image Capture PPAPI, we can put
"get_resolution_list"
> API in Media Stream PPAPI instead. 
> 
> In addition, I expect both Image Capture and Video Capture will be converged
in
> video_capture_impl_manager layer (one layer downward), only the PPAPI host are
> different. So there won't be a lot of duplicated code I think.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_image_capture_host.cc:118: int32_t
PepperImageCaptureHost::Close() {
On 2015/01/26 15:00:50, Justin Chuang wrote:
> On 2015/01/19 14:05:02, wuchengli wrote:
> > Close and DetachPlatformImageCapture are the same. Remove Close and let
others
> > call DetachPlatformImageCapture directly.
> 
> When we support CaptureStillImage, then it will become two separate functions
> because we need to handle operation abortion. I think it's okay to have two
> functions look the same for now. It doesn't cause confusions, IMO.
This is the same problem like the previous comment. We should only look at the
code at this moment. If something is necessary for supporting CaptureStillImage,
it should be added in that CL, not this one. From this CL, Close and
DetachPlatformImageCapture are the same and one of them should be removed.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/pepper_platform_image_capture.cc (right):

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_platform_image_capture.cc:92: NOTREACHED();
On 2015/01/26 15:00:50, Justin Chuang wrote:
> On 2015/01/19 14:05:03, wuchengli wrote:
> > I'd prefer following PepperPlatformVideoCapture::OnDeviceOpened unless you
are
> > sure about this. Otherwise remove this if check and add if (handler_) in
line
> > 109.
> 
> I think the code is correct. handler_ is deregistered immediately before
> PepperPlatformImageCapture is destroyed in upper layer. No harm to add
> NONREACHED() here.  But I'm also okay to remove it since it shouldn't assert.
You are right. PepperPlatformImageCapture::DetachEventHandler set |handler_| to
NULL and call device_manager->CancelOpenDevice. This shouldn't happen. Let's
change it to DCHECK(handler_);

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_platform_image_capture.cc:116: NOTREACHED();
On 2015/01/26 15:00:50, Justin Chuang wrote:
> On 2015/01/19 14:05:03, wuchengli wrote:
> > PepperPlatformVideoCapture::OnStateUpdate doesn't have NOTREACHED(). Is it
> > possible PepperImageCaptureHost::Close() is called before
> > OnDeviceSupportedFormatsEnumerated arrives?
> 
> dittos. The pointer to this object is passed as weak pointer.  And it should
be
> invoked in the same loop.
That's correct. Then this can be changed to DCHECK(handler_);

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_platform_image_capture.cc:125:
sizes.push_back(PP_FromGfxSize(it->frame_size));
On 2015/01/26 15:00:50, Justin Chuang wrote:
> On 2015/01/19 14:05:03, wuchengli wrote:
> > We should du-dup here. Right? Some frame sizes will be the same.
> 
> Thanks. Can you paste result with dup? I don't see it on my webcam.
Sorry. By dup I meant same frame size but different fps or same frame size but
different format.
VideoCaptureDeviceFactoryLinux::GetDeviceSupportedFormats adds all combination
of format * size * fps. I'm sure some devices have formats like 640x480 30fps
and 640x380 15fps. We should ask Michael what he needs. I'll send an email.

https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur...

https://codereview.chromium.org/848863002/diff/80001/ppapi/api/private/ppb_ca...
File ppapi/api/private/ppb_camera_capabilities_private.idl (right):

https://codereview.chromium.org/848863002/diff/80001/ppapi/api/private/ppb_ca...
ppapi/api/private/ppb_camera_capabilities_private.idl:14: M40 = 0.1
On 2015/01/26 15:00:50, Justin Chuang wrote:
> On 2015/01/19 14:05:03, wuchengli wrote:
> > Tot is M42.
> 
> Done, but it requires to uprev pp_macro.h
I don't understand. Can you explain?

https://codereview.chromium.org/848863002/diff/120001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_image_capture.cc (right):

https://codereview.chromium.org/848863002/diff/120001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_image_capture.cc:61: void
PepperPlatformImageCapture::DetachEventHandler() {
Add DCHECK(thread_checker_.CalledOnValidThread()); for every method. Same below.

https://codereview.chromium.org/848863002/diff/120001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.cc (right):

https://codereview.chromium.org/848863002/diff/120001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:39:
PPAPI_DISPATCH_PLUGIN_RESOURCE_CALL_UNHANDLED(NOTREACHED())
On 2015/01/28 00:00:36, dmichael wrote:
> If you're not actually handling any messages here, I think you can just not
> override OnReplyReceived at all...
+1

[dmichael (off chromium), 2015-01-29 19:22:01]

https://codereview.chromium.org/848863002/diff/120001/ppapi/api/private/ppb_i...
File ppapi/api/private/ppb_image_capture_private.idl (right):

https://codereview.chromium.org/848863002/diff/120001/ppapi/api/private/ppb_i...
ppapi/api/private/ppb_image_capture_private.idl:264: [in]
PPB_ImageCapture_Private_ErrorCallback error_callback,
On 2015/01/28 08:51:42, wuchengli wrote:
> On 2015/01/28 00:00:35, dmichael wrote:
> > Can you look at the way PPB_Messaging::RegisterMessageLoop and
> > PPP_MessageHandler work?
> > 
> > I'm afraid you might find what you have too limiting, because there's no way
> to
> > specify what thread the callbacks should be invoked on.
> > 
> > You might want to have two parameters here to replace the callback function
> > pointers:
> > const PPP_ImageCapture_Private* callbacks,
> > PP_Resource message_loop
> > |callbacks| is a struct containing all the callback function pointers,
> > message_loop is a PPB_MessageLoop resource where you want the calls to be
> > invoked.
> This was an unfinished discussion in a previous CL when you were OOO. :) The
> benefits of PPP_ImageCapture is that plugin doesn't need to declare lots of
> callbacks. On the other hand, the current method allows the plugin only
> implementing the callbacks it needs.
You could do that as well by letting the user provide a NULL pointer for any
entry in the struct they don't want to implement.

> PPB_Audio_Callback is also a callback.
Right, it's not without precedent. But putting everything in a PPP struct will
make versioning more straightforward if you have to update the interface. It's a
newer pattern we didn't have when PPB_Audio was done.
> Though we have lots of callbacks and probably we should change to
> PPP_ImageCapture. I'm OK with either way.
> 
> Agree with specifying a thread to invoke on. This should belong to another CL.
sgtm

[Justin Chuang, 2015-02-02 08:43:45]

Address some review comments in Patch Set 8

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/pepper_platform_image_capture.cc (right):

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_platform_image_capture.cc:92: NOTREACHED();
On 2015/01/29 13:52:14, wuchengli wrote:
> You are right. PepperPlatformImageCapture::DetachEventHandler set |handler_|
to
> NULL and call device_manager->CancelOpenDevice. This shouldn't happen. Let's
> change it to DCHECK(handler_);

Done.

https://codereview.chromium.org/848863002/diff/80001/content/renderer/pepper/...
content/renderer/pepper/pepper_platform_image_capture.cc:116: NOTREACHED();
On 2015/01/29 13:52:14, wuchengli wrote:
> On 2015/01/26 15:00:50, Justin Chuang wrote:
> > dittos. The pointer to this object is passed as weak pointer.  And it should
> be
> > invoked in the same loop.
> That's correct. Then this can be changed to DCHECK(handler_);

Done.

https://codereview.chromium.org/848863002/diff/80001/ppapi/api/private/ppb_ca...
File ppapi/api/private/ppb_camera_capabilities_private.idl (right):

https://codereview.chromium.org/848863002/diff/80001/ppapi/api/private/ppb_ca...
ppapi/api/private/ppb_camera_capabilities_private.idl:14: M40 = 0.1
On 2015/01/29 13:52:14, wuchengli wrote:
> On 2015/01/26 15:00:50, Justin Chuang wrote:
> > On 2015/01/19 14:05:03, wuchengli wrote:
> > > Tot is M42.
> > 
> > Done, but it requires to uprev pp_macro.h
> I don't understand. Can you explain?

pp_macro.h defines M41. I need to change it to M42 to change the version here.

https://codereview.chromium.org/848863002/diff/120001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_image_capture.cc (right):

https://codereview.chromium.org/848863002/diff/120001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_image_capture.cc:61: void
PepperPlatformImageCapture::DetachEventHandler() {
On 2015/01/29 13:52:14, wuchengli wrote:
> Add DCHECK(thread_checker_.CalledOnValidThread()); for every method. Same
below.

Done.

https://codereview.chromium.org/848863002/diff/120001/ppapi/api/private/ppb_i...
File ppapi/api/private/ppb_image_capture_private.idl (right):

https://codereview.chromium.org/848863002/diff/120001/ppapi/api/private/ppb_i...
ppapi/api/private/ppb_image_capture_private.idl:209: [in] PP_Resource
image_capture);
On 2015/01/28 08:51:42, wuchengli wrote:
> On 2015/01/28 00:00:35, dmichael wrote:
> > I would rather this worked like our existing ImageData cache. When the
plugin
> > finishes with an ImageData resource, they just de-reference it and it
> > automatically goes in to a cache for possible reuse. In that case, you
> wouldn't
> > need this function. (Admittedly, this is different because the browser is
> giving
> > the plugin an ImageData, rather than the other way around...  so the
existing
> > ImageDataCache might not work. )
> JPEG size is dynamic. If we want to make plugin give browser an ImageData, we
> need another API to get the maximum JPEG size from the driver because the max
> will be different for different driver. We'll also need to pass two ImageData
to
> CaptureStillImage.
> 
> The benefits of ImageData cache is to allow multiple Jpeg ImagaData.
> ReuseBuffers assumes only one JPEG buffer at a time.
> 
> In the future (not gonna happen any time soon), ImageData can be backed by a
> different memory that can be accessed by the driver efficiently, e.g. dmabuf.
So
> we may want to allocate the memory and decide the type in the implementation,
> not in the plugin.
> 
> Justin. This change should belong to another CL. This CL should only contain
> GetSupportedPreviewSizes. Also, FYI, this is ImageDataCache Dave talked about.
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/ppb_im...
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/ppb_im...

Will move this change to a separate CL.

https://codereview.chromium.org/848863002/diff/120001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_config_resource.h (right):

https://codereview.chromium.org/848863002/diff/120001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_config_resource.h:20:
ImageCaptureConfigResource(PP_Instance instance);
On 2015/01/28 00:00:36, dmichael wrote:
> nit: explicit

Done. Thanks

https://codereview.chromium.org/848863002/diff/120001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.cc (right):

https://codereview.chromium.org/848863002/diff/120001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:39:
PPAPI_DISPATCH_PLUGIN_RESOURCE_CALL_UNHANDLED(NOTREACHED())
On 2015/01/29 13:52:14, wuchengli wrote:
> On 2015/01/28 00:00:36, dmichael wrote:
> > If you're not actually handling any messages here, I think you can just not
> > override OnReplyReceived at all...
> +1

Done.

https://codereview.chromium.org/848863002/diff/120001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:53: if (!source_string_var.get() ||
source_string_var->value().size() == 0)
On 2015/01/28 00:00:36, dmichael wrote:
> no need to say ".get()"
> and ".empty()" is better than ".size() == 0"

Done.

[Justin Chuang, 2015-02-02 10:27:15]

Patchset #9 (id:160001) has been deleted

[Justin Chuang, 2015-02-02 14:29:24]

https://codereview.chromium.org/848863002/diff/120001/ppapi/api/private/ppb_i...
File ppapi/api/private/ppb_image_capture_private.idl (right):

https://codereview.chromium.org/848863002/diff/120001/ppapi/api/private/ppb_i...
ppapi/api/private/ppb_image_capture_private.idl:209: [in] PP_Resource
image_capture);
On 2015/02/02 08:43:44, Justin Chuang wrote:
> 
> Will move this change to a separate CL.
Move the changes and discussions to 887403003

https://codereview.chromium.org/848863002/diff/120001/ppapi/api/private/ppb_i...
ppapi/api/private/ppb_image_capture_private.idl:223: * by the thread that calls
this function.
On 2015/01/28 00:00:35, dmichael wrote:
> It's probably worth noting that there must be a PPB_MessageLoop bound to this
> thread that will run to dispatch the callbacks.

Move the changes and discussions to 887403003

https://codereview.chromium.org/848863002/diff/120001/ppapi/api/private/ppb_i...
ppapi/api/private/ppb_image_capture_private.idl:264: [in]
PPB_ImageCapture_Private_ErrorCallback error_callback,
On 2015/01/29 19:22:00, dmichael wrote:
> On 2015/01/28 08:51:42, wuchengli wrote:
> > Agree with specifying a thread to invoke on. This should belong to another
CL.
> sgtm

Move the changes and discussions to 887403003

[wuchengli, 2015-02-02 14:42:33]

Only a few comments.

https://codereview.chromium.org/848863002/diff/180001/content/renderer/pepper...
File content/renderer/pepper/pepper_image_capture_host.cc (right):

https://codereview.chromium.org/848863002/diff/180001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.cc:88:
platform_image_capture_.reset(new PepperPlatformImageCapture(
The comment in patchset 4 might have been missed. I copied it over.

Using PepperPlatformVideoCapture in this CL doesn't mean we cannot add
PepperPlatformImageCapture in the future. Usually a changelist should be
self-contained and add only the necessary code that are used now. If some code
are not used now, they should be added in the future. Having said that, I'm fine
with using PepperPlatformImageCapture in this CL. It's up to you.

https://codereview.chromium.org/848863002/diff/180001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.cc:119: int32_t
PepperImageCaptureHost::Close() {
The comments in patchset 4 might have been missed. I copied it over.

We should only look at the code at this moment. If something is necessary
for supporting CaptureStillImage, it should be added in that CL, not this
one. From this CL, Close and DetachPlatformImageCapture are the same and
one of them should be removed.

https://codereview.chromium.org/848863002/diff/180001/ppapi/api/private/ppb_i...
File ppapi/api/private/ppb_image_capture_private.idl (right):

https://codereview.chromium.org/848863002/diff/180001/ppapi/api/private/ppb_i...
ppapi/api/private/ppb_image_capture_private.idl:28: typedef void
PPB_ImageCapture_Private_ShutterCallback(
No need to remove user_data because we're going to change this to
PPP_ImageCapture. Also, this is not related to GetSupportedPreviewSizes. Same
below.

https://codereview.chromium.org/848863002/diff/180001/ppapi/api/private/ppb_i...
ppapi/api/private/ppb_image_capture_private.idl:208: int32_t ReuseBuffers(
I'd prefer moving this to another CL. This is not related to
GetSupportedPreviewSizes.

https://codereview.chromium.org/848863002/diff/180001/ppapi/api/private/ppb_i...
ppapi/api/private/ppb_image_capture_private.idl:264: [in]
PPB_ImageCapture_Private_ErrorCallback error_callback,
Since we're going to add PPP_ImageCapture, no need to add error callback in this
CL. Remove.

https://codereview.chromium.org/848863002/diff/180001/ppapi/ppapi_sources.gypi
File ppapi/ppapi_sources.gypi (right):

https://codereview.chromium.org/848863002/diff/180001/ppapi/ppapi_sources.gyp...
ppapi/ppapi_sources.gypi:310: 'cpp/private/camera_capabilities_private.cc',
Need to update GN.

https://codereview.chromium.org/848863002/diff/180001/ppapi/proxy/camera_capa...
File ppapi/proxy/camera_capabilities_resource.cc (right):

https://codereview.chromium.org/848863002/diff/180001/ppapi/proxy/camera_capa...
ppapi/proxy/camera_capabilities_resource.cc:39: const std::vector<PP_Size>&
sizes) {
A reminder. You mentioned you'd move this to thunk layer.

[wuchengli, 2015-02-02 14:50:08]

You already addressed some of my previous review comments when I wrote them.
Please ignore those comments.

https://codereview.chromium.org/848863002/diff/200001/ppapi/cpp/private/image...
File ppapi/cpp/private/image_capture_private.h (right):

https://codereview.chromium.org/848863002/diff/200001/ppapi/cpp/private/image...
ppapi/cpp/private/image_capture_private.h:127: int32_t ReuseBuffers();
Forgot to remove.

https://codereview.chromium.org/848863002/diff/200001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.cc (right):

https://codereview.chromium.org/848863002/diff/200001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:109: int32_t
ImageCaptureResource::ReuseBuffers() {
Forgot to remove.

https://codereview.chromium.org/848863002/diff/200001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.h (right):

https://codereview.chromium.org/848863002/diff/200001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.h:47: virtual int32_t ReuseBuffers()
override;
Forgot to remove.

https://codereview.chromium.org/848863002/diff/200001/ppapi/thunk/ppb_image_c...
File ppapi/thunk/ppb_image_capture_api.h (right):

https://codereview.chromium.org/848863002/diff/200001/ppapi/thunk/ppb_image_c...
ppapi/thunk/ppb_image_capture_api.h:31: virtual int32_t ReuseBuffers() = 0;
Forgot to remove.

[dmichael (off chromium), 2015-02-02 16:30:49]

I heard we might not need the full capability of this API, and that maybe all
that is needed is finding the camera capabilities...  can you check with kmixter
or somebody about that? If that's the case, it would make sense to remove a lot
of the API before implementing it. I'll forward you an e-mail.

[wuchengli, 2015-02-03 02:36:35]

On 2015/02/02 16:30:49, dmichael wrote:
> I heard we might not need the full capability of this API, and that maybe all
> that is needed is finding the camera capabilities...  can you check with
kmixter
> or somebody about that? If that's the case, it would make sense to remove a
lot
> of the API before implementing it. I'll forward you an e-mail.
As discussed, implementing only GetSupportedPreviewSizes was the conclusion we
discussed with mknowles. This CL only contains GetSupportedPreviewSizes, so Dave
and I will continue to review this. Justin will upload two other CLs -- one to
remove CaptureStillImage/ImageCaptureConfig and one to add FPS information to
GetSupportedPreviewSizes.

[wuchengli, 2015-02-03 02:46:30]

https://codereview.chromium.org/848863002/diff/200001/content/renderer/pepper...
File content/renderer/pepper/pepper_image_capture_host.cc (right):

https://codereview.chromium.org/848863002/diff/200001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.cc:88:
platform_image_capture_.reset(new PepperPlatformImageCapture(
Given we're going to remove CaptureStillImage and
ImageCaptureConfig, please just use PepperPlatformVideoCapture
here. PepperPlatformImageCapture can be easily added in the
future if we need.

https://codereview.chromium.org/848863002/diff/200001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.cc:123: // TODO(jchuang):
abort pending image capture.
Remove TODO.

https://codereview.chromium.org/848863002/diff/200001/ppapi/c/private/ppb_ima...
File ppapi/c/private/ppb_image_capture_private.h (right):

https://codereview.chromium.org/848863002/diff/200001/ppapi/c/private/ppb_ima...
ppapi/c/private/ppb_image_capture_private.h:170: void (*Close)(PP_Resource
image_capture);
Does this return immediately and the implementation release the resources
asynchronously? Or this can take a long time to finish?

https://codereview.chromium.org/848863002/diff/200001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.cc (right):

https://codereview.chromium.org/848863002/diff/200001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:134: // TODO(jchuang): support query of
JPEG sizes, too.
Remove TODO because there's no plan to do it any time soon.

[Justin Chuang, 2015-02-03 12:32:09]

https://codereview.chromium.org/848863002/diff/180001/content/renderer/pepper...
File content/renderer/pepper/pepper_image_capture_host.cc (right):

https://codereview.chromium.org/848863002/diff/180001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.cc:88:
platform_image_capture_.reset(new PepperPlatformImageCapture(
On 2015/02/02 14:42:33, wuchengli wrote:
> The comment in patchset 4 might have been missed. I copied it over.
> 
> Using PepperPlatformVideoCapture in this CL doesn't mean we cannot add
> PepperPlatformImageCapture in the future. Usually a changelist should be
> self-contained and add only the necessary code that are used now. If some code
> are not used now, they should be added in the future. Having said that, I'm
fine
> with using PepperPlatformImageCapture in this CL. It's up to you.

As discussed in draft refactoring in 883983005. We agree not to do this
refactoring.

https://codereview.chromium.org/848863002/diff/180001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.cc:119: int32_t
PepperImageCaptureHost::Close() {
On 2015/02/02 14:42:32, wuchengli wrote:
> The comments in patchset 4 might have been missed. I copied it over.
> 
> We should only look at the code at this moment. If something is necessary
> for supporting CaptureStillImage, it should be added in that CL, not this
> one. From this CL, Close and DetachPlatformImageCapture are the same and
> one of them should be removed.

Done.

https://codereview.chromium.org/848863002/diff/180001/ppapi/api/private/ppb_i...
File ppapi/api/private/ppb_image_capture_private.idl (right):

https://codereview.chromium.org/848863002/diff/180001/ppapi/api/private/ppb_i...
ppapi/api/private/ppb_image_capture_private.idl:28: typedef void
PPB_ImageCapture_Private_ShutterCallback(
On 2015/02/02 14:42:33, wuchengli wrote:
> No need to remove user_data because we're going to change this to
> PPP_ImageCapture. Also, this is not related to GetSupportedPreviewSizes. Same
> below.

Already removed in later patchset.

https://codereview.chromium.org/848863002/diff/180001/ppapi/api/private/ppb_i...
ppapi/api/private/ppb_image_capture_private.idl:208: int32_t ReuseBuffers(
On 2015/02/02 14:42:33, wuchengli wrote:
> I'd prefer moving this to another CL. This is not related to
> GetSupportedPreviewSizes.

Already removed.

https://codereview.chromium.org/848863002/diff/180001/ppapi/api/private/ppb_i...
ppapi/api/private/ppb_image_capture_private.idl:264: [in]
PPB_ImageCapture_Private_ErrorCallback error_callback,
On 2015/02/02 14:42:33, wuchengli wrote:
> Since we're going to add PPP_ImageCapture, no need to add error callback in
this
> CL. Remove.

Already removed.

https://codereview.chromium.org/848863002/diff/180001/ppapi/ppapi_sources.gypi
File ppapi/ppapi_sources.gypi (right):

https://codereview.chromium.org/848863002/diff/180001/ppapi/ppapi_sources.gyp...
ppapi/ppapi_sources.gypi:310: 'cpp/private/camera_capabilities_private.cc',
On 2015/02/02 14:42:33, wuchengli wrote:
> Need to update GN.

Done. But haven't verified GN working.

https://codereview.chromium.org/848863002/diff/180001/ppapi/proxy/camera_capa...
File ppapi/proxy/camera_capabilities_resource.cc (right):

https://codereview.chromium.org/848863002/diff/180001/ppapi/proxy/camera_capa...
ppapi/proxy/camera_capabilities_resource.cc:39: const std::vector<PP_Size>&
sizes) {
On 2015/02/02 14:42:33, wuchengli wrote:
> A reminder. You mentioned you'd move this to thunk layer.

Done. I was wrong. The added argument is still added at to the constructor of
this class. Also remove Create() method in PPB_CameraCapabilities_Private()
because it's not needed. (similar example: PPB_NetworkList)

https://codereview.chromium.org/848863002/diff/200001/content/renderer/pepper...
File content/renderer/pepper/pepper_image_capture_host.cc (right):

https://codereview.chromium.org/848863002/diff/200001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.cc:88:
platform_image_capture_.reset(new PepperPlatformImageCapture(
On 2015/02/03 02:46:29, wuchengli wrote:
> Given we're going to remove CaptureStillImage and
> ImageCaptureConfig, please just use PepperPlatformVideoCapture
> here. PepperPlatformImageCapture can be easily added in the
> future if we need.

Dittos. Sample refactoring is shown in 883983005, agree not to do it. Thanks

https://codereview.chromium.org/848863002/diff/200001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.cc:123: // TODO(jchuang):
abort pending image capture.
On 2015/02/03 02:46:29, wuchengli wrote:
> Remove TODO.

Done.

https://codereview.chromium.org/848863002/diff/200001/ppapi/c/private/ppb_ima...
File ppapi/c/private/ppb_image_capture_private.h (right):

https://codereview.chromium.org/848863002/diff/200001/ppapi/c/private/ppb_ima...
ppapi/c/private/ppb_image_capture_private.h:170: void (*Close)(PP_Resource
image_capture);
On 2015/02/03 02:46:29, wuchengli wrote:
> Does this return immediately and the implementation release the resources
> asynchronously? Or this can take a long time to finish?

The former case.

https://codereview.chromium.org/848863002/diff/200001/ppapi/cpp/private/image...
File ppapi/cpp/private/image_capture_private.h (right):

https://codereview.chromium.org/848863002/diff/200001/ppapi/cpp/private/image...
ppapi/cpp/private/image_capture_private.h:127: int32_t ReuseBuffers();
On 2015/02/02 14:50:08, wuchengli wrote:
> Forgot to remove.

Done. Thanks

https://codereview.chromium.org/848863002/diff/200001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.cc (right):

https://codereview.chromium.org/848863002/diff/200001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:109: int32_t
ImageCaptureResource::ReuseBuffers() {
On 2015/02/02 14:50:08, wuchengli wrote:
> Forgot to remove.

Done.

https://codereview.chromium.org/848863002/diff/200001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:134: // TODO(jchuang): support query of
JPEG sizes, too.
On 2015/02/03 02:46:29, wuchengli wrote:
> Remove TODO because there's no plan to do it any time soon.

Done.

https://codereview.chromium.org/848863002/diff/200001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.h (right):

https://codereview.chromium.org/848863002/diff/200001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.h:47: virtual int32_t ReuseBuffers()
override;
On 2015/02/02 14:50:08, wuchengli wrote:
> Forgot to remove.

Done.

https://codereview.chromium.org/848863002/diff/200001/ppapi/thunk/ppb_image_c...
File ppapi/thunk/ppb_image_capture_api.h (right):

https://codereview.chromium.org/848863002/diff/200001/ppapi/thunk/ppb_image_c...
ppapi/thunk/ppb_image_capture_api.h:31: virtual int32_t ReuseBuffers() = 0;
On 2015/02/02 14:50:08, wuchengli wrote:
> Forgot to remove.

Done.

[Justin Chuang, 2015-02-03 13:15:11]

On 2015/02/03 02:36:35, wuchengli wrote:
> On 2015/02/02 16:30:49, dmichael wrote:
> > I heard we might not need the full capability of this API, and that maybe
all
> > that is needed is finding the camera capabilities...  can you check with
> kmixter
> > or somebody about that? If that's the case, it would make sense to remove a
> lot
> > of the API before implementing it. I'll forward you an e-mail.
> As discussed, implementing only GetSupportedPreviewSizes was the conclusion we
> discussed with mknowles. This CL only contains GetSupportedPreviewSizes, so
Dave
> and I will continue to review this. Justin will upload two other CLs -- one to
> remove CaptureStillImage/ImageCaptureConfig and one to add FPS information to
> GetSupportedPreviewSizes.

Sorry, I didn't create another CLs to remove the unused APIs. Updated in Patch
Set 12 directly. ppb_image_capture_config is also removed.

[Justin Chuang, 2015-02-03 13:56:23]

Patchset #12 (id:240001) has been deleted

[Justin Chuang, 2015-02-03 13:57:38]

On 2015/02/03 13:15:11, Justin Chuang wrote:
> On 2015/02/03 02:36:35, wuchengli wrote:
> > On 2015/02/02 16:30:49, dmichael wrote:
> > and I will continue to review this. Justin will upload two other CLs -- one
to
> > remove CaptureStillImage/ImageCaptureConfig and one to add FPS information
to
> > GetSupportedPreviewSizes.
> 
> Sorry, I didn't create another CLs to remove the unused APIs. Updated in Patch
> Set 12 directly. ppb_image_capture_config is also removed.

Discuss with WuCheng, we don't remove unused API in this CL. Will add another CL
to remove them. Sorry.

[wuchengli, 2015-02-03 14:34:45]

Only some small comments.

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
File content/renderer/pepper/pepper_image_capture_host.cc (right):

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.cc:38:
PpapiPluginMsg_ImageCapture_GetSupportedPreviewSizes, OnGetPreviewSizes)
naming should be consistent. s/OnGetPreviewSizes/OnGetSupportedPreviewSizes/

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.cc:41: PPAPI_END_MESSAGE_MAP()
wrong indent. Can git cl format fix this?

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.cc:114:
platform_image_capture_->GetPreviewSizes();
s/GetPreviewSizes/GetSupportedPreviewSizes/

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
File content/renderer/pepper/pepper_image_capture_host.h (right):

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.h:9: #include
"base/memory/ref_counted.h"
not used?

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.h:11: #include
"content/common/media/video_capture.h"
not used?

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.h:14: #include
"media/video/capture/video_capture_types.h"
not used?

https://codereview.chromium.org/848863002/diff/220001/ppapi/BUILD.gn
File ppapi/BUILD.gn (right):

https://codereview.chromium.org/848863002/diff/220001/ppapi/BUILD.gn#newcode687
ppapi/BUILD.gn:687: "proxy/camera_capabilities_resource.cc",
You can run "git cl try" on all trybots. Some trybots are GN.

https://codereview.chromium.org/848863002/diff/220001/ppapi/cpp/private/image...
File ppapi/cpp/private/image_capture_private.h (right):

https://codereview.chromium.org/848863002/diff/220001/ppapi/cpp/private/image...
ppapi/cpp/private/image_capture_private.h:159: /// </code> callback to indicate
<code>CaptureStillImage()</code> has failed.
Forgot to remove.

https://codereview.chromium.org/848863002/diff/220001/ppapi/proxy/camera_capa...
File ppapi/proxy/camera_capabilities_resource.cc (right):

https://codereview.chromium.org/848863002/diff/220001/ppapi/proxy/camera_capa...
ppapi/proxy/camera_capabilities_resource.cc:13: const std::vector<PP_Size>&
preview_array)
s/preview_array/preview_sizes/

[dmichael (off chromium), 2015-02-03 19:21:29]

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right):

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
content/renderer/pepper/content_renderer_pepper_host_factory.cc:214: :
scoped_ptr<ResourceHost>();
You're probably going to want this to be more like the MediaStream stuff, right?
It needs to be availabale to NaCl but whitelisted, I think?

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_image_capture.h (right):

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_image_capture.h:23: class
PepperImageCaptureHost;
Any need to touch this file in this CL?

https://codereview.chromium.org/848863002/diff/220001/ppapi/api/private/ppb_i...
File ppapi/api/private/ppb_image_capture_private.idl (right):

https://codereview.chromium.org/848863002/diff/220001/ppapi/api/private/ppb_i...
ppapi/api/private/ppb_image_capture_private.idl:14: M42 = 0.1
Maybe best to just leave this file alone if we're going to delete it in the next
CL?

https://codereview.chromium.org/848863002/diff/220001/ppapi/cpp/private/camer...
File ppapi/cpp/private/camera_capabilities_private.cc (right):

https://codereview.chromium.org/848863002/diff/220001/ppapi/cpp/private/camer...
ppapi/cpp/private/camera_capabilities_private.cc:55: preview_sizes->clear();
might as well:
preview_sizes->reserve(array_size);

https://codereview.chromium.org/848863002/diff/220001/ppapi/proxy/camera_capa...
File ppapi/proxy/camera_capabilities_resource.cc (right):

https://codereview.chromium.org/848863002/diff/220001/ppapi/proxy/camera_capa...
ppapi/proxy/camera_capabilities_resource.cc:18: preview_sizes_[i] =
preview_array[i];
suggestion: std::copy?

https://codereview.chromium.org/848863002/diff/220001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.h (right):

https://codereview.chromium.org/848863002/diff/220001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.h:1: // Copyright 2014 The Chromium Authors.
All rights reserved.
Should this file and the .cc not be added?

https://codereview.chromium.org/848863002/diff/220001/ppapi/proxy/ppapi_messa...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/848863002/diff/220001/ppapi/proxy/ppapi_messa...
ppapi/proxy/ppapi_messages.h:1558: // ImageCapture_Private, host -> plugin
Probably don't need to change this file at all

https://codereview.chromium.org/848863002/diff/220001/ppapi/proxy/resource_cr...
File ppapi/proxy/resource_creation_proxy.h (right):

https://codereview.chromium.org/848863002/diff/220001/ppapi/proxy/resource_cr...
ppapi/proxy/resource_creation_proxy.h:179: virtual PP_Resource
CreateImageCapturePrivate(PP_Instance instance) override;
ditto; don't need this

[dmichael (off chromium), 2015-02-03 19:33:17]

Sorry about that, I was confused about which API had the Capabilities part you
need.

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_image_capture.h (right):

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_image_capture.h:23: class
PepperImageCaptureHost;
On 2015/02/03 19:21:28, dmichael wrote:
> Any need to touch this file in this CL?
Nevermind

https://codereview.chromium.org/848863002/diff/220001/ppapi/api/private/ppb_i...
File ppapi/api/private/ppb_image_capture_private.idl (right):

https://codereview.chromium.org/848863002/diff/220001/ppapi/api/private/ppb_i...
ppapi/api/private/ppb_image_capture_private.idl:14: M42 = 0.1
On 2015/02/03 19:21:29, dmichael wrote:
> Maybe best to just leave this file alone if we're going to delete it in the
next
> CL?
Nevermind

https://codereview.chromium.org/848863002/diff/220001/ppapi/proxy/ppapi_messa...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/848863002/diff/220001/ppapi/proxy/ppapi_messa...
ppapi/proxy/ppapi_messages.h:1558: // ImageCapture_Private, host -> plugin
On 2015/02/03 19:21:29, dmichael wrote:
> Probably don't need to change this file at all
Nevermind, sorry I didn't read more carefully.

https://codereview.chromium.org/848863002/diff/220001/ppapi/thunk/ppb_image_c...
File ppapi/thunk/ppb_image_capture_api.h (right):

https://codereview.chromium.org/848863002/diff/220001/ppapi/thunk/ppb_image_c...
ppapi/thunk/ppb_image_capture_api.h:35: int64_t* sequence_id) = 0;
Hmm, it's tempting to say you should land the CL deleting stuff from the API
before landing this, so you don't have to have all this boilerplate that's going
to disappear. WDYT?

[Justin Chuang, 2015-02-04 16:26:01]

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right):

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
content/renderer/pepper/content_renderer_pepper_host_factory.cc:214: :
scoped_ptr<ResourceHost>();
On 2015/02/03 19:21:28, dmichael wrote:
> You're probably going to want this to be more like the MediaStream stuff,
right?
> It needs to be availabale to NaCl but whitelisted, I think?

I guess you're right. How do I get this whitelisted in addition to moving the
code around defined(OS_NACL)?

[Justin Chuang, 2015-02-04 17:44:24]

Comments in Patch Set 11 should be addressed except for the NACL part.

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
File content/renderer/pepper/pepper_image_capture_host.cc (right):

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.cc:38:
PpapiPluginMsg_ImageCapture_GetSupportedPreviewSizes, OnGetPreviewSizes)
On 2015/02/03 14:34:45, wuchengli wrote:
> naming should be consistent. s/OnGetPreviewSizes/OnGetSupportedPreviewSizes/

Done.

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.cc:41: PPAPI_END_MESSAGE_MAP()
On 2015/02/03 14:34:45, wuchengli wrote:
> wrong indent. Can git cl format fix this?

Run git cl format again. Thanks.

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.cc:114:
platform_image_capture_->GetPreviewSizes();
On 2015/02/03 14:34:44, wuchengli wrote:
> s/GetPreviewSizes/GetSupportedPreviewSizes/

Done.

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
File content/renderer/pepper/pepper_image_capture_host.h (right):

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.h:9: #include
"base/memory/ref_counted.h"
On 2015/02/03 14:34:45, wuchengli wrote:
> not used?

Done.

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.h:11: #include
"content/common/media/video_capture.h"
On 2015/02/03 14:34:45, wuchengli wrote:
> not used?

Done.

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.h:14: #include
"media/video/capture/video_capture_types.h"
On 2015/02/03 14:34:45, wuchengli wrote:
> not used?

Done.

https://codereview.chromium.org/848863002/diff/220001/ppapi/BUILD.gn
File ppapi/BUILD.gn (right):

https://codereview.chromium.org/848863002/diff/220001/ppapi/BUILD.gn#newcode687
ppapi/BUILD.gn:687: "proxy/camera_capabilities_resource.cc",
On 2015/02/03 14:34:45, wuchengli wrote:
> You can run "git cl try" on all trybots. Some trybots are GN. 

thanks, will try after rebasing.

https://codereview.chromium.org/848863002/diff/220001/ppapi/cpp/private/camer...
File ppapi/cpp/private/camera_capabilities_private.cc (right):

https://codereview.chromium.org/848863002/diff/220001/ppapi/cpp/private/camer...
ppapi/cpp/private/camera_capabilities_private.cc:55: preview_sizes->clear();
On 2015/02/03 19:21:29, dmichael wrote:
> might as well:
> preview_sizes->reserve(array_size);

Done. Thanks.

https://codereview.chromium.org/848863002/diff/220001/ppapi/cpp/private/image...
File ppapi/cpp/private/image_capture_private.h (right):

https://codereview.chromium.org/848863002/diff/220001/ppapi/cpp/private/image...
ppapi/cpp/private/image_capture_private.h:159: /// </code> callback to indicate
<code>CaptureStillImage()</code> has failed.
On 2015/02/03 14:34:45, wuchengli wrote:
> Forgot to remove.

Done.

https://codereview.chromium.org/848863002/diff/220001/ppapi/proxy/camera_capa...
File ppapi/proxy/camera_capabilities_resource.cc (right):

https://codereview.chromium.org/848863002/diff/220001/ppapi/proxy/camera_capa...
ppapi/proxy/camera_capabilities_resource.cc:13: const std::vector<PP_Size>&
preview_array)
On 2015/02/03 14:34:45, wuchengli wrote:
> s/preview_array/preview_sizes/

Done.

https://codereview.chromium.org/848863002/diff/220001/ppapi/proxy/camera_capa...
ppapi/proxy/camera_capabilities_resource.cc:18: preview_sizes_[i] =
preview_array[i];
On 2015/02/03 19:21:29, dmichael wrote:
> suggestion: std::copy?

Done.

https://codereview.chromium.org/848863002/diff/220001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.h (right):

https://codereview.chromium.org/848863002/diff/220001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.h:1: // Copyright 2014 The Chromium Authors.
All rights reserved.
On 2015/02/03 19:21:29, dmichael wrote:
> Should this file and the .cc not be added?

This file is needed unless we merge PPB_ImageCapture and PPB_CameraCapabilities
(which I don't recommend). See discussion in 884483007.

https://codereview.chromium.org/848863002/diff/220001/ppapi/proxy/resource_cr...
File ppapi/proxy/resource_creation_proxy.h (right):

https://codereview.chromium.org/848863002/diff/220001/ppapi/proxy/resource_cr...
ppapi/proxy/resource_creation_proxy.h:179: virtual PP_Resource
CreateImageCapturePrivate(PP_Instance instance) override;
On 2015/02/03 19:21:29, dmichael wrote:
> ditto; don't need this

I think it's used in ppb_image_capture_private_thunk.cc

https://codereview.chromium.org/848863002/diff/220001/ppapi/thunk/ppb_image_c...
File ppapi/thunk/ppb_image_capture_api.h (right):

https://codereview.chromium.org/848863002/diff/220001/ppapi/thunk/ppb_image_c...
ppapi/thunk/ppb_image_capture_api.h:35: int64_t* sequence_id) = 0;
On 2015/02/03 19:33:17, dmichael wrote:
> Hmm, it's tempting to say you should land the CL deleting stuff from the API
> before landing this, so you don't have to have all this boilerplate that's
going
> to disappear. WDYT?

Sure, I'm okay in either way. I can commit the other CL (deleting stuff) and
rebase this one again. 

(But changing PPB_ImageCapture to another class name need another CL *after*
this one because it is trickier.)

[wuchengli, 2015-02-05 03:32:54]

LGTM. Remember to merge 884483007 and rebase this before merging.

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right):

https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
content/renderer/pepper/content_renderer_pepper_host_factory.cc:214: :
scoped_ptr<ResourceHost>();
On 2015/02/04 16:26:01, Justin Chuang wrote:
> On 2015/02/03 19:21:28, dmichael wrote:
> > You're probably going to want this to be more like the MediaStream stuff,
> right?
> > It needs to be availabale to NaCl but whitelisted, I think?
> 
> I guess you're right. How do I get this whitelisted in addition to moving the
> code around defined(OS_NACL)?
My understanding is this api should be added to NaCl SDK whitelist like this --
http://crrev.com/16190011. I think it can be also another CL.

https://codereview.chromium.org/848863002/diff/220001/ppapi/thunk/ppb_image_c...
File ppapi/thunk/ppb_image_capture_api.h (right):

https://codereview.chromium.org/848863002/diff/220001/ppapi/thunk/ppb_image_c...
ppapi/thunk/ppb_image_capture_api.h:35: int64_t* sequence_id) = 0;
On 2015/02/04 17:44:24, Justin Chuang wrote:
> On 2015/02/03 19:33:17, dmichael wrote:
> > Hmm, it's tempting to say you should land the CL deleting stuff from the API
> > before landing this, so you don't have to have all this boilerplate that's
> going
> > to disappear. WDYT?
> 
> Sure, I'm okay in either way. I can commit the other CL (deleting stuff) and
> rebase this one again. 
> 
> (But changing PPB_ImageCapture to another class name need another CL *after*
> this one because it is trickier.)
http://crrev.com/884483007 was already approved. So please merge that one first
and then rebase this one.

[dmichael (off chromium), 2015-02-05 22:39:28]

On 2015/02/04 16:26:01, Justin Chuang wrote:
>
https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
> File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right):
> 
>
https://codereview.chromium.org/848863002/diff/220001/content/renderer/pepper...
> content/renderer/pepper/content_renderer_pepper_host_factory.cc:214: :
> scoped_ptr<ResourceHost>();
> On 2015/02/03 19:21:28, dmichael wrote:
> > You're probably going to want this to be more like the MediaStream stuff,
> right?
> > It needs to be availabale to NaCl but whitelisted, I think?
> 
> I guess you're right. How do I get this whitelisted in addition to moving the
> code around defined(OS_NACL)?
The most important part is that it is protected in the renderer side. This CL
shows the places:
https://codereview.chromium.org/346263006

You'll also want to list the API here:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/thunk/interf...
(PPB_FileIO_Private might serve as a good example on the plugin side, because I
think it's currently in use by ARC.)

[Justin Chuang, 2015-02-06 09:10:21]

New patchsets have been uploaded after l-g-t-m from wuchengli@chromium.org

[Justin Chuang, 2015-02-06 18:56:05]

On 2015/02/05 22:39:28, dmichael wrote:
> On 2015/02/04 16:26:01, Justin Chuang wrote:
> > I guess you're right. How do I get this whitelisted in addition to moving
the
> > code around defined(OS_NACL)?
> The most important part is that it is protected in the renderer side. This CL
> shows the places:
> https://codereview.chromium.org/346263006
> 
> You'll also want to list the API here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/thunk/interf...
> (PPB_FileIO_Private might serve as a good example on the plugin side, because
I
> think it's currently in use by ARC.)

Thanks Dave and Wucheng on this, I'm still confused. I'd suggest to do it
(including moving some OS_NACL macro) in a separate CL, does it sound okay?

[dmichael (off chromium), 2015-02-09 22:27:00]

Mostly nits, lgtm. Thanks!

https://codereview.chromium.org/848863002/diff/340001/content/renderer/pepper...
File content/renderer/pepper/pepper_image_capture_host.cc (right):

https://codereview.chromium.org/848863002/diff/340001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
s/2014/2015

https://codereview.chromium.org/848863002/diff/340001/content/renderer/pepper...
File content/renderer/pepper/pepper_image_capture_host.h (right):

https://codereview.chromium.org/848863002/diff/340001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
s/2014/2015
...and in other new files, too.

https://codereview.chromium.org/848863002/diff/340001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.h:43: 
nit: Please remove the extra carriage return

https://codereview.chromium.org/848863002/diff/340001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.h:50: 
ditto

https://codereview.chromium.org/848863002/diff/340001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_image_capture.h (right):

https://codereview.chromium.org/848863002/diff/340001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_image_capture.h:32: virtual
~PepperPlatformImageCapture();
nit: Doesn't seem like this needs to be virtual

https://codereview.chromium.org/848863002/diff/340001/ppapi/cpp/private/image...
File ppapi/cpp/private/image_capture_private.cc (right):

https://codereview.chromium.org/848863002/diff/340001/ppapi/cpp/private/image...
ppapi/cpp/private/image_capture_private.cc:57: return PP_ERROR_NOINTERFACE;
You should use callback.MayForce here and in other C++ methods with completion
callbacks. It will make the callback run asynchronously if it's a callback
that's required to be async.

https://codereview.chromium.org/848863002/diff/340001/ppapi/cpp/private/image...
ppapi/cpp/private/image_capture_private.cc:66: }
nit: no curly braces here, for consistency

https://codereview.chromium.org/848863002/diff/340001/ppapi/cpp/private/image...
File ppapi/cpp/private/image_capture_private.h (right):

https://codereview.chromium.org/848863002/diff/340001/ppapi/cpp/private/image...
ppapi/cpp/private/image_capture_private.h:68: /// @param[in] callback A
<code>PP_CompletionCallback</code> to be called upon
nit:
s/PP_CompletionCallback/CompletionCallback

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/camera_capa...
File ppapi/proxy/camera_capabilities_resource.h (right):

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/camera_capa...
ppapi/proxy/camera_capabilities_resource.h:34: // PPB_CameraCapabilities_API
overrides
s/overrides/implementation:

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.cc (right):

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:8: #include "ppapi/proxy/plugin_globals.h"
unused

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:11: #include
"ppapi/proxy/ppb_buffer_proxy.h"
unused

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:12: #include
"ppapi/shared_impl/proxy_lock.h"
unused

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:61: *get_capabilities_output_ = 0;
nit: Better to just not write the output param if you abort the callback. The
PP_ERROR_ABORTED result code is good enough.

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:87: get_capabilities_callback_ = callback;
As an alternative, you can simply bind these below, and make them parameters to
OnPluginMsgGetPreviewSizesReply. That's an easy way to allow multiple in-flight
calls to GetCameraCapabilities on one ImageCaptureResource. I generally prefer
it because it means storing less state and providing more capability.

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.h (right):

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.h:9: #include "base/memory/scoped_ptr.h"
nit: Not using this header

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.h:28: virtual ~ImageCaptureResource();
This should be override (not marked virtual). Same for
CameraCapabilitiesResource

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.h:31: virtual thunk::PPB_ImageCapture_API*
AsPPB_ImageCapture_API() override {
We should only mark a function virtual OR override; not both. Here you want
override. Same on the PPB_ImageCapture_API overrides.

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.h:41: scoped_refptr<TrackedCallback>
callback) override;
nit: Maybe slightly prefer const scoped_refptr<>&

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.h:51: // reply to open
You could leave out this comment and just put this method right under
OnPluginMsgGetPreviewSizesReply.

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.h:54: // other
unnecessary comment

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.h:55: bool IsOpened() const { return
open_state_ == OPENED; }
nit: I'm used to seeing simple inlined functions like this using lowercase with
underscores...
is_opened

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/ppapi_messa...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/ppapi_messa...
ppapi/proxy/ppapi_messages.h:1558: // ImageCapture_Private, host -> plugin
This and the above "plugin -> host" message aren't really accurate, since you
don't have the requests and replies ordered that way. Please remove those
comments (the message ordering is fine).

[Justin Chuang, 2015-02-10 16:01:03]

Thanks for the reviews.

https://codereview.chromium.org/848863002/diff/340001/content/renderer/pepper...
File content/renderer/pepper/pepper_image_capture_host.cc (right):

https://codereview.chromium.org/848863002/diff/340001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2015/02/09 22:26:59, dmichael wrote:
> s/2014/2015

Done.

https://codereview.chromium.org/848863002/diff/340001/content/renderer/pepper...
File content/renderer/pepper/pepper_image_capture_host.h (right):

https://codereview.chromium.org/848863002/diff/340001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2015/02/09 22:26:59, dmichael wrote:
> s/2014/2015
> ...and in other new files, too.

Done. Only change new files.

https://codereview.chromium.org/848863002/diff/340001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.h:43: 
On 2015/02/09 22:26:59, dmichael wrote:
> nit: Please remove the extra carriage return

Done.

https://codereview.chromium.org/848863002/diff/340001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.h:50: 
On 2015/02/09 22:26:59, dmichael wrote:
> ditto

Done.

https://codereview.chromium.org/848863002/diff/340001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_image_capture.h (right):

https://codereview.chromium.org/848863002/diff/340001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_image_capture.h:32: virtual
~PepperPlatformImageCapture();
On 2015/02/09 22:26:59, dmichael wrote:
> nit: Doesn't seem like this needs to be virtual

Done. Thanks

https://codereview.chromium.org/848863002/diff/340001/ppapi/cpp/private/image...
File ppapi/cpp/private/image_capture_private.cc (right):

https://codereview.chromium.org/848863002/diff/340001/ppapi/cpp/private/image...
ppapi/cpp/private/image_capture_private.cc:57: return PP_ERROR_NOINTERFACE;
On 2015/02/09 22:26:59, dmichael wrote:
> You should use callback.MayForce here and in other C++ methods with completion
> callbacks. It will make the callback run asynchronously if it's a callback
> that's required to be async.

Done.

https://codereview.chromium.org/848863002/diff/340001/ppapi/cpp/private/image...
ppapi/cpp/private/image_capture_private.cc:66: }
On 2015/02/09 22:26:59, dmichael wrote:
> nit: no curly braces here, for consistency

Done.

https://codereview.chromium.org/848863002/diff/340001/ppapi/cpp/private/image...
File ppapi/cpp/private/image_capture_private.h (right):

https://codereview.chromium.org/848863002/diff/340001/ppapi/cpp/private/image...
ppapi/cpp/private/image_capture_private.h:68: /// @param[in] callback A
<code>PP_CompletionCallback</code> to be called upon
On 2015/02/09 22:26:59, dmichael wrote:
> nit:
> s/PP_CompletionCallback/CompletionCallback

Done.

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/camera_capa...
File ppapi/proxy/camera_capabilities_resource.h (right):

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/camera_capa...
ppapi/proxy/camera_capabilities_resource.h:34: // PPB_CameraCapabilities_API
overrides
On 2015/02/09 22:26:59, dmichael wrote:
> s/overrides/implementation:

Done, thanks. But I found most files use 'implementation.' instead of
'implementation:'. So I use the former style.

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.cc (right):

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:8: #include "ppapi/proxy/plugin_globals.h"
On 2015/02/09 22:27:00, dmichael wrote:
> unused

Done.

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:11: #include
"ppapi/proxy/ppb_buffer_proxy.h"
On 2015/02/09 22:27:00, dmichael wrote:
> unused

Done.

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:12: #include
"ppapi/shared_impl/proxy_lock.h"
On 2015/02/09 22:26:59, dmichael wrote:
> unused

Done.

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:61: *get_capabilities_output_ = 0;
On 2015/02/09 22:26:59, dmichael wrote:
> nit: Better to just not write the output param if you abort the callback. The
> PP_ERROR_ABORTED result code is good enough.

Removed this line. Thanks

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:87: get_capabilities_callback_ = callback;
On 2015/02/09 22:26:59, dmichael wrote:
> As an alternative, you can simply bind these below, and make them parameters
to
> OnPluginMsgGetPreviewSizesReply. That's an easy way to allow multiple
in-flight
> calls to GetCameraCapabilities on one ImageCaptureResource. I generally prefer
> it because it means storing less state and providing more capability.
Good point. To do in the next patchset.

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.h (right):

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.h:9: #include "base/memory/scoped_ptr.h"
On 2015/02/09 22:27:00, dmichael wrote:
> nit: Not using this header

Done.

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.h:28: virtual ~ImageCaptureResource();
On 2015/02/09 22:27:00, dmichael wrote:
> This should be override (not marked virtual). Same for
> CameraCapabilitiesResource

Done. Thank you! I haven't noticed this rule also applies to virtual destructor.

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.h:31: virtual thunk::PPB_ImageCapture_API*
AsPPB_ImageCapture_API() override {
On 2015/02/09 22:27:00, dmichael wrote:
> We should only mark a function virtual OR override; not both. Here you want
> override. Same on the PPB_ImageCapture_API overrides.

Done in this file and the other files.

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.h:41: scoped_refptr<TrackedCallback>
callback) override;
On 2015/02/09 22:27:00, dmichael wrote:
> nit: Maybe slightly prefer const scoped_refptr<>&

Done.

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.h:51: // reply to open
On 2015/02/09 22:27:00, dmichael wrote:
> You could leave out this comment and just put this method right under
> OnPluginMsgGetPreviewSizesReply.

Removed the comments and blank line.

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.h:54: // other
On 2015/02/09 22:27:00, dmichael wrote:
> unnecessary comment

Done.

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.h:55: bool IsOpened() const { return
open_state_ == OPENED; }
On 2015/02/09 22:27:00, dmichael wrote:
> nit: I'm used to seeing simple inlined functions like this using lowercase
with
> underscores...
> is_opened 

It's true. Done.

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/ppapi_messa...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/ppapi_messa...
ppapi/proxy/ppapi_messages.h:1558: // ImageCapture_Private, host -> plugin
On 2015/02/09 22:27:00, dmichael wrote:
> This and the above "plugin -> host" message aren't really accurate, since you
> don't have the requests and replies ordered that way. Please remove those
> comments (the message ordering is fine).

Removed the comments. 

But I don't really understand. I think the requests and replies are indeed
'plugin -> host -> plugin' as in above comments?

[Justin Chuang, 2015-02-10 16:02:23]

New patchsets have been uploaded after l-g-t-m from dmichael@chromium.org

[dmichael (off chromium), 2015-02-10 16:06:59]

found one more tiny nit; still lgtm

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/ppapi_messa...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/ppapi_messa...
ppapi/proxy/ppapi_messages.h:1554:
IPC_MESSAGE_CONTROL0(PpapiPluginMsg_ImageCapture_GetSupportedPreviewSizes)
nit: This should be PpapiHostMsg_...
sorry I missed it before.

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/ppapi_messa...
ppapi/proxy/ppapi_messages.h:1558: // ImageCapture_Private, host -> plugin
On 2015/02/10 16:01:03, Justin Chuang wrote:
> On 2015/02/09 22:27:00, dmichael wrote:
> > This and the above "plugin -> host" message aren't really accurate, since
you
> > don't have the requests and replies ordered that way. Please remove those
> > comments (the message ordering is fine).
> 
> Removed the comments. 
> 
> But I don't really understand. I think the requests and replies are indeed
> 'plugin -> host -> plugin' as in above comments?
I see, I missed the comment on 1549. This last comment threw me off. I still
think the comments were probably more confusing than helpful, so thanks for
removing them.

[Justin Chuang, 2015-02-10 17:44:09]

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.cc (right):

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:87: get_capabilities_callback_ = callback;
On 2015/02/10 16:01:02, Justin Chuang wrote:
> On 2015/02/09 22:26:59, dmichael wrote:
> > As an alternative, you can simply bind these below, and make them parameters
> to
> > OnPluginMsgGetPreviewSizesReply. That's an easy way to allow multiple
> in-flight
> > calls to GetCameraCapabilities on one ImageCaptureResource. I generally
prefer
> > it because it means storing less state and providing more capability.
> Good point. To do in the next patchset.

Done in Patch Set 18. PTAL.

Note that I still don't allow multiple in-flight calls. The
CameraCapabilitiesResource object will be cached once it's created, so easier to
forbid multiple in-flights. In addition, in current code, returning
PP_ERROR_INPROGRESS in render host layer is simpler than adding multiple reply
context there.

However, It's still nice to reduce some states.

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/ppapi_messa...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/848863002/diff/340001/ppapi/proxy/ppapi_messa...
ppapi/proxy/ppapi_messages.h:1554:
IPC_MESSAGE_CONTROL0(PpapiPluginMsg_ImageCapture_GetSupportedPreviewSizes)
On 2015/02/10 16:06:59, dmichael wrote:
> nit: This should be PpapiHostMsg_...
> sorry I missed it before.

Done. Sorry I missed it.

[Justin Chuang, 2015-02-10 17:44:57]

New patchsets have been uploaded after l-g-t-m from dmichael@chromium.org

[Justin Chuang, 2015-02-10 17:56:39]

A question:

https://codereview.chromium.org/848863002/diff/380001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.cc (right):

https://codereview.chromium.org/848863002/diff/380001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:56: 
Note that it doesn't handle a race condition because I can no longer abort the
callback in Close().

1. Call ImageCaptureResource::GetCameraCapabilities()
2. Call ImageCaptureResource::Close()
3. OnPluginMsgGetPreviewSizesReply() is invoked, but this object is already
destroyed because I use Unretained() to this.

But I don't find reference code in ppapi/proxy handling it. Most seem to assume
the caller doesn't mess up.

Wonder if we should use WeakPtr to this in ::GetCameraCapabilities()

[dmichael (off chromium), 2015-02-10 18:12:33]

https://codereview.chromium.org/848863002/diff/380001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.cc (right):

https://codereview.chromium.org/848863002/diff/380001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:56: 
On 2015/02/10 17:56:38, Justin Chuang wrote:
> Note that it doesn't handle a race condition because I can no longer abort the
> callback in Close().
> 
> 1. Call ImageCaptureResource::GetCameraCapabilities()
> 2. Call ImageCaptureResource::Close()
> 3. OnPluginMsgGetPreviewSizesReply() is invoked, but this object is already
> destroyed because I use Unretained() to this.
Oh, thanks for bringing that up. I wasn't thinking about Close() aborting the
callback. That's the behavior we would usually do for a "Close" method, so maybe
the callback really should be a member, just so that you can abort it at the
right time. Sorry about that.

> 
> But I don't find reference code in ppapi/proxy handling it. Most seem to
assume
> the caller doesn't mess up.
> 
> Wonder if we should use WeakPtr to this in ::GetCameraCapabilities()
Unretained is safe for reply handlers. This comment mentions it:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/plugin...
...though I realize now it's not explained that well. Really, it's because the
reply message isn't dispatched to a deleted resource:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/plugin...

[Justin Chuang, 2015-02-10 19:02:08]

https://codereview.chromium.org/848863002/diff/380001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.cc (right):

https://codereview.chromium.org/848863002/diff/380001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:56: 
On 2015/02/10 18:12:32, dmichael wrote:
> On 2015/02/10 17:56:38, Justin Chuang wrote:
> > Note that it doesn't handle a race condition because I can no longer abort
the
> > callback in Close().
> > 
> > 1. Call ImageCaptureResource::GetCameraCapabilities()
> > 2. Call ImageCaptureResource::Close()
> > 3. OnPluginMsgGetPreviewSizesReply() is invoked, but this object is already
> > destroyed because I use Unretained() to this.
> Oh, thanks for bringing that up. I wasn't thinking about Close() aborting the
> callback. That's the behavior we would usually do for a "Close" method, so
maybe
> the callback really should be a member, just so that you can abort it at the
> right time. Sorry about that.
> 
Do you think it's worth the extra code to just for implementing the callback
abortion when Close()? 

I like the new method much more than the old one.  :)

If the abortion is recommended, I'll revert it back. 

> > 
> > Wonder if we should use WeakPtr to this in ::GetCameraCapabilities()
> Unretained is safe for reply handlers. This comment mentions it:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/plugin...
> ...though I realize now it's not explained that well. Really, it's because the
> reply message isn't dispatched to a deleted resource:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/plugin...
Sounds great. Thanks for explanation.

[dmichael (off chromium), 2015-02-10 19:07:47]

https://codereview.chromium.org/848863002/diff/380001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.cc (right):

https://codereview.chromium.org/848863002/diff/380001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:56: 
On 2015/02/10 19:02:08, Justin Chuang wrote:
> On 2015/02/10 18:12:32, dmichael wrote:
> > On 2015/02/10 17:56:38, Justin Chuang wrote:
> > > Note that it doesn't handle a race condition because I can no longer abort
> the
> > > callback in Close().
> > > 
> > > 1. Call ImageCaptureResource::GetCameraCapabilities()
> > > 2. Call ImageCaptureResource::Close()
> > > 3. OnPluginMsgGetPreviewSizesReply() is invoked, but this object is
already
> > > destroyed because I use Unretained() to this.
> > Oh, thanks for bringing that up. I wasn't thinking about Close() aborting
the
> > callback. That's the behavior we would usually do for a "Close" method, so
> maybe
> > the callback really should be a member, just so that you can abort it at the
> > right time. Sorry about that.
> > 
> Do you think it's worth the extra code to just for implementing the callback
> abortion when Close()? 
> 
> I like the new method much more than the old one.  :)
> 
> If the abortion is recommended, I'll revert it back. 
Yes, I would recommend that. It's our convention for Close() methods on Pepper
resources. I'd prefer this class didn't have a Close that worked differently.

[Justin Chuang, 2015-02-11 03:35:29]

On 2015/02/10 19:07:47, dmichael wrote:
>
https://codereview.chromium.org/848863002/diff/380001/ppapi/proxy/image_captu...
> File ppapi/proxy/image_capture_resource.cc (right):
> 
>
https://codereview.chromium.org/848863002/diff/380001/ppapi/proxy/image_captu...
> ppapi/proxy/image_capture_resource.cc:56: 
> On 2015/02/10 19:02:08, Justin Chuang wrote:
> > Do you think it's worth the extra code to just for implementing the callback
> > abortion when Close()? 
> > 
> > I like the new method much more than the old one.  :)
> > 
> > If the abortion is recommended, I'll revert it back. 
> Yes, I would recommend that. It's our convention for Close() methods on Pepper
> resources. I'd prefer this class didn't have a Close that worked differently.

Done. Callback object is kept as class member. Can refactor output variable as
argument of callback routine. Thanks

[Justin Chuang, 2015-02-11 03:36:20]

The CQ bit was checked by jchuang@chromium.org

[commit-bot: I haz the power, 2015-02-11 03:37:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/848863002/440001

[commit-bot: I haz the power, 2015-02-11 03:44:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-11 03:44:20]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Justin Chuang, 2015-02-11 07:41:04]

jchuang@chromium.org changed reviewers:
+ dcheng@chromium.org

[Justin Chuang, 2015-02-11 07:41:05]

dcheng: Please OWNERS review changes in ppapi/proxy/ppapi_messages.h. Thanks

[Justin Chuang, 2015-02-11 08:24:31]

jchuang@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Justin Chuang, 2015-02-11 08:24:33]

Alexei, lease OWNERS review changes in tools/metrics/histograms/histograms.xml.
Thanks

[Alexei Svitkine (slow), 2015-02-11 15:24:01]

lgtm

[dcheng, 2015-02-12 21:47:20]

https://codereview.chromium.org/848863002/diff/480001/content/renderer/pepper...
File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right):

https://codereview.chromium.org/848863002/diff/480001/content/renderer/pepper...
content/renderer/pepper/content_renderer_pepper_host_factory.cc:210: return
host->Init() ? scoped_ptr<ResourceHost>(host.Pass())
return host->Init() ? host : nullptr;

should work.

https://codereview.chromium.org/848863002/diff/480001/content/renderer/pepper...
File content/renderer/pepper/pepper_image_capture_host.cc (right):

https://codereview.chromium.org/848863002/diff/480001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.cc:36:
PPAPI_DISPATCH_HOST_RESOURCE_CALL(PpapiHostMsg_ImageCapture_Open, OnOpen)
Mind indenting the lines between BEGIN_MESSAGE_MAP and END_MESSAGE_MAP manually?
clang-format doesn't understand the convention for this just yet =(

https://codereview.chromium.org/848863002/diff/480001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_image_capture.cc (right):

https://codereview.chromium.org/848863002/diff/480001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_image_capture.cc:114: for
(media::VideoCaptureFormats::const_iterator it = formats.begin();
Consider using a for each?

for (const auto& format : formats)
  sizes.push_back(PP_FromGfxSize(it->frame_size));

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/camera_capa...
File ppapi/proxy/camera_capabilities_resource.cc (right):

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/camera_capa...
ppapi/proxy/camera_capabilities_resource.cc:15: num_preview_sizes_ =
preview_sizes.size();
Put lines 15-16 in the initializer list?

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.cc (right):

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:35: source_string_var =
StringVar::FromPPVar(device_id);
Nit: combine declaration and initialization

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:44: base::Unretained(this)));
Why is it OK to use base::Unretained() here? A roughly corresponding IPC
elsewhere does not use base::Unretained:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/video_...

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:54: open_callback_ = NULL;
Use nullptr instead of NULL.

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:57: if
(TrackedCallback::IsPending(get_capabilities_callback_)) {
This doesn't seem scalable. Are we going to need to manually cancel each pending
callback as we keep adding callbacks?

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:85: base::Unretained(this),
capabilities));
Ditto--why is base::Unretained() safe here?

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:95: if
(TrackedCallback::IsPending(open_callback_))
Shouldn't this happen before we update open_state_ ?

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.h (right):

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.h:43: enum OpenState { BEFORE_OPEN, OPENED,
CLOSED };
Prefer enum class.

[Justin Chuang, 2015-02-16 19:15:24]

Thanks Daniel for spending time reviewing again.

https://codereview.chromium.org/848863002/diff/480001/content/renderer/pepper...
File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right):

https://codereview.chromium.org/848863002/diff/480001/content/renderer/pepper...
content/renderer/pepper/content_renderer_pepper_host_factory.cc:210: return
host->Init() ? scoped_ptr<ResourceHost>(host.Pass())
On 2015/02/12 21:47:20, dcheng wrote:
> return host->Init() ? host : nullptr;
> 
> should work.

Actually no, it needs to upcast to ResourceHost in scoped_ptr. There are several
places of similar code in this file.

https://codereview.chromium.org/848863002/diff/480001/content/renderer/pepper...
File content/renderer/pepper/pepper_image_capture_host.cc (right):

https://codereview.chromium.org/848863002/diff/480001/content/renderer/pepper...
content/renderer/pepper/pepper_image_capture_host.cc:36:
PPAPI_DISPATCH_HOST_RESOURCE_CALL(PpapiHostMsg_ImageCapture_Open, OnOpen)
On 2015/02/12 21:47:20, dcheng wrote:
> Mind indenting the lines between BEGIN_MESSAGE_MAP and END_MESSAGE_MAP
manually?
> clang-format doesn't understand the convention for this just yet =(

Done. Thanks

https://codereview.chromium.org/848863002/diff/480001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_image_capture.cc (right):

https://codereview.chromium.org/848863002/diff/480001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_image_capture.cc:114: for
(media::VideoCaptureFormats::const_iterator it = formats.begin();
On 2015/02/12 21:47:20, dcheng wrote:
> Consider using a for each?
> 
> for (const auto& format : formats)
>   sizes.push_back(PP_FromGfxSize(it->frame_size));

Done. Interesting.

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/camera_capa...
File ppapi/proxy/camera_capabilities_resource.cc (right):

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/camera_capa...
ppapi/proxy/camera_capabilities_resource.cc:15: num_preview_sizes_ =
preview_sizes.size();
On 2015/02/12 21:47:20, dcheng wrote:
> Put lines 15-16 in the initializer list?

Done.

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.cc (right):

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:35: source_string_var =
StringVar::FromPPVar(device_id);
On 2015/02/12 21:47:20, dcheng wrote:
> Nit: combine declaration and initialization

Done. Thx

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:44: base::Unretained(this)));
On 2015/02/12 21:47:20, dcheng wrote:
> Why is it OK to use base::Unretained() here? A roughly corresponding IPC
> elsewhere does not use base::Unretained:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/video_...

Actually I see most code use Unretained(this), video_capture_resource is not the
majority case. 
Unretained is safe here. It's also not recommended to let a callback to hold
reference to PluginResource from the API.

Please check:

1.
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/plugin...
2.
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/plugin...

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:54: open_callback_ = NULL;
On 2015/02/12 21:47:20, dcheng wrote:
> Use nullptr instead of NULL.

Done.

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:57: if
(TrackedCallback::IsPending(get_capabilities_callback_)) {
On 2015/02/12 21:47:20, dcheng wrote:
> This doesn't seem scalable. Are we going to need to manually cancel each
pending
> callback as we keep adding callbacks?

Hmm... There are only two. For now, I prefer not to refactor too early.

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:85: base::Unretained(this),
capabilities));
On 2015/02/12 21:47:20, dcheng wrote:
> Ditto--why is base::Unretained() safe here?

Ditto

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:95: if
(TrackedCallback::IsPending(open_callback_))
On 2015/02/12 21:47:20, dcheng wrote:
> Shouldn't this happen before we update open_state_ ?

No, I think the state should be updated before invoking the completion callback
of Open().

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.h (right):

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.h:43: enum OpenState { BEFORE_OPEN, OPENED,
CLOSED };
On 2015/02/12 21:47:20, dcheng wrote:
> Prefer enum class.

Done. Thanks. Looks better.

[Justin Chuang, 2015-02-16 19:24:10]

New patchsets have been uploaded after l-g-t-m from asvitkine@chromium.org

[dcheng, 2015-02-16 19:24:23]

https://codereview.chromium.org/848863002/diff/480001/content/renderer/pepper...
File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right):

https://codereview.chromium.org/848863002/diff/480001/content/renderer/pepper...
content/renderer/pepper/content_renderer_pepper_host_factory.cc:210: return
host->Init() ? scoped_ptr<ResourceHost>(host.Pass())
On 2015/02/16 19:15:23, Justin Chuang wrote:
> On 2015/02/12 21:47:20, dcheng wrote:
> > return host->Init() ? host : nullptr;
> > 
> > should work.
> 
> Actually no, it needs to upcast to ResourceHost in scoped_ptr. There are
several
> places of similar code in this file.

return host->Init() ? host.Pass() : nullptr;

should still work.

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.cc (right):

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:95: if
(TrackedCallback::IsPending(open_callback_))
On 2015/02/16 19:15:24, Justin Chuang wrote:
> On 2015/02/12 21:47:20, dcheng wrote:
> > Shouldn't this happen before we update open_state_ ?
> 
> No, I think the state should be updated before invoking the completion
callback
> of Open().

Let's say I call Open(), and then I call Close() before the initial Open()
completes.

Should open_state_ be CLOSED or OPENED?

[Justin Chuang, 2015-02-16 19:54:22]

https://codereview.chromium.org/848863002/diff/480001/content/renderer/pepper...
File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right):

https://codereview.chromium.org/848863002/diff/480001/content/renderer/pepper...
content/renderer/pepper/content_renderer_pepper_host_factory.cc:210: return
host->Init() ? scoped_ptr<ResourceHost>(host.Pass())
On 2015/02/16 19:24:22, dcheng wrote:
> On 2015/02/16 19:15:23, Justin Chuang wrote:
> > On 2015/02/12 21:47:20, dcheng wrote:
> > > return host->Init() ? host : nullptr;
> > > 
> > > should work.
> > 
> > Actually no, it needs to upcast to ResourceHost in scoped_ptr. There are
> several
> > places of similar code in this file.
> 
> return host->Init() ? host.Pass() : nullptr;
> 
> should still work.

You're right. Pass() properly handles upcast. cl upload again.

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.cc (right):

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:95: if
(TrackedCallback::IsPending(open_callback_))
On 2015/02/16 19:24:22, dcheng wrote:
> On 2015/02/16 19:15:24, Justin Chuang wrote:
> > On 2015/02/12 21:47:20, dcheng wrote:
> > > Shouldn't this happen before we update open_state_ ?
> > 
> > No, I think the state should be updated before invoking the completion
> callback
> > of Open().
> 
> Let's say I call Open(), and then I call Close() before the initial Open()
> completes.
> 
> Should open_state_ be CLOSED or OPENED?

It will be CLOSED
1. Close() set it to CLOSED and abort the open_callback_

When renderer replies:
2.a. This object can have been deleted, then OnPluginMsgOpenReply() won't be
invoked by PluginMessageFilter::DispatchResourceReply()

2.b. If this object is not deleted after Close(), OnPluginMsgOpenReply will
still be triggered, but it won't change open_state_ due to above check
(open_state == BEFORE_OPEN).

[dcheng, 2015-02-16 20:00:14]

ipc changes lgtm

I'd prefer the issue about callback cancellation consistency to be addressed,
but I'll leave it up to your discretion.

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/image_captu...
File ppapi/proxy/image_capture_resource.cc (right):

https://codereview.chromium.org/848863002/diff/480001/ppapi/proxy/image_captu...
ppapi/proxy/image_capture_resource.cc:95: if
(TrackedCallback::IsPending(open_callback_))
On 2015/02/16 19:54:22, Justin Chuang wrote:
> On 2015/02/16 19:24:22, dcheng wrote:
> > On 2015/02/16 19:15:24, Justin Chuang wrote:
> > > On 2015/02/12 21:47:20, dcheng wrote:
> > > > Shouldn't this happen before we update open_state_ ?
> > > 
> > > No, I think the state should be updated before invoking the completion
> > callback
> > > of Open().
> > 
> > Let's say I call Open(), and then I call Close() before the initial Open()
> > completes.
> > 
> > Should open_state_ be CLOSED or OPENED?
> 
> It will be CLOSED
> 1. Close() set it to CLOSED and abort the open_callback_
> 
> When renderer replies:
> 2.a. This object can have been deleted, then OnPluginMsgOpenReply() won't be
> invoked by PluginMessageFilter::DispatchResourceReply()
> 
> 2.b. If this object is not deleted after Close(), OnPluginMsgOpenReply will
> still be triggered, but it won't change open_state_ due to above check
> (open_state == BEFORE_OPEN). 

As a general pattern, and for consistency, I think it's best for callbacks to
check if they've been aborted before doing any work (after all, this is what the
other callback this class uses does).

[Justin Chuang, 2015-02-16 20:14:15]

On 2015/02/16 20:00:14, dcheng wrote:
> > 2.b. If this object is not deleted after Close(), OnPluginMsgOpenReply will
> > still be triggered, but it won't change open_state_ due to above check
> > (open_state == BEFORE_OPEN). 
> 
> As a general pattern, and for consistency, I think it's best for callbacks to
> check if they've been aborted before doing any work (after all, this is what
the
> other callback this class uses does).

Thanks. I see what you mean now. Will fix this part.

[Justin Chuang, 2015-02-16 20:16:22]

New patchsets have been uploaded after l-g-t-m from dcheng@chromium.org

[Justin Chuang, 2015-02-16 20:47:51]

The CQ bit was checked by jchuang@chromium.org

[commit-bot: I haz the power, 2015-02-16 20:48:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/848863002/560001

[commit-bot: I haz the power, 2015-02-16 21:00:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-16 21:00:24]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Justin Chuang, 2015-02-17 03:30:10]

The CQ bit was checked by jchuang@chromium.org

[commit-bot: I haz the power, 2015-02-17 03:31:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/848863002/580001

[commit-bot: I haz the power, 2015-02-17 04:36:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-17 04:36:17]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Justin Chuang, 2015-02-17 09:01:19]

The CQ bit was checked by jchuang@chromium.org

[commit-bot: I haz the power, 2015-02-17 09:02:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/848863002/620001

[commit-bot: I haz the power, 2015-02-17 10:11:17]

Committed patchset #30 (id:620001)

[commit-bot: I haz the power, 2015-02-17 10:12:15]

Patchset 30 (id:??) landed as
https://crrev.com/add2f05d32384647371c56bba67bec3fc2f1ee2f
Cr-Commit-Position: refs/heads/master@{#316552}