PPAPI: implement GetSupportedPreviewSizes

ppapi/proxy/image_capture_resource.cc

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ppapi/proxy/image_capture_resource.h"
+
+#include "ppapi/proxy/camera_capabilities_resource.h"
+#include "ppapi/proxy/plugin_resource_tracker.h"
+#include "ppapi/proxy/ppapi_messages.h"
+#include "ppapi/shared_impl/var.h"
+
+namespace ppapi {
+namespace proxy {
+
+ImageCaptureResource::ImageCaptureResource(Connection connection,
+                                           PP_Instance instance)
+    : PluginResource(connection, instance),
+      open_state_(BEFORE_OPEN) {
+  SendCreate(RENDERER, PpapiHostMsg_ImageCapture_Create());
+}
+
+ImageCaptureResource::~ImageCaptureResource() {
+}
+
+int32_t ImageCaptureResource::Open(
+    PP_Var device_id,
+    const scoped_refptr<TrackedCallback>& callback) {
+  if (open_state_ != BEFORE_OPEN)
+    return PP_ERROR_FAILED;
+
+  if (TrackedCallback::IsPending(open_callback_))
+    return PP_ERROR_INPROGRESS;
+
+  scoped_refptr<StringVar> source_string_var;
+  source_string_var = StringVar::FromPPVar(device_id);

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

Nit: combine declaration and initialization

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

Done. Thx

+  if (!source_string_var || source_string_var->value().empty())
+    return PP_ERROR_BADARGUMENT;
+
+  open_callback_ = callback;
+
+  Call<PpapiPluginMsg_ImageCapture_OpenReply>(
+      RENDERER, PpapiHostMsg_ImageCapture_Open(source_string_var->value()),
+      base::Bind(&ImageCaptureResource::OnPluginMsgOpenReply,
+                 base::Unretained(this)));

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

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_...

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

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...

+  return PP_OK_COMPLETIONPENDING;
+}
+
+void ImageCaptureResource::Close() {
+  if (open_state_ == CLOSED)
+    return;
+
+  if (TrackedCallback::IsPending(open_callback_)) {
+    open_callback_->PostAbort();
+    open_callback_ = NULL;

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

Use nullptr instead of NULL.

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

Done.

+  }
+
+  if (TrackedCallback::IsPending(get_capabilities_callback_)) {

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

This doesn't seem scalable. Are we going to need to manually cancel each pending
callback as we keep adding callbacks?

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

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

+    get_capabilities_callback_->PostAbort();
+    get_capabilities_callback_ = NULL;
+  }
+
+  Post(RENDERER, PpapiHostMsg_ImageCapture_Close());
+
+  open_state_ = CLOSED;
+}
+
+int32_t ImageCaptureResource::GetCameraCapabilities(
+    PP_Resource* capabilities,
+    const scoped_refptr<TrackedCallback>& callback) {
+  if (!is_opened())
+    return PP_ERROR_FAILED;
+
+  if (TrackedCallback::IsPending(get_capabilities_callback_))
+    return PP_ERROR_INPROGRESS;
+
+  if (camera_capabilities_.get()) {
+    *capabilities = camera_capabilities_->GetReference();
+    return PP_OK;
+  }
+
+  get_capabilities_callback_ = callback;
+  Call<PpapiPluginMsg_ImageCapture_GetSupportedPreviewSizesReply>(
+      RENDERER, PpapiHostMsg_ImageCapture_GetSupportedPreviewSizes(),
+      base::Bind(&ImageCaptureResource::OnPluginMsgGetPreviewSizesReply,
+                 base::Unretained(this), capabilities));

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

Ditto--why is base::Unretained() safe here?

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

Ditto

+  return PP_OK_COMPLETIONPENDING;
+}
+
+void ImageCaptureResource::OnPluginMsgOpenReply(
+    const ResourceMessageReplyParams& params) {
+  if (open_state_ == BEFORE_OPEN && params.result() == PP_OK)
+    open_state_ = OPENED;
+
+  // The callback may have been aborted by Close().
+  if (TrackedCallback::IsPending(open_callback_))

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

Shouldn't this happen before we update open_state_ ?

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

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

[dcheng, 2015/02/16 19:24:22]

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]

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]

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).

+    open_callback_->Run(params.result());
+}
+
+void ImageCaptureResource::OnPluginMsgGetPreviewSizesReply(
+    PP_Resource* capabilities_output,
+    const ResourceMessageReplyParams& params,
+    const std::vector<PP_Size>& preview_sizes) {
+  if (!TrackedCallback::IsPending(get_capabilities_callback_))
+    return;
+
+  // Return camera capabilities.
+  int32_t result = params.result();
+  scoped_refptr<TrackedCallback> callback;
+  callback.swap(get_capabilities_callback_);
+  if (result == PP_OK) {
+    camera_capabilities_ =
+        new CameraCapabilitiesResource(pp_instance(), preview_sizes);
+    *capabilities_output = camera_capabilities_->GetReference();
+  }
+  callback->Run(result == PP_OK ? PP_OK : PP_ERROR_FAILED);
+}
+
+}  // namespace proxy
+}  // namespace ppapi