Revert of webcamPrivate API: Fix regression from r351343. (patchset #7 id:130001 of https://codereview.chromium.org/1412673007/ )

Reason for revert:
Suspected of breaking Chrome OS compilation.

http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20GN%20%28dbg%29/builds/11367/steps/compile/logs/stdio

Original issue's description:
> webcamPrivate API: Fix regression from r351343.
>
> Additionally:
> - Add unit test to test for this regression.
> - Modify classes to support VISCA webcam unit tests.
> - Modify VISCA webcam code to simplify code and to be more robust
>   against bad responses.
>
> BUG=548043
>
> Committed: https://crrev.com/d088b80e0203d6f819a714d39ef87221a02549f9
> Cr-Commit-Position: refs/heads/master@{#356910}

TBR=xdai@chromium.org,rockot@chromium.org,thestig@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=548043

Review URL: https://codereview.chromium.org/1428803003

Cr-Commit-Position: refs/heads/master@{#356920}

4 files changed, 88 insertions, 279 deletions

diff --git a/extensions/browser/api/webcam_private/visca_webcam.cc b/extensions/browser/api/webcam_private/visca_webcam.cc
index 5eb4f02..d3cf70f 100644
--- a/extensions/browser/api/webcam_private/visca_webcam.cc
+++ b/extensions/browser/api/webcam_private/visca_webcam.cc
@@ -95,11 +95,6 @@ int ShiftResponseLowerBits(char c, size_t shift) {
   return static_cast<int>(c & 0x0F) << shift;
 }
 
-bool CanBuildResponseInt(const std::vector<char>& response,
-                         size_t start_index) {
-  return response.size() >= start_index + 4;
-}
-
 int BuildResponseInt(const std::vector<char>& response, size_t start_index) {
   return ShiftResponseLowerBits(response[start_index], 12) +
          ShiftResponseLowerBits(response[start_index + 1], 8) +
@@ -131,23 +126,25 @@ int GetPositiveValue(int value) {
 
 namespace extensions {
 
-ViscaWebcam::ViscaWebcam() : pan_(0), tilt_(0), weak_ptr_factory_(this) {}
+ViscaWebcam::ViscaWebcam(const std::string& path,
+                         const std::string& extension_id)
+    : path_(path),
+      extension_id_(extension_id),
+      pan_(0),
+      tilt_(0),
+      weak_ptr_factory_(this) {}
 
 ViscaWebcam::~ViscaWebcam() {
 }
 
-void ViscaWebcam::Open(const std::string& path,
-                       const std::string& extension_id,
-                       const OpenCompleteCallback& open_callback) {
+void ViscaWebcam::Open(const OpenCompleteCallback& open_callback) {
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&ViscaWebcam::OpenOnIOThread, weak_ptr_factory_.GetWeakPtr(),
-                 path, extension_id, open_callback));
+                 open_callback));
 }
 
-void ViscaWebcam::OpenOnIOThread(const std::string& path,
-                                 const std::string& extension_id,
-                                 const OpenCompleteCallback& open_callback) {
+void ViscaWebcam::OpenOnIOThread(const OpenCompleteCallback& open_callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   api::serial::ConnectionOptions options;
@@ -164,7 +161,7 @@ void ViscaWebcam::OpenOnIOThread(const std::string& path,
   options.parity_bit = api::serial::PARITY_BIT_NO;
   options.stop_bits = api::serial::STOP_BITS_ONE;
 
-  serial_connection_.reset(new SerialConnection(path, extension_id));
+  serial_connection_.reset(new SerialConnection(path_, extension_id_));
   serial_connection_->Open(
       options, base::Bind(&ViscaWebcam::OnConnected,
                           weak_ptr_factory_.GetWeakPtr(), open_callback));
@@ -224,7 +221,6 @@ void ViscaWebcam::Send(const std::vector<char>& command,
 void ViscaWebcam::OnSendCompleted(const CommandCompleteCallback& callback,
                                   int bytes_sent,
                                   api::serial::SendError error) {
-  // TODO(xdai): Check |bytes_sent|?
   if (error == api::serial::SEND_ERROR_NONE) {
     ReceiveLoop(callback);
   } else {
@@ -243,39 +239,34 @@ void ViscaWebcam::OnReceiveCompleted(const CommandCompleteCallback& callback,
                                      api::serial::ReceiveError error) {
   data_buffer_.insert(data_buffer_.end(), data.begin(), data.end());
 
-  if (error != api::serial::RECEIVE_ERROR_NONE || data_buffer_.empty()) {
+  if (error == api::serial::RECEIVE_ERROR_NONE) {
+    // Loop until encounter the terminator.
+    if (static_cast<int>(data_buffer_.back()) != kViscaTerminator) {
+      base::MessageLoop::current()->PostTask(
+          FROM_HERE, base::Bind(&ViscaWebcam::ReceiveLoop,
+                                weak_ptr_factory_.GetWeakPtr(), callback));
+    } else {
+      // Clear |data_buffer_|.
+      std::vector<char> response;
+      response.swap(data_buffer_);
+
+      if ((static_cast<int>(response[1]) & 0xF0) == kViscaResponseError) {
+        callback.Run(false, response);
+      } else if ((static_cast<int>(response[1]) & 0xF0) != kViscaResponseAck &&
+                 (static_cast<int>(response[1]) & 0xFF) !=
+                     kViscaResponseNetworkChange) {
+        callback.Run(true, response);
+      } else {
+        base::MessageLoop::current()->PostTask(
+            FROM_HERE, base::Bind(&ViscaWebcam::ReceiveLoop,
+                                  weak_ptr_factory_.GetWeakPtr(), callback));
+      }
+    }
+  } else {
     // Clear |data_buffer_|.
     std::vector<char> response;
     response.swap(data_buffer_);
     callback.Run(false, response);
-    return;
-  }
-
-  // Success case. If waiting for more data, then loop until encounter the
-  // terminator.
-  if (data_buffer_.back() != kViscaTerminator) {
-    base::MessageLoop::current()->PostTask(
-        FROM_HERE, base::Bind(&ViscaWebcam::ReceiveLoop,
-                              weak_ptr_factory_.GetWeakPtr(), callback));
-    return;
-  }
-
-  // Success case, and a complete response has been received.
-  // Clear |data_buffer_|.
-  std::vector<char> response;
-  response.swap(data_buffer_);
-
-  if (response.size() < 2 ||
-      (static_cast<int>(response[1]) & 0xF0) == kViscaResponseError) {
-    callback.Run(false, response);
-  } else if ((static_cast<int>(response[1]) & 0xF0) != kViscaResponseAck &&
-             (static_cast<int>(response[1]) & 0xFF) !=
-                 kViscaResponseNetworkChange) {
-    callback.Run(true, response);
-  } else {
-    base::MessageLoop::current()->PostTask(
-        FROM_HERE, base::Bind(&ViscaWebcam::ReceiveLoop,
-                              weak_ptr_factory_.GetWeakPtr(), callback));
   }
 }
 
@@ -285,73 +276,58 @@ void ViscaWebcam::OnCommandCompleted(const SetPTZCompleteCallback& callback,
   // TODO(xdai): Error handling according to |response|.
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                           base::Bind(callback, success));
-  ProcessNextCommand();
+  commands_.pop_front();
+
+  // If there are pending commands, process the next one.
+  if (!commands_.empty()) {
+    const std::vector<char> next_command = commands_.front().first;
+    const CommandCompleteCallback next_callback = commands_.front().second;
+    serial_connection_->Send(
+        next_command,
+        base::Bind(&ViscaWebcam::OnSendCompleted,
+                   weak_ptr_factory_.GetWeakPtr(), next_callback));
+  }
 }
 
 void ViscaWebcam::OnInquiryCompleted(InquiryType type,
                                      const GetPTZCompleteCallback& callback,
                                      bool success,
                                      const std::vector<char>& response) {
+  int value = 0;
   if (!success) {
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                            base::Bind(callback, false, 0));
-    ProcessNextCommand();
-    return;
-  }
-
-  bool is_valid_response = false;
-  switch (type) {
-    case INQUIRY_PAN:
-      is_valid_response = CanBuildResponseInt(response, 2);
-      break;
-    case INQUIRY_TILT:
-      is_valid_response = CanBuildResponseInt(response, 6);
-      break;
-    case INQUIRY_ZOOM:
-      is_valid_response = CanBuildResponseInt(response, 2);
-      break;
-  }
-  if (!is_valid_response) {
+                            base::Bind(callback, false, value));
+  } else {
+    switch (type) {
+      case INQUIRY_PAN:
+        // See kGetPanTiltCommand for the format of response.
+        pan_ = BuildResponseInt(response, 2);
+        value = GetPositiveValue(pan_);
+        break;
+      case INQUIRY_TILT:
+        // See kGetPanTiltCommand for the format of response.
+        tilt_ = BuildResponseInt(response, 6);
+        value = GetPositiveValue(tilt_);
+        break;
+      case INQUIRY_ZOOM:
+        // See kGetZoomCommand for the format of response.
+        value = BuildResponseInt(response, 2);
+        break;
+    }
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                            base::Bind(callback, false, 0));
-    ProcessNextCommand();
-    return;
-  }
-
-  int value = 0;
-  switch (type) {
-    case INQUIRY_PAN:
-      // See kGetPanTiltCommand for the format of response.
-      pan_ = BuildResponseInt(response, 2);
-      value = GetPositiveValue(pan_);
-      break;
-    case INQUIRY_TILT:
-      // See kGetPanTiltCommand for the format of response.
-      tilt_ = BuildResponseInt(response, 6);
-      value = GetPositiveValue(tilt_);
-      break;
-    case INQUIRY_ZOOM:
-      // See kGetZoomCommand for the format of response.
-      value = BuildResponseInt(response, 2);
-      break;
+                            base::Bind(callback, true, value));
   }
-  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                          base::Bind(callback, true, value));
-  ProcessNextCommand();
-}
-
-void ViscaWebcam::ProcessNextCommand() {
   commands_.pop_front();
 
-  if (commands_.empty())
-    return;
-
   // If there are pending commands, process the next one.
-  const std::vector<char> next_command = commands_.front().first;
-  const CommandCompleteCallback next_callback = commands_.front().second;
-  serial_connection_->Send(
-      next_command, base::Bind(&ViscaWebcam::OnSendCompleted,
-                               weak_ptr_factory_.GetWeakPtr(), next_callback));
+  if (!commands_.empty()) {
+    const std::vector<char> next_command = commands_.front().first;
+    const CommandCompleteCallback next_callback = commands_.front().second;
+    serial_connection_->Send(
+        next_command,
+        base::Bind(&ViscaWebcam::OnSendCompleted,
+                   weak_ptr_factory_.GetWeakPtr(), next_callback));
+  }
 }
 
 void ViscaWebcam::PostOpenFailureTask(
@@ -411,7 +387,7 @@ void ViscaWebcam::SetTilt(int value,
 }
 
 void ViscaWebcam::SetZoom(int value, const SetPTZCompleteCallback& callback) {
-  int actual_value = std::max(value, 0);
+  int actual_value = std::min(value, 0);
   std::vector<char> command = CHAR_VECTOR_FROM_ARRAY(kSetZoomCommand);
   ResponseToCommand(&command, 4, actual_value);
   Send(command, base::Bind(&ViscaWebcam::OnCommandCompleted,
@@ -483,13 +459,4 @@ void ViscaWebcam::Reset(bool pan,
   }
 }
 
-void ViscaWebcam::OpenForTesting(
-    scoped_ptr<SerialConnection> serial_connection) {
-  serial_connection_ = serial_connection.Pass();
-}
-
-SerialConnection* ViscaWebcam::GetSerialConnectionForTesting() {
-  return serial_connection_.get();
-}
-
 }  // namespace extensions
diff --git a/extensions/browser/api/webcam_private/visca_webcam.h b/extensions/browser/api/webcam_private/visca_webcam.h
index 7277788..7b56196 100644
--- a/extensions/browser/api/webcam_private/visca_webcam.h
+++ b/extensions/browser/api/webcam_private/visca_webcam.h
@@ -17,20 +17,18 @@ namespace extensions {
 
 class ViscaWebcam : public Webcam {
  public:
-  ViscaWebcam();
+  ViscaWebcam(const std::string& path, const std::string& extension_id);
 
   using OpenCompleteCallback = base::Callback<void(bool)>;
 
   // Open and initialize the web camera. This is done by the following three
-  // steps (in order): 1. Open the serial port; 2. Request address; 3. Clear the
-  // command buffer. After these three steps completes, |open_callback| will be
-  // called.
-  void Open(const std::string& path,
-            const std::string& extension_id,
-            const OpenCompleteCallback& open_callback);
+  // steps (in order): 1. Open the serial port;  2. Request address; 3. Clear
+  // the command buffer. After these three steps completes, |open_callback| will
+  // be called.
+  void Open(const OpenCompleteCallback& open_callback);
 
  private:
-  friend class ViscaWebcamTest;
+  ~ViscaWebcam() override;
 
   enum InquiryType {
     INQUIRY_PAN,
@@ -41,23 +39,15 @@ class ViscaWebcam : public Webcam {
   using CommandCompleteCallback =
       base::Callback<void(bool, const std::vector<char>&)>;
 
-  // Private because WebCam is base::RefCounted.
-  ~ViscaWebcam() override;
-
-  void OpenOnIOThread(const std::string& path,
-                      const std::string& extension_id,
-                      const OpenCompleteCallback& open_callback);
-
+  void OpenOnIOThread(const OpenCompleteCallback& open_callback);
   // Callback function that will be called after the serial connection has been
   // opened successfully.
   void OnConnected(const OpenCompleteCallback& open_callback, bool success);
-
   // Callback function that will be called after the address has been set
   // successfully.
   void OnAddressSetCompleted(const OpenCompleteCallback& open_callback,
                              bool success,
                              const std::vector<char>& response);
-
   // Callback function that will be called after the command buffer has been
   // cleared successfully.
   void OnClearAllCompleted(const OpenCompleteCallback& open_callback,
@@ -87,7 +77,6 @@ class ViscaWebcam : public Webcam {
                           bool success,
                           const std::vector<char>& response);
 
-  void ProcessNextCommand();
   void PostOpenFailureTask(const OpenCompleteCallback& open_callback);
 
   // Webcam Overrides:
@@ -112,18 +101,13 @@ class ViscaWebcam : public Webcam {
              bool zoom,
              const SetPTZCompleteCallback& callback) override;
 
-  // Used only in unit tests in place of Open().
-  void OpenForTesting(scoped_ptr<SerialConnection> serial_connection);
-
-  // Used only in unit tests to retrieve |serial_connection_| since this class
-  // owns it.
-  SerialConnection* GetSerialConnectionForTesting();
+  const std::string path_;
+  const std::string extension_id_;
 
   scoped_ptr<SerialConnection> serial_connection_;
 
   // Stores the response for the current command.
   std::vector<char> data_buffer_;
-
   // Queues commands till the current command completes.
   std::deque<std::pair<std::vector<char>, CommandCompleteCallback>> commands_;
 
diff --git a/extensions/browser/api/webcam_private/visca_webcam_unittest.cc b/extensions/browser/api/webcam_private/visca_webcam_unittest.cc
deleted file mode 100644
index fa97f0a..0000000
--- a/extensions/browser/api/webcam_private/visca_webcam_unittest.cc
+++ /dev/null
@@ -1,140 +0,0 @@
-// 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 <vector>
-
-#include "content/public/test/test_browser_thread_bundle.h"
-#include "extensions/browser/api/webcam_private/visca_webcam.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace extensions {
-
-namespace {
-
-class TestSerialConnection : public SerialConnection {
- public:
-  TestSerialConnection() : SerialConnection("dummy_path", "dummy_id") {}
-  ~TestSerialConnection() override {}
-
-  void SetReceiveBuffer(const std::vector<char>& receive_buffer) {
-    receive_buffer_ = receive_buffer;
-  }
-
-  void CheckSendBufferAndClear(const std::vector<char>& expectations) {
-    EXPECT_EQ(send_buffer_, expectations);
-    send_buffer_.clear();
-  }
-
- private:
-  // SerialConnection:
-  void Open(const api::serial::ConnectionOptions& options,
-            const OpenCompleteCallback& callback) override {
-    NOTREACHED();
-  }
-
-  bool Receive(const ReceiveCompleteCallback& callback) override {
-    callback.Run(receive_buffer_, api::serial::RECEIVE_ERROR_NONE);
-    receive_buffer_.clear();
-    return true;
-  }
-
-  bool Send(const std::vector<char>& data,
-            const SendCompleteCallback& callback) override {
-    send_buffer_.insert(send_buffer_.end(), data.begin(), data.end());
-    callback.Run(data.size(), api::serial::SEND_ERROR_NONE);
-    return true;
-  }
-
-  std::vector<char> receive_buffer_;
-  std::vector<char> send_buffer_;
-
-  DISALLOW_COPY_AND_ASSIGN(TestSerialConnection);
-};
-
-class GetPTZExpectations {
- public:
-  GetPTZExpectations(bool expected_success, int expected_value)
-      : expected_success_(expected_success), expected_value_(expected_value) {}
-
-  void OnCallback(bool success, int value) {
-    EXPECT_EQ(expected_success_, success);
-    EXPECT_EQ(expected_value_, value);
-  }
-
- private:
-  const bool expected_success_;
-  const int expected_value_;
-
-  DISALLOW_COPY_AND_ASSIGN(GetPTZExpectations);
-};
-
-class SetPTZExpectations {
- public:
-  explicit SetPTZExpectations(bool expected_success)
-      : expected_success_(expected_success) {}
-
-  void OnCallback(bool success) { EXPECT_EQ(expected_success_, success); }
-
- private:
-  const bool expected_success_;
-
-  DISALLOW_COPY_AND_ASSIGN(SetPTZExpectations);
-};
-
-#define CHAR_VECTOR_FROM_ARRAY(array) \
-  std::vector<char>(array, array + arraysize(array))
-
-}  // namespace
-
-class ViscaWebcamTest : public testing::Test {
- protected:
-  ViscaWebcamTest() {
-    webcam_ = new ViscaWebcam;
-    webcam_->OpenForTesting(make_scoped_ptr(new TestSerialConnection));
-  }
-  ~ViscaWebcamTest() override {}
-
-  Webcam* webcam() { return webcam_.get(); }
-
-  TestSerialConnection* serial_connection() {
-    return static_cast<TestSerialConnection*>(
-        webcam_->GetSerialConnectionForTesting());
-  }
-
- private:
-  content::TestBrowserThreadBundle thread_bundle_;
-  scoped_refptr<ViscaWebcam> webcam_;
-};
-
-TEST_F(ViscaWebcamTest, Zoom) {
-  // Check getting the zoom.
-  const char kGetZoomCommand[] = {0x81, 0x09, 0x04, 0x47, 0xFF};
-  const char kGetZoomResponse[] = {0x00, 0x50, 0x01, 0x02, 0x03, 0x04, 0xFF};
-  serial_connection()->SetReceiveBuffer(
-      CHAR_VECTOR_FROM_ARRAY(kGetZoomResponse));
-  Webcam::GetPTZCompleteCallback receive_callback =
-      base::Bind(&GetPTZExpectations::OnCallback,
-                 base::Owned(new GetPTZExpectations(true, 0x1234)));
-  webcam()->GetZoom(receive_callback);
-  serial_connection()->CheckSendBufferAndClear(
-      CHAR_VECTOR_FROM_ARRAY(kGetZoomCommand));
-
-  // Check setting the zoom.
-  const char kSetZoomCommand[] = {0x81, 0x01, 0x04, 0x47, 0x06,
-                                  0x02, 0x05, 0x03, 0xFF};
-  // Note: this is a valid, but empty value because nothing is checking it.
-  const char kSetZoomResponse[] = {0x00, 0x50, 0x00, 0x00, 0x00, 0x00, 0xFF};
-  serial_connection()->SetReceiveBuffer(
-      CHAR_VECTOR_FROM_ARRAY(kSetZoomResponse));
-  Webcam::SetPTZCompleteCallback send_callback =
-      base::Bind(&SetPTZExpectations::OnCallback,
-                 base::Owned(new SetPTZExpectations(true)));
-  serial_connection()->SetReceiveBuffer(
-      CHAR_VECTOR_FROM_ARRAY(kSetZoomResponse));
-  webcam()->SetZoom(0x6253, send_callback);
-  serial_connection()->CheckSendBufferAndClear(
-      CHAR_VECTOR_FROM_ARRAY(kSetZoomCommand));
-}
-
-}  // namespace extensions
diff --git a/extensions/browser/api/webcam_private/webcam_private_api_chromeos.cc b/extensions/browser/api/webcam_private/webcam_private_api_chromeos.cc
index 02edcfa..ca7ad41 100644
--- a/extensions/browser/api/webcam_private/webcam_private_api_chromeos.cc
+++ b/extensions/browser/api/webcam_private/webcam_private_api_chromeos.cc
@@ -76,12 +76,10 @@ bool WebcamPrivateAPI::OpenSerialWebcam(
   if (webcam_resource)
     return false;
 
-  ViscaWebcam* visca_webcam = new ViscaWebcam;
-  visca_webcam->Open(
-      device_path, extension_id,
-      base::Bind(&WebcamPrivateAPI::OnOpenSerialWebcam,
-                 weak_ptr_factory_.GetWeakPtr(), extension_id, device_path,
-                 make_scoped_refptr(visca_webcam), callback));
+  ViscaWebcam* visca_webcam(new ViscaWebcam(device_path, extension_id));
+  visca_webcam->Open(base::Bind(
+      &WebcamPrivateAPI::OnOpenSerialWebcam, weak_ptr_factory_.GetWeakPtr(),
+      extension_id, device_path, make_scoped_refptr(visca_webcam), callback));
   return true;
 }