ScreenCapture for Android phase1, part I

content/browser/media/capture/screen_capture_device_android.cc

+// Copyright 2016 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 "content/browser/media/capture/screen_capture_device_android.h"
+
+#include "base/logging.h"
+#include "base/memory/ptr_util.h"
+#include "media/capture/content/android/screen_capture_machine_android.h"
+
+namespace content {
+
+ScreenCaptureDeviceAndroid::ScreenCaptureDeviceAndroid()
+    : core_(new media::ScreenCaptureDeviceCore(
+          base::WrapUnique(new media::ScreenCaptureMachineAndroid()))) {}
+
+ScreenCaptureDeviceAndroid::~ScreenCaptureDeviceAndroid() {
+  DVLOG(2) << "ScreenCaptureDeviceAndroid@" << this << " destroying.";
+}
+
+// static
+std::unique_ptr<media::VideoCaptureDevice>
+ScreenCaptureDeviceAndroid::Create() {
+  return std::unique_ptr<media::VideoCaptureDevice>(

[mcasas, 2016/05/19 19:11:39]

Consider base::WrapUnique(), however see my 
next comment.

[braveyao, 2016/05/20 22:27:24]

Done.

+      new ScreenCaptureDeviceAndroid());

[mcasas, 2016/05/19 19:11:39]

If we're just returning a new ScreenCaptureDeviceAndroid,
you could just remove this method altogether and have it
created directly at the caller site.

[braveyao, 2016/05/20 22:27:24]

In video_capture_manager.cc, all other capture instances are created by a
'Create' factory method. So just keep the consistence here too :)

+}
+
+void ScreenCaptureDeviceAndroid::AllocateAndStart(
+    const media::VideoCaptureParams& params,
+    std::unique_ptr<Client> client) {
+  DVLOG(1) << "Allocating " << params.requested_format.frame_size.ToString();
+  core_->AllocateAndStart(params, std::move(client));
+
+  power_save_blocker_.reset(
+      content::PowerSaveBlocker::Create(
+          content::PowerSaveBlocker::kPowerSaveBlockPreventDisplaySleep,
+          content::PowerSaveBlocker::kReasonOther,
+          "ScreenCaptureMachineAndroid is running")
+          .release());

[mcasas, 2016/05/19 19:11:39]

Strange somersault? Consider:

power_save_blocker_ = std::move(PowerSaverBlocker::Create(...));

Also, no need for content:: since you are in that
namespace.

[braveyao, 2016/05/20 22:27:24]

Done.

I see "power_save_blocker_ = PowerSaverBlocker::Create(...)" and
"power_save_blocker_.reset(PowerSaverBlocker::Create(...))" in half-half chance.
Seem std::move can't be applied here.

[mcasas, 2016/05/23 18:19:55]

I don't understand this response.

[braveyao, 2016/05/24 00:03:26]

Some people use "power_save_blocker_.reset(PowerSaveBlocker::Create(...))", some
people use "power_save_blocker_ = PowerSaverBlocker::Create(...)". I'm OK with
either of them.
And adding std::move() will cause compile failure, "moving a temporary object
prevents copy elision". So I don't use it here, as others do.

+}
+
+void ScreenCaptureDeviceAndroid::StopAndDeAllocate() {
+  power_save_blocker_.reset();
+  core_->StopAndDeAllocate();
+}
+
+void ScreenCaptureDeviceAndroid::RequestRefreshFrame() {
+  core_->RequestRefreshFrame();
+}
+}  // namespace content